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.33k stars 2.76k forks source link

Switch to OD - Background changes to Profile after proceeding to "Before you go" page #48295

Closed lanitochka17 closed 1 week 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.26-2 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: applausetester+kh050806@applause.expensifail.com Issue reported by: Applause - Internal Team

Action Performed:

  1. Go to staging.new.expensify.com
  2. Go to Account settings
  3. Go to Workspaces
  4. Click Switch to Expensify Classic
  5. Select an option
  6. Click Next

Expected Result:

The background will remain in Workspaces page

Actual Result:

The background changes to Profile after proceeding to "Before you go" page

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/784425e5-77cd-45cb-b7b8-6c99bb52e7c9

View all open jobs on GitHub

melvin-bot[bot] commented 2 weeks ago

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

Nodebrute commented 2 weeks ago

Proposal

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

Background changes to Profile after proceeding to "Before you go" page

What is the root cause of that problem?

We are adding these pages in profile root https://github.com/Expensify/App/blob/9ddca5c4ac8168ad36da570192a6e4ef10820e67/src/libs/Navigation/linkingConfig/CENTRAL_PANE_TO_RHP_MAPPING.ts#L22-L24

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

We can remove these pages from profile root https://github.com/Expensify/App/blob/9ddca5c4ac8168ad36da570192a6e4ef10820e67/src/libs/Navigation/linkingConfig/CENTRAL_PANE_TO_RHP_MAPPING.ts#L22-L24

What alternative solutions did you explore? (Optional)

Nodebrute commented 2 weeks ago

I think this is expected https://github.com/Expensify/App/pull/47115

bernhardoj commented 2 weeks ago

Proposal

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

The screen below the overlay changes to profile when go to switch to OD response page.

What is the root cause of that problem?

The survey page (reason, response, and confirm) mapped to the profile page. https://github.com/Expensify/App/blob/9e82ed89b07c62ccb5d21a70398975bf0c37d11b/src/libs/Navigation/linkingConfig/CENTRAL_PANE_TO_RHP_MAPPING.ts#L5-L25

So, when we open those pages, the matching root will be the profile page. https://github.com/Expensify/App/blob/9e82ed89b07c62ccb5d21a70398975bf0c37d11b/src/libs/Navigation/linkingConfig/getAdaptedStateFromPath.ts#L192

It doesn't happen on the reason page (the 1st page) because somehow the state here is undefined. https://github.com/Expensify/App/blob/9e82ed89b07c62ccb5d21a70398975bf0c37d11b/src/libs/Navigation/AppNavigator/createCustomStackNavigator/CustomRouter.ts#L38-L41

Switch to OD page can be accessed from many pages, not just profiles, so always mapping it to the profile page isn't eniterly correct.

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

To fix this, we can add a backTo params to each page, so the matching root will be based on the backTo. https://github.com/Expensify/App/blob/9e82ed89b07c62ccb5d21a70398975bf0c37d11b/src/libs/Navigation/linkingConfig/getAdaptedStateFromPath.ts#L112-L116

Steps:

  1. First, update the reason route to accept backTo. Response and confirm pages already accept backTo. https://github.com/Expensify/App/blob/9e82ed89b07c62ccb5d21a70398975bf0c37d11b/src/ROUTES.ts#L222-L231

  2. Pass the current route as the backTo to the reason page. https://github.com/Expensify/App/blob/9e82ed89b07c62ccb5d21a70398975bf0c37d11b/src/pages/settings/InitialSettingsPage.tsx#L144-L148

action: () => Navigation.navigate(ROUTES.SETTINGS_EXIT_SURVEY_REASON.getRoute(Navigation.getActiveRoute())),
  1. Keep passing the backTo to response and confirm page. https://github.com/Expensify/App/blob/9e82ed89b07c62ccb5d21a70398975bf0c37d11b/src/pages/settings/ExitSurvey/ExitSurveyReasonPage.tsx#L72
    Navigation.navigate(ROUTES.SETTINGS_EXIT_SURVEY_RESPONSE.getRoute(reason, route.params?.backTo));

https://github.com/Expensify/App/blob/9e82ed89b07c62ccb5d21a70398975bf0c37d11b/src/pages/settings/ExitSurvey/ExitSurveyResponsePage.tsx#L64-L66

Navigation.navigate(ROUTES.SETTINGS_EXIT_SURVEY_CONFIRM.getRoute(route.params?.backTo));
  1. Currently, we have a logic to modify the backTo on response and confirm pages when offline which still causes the issue. https://github.com/Expensify/App/blob/9e82ed89b07c62ccb5d21a70398975bf0c37d11b/src/pages/settings/ExitSurvey/ExitSurveyResponsePage.tsx#L50-L56

The reason for that is we want to close the page when offline instead of going back to the prev page. Instead of modifying the backTo, we can just conditionally do the navigation to either dismiss or going back.

For response page

onBackButtonPress={() => {
    if (isOffline) Navigation.dismissModal();
    else Navigation.goBack(ROUTES.SETTINGS_EXIT_SURVEY_REASON.getRoute(backTo));
}}

For confirm page

onBackButtonPress={() => {
    if (isOffline || !exitReason) Navigation.dismissModal();
    else Navigation.goBack(ROUTES.SETTINGS_EXIT_SURVEY_RESPONSE.getRoute(exitReason, route.params?.backTo));
}}
  1. Last, even though we already pass the backTo, it will still fail because the central pane route doesn't have a state. https://github.com/Expensify/App/blob/9e82ed89b07c62ccb5d21a70398975bf0c37d11b/src/libs/Navigation/linkingConfig/getAdaptedStateFromPath.ts#L130-L133

Central pane isn't a navigator anymore, so it doesn't have the state just like other navigator (RHP, full screen) that contains the child screens. To fix this, we can check for state only for full screen navigator.

melvin-bot[bot] commented 1 week ago

@VictoriaExpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

VictoriaExpensify commented 1 week ago

I need to double-check the expected behaviour as @Nodebrute has mentioned here

VictoriaExpensify commented 1 week ago

I agree that this is expected, and I think it's way to minor to worry about at this point - it does not have any impact on the user experience from what I can see. I'm going to close this out.

bernhardoj commented 1 week ago

@VictoriaExpensify hi, I don't agree that it's expected. In https://github.com/Expensify/App/pull/47115, the issue is that, after refreshing, the report screen is shown and the step performed specifically done from the profile page (open the switch to expensify while on the profile page) while in this issue, we open the switch to expensify while on other pages and expect the page to not change to profile.

We fixed this kind of issue here: https://github.com/Expensify/App/issues/47612, https://github.com/Expensify/App/issues/47069, https://github.com/Expensify/App/issues/46773