Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
2 of 6 tasks
lanitochka17 opened this issue Aug 29, 2024 · 8 comments
Closed
2 of 6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2

Comments

@lanitochka17
Copy link

lanitochka17 commented Aug 29, 2024

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?

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

Screenshots/Videos

Add any screenshot/video evidence

Bug6586681_1724942316594.20240829_223508.mp4

View all open jobs on GitHub

@lanitochka17 lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Aug 29, 2024
Copy link

melvin-bot bot commented Aug 29, 2024

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
Copy link
Contributor

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

SCREENS.SETTINGS.EXIT_SURVEY.REASON,
SCREENS.SETTINGS.EXIT_SURVEY.RESPONSE,
SCREENS.SETTINGS.EXIT_SURVEY.CONFIRM,

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

We can remove these pages from profile root

SCREENS.SETTINGS.EXIT_SURVEY.REASON,
SCREENS.SETTINGS.EXIT_SURVEY.RESPONSE,
SCREENS.SETTINGS.EXIT_SURVEY.CONFIRM,

What alternative solutions did you explore? (Optional)

@Nodebrute
Copy link
Contributor

I think this is expected #47115

@bernhardoj
Copy link
Contributor

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.

[SCREENS.SETTINGS.PROFILE.ROOT]: [
SCREENS.SETTINGS.PROFILE.DISPLAY_NAME,
SCREENS.SETTINGS.PROFILE.CONTACT_METHODS,
SCREENS.SETTINGS.PROFILE.CONTACT_METHOD_DETAILS,
SCREENS.SETTINGS.PROFILE.NEW_CONTACT_METHOD,
SCREENS.SETTINGS.PROFILE.STATUS_CLEAR_AFTER,
SCREENS.SETTINGS.PROFILE.STATUS_CLEAR_AFTER_DATE,
SCREENS.SETTINGS.PROFILE.STATUS_CLEAR_AFTER_TIME,
SCREENS.SETTINGS.PROFILE.STATUS,
SCREENS.SETTINGS.PROFILE.PRONOUNS,
SCREENS.SETTINGS.PROFILE.TIMEZONE,
SCREENS.SETTINGS.PROFILE.TIMEZONE_SELECT,
SCREENS.SETTINGS.PROFILE.LEGAL_NAME,
SCREENS.SETTINGS.PROFILE.DATE_OF_BIRTH,
SCREENS.SETTINGS.PROFILE.ADDRESS,
SCREENS.SETTINGS.PROFILE.ADDRESS_COUNTRY,
SCREENS.SETTINGS.SHARE_CODE,
SCREENS.SETTINGS.EXIT_SURVEY.REASON,
SCREENS.SETTINGS.EXIT_SURVEY.RESPONSE,
SCREENS.SETTINGS.EXIT_SURVEY.CONFIRM,
],

So, when we open those pages, the matching root will be the profile page.

let matchingRootRoute = getMatchingRootRouteForRHPRoute(focusedRHPRoute);

It doesn't happen on the reason page (the 1st page) because somehow the state here is undefined.

// If the state of the last path is not defined the getPathFromState won't work correctly.
if (!state?.routes.at(-1)?.state) {
return;
}

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.

function getMatchingRootRouteForRHPRoute(route: NavigationPartialRoute): NavigationPartialRoute<CentralPaneName | typeof NAVIGATORS.FULL_SCREEN_NAVIGATOR> | undefined {
// Check for backTo param. One screen with different backTo value may need diferent screens visible under the overlay.
if (route.params && 'backTo' in route.params && typeof route.params.backTo === 'string') {
const stateForBackTo = getStateFromPath(route.params.backTo, config);
if (stateForBackTo) {

Steps:

  1. First, update the reason route to accept backTo. Response and confirm pages already accept backTo.

    App/src/ROUTES.ts

    Lines 222 to 231 in 9e82ed8

    SETTINGS_EXIT_SURVEY_REASON: 'settings/exit-survey/reason',
    SETTINGS_EXIT_SURVEY_RESPONSE: {
    route: 'settings/exit-survey/response',
    getRoute: (reason?: ValueOf<typeof CONST.EXIT_SURVEY.REASONS>, backTo?: string) =>
    getUrlWithBackToParam(`settings/exit-survey/response${reason ? `?reason=${encodeURIComponent(reason)}` : ''}`, backTo),
    },
    SETTINGS_EXIT_SURVEY_CONFIRM: {
    route: 'settings/exit-survey/confirm',
    getRoute: (backTo?: string) => getUrlWithBackToParam('settings/exit-survey/confirm', backTo),
    },

  2. Pass the current route as the backTo to the reason page.

    {
    translationKey: 'exitSurvey.goToExpensifyClassic',
    icon: Expensicons.ExpensifyLogoNew,
    routeName: ROUTES.SETTINGS_EXIT_SURVEY_REASON,
    },

action: () => Navigation.navigate(ROUTES.SETTINGS_EXIT_SURVEY_REASON.getRoute(Navigation.getActiveRoute())),
  1. Keep passing the backTo to response and confirm page.
    Navigation.navigate(ROUTES.SETTINGS_EXIT_SURVEY_RESPONSE.getRoute(reason, ROUTES.SETTINGS_EXIT_SURVEY_REASON));
Navigation.navigate(ROUTES.SETTINGS_EXIT_SURVEY_RESPONSE.getRoute(reason, route.params?.backTo));

const submitForm = useCallback(() => {
ExitSurvey.saveResponse(draftResponse);
Navigation.navigate(ROUTES.SETTINGS_EXIT_SURVEY_CONFIRM.getRoute(ROUTES.SETTINGS_EXIT_SURVEY_RESPONSE.route));

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.
    const {isOffline} = useNetwork({
    onReconnect: () => {
    navigation.setParams({
    backTo: ROUTES.SETTINGS_EXIT_SURVEY_REASON,
    });
    },
    });

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.
    // If we know that backTo targets the root route (central pane or full screen) we want to use it.
    if (centralPaneOrFullScreenNavigator && centralPaneOrFullScreenNavigator.state) {
    return centralPaneOrFullScreenNavigator as NavigationPartialRoute<CentralPaneName | typeof NAVIGATORS.FULL_SCREEN_NAVIGATOR>;
    }

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 melvin-bot bot added the Overdue label Sep 2, 2024
Copy link

melvin-bot bot commented Sep 2, 2024

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

@VictoriaExpensify
Copy link
Contributor

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

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Sep 3, 2024
@VictoriaExpensify
Copy link
Contributor

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
Copy link
Contributor

@VictoriaExpensify hi, I don't agree that it's expected. In #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:
#47612, #47069, #46773

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2
Projects
None yet
Development

No branches or pull requests

4 participants