[PM-11588] Bugfix - parse user input value for combined expiry date when creating/adding a card cipher #11103
Conversation
|
New Issues
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #11103 +/- ##
==========================================
+ Coverage 35.05% 35.15% +0.10%
==========================================
Files 2711 2712 +1
Lines 84576 84661 +85
Branches 16069 16093 +24
==========================================
+ Hits 29649 29766 +117
+ Misses 53956 53924 -32
Partials 971 971 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
I really like the extensive test suite you implemented for the utils methods, nice work on this.
My biggest suggestion would be to consider breaking down the parseYearMothExpiry method into several targeted functions. Reading through the nested if/else structure can be tricky..
I don't have any major blockers at the moment, just suggestions. Let me know when you want a re-review of this.
libs/common/src/vault/utils.ts
Outdated
| * @param {string} combinedExpiryValue | ||
| * @return {*} {([string | null, string | null])} | ||
| */ | ||
| export function parseYearMonthExpiry(combinedExpiryValue: string): [Year | null, string | null] { |
There was a problem hiding this comment.
🎨 - This method is crazy busy... might be a good idea to break this down into separate methods that handle scoped logical concerns... even if those methods aren't exported, it'd help with maintaining the business logic later on.
There was a problem hiding this comment.
Yeah, totally. I made a few attempts to consolidate cases for conciseness, but the resulting logic ended up doing unnecessary evaluations (less-likely cases). That's not to say we can't break things up internally, but I need to think about how to make that understandable, a bit more.
There was a problem hiding this comment.
addressed in 0722aff
libs/common/src/vault/utils.ts
Outdated
| // If the parsed delimiter is a whitespace character, assign 's' (character class) instead | ||
| const delimiterPattern = /\s/.test(parsedDelimiter) ? "\\s" : "\\" + parsedDelimiter; |
There was a problem hiding this comment.
A caveat here; this does have the downside of treating all whitespace characters stored in CardExpiryDateDelimiters as equivalent to each other during this string splitting/normalization. This is fine for now, but if we ever need to explicitly distinguish different whitespace characters from each other (though I don't think that need will be likely) we'll need to rethink some of this.
libs/common/src/vault/utils.ts
Outdated
There was a problem hiding this comment.
I've been putting these utility functions in vault as that seemed the most appropriate functional domain (rather than team ownership), but I'm open to other thoughts on location
There was a problem hiding this comment.
So no need to change this really if you don't want... but I feel like if we're writing the validation code here, then it makes sense to incorporate this as part of Autofill team's scope, even if it applies to other external elements.
I'm in favor of pulling the work to our scope.
jprusik
dismissed stale reviews from djsmith85, shane-melton, and cagonzalezcs
via
5870a1f
🎟️ Tracking
PM-11588
📔 Objective
When a combined expiration value is captured from a payment form to be added to/create a new cipher, the date value is not parsed to the separate expMonth and expYear properties.
🦮 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