Conversation
|
@MelvinBot can you polish this PR? |
Co-authored-by: Carlos Miceli <carlosmiceli@users.noreply.github.com>
|
Polished this PR with the following changes: Code fix:
PR description:
All 9 |
|
@marcochavezf Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
marcochavezf
left a comment
There was a problem hiding this comment.
Code Review: Add isMatchingCard tests
Overall assessment: Looks good. This is a well-structured, test-only PR that adds coverage for the isMatchingCard utility. The tests are clear, well-organized, and cover the main branches of the function. A few observations below.
Positive aspects
- Good coverage of both matching paths:
encryptedCardNumberexact match and normalizedcardNamefallback. - Appropriate negative test cases (different names, undefined
cardName, empty input). - The special character normalization tests (registered, trademark, copyright symbols) directly exercise the core regex logic.
Suggestions
-
Missing test for
undefinedencryptedCardNumberon the card: TheCardtype may haveencryptedCardNumberas optional/undefined. The function does a strict equality check (===), so ifcard.encryptedCardNumberisundefinedand theencryptedCardNumberparameter is'', they won't match -- which is correct. A test explicitly covering this case (card with noencryptedCardNumberproperty at all, combined with a non-emptyencryptedCardNumberarg) would make the behavior clearer and guard against future regressions. -
Test trimming from the parameter side too: The "should trim whitespace" test puts extra whitespace in
card.cardNamebut not in thecardNameparameter. Sincenormalize()trims both sides of the comparison, it would be more thorough to also test the inverse: acardNameparameter with leading/trailing whitespace matching against a card with a cleancardName. This validates trimming works symmetrically. -
Consider testing additional special characters: The normalize regex
[^\w\s-]strips all non-word, non-whitespace, non-hyphen characters -- not just the three symbols mentioned. Characters like&,', or.(e.g.,"Hilton Honors Amex - J. Smith & Co.") would also be stripped. A test with these characters would better document the breadth of normalization and prevent surprises. -
Test mirroring real-world callsite: In the actual codebase,
isMatchingCardis called as:isMatchingCard(card, cardNumberToCheck, cardNumberToCheck)
The same value is passed for both
encryptedCardNumberandcardName. None of the tests replicate this pattern. Adding a test where both parameters are the same value would help document and protect this real-world usage. -
Nit on test naming: The test "should return false when cardName parameter is empty" passes both
encryptedCardNumberas''andcardNameas''. The name could be slightly more precise, e.g., "should return false when both encryptedCardNumber and cardName parameters are empty", since thefalseis driven by the!cardNameguard after theencryptedCardNumbercheck also fails.
Verdict
These are suggestions for improved coverage, not blockers. The test suite is solid for a first pass and correctly exercises all branches of isMatchingCard. Nice work.
🤖 This comment was generated with the assistance of an AI tool.
|
Codex Review: Didn't find any major issues. 🚀 ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
@fedirjh could you add the checklist please? thank you! |
|
@carlosmiceli, let's also add [No QA] in the title to automatically ✅ this PR in the App deploy checklist |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
🚀 Deployed to staging by https://github.com/carlosmiceli in version: 9.3.17-0 🚀
|
|
🚀 Deployed to production by https://github.com/lakchote in version: 9.3.17-9 🚀
|
Explanation of Change
Adds unit tests for the
isMatchingCardutility function inCardUtils. This function matches cards by encrypted card number or by normalized card name (stripping special characters like ®, ™, ©). The tests cover:encryptedCardNumberFixed Issues
$
PROPOSAL:
Tests
npx jest tests/unit/CardUtilsTest.ts --testNamePattern="isMatchingCard"isMatchingCardtests passOffline tests
N/A - Unit tests only, no network dependency.
QA Steps
N/A - This PR adds unit tests only with no functional changes. [No QA]
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
N/A - Unit tests only
Android: mWeb Chrome
N/A - Unit tests only
iOS: Native
N/A - Unit tests only
iOS: mWeb Safari
N/A - Unit tests only
MacOS: Chrome / Safari
N/A - Unit tests only