-
Notifications
You must be signed in to change notification settings - Fork 0
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
fix(certifications): filter out certifications that aren't relevant for BL signin #219
Conversation
js/models/certification.ts
Outdated
) => { | ||
return cs.filter( | ||
(c) => | ||
(c.type === "rail" && c.railLine === line) || c.type === "right_of_way", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do ROW certs have a line associated with them? Can you use an OL ROW cert on the BL? (I don't think they have separate ROW trainings by line, so I think this is working correctly, but wanna double check.)
Also there's no test for whether an OL ROW cert can apply to BL (all other cases are covered tho, and if you think that test isn't needed that's fine.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ROW certs are line agnostic, that's correct, so that case is really undefined behavior. However, because I tested the hypothetical case whether a blue ROW cert exists, I should probably test the case where an unmatching-line ROW cert exists and is ignored.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jest.mock("../../models/certification", () => ({ | ||
...jest.requireActual("../../models/certification"), | ||
// eslint-disable-next-line @typescript-eslint/no-unsafe-return | ||
filterRelevantForOperators: jest.fn((cs, _line) => cs), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a pure function, so you shouldn't need to mock it or assert that it was called.
You could instead use test data that includes a BL rail cert and a OL rail cert, and assert that the hook only returns the BL cert. (This would probably be easier to do in the existing test case rather than writing a second case.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was doing the mock originally to prevent the helper from getting called even though it's pure (so if its behavior changed the test would still pass), but I admit that isn't strictly necessary. Changed!
Asana Task: 📐 Implement Warnings for Expiring Certifications
Mix of a feature and a fix. Filter out certifications for other lines, so that:
Checklist
(x)
Has tests( )
Doesn't need tests( )
Tests deferred (with justification)(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