Merge pull request #47860 from software-mansion-labs/approval-workflo…

…ws/simplify-hint-message [Advanced Approval Workflows] Update the hint text to be simpler
Expensify · Aug 26, 2024 · 19fd9a5 · 19fd9a5
2 parents fb73863 + 2d5e45e
commit 19fd9a5
Show file tree

Hide file tree

Showing 9 changed files with 71 additions and 68 deletions.
diff --git 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) =>
-            `<strong>${name1}</strong> already approves reports to <strong>${name2}</strong> 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) =>
             `<strong>${name1}</strong> already approves reports to <strong>${name2}</strong>. Please choose a different approver to avoid a circular workflow.`,
     },

diff --git 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) =>
-            `<strong>${name1}</strong> ya aprueba informes a <strong>${name2}</strong> 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) =>
             `<strong>${name1}</strong> ya aprueba informes a <strong>${name2}</strong>. 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
@@ -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<string, ApprovalWorkflow> = {};
 
-    // Keep track of how many times each approver is used to detect approvers in multiple workflows
-    const approverCountsByEmail: Record<string, number> = {};
+    // Keep track of used approver emails to display hints in the UI
+    const usedApproverEmails = new Set<string>();
     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
@@ -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<typeof ONYXKEYS.APPROVAL_WORKFLOW>) {
+function setApprovalWorkflow(approvalWorkflow: NullishDeep<ApprovalWorkflowOnyx>) {
     Onyx.merge(ONYXKEYS.APPROVAL_WORKFLOW, approvalWorkflow);
 }
 

diff --git 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
@@ -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 (
                         <MenuItemWithTopDescription
                             // eslint-disable-next-line react/no-array-index-key

diff --git a/src/pages/workspace/workflows/approvals/WorkspaceWorkflowsApprovalsEditPage.tsx b/src/pages/workspace/workflows/approvals/WorkspaceWorkflowsApprovalsEditPage.tsx
@@ -62,21 +62,24 @@ function WorkspaceWorkflowsApprovalsEditPage({policy, isLoadingReportData = true
         Navigation.dismissModal();
     }, [initialApprovalWorkflow, route.params.policyID]);
 
-    const {currentApprovalWorkflow, defaultWorkflowMembers} = useMemo(() => {
+    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 (
         <AccessOrNotFoundWrapper

diff --git a/src/types/onyx/ApprovalWorkflow.ts b/src/types/onyx/ApprovalWorkflow.ts
@@ -27,11 +27,6 @@ type Approver = {
      */
     displayName: string;
 
-    /**
-     * Is this user used as an approver in more than one workflow (used to show a warning)
-     */
-    isInMultipleWorkflows?: boolean;
-
     /**
      * Is this approver in a circular reference (approver forwards to themselves, or a cycle of forwards)
      *
@@ -109,6 +104,11 @@ type ApprovalWorkflowOnyx = Omit<ApprovalWorkflow, 'approvers'> & {
      */
     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
@@ -31,7 +31,6 @@ function buildApprover(accountID: number, approver: Partial<Approver> = {}): 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 = '[email protected]';
 
-            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 = '[email protected]';
 
-            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 = '[email protected]';
 
-            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 = '[email protected]';
 
-            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 = '[email protected]';
 
-            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 = '[email protected]';
             defaultWorkflow.approvers[1].forwardsTo = '[email protected]';
-            defaultWorkflow.approvers[1].isInMultipleWorkflows = true;
-            defaultWorkflow.approvers[2].isInMultipleWorkflows = true;
             const secondWorkflow = buildWorkflow([1], [2, 3, 4]);
             secondWorkflow.approvers[0].forwardsTo = '[email protected]';
             secondWorkflow.approvers[1].forwardsTo = '[email protected]';
-            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 = '[email protected]';
 
-            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 = '[email protected]';
             secondWorkflow.approvers[1].forwardsTo = '[email protected]';
-            expect(workflows).toEqual([defaultWorkflow, secondWorkflow]);
+            expect(approvalWorkflows).toEqual([defaultWorkflow, secondWorkflow]);
         });
     });