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

[IuliiaHerets]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] 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

1. Go to https://staging.new.expensify.com/home
2. Go to workspace settings- workflows
3. Go offline
4. Tap add approvals
5. Tap additional approver
6. Select a user & save it [user1]
7. Save edit approval flow
8. Tap add approvals
9. Note second approver is greyed out [user1]
10. Tap first approver and select different member and save it [user2]
11. Tap add approvals and note first approver is greyed out and second approver not greyed out now
12. Tap additional approver and select a new user and save it [user3]
13. Tap add approvals and note first+third approver greyed out & second approver is not greyed out
14. Remove the third approver & second approver ( note now we have only additional approver row)
15. Change the first approver ( to second approver who was removed [user1] ) and save
16. Now note after first approver is changed, edit approval flow automatically shows second approver with user selected
17. Now tap additional approver ( set as first approver who was selected in step 10- [ user2] )& save it
18. 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:

• Android: Standalone
• Android: HybridApp
• Android: mWeb Chrome
• iOS: Standalone
• iOS: HybridApp
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

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

[melvin-bot]

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.

[melvin-bot]

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

[melvin-bot]

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

[cretadn22]

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)

[nkdengineer]

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?

1. 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,
	},

2. 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?

1. 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,
	},

2. 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.

[huult]

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:

1. 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;
    });

2. 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});

3. 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:

1. Should update pendingFields for the current approver with the previous approver’s pendingFields
2. 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.

[melvin-bot]

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

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[melvin-bot]

@bfitzexpensify, @getusha Still overdue 6 days?! Let's take care of this!

[getusha]

I think we should hold this until #54178 is merged, there will be a change on this flow.

[bfitzexpensify]

Agreed. Updated the title

[melvin-bot]

@bfitzexpensify, @getusha Huh... This is 4 days overdue. Who can take care of this?

[melvin-bot]

@bfitzexpensify, @getusha 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

[melvin-bot]

@bfitzexpensify, @getusha 10 days overdue. Is anyone even seeing these? Hello?

[melvin-bot]

@bfitzexpensify, @getusha 12 days overdue now... This issue's end is nigh!

[bfitzexpensify]

Remains held

[melvin-bot]

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

[bfitzexpensify]

Remains held.

[melvin-bot]

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!

[cretadn22]

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

[huult]

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

[melvin-bot]

@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.