From 6b678ca0ab7e644ffdc3cb785354721a2de59540 Mon Sep 17 00:00:00 2001 From: Benjamin Limpich Date: Fri, 24 Oct 2025 12:58:44 -0700 Subject: [PATCH] Revert "Refactored usage of ONYXKEYS.PERSONAL_DETAILS_LIST from Member Actions" --- src/libs/actions/IOU.ts | 2 +- src/libs/actions/Policy/Member.ts | 101 ++++++---- src/pages/workspace/WorkspaceMembersPage.tsx | 183 ++++++++++++------ .../members/WorkspaceMemberDetailsPage.tsx | 10 +- tests/actions/PolicyMemberTest.ts | 14 +- 5 files changed, 205 insertions(+), 105 deletions(-) diff --git a/src/libs/actions/IOU.ts b/src/libs/actions/IOU.ts index 9c61ca96a358..afb03a6c0d38 100644 --- a/src/libs/actions/IOU.ts +++ b/src/libs/actions/IOU.ts @@ -5809,7 +5809,7 @@ function shareTrackedExpense(trackedExpenseParams: TrackedExpenseParams) { optimisticData: addAccountantToWorkspaceOptimisticData, successData: addAccountantToWorkspaceSuccessData, failureData: addAccountantToWorkspaceFailureData, - } = buildUpdateWorkspaceMembersRoleOnyxData(policyID, [accountantEmail], [accountantAccountID], CONST.POLICY.ROLE.ADMIN); + } = buildUpdateWorkspaceMembersRoleOnyxData(policyID, [accountantAccountID], CONST.POLICY.ROLE.ADMIN); optimisticData?.push(...addAccountantToWorkspaceOptimisticData); successData?.push(...addAccountantToWorkspaceSuccessData); failureData?.push(...addAccountantToWorkspaceFailureData); diff --git a/src/libs/actions/Policy/Member.ts b/src/libs/actions/Policy/Member.ts index 6e941e65a631..ab9a40eccf46 100644 --- a/src/libs/actions/Policy/Member.ts +++ b/src/libs/actions/Policy/Member.ts @@ -29,7 +29,17 @@ import * as ReportUtils from '@libs/ReportUtils'; import * as FormActions from '@userActions/FormActions'; import CONST from '@src/CONST'; import ONYXKEYS from '@src/ONYXKEYS'; -import type {ImportedSpreadsheetMemberData, InvitedEmailsToAccountIDs, Policy, PolicyEmployee, PolicyOwnershipChangeChecks, Report, ReportAction, ReportActions} from '@src/types/onyx'; +import type { + ImportedSpreadsheetMemberData, + InvitedEmailsToAccountIDs, + PersonalDetailsList, + Policy, + PolicyEmployee, + PolicyOwnershipChangeChecks, + Report, + ReportAction, + ReportActions, +} from '@src/types/onyx'; import type {PendingAction} from '@src/types/onyx/OnyxCommon'; import type {JoinWorkspaceResolution} from '@src/types/onyx/OriginalMessage'; import type {ApprovalRule} from '@src/types/onyx/Policy'; @@ -45,6 +55,7 @@ type OnyxDataReturnType = { }; type WorkspaceMembersRoleData = { + accountID: number; email: string; role: ValueOf; }; @@ -99,6 +110,12 @@ Onyx.connect({ }, }); +let allPersonalDetails: OnyxEntry; +Onyx.connect({ + key: ONYXKEYS.PERSONAL_DETAILS_LIST, + callback: (val) => (allPersonalDetails = val), +}); + let policyOwnershipChecks: Record; Onyx.connect({ key: ONYXKEYS.POLICY_OWNERSHIP_CHANGE_CHECKS, @@ -117,6 +134,12 @@ function isApprover(policy: OnyxEntry, employeeLogin: string) { ); } +/** Temporary function alias for isApprover with employeeAccountID */ +function isApproverTemp(policy: OnyxEntry, employeeAccountID: number) { + const employeeLogin = allPersonalDetails?.[employeeAccountID]?.login; + return isApprover(policy, employeeLogin ?? ''); +} + /** * Returns the policy of the report * @deprecated Get the data straight from Onyx - This will be fixed as part of https://github.com/Expensify/Expensify/issues/507850 @@ -411,19 +434,20 @@ function resetAccountingPreferredExporter(policyID: string, loginList: string[]) * Remove the passed members from the policy employeeList * Please see https://github.com/Expensify/App/blob/main/README.md#Security for more details */ -function removeMembers(policyID: string, selectedMemberEmails: string[], policyMemberEmailsToAccountIDs: Record) { - if (selectedMemberEmails.length === 0) { +function removeMembers(accountIDs: number[], policyID: string) { + // In case user selects only themselves (admin), their email will be filtered out and the members + // array passed will be empty, prevent the function from proceeding in that case as there is no one to remove + if (accountIDs.length === 0) { return; } - const accountIDs = selectedMemberEmails.map((email) => policyMemberEmailsToAccountIDs[email]).filter((id) => id !== undefined); - const policyKey = `${ONYXKEYS.COLLECTION.POLICY}${policyID}` as const; // This will be fixed as part of https://github.com/Expensify/Expensify/issues/507850 // eslint-disable-next-line @typescript-eslint/no-deprecated const policy = getPolicy(policyID); const workspaceChats = ReportUtils.getWorkspaceChats(policyID, accountIDs); + const emailList = accountIDs.map((accountID) => allPersonalDetails?.[accountID]?.login).filter((login) => !!login) as string[]; const optimisticClosedReportActions = workspaceChats.map(() => ReportUtils.buildOptimisticClosedReportAction(sessionEmail, policy?.name ?? '', CONST.REPORT.ARCHIVE_REASON.REMOVED_FROM_POLICY), ); @@ -433,19 +457,18 @@ function removeMembers(policyID: string, selectedMemberEmails: string[], policyM CONST.REPORT.CHAT_TYPE.POLICY_ADMINS, policy?.id, policy?.name ?? '', - selectedMemberEmails - .filter((login) => { - const role = login ? policy?.employeeList?.[login]?.role : ''; - return role === CONST.POLICY.ROLE.ADMIN || role === CONST.POLICY.ROLE.AUDITOR; - }) - .map((login) => policyMemberEmailsToAccountIDs[login]), + accountIDs.filter((accountID) => { + const login = allPersonalDetails?.[accountID]?.login; + const role = login ? policy?.employeeList?.[login]?.role : ''; + return role === CONST.POLICY.ROLE.ADMIN || role === CONST.POLICY.ROLE.AUDITOR; + }), ); - const preferredExporterOnyxData = resetAccountingPreferredExporter(policyID, selectedMemberEmails); + const preferredExporterOnyxData = resetAccountingPreferredExporter(policyID, emailList); const optimisticMembersState: OnyxCollectionInputValue = {}; const successMembersState: OnyxCollectionInputValue = {}; const failureMembersState: OnyxCollectionInputValue = {}; - selectedMemberEmails.forEach((email) => { + emailList.forEach((email) => { optimisticMembersState[email] = {pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE}; successMembersState[email] = null; failureMembersState[email] = {errors: ErrorUtils.getMicroSecondOnyxErrorWithTranslationKey('workspace.people.error.genericRemove')}; @@ -455,7 +478,7 @@ function removeMembers(policyID: string, selectedMemberEmails: string[], policyM const employee = policy?.employeeList?.[employeeEmail]; optimisticMembersState[employeeEmail] = optimisticMembersState[employeeEmail] ?? {}; failureMembersState[employeeEmail] = failureMembersState[employeeEmail] ?? {}; - if (employee?.submitsTo && selectedMemberEmails.includes(employee?.submitsTo)) { + if (employee?.submitsTo && emailList.includes(employee?.submitsTo)) { optimisticMembersState[employeeEmail] = { ...optimisticMembersState[employeeEmail], submitsTo: policy?.owner, @@ -465,7 +488,7 @@ function removeMembers(policyID: string, selectedMemberEmails: string[], policyM submitsTo: employee?.submitsTo, }; } - if (employee?.forwardsTo && selectedMemberEmails.includes(employee?.forwardsTo)) { + if (employee?.forwardsTo && emailList.includes(employee?.forwardsTo)) { optimisticMembersState[employeeEmail] = { ...optimisticMembersState[employeeEmail], forwardsTo: policy?.owner, @@ -475,7 +498,7 @@ function removeMembers(policyID: string, selectedMemberEmails: string[], policyM forwardsTo: employee?.forwardsTo, }; } - if (employee?.overLimitForwardsTo && selectedMemberEmails.includes(employee?.overLimitForwardsTo)) { + if (employee?.overLimitForwardsTo && emailList.includes(employee?.overLimitForwardsTo)) { optimisticMembersState[employeeEmail] = { ...optimisticMembersState[employeeEmail], overLimitForwardsTo: policy?.owner, @@ -488,7 +511,7 @@ function removeMembers(policyID: string, selectedMemberEmails: string[], policyM }); const approvalRules: ApprovalRule[] = policy?.rules?.approvalRules ?? []; - const optimisticApprovalRules = approvalRules.filter((rule) => !selectedMemberEmails.includes(rule?.approver ?? '')); + const optimisticApprovalRules = approvalRules.filter((rule) => !emailList.includes(rule?.approver ?? '')); const optimisticData: OnyxUpdate[] = [ { @@ -496,7 +519,7 @@ function removeMembers(policyID: string, selectedMemberEmails: string[], policyM key: policyKey, value: { employeeList: optimisticMembersState, - approver: selectedMemberEmails.includes(policy?.approver ?? '') ? policy?.owner : policy?.approver, + approver: emailList.includes(policy?.approver ?? '') ? policy?.owner : policy?.approver, rules: { ...(policy?.rules ?? {}), approvalRules: optimisticApprovalRules, @@ -611,7 +634,7 @@ function removeMembers(policyID: string, selectedMemberEmails: string[], policyM if (!isEmptyObject(policy?.primaryLoginsInvited ?? {})) { // Take the current policy members and remove them optimistically const employeeListEmails = Object.keys(allPolicies?.[`${ONYXKEYS.COLLECTION.POLICY}${policyID}`]?.employeeList ?? {}); - const remainingLogins = employeeListEmails.filter((email) => !selectedMemberEmails.includes(email)); + const remainingLogins = employeeListEmails.filter((email) => !emailList.includes(email)); const invitedPrimaryToSecondaryLogins: Record = {}; if (policy?.primaryLoginsInvited) { @@ -652,21 +675,28 @@ function removeMembers(policyID: string, selectedMemberEmails: string[], policyM }); const params: DeleteMembersFromWorkspaceParams = { - emailList: selectedMemberEmails.join(','), + emailList: emailList.join(','), policyID, }; API.write(WRITE_COMMANDS.DELETE_MEMBERS_FROM_WORKSPACE, params, {optimisticData, successData, failureData}); } -function buildUpdateWorkspaceMembersRoleOnyxData(policyID: string, selectedMemberEmails: string[], selectedMemberAccountIDs: number[], newRole: ValueOf) { +function buildUpdateWorkspaceMembersRoleOnyxData(policyID: string, accountIDs: number[], newRole: ValueOf) { const previousEmployeeList = {...allPolicies?.[`${ONYXKEYS.COLLECTION.POLICY}${policyID}`]?.employeeList}; - const memberRoles: WorkspaceMembersRoleData[] = selectedMemberEmails.map((login: string) => { - return { - email: login, + const memberRoles: WorkspaceMembersRoleData[] = accountIDs.reduce((result: WorkspaceMembersRoleData[], accountID: number) => { + if (!allPersonalDetails?.[accountID]?.login) { + return result; + } + + result.push({ + accountID, + email: allPersonalDetails?.[accountID]?.login ?? '', role: newRole, - }; - }); + }); + + return result; + }, []); const optimisticData: OnyxUpdate[] = [ { @@ -728,7 +758,7 @@ function buildUpdateWorkspaceMembersRoleOnyxData(policyID: string, selectedMembe const failureDataParticipants: Record = {...adminRoom.participants}; const optimisticParticipants: Record = {}; if (newRole === CONST.POLICY.ROLE.ADMIN || newRole === CONST.POLICY.ROLE.AUDITOR) { - selectedMemberAccountIDs.forEach((accountID) => { + accountIDs.forEach((accountID) => { if (adminRoom?.participants?.[accountID]) { return; } @@ -736,7 +766,7 @@ function buildUpdateWorkspaceMembersRoleOnyxData(policyID: string, selectedMembe failureDataParticipants[accountID] = null; }); } else { - selectedMemberAccountIDs.forEach((accountID) => { + accountIDs.forEach((accountID) => { if (!adminRoom?.participants?.[accountID]) { return; } @@ -764,8 +794,8 @@ function buildUpdateWorkspaceMembersRoleOnyxData(policyID: string, selectedMembe return {optimisticData, successData, failureData, memberRoles}; } -function updateWorkspaceMembersRole(policyID: string, selectedMemberEmails: string[], selectedMemberAccountIDs: number[], newRole: ValueOf) { - const {optimisticData, successData, failureData, memberRoles} = buildUpdateWorkspaceMembersRoleOnyxData(policyID, selectedMemberEmails, selectedMemberAccountIDs, newRole); +function updateWorkspaceMembersRole(policyID: string, accountIDs: number[], newRole: ValueOf) { + const {optimisticData, successData, failureData, memberRoles} = buildUpdateWorkspaceMembersRoleOnyxData(policyID, accountIDs, newRole); const params: UpdateWorkspaceMembersRoleParams = { policyID, @@ -1122,10 +1152,11 @@ function askToJoinPolicy(policyID: string) { /** * Removes an error after trying to delete a member */ -function clearDeleteMemberError(policyID: string, login: string) { +function clearDeleteMemberError(policyID: string, accountID: number) { + const email = allPersonalDetails?.[accountID]?.login ?? ''; Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY}${policyID}`, { employeeList: { - [login]: { + [email]: { pendingAction: null, errors: null, }, @@ -1136,10 +1167,11 @@ function clearDeleteMemberError(policyID: string, login: string) { /** * Removes an error after trying to add a member */ -function clearAddMemberError(policyID: string, login: string, accountID: number) { +function clearAddMemberError(policyID: string, accountID: number) { + const email = allPersonalDetails?.[accountID]?.login ?? ''; Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY}${policyID}`, { employeeList: { - [login]: null, + [email]: null, }, }); Onyx.merge(`${ONYXKEYS.PERSONAL_DETAILS_LIST}`, { @@ -1362,4 +1394,5 @@ export { clearWorkspaceInviteRoleDraft, setImportedSpreadsheetMemberData, clearImportedSpreadsheetMemberData, + isApproverTemp, }; diff --git a/src/pages/workspace/WorkspaceMembersPage.tsx b/src/pages/workspace/WorkspaceMembersPage.tsx index cc72643571b6..c22df9171f91 100644 --- a/src/pages/workspace/WorkspaceMembersPage.tsx +++ b/src/pages/workspace/WorkspaceMembersPage.tsx @@ -3,6 +3,7 @@ import {deepEqual} from 'fast-equals'; import React, {useCallback, useContext, useEffect, useMemo, useRef, useState} from 'react'; import type {TextInput} from 'react-native'; import {InteractionManager, View} from 'react-native'; +import type {OnyxEntry} from 'react-native-onyx'; import type {ValueOf} from 'type-fest'; import Button from '@components/Button'; import ButtonWithDropdownMenu from '@components/ButtonWithDropdownMenu'; @@ -37,7 +38,7 @@ import { clearInviteDraft, clearWorkspaceOwnerChangeFlow, downloadMembersCSV, - isApprover, + isApproverTemp, openWorkspaceMembersPage, removeMembers, updateWorkspaceMembersRole, @@ -49,7 +50,7 @@ import Navigation from '@libs/Navigation/Navigation'; import type {PlatformStackScreenProps} from '@libs/Navigation/PlatformStackNavigation/types'; import type {WorkspaceSplitNavigatorParamList} from '@libs/Navigation/types'; import {isPersonalDetailsReady, sortAlphabetically} from '@libs/OptionsListUtils'; -import {getDisplayNameOrDefault, getPersonalDetailsByIDs} from '@libs/PersonalDetailsUtils'; +import {getAccountIDsByLogins, getDisplayNameOrDefault, getPersonalDetailsByIDs} from '@libs/PersonalDetailsUtils'; import {getMemberAccountIDsForWorkspace, isDeletedPolicyEmployee, isExpensifyTeam, isPaidGroupPolicy, isPolicyAdmin as isPolicyAdminUtils} from '@libs/PolicyUtils'; import {getDisplayNameForParticipant} from '@libs/ReportUtils'; import tokenizedSearch from '@libs/tokenizedSearch'; @@ -60,8 +61,8 @@ import CONST from '@src/CONST'; import ONYXKEYS from '@src/ONYXKEYS'; import ROUTES from '@src/ROUTES'; import type SCREENS from '@src/SCREENS'; -import type {PersonalDetails, PolicyEmployee, PolicyEmployeeList} from '@src/types/onyx'; -import type {PendingAction} from '@src/types/onyx/OnyxCommon'; +import type {PersonalDetails, PersonalDetailsList, PolicyEmployee, PolicyEmployeeList} from '@src/types/onyx'; +import type {Errors, PendingAction} from '@src/types/onyx/OnyxCommon'; import {isEmptyObject} from '@src/types/utils/EmptyObject'; import MemberRightIcon from './MemberRightIcon'; import type {WithPolicyAndFullscreenLoadingProps} from './withPolicyAndFullscreenLoading'; @@ -78,14 +79,26 @@ function invertObject(object: Record): Record { return Object.fromEntries(invertedEntries); } -type MemberOption = Omit & {accountID: number; login: string}; +type MemberOption = Omit & {accountID: number}; function WorkspaceMembersPage({personalDetails, route, policy}: WorkspaceMembersPageProps) { - const policyMemberEmailsToAccountIDs = useMemo(() => getMemberAccountIDsForWorkspace(policy?.employeeList, true), [policy?.employeeList]); - const employeeListDetails = useMemo(() => policy?.employeeList ?? ({} as PolicyEmployeeList), [policy?.employeeList]); + const {policyMemberEmailsToAccountIDs, employeeListDetails} = useMemo(() => { + const emailsToAccountIDs = getMemberAccountIDsForWorkspace(policy?.employeeList, true); + const details = Object.keys(policy?.employeeList ?? {}).reduce>((acc, email) => { + const employee = policy?.employeeList?.[email]; + const accountID = emailsToAccountIDs[email]; + if (!employee) { + return acc; + } + acc[accountID] = employee; + return acc; + }, {}); + return {policyMemberEmailsToAccountIDs: emailsToAccountIDs, employeeListDetails: details}; + }, [policy?.employeeList]); const currentUserPersonalDetails = useCurrentUserPersonalDetails(); const styles = useThemeStyles(); const [removeMembersConfirmModalVisible, setRemoveMembersConfirmModalVisible] = useState(false); + const [errors, setErrors] = useState({}); const {isOffline} = useNetwork(); const prevIsOffline = usePrevious(isOffline); const accountIDs = useMemo(() => Object.values(policyMemberEmailsToAccountIDs ?? {}).map((accountID) => Number(accountID)), [policyMemberEmailsToAccountIDs]); @@ -94,20 +107,22 @@ function WorkspaceMembersPage({personalDetails, route, policy}: WorkspaceMembers const [isOfflineModalVisible, setIsOfflineModalVisible] = useState(false); const [isDownloadFailureModalVisible, setIsDownloadFailureModalVisible] = useState(false); const isOfflineAndNoMemberDataAvailable = isEmptyObject(policy?.employeeList) && isOffline; + const prevPersonalDetails = usePrevious(personalDetails); const {translate, formatPhoneNumber, localeCompare} = useLocalize(); const {isAccountLocked, showLockedAccountModal} = useContext(LockedAccountContext); const filterEmployees = useCallback( - (employee: PolicyEmployee | undefined) => { + (employee?: PolicyEmployee) => { if (!employee?.email) { return false; } - if (employee.email === policy?.owner || employee.email === currentUserPersonalDetails.login) { + const employeeAccountID = getAccountIDsByLogins([employee.email]).at(0); + if (!employeeAccountID) { return false; } - const isPendingDelete = employee.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE; - return !isPendingDelete; + const isPendingDelete = employeeListDetails?.[employeeAccountID]?.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE; + return accountIDs.includes(employeeAccountID) && !isPendingDelete; }, - [currentUserPersonalDetails.login, policy?.owner], + [accountIDs, employeeListDetails], ); const [selectedEmployees, setSelectedEmployees] = useFilteredSelection(employeeListDetails, filterEmployees); @@ -144,20 +159,29 @@ function WorkspaceMembersPage({personalDetails, route, policy}: WorkspaceMembers const canSelectMultiple = isPolicyAdmin && (shouldUseNarrowLayout ? isMobileSelectionModeEnabled : true); const confirmModalPrompt = useMemo(() => { - const approverEmail = selectedEmployees.find((selectedEmployee) => isApprover(policy, selectedEmployee)); - if (!approverEmail) { - const firstSelectedEmployeeAccountID = policyMemberEmailsToAccountIDs[selectedEmployees[0]]; + const approverAccountID = selectedEmployees.find((selectedEmployee) => isApproverTemp(policy, selectedEmployee)); + if (!approverAccountID) { return translate('workspace.people.removeMembersPrompt', { count: selectedEmployees.length, - memberName: formatPhoneNumber(getPersonalDetailsByIDs({accountIDs: [firstSelectedEmployeeAccountID], currentUserAccountID}).at(0)?.displayName ?? ''), + memberName: formatPhoneNumber(getPersonalDetailsByIDs({accountIDs: selectedEmployees, currentUserAccountID}).at(0)?.displayName ?? ''), }); } - const approverAccountID = policyMemberEmailsToAccountIDs[approverEmail]; return translate('workspace.people.removeMembersWarningPrompt', { memberName: getDisplayNameForParticipant({accountID: approverAccountID}), ownerName: getDisplayNameForParticipant({accountID: policy?.ownerAccountID}), }); - }, [selectedEmployees, policyMemberEmailsToAccountIDs, translate, policy, formatPhoneNumber, currentUserAccountID]); + }, [selectedEmployees, translate, policy, currentUserAccountID, formatPhoneNumber]); + /** + * Get filtered personalDetails list with current employeeList + */ + const filterPersonalDetails = (members: OnyxEntry, details: OnyxEntry): PersonalDetailsList => + Object.keys(members ?? {}).reduce((acc, key) => { + const memberAccountIdKey = policyMemberEmailsToAccountIDs[key] ?? ''; + if (details?.[memberAccountIdKey]) { + acc[memberAccountIdKey] = details[memberAccountIdKey]; + } + return acc; + }, {} as PersonalDetailsList); /** * Get members for the current workspace */ @@ -165,17 +189,51 @@ function WorkspaceMembersPage({personalDetails, route, policy}: WorkspaceMembers openWorkspaceMembersPage(route.params.policyID, Object.keys(getMemberAccountIDsForWorkspace(policy?.employeeList))); }, [route.params.policyID, policy?.employeeList]); + /** + * Check if the current selection includes members that cannot be removed + */ + const validateSelection = useCallback(() => { + const newErrors: Errors = {}; + selectedEmployees.forEach((member) => { + if (member !== policy?.ownerAccountID && member !== session?.accountID) { + return; + } + newErrors[member] = translate('workspace.people.error.cannotRemove'); + }); + setErrors(newErrors); + }, [selectedEmployees, policy?.ownerAccountID, session?.accountID, translate]); + useEffect(() => { getWorkspaceMembers(); }, [getWorkspaceMembers]); useEffect(() => { - if (!removeMembersConfirmModalVisible || deepEqual(accountIDs, prevAccountIDs)) { - return; + validateSelection(); + }, [validateSelection]); + + useEffect(() => { + if (removeMembersConfirmModalVisible && !deepEqual(accountIDs, prevAccountIDs)) { + setRemoveMembersConfirmModalVisible(false); } - setRemoveMembersConfirmModalVisible(false); + setSelectedEmployees((prevSelectedEmployees) => { + // Filter all personal details in order to use the elements needed for the current workspace + const currentPersonalDetails = filterPersonalDetails(policy?.employeeList ?? {}, personalDetails); + // We need to filter the previous selected employees by the new personal details, since unknown/new user id's change when transitioning from offline to online + const prevSelectedElements = prevSelectedEmployees.map((id) => { + const prevItem = prevPersonalDetails?.[id]; + const res = Object.values(currentPersonalDetails).find((item) => prevItem?.login === item?.login); + return res?.accountID ?? id; + }); + + const currentSelectedElements = Object.entries(getMemberAccountIDsForWorkspace(policy?.employeeList)) + .filter((employee) => policy?.employeeList?.[employee[0]]?.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE) + .map((employee) => employee[1]); + + // This is an equivalent of the lodash intersection function. The reduce method below is used to filter the items that exist in both arrays. + return [prevSelectedElements, currentSelectedElements].reduce((prev, members) => prev.filter((item) => members.includes(item))); + }); // eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps - }, [accountIDs]); + }, [policy?.employeeList, policyMemberEmailsToAccountIDs]); useEffect(() => { const isReconnecting = prevIsOffline && !isOffline; @@ -202,13 +260,19 @@ function WorkspaceMembersPage({personalDetails, route, policy}: WorkspaceMembers * Please see https://github.com/Expensify/App/blob/main/README.md#Security for more details */ const removeUsers = () => { - // Check if any of the members are approvers - const hasApprovers = selectedEmployees.some((email) => isApprover(policy, email)); + if (!isEmptyObject(errors)) { + return; + } + + // Remove the admin from the list + const accountIDsToRemove = session?.accountID ? selectedEmployees.filter((id) => id !== session.accountID) : selectedEmployees; + + // Check if any of the account IDs are approvers + const hasApprovers = accountIDsToRemove.some((accountID) => isApproverTemp(policy, accountID)); if (hasApprovers) { const ownerEmail = ownerDetails.login; - selectedEmployees.forEach((login) => { - const accountID = policyMemberEmailsToAccountIDs[login]; + accountIDsToRemove.forEach((accountID) => { const removedApprover = personalDetails?.[accountID]; if (!removedApprover?.login || !ownerEmail) { return; @@ -233,7 +297,7 @@ function WorkspaceMembersPage({personalDetails, route, policy}: WorkspaceMembers // eslint-disable-next-line @typescript-eslint/no-deprecated InteractionManager.runAfterInteractions(() => { setSelectedEmployees([]); - removeMembers(policyID, selectedEmployees, policyMemberEmailsToAccountIDs); + removeMembers(accountIDsToRemove, route.params.policyID); }); }; @@ -241,6 +305,9 @@ function WorkspaceMembersPage({personalDetails, route, policy}: WorkspaceMembers * Show the modal to confirm removal of the selected members */ const askForConfirmationToRemove = () => { + if (!isEmptyObject(errors)) { + return; + } setRemoveMembersConfirmModalVisible(true); }; @@ -249,42 +316,46 @@ function WorkspaceMembersPage({personalDetails, route, policy}: WorkspaceMembers */ const toggleAllUsers = (memberList: MemberOption[]) => { const enabledAccounts = memberList.filter((member) => !member.isDisabled && !member.isDisabledCheckbox); - const someSelected = selectedEmployees.length > 0; + const someSelected = enabledAccounts.some((member) => selectedEmployees.includes(member.accountID)); if (someSelected) { setSelectedEmployees([]); } else { - const everyLogin = enabledAccounts.map((member) => member.login); - setSelectedEmployees(everyLogin); + const everyAccountId = enabledAccounts.map((member) => member.accountID); + setSelectedEmployees(everyAccountId); } + + validateSelection(); }; /** * Add user from the selectedEmployees list */ const addUser = useCallback( - (login: string) => { - setSelectedEmployees((prevSelected) => [...prevSelected, login]); + (accountID: number) => { + setSelectedEmployees((prevSelected) => [...prevSelected, accountID]); + validateSelection(); }, - [setSelectedEmployees], + [validateSelection, setSelectedEmployees], ); /** * Remove user from the selectedEmployees list */ const removeUser = useCallback( - (login: string) => { - setSelectedEmployees((prevSelected) => prevSelected.filter((email) => email !== login)); + (accountID: number) => { + setSelectedEmployees((prevSelected) => prevSelected.filter((id) => id !== accountID)); + validateSelection(); }, - [setSelectedEmployees], + [validateSelection, setSelectedEmployees], ); /** * Toggle user from the selectedEmployees list */ const toggleUser = useCallback( - (login: string, pendingAction?: PendingAction) => { - if (login === policy?.owner && login !== currentUserPersonalDetails.login) { + (accountID: number, pendingAction?: PendingAction) => { + if (accountID === policy?.ownerAccountID && accountID !== session?.accountID) { return; } @@ -293,13 +364,13 @@ function WorkspaceMembersPage({personalDetails, route, policy}: WorkspaceMembers } // Add or remove the user if the checkbox is enabled - if (selectedEmployees.includes(login)) { - removeUser(login); + if (selectedEmployees.includes(accountID)) { + removeUser(accountID); } else { - addUser(login); + addUser(accountID); } }, - [policy?.owner, currentUserPersonalDetails.login, selectedEmployees, removeUser, addUser], + [selectedEmployees, addUser, removeUser, policy?.ownerAccountID, session?.accountID], ); /** Opens the member details page */ @@ -323,9 +394,9 @@ function WorkspaceMembersPage({personalDetails, route, policy}: WorkspaceMembers const dismissError = useCallback( (item: MemberOption) => { if (item.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE) { - clearDeleteMemberError(route.params.policyID, item.login); + clearDeleteMemberError(route.params.policyID, item.accountID); } else { - clearAddMemberError(route.params.policyID, item.login, item.accountID); + clearAddMemberError(route.params.policyID, item.accountID); } }, [route.params.policyID], @@ -362,9 +433,8 @@ function WorkspaceMembersPage({personalDetails, route, policy}: WorkspaceMembers const isPendingDeleteOrError = isPolicyAdmin && (policyEmployee.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE || !isEmptyObject(policyEmployee.errors)); result.push({ - keyForList: details.login ?? '', + keyForList: String(accountID), accountID, - login: details.login ?? '', isDisabledCheckbox: !(isPolicyAdmin && accountID !== policy?.ownerAccountID && accountID !== session?.accountID), isDisabled: isPendingDeleteOrError, isInteractive: !details.isOptimisticPersonalDetail, @@ -423,7 +493,7 @@ function WorkspaceMembersPage({personalDetails, route, policy}: WorkspaceMembers if (isEmptyObject(invitedEmailsToAccountIDsDraft) || accountIDs === prevAccountIDs) { return; } - const invitedEmails = Object.keys(invitedEmailsToAccountIDsDraft); + const invitedEmails = Object.values(invitedEmailsToAccountIDsDraft).map(String); selectionListRef.current?.scrollAndHighlightItem?.(invitedEmails); clearInviteDraft(route.params.policyID); }, [invitedEmailsToAccountIDsDraft, isFocused, accountIDs, prevAccountIDs, route.params.policyID]); @@ -485,14 +555,17 @@ function WorkspaceMembersPage({personalDetails, route, policy}: WorkspaceMembers }; const changeUserRole = (role: ValueOf) => { - const loginsToUpdate = selectedEmployees.filter((login) => { - return policy?.employeeList?.[login]?.role !== role; - }); + if (!isEmptyObject(errors)) { + return; + } - const accountIDsToUpdate = loginsToUpdate.map((login) => policyMemberEmailsToAccountIDs[login]).filter((id) => id !== undefined); + const accountIDsToUpdate = selectedEmployees.filter((accountID) => { + const email = personalDetails?.[accountID]?.login ?? ''; + return policy?.employeeList?.[email]?.role !== role; + }); setSelectedEmployees([]); - updateWorkspaceMembersRole(route.params.policyID, loginsToUpdate, accountIDsToUpdate, role); + updateWorkspaceMembersRole(route.params.policyID, accountIDsToUpdate, role); }; const getBulkActionsButtonOptions = () => { @@ -727,17 +800,17 @@ function WorkspaceMembersPage({personalDetails, route, policy}: WorkspaceMembers ref={selectionListRef} canSelectMultiple={canSelectMultiple} sections={[{data: filteredData, isDisabled: false}]} - selectedItems={selectedEmployees} + selectedItems={selectedEmployees.map(String)} ListItem={TableListItem} shouldUseDefaultRightHandSideCheckmark={false} turnOnSelectionModeOnLongPress={isPolicyAdmin} - onTurnOnSelectionMode={(item) => item && toggleUser(item.login)} + onTurnOnSelectionMode={(item) => item && toggleUser(item?.accountID)} shouldUseUserSkeletonView disableKeyboardShortcuts={removeMembersConfirmModalVisible} headerMessage={shouldUseNarrowLayout ? headerMessage : undefined} onSelectRow={openMemberDetails} shouldSingleExecuteRowSelect={!isPolicyAdmin} - onCheckboxPress={(item) => toggleUser(item.login)} + onCheckboxPress={(item) => toggleUser(item.accountID)} onSelectAll={filteredData.length > 0 ? () => toggleAllUsers(filteredData) : undefined} onDismissError={dismissError} showLoadingPlaceholder={isLoading} diff --git a/src/pages/workspace/members/WorkspaceMemberDetailsPage.tsx b/src/pages/workspace/members/WorkspaceMemberDetailsPage.tsx index 1272b524d09f..9b99ef51149b 100644 --- a/src/pages/workspace/members/WorkspaceMemberDetailsPage.tsx +++ b/src/pages/workspace/members/WorkspaceMemberDetailsPage.tsx @@ -177,15 +177,15 @@ function WorkspaceMemberDetailsPage({personalDetails, policy, route}: WorkspaceM // Function to remove a member and close the modal const removeMemberAndCloseModal = useCallback(() => { - removeMembers(policyID, [memberLogin], {[memberLogin]: accountID}); + removeMembers([accountID], policyID); const previousEmployeesCount = Object.keys(policy?.employeeList ?? {}).length; const remainingEmployeeCount = previousEmployeesCount - 1; if (remainingEmployeeCount === 1 && policy?.preventSelfApproval) { // We can't let the "Prevent Self Approvals" enabled if there's only one workspace user - setPolicyPreventSelfApproval(policyID, false); + setPolicyPreventSelfApproval(route.params.policyID, false); } setIsRemoveMemberConfirmModalVisible(false); - }, [accountID, memberLogin, policy?.employeeList, policy?.preventSelfApproval, policyID]); + }, [accountID, policy?.employeeList, policy?.preventSelfApproval, policyID, route.params.policyID]); const removeUser = useCallback(() => { const ownerEmail = ownerDetails?.login; @@ -264,9 +264,9 @@ function WorkspaceMemberDetailsPage({personalDetails, policy, route}: WorkspaceM const changeRole = useCallback( ({value}: ListItemType) => { setIsRoleSelectionModalVisible(false); - updateWorkspaceMembersRole(policyID, [memberLogin], [accountID], value); + updateWorkspaceMembersRole(policyID, [accountID], value); }, - [accountID, memberLogin, policyID], + [accountID, policyID], ); const startChangeOwnershipFlow = useCallback(() => { diff --git a/tests/actions/PolicyMemberTest.ts b/tests/actions/PolicyMemberTest.ts index 269cf78f2308..6b1183ab9723 100644 --- a/tests/actions/PolicyMemberTest.ts +++ b/tests/actions/PolicyMemberTest.ts @@ -109,7 +109,7 @@ describe('actions/PolicyMember', () => { Onyx.set(`${ONYXKEYS.PERSONAL_DETAILS_LIST}`, {[fakeUser2.accountID]: fakeUser2}); await waitForBatchedUpdates(); // When a user's role is set as admin on a policy - Member.updateWorkspaceMembersRole(fakePolicy.id, [fakeUser2.login ?? ''], [fakeUser2.accountID], CONST.POLICY.ROLE.ADMIN); + Member.updateWorkspaceMembersRole(fakePolicy.id, [fakeUser2.accountID], CONST.POLICY.ROLE.ADMIN); await waitForBatchedUpdates(); await new Promise((resolve) => { const connection = Onyx.connect({ @@ -152,7 +152,7 @@ describe('actions/PolicyMember', () => { }); await waitForBatchedUpdates(); // When an admin is demoted from their admin role to a user role - Member.updateWorkspaceMembersRole(fakePolicy.id, [fakeUser2.login ?? ''], [fakeUser2.accountID], CONST.POLICY.ROLE.USER); + Member.updateWorkspaceMembersRole(fakePolicy.id, [fakeUser2.accountID], CONST.POLICY.ROLE.USER); await waitForBatchedUpdates(); await new Promise((resolve) => { const connection = Onyx.connect({ @@ -458,12 +458,7 @@ describe('actions/PolicyMember', () => { // When removing am admin, auditor, and user members mockFetch?.pause?.(); - const memberEmailsToAccountIDs = { - [adminEmail]: adminAccountID, - [auditorEmail]: auditorAccountID, - [userEmail]: userAccountID, - }; - Member.removeMembers(policyID, [adminEmail, auditorEmail, userEmail], memberEmailsToAccountIDs); + Member.removeMembers([adminAccountID, auditorAccountID, userAccountID], policyID); await waitForBatchedUpdates(); @@ -504,7 +499,6 @@ describe('actions/PolicyMember', () => { const workspaceReportID = '1'; const expenseReportID = '2'; const userAccountID = 1236; - const userEmail = 'user@example.com'; await Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${workspaceReportID}`, { ...createRandomReport(Number(workspaceReportID)), @@ -523,7 +517,7 @@ describe('actions/PolicyMember', () => { // When removing a member from the workspace mockFetch?.pause?.(); - Member.removeMembers(policyID, [userEmail], {[userEmail]: userAccountID}); + Member.removeMembers([userAccountID], policyID); await waitForBatchedUpdates();