Fix approval chain when the admin bypasses approvers by fahimj · Pull Request #70283 · Expensify/App

fahimj · 2025-09-10T15:46:41Z

Explanation of Change

This PR fixes an issue where admins who bypassed approvers using "Change approver" → "Bypass approvers" would see incorrect next step messages and missing Pay buttons when approving expense reports offline.

The root cause was that the getApprovalChain function in ReportUtils.ts calculated approval chains based on policy workflows, ignoring the current report state where an admin had become the manager by bypassing approvers. This caused isLastApprover to return false instead of true, leading to incorrect status predictions.

The fix adds a check at the beginning of getApprovalChain to detect when the current user is the manager (indicating they bypassed approvers) and returns an approval chain containing only them, ensuring correct approval flow logic.

Fixed Issues

$ #69249
PROPOSAL: #69249 (comment)

Tests

Create a test account with admin privileges on a workspace
Create an expense report as a regular employee and submit it
As an admin, go to the expense report and select "Change approver" → "Bypass approvers"
Verify the admin becomes the manager of the report (managerID should match admin's accountID)
Go offline (disconnect internet or use airplane mode)
As the admin, approve the expense report
Verify the next step message shows "Waiting for an admin to pay expense(s)" (not "Waiting for approver to approve expense(s)")
Verify the Pay button appears and is functional
Test on a regular approval flow (without bypassing) to ensure no regression
Verify normal approval chain still works when current user is not the manager

Verify that no errors appear in the JS console

Offline tests

Follow the main test steps above while offline
Verify the correct next step message appears when approving while offline
Verify the Pay button is visible and functional while offline
Test reconnecting to internet and ensure the approval status syncs correctly

QA Steps

Same as tests above. Additional QA considerations:

Test with multiple approvers in the chain before bypassing
Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

android-fix.mov

Android: mWeb Chrome

android-web.mp4

iOS: Native

ios.mov

iOS: mWeb Safari

ios.mov

MacOS: Chrome / Safari

after-web.mp4

MacOS: Desktop

desktop.mp4

… corresponding unit tests. The function now returns only the current user's email if they are the manager of the expense report, indicating that they bypassed other approvers. Unit tests cover both bypass and normal approval scenarios.

melvin-bot · 2025-09-10T15:46:47Z

@parasharrajat 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]

parasharrajat · 2025-09-10T17:37:02Z

+    // Check if the current user is the manager of the expense report (indicating they bypassed approvers)
+    const managerAccountID = expenseReport?.managerID ?? CONST.DEFAULT_NUMBER_ID;
+    if (managerAccountID === currentUserAccountID) {
+        const currentUserEmailForApproval = currentUserEmail ?? '';
+        // Return an approval chain containing only the current user since they are now the sole approver
+        return currentUserEmailForApproval ? [currentUserEmailForApproval] : [];
+    }
+


How does this work when I login with a approver account.

User A submitted an expenes to workspace P.

User B is now assigned as approver via approver workflows where User C is next approver is line.

User B logined and approved that expense.

Whether you will see User C as next approver or not.

I guess you’re referring to the regular flow where the admin does not bypass approvers. From what I’ve observed:

Without my change: getApprovalChain simply returns the value from fullApprovalChain, which in this case is [userB].

With my change: since currentAccount equals managerAccountId (i.e. userB), the logic goes into the if block, and the function also returns [userB].

So in both scenarios, getApprovalChain does not include userC in its return value.

Here is my workflow in my test workspace:

Do I miss anything?

Hmm, it looks like I need to verify this with a case that includes additional approvers. Here’s my updated workflow:

I’ll double-check this workflow and get back to you shortly.

Yes, thank you. I was referring to same advaned approvals.

marcaaron · 2025-09-12T00:37:27Z

+    const nextApprover = fullApprovalChain.length > 0 ? fullApprovalChain[0] : null;
+    const shouldCurrentUserBeNextApprover = nextApprover === currentUserEmailForApproval;
+
+    if (isCurrentUserManager && !shouldCurrentUserBeNextApprover && currentUserEmailForApproval) {


I'm lost on why we are look at the currentUserEmail. What does everyone else see for the next step?

You’re right, getApprovalChain shouldn’t depend on currentUserEmail at all. The approval chain should remain the same regardless of which user is logged in.

marcaaron · 2025-09-12T00:41:33Z

I am questioning the current approach...

Wrong next action message, missing Pay button after bypassing approver & approving offline

That's what we are fixing. I'm not sure if we should change what actually gets returned by the method being modified as it could have unintended consequences elsewhere in the UI? The approval workflow itself does not really change if we bypass approvers on a single report. It is still the same workflow. It was simply taken over in the case of one report.

marcaaron · 2025-09-12T00:44:20Z

IMO the way to determine if the current user got here via "Bypass approvers" and should be the final approver is to look at the report actions. If we see a "TAKECONTROL" action then they should be the final approver and see the option to pay.

marcaaron · 2025-09-12T00:51:50Z

Here's a longer explanation with an edge case to be aware of....

For Bypass Approver we need to:

Check if a manager "took control" of a report by looking at the report's action history
Look for a "TAKE_CONTROL" action by the current approver
If there's a "SUBMITTED" action AFTER the take control action, then the take control is invalidated

When someone takes control, they're essentially saying "I'll handle this report from here, skip the normal approval chain". If someone took control in NewDot they become final approver. But if the report gets resubmitted later (creating a new SUBMITTED action), the take control is canceled and the normal approval chain resumes.

Real-world example:

Employee submits expense → goes to Manager A
Admin sees the report and clicks "Take Control"
Admin becomes the approver, skipping Manager A
Admin can now give final approval
If employee retracts and resubmits the report, the take control is canceled and it goes back to Manager A

LMK if that makes sense.

…or managers" This reverts commit da07184.

… and add corresponding unit tests. The function now returns only the current user's email if they are the manager of the expense report, indicating that they bypassed other approvers. Unit tests cover both bypass and normal approval scenarios." This reverts commit 95d4111.

fahimj · 2025-09-12T03:18:00Z

Thanks for the explanation, that makes sense. I’ll make sure my change covers this edge case.

…actions in expense reports. Update getApprovalChain to incorporate bypass approver logic based on recent actions.

fahimj · 2025-09-12T09:14:03Z

I believe the edge case should be handled now. However, the change is still within getApprovalChain. As @marcaaron mentioned, ideally getApprovalChain should remain untouched.

My proposal was to update the getApprovalChain logic since a bypass approver / take control action effectively changes the approval chain. What would be the better approach?

An alternative would be to adjust isLastApprover instead. That way, we isolate the behavior without changing how the approval chain itself is built. For example:

function isLastApprover(approvalChain: string[], expenseReport?: OnyxEntry<Report>): boolean {
    // Check for take control scenario first
    if (expenseReport) {
        const bypassApprover = getBypassApproverIfTakeControl(expenseReport);
        if (bypassApprover) {
            return bypassApprover === currentUserEmail;
        }
    }
    
    // Original logic
    if (approvalChain.length === 0) {
        return true;
    }
    return approvalChain.at(-1) === currentUserEmail;
}

What do you think about this approach?

parasharrajat · 2025-09-12T14:57:18Z

I will check this today.

marcaaron · 2025-09-12T19:34:42Z

Everyone can see the next steps so we still need to fix:

Wrong next action message

I would propose something like a getNextReciever() method that can tell us who is the next receiver. That's how we do it in the backend so it makes sense to recreate similar logic for the optimistic frontend case.

marcaaron · 2025-09-12T19:36:07Z

And to be clear, if there is no next receiver then we would maybe return null or empty string or something like that. With that information I think everyone can know it is waiting for approval.

fahimj · 2025-09-16T03:32:02Z

We have the wrong next action message because when a user clicks the approve button, we generate an invalid optimistic message in the approveMoneyRequest action. To fix the wrong next action message, we need to add bypass approver logic in one of these locations:

approveMoneyRequest action: This is the action executed when a user clicks the approve button
One of the helper functions that approveMoneyRequest uses:
- getApprovalChain
- isLastApprover
- buildNextStep
One of the helper functions that buildNextStep uses:
- getNextApproverAccountID

I think the closest equivalent to the backend's getNextReceiver function is getNextApproverAccountID. However, only changing getNextApproverAccountID would not correct the predictedNextStatus (needed for buildNextStep to create the next step message) and predictedNextState (needed to create the correct action button) values that are calculated in the approveMoneyRequest action.

Therefore, my suggestion would be to add the bypass approver logic either in getApprovalChain or isLastApprover.

fahimj · 2025-09-16T06:13:53Z

Alternatively, we can create a getApprovalChainWithBypassApprover function that handles the bypass approver logic separately and use it in approveMoneyRequest to get the correct approval chain and getApprovalChain would remain unchanged. You can review the changes using getApprovalChainWithBypassApprover in my separate branch here: main...fahimj:App:fix/69249-getApprovalChainWithBypassApprover

parasharrajat · 2025-09-23T19:15:31Z

I will review this in morning. Thanks for waiting.

parasharrajat · 2025-09-26T18:05:27Z

+    // Check if someone took control and it's still valid
+    const bypassApprover = getBypassApproverIfTakeControl(expenseReport);
+    if (bypassApprover && bypassApprover !== submitterEmail) {
+        return [bypassApprover];
+    }
+


We shouldn't update this method as it gives approver by reading the workspace settings.

We should instead update other places where we are actually trying to get the next approver.

So when user takes an approve action who took control of the report, we find next approver and assign that. But here we should not assign the next approver. We will set the report to the approved state and consider this(current) manager as the last approver.

I’ve updated the code and moved this part to getNextApproverAccountID. Could you take a look and review the changes?

fahimj · 2025-10-14T08:13:11Z

ok merged from main

parasharrajat · 2025-10-14T09:10:32Z

Screenshots

🔲 iOS / native

14.10.2025_17.49.51_REC.mp4

🔲 iOS / Safari

14.10.2025_18.13.46_REC.mp4

🔲 MacOS / Desktop

14.10.2025_17.39.39_REC.mp4

🔲 MacOS / Chrome

14.10.2025_17.31.12_REC.mp4

🔲 Android / Chrome

🔲 Android / native

14.10.2025_17.45.36_REC.mp4

parasharrajat

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android: Native
- Android: mWeb Chrome
- iOS: Native
- iOS: mWeb Safari
- MacOS: Chrome / Safari
- MacOS: Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If the PR modifies the form input styles:
- I verified that all the inputs inside a form are aligned with each other.
- I added Design label so the design team can review the changes.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

🎀 👀 🎀 C+ reviewed

parasharrajat · 2025-10-14T12:47:53Z

@fahimj Please fix the author checklist.

parasharrajat · 2025-10-14T12:52:33Z

LGTM. Tested a few different approval flows.

amyevans

A couple thoughts!

…tainability

amyevans

Looks good!

OSBotify · 2025-10-15T13:49:28Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2025-10-15T23:13:56Z

🚀 Deployed to staging by https://github.com/amyevans in version: 9.2.32-0 🚀

platform	result
🖥 desktop 🖥	success ✅
🕸 web 🕸	success ✅
🤖 android 🤖	success ✅
🍎 iOS 🍎	success ✅

parasharrajat · 2025-10-16T15:22:31Z

We are going to revert this PR due to a timely fix for the issues caused. @fahimj, please create a new PR for the same with fixes for all the issues caused. if you don't have solution, please let us know as well as soon as possible.

[CP Staging] Revert #70283

[CP Staging] Revert #70283 (cherry picked from commit 745e1e7) (cherry-picked to staging by mountiny)

fahimj · 2025-10-16T22:04:55Z

I’ve created a test branch for this issue (#72742 (comment)). I’m still verifying that it doesn’t introduce any other issues. I’ll share an update once I’ve confirmed.

OSBotify · 2025-10-16T23:20:23Z

🚀 Deployed to production by https://github.com/mountiny in version: 9.2.32-6 🚀

platform	result
🖥 desktop 🖥	success ✅
🕸 web 🕸	success ✅
🤖 android 🤖	success ✅
🍎 iOS 🍎	success ✅

fahimj requested a review from a team as a code owner September 10, 2025 15:46

melvin-bot Bot requested a review from parasharrajat September 10, 2025 15:46

melvin-bot Bot assigned fahimj Sep 10, 2025

melvin-bot Bot removed the request for review from a team September 10, 2025 15:46

parasharrajat reviewed Sep 10, 2025

View reviewed changes

fahimj added 2 commits September 11, 2025 06:27

Merge remote-tracking branch 'origin/main' into fix/69249

4069d85

update getApprovalChain function to handle bypass scenarios for managers

da07184

marcaaron reviewed Sep 12, 2025

View reviewed changes

fahimj added 2 commits September 12, 2025 09:09

Revert "update getApprovalChain function to handle bypass scenarios f…

ea3d8a3

…or managers" This reverts commit da07184.

fahimj added 2 commits September 12, 2025 14:31

Add unit tests for take control functionality in getApprovalChain

84221b3

Add getBypassApproverIfTakeControl function to validate take control …

c2a6eac

…actions in expense reports. Update getApprovalChain to incorporate bypass approver logic based on recent actions.

fahimj marked this pull request as draft September 12, 2025 08:26

fahimj marked this pull request as ready for review September 12, 2025 08:33

parasharrajat reviewed Sep 26, 2025

View reviewed changes

fahimj added 2 commits September 27, 2025 17:09

Merge branch 'main' into fix/69249

59741f3

Merge branch 'main' into fix/69249

c4cb8d5

Merge branch 'main' into fix/69249

3e84086

parasharrajat approved these changes Oct 14, 2025

View reviewed changes

melvin-bot Bot requested a review from marcaaron October 14, 2025 12:46

amyevans requested changes Oct 14, 2025

View reviewed changes

fahimj added 5 commits October 15, 2025 07:00

use !nextApproverAccountID directly

18d01e5

update comments

b150943

Refactor role assignments in IOUTest to use constants for better main…

60e21a7

…tainability

remove redundant take control SUBMITTED_AND_CLOSED test case

a13a819

Remove unused personal details entry from ReportUtilsTest

4b6dc18

fahimj requested a review from amyevans October 15, 2025 01:45

amyevans approved these changes Oct 15, 2025

View reviewed changes

amyevans merged commit b2bc284 into Expensify:main Oct 15, 2025
28 checks passed

OSBotify mentioned this pull request Oct 15, 2025

Deploy Checklist: New Expensify 2025-10-15 #72723

Closed

52 tasks

parasharrajat mentioned this pull request Oct 16, 2025

[Due for payment 2025-10-27] [Due for payment 2025-10-23] Expense - Approve button reappears when unapproving and approving report offline #72742

Closed

8 tasks

mountiny mentioned this pull request Oct 16, 2025

[CP Staging] Revert #70283 #72772

Merged

54 tasks

grgia added a commit that referenced this pull request Oct 16, 2025

Merge pull request #72772 from parasharrajat/revert/70283

745e1e7

[CP Staging] Revert #70283

mountiny pushed a commit that referenced this pull request Oct 16, 2025

Merge pull request #72772 from parasharrajat/revert/70283

371e67d

[CP Staging] Revert #70283 (cherry picked from commit 745e1e7) (cherry-picked to staging by mountiny)

lanitochka17 mentioned this pull request Oct 16, 2025

Expense - Wrong approver in the next step message when report is opened as second approver #72801

Closed

16 tasks

melvin-bot Bot mentioned this pull request Oct 16, 2025

[Due for payment 2025-11-13] [Due for payment 2025-10-23] [$125] [Change Approver] Wrong next action message, missing Pay button after bypassing approver & approving offline #69249

Closed

8 tasks

Conversation

fahimj commented Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Explanation of Change

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Uh oh!

melvin-bot Bot commented Sep 10, 2025

Uh oh!

parasharrajat Sep 10, 2025

Choose a reason for hiding this comment

Uh oh!

fahimj Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fahimj Sep 11, 2025

Choose a reason for hiding this comment

Uh oh!

parasharrajat Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcaaron Sep 12, 2025

Choose a reason for hiding this comment

Uh oh!

fahimj Sep 12, 2025

Choose a reason for hiding this comment

Uh oh!

marcaaron commented Sep 12, 2025

Uh oh!

marcaaron commented Sep 12, 2025

Uh oh!

marcaaron commented Sep 12, 2025

Uh oh!

fahimj commented Sep 12, 2025

Uh oh!

fahimj commented Sep 12, 2025

Uh oh!

parasharrajat commented Sep 12, 2025

Uh oh!

marcaaron commented Sep 12, 2025

Uh oh!

marcaaron commented Sep 12, 2025

Uh oh!

fahimj commented Sep 16, 2025

Uh oh!

fahimj commented Sep 16, 2025

Uh oh!

parasharrajat commented Sep 23, 2025

Uh oh!

parasharrajat Sep 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fahimj Sep 27, 2025

Choose a reason for hiding this comment

Uh oh!

fahimj commented Oct 14, 2025

Uh oh!

parasharrajat commented Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Screenshots

🔲 iOS / native

🔲 iOS / Safari

🔲 MacOS / Desktop

🔲 MacOS / Chrome

🔲 Android / Chrome

🔲 Android / native

Uh oh!

parasharrajat left a comment

Choose a reason for hiding this comment

Reviewer Checklist

Uh oh!

parasharrajat commented Oct 14, 2025

Uh oh!

fahimj commented Sep 10, 2025 •

edited

Loading

fahimj Sep 11, 2025 •

edited

Loading

parasharrajat Sep 11, 2025 •

edited

Loading

parasharrajat Sep 26, 2025 •

edited

Loading

parasharrajat commented Oct 14, 2025 •

edited

Loading