From 1e9ac18ba4ff083e244c4134bf7f967c1b47bbdc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dafydd=20Ll=C5=B7r=20Pearson?= Date: Wed, 3 Apr 2024 07:20:01 +0100 Subject: [PATCH 1/3] feat: Add per-file validation to schema, not whole array --- .../FileUploadAndLabel/schema.test.ts | 12 ++++-- .../components/FileUploadAndLabel/schema.ts | 38 +++++++++++++++---- 2 files changed, 39 insertions(+), 11 deletions(-) diff --git a/editor.planx.uk/src/@planx/components/FileUploadAndLabel/schema.test.ts b/editor.planx.uk/src/@planx/components/FileUploadAndLabel/schema.test.ts index ab978c4cfa..6c3cc85b0b 100644 --- a/editor.planx.uk/src/@planx/components/FileUploadAndLabel/schema.test.ts +++ b/editor.planx.uk/src/@planx/components/FileUploadAndLabel/schema.test.ts @@ -212,12 +212,18 @@ describe("fileLabelSchema", () => { }); it("rejects if any slots are untagged", async () => { - const mockSlots = [{ id: "123" }, { id: "456" }] as FileUploadSlot[]; + const mockSlots = [ + { id: "123", file: { path: "first.jpg" } }, + { id: "456", file: { path: "second.jpg" } }, + ] as FileUploadSlot[]; const mockFileList = { required: [ // Second slot is not assigned to any fileTypes - { ...mockFileTypes.AlwaysRequired, slots: [{ id: "123" }] }, + { + ...mockFileTypes.AlwaysRequired, + slots: [{ id: "123", file: { path: "abc.jpg" } }], + }, ], recommended: [], optional: [], @@ -225,7 +231,7 @@ describe("fileLabelSchema", () => { await expect(() => fileLabelSchema.validate(mockFileList, { context: { slots: mockSlots } }), - ).rejects.toThrow(/Please label all files/); + ).rejects.toThrow(/File second.jpg is not labeled/); }); it("allows fileLists where all files are tagged, but requirements are not satisfied yet", async () => { diff --git a/editor.planx.uk/src/@planx/components/FileUploadAndLabel/schema.ts b/editor.planx.uk/src/@planx/components/FileUploadAndLabel/schema.ts index 43d978bfe8..42d242f883 100644 --- a/editor.planx.uk/src/@planx/components/FileUploadAndLabel/schema.ts +++ b/editor.planx.uk/src/@planx/components/FileUploadAndLabel/schema.ts @@ -6,6 +6,7 @@ import { SchemaOf, string, TestContext, + ValidationError, } from "yup"; import { FileUploadSlot } from "../FileUpload/Public"; @@ -103,20 +104,41 @@ interface FileListSchemaTestContext extends TestContext { export const fileLabelSchema = object().test({ name: "allFilesTagged", - message: "Please label all files", test: (fileList, { options: { context } }) => { if (!context) throw new Error("Missing context for fileListSchema"); const { slots } = context as FileListSchemaTestContext; - const isEveryFileTagged = Boolean( - slots?.every( - (slot) => - getTagsForSlot(slot.id, fileList as unknown as FileList).length, - ), - ); - return isEveryFileTagged; + const errors: ValidationError[] = []; + slots?.forEach((slot) => { + const tagsForSlot = getTagsForSlot( + slot.id, + fileList as unknown as FileList, + ); + if (!tagsForSlot.length) { + errors.push( + new ValidationError( + `File ${slot.file.path} is not labeled`, + undefined, + slot.id, + ), + ); + } + }); + + // Valid fileLabelSchema + if (!errors.length) return true; + + return new ValidationError(errors, undefined, undefined, "allFilesTagged"); }, }); +/** + * Format errors generated by fileLabelSchema so that we can access them by path (file id) in the UI + */ +export const formatFileLabelSchemaErrors = ( + errors: ValidationError, +): Record => + Object.fromEntries(errors.inner.map(({ path, message }) => [path, message])); + export const fileListSchema = object({ required: array().test({ name: "allRequiredFilesUploaded", From c9afde8a3fc8553a46e0d69937e83e2e02adf2d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dafydd=20Ll=C5=B7r=20Pearson?= Date: Wed, 3 Apr 2024 07:20:35 +0100 Subject: [PATCH 2/3] test: Check for new error messages in modal and public component --- .../@planx/components/FileUploadAndLabel/Public.test.tsx | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/editor.planx.uk/src/@planx/components/FileUploadAndLabel/Public.test.tsx b/editor.planx.uk/src/@planx/components/FileUploadAndLabel/Public.test.tsx index 2d4a65557b..0f69f50d9c 100644 --- a/editor.planx.uk/src/@planx/components/FileUploadAndLabel/Public.test.tsx +++ b/editor.planx.uk/src/@planx/components/FileUploadAndLabel/Public.test.tsx @@ -557,7 +557,7 @@ describe("Error handling", () => { await user.click(submitModalButton); expect(true).toBeTruthy(); const modalError = await within(fileTaggingModal).findByText( - "Please label all files", + /File test.jpg is not labeled/, ); expect(modalError).toBeVisible(); }); @@ -594,7 +594,9 @@ describe("Error handling", () => { // User cannot submit without uploading a file await user.click(screen.getByTestId("continue-button")); expect(handleSubmit).not.toHaveBeenCalled(); - const fileListError = await screen.findByText("Please label all files"); + const fileListError = await screen.findByText( + /File test.jpg is not labeled/, + ); expect(fileListError).toBeVisible(); // Re-open modal and tag file From 92dbc76b5b0e3d627237fdca9212dbe5605ea396 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dafydd=20Ll=C5=B7r=20Pearson?= Date: Wed, 3 Apr 2024 07:21:09 +0100 Subject: [PATCH 3/3] feat: Display error messages in modal and public component --- .../components/FileUploadAndLabel/Modal.tsx | 78 ++++++++++--------- .../components/FileUploadAndLabel/Public.tsx | 63 +++++++++++---- 2 files changed, 91 insertions(+), 50 deletions(-) diff --git a/editor.planx.uk/src/@planx/components/FileUploadAndLabel/Modal.tsx b/editor.planx.uk/src/@planx/components/FileUploadAndLabel/Modal.tsx index f12796d522..9ac7b83b5a 100644 --- a/editor.planx.uk/src/@planx/components/FileUploadAndLabel/Modal.tsx +++ b/editor.planx.uk/src/@planx/components/FileUploadAndLabel/Modal.tsx @@ -19,6 +19,7 @@ import capitalize from "lodash/capitalize"; import React, { useEffect, useState } from "react"; import { usePrevious } from "react-use"; import ErrorWrapper from "ui/shared/ErrorWrapper"; +import { ValidationError } from "yup"; import { FileUploadSlot } from "../FileUpload/Public"; import { UploadedFileCard } from "../shared/PrivateFileUpload/UploadedFileCard"; @@ -28,7 +29,7 @@ import { getTagsForSlot, removeSlots, } from "./model"; -import { fileLabelSchema } from "./schema"; +import { fileLabelSchema, formatFileLabelSchemaErrors } from "./schema"; interface FileTaggingModalProps { uploadedFiles: FileUploadSlot[]; @@ -55,9 +56,9 @@ export const FileTaggingModal = ({ setShowModal, removeFile, }: FileTaggingModalProps) => { - const [error, setError] = useState(); + const [errors, setErrors] = useState | undefined>(); - const closeModal = (event: any, reason?: string) => { + const closeModal = (_event: any, reason?: string) => { if (reason && reason == "backdropClick") { return; } @@ -68,7 +69,12 @@ export const FileTaggingModal = ({ fileLabelSchema .validate(fileList, { context: { slots: uploadedFiles } }) .then(closeModal) - .catch((err) => setError(err.message)); + .catch((err) => { + if (err instanceof ValidationError) { + const formattedErrors = formatFileLabelSchemaErrors(err); + setErrors(formattedErrors); + } + }); }; return ( @@ -97,16 +103,20 @@ export const FileTaggingModal = ({ {uploadedFiles.map((slot) => ( - removeFile(slot)} - /> - + + <> + removeFile(slot)} + /> + + + ))} @@ -117,27 +127,25 @@ export const FileTaggingModal = ({ padding: 2, }} > - - - - - - + + + + ); diff --git a/editor.planx.uk/src/@planx/components/FileUploadAndLabel/Public.tsx b/editor.planx.uk/src/@planx/components/FileUploadAndLabel/Public.tsx index 9c5decf462..291978f97c 100644 --- a/editor.planx.uk/src/@planx/components/FileUploadAndLabel/Public.tsx +++ b/editor.planx.uk/src/@planx/components/FileUploadAndLabel/Public.tsx @@ -39,7 +39,12 @@ import { getTagsForSlot, removeSlots, } from "./model"; -import { fileLabelSchema, fileListSchema, slotsSchema } from "./schema"; +import { + fileLabelSchema, + fileListSchema, + formatFileLabelSchemaErrors, + slotsSchema, +} from "./schema"; type Props = PublicProps; @@ -106,6 +111,7 @@ function Component(props: Props) { if (isUserReturningToNode) return setIsUserReturningToNode(false); if (slots.length && dropzoneError) setDropzoneError(undefined); if (!slots.length && fileListError) setFileListError(undefined); + if (!slots.length && fileLabelErrors) setFileLabelErrors(undefined); if (slots.length > previousSlotCount) setShowModal(true); }, [slots.length]); @@ -114,6 +120,9 @@ function Component(props: Props) { ); const [dropzoneError, setDropzoneError] = useState(); + const [fileLabelErrors, setFileLabelErrors] = useState< + Record | undefined + >(); const [fileListError, setFileListError] = useState(); const [showModal, setShowModal] = useState(false); const [isUserReturningToNode, setIsUserReturningToNode] = @@ -129,15 +138,27 @@ function Component(props: Props) { const payload = generatePayload(fileList); props.handleSubmit?.(payload); }) - .catch((err) => - err?.type === "minFileUploaded" - ? setDropzoneError(err?.message) - : setFileListError(err?.message), - ); + .catch((err) => { + switch (err?.type) { + case "minFileUploaded": + case "nonUploading": + setDropzoneError(err?.message); + break; + case "allFilesTagged": { + const formattedErrors = formatFileLabelSchemaErrors(err); + setFileLabelErrors(formattedErrors); + break; + } + case "allRequiredFilesUploaded": + setFileListError(err?.message); + break; + } + }); }; const onUploadedFileCardChange = () => { setFileListError(undefined); + setFileLabelErrors(undefined); setShowModal(true); }; @@ -238,13 +259,15 @@ function Component(props: Props) { )} {slots.map((slot) => { return ( - removeFile(slot)} - /> + + removeFile(slot)} + /> + ); })} @@ -322,8 +345,18 @@ const InteractiveFileListItem = (props: FileListItemProps) => { {howMeasured && howMeasured !== emptyContent ? ( <> - {definitionImg && } - + {definitionImg && ( + + )} + ) : undefined}