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

…ws/cleanup-building-logic [Advanced Approval Workflows] Cleanup the logic responsible for building policy.employeeList
Expensify · Sep 4, 2024 · db68f8f · db68f8f
2 parents 8939351 + 610be9e
commit db68f8f
Show file tree

Hide file tree

Showing 5 changed files with 72 additions and 14 deletions.
diff --git a/src/libs/WorkflowUtils.ts b/src/libs/WorkflowUtils.ts
@@ -169,19 +169,35 @@ type ConvertApprovalWorkflowToPolicyEmployeesParams = {
      */
     approvalWorkflow: ApprovalWorkflow;
 
+    /**
+     * The previous employee list before the approval workflow was created
+     */
+    previousEmployeeList: PolicyEmployeeList;
+
     /**
      * Members to remove from the approval workflow
      */
     membersToRemove?: Member[];
 
+    /**
+     * Approvers to remove from the approval workflow
+     */
+    approversToRemove?: Approver[];
+
     /**
      * Mode to use when converting the approval workflow
      */
     type: ValueOf<typeof CONST.APPROVAL_WORKFLOW.TYPE>;
 };
 
 /** Convert an approval workflow to a list of policy employees */
-function convertApprovalWorkflowToPolicyEmployees({approvalWorkflow, membersToRemove, type}: ConvertApprovalWorkflowToPolicyEmployeesParams): PolicyEmployeeList {
+function convertApprovalWorkflowToPolicyEmployees({
+    approvalWorkflow,
+    previousEmployeeList,
+    membersToRemove,
+    approversToRemove,
+    type,
+}: ConvertApprovalWorkflowToPolicyEmployeesParams): PolicyEmployeeList {
     const updatedEmployeeList: PolicyEmployeeList = {};
     const firstApprover = approvalWorkflow.approvers.at(0);
 
@@ -191,26 +207,60 @@ function convertApprovalWorkflowToPolicyEmployees({approvalWorkflow, membersToRe
 
     approvalWorkflow.approvers.forEach((approver, index) => {
         const nextApprover = approvalWorkflow.approvers.at(index + 1);
+        const forwardsTo = type === CONST.APPROVAL_WORKFLOW.TYPE.REMOVE ? '' : nextApprover?.email ?? '';
+
+        if (previousEmployeeList[approver.email]?.forwardsTo === forwardsTo) {
+            return;
+        }
+
         updatedEmployeeList[approver.email] = {
             email: approver.email,
-            forwardsTo: type === CONST.APPROVAL_WORKFLOW.TYPE.REMOVE ? '' : nextApprover?.email ?? '',
+            forwardsTo,
         };
     });
 
     approvalWorkflow.members.forEach(({email}) => {
+        const submitsTo = type === CONST.APPROVAL_WORKFLOW.TYPE.REMOVE ? '' : firstApprover.email ?? '';
+
+        if (previousEmployeeList[email]?.submitsTo === submitsTo) {
+            return;
+        }
+
+        if (updatedEmployeeList[email]) {
+            updatedEmployeeList[email].submitsTo = submitsTo;
+            return;
+        }
+
         updatedEmployeeList[email] = {
-            ...(updatedEmployeeList[email] ? updatedEmployeeList[email] : {email}),
-            submitsTo: type === CONST.APPROVAL_WORKFLOW.TYPE.REMOVE ? '' : firstApprover.email ?? '',
+            email,
+            submitsTo,
         };
     });
 
     membersToRemove?.forEach(({email}) => {
+        if (updatedEmployeeList[email]) {
+            updatedEmployeeList[email].submitsTo = '';
+            return;
+        }
+
         updatedEmployeeList[email] = {
-            ...(updatedEmployeeList[email] ? updatedEmployeeList[email] : {email}),
+            email,
             submitsTo: '',
         };
     });
 
+    approversToRemove?.forEach(({email}) => {
+        if (updatedEmployeeList[email]) {
+            updatedEmployeeList[email].forwardsTo = '';
+            return;
+        }
+
+        updatedEmployeeList[email] = {
+            email,
+            forwardsTo: '',
+        };
+    });
+
     return updatedEmployeeList;
 }
 

diff --git a/src/libs/actions/Workflow.ts b/src/libs/actions/Workflow.ts
@@ -54,7 +54,7 @@ function createApprovalWorkflow(policyID: string, approvalWorkflow: ApprovalWork
 
     const previousEmployeeList = {...policy.employeeList};
     const previousApprovalMode = policy.approvalMode;
-    const updatedEmployees = convertApprovalWorkflowToPolicyEmployees({approvalWorkflow, type: CONST.APPROVAL_WORKFLOW.TYPE.CREATE});
+    const updatedEmployees = convertApprovalWorkflowToPolicyEmployees({previousEmployeeList, approvalWorkflow, type: CONST.APPROVAL_WORKFLOW.TYPE.CREATE});
 
     const optimisticData: OnyxUpdate[] = [
         {
@@ -111,7 +111,7 @@ function createApprovalWorkflow(policyID: string, approvalWorkflow: ApprovalWork
     API.write(WRITE_COMMANDS.CREATE_WORKSPACE_APPROVAL, parameters, {optimisticData, failureData, successData});
 }
 
-function updateApprovalWorkflow(policyID: string, approvalWorkflow: ApprovalWorkflow, membersToRemove: Member[]) {
+function updateApprovalWorkflow(policyID: string, approvalWorkflow: ApprovalWorkflow, membersToRemove: Member[], approversToRemove: Approver[]) {
     const policy = allPolicies?.[`${ONYXKEYS.COLLECTION.POLICY}${policyID}`];
 
     if (!authToken || !policy) {
@@ -121,7 +121,13 @@ function updateApprovalWorkflow(policyID: string, approvalWorkflow: ApprovalWork
     const previousDefaultApprover = policy.approver ?? policy.owner;
     const newDefaultApprover = approvalWorkflow.isDefault ? approvalWorkflow.approvers[0].email : undefined;
     const previousEmployeeList = {...policy.employeeList};
-    const updatedEmployees = convertApprovalWorkflowToPolicyEmployees({approvalWorkflow, type: CONST.APPROVAL_WORKFLOW.TYPE.UPDATE, membersToRemove});
+    const updatedEmployees = convertApprovalWorkflowToPolicyEmployees({
+        previousEmployeeList,
+        approvalWorkflow,
+        type: CONST.APPROVAL_WORKFLOW.TYPE.UPDATE,
+        membersToRemove,
+        approversToRemove,
+    });
 
     const optimisticData: OnyxUpdate[] = [
         {
@@ -191,7 +197,7 @@ function removeApprovalWorkflow(policyID: string, approvalWorkflow: ApprovalWork
     }
 
     const previousEmployeeList = {...policy.employeeList};
-    const updatedEmployees = convertApprovalWorkflowToPolicyEmployees({approvalWorkflow, type: CONST.APPROVAL_WORKFLOW.TYPE.REMOVE});
+    const updatedEmployees = convertApprovalWorkflowToPolicyEmployees({previousEmployeeList, approvalWorkflow, type: CONST.APPROVAL_WORKFLOW.TYPE.REMOVE});
     const updatedEmployeeList = {...previousEmployeeList, ...updatedEmployees};
 
     const defaultApprover = policy.approver ?? policy.owner;

diff --git a/src/pages/workspace/workflows/approvals/WorkspaceWorkflowsApprovalsEditPage.tsx b/src/pages/workspace/workflows/approvals/WorkspaceWorkflowsApprovalsEditPage.tsx
@@ -46,8 +46,10 @@ function WorkspaceWorkflowsApprovalsEditPage({policy, isLoadingReportData = true
             return;
         }
 
+        // We need to remove members and approvers that are no longer in the updated workflow
         const membersToRemove = initialApprovalWorkflow.members.filter((initialMember) => !approvalWorkflow.members.some((member) => member.email === initialMember.email));
-        Workflow.updateApprovalWorkflow(route.params.policyID, approvalWorkflow, membersToRemove);
+        const approversToRemove = initialApprovalWorkflow.approvers.filter((initialApprover) => !approvalWorkflow.approvers.some((approver) => approver.email === initialApprover.email));
+        Workflow.updateApprovalWorkflow(route.params.policyID, approvalWorkflow, membersToRemove, approversToRemove);
         Navigation.dismissModal();
     }, [approvalWorkflow, initialApprovalWorkflow, route.params.policyID]);
 

diff --git a/src/pages/workspace/workflows/approvals/WorkspaceWorkflowsApprovalsExpensesFromPage.tsx b/src/pages/workspace/workflows/approvals/WorkspaceWorkflowsApprovalsExpensesFromPage.tsx
@@ -142,7 +142,7 @@ function WorkspaceWorkflowsApprovalsExpensesFromPage({policy, isLoadingReportDat
             Navigation.navigate(ROUTES.WORKSPACE_WORKFLOWS_APPROVALS_APPROVER.getRoute(route.params.policyID, 0));
         } else {
             const firstApprover = approvalWorkflow?.approvers?.[0]?.email ?? '';
-            Navigation.navigate(ROUTES.WORKSPACE_WORKFLOWS_APPROVALS_EDIT.getRoute(route.params.policyID, firstApprover));
+            Navigation.goBack(ROUTES.WORKSPACE_WORKFLOWS_APPROVALS_EDIT.getRoute(route.params.policyID, firstApprover));
         }
     }, [approvalWorkflow, route.params.backTo, route.params.policyID, selectedMembers]);
 

diff --git a/tests/unit/WorkflowUtilsTest.ts b/tests/unit/WorkflowUtilsTest.ts
@@ -380,7 +380,7 @@ describe('WorkflowUtils', () => {
                 isDefault: true,
             };
 
-            const convertedEmployees = WorkflowUtils.convertApprovalWorkflowToPolicyEmployees({approvalWorkflow, type: 'create'});
+            const convertedEmployees = WorkflowUtils.convertApprovalWorkflowToPolicyEmployees({previousEmployeeList: {}, approvalWorkflow, type: 'create'});
 
             expect(convertedEmployees).toEqual({
                 '1@example.com': buildPolicyEmployee(1, {forwardsTo: '', submitsTo: '1@example.com'}),
@@ -395,7 +395,7 @@ describe('WorkflowUtils', () => {
                 isDefault: false,
             };
 
-            const convertedEmployees = WorkflowUtils.convertApprovalWorkflowToPolicyEmployees({approvalWorkflow, type: 'create'});
+            const convertedEmployees = WorkflowUtils.convertApprovalWorkflowToPolicyEmployees({previousEmployeeList: {}, approvalWorkflow, type: 'create'});
 
             expect(convertedEmployees).toEqual({
                 '1@example.com': buildPolicyEmployee(1, {forwardsTo: '2@example.com'}),
@@ -414,7 +414,7 @@ describe('WorkflowUtils', () => {
                 isDefault: false,
             };
 
-            const convertedEmployees = WorkflowUtils.convertApprovalWorkflowToPolicyEmployees({approvalWorkflow, type: 'remove'});
+            const convertedEmployees = WorkflowUtils.convertApprovalWorkflowToPolicyEmployees({previousEmployeeList: {}, approvalWorkflow, type: 'remove'});
 
             expect(convertedEmployees).toEqual({
                 '1@example.com': buildPolicyEmployee(1, {forwardsTo: ''}),