fix: show "Waiting for this report to be paid." generic message when a non-admin approver approves request #74647
fix: show "Waiting for this report to be paid." generic message when a non-admin approver approves request #74647etCoderDysto wants to merge 15 commits intoExpensify:mainfrom
Conversation
|
@aimane-chnaif 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] |
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.
|
tests/actions/IOUTest.ts
Outdated
| }); | ||
|
|
||
| it('should mention an admin to pay expenses in optimistic next step message when admin takes control and approves', async () => { | ||
| it('should refer an admin as "you" in optimistic next step message when admin takes control and approves', async () => { |
There was a problem hiding this comment.
Please also add test case of referring an admin as other person
There was a problem hiding this comment.
Sure! @aimane-chnaif I have some failing tests. I will change it to draft pr, and I will ping you once it is ready for review
There was a problem hiding this comment.
Please also add test case of referring an admin as other person
@aimane-chnaif here we are assigning No further action required to the optimistic next step when the approver is not the payer. For that reason, there won't be any case where Waiting for an admin to pay next message is returned when an approver approves the request.
Lines 851 to 853 in 654606a
We show No further action required optimistically, and backend returns Waiting for an admin to pay message
Screen.Recording.2025-11-11.at.3.23.59.in.the.afternoon.mp4
Should I change the above condition to show Waiting for an admin to pay message when a non-admin approver approves the payment? I will write an admin test case if we made the change.
if (isInvoiceReport(report) || reimbursableSpend === 0) {
optimisticNextStep = noActionRequired;
break;
}There was a problem hiding this comment.
@carlosmiceli what do you think? I feel like it's not bug.
There was a problem hiding this comment.
Sorry, I'm not quite following what's the discussed solution, is it what message should be returned from the BE and also optimistically when a non-admin approves?
There was a problem hiding this comment.
Cool, thank you!
There was a problem hiding this comment.
There was a problem hiding this comment.
Hi @carlosmiceli, did we get an answer for the issue?
There was a problem hiding this comment.
Asking about this on Slack as we speak, lol 😄
There was a problem hiding this comment.
| }, | ||
| { | ||
| text: `an admin`, | ||
| text: `you`, |
There was a problem hiding this comment.
Same here. Please add test case when text: an admin
There was a problem hiding this comment.
Sorry, I updated a wrong unit test function here. The test I meant to change was for buildNextStepNew function. But I changed the test for buildNextStep earlier. I have reverted the chagne.
|
The type check error comes from main. I think the unit test it a time out error. |
JmillsExpensify
left a comment
There was a problem hiding this comment.
PR looks good from a product perspective.
|
@etCoderDysto @aimane-chnaif we decided to simplify the solution to the bug, and go with a generic message that works across the board instead. Let's go with |
|
@etCoderDysto Backend PR is up for review, confirming again that new BE message is going to be "Waiting for this report to be paid", except for the case where it says "Waiting for this bill to be paid". Want to update the PR here to match that so we can have it ready to merge once BE is deployed? |
|
@etCoderDysto @aimane-chnaif updating the plan here... Could we implement this new standard message just in App regardless of the message sent from the BE? I realize that we don't actually need to make BE changes for such a generic message. |
Sure! I will make the update by early Monday. |
|
@aimane-chnaif @carlosmiceli I have made the change to show "Waiting for this report to be paid." when a non-admin approver aproves a report. The original issue is fixed by this pr #73270. And I have removed the code for the original issue fix. And i have noticed that there is a plan to depreciate Lines 256 to 268 in 036fc27 |
I don't think we are using both to determine what next step message to display for user. For now we are only using optimistic next message built using When we are using Lines 9802 to 9803 in 90ca5a8 Then we are storing the optimistic next step message built using buildNextStepNew in reportNext_ onyx key Lines 9858 to 9861 in 90ca5a8 And we use it in MoneyRequestHeader to show next step message in MoneyReportHeaderStatusBarApp/src/components/MoneyReportHeader.tsx Line 194 in 90ca5a8 But for the optimistic next step message from buildOptimisticNextStep, we are storing the value in report.nextStep, and we are not using it to display the next step message in Lines 9838 to 9855 in 90ca5a8 |
|
That confused me a bit more 😅 Am I to understand that all we need in this case then is |
I have made things complicated 😄. To make things simple we are not using next message data from |
|
@aimane-chnaif PR is ready for further review. Please take a look at this comment for latest change on this PR |
@aimane-chnaif a gentle reminder to review the new changes |
|
@etCoderDysto please fix conflict. reviewing |
|
Please also update outdated tests steps. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppios.moviOS: mWeb SafariMacOS: Chrome / Safari
web.mov |
I have resolved conflict and updated old test steps |
Is this correct? I am seeing "No further action required!" |
|
job 3 failure came from this PR |
@etCoderDysto I mean above is current behavior from backend, and "Waiting for admin to pay the expense" in your test case is wrong. |
|
I am totally not sure this PR is needed as the current optimistic message ""No further action required!" matches with backend. Also, it doesn't make sense to show "Waiting for this report to be paid" when "Approve" is last action (No "Pay" action needed) ios.mov |
@aimane-chnaif I am still seeing "waiting for an admin to pay expenses" message from BE. I agree that "Waiting for this report to be paid" might be confusing when approvers last action is "approve". Perhaps changing it to "waiting for an admin to pay expenses" mirroring BE value might be good. Here is my workflow configuration Screen.Recording.2025-12-03.at.12.19.04.in.the.morning.mp4 |
fixed |
|
@etCoderDysto please check this video. "Approve" is last action. No one will "Pay" this expense. Screen.Recording.2025-12-02.at.10.31.55.pm.mov |
@aimane-chnaif Got it! In that case, I think we should display a generic |
|
@carlosmiceli what's the next step you're proposing? |
|
Could you provide a summary of the conclusion as to why this PR is no longer needed? |
|
@etCoderDysto bump on above question |
|
|
Ok, cool, if it was fixed already then we can close. I'll close the issue as well. |

Explanation of Change
Fixed Issues
$ #74281
PROPOSAL: #74281 (comment)
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Test 1
Prerequisite1: Account has at least one workspace.
Prerequisite 2: Delayed submissions and approvals enabled.
Prerequisite 3: Owner has invited employee and approver
Prerequisite 4. Make the approver account the only approver of the employee
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.Screen.Recording.2025-11-24.at.10.57.30.at.night.mp4
Android: mWeb Chrome
Android.chrome.Screen.Recording.2025-11-24.at.4.31.33.in.the.afternoon.mp4
iOS: Native
ios.Screen.Recording.2025-11-24.at.4.25.46.in.the.afternoon.mp4
iOS: mWeb Safari
ios.safari.Screen.Recording.2025-11-24.at.4.28.27.in.the.afternoon.mp4
MacOS: Chrome / Safari
webScreen.Recording.2025-11-24.at.4.04.39.in.the.afternoon.mp4
MacOS: Desktop
Desktop.Screen.Recording.2025-11-24.at.4.20.39.in.the.afternoon.mp4