[HOLD for #54178] [$250] Expense - On changing 1st approver, 2nd approver is not greyed out&error for not added approvers

@getusha

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Version Number: 9.0.78-1
Reproducible in staging?: Yes
Reproducible in production?: Yes
Issue reported by: Applause Internal Team
Device used: Redmi note 10s Android 13
App Component: Money Requests

Action Performed:

Pre-condition: 1. Have a workspace with few members
2. Enable workflows and add approval flow

Go to https://staging.new.expensify.com/home
Go to workspace settings- workflows
Go offline
Tap add approvals
Tap additional approver
Select a user & save it [user1]
Save edit approval flow
Tap add approvals
Note second approver is greyed out [user1]
Tap first approver and select different member and save it [user2]
Tap add approvals and note first approver is greyed out and second approver not greyed out now
Tap additional approver and select a new user and save it [user3]
Tap add approvals and note first+third approver greyed out & second approver is not greyed out
Remove the third approver & second approver ( note now we have only additional approver row)
Change the first approver ( to second approver who was removed [user1] ) and save
Now note after first approver is changed, edit approval flow automatically shows second approver with user selected
Now tap additional approver ( set as first approver who was selected in step 10- [ user2] )& save it
Note fourth and fifth approver automatically added and error is displayed.

Expected Result:

After changing 1st approver, 2nd approver must be same as it was before changing 1st approver & not added approvers must not be shown with error message.

Actual Result:

After changing 1st approver, 2nd approver is not greyed out & not added approvers are shown with error message.

Workaround:

Unknown

Platforms:

Screenshots/Videos

Bug6701635_1735015172299.Screenrecorder-2024-12-24-09-29-51-391_compress_1.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~021871540038795816648
Upwork Job ID: 1871540038795816648
Last Price Increase: 2024-12-31

Issue Owner

Current Issue Owner: @getusha

@bfitzexpensify

Triggered auto assignment to @bfitzexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

Job added to Upwork: https://www.upwork.com/jobs/~021871540038795816648

@getusha

Triggered auto assignment to Contributor-plus team member for initial proposal review - @getusha (External)

Proposal

Please re-state the problem that we are trying to solve in this issue.

On changing 1st approver, 2nd approver is not greyed out&error for not added approvers

What is the root cause of that problem?

We have identified two separate bugs:

Bug 1 (Steps 1 to 13): The app incorrectly greys out the approver

The second approver is greyed out because the first approver has pendingFields.forwardsTo. However, when the first approver is updated, the pendingFields.forwardsTo of the first approver is not updated to reflect the new approver. This results in the second approver not being greyed out.

Bug 2 (Steps 14 to 18): The app incorrectly adds approvers automatically

When removing the second and third approvers in step 14, the changes are not saved. Consequently, in policy.employeeList, the first approver still forwards to the second approver, and the second approver forwards to the third approver.
This explains why, when changing the first approver to the second approver, the third approver is added automatically. This happens because the app retrieves information from employeeList, causing it to add more approvers automatically during the change.

App/src/libs/actions/Workflow.ts

Line 253 in d5cdd55

if (policy.employeeList[approver.email]?.forwardsTo) {

What changes do you think we should make in order to solve the problem?

Solution for bug 1:

App/src/libs/actions/Workflow.ts

Line 280 in b1d0d70

Onyx.merge(ONYXKEYS.APPROVAL_WORKFLOW, {approvers: updatedApprovers, errors});

We need to update the APPROVAL_WORKFLOW in onyx to migrate pendingFields.forwardsTo from the old approver to the new approver.

    const oldApprover = currentApprovalWorkflow.approvers.find((approver, index) => index === approverIndex);
    const oldApproverInMember = currentApprovalWorkflow.members.find((member) => member.email === oldApprover?.email);

    const updatedMembers = currentApprovalWorkflow.members.map((member, index) => {
        if (member.email === approver?.email) {
            return {
                ...member,
                pendingFields: {
                    forwardsTo: oldApproverInMember?.pendingFields?.forwardsTo
                }
            }
        }
        if (member.email === oldApprover?.email) {
            return {
                ...member,
                pendingFields: {
                    forwardsTo: null,
                }
            }
        }
        return {...member}
    })

    Onyx.merge(ONYXKEYS.APPROVAL_WORKFLOW, {approvers: updatedApprovers, members: updatedMembers, errors});

Additionally, we need to migrate the pendingFields.forwardTo from the approvalWorkflow to the employee list

App/src/libs/WorkflowUtils.ts

Line 238 in b1d0d70

forwardsTo: pendingAction,

    approvalWorkflow.members.forEach(({email, pendingFields}) => {
            ......
            pendingFields: {
                submitsTo: pendingAction,
                forwardsTo: pendingFields?.forwardsTo
            },

Solution for bug 2:

When removing an approver from the Approver Page, we need to introduce an additional field, perhaps named pendingRemove, in the clearApprovalWorkflowApprover function. This field will mark the approver as already removed from the current workflow. Then, we need to ensure that an approver is added automatically only if pendingRemove is false.

App/src/libs/actions/Workflow.ts

Line 253 in d5cdd55

if (policy.employeeList[approver.email]?.forwardsTo) {

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

Bug 1:

We might need to add a new test for this scenario in https://github.com/Expensify/App/blob/main/tests/unit/WorkflowUtilsTest.ts

This test will validate that pendingFields is correctly handled in the edit approval workflow. The input will consist of an employee list, and it will verify which indices should be greyed

Bug 2:

Testing this case is quite challenging, as it would be complicated to mock the test. However, if we still want to cover it with a unit test, we need to validate that the approver who was just deleted cannot be automatically added back to the approval chain

What alternative solutions did you explore? (Optional)

Proposal

Please re-state the problem that we are trying to solve in this issue.

After changing 1st approver, 2nd approver is not greyed out & not added approvers are shown with error message.

What is the root cause of that problem?

When we change the first approver, pendingFields.forwardsTo of the new approver is undefined then the second row is not greyed out.

App/src/libs/actions/Workflow.ts

Line 242 in ad99c20

    
           function setApprovalWorkflowApprover(approver: Approver, approverIndex: number, policyID: string) {

After we click on the save button, we've had the logic to add forwardsTo pending field here

App/src/libs/WorkflowUtils.ts

Lines 237 to 239 in ad99c20

    
           pendingFields: { 
        
               forwardsTo: pendingAction, 
        
           },

But it's overridden here when the submitsTo is changed here. That is why this bug only happens when we change the first approver

App/src/libs/WorkflowUtils.ts

Lines 256 to 258 in ad99c20

    
           pendingFields: { 
        
               submitsTo: pendingAction, 
        
           },

When we change the approver without saving, we add the forwardsTo of the approver automatically based on the policy employee list here then that causes the error because the first approver still forwards to user 1 and user 1 still forwards to user 2.

App/src/libs/actions/Workflow.ts

Lines 253 to 255 in ad99c20

    
           if (policy.employeeList[approver.email]?.forwardsTo) { 
        
               const additionalApprovers = calculateApprovers({employees: policy.employeeList, firstEmail: approver.email, personalDetailsByEmail}); 
        
               approvers.splice(approverIndex, approvers.length, ...additionalApprovers);

What changes do you think we should make in order to solve the problem?

When we update the approver here, we can update forwardsTo pending fields of the new approver based on the old approver

const prevApprover = currentApprovalWorkflow.approvers.find((approver, index) => index === approverIndex);
const prevApproverInMember = currentApprovalWorkflow.members.find((member) => member.email === prevApprover?.email);

const updatedMembers = currentApprovalWorkflow.members.map((member, index) => {
    if (member.email === approver?.email) {
        return {
            ...member,
            pendingFields: {
                forwardsTo: prevApproverInMember?.pendingFields?.forwardsTo
            }
        }
    }
    if (member.email === prevApprover?.email) {
        return {
            ...member,
            pendingFields: {
                forwardsTo: null,
            }
        }
    }
    return {...member}
})

Onyx.merge(ONYXKEYS.APPROVAL_WORKFLOW, {approvers: updatedApprovers, members: updatedMembers, errors});

App/src/libs/actions/Workflow.ts

Line 242 in ad99c20

    
           function setApprovalWorkflowApprover(approver: Approver, approverIndex: number, policyID: string) {

When we save the workflow, we should keep the rest pending field here

const pendingFields = updatedEmployeeList?.[email]?.pendingFields ?? {};
updatedEmployeeList[email] = {
    ...(updatedEmployeeList[email] ? updatedEmployeeList[email] : {email}),
    submitsTo,
    pendingAction,
    pendingFields: {
        ...pendingFields,
        submitsTo: pendingAction,
    },
};

App/src/libs/WorkflowUtils.ts

Lines 256 to 258 in ad99c20

    
           pendingFields: { 
        
               submitsTo: pendingAction, 
        
           },

I think we can remove this logic since it's not necessary

App/src/libs/actions/Workflow.ts

Lines 253 to 255 in ad99c20

    
           if (policy.employeeList[approver.email]?.forwardsTo) { 
        
               const additionalApprovers = calculateApprovers({employees: policy.employeeList, firstEmail: approver.email, personalDetailsByEmail}); 
        
               approvers.splice(approverIndex, approvers.length, ...additionalApprovers);

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

What alternative solutions did you explore? (Optional)

NA

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

Proposal

Please re-state the problem that we are trying to solve in this issue.

On changing 1st approver, 2nd approver is not greyed out&error for not added approvers

What is the root cause of that problem?

When we change an approver who has a forwardTo set to another approver, the pendingFields.forwardsTo of the previous approver is not updated to reflect the new approver. As a result, when we switch to a new approver, the approver being forwarded to will not be grayed out because the pendingFields.forwardsTo of the new approver is undefined.

Logic to getApprovalPendingAction

App/src/pages/workspace/workflows/approvals/ApprovalWorkflowEditor.tsx

Line 48 in ab40264

const getApprovalPendingAction = useCallback(

We updated the approver but did not update the pendingFields.forwardsTo of the new approver

App/src/libs/actions/Workflow.ts

Line 242 in d14b37f

    
           function setApprovalWorkflowApprover(approver: Approver, approverIndex: number, policyID: string) {

What changes do you think we should make in order to solve the problem?

To resolve this issue, we need to update the pendingFields.forwardsTo for the new approver by implementing the following steps:

We need to retrieve the pendingFields.forwardsTo of the previous approver and update it for the new approver.

App/src/libs/actions/Workflow.ts

Line 249 in d14b37f

    
           const approvers: Array<Approver | undefined> = [...currentApprovalWorkflow.approvers];

    const approvers: Array<Approver | undefined> = [...currentApprovalWorkflow.approvers];
    const previousApprover = approvers.at(approverIndex)?.email;
    const pendingFieldsOfForwardsTo = previousApprover ? currentApprovalWorkflow.members.find((item) => item.email === previousApprover) : undefined;
    const newMembers = currentApprovalWorkflow.members.map((item) => {
        if (item.email === approver.email) {
            return {
                ...item,
                pendingFields: pendingFieldsOfForwardsTo?.pendingFields ?? item.pendingFields, // Fallback to item's original pendingFields if undefined
            };
        }

        if (item.email === approvers.at(approverIndex)?.email) {
            return {
                ...item,
                pendingFields: {
                    forwardsTo: null,
                },
            };
        }

        return item;
    });

Update Onyx

App/src/libs/actions/Workflow.ts

Line 280 in d14b37f

Onyx.merge(ONYXKEYS.APPROVAL_WORKFLOW, {approvers: updatedApprovers, errors});

Update to:

    Onyx.merge(ONYXKEYS.APPROVAL_WORKFLOW, {approvers: updatedApprovers, members: newMembers, errors});

Update member data in convertApprovalWorkflowToPolicyEmployees when updating updateApprovalWorkflow.

App/src/libs/WorkflowUtils.ts

Lines 243 to 260 in d14b37f

    
           approvalWorkflow.members.forEach(({email}) => { 
        
               const submitsTo = type === CONST.APPROVAL_WORKFLOW.TYPE.REMOVE ? '' : firstApprover.email ?? ''; 
        
               // For every member, we check if the submitsTo field has changed. 
        
               // If it has, we update the employee list with the new submitsTo value. 
        
               if (previousEmployeeList[email]?.submitsTo === submitsTo) { 
        
                   return; 
        
               } 
        
               updatedEmployeeList[email] = { 
        
                   ...(updatedEmployeeList[email] ? updatedEmployeeList[email] : {email}), 
        
                   submitsTo, 
        
                   pendingAction, 
        
                   pendingFields: { 
        
                       submitsTo: pendingAction, 
        
                   }, 
        
               }; 
        
           });

Update to:

    approvalWorkflow.members.forEach(({email}) => {
        const submitsTo = type === CONST.APPROVAL_WORKFLOW.TYPE.REMOVE ? '' : firstApprover.email ?? '';

        // For every member, we check if the submitsTo field has changed.
        // If it has, we update the employee list with the new submitsTo value.
        if (previousEmployeeList[email]?.submitsTo === submitsTo) {
            return;
        }

        updatedEmployeeList[email] = {
            ...(updatedEmployeeList[email] ? updatedEmployeeList[email] : {email}),
            submitsTo,
            pendingAction,
            pendingFields: {
                submitsTo: pendingAction,
                forwardsTo: updatedEmployeeList[email]?.pendingFields?.forwardsTo,
            },
        };
    });

POC

Screen.Recording.2024-12-25.at.13.16.22.mp4

Test branch

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

Add test case for members data:

Should update pendingFields for the current approver with the previous approver’s pendingFields
Should not update pendingFields when approverIndex is invalid

What alternative solutions did you explore? (Optional)

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

@bfitzexpensify

@bfitzexpensify, @getusha Eep! 4 days overdue now. Issues have feelings too...

@bfitzexpensify

This issue has not been updated in over 14 days. @bfitzexpensify, @getusha eroding to Weekly issue.

Remains held.

@bfitzexpensify

This issue has not been updated in over 15 days. @bfitzexpensify, @getusha eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

@getusha

@getusha Could you review the proposals? the PR 54178 was merged

@getusha

@getusha @bfitzexpensify Is this issue still open to be fixed?

@bfitzexpensify

@bfitzexpensify, @getusha, this Monthly task hasn't been acted upon in 6 weeks; closing.

If you disagree, feel encouraged to reopen it -- but pick your least important issue to close instead.

Navigation Menu

Repository navigation

[HOLD for #54178] [$250] Expense - On changing 1st approver, 2nd approver is not greyed out&error for not added approvers #54524

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

Proposal

Please re-state the problem that we are trying to solve in this issue.

What is the root cause of that problem?

What changes do you think we should make in order to solve the problem?

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

What alternative solutions did you explore? (Optional)

Proposal

Please re-state the problem that we are trying to solve in this issue.

What is the root cause of that problem?

What changes do you think we should make in order to solve the problem?

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

What alternative solutions did you explore? (Optional)

Proposal

Please re-state the problem that we are trying to solve in this issue.

What is the root cause of that problem?

What changes do you think we should make in order to solve the problem?

Test branch

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

What alternative solutions did you explore? (Optional)

15 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	if (policy.employeeList[approver.email]?.forwardsTo) {
	const additionalApprovers = calculateApprovers({employees: policy.employeeList, firstEmail: approver.email, personalDetailsByEmail});
	approvers.splice(approverIndex, approvers.length, ...additionalApprovers);

	approvalWorkflow.members.forEach(({email}) => {
	const submitsTo = type === CONST.APPROVAL_WORKFLOW.TYPE.REMOVE ? '' : firstApprover.email ?? '';

	// For every member, we check if the submitsTo field has changed.
	// If it has, we update the employee list with the new submitsTo value.
	if (previousEmployeeList[email]?.submitsTo === submitsTo) {
	return;
	}

	updatedEmployeeList[email] = {
	...(updatedEmployeeList[email] ? updatedEmployeeList[email] : {email}),
	submitsTo,
	pendingAction,
	pendingFields: {
	submitsTo: pendingAction,
	},
	};
	});

[HOLD for #54178] [$250] Expense - On changing 1st approver, 2nd approver is not greyed out&error for not added approvers #54524

Description

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

Activity

melvin-bot commented on Dec 24, 2024

melvin-bot commented on Dec 24, 2024

melvin-bot commented on Dec 24, 2024

cretadn22 commented on Dec 24, 2024

Proposal

Please re-state the problem that we are trying to solve in this issue.

What is the root cause of that problem?

What changes do you think we should make in order to solve the problem?

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

What alternative solutions did you explore? (Optional)

nkdengineer commented on Dec 25, 2024

Proposal

Please re-state the problem that we are trying to solve in this issue.

What is the root cause of that problem?

What changes do you think we should make in order to solve the problem?

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

What alternative solutions did you explore? (Optional)

huult commented on Dec 25, 2024

Proposal

Please re-state the problem that we are trying to solve in this issue.

What is the root cause of that problem?

What changes do you think we should make in order to solve the problem?

Test branch

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

What alternative solutions did you explore? (Optional)

melvin-bot commented on Dec 30, 2024

15 remaining items

melvin-bot commented on Jan 20, 2025

bfitzexpensify commented on Jan 21, 2025

melvin-bot commented on Feb 13, 2025

cretadn22 commented on Mar 17, 2025

huult commented on Apr 20, 2025

melvin-bot commented on Apr 28, 2025

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions