diff --git a/src/languages/en.ts b/src/languages/en.ts index e6fbe9f5b5a..8ccb40d3d05 100755 --- a/src/languages/en.ts +++ b/src/languages/en.ts @@ -1342,8 +1342,7 @@ export default { /* eslint-enable @typescript-eslint/naming-convention */ }, }, - approverInMultipleWorkflows: ({name1, name2}: ApprovalWorkflowErrorParams) => - `${name1} already approves reports to ${name2} in a separate workflow. If you change this approval relationship, all other workflows will be updated.`, + approverInMultipleWorkflows: 'This member already belongs to another approval workflow. Any updates here will reflect there too.', approverCircularReference: ({name1, name2}: ApprovalWorkflowErrorParams) => `${name1} already approves reports to ${name2}. Please choose a different approver to avoid a circular workflow.`, }, diff --git a/src/languages/es.ts b/src/languages/es.ts index 560f73c4d18..3e86062ba86 100644 --- a/src/languages/es.ts +++ b/src/languages/es.ts @@ -1350,8 +1350,7 @@ export default { /* eslint-enable @typescript-eslint/naming-convention */ }, }, - approverInMultipleWorkflows: ({name1, name2}: ApprovalWorkflowErrorParams) => - `${name1} ya aprueba informes a ${name2} en otro flujo de trabajo Si modificas esta relación de aprobación, se actualizarán todos los demás flujos de trabajo.`, + approverInMultipleWorkflows: 'Este miembro ya pertenece a otro flujo de aprobación. Cualquier actualización aquí se reflejará allí también.', approverCircularReference: ({name1, name2}: ApprovalWorkflowErrorParams) => `${name1} ya aprueba informes a ${name2}. Por favor, elige un aprobador diferente para evitar un flujo de trabajo circular.`, }, diff --git a/src/libs/WorkflowUtils.ts b/src/libs/WorkflowUtils.ts index d9a3782987f..88e420dfaf7 100644 --- a/src/libs/WorkflowUtils.ts +++ b/src/libs/WorkflowUtils.ts @@ -10,6 +10,7 @@ const INITIAL_APPROVAL_WORKFLOW: ApprovalWorkflowOnyx = { members: [], approvers: [], availableMembers: [], + usedApproverEmails: [], isDefault: false, action: CONST.APPROVAL_WORKFLOW.ACTION.CREATE, isLoading: false, @@ -50,7 +51,6 @@ function calculateApprovers({employees, firstEmail, personalDetailsByEmail}: Get forwardsTo: employees[nextEmail].forwardsTo, avatar: personalDetailsByEmail[nextEmail]?.avatar, displayName: personalDetailsByEmail[nextEmail]?.displayName ?? nextEmail, - isInMultipleWorkflows: false, isCircularReference, }); @@ -67,7 +67,7 @@ function calculateApprovers({employees, firstEmail, personalDetailsByEmail}: Get return approvers; } -type ConvertPolicyEmployeesToApprovalWorkflowsParams = { +type PolicyConversionParams = { /** * List of employees in the policy */ @@ -82,14 +82,36 @@ type ConvertPolicyEmployeesToApprovalWorkflowsParams = { * Email of the default approver for the policy */ defaultApprover: string; + + /** + * Email of the first approver in current edited workflow + */ + firstApprover?: string; +}; + +type PolicyConversionResult = { + /** + * List of approval workflows + */ + approvalWorkflows: ApprovalWorkflow[]; + + /** + * List of available members that can be selected in the workflow + */ + availableMembers: Member[]; + + /** + * Emails that are used as approvers in currently configured workflows + */ + usedApproverEmails: string[]; }; /** Convert a list of policy employees to a list of approval workflows */ -function convertPolicyEmployeesToApprovalWorkflows({employees, defaultApprover, personalDetails}: ConvertPolicyEmployeesToApprovalWorkflowsParams): ApprovalWorkflow[] { +function convertPolicyEmployeesToApprovalWorkflows({employees, defaultApprover, personalDetails, firstApprover}: PolicyConversionParams): PolicyConversionResult { const approvalWorkflows: Record = {}; - // Keep track of how many times each approver is used to detect approvers in multiple workflows - const approverCountsByEmail: Record = {}; + // Keep track of used approver emails to display hints in the UI + const usedApproverEmails = new Set(); const personalDetailsByEmail = lodashMapKeys(personalDetails, (value, key) => value?.login ?? key); // Add each employee to the appropriate workflow @@ -102,7 +124,9 @@ function convertPolicyEmployeesToApprovalWorkflows({employees, defaultApprover, const member: Member = {email, avatar: personalDetailsByEmail[email]?.avatar, displayName: personalDetailsByEmail[email]?.displayName ?? email}; if (!approvalWorkflows[submitsTo]) { const approvers = calculateApprovers({employees, firstEmail: submitsTo, personalDetailsByEmail}); - approvers.forEach((approver) => (approverCountsByEmail[approver.email] = (approverCountsByEmail[approver.email] ?? 0) + 1)); + if (submitsTo !== firstApprover) { + approvers.forEach((approver) => usedApproverEmails.add(approver.email)); + } approvalWorkflows[submitsTo] = { members: [], @@ -136,14 +160,7 @@ function convertPolicyEmployeesToApprovalWorkflows({employees, defaultApprover, }); } - // Add a flag to each approver to indicate if they are in multiple workflows - return sortedApprovalWorkflows.map((workflow) => ({ - ...workflow, - approvers: workflow.approvers.map((approver) => ({ - ...approver, - isInMultipleWorkflows: approverCountsByEmail[approver.email] > 1, - })), - })); + return {approvalWorkflows: sortedApprovalWorkflows, usedApproverEmails: [...usedApproverEmails], availableMembers: sortedApprovalWorkflows.at(0)?.members ?? []}; } type ConvertApprovalWorkflowToPolicyEmployeesParams = { diff --git a/src/libs/actions/Workflow.ts b/src/libs/actions/Workflow.ts index ec1b9c08025..946d7682675 100644 --- a/src/libs/actions/Workflow.ts +++ b/src/libs/actions/Workflow.ts @@ -1,6 +1,6 @@ import lodashDropRightWhile from 'lodash/dropRightWhile'; import lodashMapKeys from 'lodash/mapKeys'; -import type {OnyxCollection, OnyxMergeInput, OnyxUpdate} from 'react-native-onyx'; +import type {NullishDeep, OnyxCollection, OnyxUpdate} from 'react-native-onyx'; import Onyx from 'react-native-onyx'; import * as API from '@libs/API'; import type {CreateWorkspaceApprovalParams, RemoveWorkspaceApprovalParams, UpdateWorkspaceApprovalParams} from '@libs/API/parameters'; @@ -282,10 +282,7 @@ function setApprovalWorkflowApprover(approver: Approver, approverIndex: number, // Check if the approver forwards to other approvers and add them to the list if (policy.employeeList[approver.email]?.forwardsTo) { const personalDetailsByEmail = lodashMapKeys(personalDetails, (value, key) => value?.login ?? key); - const additionalApprovers = calculateApprovers({employees: policy.employeeList, firstEmail: approver.email, personalDetailsByEmail}).map((additionalApprover) => ({ - ...additionalApprover, - isInMultipleWorkflows: true, - })); + const additionalApprovers = calculateApprovers({employees: policy.employeeList, firstEmail: approver.email, personalDetailsByEmail}); approvers.splice(approverIndex, approvers.length, ...additionalApprovers); } @@ -327,7 +324,7 @@ function clearApprovalWorkflowApprovers() { Onyx.merge(ONYXKEYS.APPROVAL_WORKFLOW, {approvers: []}); } -function setApprovalWorkflow(approvalWorkflow: OnyxMergeInput) { +function setApprovalWorkflow(approvalWorkflow: NullishDeep) { Onyx.merge(ONYXKEYS.APPROVAL_WORKFLOW, approvalWorkflow); } diff --git a/src/pages/workspace/workflows/WorkspaceWorkflowsPage.tsx b/src/pages/workspace/workflows/WorkspaceWorkflowsPage.tsx index 23f341d308e..bd0f2775a88 100644 --- a/src/pages/workspace/workflows/WorkspaceWorkflowsPage.tsx +++ b/src/pages/workspace/workflows/WorkspaceWorkflowsPage.tsx @@ -62,7 +62,7 @@ function WorkspaceWorkflowsPage({policy, betas, route}: WorkspaceWorkflowsPagePr const [isCurrencyModalOpen, setIsCurrencyModalOpen] = useState(false); const [personalDetails] = useOnyx(ONYXKEYS.PERSONAL_DETAILS_LIST); const policyApproverName = useMemo(() => PersonalDetailsUtils.getPersonalDetailByEmail(policyApproverEmail ?? '')?.displayName ?? policyApproverEmail, [policyApproverEmail]); - const approvalWorkflows = useMemo( + const {approvalWorkflows, availableMembers, usedApproverEmails} = useMemo( () => convertPolicyEmployeesToApprovalWorkflows({ employees: policy?.employeeList ?? {}, @@ -111,10 +111,11 @@ function WorkspaceWorkflowsPage({policy, betas, route}: WorkspaceWorkflowsPagePr Workflow.setApprovalWorkflow({ ...INITIAL_APPROVAL_WORKFLOW, - availableMembers: approvalWorkflows.at(0)?.members ?? [], + availableMembers, + usedApproverEmails, }); Navigation.navigate(ROUTES.WORKSPACE_WORKFLOWS_APPROVALS_EXPENSES_FROM.getRoute(route.params.policyID)); - }, [approvalWorkflows, policy, route.params.policyID]); + }, [policy, route.params.policyID, availableMembers, usedApproverEmails]); const optionItems: ToggleSettingOptionRowProps[] = useMemo(() => { const {accountNumber, addressName, bankName, bankAccountID} = policy?.achAccount ?? {}; diff --git a/src/pages/workspace/workflows/approvals/ApprovalWorkflowEditor.tsx b/src/pages/workspace/workflows/approvals/ApprovalWorkflowEditor.tsx index cbbce324f64..a07aa150d0e 100644 --- a/src/pages/workspace/workflows/approvals/ApprovalWorkflowEditor.tsx +++ b/src/pages/workspace/workflows/approvals/ApprovalWorkflowEditor.tsx @@ -79,19 +79,6 @@ function ApprovalWorkflowEditor({approvalWorkflow, removeApprovalWorkflow, polic [approvalWorkflow.approvers, approvalWorkflow.errors, translate], ); - const approverHintMessage = useCallback( - (approver: Approver | undefined, approverIndex: number) => { - const previousApprover = approvalWorkflow.approvers.slice(0, approverIndex).filter(Boolean).at(-1); - if (approver?.isInMultipleWorkflows && approver.email === previousApprover?.forwardsTo) { - return translate('workflowsPage.approverInMultipleWorkflows', { - name1: approver.displayName, - name2: previousApprover.displayName, - }); - } - }, - [approvalWorkflow.approvers, translate], - ); - const editMembers = useCallback(() => { const backTo = approvalWorkflow.action === CONST.APPROVAL_WORKFLOW.ACTION.CREATE ? ROUTES.WORKSPACE_WORKFLOWS_APPROVALS_NEW.getRoute(policyID) : undefined; Navigation.navigate(ROUTES.WORKSPACE_WORKFLOWS_APPROVALS_EXPENSES_FROM.getRoute(policyID, backTo), CONST.NAVIGATION.ACTION_TYPE.PUSH); @@ -143,7 +130,11 @@ function ApprovalWorkflowEditor({approvalWorkflow, removeApprovalWorkflow, polic {approvalWorkflow.approvers.map((approver, approverIndex) => { const errorText = approverErrorMessage(approver, approverIndex); - const hintText = !errorText && approverHintMessage(approver, approverIndex); + const hintText = + !errorText && approvalWorkflow.usedApproverEmails.some((approverEmail) => approverEmail === approver?.email) + ? translate('workflowsPage.approverInMultipleWorkflows') + : undefined; + return ( { + const {currentApprovalWorkflow, defaultWorkflowMembers, usedApproverEmails} = useMemo(() => { if (!policy || !personalDetails) { return {}; } const defaultApprover = policy?.approver ?? policy.owner; - const allApprovalWorkflows = convertPolicyEmployeesToApprovalWorkflows({ + const firstApprover = route.params.firstApproverEmail; + const result = convertPolicyEmployeesToApprovalWorkflows({ employees: policy.employeeList ?? {}, defaultApprover, personalDetails, + firstApprover, }); return { - defaultWorkflowMembers: allApprovalWorkflows.at(0)?.members ?? [], - currentApprovalWorkflow: allApprovalWorkflows.find((workflow) => workflow.approvers.at(0)?.email === route.params.firstApproverEmail), + defaultWorkflowMembers: result.availableMembers, + usedApproverEmails: result.usedApproverEmails, + currentApprovalWorkflow: result.approvalWorkflows.find((workflow) => workflow.approvers.at(0)?.email === firstApprover), }; }, [personalDetails, policy, route.params.firstApproverEmail]); @@ -97,12 +100,13 @@ function WorkspaceWorkflowsApprovalsEditPage({policy, isLoadingReportData = true Workflow.setApprovalWorkflow({ ...currentApprovalWorkflow, availableMembers: [...currentApprovalWorkflow.members, ...defaultWorkflowMembers], + usedApproverEmails, action: CONST.APPROVAL_WORKFLOW.ACTION.EDIT, isLoading: false, errors: null, }); setInitialApprovalWorkflow(currentApprovalWorkflow); - }, [currentApprovalWorkflow, defaultWorkflowMembers, initialApprovalWorkflow]); + }, [currentApprovalWorkflow, defaultWorkflowMembers, initialApprovalWorkflow, usedApproverEmails]); return ( & { */ availableMembers: Member[]; + /** + * List of emails that are already in use in other workflows + */ + usedApproverEmails: string[]; + /** * Errors for the workflow */ diff --git a/tests/unit/WorkflowUtilsTest.ts b/tests/unit/WorkflowUtilsTest.ts index cc5649a24fc..8ef2b873bd8 100644 --- a/tests/unit/WorkflowUtilsTest.ts +++ b/tests/unit/WorkflowUtilsTest.ts @@ -31,7 +31,6 @@ function buildApprover(accountID: number, approver: Partial = {}): App forwardsTo: undefined, avatar: 'https://d2k5nsl2zxldvw.cloudfront.net/images/avatars/avatar_7.png', displayName: `${accountID}@example.com User`, - isInMultipleWorkflows: false, isCircularReference: false, ...approver, }; @@ -201,9 +200,9 @@ describe('WorkflowUtils', () => { const employees: PolicyEmployeeList = {}; const defaultApprover = '1@example.com'; - const workflows = WorkflowUtils.convertPolicyEmployeesToApprovalWorkflows({employees, defaultApprover, personalDetails}); + const {approvalWorkflows} = WorkflowUtils.convertPolicyEmployeesToApprovalWorkflows({employees, defaultApprover, personalDetails}); - expect(workflows).toEqual([]); + expect(approvalWorkflows).toEqual([]); }); it('Should transform all users into one default workflow', () => { @@ -221,9 +220,9 @@ describe('WorkflowUtils', () => { }; const defaultApprover = '1@example.com'; - const workflows = WorkflowUtils.convertPolicyEmployeesToApprovalWorkflows({employees, defaultApprover, personalDetails}); + const {approvalWorkflows} = WorkflowUtils.convertPolicyEmployeesToApprovalWorkflows({employees, defaultApprover, personalDetails}); - expect(workflows).toEqual([buildWorkflow([1, 2], [1], {isDefault: true})]); + expect(approvalWorkflows).toEqual([buildWorkflow([1, 2], [1], {isDefault: true})]); }); it('Should transform all users into two workflows', () => { @@ -251,9 +250,9 @@ describe('WorkflowUtils', () => { }; const defaultApprover = '1@example.com'; - const workflows = WorkflowUtils.convertPolicyEmployeesToApprovalWorkflows({employees, defaultApprover, personalDetails}); + const {approvalWorkflows} = WorkflowUtils.convertPolicyEmployeesToApprovalWorkflows({employees, defaultApprover, personalDetails}); - expect(workflows).toEqual([buildWorkflow([2, 3], [1], {isDefault: true}), buildWorkflow([1, 4], [4])]); + expect(approvalWorkflows).toEqual([buildWorkflow([2, 3], [1], {isDefault: true}), buildWorkflow([1, 4], [4])]); }); it('Should sort the workflows (first the default and then based on the first approver display name)', () => { @@ -286,9 +285,9 @@ describe('WorkflowUtils', () => { }; const defaultApprover = '1@example.com'; - const workflows = WorkflowUtils.convertPolicyEmployeesToApprovalWorkflows({employees, defaultApprover, personalDetails}); + const {approvalWorkflows} = WorkflowUtils.convertPolicyEmployeesToApprovalWorkflows({employees, defaultApprover, personalDetails}); - expect(workflows).toEqual([buildWorkflow([3, 2], [1], {isDefault: true}), buildWorkflow([5], [3]), buildWorkflow([4, 1], [4])]); + expect(approvalWorkflows).toEqual([buildWorkflow([3, 2], [1], {isDefault: true}), buildWorkflow([5], [3]), buildWorkflow([4, 1], [4])]); }); it('Should mark approvers that are used in multiple workflows', () => { @@ -316,20 +315,16 @@ describe('WorkflowUtils', () => { }; const defaultApprover = '1@example.com'; - const workflows = WorkflowUtils.convertPolicyEmployeesToApprovalWorkflows({employees, defaultApprover, personalDetails}); + const {approvalWorkflows} = WorkflowUtils.convertPolicyEmployeesToApprovalWorkflows({employees, defaultApprover, personalDetails}); const defaultWorkflow = buildWorkflow([2, 3, 4], [1, 3, 4], {isDefault: true}); defaultWorkflow.approvers[0].forwardsTo = '3@example.com'; defaultWorkflow.approvers[1].forwardsTo = '4@example.com'; - defaultWorkflow.approvers[1].isInMultipleWorkflows = true; - defaultWorkflow.approvers[2].isInMultipleWorkflows = true; const secondWorkflow = buildWorkflow([1], [2, 3, 4]); secondWorkflow.approvers[0].forwardsTo = '3@example.com'; secondWorkflow.approvers[1].forwardsTo = '4@example.com'; - secondWorkflow.approvers[1].isInMultipleWorkflows = true; - secondWorkflow.approvers[2].isInMultipleWorkflows = true; - expect(workflows).toEqual([defaultWorkflow, secondWorkflow]); + expect(approvalWorkflows).toEqual([defaultWorkflow, secondWorkflow]); }); it('Should build multiple workflows with many approvers', () => { @@ -367,13 +362,13 @@ describe('WorkflowUtils', () => { }; const defaultApprover = '1@example.com'; - const workflows = WorkflowUtils.convertPolicyEmployeesToApprovalWorkflows({employees, defaultApprover, personalDetails}); + const {approvalWorkflows} = WorkflowUtils.convertPolicyEmployeesToApprovalWorkflows({employees, defaultApprover, personalDetails}); const defaultWorkflow = buildWorkflow([1, 4, 5, 6], [1], {isDefault: true}); const secondWorkflow = buildWorkflow([2, 3], [4, 5, 6]); secondWorkflow.approvers[0].forwardsTo = '5@example.com'; secondWorkflow.approvers[1].forwardsTo = '6@example.com'; - expect(workflows).toEqual([defaultWorkflow, secondWorkflow]); + expect(approvalWorkflows).toEqual([defaultWorkflow, secondWorkflow]); }); });