Skip to content

Show Bypass approver option to the workspace admin when they are the report's current approver #98717

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
hungvu193 wants to merge 6 commits into
base: main
Choose a base branch
from
+80 −21
Open

Show Bypass approver option to the workspace admin when they are the report's current approver #98717

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4b87753
bring back byass approver for manager
hungvu193 Aug 15, 2026
1c3176f
Merge remote-tracking branch 'origin' into hans-enable-bypass-approve…
hungvu193 Aug 18, 2026
6d282f5
early return if user is logged in as delegate
hungvu193 Aug 18, 2026
928c86e
continue the loop instead of returning
hungvu193 Aug 18, 2026
f1e636d
Fix eslint
hungvu193 Aug 18, 2026
49457a6
Merge remote-tracking branch 'origin' into hans-enable-bypass-approve…
hungvu193 Aug 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 34 additions & 4 deletions src/pages/DynamicReportChangeApproverPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import FormHelpMessage from '@components/FormHelpMessage';
import HeaderWithBackButton from '@components/HeaderWithBackButton';
import MoneyReportHeaderModals from '@components/MoneyReportHeaderModals';
import useConfirmApproval from '@components/MoneyReportHeaderPrimaryAction/useConfirmApproval';
import RenderHTML from '@components/RenderHTML';
import ScreenWrapper from '@components/ScreenWrapper';
import SelectionList from '@components/SelectionList';
Expand Down Expand Up @@ -62,6 +64,9 @@ function DynamicReportChangeApproverPage({report, policy, isLoadingReportData}:
const hasAutoAppliedRef = useRef(false);
const hasNavigatedToAddApproverRef = useRef(false);
const backPath = useDynamicBackPath(DYNAMIC_ROUTES.REPORT_CHANGE_APPROVER.path);
const isCurrentUserManager = report.managerID === currentUserDetails.accountID;
// The approved animation is part of the report header, which isn't mounted in this RHP, so there is nothing to animate here
const confirmApproval = useConfirmApproval(report.reportID, () => {});

const goBack = () => {
Navigation.goBack(backPath);
Expand All @@ -88,8 +93,25 @@ function DynamicReportChangeApproverPage({report, policy, isLoadingReportData}:
return;
}
assignReportToMe(report, currentUserDetails.accountID, currentUserDetails.email ?? '', policy, hasViolations, isASAPSubmitBetaEnabled, isTrackIntentUser, formatPhoneNumber);
Comment thread
hungvu193 marked this conversation as resolved.
// Taking control only makes the current user the final approver. When they already are the manager, the report
// stays waiting on them, so approve it as well to actually bypass the remaining approvers.
if (isCurrentUserManager) {
confirmApproval();
}
Navigation.dismissToPreviousRHP();
}, [selectedApproverType, report, currentUserDetails.accountID, currentUserDetails.email, policy, hasViolations, isASAPSubmitBetaEnabled, isTrackIntentUser, formatPhoneNumber]);
}, [
selectedApproverType,
report,
currentUserDetails.accountID,
currentUserDetails.email,
policy,
hasViolations,
isASAPSubmitBetaEnabled,
isTrackIntentUser,
formatPhoneNumber,
isCurrentUserManager,
confirmApproval,
]);

const approverTypes = useMemo(() => {
const data: Array<ListItem<ApproverType>> = [
Expand All @@ -101,8 +123,7 @@ function DynamicReportChangeApproverPage({report, policy, isLoadingReportData}:
},
];

const isCurrentUserManager = report.managerID === currentUserDetails.accountID;
if (!isCurrentUserManager && isAllowedToApproveExpenseReport(report, currentUserDetails.accountID, policy)) {
if (isAllowedToApproveExpenseReport(report, currentUserDetails.accountID, policy)) {
data.push({
text: translate('iou.changeApprover.actions.bypassApprovers'),
keyForList: APPROVER_TYPE.BYPASS_APPROVER,
Expand Down Expand Up @@ -188,6 +209,15 @@ function DynamicReportChangeApproverPage({report, policy, isLoadingReportData}:
);
}

export default withReportOrNotFound()(DynamicReportChangeApproverPage);
// The page reads the hold menu from the MoneyReportHeaderModals context, so the provider has to sit above it
function DynamicReportChangeApproverPageWithModals(props: DynamicReportChangeApproverPageProps) {
return (
<MoneyReportHeaderModals reportID={props.report.reportID}>
<DynamicReportChangeApproverPage {...props} />
</MoneyReportHeaderModals>
);
}

export default withReportOrNotFound()(DynamicReportChangeApproverPageWithModals);
export {APPROVER_TYPE};
export type {ApproverType};
64 changes: 48 additions & 16 deletions src/pages/Search/SearchChangeApproverPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import FullPageOfflineBlockingView from '@components/BlockingViews/FullPageOfflineBlockingView';
import {useDelegateNoAccessActions, useDelegateNoAccessState} from '@components/DelegateNoAccessModalProvider';
import FullScreenLoadingIndicator from '@components/FullscreenLoadingIndicator';
import HeaderWithBackButton from '@components/HeaderWithBackButton';
import RenderHTML from '@components/RenderHTML';
Expand All @@ -9,17 +10,20 @@ import SingleSelectListItem from '@components/SelectionList/ListItem/SingleSelec
import type {ListItem} from '@components/SelectionList/types';
import Text from '@components/Text';

import {useCurrencyListActions} from '@hooks/useCurrencyList';
import useCurrentUserPersonalDetails from '@hooks/useCurrentUserPersonalDetails';
import useDelegateAccountID from '@hooks/useDelegateAccountID';
import useEnvironment from '@hooks/useEnvironment';
import useLocalize from '@hooks/useLocalize';
import useNetwork from '@hooks/useNetwork';
import useOnyx from '@hooks/useOnyx';
import usePermissions from '@hooks/usePermissions';
import useThemeStyles from '@hooks/useThemeStyles';

import {assignReportToMe} from '@libs/actions/IOU/ReportWorkflow';
import {approveMoneyRequest, assignReportToMe} from '@libs/actions/IOU/ReportWorkflow';
import {openBulkChangeApproverPage} from '@libs/actions/Search';
import Navigation from '@libs/Navigation/Navigation';
import {getLoginByAccountID} from '@libs/PersonalDetailsUtils';
import {isControlPolicy, isPolicyAdmin} from '@libs/PolicyUtils';
import {hasViolations as hasViolationsReportUtils, isAllowedToApproveExpenseReport} from '@libs/ReportUtils';

Expand All @@ -33,6 +37,7 @@ import type {Policy, Report} from '@src/types/onyx';

import type {OnyxCollection} from 'react-native-onyx';

import {delegateEmailSelector} from '@selectors/Account';
import {isTrackIntentUserSelector} from '@selectors/Onboarding';
import React, {useEffect, useLayoutEffect, useRef, useState} from 'react';
import {View} from 'react-native';
Expand Down Expand Up @@ -87,6 +92,16 @@ function SearchChangeApproverPage() {
const [hasLoadedApp] = useOnyx(ONYXKEYS.HAS_LOADED_APP);
const [isLoadingBulkChangeApproverPage = true] = useOnyx(ONYXKEYS.IS_LOADING_BULK_CHANGE_APPROVER_PAGE);
const {isOffline} = useNetwork();
const {getCurrencyDecimals} = useCurrencyListActions();
const {isDelegateAccessRestricted} = useDelegateNoAccessState();
const {showDelegateNoAccessModal} = useDelegateNoAccessActions();
const [betas] = useOnyx(ONYXKEYS.BETAS);
const [userBillingGracePeriodEnds] = useOnyx(ONYXKEYS.COLLECTION.SHARED_NVP_PRIVATE_USER_BILLING_GRACE_PERIOD_END);
const [amountOwed] = useOnyx(ONYXKEYS.NVP_PRIVATE_AMOUNT_OWED);
const [ownerBillingGracePeriodEnd] = useOnyx(ONYXKEYS.NVP_PRIVATE_OWNER_BILLING_GRACE_PERIOD_END);
const [personalDetails] = useOnyx(ONYXKEYS.PERSONAL_DETAILS_LIST);
Comment thread
hungvu193 marked this conversation as resolved.
const [delegateEmail] = useOnyx(ONYXKEYS.ACCOUNT, {selector: delegateEmailSelector});
const delegateAccountID = useDelegateAccountID();

const getOnyxReports = (allReports: OnyxCollection<Report>) => {
const reports = Object.create(null) as Record<string, Report>;
Expand Down Expand Up @@ -164,17 +179,46 @@ function SearchChangeApproverPage() {
return;
}

if (isDelegateAccessRestricted) {
showDelegateNoAccessModal();

@bernhardoj bernhardoj Aug 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We check for delegate access here, but we don't do the same in DynamicReportChangeApproverPage

@bernhardoj bernhardoj Aug 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just done reading through the AI reviews, this is the same as #98717 (comment). Is it really out of scope? This PR adds the delegate access check before reassigning and approving the report.

Before this PR: bypass approver only reassigns the approver; no delegate access check. This means a delegate can reassign the approver, bulk or individual.
After this PR: bypass approver also approves the report when the current user is the manager. Bulk bypass approver on the search page is totally prevented for a delegate, while individual bypass approver allows delegate to reassign the approver, but not approving the report.

@hungvu193 hungvu193 Aug 18, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking about removing this check because we don't have this before 🤔 Wdyt

@bernhardoj bernhardoj Aug 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we remove the delegate check (so it works like before), we are still left with confirmApproval which prevents approving as a delegate and shows the restricted access modal.

I think it would be weird if we also show the restricted access modal when doing bulk approval before approveMoneyRequest because there would be n calls to the showDelegateNoAccessModal function. Wdyt of just returning early? So, bypass approver will

  1. reassign the approver (allowed for delegate)
  2. approve (not allowed for delegate; return early; nothing happens; approver is successfully reassigned in step 1, so when the user presses Approve manually, they will see the restricted modal)

But this means we need to customize useConfirmApproval for this case (bypass approver).

@hungvu193 hungvu193 Aug 18, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I think that works, early return is better since we shouldn't show delegate modal inside the bulk actions

@hungvu193 hungvu193 Aug 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That works fine, but I think it's a little bit weird that I can bypass the approver by reassigning the approver to me multiple times. If that's expected or out of scope, then we are good.

@bernhardoj How did you do this? AFter approving the Change approver option will be hide 🤔

Screen.Recording.2026-08-22.at.22.07.45.mov

@bernhardoj bernhardoj Aug 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, it's when you are a delegate without access, so it only reassigns without approving.

@hungvu193 hungvu193 Aug 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh... So I guess we don't show By pass option with the delegate access then 🤔

@bernhardoj bernhardoj Aug 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean delegate without access?

I'm actually kinda confuse with this bypass approver feature. If we are already the current approver (manager) (which is the case here), why do we need a bypass approver?

I think this "bypass multiple times" can happen too in a case when the user is not the current manager. When the user is not the current manager, choosing bypass approver will only reassign the final approver to the current user, but will not approve the report. So, since the report is not approved yet, the user can choose bypass approver once again to finally approve it.

If we disable bypass approver for delegate without access, then a delegate without access that is also not the current manager won't be able to reassign the approver to themselve, even though it's allowed.

@hungvu193 hungvu193 Aug 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to double check but, we only show this option when the user is allowed to approve the report. Once report is approved, this option should be hide.

return;
}

for (const selectedReport of selectedReports) {
const policy = allPolicies?.[`${ONYXKEYS.COLLECTION.POLICY}${selectedReport.policyID}`];
const report = selectedReport.reportID ? onyxReports?.[selectedReport.reportID] : undefined;
if (!policy || !report) {
continue;
}

const hasViolations = hasViolationsReportUtils(report.reportID, transactionViolations, currentUserDetails.accountID, currentUserDetails.email ?? '');
assignReportToMe(report, currentUserDetails.accountID, currentUserDetails.email ?? '', policy, hasViolations, isASAPSubmitBetaEnabled, isTrackIntentUser, formatPhoneNumber);
Comment thread
hungvu193 marked this conversation as resolved.

// Taking control only makes the current user the final approver. When they already are the manager, the
// report stays waiting on them, so approve it as well to actually bypass the remaining approvers.
if (report.managerID !== currentUserDetails.accountID) {
const hasViolations = hasViolationsReportUtils(report.reportID, transactionViolations, currentUserDetails.accountID, currentUserDetails.email ?? '');
assignReportToMe(report, currentUserDetails.accountID, currentUserDetails.email ?? '', policy, hasViolations, isASAPSubmitBetaEnabled, isTrackIntentUser, formatPhoneNumber);
continue;
}

approveMoneyRequest({
Comment thread
hungvu193 marked this conversation as resolved.
getCurrencyDecimals,
expenseReport: report,
expenseReportPolicy: policy,
currentUserAccountIDParam: currentUserDetails.accountID,
currentUserEmailParam: currentUserDetails.email ?? '',
hasViolations,
isASAPSubmitBetaEnabled,
betas,
userBillingGracePeriodEnds,
amountOwed,
ownerBillingGracePeriodEnd,
ownerLogin: getLoginByAccountID(report.ownerAccountID, personalDetails),
delegateEmail,
delegateAccountID,
full: true,
shouldPlaySuccessSound: false,
isTrackIntentUser,
});
}

// Note: This clears both reports and transactions
Expand Down Expand Up @@ -202,19 +246,7 @@ function SearchChangeApproverPage() {
return isPolicyAdmin(policy) && isAllowedToApproveExpenseReport(report, currentUserDetails.accountID, policy);
});

const shouldShowBypassApproversOption =
hasPermission &&
selectedReports.some((selectedReport) => {
const report = selectedReport.reportID ? onyxReports?.[selectedReport.reportID] : undefined;

if (!report) {
return false;
}

return report.managerID !== currentUserDetails.accountID;
});

if (shouldShowBypassApproversOption) {
if (hasPermission) {
data.push({
text: translate('iou.changeApprover.actions.bypassApprovers'),
keyForList: APPROVER_TYPE.BYPASS_APPROVER,
Expand Down
Loading