search

Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.49k stars 2.84k forks source link

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

Open lanitochka17 opened 2 weeks ago

lanitochka17 commented 2 weeks ago

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?

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/116d6380-9b59-46f2-907f-f5f120126219

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 OwnerCurrent Issue Owner: @garrettmknight
melvin-bot[bot] commented 2 weeks ago

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 commented 2 weeks ago

@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 commented 2 weeks ago

Proposal

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

What is the root cause of that problem?

https://github.com/Expensify/App/blob/66d1025282196cbe175f257dfc6d25ee19a495f0/src/pages/workspace/workflows/approvals/WorkspaceWorkflowsApprovalsExpensesFromPage.tsx#L149-L155

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[bot] commented 2 weeks ago

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

melvin-bot[bot] commented 2 weeks ago

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

nkdengineer commented 2 weeks ago

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

https://github.com/Expensify/App/blob/f325244648646bff8fcba62e60979a530f14d2d3/src/pages/workspace/workflows/approvals/WorkspaceWorkflowsApprovalsExpensesFromPage.tsx#L149-L156

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

https://github.com/Expensify/App/blob/19a137d26fee56f913eaec7c9b896915ddb002a9/src/pages/workspace/workflows/WorkspaceWorkflowsPage.tsx#L103-L109

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]);

https://github.com/Expensify/App/blob/f325244648646bff8fcba62e60979a530f14d2d3/src/pages/workspace/workflows/approvals/WorkspaceWorkflowsApprovalsExpensesFromPage.tsx#L149-L156

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

https://github.com/Expensify/App/blob/19a137d26fee56f913eaec7c9b896915ddb002a9/src/pages/workspace/workflows/WorkspaceWorkflowsPage.tsx#L103-L109

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.

https://github.com/Expensify/App/blob/19a137d26fee56f913eaec7c9b896915ddb002a9/src/libs/actions/Workflow.ts#L337

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.

https://github.com/Expensify/App/blob/19a137d26fee56f913eaec7c9b896915ddb002a9/src/pages/workspace/workflows/WorkspaceWorkflowsPage.tsx#L103-L109

What alternative solutions did you explore? (Optional)

abzokhattab commented 2 weeks ago

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: https://github.com/Expensify/App/blob/f325244648646bff8fcba62e60979a530f14d2d3/src/pages/workspace/workflows/approvals/WorkspaceWorkflowsApprovalsExpensesFromPage.tsx#L149-L156 https://github.com/Expensify/App/blob/9d252e8b37c441d62d6b299421870d28fe2ed398/src/libs/actions/Workflow.ts#L340-L342 This causes the shouldShowListEmptyContent variable to resolve to undefined, not false: https://github.com/Expensify/App/blob/9d252e8b37c441d62d6b299421870d28fe2ed398/src/pages/workspace/workflows/approvals/WorkspaceWorkflowsApprovalsExpensesFromPage.tsx#L63 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: https://github.com/Expensify/App/blob/0993f367649a5c978cc870347dbc93eaf5bd8736/src/components/SelectionList/BaseSelectionList.tsx#L103 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: https://github.com/Expensify/App/blob/9d252e8b37c441d62d6b299421870d28fe2ed398/src/pages/workspace/workflows/approvals/WorkspaceWorkflowsApprovalsExpensesFromPage.tsx#L63 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 commented 2 weeks ago

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. https://github.com/Expensify/App/blob/19a137d26fee56f913eaec7c9b896915ddb002a9/src/pages/workspace/workflows/approvals/WorkspaceWorkflowsApprovalsExpensesFromPage.tsx#L149-L154

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 https://github.com/Expensify/App/blob/19a137d26fee56f913eaec7c9b896915ddb002a9/src/pages/workspace/workflows/approvals/WorkspaceWorkflowsApprovalsExpensesFromPage.tsx#L149-L154

  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 https://github.com/Expensify/App/blob/19a137d26fee56f913eaec7c9b896915ddb002a9/src/libs/actions/Workflow.ts#L336-L338

bernhardoj commented 2 weeks ago

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. https://github.com/Expensify/App/blob/e61ee7fbe60acc12879b9d5cc6ef92c4a0b98caa/src/pages/workspace/workflows/WorkspaceWorkflowsPage.tsx#L103-L108

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. https://github.com/Expensify/App/blob/e61ee7fbe60acc12879b9d5cc6ef92c4a0b98caa/src/pages/workspace/workflows/approvals/WorkspaceWorkflowsApprovalsExpensesFromPage.tsx#L149-L154

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. https://github.com/Expensify/App/blob/e61ee7fbe60acc12879b9d5cc6ef92c4a0b98caa/src/libs/Navigation/setNavigationActionToMicrotaskQueue.ts#L1-L5

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. https://github.com/Expensify/App/blob/e61ee7fbe60acc12879b9d5cc6ef92c4a0b98caa/src/pages/workspace/workflows/approvals/WorkspaceWorkflowsApprovalsEditPage.tsx#L63-L67

s77rt commented 2 weeks ago

@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 commented 2 weeks ago

@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 https://github.com/Expensify/App/issues/50094#issuecomment-2392032487) Link to proposal

melvin-bot[bot] commented 2 weeks ago

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

s77rt commented 2 weeks ago

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

https://github.com/user-attachments/assets/f1e724ae-33bc-49e1-b035-d724a9d58692

nkdengineer commented 2 weeks ago

@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 commented 2 weeks ago

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

https://github.com/user-attachments/assets/8ebe778e-5c5e-483c-949e-66e4dfc7c366

s77rt commented 2 weeks ago

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

s77rt commented 2 weeks ago

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

abzokhattab commented 2 weeks ago

I see, thanks for the review!

s77rt commented 2 weeks ago

@Nodebrute's proposal looks good to me

cc @stitesExpensify

nkdengineer commented 2 weeks ago

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 commented 2 weeks ago

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

nkdengineer commented 2 weeks ago

@s77rt Using Onyx.set in setApprovalWorkflow is mandatory to fix the Android native bug.

Nodebrute commented 2 weeks ago

@nkdengineer Using a set is part of my alternate proposal and differs from your proposal.

s77rt commented 2 weeks ago

@nkdengineer

Using Onyx.set in setApprovalWorkflow is mandatory to fix the Android native bug.

Not sure that's the case based on my testings. Can you double check?

Nodebrute commented 2 weeks ago

@s77rt We need set to fix this bug completely. And my alternate solution is complete. Let me explain my alternate solution.

  1. We should remove clearApprovalWorkflow from WorkspaceWorkflowsApprovalsExpensesFromPage and only use goBack() as mentioned in my main proposal.
  2. We can use set method in setApprovalWorkflow.

Note: If we use a set, we don't need to add clearApprovalWorkflow anywhere, because each time the set is called, it will reset the ONYXKEYS.APPROVAL_WORKFLOW and I believe my alternate proposal is different from @nkdengineer's proposal

melvin-bot[bot] commented 2 weeks ago

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

s77rt commented 2 weeks ago

@stitesExpensify I think we can go ahead and assign @Nodebrute (proposal)

melvin-bot[bot] commented 2 weeks ago

📣 @Nodebrute 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

melvin-bot[bot] commented 1 week ago

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

melvin-bot[bot] commented 1 week ago

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

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

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

melvin-bot[bot] commented 1 week ago

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 commented 5 days ago