feat: frontend expiring certification warnings and sign-in prevention #205

mathcolo · 2024-12-20T22:22:38Z

Asana Task: 📐 Implement Warnings for Expiring Certifications

Checklist

https://github.com/mbta/terraform_modules/pull/58
Tests:
- (x) Has tests
- ( ) Doesn't need tests
- ( ) Tests deferred (with justification)
Product/Design sign off:
- (x) Okayed the plan for the feature (e.g. the design files, or the Asana task)
- ( ) Reviewed the feature as implemented (e.g. on dev-green, or saw screenshots)
- ( ) No review needed

skyqrose

There was some talk about whether we should warn on someone not having any certifications in the database. I don't remember the resolution of that discussion. Should anything like that be incorporated into this feature?

skyqrose · 2025-01-03T18:26:18Z

js/hooks/useCertifications.ts

+  return list.map(certificationFromData);
+};
+
+export const useCertifications = (badge: string | null) => {


nitpick: Can you add a return type to this? Sometimes its obvious, but in this case it isn't (you have to go through the type of useApiResult and parse to figure it out), and this is a pretty important abstraction boundary so should be clearly typed.

skyqrose · 2025-01-03T18:30:06Z

js/models/certification.ts

+
+export type Certification = {
+  type: string;
+  rail_line: string;


nitpick: Should this be railLine? The json data below definitely shouldn't be, but this one is the type we'd use within the react app.

skyqrose · 2025-01-03T18:56:24Z

js/components/operatorSignIn/attestation.tsx

+          </form>
+        </>
+      : <>
+          <Instructions displayName={name} />


naming: Instructions is a bit ambiguous, when I first saw this I thought it meant instructions for filling out the form normally, but this is specifically instructions for dealing with an expired cert.

skyqrose · 2025-01-03T19:02:17Z

js/test/components/operatorSignIn/attestation.test.tsx

+    test("displays expired mode if expired", () => {
+      const view = render(
+        <Attestation
+          badge="123"
+          prefill={false}
+          onComplete={jest.fn()}
+          loading={false}
+          employees={EMPLOYEES}
+          certifications={CERTIFICATIONS_ONE_EXPIRED}
+        />,
+      );
+      expect(view.getByText("Expired card")).toBeInTheDocument();
+      expect(
+        view.getByText("Continue to Fit for Duty Check."),
+      ).toBeInTheDocument();
+    });
+
+    test("can bypass expired mode", async () => {
+      const view = render(
+        <Attestation
+          badge="123"
+          prefill={false}
+          onComplete={jest.fn()}
+          loading={false}
+          employees={EMPLOYEES}
+          certifications={CERTIFICATIONS_ONE_EXPIRED}
+        />,
+      );
+      await userEvent.click(
+        view.getByRole("button", { name: "Continue to Fit for Duty Check →" }),
+      );
+      expect(
+        view.getByRole("button", { name: "Complete Fit for Duty Check" }),
+      ).toBeInTheDocument();
+    });
+  });


these tests are redundant. You don't need to assert the existence of the button and then assert that you can click it, just one test that asserts clicking the button would work.

I suggest deleting the first test, and copying the one unique assertion from that test (getByText("ExpiredCard")) into the second.

Suggested change

test("displays expired mode if expired", () => {

const view = render(

<Attestation

badge="123"

prefill={false}

onComplete={jest.fn()}

loading={false}

employees={EMPLOYEES}

certifications={CERTIFICATIONS_ONE_EXPIRED}

/>,

);

expect(view.getByText("Expired card")).toBeInTheDocument();

expect(

view.getByText("Continue to Fit for Duty Check."),

).toBeInTheDocument();

});

test("can bypass expired mode", async () => {

const view = render(

<Attestation

badge="123"

prefill={false}

onComplete={jest.fn()}

loading={false}

employees={EMPLOYEES}

certifications={CERTIFICATIONS_ONE_EXPIRED}

/>,

);

await userEvent.click(

view.getByRole("button", { name: "Continue to Fit for Duty Check →" }),

);

expect(

view.getByRole("button", { name: "Complete Fit for Duty Check" }),

).toBeInTheDocument();

});

});

test("can bypass expired mode", async () => {

const view = render(

<Attestation

badge="123"

prefill={false}

onComplete={jest.fn()}

loading={false}

employees={EMPLOYEES}

certifications={CERTIFICATIONS_ONE_EXPIRED}

/>,

);

expect(view.getByText("Expired card")).toBeInTheDocument();

await userEvent.click(

view.getByRole("button", { name: "Continue to Fit for Duty Check →" }),

);

expect(

view.getByRole("button", { name: "Complete Fit for Duty Check" }),

).toBeInTheDocument();

});

});

skyqrose · 2025-01-03T19:05:44Z

js/test/components/operatorSignIn/attestation.test.tsx

@@ -216,4 +227,42 @@ describe("Attestation", () => {
    expect(onComplete).not.toHaveBeenCalled();
    jest.useRealTimers();
  });
+
+  describe("expiry", () => {


attestation.test.tsx now has a test for the expired case. But does it need a test that it shows the about to expire warning? I see that the details of that situation are asserted in expiry.test.tsx, but I'm thinking about a test that proves that CertificationBoxes is called.

skyqrose · 2025-01-03T19:17:08Z

js/components/operatorSignIn/expiry.tsx

+  return (
+    <WarningParagraph
+      className={className([
+        "border-0 light:bg-opacity-40 dark:bg-opacity-30 dark:text-white",


refactoring suggestion: This is the only use of WarningParagraph, and these styles don't need to be controlled dynamically, so should they be part of WarningParagraph instead of passed in as an argument?

And should the background color be controlled by passing in "warning" or "error" instead of by giving an extraClassName?

Or, alternatively, WarningParagraph is just 2 divs and some styles, and it's not used anywhere else, so maybe it should be inlined here instead of abstracted?

skyqrose · 2025-01-03T19:22:52Z

js/components/operatorSignIn/expiry.tsx

+        <CertificateBox
+          now={now}
+          mode="error"
+          title={expired.length === 2 ? "Expired cards" : "Expired card"}


nitpick: This is fragile if anybody ever has more than 2 certifications (could happen if we track more than two types, or if someone has certs on more than one rail line). It should be expired.length === 1 ? singular : plural or expired.length > 1 ? plural : singular.

Also applies to the previous call 10 lines above this.

skyqrose · 2025-01-03T19:24:57Z

js/components/operatorSignIn/expiry.tsx

+        <ol className="mb-4 ml-10 mr-0 mt-2 list-decimal">
+          <li className="m-0">
+            Take a picture of{" "}
+            {expireds.length === 2 ? "both cards" : "the card"}.


What if length > 2?

skyqrose · 2025-01-03T19:31:35Z

js/components/operatorSignIn/expiry.tsx

+  now: DateTime;
+  onContinue: () => void;
+}): ReactElement => {
+  const expireds = certifications.filter((c) => isExpired(c, now));


I don't think this would actually cause problems, but it seems like we shouldn't have to recalculate which cards are expired at this point. The calculations are all in sync now but what if something changes? Perhaps the caller should calculate expireds once, and use the same value both for deciding whether to show Bypass and for passing in as an argument:

expireds = certifications.filter((c) => isExpired(c, now)); ... (expireds.length > 0) ? <Bypass expireds={expireds} displayName={...} onContinue={...} />

skyqrose · 2025-01-03T19:35:18Z

js/test/components/operatorSignIn/operatorSignInModal.test.tsx

@@ -161,7 +176,6 @@ describe("OperatorSignInModal", () => {
    await userEvent.type(view.getByRole("textbox"), "123");
    await userEvent.click(view.getByRole("button", { name: "OK" }));

-    expect(view.getByText("Step 2 of 2")).toBeInTheDocument();


Why are these assertions removed?

feat: frontend expiring certification warnings and sign-in prevention

614450e

mathcolo force-pushed the pim-load-certifications-frontend branch from f2d8c5e to 614450e Compare January 2, 2025 20:38

mathcolo added 2 commits January 2, 2025 16:04

fix: handle when TZ=Etc/UTC on CI

c5793e3

Bring up to date

bd1ac00

mathcolo added the deploy-to-sandbox deploy this PR to sandbox label Jan 2, 2025

mathcolo deployed to sandbox January 2, 2025 22:13 — with GitHub Actions Active

mathcolo marked this pull request as ready for review January 2, 2025 23:23

mathcolo requested a review from a team as a code owner January 2, 2025 23:23

skyqrose reviewed Jan 3, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: frontend expiring certification warnings and sign-in prevention #205

feat: frontend expiring certification warnings and sign-in prevention #205

mathcolo commented Dec 20, 2024 •

edited

Loading

skyqrose left a comment

skyqrose Jan 3, 2025

skyqrose Jan 3, 2025

skyqrose Jan 3, 2025

skyqrose Jan 3, 2025

skyqrose Jan 3, 2025

skyqrose Jan 3, 2025

skyqrose Jan 3, 2025

skyqrose Jan 3, 2025

skyqrose Jan 3, 2025

skyqrose Jan 3, 2025

feat: frontend expiring certification warnings and sign-in prevention #205

Are you sure you want to change the base?

feat: frontend expiring certification warnings and sign-in prevention #205

Conversation

mathcolo commented Dec 20, 2024 • edited Loading

skyqrose left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathcolo commented Dec 20, 2024 •

edited

Loading