Repository navigation
fix: auto report when card expense belongs to other workspace - #102017
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.
|
|
@mkhutornyi 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] |
|
Please fix conflict |
Review: 1 likely regressionAuto report can disappear for valid admin moves when workspace card lists aren't loaded in Onyx. The new gate Auto report only shows when In App code, How to reproduce:
Auto report is hidden, but the move would succeed. Before this PR, the option showed here. The same happens if the admin only opened one feed, because each feed's cards load separately. Suggested fix: Load the admin's workspace card lists before gating. Or gate only on cards you know can't resolve, for example a card on a feed of a workspace the viewer doesn't administer, instead of treating "not in Onyx" as "can't resolve." The backend may already return these lists at sign-in. I can't confirm that from App code, so please check. Other notes (no action needed)
|
|
@mkhutornyi i resolved |
|
What do you think of #102017 (comment)? |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari102017.mov |
|
Please confirm if we should fix #102017 (comment) or demote as edge case |
|
The failing test is not related to this PR, will merge main to fix it soon |
I can't reproduce this bug on my end. |
|
@mkhutornyi could you please update your checklist? |
|
Author Checklist is failing |
|
I updated |
|
All checks pass |
| function canResolveTransactionCard(transaction: OnyxEntry<Pick<Transaction, 'managedCard' | 'cardID'>>, nonPersonalAndWorkspaceCards: OnyxEntry<CardList>): boolean { | ||
| // Inlined rather than calling isManagedCardTransaction: TransactionUtils already imports this file. | ||
| if (!transaction?.managedCard || !transaction.cardID) { | ||
| return false; |
There was a problem hiding this comment.
nonPersonalAndWorkspaceCards is not authoritative: OpenSearchPage loads only 50 admin-visible cards. A valid card outside that subset makes this return false, hiding Move even though auto-report would succeed.
I think you would need to load or resolve the selected card IDs before treating absence as no access.
There was a problem hiding this comment.
@yuwenmemon i updated, canResolveTransactionCard now takes whether the card list is complete, and only reads a card's absence as no access once it is
yuwenmemon
left a comment
There was a problem hiding this comment.
Thanks -- this handles the 50-card case after a successful fetch, but returning true while the list is incomplete reintroduces the original bug.
Also, you have Jest tests failing.
yuwenmemon
left a comment
There was a problem hiding this comment.
This still treats an unknown card as accessible while the full list is pending or unavailable offline, which can reproduce the original error. The completion flag is also set in finallyData, so failed requests are treated as complete. Please keep Auto report unavailable/loading until the request succeeds and retry after failure.
Review: 2 regressions, 1 riskAll three come from how the 1. The original bug still reproduces while the card list is loading or offline. To reproduce:
2. A failed fetch marks the list complete and hides valid Auto report. 3. Risk: the flag never resets until sign-out. It isn't RAM-only. If the admin later gets a new feed or workspace, Auto report hides for valid moves unless those cards reach Suggested fix:
Other notes
|
|
@mkhutornyi @yuwenmemon i resolved all comments, could you please check again? |
|
@daledah Unit tests are still failing |
|
@yuwenmemon all looks good now |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 yuwenmemon 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/yuwenmemon in version: 9.5.6-0 🚀
|
|
No help site update is needed. This bug fix only hides the Auto report option when a selected card expense uses a card on a feed the user doesn't administer. That is an edge case, and no article under |
|
🚀 Deployed to production by https://github.com/puneetlath in version: 9.5.6-6 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
Fixed Issues
$ #101767
PROPOSAL:
Tests
Precondition:
For Admin:
For User B:
Perform the steps below as admin:
Offline tests
Same as tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, 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.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
iOS: Native
ios.mov
MacOS: Chrome / Safari
web.mov