Skip PAY badge in LHN for non-reimbursable-only reports - #89219
Conversation
Reports containing only non-reimbursable expenses were getting a pinned
PAY badge in the LHN because getBadgeFromIOUReport treated the
onlyShowPayElsewhere path the same as a direct pay path. Since paying
these reports is optional and they are never added to the pay queue,
pinning them clutters the LHN with unresolvable actions.
Split the two canIOUBePaid calls in getBadgeFromIOUReport so that
when only the pay-elsewhere path returns true and the report has
only non-reimbursable transactions, the PAY badge is skipped.
The pay-elsewhere path for negative reimbursable spend (mark-as-paid)
is preserved.
Co-authored-by: {"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"} <{"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"}@users.noreply.github.com>
The test uses createRandomTransaction which sets reimbursable to a random boolean. The new hasOnlyNonReimbursableTransactions check in getBadgeFromIOUReport depends on this value, causing flaky failures when reimbursable randomly becomes false. Explicitly setting reimbursable: true makes the test deterministic. Co-authored-by: Situ Chandra Shil <situchan@users.noreply.github.com>
|
Fixed the failing check: the test "should return PAY badge for negative expense (credit) via onlyShowPayElsewhere path" was flaky because |
|
@MelvinBot pull main |
…rNonReimbursableReports Co-authored-by: Situ Chandra Shil <situchan@users.noreply.github.com> # Conflicts: # src/libs/actions/IOU/ReportWorkflow.ts
|
Merged main and resolved the merge conflict in |
…romIOUReport Co-authored-by: Situ Chandra Shil <situchan@users.noreply.github.com>
| const isNonReimbursableOnly = hasOnlyNonReimbursableTransactions(iouReport?.reportID); | ||
| if (!isNonReimbursableOnly) { | ||
| return CONST.REPORT.ACTION_BADGE.PAY; | ||
| } |
There was a problem hiding this comment.
Done — moved the hasOnlyNonReimbursableTransactions check to the top of getBadgeFromIOUReport so it early-returns undefined before any pay/approve/submit logic runs.
|
@MelvinBot revert last commit |
…etBadgeFromIOUReport" This reverts commit ef33b06.
|
Reverted the last commit ( |
|
MelvinBot has made 5 consecutive commits trying to fix CI failures. Automatic CI fix dispatches are now paused for this PR. Next steps: A human should review the recent commits and resolve the conflicting CI errors manually. |
Co-authored-by: Situ Chandra Shil <situchan@users.noreply.github.com>
| const isNonReimbursableOnly = hasOnlyNonReimbursableTransactions(iouReport?.reportID); | ||
| if (!isNonReimbursableOnly) { | ||
| return CONST.REPORT.ACTION_BADGE.PAY; | ||
| } |
There was a problem hiding this comment.
Applied — simplified to a ternary.
| } | ||
| const canBePaidElsewhere = canIOUBePaid(iouReport, chatReport, policy, undefined, currentUserLogin, currentUserAccountID, undefined, true, undefined, invoiceReceiverPolicy); | ||
| if (canBePaidElsewhere) { | ||
| return hasOnlyNonReimbursableTransactions(iouReport?.reportID) ? undefined : CONST.REPORT.ACTION_BADGE.PAY; |
Add inline comment to getBadgeFromIOUReport explaining why PAY badge is skipped for non-reimbursable-only reports on the pay-elsewhere path. Add two unit tests: - Non-reimbursable-only report returns undefined (no PAY badge) - Mixed reimbursable/non-reimbursable report still returns PAY badge Co-authored-by: Situ Chandra Shil <situchan@users.noreply.github.com>
|
Added an inline comment explaining the non-reimbursable pay-elsewhere logic and two unit tests for
All tests pass. |
|
@MelvinBot add Tests/QA Steps and check off all items in author checklist as verified by me |
|
Updated the PR body per
|
|
@MelvinBot update Tests / QA Steps to be exactly same as in the linked issue |
|
Updated the Tests and QA Steps sections to match the reproduction steps from the linked issue. |
|
@MelvinBot fix failing test |
After merging main, canApproveIOU now receives currentUserAccountID explicitly. The test was setting managerID to RORY_ACCOUNT_ID and passing the same ID as currentUserAccountID, causing canApproveIOU to return true and the APPROVE badge to appear. Use a different managerID so the test isolates the non-reimbursable PAY badge skip behavior. Co-authored-by: Situ Chandra Shil <situchan@users.noreply.github.com>
|
Fixed the failing test. After merging main, |
|
@MelvinBot mark as ready for review |
|
Marked as ready for review ✅ |
| expect(result).toBe(CONST.REPORT.ACTION_BADGE.PAY); | ||
| }); | ||
|
|
||
| it('should skip PAY badge for non-reimbursable-only report via pay-elsewhere path', async () => { |
There was a problem hiding this comment.
@MelvinBot this test still pass even on main branch. It should be failed without fix.
There was a problem hiding this comment.
Good catch — the test was using REIMBURSEMENT_NO which makes arePaymentsEnabled() return false, so canIOUBePaid could never return true regardless of the fix. Updated to use REIMBURSEMENT_MANUAL with APPROVED status and correct totals (total = nonReimbursableTotal = -5000). Verified the test now fails on main (returns PAY) and passes with the fix (returns undefined).
|
Marked as ready for review ✅ |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid.movAndroid: mWeb ChromeiOS: HybridAppios.moviOS: mWeb SafariMacOS: Chrome / Safariweb.mov |
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #87762 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
@MelvinBot when you said this in OP of the PR, did you mean non-reimbursable spend, instead of negative? |
|
No, the wording is intentional. "Negative reimbursable spend" and "non-reimbursable spend" are two different things:
The sentence is saying: "we preserved the mark-as-paid flow for credit-type reimbursable expenses, even though we removed the badge for non-reimbursable-only reports." |
|
@MelvinBot I don't understand the distinction on the negative reimbursable aspect. We show the mark as paid option for any reimbursable expense that is being paid, no? What you described for skipping this badge with non-reimbursable is correct. |
|
You're right — the "negative reimbursable" callout in the description is misleading. The mark-as-paid flow works for any reimbursable expense (positive or negative), and this PR doesn't change that. What happened: I'll simplify the description to just say: non-reimbursable-only reports skip the PAY badge; all reimbursable reports (including negative amounts) continue to show it as before. |
|
@joekaufmanexpensify you can ignore negative amount mention here. We don't change that production behavior in this PR. Just mentioned for regression test case. |
flaviadefaria
left a comment
flaviadefaria
left a comment
There was a problem hiding this comment.
From a product standpoint, the fix looks good to me.
|
@chiragsalian looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
|
Not an emergency. I'm pretty sure i saw all green when i merged it. Checking. |
|
Looks like action "PR Reviews with Claude Code" was cancelled because a second run was triggered. Nothing concerning, moving on. |
|
🚧 @chiragsalian 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/chiragsalian in version: 9.3.79-1 🚀
Bundle Size Analysis (Sentry): |
|
After reviewing the PR changes and the help site articles under This PR modifies internal badge display logic ( The existing FAQ in |
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.3.79-4 🚀
|
6 participants
Explanation of Change
Reports containing only non-reimbursable expenses were getting a pinned PAY badge (GBR) in the LHN because
getBadgeFromIOUReport()treated theonlyShowPayElsewherepath the same as a direct pay path. Since paying these reports is optional (they are never added to the pay queue), pinning them clutters the LHN with actions users will never resolve.This PR splits the two
canIOUBePaidcalls ingetBadgeFromIOUReport()so that when only the pay-elsewhere path returns true and the report has only non-reimbursable transactions, the PAY badge is skipped. The pay-elsewhere path for negative reimbursable spend (mark-as-paid flow) is preserved.Fixed Issues
$ #87762
PROPOSAL: #87762 (comment)
Tests
Offline tests
QA Steps
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