[PM-16667] Followup clarifying work #12665
Conversation
| ["0", `${currentYear}`, true], // invalid month | ||
| ["0", `${currentYear}`, false], // invalid month |
There was a problem hiding this comment.
updated test expectation to match consistency changes for uncertain cases
|
No New Or Fixed Issues Found |
| @@ -53,7 +51,7 @@ export function normalizeExpiryYearFormat(yearInput: string | number): Year | nu | |||
|
|
|||
| /** | |||
| * Takes a cipher card view and returns "true" if the month and year affirmativey indicate | |||
| * the card is expired. | |||
| * the card is expired. Uncertain cases return "false". | |||
There was a problem hiding this comment.
⛏️ I usually use ticks to indicate literal values instead of strings to avoid confusion.
| * the card is expired. Uncertain cases return "false". | |
| * the card is expired. Uncertain cases return `false`. |
| const normalizedYear = normalizeExpiryYearFormat(expYear); | ||
| const parsedYear = parseInt(normalizedYear, 10); | ||
| const parsedYear = normalizedYear ? parseInt(normalizedYear, 10) : NaN; |
There was a problem hiding this comment.
🎨 Consider using nullish resolution (??) instead of a ternary if normalizeExpiryYearFormat returns undefined or null. If it returns a false value consider using a lazy or (||). If it returns a falsy value (e.g. 0) then test for the value directly.
| // If the parsed month value is 0, we don't know when the expiry passes this year, so do not treat it as expired | ||
| if (!parsedMonthIsValid) { | ||
| return false; |
There was a problem hiding this comment.
👍🏻 Thank you for addressing my feedback from the other review!
| } | ||
|
|
||
| const normalizedParsedYear = normalizeExpiryYearFormat(parsedYear); | ||
| const normalizedParsedYear = parsedYear ? normalizeExpiryYearFormat(parsedYear) : null; |
There was a problem hiding this comment.
⛏️ Fun fact! null should be avoided where you don't need to distinguish it from undefined.
💭 My fun facts are not very fun.
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #12665 +/- ##
==========================================
- Coverage 33.74% 33.73% -0.01%
==========================================
Files 2918 2918
Lines 90911 90920 +9
Branches 17180 17187 +7
==========================================
- Hits 30677 30676 -1
- Misses 57841 57848 +7
- Partials 2393 2396 +3 ☔ View full report in Codecov by Sentry. |
🎟️ Tracking
PM-16667
📔 Objective
ts-strict-ignoreand make necessary changestrueif the month is not provided/invalid, and the year is the current year. However,isCardExpiredis meant to returnfalsein uncertain cases (put another way, we're asking "is the card definitely expired?"). The outcome of this change is that the expiration notice/banner would have appeared if the user specified the current year in their card expiry but no month. After these changes, the banner will not appear in this case.🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes