[HOLD for payment 2024-10-22] [$250] Book travel - Report screen changes to profile view when clicking Country field

@trjExpensify

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

Action Performed:

Go to staging.new.expensify.com
Go to FAB > Book travel.
Click Book travel.
Click Country.

Expected Result:

The report screen in the background will not change to profile page.

Actual Result:

The report screen in the background changes to profile page while LHN remains the same after opening Country RHP.

Workaround:

Unknown

Platforms:

Screenshots/Videos

Bug6623236_1727962477743.20241003_211113.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~021843246313566765560
Upwork Job ID: 1843246313566765560
Last Price Increase: 2024-10-07
Automatic offers:

dukenv0307 | Contributor | 104317443

Issue Owner

Current Issue Owner: @trjExpensify

@trjExpensify

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

We think that this bug might be related to #vip-travel

@trjExpensify

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

Proposal

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

Report screen changes to profile screen when open the Country page from the book travel page.

What is the root cause of that problem?

By default, the address country RHP maps to the profile screen.

App/src/libs/Navigation/linkingConfig/CENTRAL_PANE_TO_RHP_MAPPING.ts

Lines 5 to 21 in 91f610c

    
           [SCREENS.SETTINGS.PROFILE.ROOT]: [ 
        
               SCREENS.SETTINGS.PROFILE.DISPLAY_NAME, 
        
               SCREENS.SETTINGS.PROFILE.CONTACT_METHODS, 
        
               SCREENS.SETTINGS.PROFILE.CONTACT_METHOD_DETAILS, 
        
               SCREENS.SETTINGS.PROFILE.CONTACT_METHOD_VALIDATE_ACTION, 
        
               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,

But we have a logic to find the matching central pane screen based on the backTo params.

App/src/libs/Navigation/linkingConfig/getAdaptedStateFromPath.ts

Lines 114 to 139 in 91f610c

    
           if (route.params && 'backTo' in route.params && typeof route.params.backTo === 'string') { 
        
               const stateForBackTo = getStateFromPath(route.params.backTo, config); 
        
               if (stateForBackTo) { 
        
                   // If there is rhpNavigator in the state generated for backTo url, we want to get root route matching to this rhp screen. 
        
                   const rhpNavigator = stateForBackTo.routes.find((rt) => rt.name === NAVIGATORS.RIGHT_MODAL_NAVIGATOR); 
        
                   if (rhpNavigator && rhpNavigator.state) { 
        
                       const isRHPinState = stateForBackTo.routes.at(0)?.name === NAVIGATORS.RIGHT_MODAL_NAVIGATOR; 
        
                       if (isRHPinState) { 
        
                           return getMatchingRootRouteForRHPRoute(findFocusedRoute(stateForBackTo) as NavigationPartialRoute); 
        
                       } 
        
                   } 
        
                   // If we know that backTo targets the root route (full screen) we want to use it. 
        
                   const fullScreenNavigator = stateForBackTo.routes.find((rt) => rt.name === NAVIGATORS.FULL_SCREEN_NAVIGATOR); 
        
                   if (fullScreenNavigator && fullScreenNavigator.state) { 
        
                       return fullScreenNavigator as NavigationPartialRoute<CentralPaneName | typeof NAVIGATORS.FULL_SCREEN_NAVIGATOR>; 
        
                   } 
        
                   // If we know that backTo targets a central pane screen we want to use it. 
        
                   const centralPaneScreen = stateForBackTo.routes.find((rt) => isCentralPaneName(rt.name)); 
        
                   if (centralPaneScreen) { 
        
                       return centralPaneScreen as NavigationPartialRoute<CentralPaneName | typeof NAVIGATORS.FULL_SCREEN_NAVIGATOR>; 
        
                   } 
        
               } 
        
           }

In our case, the last backTo param is the travel page which doesn't have a central pane screen mapping, which will later be defaulted to the report screen.

App/src/libs/Navigation/linkingConfig/getAdaptedStateFromPath.ts

Line 203 in 91f610c

    
           matchingRootRoute = matchingRootRoute ?? (!isNarrowLayout ? {name: SCREENS.REPORT} : undefined);

However, the result is ignored because of isRHPinState is false here.

App/src/libs/Navigation/linkingConfig/getAdaptedStateFromPath.ts

Lines 119 to 125 in 91f610c

    
           if (rhpNavigator && rhpNavigator.state) { 
        
               const isRHPinState = stateForBackTo.routes.at(0)?.name === NAVIGATORS.RIGHT_MODAL_NAVIGATOR; 
        
               if (isRHPinState) { 
        
                   return getMatchingRootRouteForRHPRoute(findFocusedRoute(stateForBackTo) as NavigationPartialRoute); 
        
               } 
        
           }

It's because stateForBackTo.routes contains [BottomTabNavigator, RHPNavigator], so always accessing the first index won't work. We can just remove isRHPinState because we already have rhpNavigator which already finds RHP inside stateForBackTo.routes.

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

Remove the isRHPInState. We previously fixed this before, but the isRHPinState is reintroduced in #49534, specifically this changes. I believe it's a bad merge.

@stitesExpensify

Filing this one under vip-travel. CC: @stitesExpensify

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

BugZero Checklist:

The PR that introduced the bug has been identified. Link to the PR: Add prefer-at condition to eslint #49534
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: https://github.com/Expensify/App/pull/49534/files#r1815522422
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: N/A
Determine if we should create a regression test for this bug. Yes
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.

Regression test:

Press FAB > Book Travel
Press Book travel
Press Country
(Web/Desktop) Verify the report screen behind RHP doesn't change
(small screen) Close the book travel page
(small screen) Verify you are still at LHN

Do we 👍 or 👎

Personally I think that is a very odd test to have, I think we don't need one for this bug.

@dukenv0307

Standalone as a test, I agree. But in some wider regression test about Book travel, having a one liner added doesn't sound so bad.

The PR that introduced the bug has been identified. Link to the PR: N/A
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: N/A

@dukenv0307 can you provide context behind these "N/A"'s please?

@trjExpensify

@trjExpensify Sorry I missed adding the links. I updated the checklists

@bernhardoj

Thanks! Confirming the payment summary as follows:

$250 to @bernhardoj for the fix (go ahead and request!)
$250 to @dukenv0307 for the C+ review (paid!)

Closing!

@rlinoz

@rlinoz @trjExpensify Be sure to fill out the Contact List!

@mallenexpensify

^^ @mallenexpensify what is this?

Requested in ND.

@bernhardoj

$250 approved for @bernhardoj

@trjExpensify

@trjExpensify posted about in #bug-zero here

Navigation Menu

Repository navigation

[HOLD for payment 2024-10-22] [$250] Book travel - Report screen changes to profile view when clicking Country field #50155

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

Proposal

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

What is the root cause of that problem?

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

26 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	[SCREENS.SETTINGS.PROFILE.ROOT]: [
	SCREENS.SETTINGS.PROFILE.DISPLAY_NAME,
	SCREENS.SETTINGS.PROFILE.CONTACT_METHODS,
	SCREENS.SETTINGS.PROFILE.CONTACT_METHOD_DETAILS,
	SCREENS.SETTINGS.PROFILE.CONTACT_METHOD_VALIDATE_ACTION,
	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,

	if (route.params && 'backTo' in route.params && typeof route.params.backTo === 'string') {
	const stateForBackTo = getStateFromPath(route.params.backTo, config);
	if (stateForBackTo) {
	// If there is rhpNavigator in the state generated for backTo url, we want to get root route matching to this rhp screen.
	const rhpNavigator = stateForBackTo.routes.find((rt) => rt.name === NAVIGATORS.RIGHT_MODAL_NAVIGATOR);
	if (rhpNavigator && rhpNavigator.state) {
	const isRHPinState = stateForBackTo.routes.at(0)?.name === NAVIGATORS.RIGHT_MODAL_NAVIGATOR;

	if (isRHPinState) {
	return getMatchingRootRouteForRHPRoute(findFocusedRoute(stateForBackTo) as NavigationPartialRoute);
	}
	}

	// If we know that backTo targets the root route (full screen) we want to use it.
	const fullScreenNavigator = stateForBackTo.routes.find((rt) => rt.name === NAVIGATORS.FULL_SCREEN_NAVIGATOR);
	if (fullScreenNavigator && fullScreenNavigator.state) {
	return fullScreenNavigator as NavigationPartialRoute<CentralPaneName \| typeof NAVIGATORS.FULL_SCREEN_NAVIGATOR>;
	}

	// If we know that backTo targets a central pane screen we want to use it.
	const centralPaneScreen = stateForBackTo.routes.find((rt) => isCentralPaneName(rt.name));
	if (centralPaneScreen) {
	return centralPaneScreen as NavigationPartialRoute<CentralPaneName \| typeof NAVIGATORS.FULL_SCREEN_NAVIGATOR>;
	}
	}
	}

[HOLD for payment 2024-10-22] [$250] Book travel - Report screen changes to profile view when clicking Country field #50155

Description

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

Activity

melvin-bot commented on Oct 3, 2024

IuliiaHerets commented on Oct 3, 2024

IuliiaHerets commented on Oct 3, 2024

bernhardoj commented on Oct 3, 2024

Proposal

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

What is the root cause of that problem?

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

trjExpensify commented on Oct 7, 2024

melvin-bot commented on Oct 7, 2024

26 remaining items

dukenv0307 commented on Oct 22, 2024

rlinoz commented on Oct 23, 2024

trjExpensify commented on Oct 23, 2024

dukenv0307 commented on Oct 24, 2024

trjExpensify commented on Oct 24, 2024

melvin-bot commented on Oct 24, 2024

trjExpensify commented on Oct 24, 2024

bernhardoj commented on Oct 25, 2024

JmillsExpensify commented on Oct 25, 2024

mallenexpensify commented on Oct 25, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions