From 89b4fcc7d3e1dabc04945a35698944c9309bfa96 Mon Sep 17 00:00:00 2001 From: Jo Humphrey <31373245+jamdelion@users.noreply.github.com> Date: Tue, 12 Nov 2024 10:14:33 +0000 Subject: [PATCH 1/6] Initial tests and notes --- .../Feedback/Editor/Editor.test.tsx | 4 + .../Feedback/Public/Public.test.tsx | 84 ++++++++++++++++--- .../components/Feedback/Public/Public.tsx | 1 + 3 files changed, 79 insertions(+), 10 deletions(-) diff --git a/editor.planx.uk/src/@planx/components/Feedback/Editor/Editor.test.tsx b/editor.planx.uk/src/@planx/components/Feedback/Editor/Editor.test.tsx index e005ed1193..385185793b 100644 --- a/editor.planx.uk/src/@planx/components/Feedback/Editor/Editor.test.tsx +++ b/editor.planx.uk/src/@planx/components/Feedback/Editor/Editor.test.tsx @@ -16,3 +16,7 @@ describe("When the Feedback editor modal is rendered", () => { expect(screen.getByText("Feedback")).toBeInTheDocument(); }); }); + +// if none of the fields are edited, it should have all the defaults + +// if one of the fields is edited, it should show the edited version diff --git a/editor.planx.uk/src/@planx/components/Feedback/Public/Public.test.tsx b/editor.planx.uk/src/@planx/components/Feedback/Public/Public.test.tsx index 4f50a17f86..2d07280fc2 100644 --- a/editor.planx.uk/src/@planx/components/Feedback/Public/Public.test.tsx +++ b/editor.planx.uk/src/@planx/components/Feedback/Public/Public.test.tsx @@ -10,36 +10,100 @@ import { axe } from "vitest-axe"; import FeedbackComponent from "./Public"; +describe("when the Feedback component is rendered", async () => { + it("should not have any accessibility violations", async () => { + const { container } = setup(); + const results = await axe(container); + expect(results).toHaveNoViolations(); + }); +}); + const handleSubmit = vi.fn(); vi.mock("lib/feedback", () => ({ getInternalFeedbackMetadata: vi.fn(), insertFeedbackMutation: vi.fn(), })); -describe("when the Feedback component is rendered", async () => { - it("should not have any accessibility violations", async () => { - const { container } = setup( +describe("when the user only submits a rating on a feedback component where feedback is not required", async () => { + beforeEach(async () => { + const { user } = setup( , ); - const results = await axe(container); - expect(results).toHaveNoViolations(); + await user.click(screen.getByTestId("feedback-button-terrible")); + await user.click(screen.getByTestId("continue-button")); }); - it("should call handleSubmit when the continue button is pressed", async () => { + + it("should call the handleSubmit function with the correct data", async () => { + expect(getInternalFeedbackMetadata).toBeCalled(); + expect(insertFeedbackMutation).toBeCalledWith({ + feedbackScore: 1, + feedbackType: "component", + userComment: "", + }); + }); +}); + +describe("when the user only submits a comment on a feedback component where feedback is not required", async () => { + beforeEach(async () => { const { user } = setup( , ); + const textInput = screen.getByTestId("user-comment"); + await user.type(textInput, "I had a great time"); + await user.click(screen.getByTestId("continue-button")); + }); - await user.click(screen.getByTestId("feedback-button-terrible")); + it("should call the handleSubmit function with the correct data", async () => { + expect(getInternalFeedbackMetadata).toBeCalled(); + expect(insertFeedbackMutation).toBeCalledWith({ + feedbackScore: "", + feedbackType: "component", + userComment: "I had a great time", + }); + }); +}); + +describe("when the user does not submit any data on a feedback component where feedback is not required", async () => { + beforeEach(async () => { + const { user } = setup( + , + ); await user.click(screen.getByTestId("continue-button")); + }); + it("should call the handleSubmit function with the correct data", async () => { expect(getInternalFeedbackMetadata).toBeCalled(); - expect(insertFeedbackMutation).toBeCalled(); + expect(insertFeedbackMutation).toBeCalledWith({ + feedbackScore: "", + feedbackType: "component", + userComment: "", + }); }); }); + +describe("when feedback is required but the user does not submit any data", async () => { + beforeEach(async () => { + const { user } = setup( + , + ); + await user.click(screen.getByTestId("continue-button")); + }); + + it("displays an appropriate error message", async () => { + const errorMessage = "required"; + expect(errorMessage).toBeVisible(); + }); +}); + +// somethhing about recovering state when pressing the back button + +// should be navigable by keyboard diff --git a/editor.planx.uk/src/@planx/components/Feedback/Public/Public.tsx b/editor.planx.uk/src/@planx/components/Feedback/Public/Public.tsx index f5d64d39d7..e5db69b181 100644 --- a/editor.planx.uk/src/@planx/components/Feedback/Public/Public.tsx +++ b/editor.planx.uk/src/@planx/components/Feedback/Public/Public.tsx @@ -153,6 +153,7 @@ const FeedbackComponent = (props: PublicProps): FCReturn => { bordered onChange={formik.handleChange} aria-label="user comment" + data-testid="user-comment" /> {props.disclaimer && } From 2d5f5a56419c391fa5d2506eebc31aa71957868d Mon Sep 17 00:00:00 2001 From: Jo Humphrey <31373245+jamdelion@users.noreply.github.com> Date: Tue, 12 Nov 2024 11:57:19 +0000 Subject: [PATCH 2/6] Validate feedback required and show error messages --- .../Feedback/Editor/Editor.test.tsx | 13 ++- .../Feedback/Public/Public.test.tsx | 11 +- .../components/Feedback/Public/Public.tsx | 101 ++++++++++-------- .../src/@planx/components/Feedback/model.ts | 21 ++++ 4 files changed, 92 insertions(+), 54 deletions(-) diff --git a/editor.planx.uk/src/@planx/components/Feedback/Editor/Editor.test.tsx b/editor.planx.uk/src/@planx/components/Feedback/Editor/Editor.test.tsx index 385185793b..dfad5c904e 100644 --- a/editor.planx.uk/src/@planx/components/Feedback/Editor/Editor.test.tsx +++ b/editor.planx.uk/src/@planx/components/Feedback/Editor/Editor.test.tsx @@ -7,16 +7,19 @@ import { setup } from "testUtils"; import { FeedbackEditor } from "./Editor"; describe("When the Feedback editor modal is rendered", () => { - it("does not throw an error", () => { + beforeEach(() => { setup( , ); + }); + it("does not throw an error", () => { expect(screen.getByText("Feedback")).toBeInTheDocument(); }); + it("displays the default title if no edits are made", () => { + expect( + screen.getByPlaceholderText("Tell us what you think"), + ).toBeInTheDocument(); + }); }); - -// if none of the fields are edited, it should have all the defaults - -// if one of the fields is edited, it should show the edited version diff --git a/editor.planx.uk/src/@planx/components/Feedback/Public/Public.test.tsx b/editor.planx.uk/src/@planx/components/Feedback/Public/Public.test.tsx index 2d07280fc2..f546a12a1c 100644 --- a/editor.planx.uk/src/@planx/components/Feedback/Public/Public.test.tsx +++ b/editor.planx.uk/src/@planx/components/Feedback/Public/Public.test.tsx @@ -98,9 +98,14 @@ describe("when feedback is required but the user does not submit any data", asyn await user.click(screen.getByTestId("continue-button")); }); - it("displays an appropriate error message", async () => { - const errorMessage = "required"; - expect(errorMessage).toBeVisible(); + it("displays an appropriate error message for each missing field", async () => { + const errorMessages = [ + "Please provide a feedback score", + "Enter your feedback", + ]; + errorMessages.map((error) => { + expect(screen.getByText(error)).toBeVisible(); + }); }); }); diff --git a/editor.planx.uk/src/@planx/components/Feedback/Public/Public.tsx b/editor.planx.uk/src/@planx/components/Feedback/Public/Public.tsx index e5db69b181..975455b3c7 100644 --- a/editor.planx.uk/src/@planx/components/Feedback/Public/Public.tsx +++ b/editor.planx.uk/src/@planx/components/Feedback/Public/Public.tsx @@ -17,17 +17,20 @@ import NeutralFace from "ui/images/feedback_filled-03.svg"; import GoodFace from "ui/images/feedback_filled-04.svg"; import ExcellentFace from "ui/images/feedback_filled-05.svg"; import InputLabel from "ui/public/InputLabel"; +import ErrorWrapper from "ui/shared/ErrorWrapper"; import Input from "ui/shared/Input/Input"; import ReactMarkdownOrHtml from "ui/shared/ReactMarkdownOrHtml/ReactMarkdownOrHtml"; import { getPreviouslySubmittedData, makeData } from "../../shared/utils"; import { FaceBox } from "../components/FaceBox"; -import { Feedback, FormProps } from "../model"; +import { createFeedbackSchema, Feedback, FormProps } from "../model"; import { StyledToggleButtonGroup } from "../styled"; export const PASSPORT_FEEDBACK_KEY = "_feedback"; const FeedbackComponent = (props: PublicProps): FCReturn => { + const feedbackDataSchema = createFeedbackSchema(props.feedbackRequired); + const handleSubmitFeedback = async (values: FormProps) => { const metadata = await getInternalFeedbackMetadata(); const data = { @@ -52,6 +55,9 @@ const FeedbackComponent = (props: PublicProps): FCReturn => { userComment: "", }, onSubmit: handleSubmitFeedback, + validateOnBlur: false, + validateOnChange: false, + validationSchema: feedbackDataSchema, }); const handleFeedbackChange = ( @@ -88,52 +94,54 @@ const FeedbackComponent = (props: PublicProps): FCReturn => { } /> )} - - + - - - - - - - + + + + + + + + + {props.freeformQuestion && ( ): FCReturn => { onChange={formik.handleChange} aria-label="user comment" data-testid="user-comment" + errorMessage={formik.errors.userComment} /> {props.disclaimer && } diff --git a/editor.planx.uk/src/@planx/components/Feedback/model.ts b/editor.planx.uk/src/@planx/components/Feedback/model.ts index ee1b1cd94f..ad5247e454 100644 --- a/editor.planx.uk/src/@planx/components/Feedback/model.ts +++ b/editor.planx.uk/src/@planx/components/Feedback/model.ts @@ -1,5 +1,8 @@ +import { number, object, string } from "yup"; + import { BaseNodeData, parseBaseNodeData } from "../shared"; import { defaultContent } from "./components/defaultContent"; + export interface Feedback extends BaseNodeData { title?: string; description?: string; @@ -24,3 +27,21 @@ export const parseFeedback = ( feedbackRequired: data?.feedbackRequired || defaultContent.feedbackRequired, ...parseBaseNodeData(data), }); + +export const createFeedbackSchema = (feedbackRequired: boolean) => { + return object().shape({ + userComment: feedbackRequired + ? string().required("Enter your feedback") + : string(), + feedbackScore: feedbackRequired + ? number() + .integer() + .min(1, "Feedback score must be at least 1") + .max(5, "Feedback score cannot exceed 5") + .required("Please provide a feedback score") + : number() + .integer() + .min(1, "Feedback score must be at least 1") + .max(5, "Feedback score cannot exceed 5"), + }); +}; From 6b95259bd40dbd76542d88fca4fb174abd518f2a Mon Sep 17 00:00:00 2001 From: Jo Humphrey <31373245+jamdelion@users.noreply.github.com> Date: Tue, 12 Nov 2024 12:12:50 +0000 Subject: [PATCH 3/6] Refactor public tests to use describe.each --- .../Feedback/Public/Public.test.tsx | 114 ------------------ .../tests/Public.accessibility.test.tsx | 17 +++ .../Public/tests/Public.submit.test.tsx | 97 +++++++++++++++ 3 files changed, 114 insertions(+), 114 deletions(-) delete mode 100644 editor.planx.uk/src/@planx/components/Feedback/Public/Public.test.tsx create mode 100644 editor.planx.uk/src/@planx/components/Feedback/Public/tests/Public.accessibility.test.tsx create mode 100644 editor.planx.uk/src/@planx/components/Feedback/Public/tests/Public.submit.test.tsx diff --git a/editor.planx.uk/src/@planx/components/Feedback/Public/Public.test.tsx b/editor.planx.uk/src/@planx/components/Feedback/Public/Public.test.tsx deleted file mode 100644 index f546a12a1c..0000000000 --- a/editor.planx.uk/src/@planx/components/Feedback/Public/Public.test.tsx +++ /dev/null @@ -1,114 +0,0 @@ -import { screen } from "@testing-library/react"; -import { - getInternalFeedbackMetadata, - insertFeedbackMutation, -} from "lib/feedback"; -import React from "react"; -import { setup } from "testUtils"; -import { vi } from "vitest"; -import { axe } from "vitest-axe"; - -import FeedbackComponent from "./Public"; - -describe("when the Feedback component is rendered", async () => { - it("should not have any accessibility violations", async () => { - const { container } = setup(); - const results = await axe(container); - expect(results).toHaveNoViolations(); - }); -}); - -const handleSubmit = vi.fn(); -vi.mock("lib/feedback", () => ({ - getInternalFeedbackMetadata: vi.fn(), - insertFeedbackMutation: vi.fn(), -})); - -describe("when the user only submits a rating on a feedback component where feedback is not required", async () => { - beforeEach(async () => { - const { user } = setup( - , - ); - await user.click(screen.getByTestId("feedback-button-terrible")); - await user.click(screen.getByTestId("continue-button")); - }); - - it("should call the handleSubmit function with the correct data", async () => { - expect(getInternalFeedbackMetadata).toBeCalled(); - expect(insertFeedbackMutation).toBeCalledWith({ - feedbackScore: 1, - feedbackType: "component", - userComment: "", - }); - }); -}); - -describe("when the user only submits a comment on a feedback component where feedback is not required", async () => { - beforeEach(async () => { - const { user } = setup( - , - ); - const textInput = screen.getByTestId("user-comment"); - await user.type(textInput, "I had a great time"); - await user.click(screen.getByTestId("continue-button")); - }); - - it("should call the handleSubmit function with the correct data", async () => { - expect(getInternalFeedbackMetadata).toBeCalled(); - expect(insertFeedbackMutation).toBeCalledWith({ - feedbackScore: "", - feedbackType: "component", - userComment: "I had a great time", - }); - }); -}); - -describe("when the user does not submit any data on a feedback component where feedback is not required", async () => { - beforeEach(async () => { - const { user } = setup( - , - ); - await user.click(screen.getByTestId("continue-button")); - }); - - it("should call the handleSubmit function with the correct data", async () => { - expect(getInternalFeedbackMetadata).toBeCalled(); - expect(insertFeedbackMutation).toBeCalledWith({ - feedbackScore: "", - feedbackType: "component", - userComment: "", - }); - }); -}); - -describe("when feedback is required but the user does not submit any data", async () => { - beforeEach(async () => { - const { user } = setup( - , - ); - await user.click(screen.getByTestId("continue-button")); - }); - - it("displays an appropriate error message for each missing field", async () => { - const errorMessages = [ - "Please provide a feedback score", - "Enter your feedback", - ]; - errorMessages.map((error) => { - expect(screen.getByText(error)).toBeVisible(); - }); - }); -}); - -// somethhing about recovering state when pressing the back button - -// should be navigable by keyboard diff --git a/editor.planx.uk/src/@planx/components/Feedback/Public/tests/Public.accessibility.test.tsx b/editor.planx.uk/src/@planx/components/Feedback/Public/tests/Public.accessibility.test.tsx new file mode 100644 index 0000000000..c4070cca34 --- /dev/null +++ b/editor.planx.uk/src/@planx/components/Feedback/Public/tests/Public.accessibility.test.tsx @@ -0,0 +1,17 @@ +import React from "react"; +import { setup } from "testUtils"; +import { axe } from "vitest-axe"; + +import FeedbackComponent from "../Public"; + +describe("when the Feedback component is rendered", async () => { + it("should not have any accessibility violations", async () => { + const { container } = setup(); + const results = await axe(container); + expect(results).toHaveNoViolations(); + }); +}); + +// somethhing about recovering state when pressing the back button + +// should be navigable by keyboard diff --git a/editor.planx.uk/src/@planx/components/Feedback/Public/tests/Public.submit.test.tsx b/editor.planx.uk/src/@planx/components/Feedback/Public/tests/Public.submit.test.tsx new file mode 100644 index 0000000000..586d728b8f --- /dev/null +++ b/editor.planx.uk/src/@planx/components/Feedback/Public/tests/Public.submit.test.tsx @@ -0,0 +1,97 @@ +import { screen } from "@testing-library/react"; +import { + getInternalFeedbackMetadata, + insertFeedbackMutation, +} from "lib/feedback"; +import React from "react"; +import { setup } from "testUtils"; +import { vi } from "vitest"; + +import FeedbackComponent from "../Public"; + +const handleSubmit = vi.fn(); +vi.mock("lib/feedback", () => ({ + getInternalFeedbackMetadata: vi.fn(), + insertFeedbackMutation: vi.fn(), +})); + +describe.each([ + { + dataType: "a rating", + expectedData: { + feedbackScore: 1, + feedbackType: "component", + userComment: "", + }, + }, + { + dataType: "a comment", + expectedData: { + feedbackScore: "", + feedbackType: "component", + userComment: "I had a great time", + }, + }, + { + dataType: "nothing", + expectedData: { + feedbackScore: "", + feedbackType: "component", + userComment: "", + }, + }, +])( + "when a user submits $dataType on a feedback component where feedback is not required", + ({ dataType, expectedData }) => { + beforeEach(async () => { + const { user } = setup( + , + ); + + switch (dataType) { + case "a rating": + await user.click(screen.getByTestId("feedback-button-terrible")); + + break; + case "a comment": + await user.type( + screen.getByTestId("user-comment"), + "I had a great time", + ); + break; + case "nothing": + default: + break; + } + + await user.click(screen.getByTestId("continue-button")); + }); + + it("should call the handleSubmit function with the correct data", async () => { + expect(getInternalFeedbackMetadata).toBeCalled(); + expect(insertFeedbackMutation).toBeCalledWith(expectedData); + }); + }, +); + +describe("when feedback is required but the user does not submit any data", async () => { + beforeEach(async () => { + const { user } = setup( + , + ); + await user.click(screen.getByTestId("continue-button")); + }); + + it("displays an appropriate error message for each missing field", async () => { + const errorMessages = [ + "Please provide a feedback score", + "Enter your feedback", + ]; + errorMessages.map((error) => { + expect(screen.getByText(error)).toBeVisible(); + }); + }); +}); From 3985abd62687f6f8816a44625be81b3292b34fa2 Mon Sep 17 00:00:00 2001 From: Jo Humphrey <31373245+jamdelion@users.noreply.github.com> Date: Tue, 12 Nov 2024 12:53:17 +0000 Subject: [PATCH 4/6] Add keyboard navigation test --- .../tests/Public.accessibility.test.tsx | 45 +++++++++++++++++-- 1 file changed, 42 insertions(+), 3 deletions(-) diff --git a/editor.planx.uk/src/@planx/components/Feedback/Public/tests/Public.accessibility.test.tsx b/editor.planx.uk/src/@planx/components/Feedback/Public/tests/Public.accessibility.test.tsx index c4070cca34..1501cc1f5e 100644 --- a/editor.planx.uk/src/@planx/components/Feedback/Public/tests/Public.accessibility.test.tsx +++ b/editor.planx.uk/src/@planx/components/Feedback/Public/tests/Public.accessibility.test.tsx @@ -1,17 +1,56 @@ +import { screen } from "@testing-library/react"; +import { + getInternalFeedbackMetadata, + insertFeedbackMutation, +} from "lib/feedback"; import React from "react"; import { setup } from "testUtils"; +import { vi } from "vitest"; import { axe } from "vitest-axe"; import FeedbackComponent from "../Public"; +vi.mock("lib/feedback", () => ({ + getInternalFeedbackMetadata: vi.fn(), + insertFeedbackMutation: vi.fn(), +})); + describe("when the Feedback component is rendered", async () => { it("should not have any accessibility violations", async () => { const { container } = setup(); const results = await axe(container); expect(results).toHaveNoViolations(); }); -}); + it("should be navigable by keyboard", async () => { + const { user } = setup(); + + const ratingButtons = screen.getAllByRole("button"); + + // user tabs through all rating buttons and selects the last one + await user.tab(); + expect(ratingButtons[0]).toHaveFocus(); + await user.tab(); + expect(ratingButtons[1]).toHaveFocus(); + await user.tab(); + expect(ratingButtons[2]).toHaveFocus(); + await user.tab(); + expect(ratingButtons[3]).toHaveFocus(); + await user.tab(); + expect(ratingButtons[4]).toHaveFocus(); + await user.keyboard("[Space]"); // select 'Excellent' button -// somethhing about recovering state when pressing the back button + await user.tab(); + expect(screen.getByRole("textbox")).toHaveFocus(); -// should be navigable by keyboard + await user.tab(); + expect(screen.getByTestId("continue-button")).toHaveFocus(); + await user.keyboard("[Space]"); // submits + + expect(getInternalFeedbackMetadata).toBeCalled(); + expect(insertFeedbackMutation).toBeCalledWith({ + feedbackScore: 5, + feedbackType: "component", + userComment: "", + }); + }); +}); From cbff416b31dc171e0a49d5133e16fcf5214741b0 Mon Sep 17 00:00:00 2001 From: Jo Humphrey <31373245+jamdelion@users.noreply.github.com> Date: Tue, 12 Nov 2024 14:47:59 +0000 Subject: [PATCH 5/6] Update story with feedbackRequired --- .../@planx/components/Feedback/Public/Feedback.stories.tsx | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/editor.planx.uk/src/@planx/components/Feedback/Public/Feedback.stories.tsx b/editor.planx.uk/src/@planx/components/Feedback/Public/Feedback.stories.tsx index e8f0b3d8bb..2bc537e27f 100644 --- a/editor.planx.uk/src/@planx/components/Feedback/Public/Feedback.stories.tsx +++ b/editor.planx.uk/src/@planx/components/Feedback/Public/Feedback.stories.tsx @@ -15,3 +15,7 @@ export default meta; export const Basic = { args: defaultContent, } satisfies Story; + +export const WithFeedbackRequired = { + args: { ...defaultContent, feedbackRequired: true }, +} satisfies Story; From 805f030323e7543972eeb8f1654f4efe1164bdc2 Mon Sep 17 00:00:00 2001 From: Jo Humphrey <31373245+jamdelion@users.noreply.github.com> Date: Tue, 12 Nov 2024 15:31:26 +0000 Subject: [PATCH 6/6] Reword error message and remove min max from validation --- .../Feedback/Public/tests/Public.submit.test.tsx | 2 +- .../src/@planx/components/Feedback/model.ts | 11 ++--------- 2 files changed, 3 insertions(+), 10 deletions(-) diff --git a/editor.planx.uk/src/@planx/components/Feedback/Public/tests/Public.submit.test.tsx b/editor.planx.uk/src/@planx/components/Feedback/Public/tests/Public.submit.test.tsx index 586d728b8f..abbf2b084f 100644 --- a/editor.planx.uk/src/@planx/components/Feedback/Public/tests/Public.submit.test.tsx +++ b/editor.planx.uk/src/@planx/components/Feedback/Public/tests/Public.submit.test.tsx @@ -87,7 +87,7 @@ describe("when feedback is required but the user does not submit any data", asyn it("displays an appropriate error message for each missing field", async () => { const errorMessages = [ - "Please provide a feedback score", + "Please rate your experience", "Enter your feedback", ]; errorMessages.map((error) => { diff --git a/editor.planx.uk/src/@planx/components/Feedback/model.ts b/editor.planx.uk/src/@planx/components/Feedback/model.ts index ad5247e454..daeaa81124 100644 --- a/editor.planx.uk/src/@planx/components/Feedback/model.ts +++ b/editor.planx.uk/src/@planx/components/Feedback/model.ts @@ -34,14 +34,7 @@ export const createFeedbackSchema = (feedbackRequired: boolean) => { ? string().required("Enter your feedback") : string(), feedbackScore: feedbackRequired - ? number() - .integer() - .min(1, "Feedback score must be at least 1") - .max(5, "Feedback score cannot exceed 5") - .required("Please provide a feedback score") - : number() - .integer() - .min(1, "Feedback score must be at least 1") - .max(5, "Feedback score cannot exceed 5"), + ? number().integer().required("Please rate your experience") + : number().integer(), }); };