Fix OAuth company cards showing as unassigned#81936
Conversation
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
melvin-bot
bot
requested review from
deetergp and
joekaufmanexpensify
and removed request for
a team
|
@chrispader @fedirjh @hungvu193 not sure who can review this sooner, but it'd be great to get it out there soon to see if it solves the issue that's been going on for a while for our users 🙏 |
|
@MelvinBot can you review too? |
carlosmiceli
changed the title
|
The changes look straightforward, let me do some testing. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d8d3564dca
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const cardLast4 = card.lastFourPAN ?? card.cardName?.match(/(\d{4})$/)?.[1]; | ||
| const searchLast4 = cardName?.match(/(\d{4})$/)?.[1]; | ||
|
|
||
| return cardLast4 && searchLast4 && cardLast4 === searchLast4; |
There was a problem hiding this comment.
Matching OAuth rows solely by lastFourPAN can map multiple table rows to the same assigned card when two cards in a feed share the same last 4 digits. In cardsData, find() returns the first card with a matching suffix, so a second card with the same suffix is shown as assigned to the wrong person (and the real card may appear unassigned), which is a correctness issue for large card sets where suffix collisions are possible.
Useful? React with 👍 / 👎.
🤖 MelvinBot Code ReviewThe approach of falling back to last-4-digit matching for OAuth feeds makes sense given the problem (special characters like 1. False positive risk with last-4-digit matchingThe current fallback matches only on last 4 digits: const cardLast4 = card.lastFourPAN ?? card.cardName?.match(/(\d{4})$/)?.[1];
const searchLast4 = cardName?.match(/(\d{4})$/)?.[1];
return cardLast4 && searchLast4 && cardLast4 === searchLast4;If a company has two cards ending in the same 4 digits (e.g., two Amex cards both ending in Suggestion: Consider adding an additional signal to reduce false positives. For example, since the issue is specifically about special characters (like // Normalize both strings to remove special characters, then compare
const normalizedCardName = card.cardName?.normalize('NFKD').replace(/[^\w\s-]/g, '').trim();
const normalizedSearchName = cardName?.normalize('NFKD').replace(/[^\w\s-]/g, '').trim();
return normalizedCardName && normalizedSearchName && normalizedCardName === normalizedSearchName;This would be immune to 2. Existing utility functionThere's already a function lastFourNumbersFromCardName(cardName: string | undefined): string {
const name = cardName ?? '';
const hasSpace = /\s/.test(name);
const match = name.match(/(\d{4})$/);
if (!cardName || cardName === '' || !hasSpace || !match) {
return '';
}
return match[1];
}Note that the utility has an additional 3.
|
joekaufmanexpensify
left a comment
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
Change makes sense from a product perspective
| const cardLast4 = card.lastFourPAN ?? card.cardName?.match(/(\d{4})$/)?.[1]; | ||
| const searchLast4 = cardName?.match(/(\d{4})$/)?.[1]; | ||
|
|
||
| return cardLast4 && searchLast4 && cardLast4 === searchLast4; |
There was a problem hiding this comment.
@carlosmiceli Can we fallback to cardName checking, in case Last4 is empty or we have multiple cards that share the same Last4 ?
| if (card.encryptedCardNumber === encryptedCardNumber) { | ||
| return true; | ||
| } | ||
|
|
There was a problem hiding this comment.
| // Normalize the card name | |
| if (card.cardName.replace(/[^\w\s-]/g, '') === cardName.replace(/[^\w\s-]/g, '')) { | |
| return true; | |
| } |
I think we can also normalize the card names, similar to this PR :
There was a problem hiding this comment.
If last4 can be the same between multiple cards, I think that we only need to compare the cardName here? 🤔
@carlosmiceli Also @MelvinBot 's suggestions are correct, we should also update isCardAlreadyAssigned. So let's create a function and reuse it then.
|
@hungvu193 Agreed — matching on last 4 alone is too loose. Here's a concrete suggestion for how to structure this: Create a shared /**
* Checks if a card matches a given card identifier (encrypted card number or card name).
* Uses exact match for encrypted card numbers, and normalized string comparison
* for card names to handle special character differences from OAuth providers (e.g., ® vs no ®).
*/
function isMatchingCard(card: Card, encryptedCardNumber: string, cardName: string): boolean {
if (card.encryptedCardNumber === encryptedCardNumber) {
return true;
}
if (!card.cardName || !cardName) {
return false;
}
// Normalize both strings: decompose Unicode (NFKD) and strip non-alphanumeric/space/dash chars
// This handles ®, ™, and similar special character differences from OAuth providers
const normalize = (s: string) => s.normalize('NFKD').replace(/[^\w\s-]/g, '').trim();
return normalize(card.cardName) === normalize(cardName);
}Then:
This keeps the full card name comparison (so two cards ending in the same 4 digits won't collide) while handling the @carlosmiceli Would you like me to implement this, or would you prefer to update the PR yourself? |
|
@hungvu193 @fedirjh implemented suggestions, ready again! |
Reviewer Checklist
Screenshots/VideosiOS: HybridAppios.moviOS: mWeb SafarimMsafari.movMacOS: Chrome / SafariChrome.mov |
|
@cristipaval 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] |
|
🎯 @hungvu193, thanks for reviewing and testing this PR! 🎉 An E/App issue has been created to issue payment here: #82024. |
cristipaval
left a comment
cristipaval
left a comment
There was a problem hiding this comment.
NAB: Could we also add some tests for the isMatchingCard function?
Sounds good, will do in a follow up PR! |
|
🚧 @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! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/carlosmiceli in version: 9.3.17-0 🚀
|
|
Hi @carlosmiceli. Do we need QA this? |
|
@IuliiaHerets Mmmm, probably, yes. I'd make this go through any Amex feed QA we have. |
|
@carlosmiceli, please add QA steps then |
|
@IuliiaHerets don't we have any Test case for connecting to an Amex feed and ensuring assigned and unassigned cards are properly displayed? Maybe we don't, I just assumed we did. |
|
@carlosmiceli, we have only this one? |
|
cc @hungvu193 the card still shows as |
|
Checked with @carlosmiceli in Slack, and the PR has mostly fixed the bug and uncovered another existing bug. Going to check off the PR from the checklist, since there's no regression. |
|
🚀 Deployed to production by https://github.com/lakchote in version: 9.3.17-9 🚀
|
9 participants
Explanation of Change
Fixes company card matching logic to correctly identify assigned cards for OAuth feeds (Amex, Chase, Citi, etc.) by normalizing strings to remove special characters instead of exact card name matching.
Problem
Cards are incorrectly showing as "Unassigned" in the Company Cards table even though they have been assigned to employees.
Root Cause
The existing matching logic compares card names as exact strings. This fails for OAuth feeds when card names contain special characters:
Fixed Issues
https://github.com/Expensify/Expensify/issues/596544
#80745
PROPOSAL:
Tests
This is easier to test in staging with real customer data because OAuth providers return actual card names with special characters that we should check for.
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari