Repository navigation
Show Mark as paid on approved reports with nothing to reimburse - #102347
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] |
ReviewThe header, report preview, and More menu look correct. For the new $0 cases they show only Mark as paid. I found one bug in the Search row and one smaller gap. 1. Search row can offer real bank payment on a $0 report (should fix)The Search row decides to show Pay using one set of transactions, then picks the payment options using another:
To reproduce: as the payer, on a fresh session, go straight to Reports > All reports without opening the $50/-$50 report. The test steps approve the report first, which loads its transactions into Onyx, so they won't catch this. Suggested fix: pass the row's snapshot transactions into Bulk pay has the same mismatch. 2. Scanning check depends on
|
RegressionsI found three regressions from existing behavior. All three affect approved or closed expense reports with $0 reimbursable spend, which never offered Pay before this PR. 1. Pay replaces Export as the one-click action (should fix or get sign-off)On a policy with an accounting connection, a $0 report used to show Export. It now shows Pay, because Pay is checked before Export:
To reproduce: on a QBO or Xero policy with auto-sync off, approve a $50 / -$50 report as an admin who is also the payer. 2. Bulk pay drops to Mark as paid onlyIn Reports, select a $100 report that can be paid by bank plus any $0 report. Before this PR, the $0 report wasn't payable, so all bank options showed. Now 3. It reverses a previous deploy-blocker fixThe two tests this PR flips were added by [cp stag] fix: Hide pay button for non-reimbursable transactions report with 0 amount. That PR fixed [Due for payment 2026-04-14] [Due for payment 2026-04-14] Reports- Nothing happens after clicking bank account when paying non-reimbursable 0.00 expense. The header and preview avoid that symptom now because they offer only Mark as paid. The Search row mismatch from my earlier review brings it back: bank options on a $0 report, and a $0 VBBA payment is sent now that Also changed (likely intended, worth confirming)
I found these by tracing the code against the PR's parent commit. I did not run the app, because these flows need an accounting connection and a business bank account. |
Review of the latest commit (
|
Regressions at
|
|
While testing, I found multiple bugs happening on production. If needed, I can help to fix these separately as followup. Bug 1. Expenses tab: Pay does nothing on a non-reimbursable-only expense
Expected: the report is marked as paid, or the row shows View like its Reports row does bug1.movBug 2. Bulk pay shows bank options when an non-reimbursable $0 expense is selected
Expected: No bank option bug2.movBug 3. Report breakdown hidden when a reimbursable and a non-reimbursable expense cancel out
Expected: Reimbursable $50.00 and Non-reimbursable -$50.00 breakdown shows bug3.movBug4. Green dot 🟢 shows when a reimbursable and a non-reimbursable expense cancel out
Expected: Either show Pay button or hide 🟢 (depending on product decision) bug4.mov |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safariweb1.movweb2.movweb3.mov |
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
Good for product. I like that we are reusing the existing pattern from paying non-reimbursable expenses where we do not show the pay action on the spend page.
|
@apeyada Please fix conflict |
|
Conflicts fixed |
|
Should we create followup issues for them separately? |
mollfpr
left a comment
There was a problem hiding this comment.
Thanks @apeyada! Approving this! I'll create follow up issues for:
- Expenses tab: Pay does nothing on a non-reimbursable-only expense
- Bulk pay shows bank options when an non-reimbursable $0 expense is selected
- Report breakdown hidden when a reimbursable and a non-reimbursable expense cancel out
@apeyada let me know if the conflict is resolved!
|
tests failing on main. I will merge main again once it's fixed |
conflict is resolved. thanks |
|
@apeyada Quick update on bug 2 (bulk pay showing bank options for a $0 non-reimbursable expense). We're going to fix it in this PR: when a $0 non-reimbursable report is selected together with a payable one, the bank option will be hidden for the whole selection, same as what already happens today when the non-reimbursable report isn't $0. We'll open a follow-up issue separately for better, per-report handling of this case instead of hiding the bank option for the whole selection, so look out for that one too. For bug 1 and the report-level bulk action behavior, we'll also handle those as follow-up issues. |
I fixed it now. Thanks this branch: this.branch.mp4production: production has bug of showing bank option when bulk pay includes non-reimbursable-only (either zero or non-zero) expense production.mov |
|
🚧 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! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/mollfpr in version: 9.5.6-0 🚀
|
|
No help site update is needed. This bug fix lets admins close approved $0 reports with Pay > Mark as paid, which is what users already expect, and no article says these reports can't be paid. What I checked
|
|
Hi, @mkhutornyi. Could you please share more detailed steps? |
|
@IuliiaHerets can you pleas take example where any step is unclear? |
|
@IuliiaHerets here is a rewritten version with more defined steps to follow: QA StepsAccounts used in every test
Where to check (as Account B, after A submits and B approves)
1. One positive reimbursable expense (no change expected)Prereq: business bank account connected to the workspace.
2. One negative reimbursable expense (no change expected)Prereq: none.
3. Reimbursable expenses cancel out, total $0 (the reported bug)Prereq: none.
4. One $0 reimbursable expensePrereq: none.
5. Positive reimbursable plus non-reimbursable (no change expected)Prereq: business bank account connected to the workspace.
6. Negative reimbursable plus non-reimbursable (no change expected)Prereq: none.
7. Reimbursable expenses cancel out, non-reimbursable spend remainsPrereq: none.
8. Reimbursable expense offset by a non-reimbursable credit, total $0Prereq: business bank account connected to the workspace (to confirm no bank option appears).
9. One non-reimbursable expense (no change expected)Prereq: none.
10. Non-reimbursable expenses cancel out, total $0Prereq: none.
11. Expense still scanning or failed to scanPrereq: a receipt to scan.
12. Bulk pay with a $0 non-reimbursable reportPrereq: business bank account connected to the workspace.
13. Closed report with payments turned on after it closedPrereq: a workspace where a report can be closed before payments are enabled. Confirm the exact setup with the PR author before testing.
|
|
🚀 Deployed to production by https://github.com/puneetlath in version: 9.5.6-6 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
An approved expense report with nothing left to reimburse had no Pay button anywhere in New Expensify, so it couldn't be closed out. Examples are a $50 and a -$50 expense, a single $0 expense, or a $0 report with only non-reimbursable expenses. Classic offers Mark as paid for these reports.
Pay is controlled by three checks, and each required non-zero reimbursable spend:
isPrimaryPayAction: report header and More menuReportPreviewActionUtils.canPay: chat report previewcanIOUBePaid: whether Pay can be used, plus the LHN badge and the Search rowThis PR adds
hasSettledZeroReimbursableSpendtoReportUtilsand uses it in all three checks. It returns true only when all of these are true:canIOUBePaidonly uses it for pay-elsewhere, so these reports get Mark as paid and never a real payment.Nothing is owed on these reports, so they follow the existing non-reimbursable-only behavior: paying is optional and they aren't pushed to the payer. A new
ReportUtils.isPayOptionalhelper is true when every expense is non-reimbursable or the reimbursable spend is settled at $0. It replaceshasOnlyNonReimbursableTransactionsin the three places that already skipped non-reimbursable-only reports:getBadgeFromIOUReport)SearchUIUtils), which falls back to ViewTodosUtils)The Submit button's "expense has no final value yet" condition is moved into
TransactionUtils.isExpenseValueUnsettled.isTransactionSubmittablenow calls it, so the Submit and Pay rules stay in sync.$0 reimbursable spend (what this PR changes)
LHN badge, Reports row and Ready to pay are the same in prod and this PR, except where noted.
Non-zero reimbursable spend (this PR doesn't change these)
Fixed Issues
$ #102215
PROPOSAL: #102215 (comment)
Tests
Preconditions
How to run each test: as A, create the report described and submit it. As B, approve it. Then as B, check these places:
The test lists what each place should show. Where it says Mark as paid, also click Pay > Mark as paid once and verify the report moves to Paid.
1. Positive reimbursable total (should not change)
2. Negative reimbursable total (should not change)
3. Reimbursable expenses cancel out, report total $0 (the reported bug)
4. A single $0 reimbursable expense
5. Positive reimbursable plus non-reimbursable spend (should not change)
6. Negative reimbursable plus non-reimbursable spend (should not change)
7. Reimbursable expenses cancel out, non-reimbursable spend remains
8. Reimbursable expense offset by a non-reimbursable credit, report total $0
9. Only non-reimbursable spend (should not change)
10. Non-reimbursable expenses cancel out, report total $0
11. An expense still scanning or failed to scan
Report: a $50 and a -$50 reimbursable expense, plus a scanned receipt that SmartScan hasn't finished or that failed and has no amount.
Verify: no Pay button in the header, preview or Reports row, no PAY badge in the LHN, and the report isn't listed in Ready to pay.
Verify that no errors appear in the JS console
Offline tests
Same as Tests
QA Steps
Same as Tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, 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
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
case1.mov
case2.mov
case3.mov
case5.mov
case6.mov
case7.mov
case8.mov
case9.mov
case10.mov