Skip to content
Open
9 changes: 7 additions & 2 deletions src/components/WorkspaceMemberRoleList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,12 @@ type WorkspaceMemberRoleListProps = {
navigateBackTo?: Route;
isLoading?: boolean;
onSelectRole?: (value: ListItemType) => void;

/** When provided, restricts the selectable roles to this set (e.g. an Authorized Payer may only be an Admin or Payments Admin) */
allowedRoles?: Array<ValueOf<typeof CONST.POLICY.ROLE>>;
};

function WorkspaceMemberRoleList({role, policy, navigateBackTo = undefined, isLoading = false, onSelectRole = () => {}}: WorkspaceMemberRoleListProps) {
function WorkspaceMemberRoleList({role, policy, navigateBackTo = undefined, isLoading = false, onSelectRole = () => {}, allowedRoles = undefined}: WorkspaceMemberRoleListProps) {
const {translate} = useLocalize();
const styles = useThemeStyles();
const {login: currentUserLogin = ''} = useCurrentUserPersonalDetails();
Expand Down Expand Up @@ -86,7 +89,9 @@ function WorkspaceMemberRoleList({role, policy, navigateBackTo = undefined, isLo
},
];

const availableRoleItems: ListItemType[] = workspaceRoles.filter((item) => canMemberAssignRole(policy, currentUserLogin, item.value));
const availableRoleItems: ListItemType[] = workspaceRoles.filter(
(item) => canMemberAssignRole(policy, currentUserLogin, item.value) && (!allowedRoles || allowedRoles.includes(item.value)),
);

return (
<>
Expand Down
15 changes: 15 additions & 0 deletions src/libs/PolicyUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -697,6 +697,19 @@ function getReimburserEmail(policy: OnyxEntry<Policy>): string | undefined {
return policy.reimburser ?? policy.achAccount?.reimburser ?? (isManualReimbursement ? policy.owner : undefined);
}

/**
* Whether the given role is allowed to pay (reimburse) on a workspace.
*/
function canRolePay(role: string | undefined): boolean {
return !!role && ROLE_PERMISSION_BUNDLES[role]?.[CONST.POLICY.POLICY_FEATURE.WORKFLOWS_PAYMENTS] === CONST.POLICY.POLICY_FEATURE_ACCESS.WRITE;
}

/**
* The roles that are allowed to pay (reimburse) on a workspace, derived from the WORKFLOWS_PAYMENTS permission. The
* Authorized Payer (reimburser) must always hold one of these, so any role change for a payer is restricted to this set.
*/
const PAYER_ROLES = Object.values(CONST.POLICY.ROLE).filter(canRolePay);

function isPolicyPayer(policy: OnyxEntry<Policy>, currentUserLogin: string | undefined): boolean {
if (!policy) {
return false;
Expand Down Expand Up @@ -3174,6 +3187,8 @@ export {
isPolicyMember,
isPolicyPayer,
getReimburserEmail,
PAYER_ROLES,
canRolePay,
arePaymentsEnabled,
isSubmitterAndApprover,
isSubmitAndClose,
Expand Down
6 changes: 4 additions & 2 deletions src/pages/workspace/WorkspaceMembersPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -589,7 +589,8 @@ function WorkspaceMembersPage({personalDetails, route, policy}: WorkspaceMembers
options.push(memberOption);
}

if (hasAtLeastOneNonAdminRole && !hasAtLeastOnePayer && canAssignElevatedRoles) {
// Admin is a valid payer role, so the payer may be promoted to Admin (Admin and Payments Admin are the two roles that can pay).
if (hasAtLeastOneNonAdminRole && canAssignElevatedRoles) {
options.push(adminOption);
}

Expand All @@ -611,7 +612,8 @@ function WorkspaceMembersPage({personalDetails, route, policy}: WorkspaceMembers
options.push(peopleAdminOption);
}

if (hasAtLeastOneNonPaymentsAdminRole && isControlPolicy(policy) && !hasAtLeastOnePayer && canAssignElevatedRoles) {
// Payments Admin is a valid payer role, so the payer may be changed to Payments Admin (Admin and Payments Admin are the two roles that can pay).
if (hasAtLeastOneNonPaymentsAdminRole && isControlPolicy(policy) && canAssignElevatedRoles) {
options.push(paymentsAdminOption);
}

Expand Down
15 changes: 9 additions & 6 deletions src/pages/workspace/members/WorkspaceMemberDetailsPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ import {useCompanyCardFeedIcons} from '@hooks/useCompanyCardIcons';
import useConfirmModal from '@hooks/useConfirmModal';
import {useCurrencyListActions} from '@hooks/useCurrencyList';
import useCurrentUserPersonalDetails from '@hooks/useCurrentUserPersonalDetails';
import useEnvironment from '@hooks/useEnvironment';
import useExpensifyCardFeeds from '@hooks/useExpensifyCardFeeds';
import {useMemoizedLazyExpensifyIcons} from '@hooks/useLazyAsset';
import useLocalize from '@hooks/useLocalize';
Expand All @@ -41,6 +40,7 @@ import {
getReimburserEmail,
isControlPolicy,
isPolicyApprover,
PAYER_ROLES,
tryNavigateToSubmitWorkspaceUpgrade,
} from '@libs/PolicyUtils';
import shouldRenderTransferOwnerButton from '@libs/shouldRenderTransferOwnerButton';
Expand Down Expand Up @@ -114,7 +114,6 @@ function WorkspaceMemberDetailsPage({personalDetails, policy, route}: WorkspaceM
const illustrations = useThemeIllustrations();
const companyCardFeedIcons = useCompanyCardFeedIcons();
const {accountID: currentUserAccountID, login: currentUserLogin = ''} = useCurrentUserPersonalDetails();
const {environmentURL} = useEnvironment();
const [cardFeeds] = useCardFeeds(policyID);
const [cardList] = useOnyx(`${ONYXKEYS.COLLECTION.WORKSPACE_CARDS_LIST}`);
const [customCardNames] = useOnyx(ONYXKEYS.NVP_EXPENSIFY_COMPANY_CARDS_CUSTOM_NAMES);
Expand Down Expand Up @@ -142,12 +141,18 @@ function WorkspaceMemberDetailsPage({personalDetails, policy, route}: WorkspaceM
const ownerDetails = personalDetails?.[policy?.ownerAccountID ?? CONST.DEFAULT_NUMBER_ID] ?? ({} as PersonalDetails);
const policyOwnerDisplayName = temporaryGetDisplayNameOrDefault({passedPersonalDetails: ownerDetails, translate, formatPhoneNumber}) ?? policy?.owner ?? '';
const {cardList: assignableCards, ...workspaceCards} = getAllCardsForWorkspace(workspaceAccountID, cardList, cardFeeds, expensifyCardSettings);
const workspaceWorkflowsPageURL = `${environmentURL}/${ROUTES.WORKSPACE_WORKFLOWS.getRoute(policyID, CONST.TAB.WORKFLOWS.PAYMENTS)}`;
const isSMSLogin = Str.isSMSLogin(memberLogin);
const phoneNumber = getPhoneNumber(details);
const reimburserEmail = getReimburserEmail(policy);
const isReimburser = !!reimburserEmail && reimburserEmail === memberLogin;
const canEditSelectedMemberRole = !isSelectedMemberOwner && !isSelectedMemberCurrentUser && !isReimburser && canManageSelectedMemberRole;
// The Authorized Payer (reimburser) may only hold a role that can pay (for example Admin or Payments Admin), so they can only be
// changed to one of those roles. Keep the Role row interactive for them only when there is another payer role they can
// actually move to, for example promoting a non-admin payer to Admin, or switching between Admin and Payments Admin on a
// Control workspace. Otherwise, such as an Admin payer on a non-Control workspace where Payments Admin is unavailable,
// there is no valid change to make, so the row stays read-only.
Comment on lines +148 to +152

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.

Please make comments shorter than the code they describe.

const assignablePayerRoles = PAYER_ROLES.filter((payerRole) => canMemberAssignRole(policy, currentUserLogin, payerRole));
const canReimburserChangeRole = assignablePayerRoles.some((payerRole) => payerRole !== member?.role);
const canEditSelectedMemberRole = !isSelectedMemberOwner && !isSelectedMemberCurrentUser && canManageSelectedMemberRole && (!isReimburser || canReimburserChangeRole);
const {isAccountLocked} = useLockedAccountState();
const {showLockedAccountModal} = useLockedAccountActions();

Expand Down Expand Up @@ -401,8 +406,6 @@ function WorkspaceMemberDetailsPage({personalDetails, policy, route}: WorkspaceM
}
Navigation.navigate(ROUTES.WORKSPACE_MEMBER_DETAILS_ROLE.getRoute(policyID, accountID));
}}
hintText={isReimburser ? translate('common.roleCannotBeChanged', workspaceWorkflowsPageURL) : undefined}
shouldRenderHintAsHTML
/>
{isControlPolicy(policy) && (
<>
Expand Down
11 changes: 10 additions & 1 deletion src/pages/workspace/members/WorkspaceMemberDetailsRolePage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import {isRuleBotEnforcingRules} from '@libs/AgentRulesUtils';
import Navigation from '@libs/Navigation/Navigation';
import type {PlatformStackScreenProps} from '@libs/Navigation/PlatformStackNavigation/types';
import type {SettingsNavigatorParamList} from '@libs/Navigation/types';
import {canMemberAssignRole} from '@libs/PolicyUtils';
import {canMemberAssignRole, canRolePay, getReimburserEmail, PAYER_ROLES} from '@libs/PolicyUtils';

import AccessOrNotFoundWrapper from '@pages/workspace/AccessOrNotFoundWrapper';
import withPolicyAndFullscreenLoading from '@pages/workspace/withPolicyAndFullscreenLoading';
Expand Down Expand Up @@ -40,6 +40,10 @@ function WorkspaceMemberDetailsRolePage({policy, personalDetails, route}: Worksp
const memberLogin = personalDetails?.[accountID]?.login ?? '';
const member = policy?.employeeList?.[memberLogin];
const canManageSelectedMemberRole = canMemberAssignRole(policy, currentUserLogin, member?.role);
// The Authorized Payer (reimburser) must stay a valid payer, so restrict them to the roles that can pay (for example Admin or Payments Admin).
const reimburserEmail = getReimburserEmail(policy);
const isReimburser = !!reimburserEmail && reimburserEmail === memberLogin;
const allowedRoles = isReimburser ? [...PAYER_ROLES] : undefined;
useRedirectSubmitWorkspaceFeatureUpgrade({
policy,
backTo: ROUTES.WORKSPACE_MEMBER_DETAILS.getRoute(policyID, accountID),
Expand All @@ -53,6 +57,10 @@ function WorkspaceMemberDetailsRolePage({policy, personalDetails, route}: Worksp
if (!canMemberAssignRole(policy, currentUserLogin, value)) {
return;
}
// Guard the direct-navigation path: a reimburser must stay a valid payer, so reject any role that cannot pay.
if (isReimburser && !canRolePay(value)) {
return;
}
if (value !== CONST.POLICY.ROLE.ADMIN && isRuleBotEnforcingRules(accountID, policy)) {
showRuleBotGuardModal('changeRole', policyID);
return;
Expand All @@ -76,6 +84,7 @@ function WorkspaceMemberDetailsRolePage({policy, personalDetails, route}: Worksp
role={member?.role}
policy={policy}
onSelectRole={changeRole}
allowedRoles={allowedRoles}
navigateBackTo={ROUTES.WORKSPACE_MEMBER_DETAILS.getRoute(policyID, accountID)}
/>
</ScreenWrapper>
Expand Down
78 changes: 78 additions & 0 deletions tests/ui/WorkspaceMemberDetailsPageTest.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ describe('WorkspaceMemberDetailsPage', () => {
const primaryAccountID = 7777;
const primaryEmail = 'primary@example.com';
const secondaryEmail = 'secondary@example.com';
const adminPayerAccountID = 8888;
const adminPayerEmail = 'adminpayer@example.com';

const policy = {
...LHNTestUtils.getFakePolicy(),
Expand All @@ -83,6 +85,7 @@ describe('WorkspaceMemberDetailsPage', () => {
[invitedEmail]: {email: invitedEmail, role: CONST.POLICY.ROLE.USER},
[phoneLogin]: {email: phoneLogin, role: CONST.POLICY.ROLE.USER},
[primaryEmail]: {email: primaryEmail, role: CONST.POLICY.ROLE.USER},
[adminPayerEmail]: {email: adminPayerEmail, role: CONST.POLICY.ROLE.ADMIN},
},
};

Expand All @@ -102,6 +105,7 @@ describe('WorkspaceMemberDetailsPage', () => {
[invitedAccountID]: TestHelper.buildPersonalDetails(invitedEmail, invitedAccountID, 'Invited'),
[phoneAccountID]: TestHelper.buildPersonalDetails(phoneLogin, phoneAccountID, 'Phone'),
[primaryAccountID]: TestHelper.buildPersonalDetails(primaryEmail, primaryAccountID, 'Primary'),
[adminPayerAccountID]: TestHelper.buildPersonalDetails(adminPayerEmail, adminPayerAccountID, 'AdminPayer'),
});
await Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY}${policy.id}`, policy);
});
Expand Down Expand Up @@ -223,6 +227,80 @@ describe('WorkspaceMemberDetailsPage', () => {
await waitForBatchedUpdatesWithAct();
});

it('should not lock the Role field for a non-admin Authorized Payer so they can be promoted to Admin', async () => {
// Make the invited member (a plain USER) the workspace Authorized Payer.
await act(async () => {
await Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY}${policy.id}`, {
reimbursementChoice: CONST.POLICY.REIMBURSEMENT_CHOICES.REIMBURSEMENT_YES,
reimburser: invitedEmail,
});
});

const {unmount} = renderPage({policyID: policy.id, accountID: String(invitedAccountID)});
await waitForBatchedUpdatesWithAct();

await waitFor(() => {
expect(screen.getByTestId('WorkspaceMemberDetailsPage')).toBeOnTheScreen();
});

// The locked hint must NOT be shown — a non-admin payer can still be promoted to Admin.
expect(screen.queryByText(/Role can/)).not.toBeOnTheScreen();

unmount();
await waitForBatchedUpdatesWithAct();
});

it('should not lock the Role field for an Authorized Payer who is already an Admin so they can be changed to Payments Admin', async () => {
// The admin member is the workspace Authorized Payer — Payments Admin is also a valid payer, so a lateral change is allowed.
await act(async () => {
await Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY}${policy.id}`, {
reimbursementChoice: CONST.POLICY.REIMBURSEMENT_CHOICES.REIMBURSEMENT_YES,
reimburser: adminPayerEmail,
});
});

const {unmount} = renderPage({policyID: policy.id, accountID: String(adminPayerAccountID)});
await waitForBatchedUpdatesWithAct();

await waitFor(() => {
expect(screen.getByTestId('WorkspaceMemberDetailsPage')).toBeOnTheScreen();
});

// The locked hint must NOT be shown — an admin payer can still be changed to Payments Admin, another valid payer role.
expect(screen.queryByText(/Role can/)).not.toBeOnTheScreen();

unmount();
await waitForBatchedUpdatesWithAct();
});

it('should keep the Role field interactive for an admin Authorized Payer on a non-Control workspace because Editor also holds the payments permission', async () => {
// On a Team (non-Control) workspace, Payments Admin is not assignable, but Editor also holds the WORKFLOWS_PAYMENTS
// permission, so an admin payer still has another valid payer role to switch to. The Role row must stay interactive.
await act(async () => {
await Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY}${policy.id}`, {
type: CONST.POLICY.TYPE.TEAM,
reimbursementChoice: CONST.POLICY.REIMBURSEMENT_CHOICES.REIMBURSEMENT_YES,
reimburser: adminPayerEmail,
});
});

const {unmount} = renderPage({policyID: policy.id, accountID: String(adminPayerAccountID)});
await waitForBatchedUpdatesWithAct();

await waitFor(() => {
expect(screen.getByTestId('WorkspaceMemberDetailsPage')).toBeOnTheScreen();
});

const roleItem = await screen.findByTestId('member-role-menu-item');

// Editor is another payer role the admin payer can switch to, so the row is interactive with no lock hint.
expect(roleItem).not.toBeDisabled();
expect(screen.queryByText(/Role can/)).not.toBeOnTheScreen();

unmount();
await waitForBatchedUpdatesWithAct();
});

it('should show the not found page when the accountID matches no workspace member', async () => {
const {unmount} = renderPage({policyID: policy.id, accountID: '999999'});
await waitForBatchedUpdatesWithAct();
Expand Down
Loading
Loading