feat: add user-friendly validation to signup editor

.eslintrc.js

@@ -52,6 +52,10 @@ module.exports = {
     "radix": ["error", "as-needed"],
     // ...I know what I'm doing.
     "no-control-regex": "off",
+    // In some cases, especially if you want to comment the logic, it's much
+    // clearer to write it like a binary tree:
+    // if { if { } else { } } else { if { } else { } }
+    "no-lonely-if": "off",
     // Not usable with formik.
     "react/jsx-props-no-spreading": "off",
     // TypeScript validates prop types, no need for this.

packages/ilmomasiina-backend/package.json

@@ -52,6 +52,7 @@
     "@fastify/type-provider-typebox": "^4.0.0",
     "@sinclair/typebox": "^0.32.14",
     "@tietokilta/ilmomasiina-models": "workspace:*",
+    "@types/validator": "^13.12.0",
     "ajv": "^8.12.0",
     "ajv-formats": "^2.1.1",
     "base32-encode": "^1.2.0",
@@ -75,7 +76,8 @@
     "pg": "^8.11.3",
     "pg-hstore": "^2.3.4",
     "sequelize": "^6.37.1",
-    "umzug": "^3.7.0"
+    "umzug": "^3.7.0",
+    "validator": "^13.12.0"
   },
   "devDependencies": {
     "@faker-js/faker": "^8.4.1",

packages/ilmomasiina-backend/src/models/signup.ts

@@ -41,7 +41,10 @@ export interface SignupCreationAttributes
     | "createdAt"
   > {}
 
-export class Signup extends Model<SignupAttributes, SignupCreationAttributes> implements SignupAttributes {
+export class Signup
+  extends Model<SignupAttributes, SignupCreationAttributes>
+  implements SignupAttributes
+{
   public id!: string;
   public firstName!: string | null;
   public lastName!: string | null;
@@ -72,6 +75,9 @@ export class Signup extends Model<SignupAttributes, SignupCreationAttributes> im
 
   public readonly createdAt!: Date;
   public readonly updatedAt!: Date;
+
+  public static readonly MAX_NAME_LENGTH = 255;
+  public static readonly MAX_EMAIL_LENGTH = 255; // TODO
 }
 
 export default function setupSignupModel(sequelize: Sequelize) {

packages/ilmomasiina-backend/src/routes/index.ts

@@ -285,6 +285,7 @@ async function setupPublicRoutes(fastifyInstance: FastifyInstance, opts: RouteOp
         response: {
           ...errorResponses,
           200: schema.signupUpdateResponse,
+          400: Type.Union([schema.signupValidationError, schema.errorResponse]),
         },
       },
       // Require valid edit token:
@@ -402,7 +403,10 @@ async function setupPublicRoutes(fastifyInstance: FastifyInstance, opts: RouteOp
   );
 }
 
-export default async function setupRoutes(instance: FastifyInstance, opts: RouteOptions): Promise<void> {
+export default async function setupRoutes(
+  instance: FastifyInstance,
+  opts: RouteOptions,
+): Promise<void> {
   addLogEventHook(instance);
 
   await instance.register(setupAdminRoutes, { ...opts, prefix: "/admin" });

packages/ilmomasiina-backend/src/routes/signups/errors.ts

@@ -1,5 +1,5 @@
 /* eslint-disable max-classes-per-file */
-import { ErrorCode } from "@tietokilta/ilmomasiina-models";
+import { ErrorCode, SignupValidationErrors } from "@tietokilta/ilmomasiina-models";
 import CustomError from "../../util/customError";
 
 export class SignupsClosed extends CustomError {
@@ -19,3 +19,12 @@ export class NoSuchSignup extends CustomError {
     super(404, ErrorCode.NO_SUCH_SIGNUP, message);
   }
 }
+
+export class SignupValidationError extends CustomError {
+  public readonly errors: SignupValidationErrors;
+
+  constructor(message: string, errors: SignupValidationErrors) {
+    super(400, ErrorCode.SIGNUP_VALIDATION_ERROR, message);
+    this.errors = errors;
+  }
+}

packages/ilmomasiina-backend/src/routes/signups/updateSignup.ts

@@ -1,17 +1,22 @@
 import { FastifyReply, FastifyRequest } from "fastify";
-import { BadRequest } from "http-errors";
 import { Transaction } from "sequelize";
-
-import type { SignupPathParams, SignupUpdateBody, SignupUpdateResponse } from "@tietokilta/ilmomasiina-models";
-import { AuditEvent } from "@tietokilta/ilmomasiina-models";
+import { isEmail } from "validator";
+
+import type {
+  SignupPathParams,
+  SignupUpdateBody,
+  SignupUpdateResponse,
+  SignupValidationErrors,
+} from "@tietokilta/ilmomasiina-models";
+import { AuditEvent, SignupFieldError } from "@tietokilta/ilmomasiina-models";
 import sendSignupConfirmationMail from "../../mail/signupConfirmation";
 import { getSequelize } from "../../models";
 import { Answer } from "../../models/answer";
 import { Event } from "../../models/event";
 import { Question } from "../../models/question";
 import { Signup } from "../../models/signup";
 import { signupsAllowed } from "./createNewSignup";
-import { NoSuchSignup, SignupsClosed } from "./errors";
+import { NoSuchSignup, SignupsClosed, SignupValidationError } from "./errors";
 
 /** Requires editTokenVerification */
 export default async function updateSignup(
@@ -51,19 +56,35 @@ export default async function updateSignup(
     const notConfirmedYet = !signup.confirmedAt;
     const questions = event.questions!;
 
+    const errors: SignupValidationErrors = {};
+
     // Check that required common fields are present (if first time confirming)
     let nameFields = {};
     if (notConfirmedYet && event.nameQuestion) {
       const { firstName, lastName } = request.body;
-      if (!firstName) throw new BadRequest("Missing first name");
-      if (!lastName) throw new BadRequest("Missing last name");
+      if (!firstName) {
+        errors.firstName = SignupFieldError.MISSING;
+      } else if (firstName.length > Signup.MAX_NAME_LENGTH) {
+        errors.firstName = SignupFieldError.TOO_LONG;
+      }
+      if (!lastName) {
+        errors.lastName = SignupFieldError.MISSING;
+      } else if (lastName.length > Signup.MAX_NAME_LENGTH) {
+        errors.lastName = SignupFieldError.TOO_LONG;
+      }
       nameFields = { firstName, lastName };
     }
 
     let emailField = {};
     if (notConfirmedYet && event.emailQuestion) {
       const { email } = request.body;
-      if (!email) throw new BadRequest("Missing email");
+      if (!email) {
+        errors.email = SignupFieldError.MISSING;
+      } else if (email.length > Signup.MAX_EMAIL_LENGTH) {
+        errors.email = SignupFieldError.TOO_LONG;
+      } else if (!isEmail(email)) {
+        errors.email = SignupFieldError.INVALID_EMAIL;
+      }
       emailField = { email };
     }
 
@@ -77,59 +98,71 @@ export default async function updateSignup(
     const newAnswers = questions.map((question) => {
       // Fetch the answer to this question from the request body
       let answer = request.body.answers?.find((a) => a.questionId === question.id)?.answer;
+      let error: SignupFieldError | undefined;
 
       if (!answer || !answer.length) {
         // Disallow empty answers to required questions
         if (question.required) {
-          throw new BadRequest(`Missing answer for question ${question.question}`);
+          error = SignupFieldError.MISSING;
         }
         // Normalize empty answers to "" or [], depending on question type
         answer = question.type === "checkbox" ? [] : "";
       } else if (question.type === "checkbox") {
         // Ensure checkbox answers are arrays
         if (!Array.isArray(answer)) {
-          throw new BadRequest(`Invalid answer to question ${question.question}`);
+          error = SignupFieldError.WRONG_TYPE;
+        } else {
+          // Check that all checkbox answers are valid
+          answer.forEach((option) => {
+            if (!question.options!.includes(option)) {
+              error = SignupFieldError.NOT_AN_OPTION;
+            }
+          });
         }
-        // Check that all checkbox answers are valid
-        answer.forEach((option) => {
-          if (!question.options!.includes(option)) {
-            throw new BadRequest(`Invalid answer to question ${question.question}`);
-          }
-        });
       } else {
         // Don't allow arrays for non-checkbox questions
         if (typeof answer !== "string") {
-          throw new BadRequest(`Invalid answer to question ${question.question}`);
-        }
-        switch (question.type) {
-          case "text":
-          case "textarea":
-            break;
-          case "number":
-            // Check that a numeric answer is valid
-            if (!Number.isFinite(parseFloat(answer))) {
-              throw new BadRequest(`Invalid answer to question ${question.question}`);
-            }
-            break;
-          case "select": {
-            // Check that the select answer is valid
-            if (!question.options!.includes(answer)) {
-              throw new BadRequest(`Invalid answer to question ${question.question}`);
+          error = SignupFieldError.WRONG_TYPE;
+        } else {
+          switch (question.type) {
+            case "text":
+            case "textarea":
+              break;
+            case "number":
+              // Check that a numeric answer is valid
+              if (!Number.isFinite(parseFloat(answer))) {
+                error = SignupFieldError.NOT_A_NUMBER;
+              }
+              break;
+            case "select": {
+              // Check that the select answer is valid
+              if (!question.options!.includes(answer)) {
+                error = SignupFieldError.NOT_AN_OPTION;
+              }
+              break;
             }
-            break;
+            default:
+              throw new Error("Invalid question type");
           }
-          default:
-            throw new Error("Invalid question type");
         }
       }
 
+      if (error) {
+        errors.answers ??= {};
+        errors.answers[question.id] = error;
+      }
+
       return {
         questionId: question.id,
         answer,
         signupId: signup.id,
       };
     });
 
+    if (Object.keys(errors).length > 0) {
+      throw new SignupValidationError("Errors validating signup", errors);
+    }
+
     // Update fields for the signup (name and email only editable on first confirmation)
     const updatedFields = {
       ...nameFields,

packages/ilmomasiina-components/src/components/BaseFieldRow/index.tsx

@@ -38,7 +38,11 @@ export default function BaseFieldRow({
       </Form.Label>
       <Col sm="9">
         {children}
-        {error && <Form.Control.Feedback type="invalid">{error}</Form.Control.Feedback>}
+        {error && (
+          // Use text-danger instead of invalid-feedback here. invalid-feedback is hidden automatically when the
+          // previous element isn't .is-invalid, but that doesn't work with checkbox arrays.
+          <Form.Text className="text-danger">{error}</Form.Text>
+        )}
         {extraFeedback}
         {help && <Form.Text muted>{help}</Form.Text>}
       </Col>

packages/ilmomasiina-components/src/components/FieldRow/index.tsx

@@ -36,15 +36,16 @@ export default function FieldRow<P = unknown>({
   as: Component = Form.Control,
   children,
   type,
-  id = name,
-  controlId = id,
+  id,
+  controlId = id ?? name,
   config,
   ...props
 }: Props & P) {
   const {
     input,
-    meta: { error, invalid },
+    meta: { error: validationError, submitError, invalid },
   } = useField(name, { type, ...config });
+  const error = submitError || validationError;
 
   let field: ReactNode;
   if (children) {
@@ -54,7 +55,16 @@ export default function FieldRow<P = unknown>({
     // and calls it "label", but we still want to call the other one "label" for all other types of field. Therefore
     // we pass "checkLabel" to the field here.
     const overrideProps = checkLabel !== undefined ? { label: checkLabel } : {};
-    field = <Component required={required} {...props} id={id} {...input} {...overrideProps} />;
+    field = (
+      <Component
+        required={required}
+        isInvalid={invalid}
+        {...props}
+        id={id}
+        {...input}
+        {...overrideProps}
+      />
+    );
   }
 
   return (

packages/ilmomasiina-components/src/locales/en.json

@@ -47,6 +47,12 @@
     "editSignup.signupError.NoSuchSignup.description": "Your signup was not found. It might be already deleted.",
     "editSignup.signupError.400.description": "Signup failed. Make sure you have filled all mandatory fields and try again.",
     "editSignup.signupError.default.description": "Signup failed.",
+    "editSignup.fieldError.missing": "This field is mandatory.",
+    "editSignup.fieldError.wrongType": "The answer to this field is of the wrong type. Try refreshing the page.",
+    "editSignup.fieldError.tooLong": "The answer to this field is too long.",
+    "editSignup.fieldError.invalidEmail": "The email address is invalid.",
+    "editSignup.fieldError.notANumber": "The answer to this question must be a valid number.",
+    "editSignup.fieldError.notAnOption": "The answer to this question isn't in the allowed options. Try refreshing the page.",
     "editSignup.title.edit": "Edit signup",
     "editSignup.title.signup": "Sign up",
     "errors.returnToEvents": "Return to event list",

packages/ilmomasiina-components/src/locales/fi.json

@@ -47,6 +47,12 @@
     "editSignup.signupError.NoSuchSignup.description": "Ilmoittautumistasi ei löytynyt. Se saattaa olla jo poistettu.",
     "editSignup.signupError.400.description": "Ilmoittautuminen epäonnistui. Tarkista, että kaikki pakolliset kentät on täytetty ja yritä uudestaan.",
     "editSignup.signupError.default.description": "Ilmoittautuminen epäonnistui.",
+    "editSignup.fieldError.missing": "Tämä kenttä on pakollinen.",
+    "editSignup.fieldError.wrongType": "Kentän vastaus on väärää tyyppiä. Kokeile päivittää sivu.",
+    "editSignup.fieldError.tooLong": "Kentän vastaus on liian pitkä.",
+    "editSignup.fieldError.invalidEmail": "Sähköpostiosoite on virheellinen.",
+    "editSignup.fieldError.notANumber": "Kentän vastauksen tulee olla numero.",
+    "editSignup.fieldError.notAnOption": "Kentän vastaus ei ole sallituissa vaihtoehdoissa. Kokeile päivittää sivu.",
     "editSignup.title.edit": "Muokkaa ilmoittautumista",
     "editSignup.title.signup": "Ilmoittaudu",
     "errors.returnToEvents": "Palaa tapahtumalistaukseen",

packages/ilmomasiina-components/src/routes/EditSignup/components/CommonFields.tsx

@@ -1,13 +1,16 @@
 import React from "react";
 
 import { Form } from "react-bootstrap";
+import { useFormState } from "react-final-form";
 import { useTranslation } from "react-i18next";
 
 import FieldRow from "../../../components/FieldRow";
 import { useEditSignupContext } from "../../../modules/editSignup";
+import fieldError from "./fieldError";
 
 const CommonFields = () => {
   const { event, signup, registrationClosed } = useEditSignupContext();
+  const { submitErrors } = useFormState({ subscription: { submitErrors: true } });
   const isNew = !signup!.confirmed;
   const { t } = useTranslation();
   return (
@@ -21,6 +24,7 @@ const CommonFields = () => {
             placeholder={t("editSignup.fields.firstName.placeholder")}
             required
             readOnly={!isNew || registrationClosed}
+            alternateError={fieldError(t, submitErrors?.firstName)}
           />
           <FieldRow
             name="lastName"
@@ -29,6 +33,7 @@ const CommonFields = () => {
             placeholder={t("editSignup.fields.lastName.placeholder")}
             required
             readOnly={!isNew || registrationClosed}
+            alternateError={fieldError(t, submitErrors?.lastName)}
           />
           <FieldRow
             name="namePublic"
@@ -48,6 +53,7 @@ const CommonFields = () => {
           placeholder={t("editSignup.fields.email.placeholder")}
           required
           readOnly={!isNew || registrationClosed}
+          alternateError={fieldError(t, submitErrors?.email)}
         />
       )}
     </>