Skip to content

Revert "Refactored usage of ONYXKEYS.PERSONAL_DETAILS_LIST from Member Actions" #73458

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

Merged
tgolen merged 1 commit into from
Oct 24, 2025
Merged

Revert "Refactored usage of ONYXKEYS.PERSONAL_DETAILS_LIST from Member Actions" #73458

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion src/libs/actions/IOU.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -695,7 +695,7 @@
};

let allPersonalDetails: OnyxTypes.PersonalDetailsList = {};
Onyx.connect({

Check warning on line 698 in src/libs/actions/IOU.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check 

Onyx.connect() is deprecated. Use useOnyx() hook instead and pass the data as parameters to a pure function
key: ONYXKEYS.PERSONAL_DETAILS_LIST,
callback: (value) => {
allPersonalDetails = value ?? {};
Expand Down Expand Up @@ -772,13 +772,13 @@
};

let allBetas: OnyxEntry<OnyxTypes.Beta[]>;
Onyx.connect({

Check warning on line 775 in src/libs/actions/IOU.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check 

Onyx.connect() is deprecated. Use useOnyx() hook instead and pass the data as parameters to a pure function
key: ONYXKEYS.BETAS,
callback: (value) => (allBetas = value),
});

let allTransactions: NonNullable<OnyxCollection<OnyxTypes.Transaction>> = {};
Onyx.connect({

Check warning on line 781 in src/libs/actions/IOU.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check 

Onyx.connect() is deprecated. Use useOnyx() hook instead and pass the data as parameters to a pure function
key: ONYXKEYS.COLLECTION.TRANSACTION,
waitForCollectionCallback: true,
callback: (value) => {
Expand All @@ -792,7 +792,7 @@
});

let allTransactionDrafts: NonNullable<OnyxCollection<OnyxTypes.Transaction>> = {};
Onyx.connect({

Check warning on line 795 in src/libs/actions/IOU.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check 

Onyx.connect() is deprecated. Use useOnyx() hook instead and pass the data as parameters to a pure function
key: ONYXKEYS.COLLECTION.TRANSACTION_DRAFT,
waitForCollectionCallback: true,
callback: (value) => {
Expand All @@ -801,7 +801,7 @@
});

let allTransactionViolations: NonNullable<OnyxCollection<OnyxTypes.TransactionViolations>> = {};
Onyx.connect({

Check warning on line 804 in src/libs/actions/IOU.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check 

Onyx.connect() is deprecated. Use useOnyx() hook instead and pass the data as parameters to a pure function
key: ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS,
waitForCollectionCallback: true,
callback: (value) => {
Expand All @@ -815,7 +815,7 @@
});

let allNextSteps: NonNullable<OnyxCollection<OnyxTypes.ReportNextStep>> = {};
Onyx.connect({

Check warning on line 818 in src/libs/actions/IOU.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check 

Onyx.connect() is deprecated. Use useOnyx() hook instead and pass the data as parameters to a pure function
key: ONYXKEYS.COLLECTION.NEXT_STEP,
waitForCollectionCallback: true,
callback: (value) => {
Expand All @@ -824,14 +824,14 @@
});

let allPolicyCategories: OnyxCollection<OnyxTypes.PolicyCategories> = {};
Onyx.connect({

Check warning on line 827 in src/libs/actions/IOU.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check 

Onyx.connect() is deprecated. Use useOnyx() hook instead and pass the data as parameters to a pure function
key: ONYXKEYS.COLLECTION.POLICY_CATEGORIES,
waitForCollectionCallback: true,
callback: (val) => (allPolicyCategories = val),
});

const allPolicies: OnyxCollection<OnyxTypes.Policy> = {};
Onyx.connect({

Check warning on line 834 in src/libs/actions/IOU.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check 

Onyx.connect() is deprecated. Use useOnyx() hook instead and pass the data as parameters to a pure function
key: ONYXKEYS.COLLECTION.POLICY,
callback: (val, key) => {
if (!key) {
Expand Down Expand Up @@ -867,14 +867,14 @@
// `allRecentlyUsedTags` was moved here temporarily from `src/libs/actions/Policy/Tag.ts` during the `Deprecate Onyx.connect` refactor.
// All uses of this variable should be replaced with `useOnyx`.
let allRecentlyUsedTags: OnyxCollection<RecentlyUsedTags> = {};
Onyx.connect({

Check warning on line 870 in src/libs/actions/IOU.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check 

Onyx.connect() is deprecated. Use useOnyx() hook instead and pass the data as parameters to a pure function
key: ONYXKEYS.COLLECTION.POLICY_RECENTLY_USED_TAGS,
waitForCollectionCallback: true,
callback: (val) => (allRecentlyUsedTags = val),
});

let allReports: OnyxCollection<OnyxTypes.Report>;
Onyx.connect({

Check warning on line 877 in src/libs/actions/IOU.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check 

Onyx.connect() is deprecated. Use useOnyx() hook instead and pass the data as parameters to a pure function
key: ONYXKEYS.COLLECTION.REPORT,
waitForCollectionCallback: true,
callback: (value) => {
Expand Down Expand Up @@ -5809,7 +5809,7 @@
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);
Expand Down
101 changes: 67 additions & 34 deletions src/libs/actions/Policy/Member.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -45,6 +55,7 @@ type OnyxDataReturnType = {
};

type WorkspaceMembersRoleData = {
accountID: number;
email: string;
role: ValueOf<typeof CONST.POLICY.ROLE>;
};
Expand Down Expand Up @@ -99,6 +110,12 @@ Onyx.connect({
},
});

let allPersonalDetails: OnyxEntry<PersonalDetailsList>;

@github-actions github-actions Bot Oct 24, 2025

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.

❌ PERF-6 (docs)

Using entire allPersonalDetails object as dependency causes useCallback to re-execute whenever any personal detail changes, even unrelated ones.

Consider specifying individual properties or the specific accountID lookup instead:

const isApproverTemp = useCallback((policy: OnyxEntry<Policy>, employeeAccountID: number) => {
    const employeeLogin = allPersonalDetails?.[employeeAccountID]?.login;
    return isApprover(policy, employeeLogin ?? '');
}, [allPersonalDetails?.[employeeAccountID]?.login]); // More specific dependency

Onyx.connect({
key: ONYXKEYS.PERSONAL_DETAILS_LIST,
callback: (val) => (allPersonalDetails = val),
});

let policyOwnershipChecks: Record<string, PolicyOwnershipChangeChecks>;
Onyx.connect({
key: ONYXKEYS.POLICY_OWNERSHIP_CHANGE_CHECKS,
Expand All @@ -117,6 +134,12 @@ function isApprover(policy: OnyxEntry<Policy>, employeeLogin: string) {
);
}

/** Temporary function alias for isApprover with employeeAccountID */

@github-actions github-actions Bot Oct 24, 2025

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.

❌ PERF-6 (docs)

This temporary function isApproverTemp directly accesses allPersonalDetails without proper memoization or dependency tracking. This causes performance issues when used in array operations or frequent function calls.

Consider creating a more optimized version or memoizing the login lookup:

const getEmployeeLogin = useMemo(() => 
    (employeeAccountID: number) => allPersonalDetails?.[employeeAccountID]?.login,
    [allPersonalDetails]
);

function isApproverTemp(policy: OnyxEntry<Policy>, employeeAccountID: number) {
    const employeeLogin = getEmployeeLogin(employeeAccountID);
    return isApprover(policy, employeeLogin ?? '');
}

function isApproverTemp(policy: OnyxEntry<Policy>, 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
Expand Down Expand Up @@ -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<string, number>) {
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),
);
Expand All @@ -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<PolicyEmployee> = {};
const successMembersState: OnyxCollectionInputValue<PolicyEmployee> = {};
const failureMembersState: OnyxCollectionInputValue<PolicyEmployee> = {};
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')};
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -488,15 +511,15 @@ 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[] = [
{
onyxMethod: Onyx.METHOD.MERGE,
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,
Expand Down Expand Up @@ -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<string, string> = {};

if (policy?.primaryLoginsInvited) {
Expand Down Expand Up @@ -652,21 +675,28 @@ function removeMembers(policyID: string, selectedMemberEmails: string[], policyM
});

@github-actions github-actions Bot Oct 24, 2025

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.

❌ PERF-6 (docs)

The accountIDs.reduce() function accesses allPersonalDetails?.[accountID]?.login for each account ID, but the dependency tracking is not optimized. This causes the function to re-execute whenever any personal detail changes.

Consider memoizing the specific personal details needed or restructuring to avoid accessing the entire allPersonalDetails object:

const memberEmails = useMemo(() => 
    accountIDs.map(accountID => allPersonalDetails?.[accountID]?.login).filter(Boolean),
    accountIDs.map(id => allPersonalDetails?.[id]?.login)
);

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<typeof CONST.POLICY.ROLE>) {
function buildUpdateWorkspaceMembersRoleOnyxData(policyID: string, accountIDs: number[], newRole: ValueOf<typeof CONST.POLICY.ROLE>) {
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[] = [
{
Expand Down Expand Up @@ -728,15 +758,15 @@ function buildUpdateWorkspaceMembersRoleOnyxData(policyID: string, selectedMembe
const failureDataParticipants: Record<number, Participant | null> = {...adminRoom.participants};
const optimisticParticipants: Record<number, Participant | null> = {};
if (newRole === CONST.POLICY.ROLE.ADMIN || newRole === CONST.POLICY.ROLE.AUDITOR) {
selectedMemberAccountIDs.forEach((accountID) => {
accountIDs.forEach((accountID) => {
if (adminRoom?.participants?.[accountID]) {
return;
}
optimisticParticipants[accountID] = {notificationPreference: CONST.REPORT.NOTIFICATION_PREFERENCE.ALWAYS};
failureDataParticipants[accountID] = null;
});
} else {
selectedMemberAccountIDs.forEach((accountID) => {
accountIDs.forEach((accountID) => {
if (!adminRoom?.participants?.[accountID]) {
return;
}
Expand Down Expand Up @@ -764,8 +794,8 @@ function buildUpdateWorkspaceMembersRoleOnyxData(policyID: string, selectedMembe
return {optimisticData, successData, failureData, memberRoles};
}

function updateWorkspaceMembersRole(policyID: string, selectedMemberEmails: string[], selectedMemberAccountIDs: number[], newRole: ValueOf<typeof CONST.POLICY.ROLE>) {
const {optimisticData, successData, failureData, memberRoles} = buildUpdateWorkspaceMembersRoleOnyxData(policyID, selectedMemberEmails, selectedMemberAccountIDs, newRole);
function updateWorkspaceMembersRole(policyID: string, accountIDs: number[], newRole: ValueOf<typeof CONST.POLICY.ROLE>) {
const {optimisticData, successData, failureData, memberRoles} = buildUpdateWorkspaceMembersRoleOnyxData(policyID, accountIDs, newRole);

const params: UpdateWorkspaceMembersRoleParams = {
policyID,
Expand Down Expand Up @@ -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,
},
Expand All @@ -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}`, {
Expand Down Expand Up @@ -1362,4 +1394,5 @@ export {
clearWorkspaceInviteRoleDraft,
setImportedSpreadsheetMemberData,
clearImportedSpreadsheetMemberData,
isApproverTemp,
};
Loading
Loading