Format all masked PANs with the same style#83726
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@MelvinBot please review. |
🤖 MelvinBot ReviewCI Failures (actionable)Two checks are failing and need fixes before this can merge:
Code ReviewOverall: This is a clean, well-scoped change. The function logic, regex guard, and test coverage are all solid. A few observations: Looks good
Suggestion: narrow layout consistencyIn const narrowWidthCardName = isAssigned
? `${customCardName ?? ''}${lastCardNumbers ? ` - ${lastCardNumbers}` : ''}`
: cardName; // <-- raw, unformattedShould this also use PR DescriptionThe PR body is missing a few items:
These should be filled in per the contribution guidelines to help reviewers and QA understand the change. |
|
@MelvinBot fix the failing tests please. |
Replace IACOVINO with SMITH to fix cspell unknown word error, and fix import ordering to satisfy Prettier. Co-authored-by: Carlos Miceli <carlosmiceli@users.noreply.github.com>
|
Fixed the two failing checks:
Both checks pass locally now. Pushed as commit d5739ce. |
1 similar comment
|
Fixed the two failing checks:
Both checks pass locally now. Pushed as commit d5739ce. |
|
@mjasikowski 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] |
|
@carlosmiceli question, why were some cards showing only last 4 before (and why is only the last 4 unmasked after this change)? Is it not possible to show all cards on commercial feeds with the exact same styling? |
|
@joekaufmanexpensify didn't we want to mimick how they are displayed in Classic?
I'm not sure why we have cards stored some with only 4 digits and some with the full masked PAN, I imagine the feed didn't always provide the BIN? @fedirjh would you have a theory? The change here is purely cosmetic, and we'd be matching the style of Classic. We could extract the BIN from other cards and apply the same uncovered first 6 digits style to all of them, but that's only in the case that BINs would always be the same for all cards in the table (if no BIN is found in any card, we'd just default to Xs). |
Interesting, yeah I see that in classic too. That still strikes me as odd. I'd be interested to know why that is, since my understanding is we get the full PAN for all commercial cards from the network. But agree NAB here since that's what classic does now 👍 |
|
Cool, seems like you can review @fedirjh :) |
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
Good for product 👍
|
@fedirjh mind adding the PR reviewer checklist? |
Reviewer Checklist
Screenshots/Videos
Android: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 @carlosmiceli has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/carlosmiceli in version: 9.3.28-0 🚀
|
|
🚀 Deployed to staging by https://github.com/carlosmiceli in version: 9.3.30-0 🚀
|
|
🚀 Deployed to production by https://github.com/blimpich in version: 9.3.30-3 🚀
|


Explanation of Change
Mimicking masked style from Classic for the new Company Cards table.
Before:

After:

Fixed Issues
$
PROPOSAL:
Tests
Added tests.
Offline tests
QA Steps
Steps:
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand 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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari