Fix duplicate assign card rows for commercial feed cards#83737
Fix duplicate assign card rows for commercial feed cards#83737
Conversation
|
@MelvinBot please fix errors and review |
Co-authored-by: Carlos Miceli <carlosmiceli@users.noreply.github.com>
Co-authored-by: Carlos Miceli <carlosmiceli@users.noreply.github.com>
ESLint FixesPushed two commits to fix the failing ESLint check:
Code ReviewThe dedup logic is correct for the primary scenario and handles edge cases well. A few observations: Looks Good
Minor Concerns
Neither of these is a blocker — the test coverage confirms the primary scenario works correctly. |
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.
|
|
@MelvinBot please increase the code coverage. |
|
@ikevin127 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] |
|
@hoangzinh 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 can you use https://stackoverflowteams.com/c/expensify/questions/23399 to add screenshots (either redacted, or in the linked E/E issue) of before/after for affected account(s)? |
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@MelvinBot please merge main and fix tests |
|
@codex review |
…-cards Co-authored-by: Carlos Miceli <carlosmiceli@users.noreply.github.com>
|
Merged |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a50adf4f7a
ℹ️ 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 cardEntryID = encryptedCardNumber ?? normalizedName; | ||
|
|
||
| const existingEntry = entriesMap.get(cardEntryID); | ||
|
|
||
| const isRicherRecord = card.lastFourPAN && !existingEntry?.assignedCard?.lastFourPAN; |
There was a problem hiding this comment.
Preserve distinct assigned cards when suffix matching is ambiguous
When resolveCardListEntry() cannot disambiguate old-format cards (e.g., two cardList names both end with 0487), it returns the original cards without an encrypted ID, and this code then uses the fallback cardName as cardEntryID. If two different assigned cards share that same short name, they collide on the same map key and one row is dropped, so affected workspaces can lose visibility/access to unassign one of the assigned cards.
Useful? React with 👍 / 👎.
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
Good for product. Noting for myself that this is a follow up to #82788 (comment) for when I QA this
Added to issue description! |
|
@joekaufmanexpensify can this be merged now? |
|
@amyevans was the Slack response sufficient? Are you able to review? |
|
Sorry, I didn't see the emoji reaction on the message there 😅 |
|
🚧 @amyevans 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/amyevans in version: 9.3.43-0 🚀
Bundle Size Analysis (Sentry): |
|
@joekaufmanexpensify could you QA this one on staging please? |
|
QA'ing and I think the behavior here is not quite what we want still. Discussing. |
|
Checked off because above discussion does not block proceeding |
Explanation of Change
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/600254
PROPOSAL:
Tests
Before (production):
After (this PR):
https://github.com/Expensify/Expensify/issues/600254#issuecomment-4075647842
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
We'll QA this with @joekaufmanexpensify
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