Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mh/more info feedback component testing #2810

Merged
merged 4 commits into from
Feb 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions editor.planx.uk/src/components/Feedback/FeedbackForm.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ function FormInputs({ inputs }: { inputs: FeedbackFormInput[] }): FCReturn {
name={input.name}
value={values?.[input.name]}
onChange={handleChange}
data-testid={`${input.name}Textarea`}
/>
</InputLabel>
) : (
Expand All @@ -43,9 +44,11 @@ function FormInputs({ inputs }: { inputs: FeedbackFormInput[] }): FCReturn {
required
multiline
bordered
aria-label={"Leave your feedback"}
aria-describedby={input.ariaDescribedBy}
value={values?.[input.name]}
onChange={handleChange}
data-testid={`${input.name}Textarea`}
/>
)}
</ErrorWrapper>
Expand Down
155 changes: 155 additions & 0 deletions editor.planx.uk/src/components/Feedback/MoreInfoFeedback.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,155 @@
import "@testing-library/jest-dom/extend-expect";

import { fireEvent, waitFor } from "@testing-library/react";
// eslint-disable-next-line no-restricted-imports
import userEvent from "@testing-library/user-event";
import {
getInternalFeedbackMetadata,
insertFeedbackMutation,
} from "lib/feedback";
import React from "react";
import { axe, setup } from "testUtils";

import MoreInfoFeedbackComponent from "./MoreInfoFeedback";

jest.mock("lib/feedback", () => {
return {
getInternalFeedbackMetadata: jest.fn(),
insertFeedbackMutation: jest.fn(),
};
});

const scrollIntoView = jest.fn();
window.Element.prototype.scrollIntoView = scrollIntoView;

describe("MoreInfoFeedbackComponent presentation and functionality", () => {
beforeEach(() => {
jest.clearAllMocks();
});

// Initial load
test('Initial loads renders "Yes" and "No" buttons initially', () => {
const { getByText } = setup(<MoreInfoFeedbackComponent />);
expect(getByText("Yes")).toBeInTheDocument();
expect(getByText("No")).toBeInTheDocument();
});

test("Does not scroll into view on initial render", () => {
setup(<MoreInfoFeedbackComponent />);
expect(scrollIntoView).not.toBeCalled();
});

// Sentiment selection
test("Clicking Yes input form scrolls into view", async () => {
const { getByText } = setup(<MoreInfoFeedbackComponent />);
fireEvent.click(getByText("Yes"));
expect(scrollIntoView).toBeCalled();
await waitFor(() => {
expect(
getByText("Please help us to improve this service by sharing feedback"),
).toBeInTheDocument();
});
});

test("Clicking No input form scrolls into view", async () => {
const { getByText } = setup(<MoreInfoFeedbackComponent />);
fireEvent.click(getByText("No"));
expect(scrollIntoView).toBeCalled();
await waitFor(() => {
expect(
getByText("Please help us to improve this service by sharing feedback"),
).toBeInTheDocument();
});
});

// Form submission
test("Submitting feedback changes view to thank you message", async () => {
const { getByText, getByTestId } = setup(<MoreInfoFeedbackComponent />);

fireEvent.click(getByText("Yes"));
await waitFor(() => {
expect(getByTestId("userCommentTextarea")).toBeInTheDocument();
});

await userEvent.type(
getByTestId("userCommentTextarea"),
"Great help, thanks!",
);

fireEvent.click(getByText("Send feedback"));
await waitFor(() => {
expect(getInternalFeedbackMetadata).toBeCalled();
expect(insertFeedbackMutation).toBeCalled();
});

await waitFor(() => {
expect(getByText("Thank you for your feedback.")).toBeInTheDocument();
});
});

/*
We use the `required` property to validate that a user can't submit an empty
comment.

It doesn't seem to be possible to test that the Browser stops form
submit in the Jest environment.

Checking for `required` property currently but we could add explicit
validation.
*/
test("Feedback form requires a comment before submitting", async () => {
const { getByTestId, getByText } = setup(<MoreInfoFeedbackComponent />);

fireEvent.click(getByText("Yes"));
await waitFor(() => {
expect(getByTestId("userCommentTextarea")).toBeInTheDocument();
});

expect(getByTestId("userCommentTextarea")).toHaveAttribute("required");
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test highlights something interesting - while we can't test that the browser stops form submit in Jest, we'd normally test that an input error message becomes visible when trying to proceed without filling out required fields - but it doesn't seem like we've accounted for that with any of these components and are relying on default browser behavior?

I'd expect us to the use the same GOV.UK pattern of red left border and message that we use for all other public-facing inputs - is there a reason we aren't? I think it's very likely this will be picked up in a11y testing as inconsistent and insufficient.

Screenshot from 2024-02-23 15-43-21

Screenshot from 2024-02-23 15-36-07

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the time this feature was being built I think there were conversations about using the native required although to be fair that might have been scoped to the editor rather than the public view.

I did have a version with the gov uk style errors but they could never actually be seen by users as the required attribute stopped form submission which would be run to show these validation messages so that state could never be reached.

As this was the case at the time I thought I'd reduce complexity but not showing any code which I thought a user could never see.

It'll be really interesting to see what the a11y testing comes back with because on one hand not using required for a required field seems somewhat off? Although as you say it's inconsistent 🤔

});

describe("MoreInfoFeedbackComponent accessibility", () => {
beforeEach(() => {
jest.clearAllMocks();
});

test("Initial load should have no accessibility violations", async () => {
const { container } = setup(<MoreInfoFeedbackComponent />);
const results = await axe(container);
expect(results).toHaveNoViolations();
});

test("Form view should have no accessability violations", async () => {
const { container, getByText } = setup(<MoreInfoFeedbackComponent />);
fireEvent.click(getByText("Yes"));

const results = await axe(container);
expect(results).toHaveNoViolations();
});

test("Thank you view should have no accessibility violations", async () => {
const { container, getByText, getByTestId } = setup(
<MoreInfoFeedbackComponent />,
);

fireEvent.click(getByText("Yes"));
await waitFor(() => {
expect(getByTestId("userCommentTextarea")).toBeInTheDocument();
});

await userEvent.type(
getByTestId("userCommentTextarea"),
"Great help, thanks!",
);

fireEvent.click(getByText("Send feedback"));

await waitFor(() => {
expect(getByText("Thank you for your feedback.")).toBeInTheDocument();
});

const results = await axe(container);
expect(results).toHaveNoViolations();
});
});
7 changes: 6 additions & 1 deletion editor.planx.uk/src/components/Feedback/MoreInfoFeedback.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,12 @@ const MoreInfoFeedbackComponent: React.FC = () => {
return (
<MoreInfoFeedback>
<Container maxWidth={false}>
<Typography variant="h4" component="h3" gutterBottom>
<Typography
variant="h4"
component="h3"
gutterBottom
id="comment-title"
>
Please help us to improve this service by sharing feedback
</Typography>
<FeedbackBody>
Expand Down
Loading