[Bug] Fix RHP not closing after shipping Expensify Card - #81363
Conversation
The areAllCardsShippedSelector had a logic bug where it used AND instead of filtering. If a user had any personal or company cards, the selector would return false even when all Expensify cards were shipped. This fix: - Creates a new areAllExpensifyCardsShipped selector in Card.ts - Uses isCard and isExpensifyCard to filter to only valid Expensify cards - Checks that all filtered cards are not in STATE_NOT_ISSUED state - Adds unit tests to prevent regression of this bug
|
@parasharrajat 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] |
|
🚧 @mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
Question: is it fine to issue a physical card and add address on a test account? |
|
It is not solved yet. Reason is that the card state is is still non_issued. 04.02.2026_14.21.14_REC.mp4 |
No, it won't work with the Plaid test bank account |
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
Works for me!
2026-02-04_10-52-42.mp4
|
Any thoughts? @mountiny |
|
I will resolve the confclits, but this should be ready for a review |
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! ℹ️ 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". |
|
Actually I think we dont need a c+ review here as Joe tested it works and we can just complete the review internally given you cannot test it fully |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
🚧 @MariaHCD 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/MariaHCD in version: 9.3.13-1 🚀
|
|
🚀 Deployed to staging by https://github.com/MariaHCD in version: 9.3.15-0 🚀
|
|
🚀 Deployed to production by https://github.com/lakchote in version: 9.3.15-10 🚀
|
Explanation of Change
Fix a logic bug in the
areAllCardsShippedSelectorthat prevented the RHP from closing after successfully shipping an Expensify Card when the user also has personal or company cards.The bug was in the AND logic:
card?.state !== STATE_NOT_ISSUED && !isPersonalCard(card). This meant if a user had ANY personal/company card, the selector would returnfalseeven when all Expensify cards were shipped.Changes:
areAllExpensifyCardsShippedselector inselectors/Card.tsisCard()andisExpensifyCard()to filter to only valid Expensify cardsSTATE_NOT_ISSUEDMissingPersonalDetailsMagicCodePage.tsxto use the new selectorFixed Issues
$ #81362
Tests
Preconditions: You must have an Expensify card feed with a real bank account linked - the Plaid testing bank account feed will not ship the card actually so the state of the card is not changed
Offline tests
N/A - The card shipping requires network connectivity
QA Steps
Same as Tests
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