[Payment due @bernhardoj] Properly display CSV feeds before we load the cards#88136
Conversation
|
@bernhardoj 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] |
This comment was marked as resolved.
This comment was marked as resolved.
|
@bernhardoj can we prioritize reviewing this today? |
|
Reviewing now |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safariweb.mp4 |
bernhardoj
left a comment
There was a problem hiding this comment.
LGTM!
(note that this issue still happens to me)
|
🎯 @bernhardoj, thanks for reviewing and testing this PR! 🎉 A payment issue will be created for your review once this PR is deployed to production. If payment is not needed (e.g., regression PR review fix etc), react with 👎 to this comment to prevent the payment issue from being created. |
|
@mollfpr can you do your review of this and merge it if you're happy? Thanks. |
|
🚧 @mollfpr has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
Address Copilot review feedback on PR #88359: gate the four new linkedPolicyIDs tests on `result.current[1].status === 'loaded'` (the ResultMetadata from useOnyx) before running the assertion, so that negative checks don't pass vacuously while Onyx is still loading. Verified locally that all 7 tests pass on main, and that reverting the predicate to its pre-#88136 form still correctly fails the `linkedPolicyIDs: ['']` and `linkedPolicyIDs: []` regression tests. Made-with: Cursor
Addresses Copilot review comment on PR #88359 (#88359 (comment)): the four new tests previously matched feeds via key.includes(oauthFeed) and gated on result.current[1].status === 'loaded' (an Onyx implementation detail). Now compute the exact combined feed key with getCardFeedWithDomainID and put toHaveProperty([combinedFeedKey]) inside waitFor for the three positive cases, so the wait resolves on the actual expectation. The negative case keeps the loaded-status gate before asserting absence to avoid a vacuous pass while Onyx is still hydrating. Verified locally: - All 7 tests pass on main. - Reverting the predicate to its pre-#88136 form still fails the two linkedPolicyIDs: [''] / [] regression tests. Made-with: Cursor
Pin the corrected behavior of the include predicate in useCardFeeds: domain feeds whose `linkedPolicyIDs` is `[]` or `['']` (a legacy/invalid shape persisted by older clients and by DomainAPI's default when `preferredPolicy` was empty) should still match via `preferredPolicy` rather than being silently dropped. The predicate fix itself shipped in PR Expensify#88136; this commit adds the regression coverage that PR did not. Made-with: Cursor
|
🚀 Deployed to staging by https://github.com/mollfpr in version: 9.3.61-0 🚀
Bundle Size Analysis (Sentry): |
|
👋 I've reviewed the changes in this PR against the help site articles under No help site changes are required. This PR is a bug fix that ensures CSV company card feeds (both NewDot
No new UI flows, labels, buttons, or user-facing features were introduced — the CSV import workflow already existed. The existing help site articles (including Set up a Direct Company Card Feed Connection, Configure Company Card Settings, and Import Personal Card Transactions From a Spreadsheet) remain accurate as-is. |
|
🚀 Deployed to production by https://github.com/marcaaron in version: 9.3.61-4 🚀
|
Explanation of Change
Before, we did not display CSV feeds with no cards in Onyx. However, unassigned cards are not loaded until the feed is selected (which we couldn't do because we were not displaying it). We will now display the feed.
This likely also caused part of the issue by which we did not properly display the feed after a first import
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/623387
$ #86814
$ https://github.com/Expensify/Expensify/issues/548636
PROPOSAL:
Tests
Go to Workspace -> Company Cards
Click Add cards.
Select United States > Next.
Select Direct feed > Next.
Select Import transactions from file > Next.
Enter Company card layout name and save it.
Click Next.
Upload a valid CSV file like this one:
uploads_1594908368712-Best+Example+CSV+for+Domains.csv
Map Account number to Card number.
Click Import.
Confirm the cards import properly
Go to OldDot -> Domains and select a Newdot Workspace
Click CSV Import.
Follow the process to import cards. You can use this file:
uploads_1594908368712-Best+Example+CSV+for+Domains.csv
Go to newdot and confirm the new feed displays in Workspace -> Company Cards
Offline tests
None
QA Steps
Same as tests
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