[HOLD for payment 2024-11-28] [$250] Not found page appear briefly after updating the first approver

@DylanDylann

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.52-1
Reproducible in staging?: y
Reproducible in production?: y
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?:
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: @DylanDylann
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1729573070656459

Action Performed:

Create a new workspace with at least 2 members
Enable workflow -> enable Add approvals
Change the first approval
Click save button

Expected Result:

not found page should not display

Actual Result:

not found page is displayed briefly

Workaround:

unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Screenshots/Videos

Add any screenshot/video evidence

Screen.Recording.2024-10-22.at.11.58.41.mov

Recording.678.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~021851668069597317065
Upwork Job ID: 1851668069597317065
Last Price Increase: 2024-10-30
Automatic offers:

daledah | Contributor | 104771833

Issue Owner

Current Issue Owner: @JmillsExpensify

@JmillsExpensify

Triggered auto assignment to @JmillsExpensify (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.

Proposal

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

not found page is displayed briefly

What is the root cause of that problem?

In here, we have logic to calculate currentApprovalWorkflow base on approvalWorkflows and firstApprover get from route params

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

Line 87 in 04214cd

    
           currentApprovalWorkflow: result.approvalWorkflows.find((workflow) => workflow.approvers.at(0)?.email === firstApprover),

When we change the first approval, we only update the data of approvalWorkflows without updating the route params so currentApprovalWorkflow will be empty => shouldShowNotFoundView = true

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

Lines 92 to 93 in 04214cd

    
           const shouldShowNotFoundView = 
        
               (isEmptyObject(policy) && !isLoadingReportData) || !PolicyUtils.isPolicyAdmin(policy) || PolicyUtils.isPendingDeletePolicy(policy) || !currentApprovalWorkflow;

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

We should close the modal before updating the approvalWorkflows data using InteractionManager.runAfterInteractions
here

        Navigation.dismissModal();
        InteractionManager.runAfterInteractions(() => {
                Workflow.updateApprovalWorkflow(route.params.policyID, approvalWorkflow, membersToRemove, approversToRemove);
        });

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

Lines 53 to 54 in 04214cd

    
           Workflow.updateApprovalWorkflow(route.params.policyID, approvalWorkflow, membersToRemove, approversToRemove); 
        
           Navigation.dismissModal();

What alternative solutions did you explore? (Optional)

@JmillsExpensify

@JmillsExpensify I discovered this bug and have more context because I was also the main C+ on the approval workflow feature. I am happy to take this issue as C+ if it is external

This bug is handled on removing flow on #48089 and we also fix for this case where users update the first approver

Proposal

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

A not found is shown after updating the first approver.

What is the root cause of that problem?

The not found will show if the current approval workflow isn't found.

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

Lines 92 to 93 in faf48fc

    
           const shouldShowNotFoundView = 
        
               (isEmptyObject(policy) && !isLoadingReportData) || !PolicyUtils.isPolicyAdmin(policy) || PolicyUtils.isPendingDeletePolicy(policy) || !currentApprovalWorkflow;

The current approval workflow finds the workflow where the approver is the same as the first approver route params.

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

Line 87 in faf48fc

    
           currentApprovalWorkflow: result.approvalWorkflows.find((workflow) => workflow.approvers.at(0)?.email === firstApprover),

When we open the page with the first approver of A, then update it to B, A won't be in the approver list anymore, so current approval workflow can't be found.

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

Instead of using navigation focused state to prevent the not found page (which is not reliable IMO), we can prevent the current approval workflow data to be updated when we are submitting the update. To do that, we need 2 new ref.

const isUpdatedRef = useRef(false);
const prevApprovalData = useRef({});

isUpdatedRef tells whether the user updates the data or not. prevApprovalData contains the previous approval workflow data.

Set isUpdatedRef to true when updating the data.

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

Lines 41 to 55 in faf48fc

    
           const updateApprovalWorkflow = useCallback(() => { 
        
               if (!approvalWorkflow || !initialApprovalWorkflow) { 
        
                   return; 
        
               } 
        
               if (!Workflow.validateApprovalWorkflow(approvalWorkflow)) { 
        
                   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)); 
        
               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]);

Store the approval data to prevApprovalData so we can use it later and return the prevApprovalData if isUpdatedRef is true.

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

Lines 70 to 89 in faf48fc

    
           const {currentApprovalWorkflow, defaultWorkflowMembers, usedApproverEmails} = useMemo(() => { 
        
               if (!policy || !personalDetails) { 
        
                   return {}; 
        
               } 
        
               const defaultApprover = policy?.approver ?? policy.owner; 
        
               const firstApprover = route.params.firstApproverEmail; 
        
               const result = convertPolicyEmployeesToApprovalWorkflows({ 
        
                   employees: policy.employeeList ?? {}, 
        
                   defaultApprover, 
        
                   personalDetails, 
        
                   firstApprover, 
        
               }); 
        
               return { 
        
                   defaultWorkflowMembers: result.availableMembers, 
        
                   usedApproverEmails: result.usedApproverEmails, 
        
                   currentApprovalWorkflow: result.approvalWorkflows.find((workflow) => workflow.approvers.at(0)?.email === firstApprover), 
        
               }; 
        
           }, [personalDetails, policy, route.params.firstApproverEmail]);

const {currentApprovalWorkflow, defaultWorkflowMembers, usedApproverEmails} = useMemo(() => {
    if (!policy || !personalDetails) {
        return {};
    }

    if (isUpdatedRef.current) {
        return prevApprovalData.current;
    }

    const defaultApprover = policy?.approver ?? policy.owner;
    const firstApprover = route.params.firstApproverEmail;
    const result = convertPolicyEmployeesToApprovalWorkflows({...});

    const approvalData = {
        defaultWorkflowMembers: result.availableMembers,
        usedApproverEmails: result.usedApproverEmails,
        currentApprovalWorkflow: result.approvalWorkflows.find((workflow) => workflow.approvers.at(0)?.email === firstApprover),
    };
    prevApprovalData.current = approvalData;
    return approvalData;
}, [personalDetails, policy, route.params.firstApproverEmail]);

What alternative solutions did you explore? (Optional)

Prevent the not found from showing if the page isn't focused if we prefer that way.

const shouldShowNotFoundView = isFocused && ...

@JmillsExpensify

@JmillsExpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

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

@thesahindia

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

BugZero Checklist:

[Contributor] Classify the bug:

Bug classification

Source of bug:

1a. Result of the original design (eg. a case wasn't considered)
1b. Mistake during implementation
1c. Backend bug
1z. Other:

Where bug was reported:

2a. Reported on production
2b. Reported on staging (deploy blocker)
2c. Reported on both staging and production
2d. Reported on a PR
2z. Other:

Who reported the bug:

[Contributor] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake.

Link to comment: [CP Staging][CRITICAL] Implement <WorkspaceWorkflowsApprovalsEditPage /> for Editing Workflow #47249 (comment)
[Contributor] If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner.

Link to discussion:
[Contributor] If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again.
[BugZero Assignee] Create a GH issue for creating/updating the regression test once above steps have been agreed upon.

Link to issue:

Regression Test Proposal

Test:

Create a new workspace with at least 2 members
Enable workflow -> enable Add approvals
Change the first approver in the first approval workflow
Click save button
Verify that: Not found page doesn't appear

Do we agree 👍 or 👎

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

@deetergp

@deetergp, @JmillsExpensify, @daledah Huh... This is 4 days overdue. Who can take care of this?

@daledah

Payment summary:

Contributor: $250 @daledah via Upwork
Reviewer: $250 @DylanDylann via Upwork

Regression test created and one contributor payment made.

@deetergp

@deetergp, @JmillsExpensify, @daledah Eep! 4 days overdue now. Issues have feelings too...

All paid out.

Navigation Menu

Repository navigation

[HOLD for payment 2024-11-28] [$250] Not found page appear briefly after updating the first approver #51282

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 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 alternative solutions did you explore? (Optional)

27 remaining items

BugZero Checklist:

Regression Test Proposal

Test:

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	const shouldShowNotFoundView =
	(isEmptyObject(policy) && !isLoadingReportData) \|\| !PolicyUtils.isPolicyAdmin(policy) \|\| PolicyUtils.isPendingDeletePolicy(policy) \|\| !currentApprovalWorkflow;

	Workflow.updateApprovalWorkflow(route.params.policyID, approvalWorkflow, membersToRemove, approversToRemove);
	Navigation.dismissModal();

	const updateApprovalWorkflow = useCallback(() => {
	if (!approvalWorkflow \|\| !initialApprovalWorkflow) {
	return;
	}

	if (!Workflow.validateApprovalWorkflow(approvalWorkflow)) {
	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));
	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]);

	const {currentApprovalWorkflow, defaultWorkflowMembers, usedApproverEmails} = useMemo(() => {
	if (!policy \|\| !personalDetails) {
	return {};
	}

	const defaultApprover = policy?.approver ?? policy.owner;
	const firstApprover = route.params.firstApproverEmail;
	const result = convertPolicyEmployeesToApprovalWorkflows({
	employees: policy.employeeList ?? {},
	defaultApprover,
	personalDetails,
	firstApprover,
	});

	return {
	defaultWorkflowMembers: result.availableMembers,
	usedApproverEmails: result.usedApproverEmails,
	currentApprovalWorkflow: result.approvalWorkflows.find((workflow) => workflow.approvers.at(0)?.email === firstApprover),
	};
	}, [personalDetails, policy, route.params.firstApproverEmail]);

[HOLD for payment 2024-11-28] [$250] Not found page appear briefly after updating the first approver #51282

Description

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

Activity

melvin-bot commented on Oct 22, 2024

daledah commented on Oct 22, 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 alternative solutions did you explore? (Optional)

DylanDylann commented on Oct 23, 2024

bernhardoj commented on Oct 23, 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 alternative solutions did you explore? (Optional)

melvin-bot commented on Oct 25, 2024

melvin-bot commented on Oct 30, 2024

melvin-bot commented on Oct 30, 2024

27 remaining items

DylanDylann commented on Nov 28, 2024

BugZero Checklist:

Regression Test Proposal

Test:

melvin-bot commented on Nov 29, 2024

melvin-bot commented on Dec 2, 2024

JmillsExpensify commented on Dec 3, 2024

JmillsExpensify commented on Dec 3, 2024

melvin-bot commented on Dec 9, 2024

JmillsExpensify commented on Dec 9, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions