Search - App navigates to Inbox page when submitting an expense via QAB

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.44-0 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 Email or phone of affected tester (no customers): applausetester+shsb22cp111@applause.expensifail.com Issue reported by: Applause - Internal Team

Issue found when executing PR https://github.com/Expensify/App/pull/50028

Action Performed:

Login to any account
Switch to the Search page tab
Click the global create button (FAB) > Submit expense > Manual > Enter amount > Choose recipient and submit
Notice that you remain on the search page
Click on the FAB button again > Quick Action Button > Enter amount and submit

Expected Result:

The user remains on the search page and the newly added expense appears in the table and the row is briefly highlighted

Actual Result:

The user is navigated to inbox page, there is an inconsistency when submitting an expense via FAB and QAB

Workaround:

Unknown

Platforms:

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

[ ] Android: Native
[x] Android: mWeb Chrome
[ ] iOS: Native
[ ] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/8332af02-fa13-4c54-8496-908a3e9b7092

View all open jobs on GitHub

melvin-bot[bot] commented 2 weeks ago

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

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

lanitochka17 commented 2 weeks ago

We think that this bug might be related to #wave-control

trjExpensify commented 2 weeks ago

Yep, like the create options, we'd want someone using the QAB to remain on the search page if they created from the search page as well.

@mananjadhav can you take this?

mananjadhav commented 2 weeks ago

Will take this up.

mananjadhav commented 2 weeks ago

I have a solution that works. This definitely seems to be related to Navigation so I was verifying if it would break something. I also feel we should get an opinion from the expert agency as it relates to Navigation.

Proposal

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

When we click on the "Quick action" from search or other pages on FAB, it navigates to "Inbox" page.

What is the root cause of that problem?

We're calling IOU.startMoneyRequest with parameters IOU.startMoneyRequest(CONST.IOU.TYPE.SUBMIT, quickActionReportID, CONST.IOU.REQUEST_TYPE.MANUAL, true).
When this is called, the navigation path doesn't directly called the /create/submit/start/{reportId}/manual. Instead this calls /create/submit/{reportId}?initial=true&screen=scan&params=object&path=/create/submit/start/.... I think this is related to the linkTo implementation.

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

For all quick action cases in FloatingActionButtonAndPopover.tsx, we can get rid of the third parameter requestType and set it as undefined.

https://github.com/Expensify/App/blob/b027baaf086c4a8f57bec48dd1cbbfb3c317726b/src/pages/home/sidebar/SidebarScreen/FloatingActionButtonAndPopover.tsx#L249-L282

The reason I suggested this is because we the function startMoneyRequest has four parameters (iouType: ValueOf<typeof CONST.IOU.TYPE>, reportID: string, requestType?: IOURequestType, skipConfirmation = false) but across the application we never use the requestType argument.

Here's a quick video of the implementation.

https://github.com/user-attachments/assets/845e6ced-77c4-460b-9b18-917b9474c19f

What alternative solutions did you explore? (Optional)

Fix this at the navigation level.

c3024 commented 2 weeks ago

Proposal

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

BottomTab and CentralPane change when we open RHP with QAB in Search Central Pane.

What is the root cause of that problem?

When we start the flow with QAB the report in the URL param is a valid report and it finds a matching route for RHP route here. https://github.com/Expensify/App/blob/66aefc37f0d38ea2a2f2246cb037db26f3686cba/src/libs/Navigation/linkingConfig/getAdaptedStateFromPath.ts#L162-L164 and this is pushed to routes with metaInfo as true here. https://github.com/Expensify/App/blob/66aefc37f0d38ea2a2f2246cb037db26f3686cba/src/libs/Navigation/linkingConfig/getAdaptedStateFromPath.ts#L215-L225 Since metaInfo fields are both true, we find if there are diff actions that need to be dispatched here and we dispatch them. https://github.com/Expensify/App/blob/66aefc37f0d38ea2a2f2246cb037db26f3686cba/src/libs/Navigation/linkTo/index.ts#L136-L143 In this case, the bottom tab navigators and central panes are different (Search_Bottom_Tab/Home and Search_Central_Pane and Report respectively). So, these diffs are dispatched which cause the QAB report and Home to be added for the Central Pane and Bottom Tab respectively.

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

When on Search_Central_Pane I have not found any cases where we need to change the bottom tab or central pane when opening RHP. So we can dispatch the diffs only when when do not open the RHP when on Search Central Pane. So, we can wrap this in a check https://github.com/Expensify/App/blob/66aefc37f0d38ea2a2f2246cb037db26f3686cba/src/libs/Navigation/linkTo/index.ts#L135-L143

 if (!(isSideModalNavigator(action.payload.name) && lastRoute?.name === "Search_Central_Pane")) {
            const {adaptedState, metainfo} = getAdaptedStateFromPath(path, linkingConfig.config);
            if (adaptedState && (metainfo.isCentralPaneAndBottomTabMandatory || metainfo.isFullScreenNavigatorMandatory)) {
                const diff = getPartialStateDiff(rootState, adaptedState as State<RootStackParamList>, metainfo);
                const diffActions = getActionsFromPartialDiff(diff);
                for (const diffAction of diffActions) {
                    root.dispatch(diffAction);
                }
            }
}

What alternative solutions did you explore? (Optional)

c3024 commented 2 weeks ago

I can work on this as contributor and @mananjadhav as C+ if this solution above looks good.

mananjadhav commented 2 weeks ago

I was working on a solution to update linkTo. Let me review your proposal.

mananjadhav commented 2 weeks ago

Yeah I think @c3024's solution works.

where we need to change the bottom tab or central pane when opening RHP.

Just need to clarify this. Otherwise we'll need to modify the startMoneyRequest calls as I recommended.

c3024 commented 2 weeks ago

My bad! My solution is not good enough. The issue can happen when we click on QAB from other pages like the settings page with related centre panes.

I think we should keep a global value for tracking if we clicked on QAB and just ignore adding the report here

https://github.com/Expensify/App/blob/66aefc37f0d38ea2a2f2246cb037db26f3686cba/src/libs/Navigation/linkingConfig/getAdaptedStateFromPath.ts#L162-L164

even if it matches with an existing report like this.

trjExpensify commented 2 weeks ago

How did we approach this for the create flow when actioned from the search page, wouldn't we follow that? I.e if you're on the search page and choose Submit expense or the QAB, the outcome is the same on completion. You remain on the search page, and the expense is highlighted in the table.

c3024 commented 2 weeks ago

We generate the navigation state required based on the URL. When we use QAB, we have a valid report ID in the URL. In normal cases (that is, other than QAB), we do not have a valid report ID in the URL. The report is chosen on the participants' page.

When we deeplink into an RHP flow and the URL has an existing report ID, we show the report as an underlay when we navigate to it. Therefore, we add this report screen to the navigation state when the URL has an existing report ID.

The URL is the same for the QAB flow as well, so our navigation code pushes the existing report to the central pane, and this navigation to a different bottom navigator (HOME) and central pane (REPORT) occurs. When we click on the normal 'Submit Expense' flow, there is no valid report ID in the URL, and no report route is pushed into the state. This is peculiar to QAB alone due to the initial URL having a valid report ID.

c3024 commented 2 weeks ago

The bug occurs in other pages also, not just in the Search Page.

https://github.com/user-attachments/assets/1f9658d8-1e3e-48ee-8b0d-5dbb9cc55b9f

mananjadhav commented 2 weeks ago

How did we approach this for the create flow when actioned from the search page, wouldn't we follow that?

@c3024 is right. I think it's just how we've implemented @trjExpensify. Hence we have both the options. Either we go ahead an update the Navigation which @c3024 has recommended or what you asked and that's what my proposal does.

trjExpensify commented 2 weeks ago

I think it's just how we've implemented @trjExpensify.

Sorry to be clear, this "stay on the search page if you Submit expense from the search page" feature is a relatively new feature in the last couple of weeks. So I was wondering how that was implemented, and why we can't do the same for the QAB. CC: @ikevin127 who might have some context on this to chime in as well.

trjExpensify commented 2 weeks ago

Whatcha' think @mananjadhav?

mananjadhav commented 1 week ago

I think we need to decide whether we want to update the navigation for this. I feel we should do that.

We have both the proposals and I think we should ask an internal engineer to decide this.

🎀 👀 🎀 C+ reviewed (adding this to assign an internal engineer). We can decide between @c3024 or me to implement + review the PR.

melvin-bot[bot] commented 1 week ago

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

francoisl commented 1 week ago

I'm leaning towards @mananjadhav's solution as it simplifies the code without adding a new global tracking value, but one clarification about this please:

The reason I suggested this is because we the function startMoneyRequest has four parameters (iouType: ValueOf, reportID: string, requestType?: IOURequestType, skipConfirmation = false) but across the application we never use the requestType argument.

Are you suggesting also removing this switch block and just keeping the default Navigation.navigate(..) then? Would the QAB still work if your quick action is Track Distance or Scan?

c3024 commented 1 week ago

Instead of keeping a global variable we can pass a param to the route like ?isQuickAction=true and use that to check if we need to add a central report pane or not like this.

In case if we stumble into something difficult with the simplification of code as specified above, we can try this route param approach also instead of using a global variable.

mananjadhav commented 1 week ago

@francoisl Yes it works even if the quick action is Track Distance or Scan. My proposal was to pass undefined as the param. I tried removing the switch and it worked fine too. My concern with switch case removal is whether it was added for deeplinking. But as I said I checked all the method calls and I didn't find a single instance where we're setting the third and fourth param except for QAB.

francoisl commented 1 week ago

Ok sounds good, let's go with your solution then @mananjadhav.

Not sure if we need to assign another C+ though in this case, for the review? 🤔

mananjadhav commented 1 week ago

I’ll implement tn

mananjadhav commented 1 week ago

I’ll implement the PR, but we can assign @c3024 if we want a C+ review.

mananjadhav commented 1 week ago

Ohh I didn't see the emoji reactions. I'll get started on the PR and finish by tomorrow.

trjExpensify commented 5 days ago

How you looking on a PR for this?

melvin-bot[bot] commented 4 days ago

@francoisl @mananjadhav @trjExpensify @c3024 this issue was created 2 weeks ago. Are we close to a solution? Let's make sure we're treating this as a top priority. Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

mananjadhav commented 4 days ago

I am out today, but I'll finish this by Friday.

mananjadhav commented 4 days ago

@francoisl I am getting a lint error for withOnyx deprecation. Should we be fixing this in the current PR?

/home/runner/work/App/App/src/pages/home/sidebar/SidebarScreen/FloatingActionButtonAndPopover.tsx
  513:16  error  'withOnyx' is deprecated. Use `useOnyx` instead of `withOnyx` whenever possible.

I can see several onyx keys being used here.

export default withOnyx<FloatingActionButtonAndPopoverProps & RefAttributes<FloatingActionButtonAndPopoverRef>, FloatingActionButtonAndPopoverOnyxProps>({
    allPolicies: {
        key: ONYXKEYS.COLLECTION.POLICY,
        selector: policySelector,
    },
    isLoading: {
        key: ONYXKEYS.IS_LOADING_APP,
    },
    quickAction: {
        key: ONYXKEYS.NVP_QUICK_ACTION_GLOBAL_CREATE,
    },
    quickActionReport: {
        key: ({quickAction}) => `${ONYXKEYS.COLLECTION.REPORT}${quickAction?.chatReportID}`,
    },
    quickActionPolicy: {
        key: ({quickActionReport}) => `${ONYXKEYS.COLLECTION.POLICY}${quickActionReport?.policyID}`,
    },
    personalDetails: {
        key: ONYXKEYS.PERSONAL_DETAILS_LIST,
    },
    session: {
        key: ONYXKEYS.SESSION,
    },
    hasSeenTrackTraining: {
        key: ONYXKEYS.NVP_HAS_SEEN_TRACK_TRAINING,
    },
})(forwardRef(FloatingActionButtonAndPopover));

francoisl commented 4 days ago

Yes I think the current expectation is that we fix those when we run into them – see https://expensify.slack.com/archives/C01GTK53T8Q/p1726169039084589

mananjadhav commented 4 days ago

Thanks, I'll try to cover these and raise a PR by weekend then.

melvin-bot[bot] commented 20 hours ago

@francoisl, @mananjadhav, @trjExpensify, @c3024 Whoops! This issue is 2 days overdue. Let's get this updated quick!

trjExpensify commented 16 hours ago

@mananjadhav mind providing an update here? Thanks!

Expensify / App