[HOLD for payment 2024-10-22] [$250] Workflow - Tapping add approval workflow or tapping back shows diff. behavior in mweb&android

[lanitochka17]

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.43-1
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Issue reported by: Applause - Internal Team

Action Performed:

1. Launch both mweb and Android
2. Tap profile icon -- workspaces -- new workspace
3. Tap more features -- enable workflow
4. Tap workflow -- enable add approvals
5. Tap add approval workflow
6. Tap app back button

Expected Result:

While tapping add approval workflow or on tapping app back button "No members to display " page must not be shown

Actual Result:

In Android, tapping add approval workflow " No members to display " shown briefly but in mweb while tapping back button No members to display " page is shown

Workaround:

Unknown

Platforms:

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

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6622365_1727889041861.Screenrecorder-2024-10-02-22-26-36-193_compress_1.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021841560364430821283
• Upwork Job ID: 1841560364430821283
• Last Price Increase: 2024-10-02
• Automatic offers:
• Nodebrute | Contributor | 104312293

Issue Owner

Current Issue Owner: @s77rt

[melvin-bot]

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

[lanitochka17]

@garrettmknight FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

[cretadn22]

Proposal

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

What is the root cause of that problem?

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

Lines 149 to 155 in 66d1025

	const goBack = useCallback(() => {
	if (isInitialCreationFlow) {
	Workflow.clearApprovalWorkflow();
	}
	Navigation.goBack();
	}, [isInitialCreationFlow]);

We clear the approval workflow data before closing the modal.

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

We should first call Navigation.goBack(); and only clear the data once the transition is complete.

        Navigation.goBack();  // We also can use dismissModal and Modal.close 
        if (isInitialCreationFlow) {
                InteractionManager.runAfterInteractions(() => {
                    Workflow.clearApprovalWorkflow();
                });
        }

[melvin-bot]

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

[melvin-bot]

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

[nkdengineer]

Proposal

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

In Android, tapping add approval workflow " No members to display " shown briefly but in mweb while tapping back button No members to display " page is shown

What is the root cause of that problem?

1. For mweb, the approval flow is cleared before we complete navigating back to the previous screen then the empty screen appears briefly

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

Lines 149 to 156 in f325244

	const goBack = useCallback(() => {
	if (isInitialCreationFlow) {
	Workflow.clearApprovalWorkflow();
	}
	Navigation.goBack();
	}, [isInitialCreationFlow]);
	const button = useMemo(() => {

2. For Android, we merge the approval workflow data to Onyx here and then navigate to the approval screen but the data is merged after we navigate to the approval screen. So the empty screen appears briefly based on this condition here

App/src/pages/workspace/workflows/WorkspaceWorkflowsPage.tsx

Lines 103 to 109 in 19a137d

	Workflow.setApprovalWorkflow({
	...INITIAL_APPROVAL_WORKFLOW,
	availableMembers,
	usedApproverEmails,
	});
	Navigation.navigate(ROUTES.WORKSPACE_WORKFLOWS_APPROVALS_EXPENSES_FROM.getRoute(route.params.policyID));
	}, [policy, route.params.policyID, availableMembers, usedApproverEmails]);

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

1. For mweb bug, we should navigate back first and wrap the logic to clear approval workflow in InteractionManager.runAfterInteractions or use Modal.close

const goBack = useCallback(() => {
    Navigation.goBack();
    InteractionManager.runAfterInteractions(() => {
        if (!isInitialCreationFlow) {
            return;
        }
        Workflow.clearApprovalWorkflow();
    });
}, [isInitialCreationFlow]);

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

Lines 149 to 156 in f325244

	const goBack = useCallback(() => {
	if (isInitialCreationFlow) {
	Workflow.clearApprovalWorkflow();
	}
	Navigation.goBack();
	}, [isInitialCreationFlow]);
	const button = useMemo(() => {

2. For Android bug, we should navigate to the approval screen after the data is merged to Onyx completely.

App/src/pages/workspace/workflows/WorkspaceWorkflowsPage.tsx

Lines 103 to 109 in 19a137d

	Workflow.setApprovalWorkflow({
	...INITIAL_APPROVAL_WORKFLOW,
	availableMembers,
	usedApproverEmails,
	});
	Navigation.navigate(ROUTES.WORKSPACE_WORKFLOWS_APPROVALS_EXPENSES_FROM.getRoute(route.params.policyID));
	}, [policy, route.params.policyID, availableMembers, usedApproverEmails]);

To do that we can use set method here which is faster because I see that we always pass full approval workflow data to this function.

App/src/libs/actions/Workflow.ts

Line 337 in 19a137d

Onyx.merge(ONYXKEYS.APPROVAL_WORKFLOW, approvalWorkflow);

Or we can return the Onyx.merge promise in this function then move the navigate logic to .then of the promise returned by setApprovalWorkflow function.

App/src/pages/workspace/workflows/WorkspaceWorkflowsPage.tsx

Lines 103 to 109 in 19a137d

	Workflow.setApprovalWorkflow({
	...INITIAL_APPROVAL_WORKFLOW,
	availableMembers,
	usedApproverEmails,
	});
	Navigation.navigate(ROUTES.WORKSPACE_WORKFLOWS_APPROVALS_EXPENSES_FROM.getRoute(route.params.policyID));
	}, [policy, route.params.policyID, availableMembers, usedApproverEmails]);

What alternative solutions did you explore? (Optional)

[abzokhattab]

Edited by proposal-police: This proposal was edited at 2024-10-02 20:22:02 UTC.

Proposal

Please restate the problem we are trying to solve in this issue.

Workflow - When tapping "Add Approval Workflow" or navigating back, the message "No members to display" is briefly shown.

What is the root cause of the problem?

When tapping the back button, the approval workflow object is set to null:

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

Lines 149 to 156 in f325244

	const goBack = useCallback(() => {
	if (isInitialCreationFlow) {
	Workflow.clearApprovalWorkflow();
	}
	Navigation.goBack();
	}, [isInitialCreationFlow]);
	const button = useMemo(() => {

App/src/libs/actions/Workflow.ts

Lines 340 to 342 in 9d252e8

	function clearApprovalWorkflow() {
	Onyx.set(ONYXKEYS.APPROVAL_WORKFLOW, null);
	}

This causes the shouldShowListEmptyContent variable to resolve to undefined, not false:

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

Line 63 in 9d252e8

const shouldShowListEmptyContent = approvalWorkflow && approvalWorkflow.availableMembers.length === 0;

Since we display the listEmptyContent if shouldShowListEmptyContent is true, the issue arises when shouldShowListEmptyContent is undefined. In such cases, the BaseSelectionList component uses a default value of true for shouldShowListEmptyContent:

App/src/components/SelectionList/BaseSelectionList.tsx

Line 103 in 0993f36

shouldShowListEmptyContent = true,

When an undefined value is passed, the component interprets it as not provided, and defaults to true.

What changes should we make to solve the problem?

We should ensure that the value passed is always a boolean, not undefined.To do this, we can update the code here:

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

Line 63 in 9d252e8

const shouldShowListEmptyContent = approvalWorkflow && approvalWorkflow.availableMembers.length === 0;

as follows:

const shouldShowListEmptyContent = !!(approvalWorkflow && approvalWorkflow.availableMembers.length === 0);

This ensures the empty page is only displayed when appropriate. we can do the same change in other arias that uses the shouldShowListEmptyContent prop in the selectionlist

now, we will have an issue where the members list becomes empty before navigating back. To address this, and the problem where the sections list becomes empty after clearing the members list, we can introduce a ref or state, isNavigatingBack. This state will be set to true if the user is navigating back after clearing the workflow.We will also use a usePrevious ref for approvalWorkflow?.availableMembers, which will be utilized in the section memo when the app is navigating back.
By implementing this solution, we maintain the synchronous behavior of first clearing the data and then performing the back action.

Changes:

1. Introduce the following:

    const [isNavigatingBack, setIsNavigatingBack] = useState(false);
const previousAvailableMembers = usePrevious(approvalWorkflow?.availableMembers);

2. add setIsNavigatingBack(true) in the goBack function before clearing the flow
3. Modify this condition as follows:

if (!approvalWorkflow?.members || isNavigatingBack) {

4. Use previousAvailableMembers when navigating back instead of this :

        if (approvalWorkflow?.availableMembers ?? isNavigatingBack) {
            const workflowAvailableMembers = (isNavigatingBack ? previousAvailableMembers : approvalWorkflow?.availableMembers) ?? [];
            const availableMembers = workflowAvailableMembers

5. Add || isNavigatingBack in this line .

What alternative solutions did you explore? (Optional)

[Nodebrute]

Proposal

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

Tapping add approval workflow or tapping back shows diff. behavior in mweb&android

What is the root cause of that problem?

First, we only clear the workflow when the user presses Go Back. The workflow will not be cleared if the user clicks on the central pane to navigate the workflow page. The second issue is that we encounter the member's empty state because of this.

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

Lines 149 to 154 in 19a137d

	const goBack = useCallback(() => {
	if (isInitialCreationFlow) {
	Workflow.clearApprovalWorkflow();
	}
	Navigation.goBack();
	}, [isInitialCreationFlow]);

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

We should remove this implementation from here. In this component WorkspaceWorkflowsApprovalsExpensesFromPage, we will only go back

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

Lines 149 to 154 in 19a137d

	const goBack = useCallback(() => {
	if (isInitialCreationFlow) {
	Workflow.clearApprovalWorkflow();
	}
	Navigation.goBack();
	}, [isInitialCreationFlow]);

  onBackButtonPress={()=>Navigation.goBack()}

And then in WorkspaceWorkflowsPage We will clearApprovalWorkflow before starting a new one here

https://github.com/Expensify/App/blob/main/src/pages/workspace/workflows/WorkspaceWorkflowsPage.tsx#L102

      Workflow.clearApprovalWorkflow();
        Workflow.setApprovalWorkflow({
            ...INITIAL_APPROVAL_WORKFLOW,
            availableMembers,
            usedApproverEmails,
        });

We can implement the same solution in other places too where we have this issue

What alternative solutions did you explore? (Optional)

In the alternative solution, we should also remove the clearApprovalWorkflow from WorkspaceWorkflowsApprovalsExpensesFromPage and use only goBack()

and in here we can use set by doing this we'll make sure that every time we are setting value instead of merging

App/src/libs/actions/Workflow.ts

Lines 336 to 338 in 19a137d

	function setApprovalWorkflow(approvalWorkflow: NullishDeep<ApprovalWorkflowOnyx>) {
	Onyx.merge(ONYXKEYS.APPROVAL_WORKFLOW, approvalWorkflow);
	}

[bernhardoj]

Proposal

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

An empty state view is shown when opening/closing the approval workflow page.

What is the root cause of that problem?

When we open the page, we will set the approval data first and then navigate to the page.

App/src/pages/workspace/workflows/WorkspaceWorkflowsPage.tsx

Lines 103 to 108 in e61ee7f

	Workflow.setApprovalWorkflow({
	...INITIAL_APPROVAL_WORKFLOW,
	availableMembers,
	usedApproverEmails,
	});
	Navigation.navigate(ROUTES.WORKSPACE_WORKFLOWS_APPROVALS_EXPENSES_FROM.getRoute(route.params.policyID));

However, the onyx data is not immediately available which is noticeable in slower devices like Android.

And when we close the page, we clear the approval data.

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

Lines 149 to 154 in e61ee7f

	const goBack = useCallback(() => {
	if (isInitialCreationFlow) {
	Workflow.clearApprovalWorkflow();
	}
	Navigation.goBack();
	}, [isInitialCreationFlow]);

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

For the first problem, we can use Navigation.setNavigationActionToMicrotaskQueue to delay the navigation.

App/src/libs/Navigation/setNavigationActionToMicrotaskQueue.ts

Lines 1 to 5 in e61ee7f

	export default function setNavigationActionToMicrotaskQueue(navigationAction: () => void) {
	/**
	* The function is used when the app needs to set a navigation action to the microtask queue, it guarantees to execute Onyx.update first, then the navigation action.
	* More details - https://github.com/Expensify/App/issues/37785#issuecomment-1989056726.
	*/

Navigation.setNavigationActionToMicrotaskQueue(() => Navigation.navigate(ROUTES.WORKSPACE_WORKFLOWS_APPROVALS_EXPENSES_FROM.getRoute(route.params.policyID)));

For the second problem, we can delay the clearing of data using InteractionManager, just like we did here.

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

Lines 63 to 67 in e61ee7f

	Navigation.dismissModal();
	InteractionManager.runAfterInteractions(() => {
	// Remove the approval workflow using the initial data as it could be already edited
	Workflow.removeApprovalWorkflow(route.params.policyID, initialApprovalWorkflow);
	});

[s77rt]

@cretadn22, @nkdengineer, @Nodebrute and @bernhardoj Thank you for the proposals. The RCA makes sense in general but the not found view should only be shown if the approvalWorkflow data is available and we have no members.

[s77rt]

@abzokhattab Thanks for the proposal. The RCA makes sense. I find your solution the easiest to implement even though it will cause blank space to appear on animation instead of the not found view but I don't think thats something that we should optimize for. Overall looks good to me.

🎀 👀 🎀 C+ reviewed (see #50094 (comment))
Link to proposal

[melvin-bot]

Triggered auto assignment to @stitesExpensify, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[s77rt]

@abzokhattab Actually I found that the solution is not enough. We get a weird view after selecting some users and going back

Screen.Recording.2024-10-03.at.7.12.54.PM.mov

[nkdengineer]

@s77rt The selected proposal is not enough because we still need to fix the approval workflow will be cleared briefly before we go back to the workflow page.

[abzokhattab]

Thanks @s77rt for the review, i extended my proposal to cover the other problem where the members list becomes empty when going back

to solve it i used a help state isNavigatingBack that would allow to freeze the members list while clearing the flow.

the important this in my proposal is that it keeps the synchronous nature of clearing the data first then going back, not the other way around.

POC

Screen.Recording.2024-10-03.at.9.15.32.PM.mov

[s77rt]

Since we need another "complementary" solution, I'm giving this another review:

• @cretadn22 Proposal is not complete (only handles the mWeb cause)
• @nkdengineer Let's avoid delays i.e. InteractionManager.runAfterInteractions
• @Nodebrute Looks good to me
• @bernhardoj both setNavigationActionToMicrotaskQueue and InteractionManager.runAfterInteractions add delays and should be avoided

[s77rt]

@abzokhattab The new changes making this more complex and it feels like band-aid solution

[abzokhattab]

I see, thanks for the review!

[s77rt]

@Nodebrute's proposal looks good to me

cc @stitesExpensify

[nkdengineer]

To do that we can use set method here which is faster because I see that we always pass full approval workflow data to this function.

@s77rt For the selected proposal, with the main or alternative we also need to use set method in setApprovalWorkflow function which is the key to fixing the Android native bug and I'm the first one who suggested this. For the mweb bug, I think moving clearApprovalWorkflow or delaying the call of this function is fine because we don't call any API.

[s77rt]

@nkdengineer I believe the key in @Nodebrute's proposal is to not call Workflow.clearApprovalWorkflow on going back. Using Onyx.set is not mandatory.

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.48-2 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• fix approval flow #50437

If no regressions arise, payment will be issued on 2024-10-22. 🎊

For reference, here are some details about the assignees on this issue:

• @s77rt requires payment through NewDot Manual Requests
• @Nodebrute requires payment automatic offer (Contributor)

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@s77rt] The PR that introduced the bug has been identified. Link to the PR: The PR that introduced the bug has been identified: [CRITICAL] Implement <WorkspaceWorkflowsApprovalsExpensesFromPage /> for Member Selection #46484
• [@s77rt] 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: The offending PR has been commented on: [CRITICAL] Implement <WorkspaceWorkflowsApprovalsExpensesFromPage /> for Member Selection #46484 (comment)
• [@s77rt] A discussion in #expensify-bugs 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: Not needed
• [@s77rt] Determine if we should create a regression test for this bug: No
• [@s77rt] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@garrettmknight] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[s77rt]

• The PR that introduced the bug has been identified: [CRITICAL] Implement <WorkspaceWorkflowsApprovalsExpensesFromPage /> for Member Selection #46484
• The offending PR has been commented on: [CRITICAL] Implement <WorkspaceWorkflowsApprovalsExpensesFromPage /> for Member Selection #46484 (comment)
• A discussion in #expensify-bugs has been started: Not needed
• Determine if we should create a regression test for this bug: No

[garrettmknight]

Payment summary:

• Contributor: @Nodebrute $250 via NewDot
• Contributor+: @s77rt $250

[garrettmknight]

Paid out in Upwork. @s77rt request when you're ready.

[tgolen]

Reopening for @s77rt and @garrettmknight to complete the regression checklist.

[s77rt]

@tgolen Completed here #50094 (comment)

[JmillsExpensify]

$250 approved for @s77rt

[tgolen]

Thanks @s77rt, and apologies that I missed it. @garrettmknight can you please be sure to copy those answers into the actual checklist and then complete it out?

@s77rt Can you give a little more context behind the regression? Would you say it was caused by one of these?

• Initial design was incorrect or incomplete
• Bug introduced during implementation
• Bug caused by some unrelated change
• Other (please specify)

[s77rt]

I would say it's a bug that is introduced during implementation but it could also be that the design was incorrect if it was specific about the area where the bug occurred (I don't have access to the doc to confirm).

[melvin-bot]

This issue has not been updated in over 15 days. @garrettmknight, @s77rt, @stitesExpensify, @Nodebrute 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!