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

[$250] Book travel - Report screen changes to profile view when clicking Country field #47069

Closed
2 of 6 tasks
izarutskaya opened this issue Aug 8, 2024 · 25 comments
Closed
2 of 6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@izarutskaya
Copy link

izarutskaya commented Aug 8, 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.18.1
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers): applausetester+kh050806@applause.expensifail.com
Logs: https://stackoverflow.com/c/expensify/questions/4856
Issue reported by: Applause-Internal team

Action Performed:

  1. Go to staging.new.expensify.com
  2. Go to FAB > Book travel.
  3. Click Book travel.
  4. 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:

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

Bug6565224_1723088792855.20240808_113943.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~011952935e51b38f07
  • Upwork Job ID: 1826610853854953560
  • Last Price Increase: 2024-08-22
  • Automatic offers:
    • rojiphil | Reviewer | 103684843
Issue OwnerCurrent Issue Owner: @rojiphil
@izarutskaya izarutskaya added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Aug 8, 2024
Copy link

melvin-bot bot commented Aug 8, 2024

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

@izarutskaya
Copy link
Author

We think this issue might be related to the #vip-travel

@melvin-bot melvin-bot bot added the Overdue label Aug 12, 2024
@abekkala
Copy link
Contributor

@izarutskaya is it only when clicking Country that the pages changes or is it any of them?

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Aug 13, 2024
Copy link

melvin-bot bot commented Aug 16, 2024

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

@abekkala
Copy link
Contributor

@izarutskaya bumping my question above from last week

@melvin-bot melvin-bot bot removed the Overdue label Aug 19, 2024
@bernhardoj
Copy link
Contributor

Proposal

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

The background screen changes to workspace profile view when open address page from travel page.

What is the root cause of that problem?

The behavior is a bit different now than the video. When we press Book travel, it will open the profile address page and based on the mapping, the workspace address RHP maps to the workspace profile full screen page.

[SCREENS.WORKSPACE.PROFILE]: [SCREENS.WORKSPACE.NAME, SCREENS.WORKSPACE.ADDRESS, SCREENS.WORKSPACE.CURRENCY, SCREENS.WORKSPACE.DESCRIPTION, SCREENS.WORKSPACE.SHARE],

So, the workspace profile page is shown. It's the same case with the country selection page where the backTo param is the workspace address page which we recursively find the matching root for the backTo params page.

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) {
// eslint-disable-next-line @typescript-eslint/no-shadow
const rhpNavigator = stateForBackTo.routes.find((route) => route.name === NAVIGATORS.RIGHT_MODAL_NAVIGATOR);
const centralPaneOrFullScreenNavigator = stateForBackTo.routes.find(
// eslint-disable-next-line @typescript-eslint/no-shadow
(route) => isCentralPaneName(route.name) || route.name === NAVIGATORS.FULL_SCREEN_NAVIGATOR,
);
// If there is rhpNavigator in the state generated for backTo url, we want to get root route matching to this rhp screen.
if (rhpNavigator && rhpNavigator.state) {
return getMatchingRootRouteForRHPRoute(findFocusedRoute(stateForBackTo) as NavigationPartialRoute);
}
// 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>;
}
}
}

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

We need to add a backTo params to the workspace address page that points to the travel page so getMatchingRootRouteForRHPRoute will return the matching root for the travel page, which is nothing.

App/src/ROUTES.ts

Lines 546 to 549 in 3559aed

WORKSPACE_PROFILE_ADDRESS: {
route: 'settings/workspaces/:policyID/profile/address',
getRoute: (policyID: string) => `settings/workspaces/${policyID}/profile/address` as const,
},

getRoute: (policyID: string, backTo?: string) => getUrlWithBackToParam(`settings/workspaces/${policyID}/profile/address` as const, backTo),

Then, pass the active route as the backTo.

Navigation.navigate(ROUTES.WORKSPACE_PROFILE_ADDRESS.getRoute(activePolicyID ?? '-1'));

Navigation.navigate(ROUTES.WORKSPACE_PROFILE_ADDRESS.getRoute(activePolicyID ?? '-1', Navigation.getActiveRoute()));

Next, we need to do the same thing for the address sub-page, that is country and state selection page. Both pages already have a backTo, but for the country selection page, we pass the active route without params, so the nested backTo is removed.

const activeRoute = Navigation.getActiveRouteWithoutParams();
didOpenContrySelector.current = true;
Navigation.navigate(ROUTES.SETTINGS_ADDRESS_COUNTRY.getRoute(countryCode ?? '', activeRoute));

So, the matching root will still be the workspace profile page. We need to replace it with Navigation.getActiveRoute().

Last, we need to update the route here so it appends the param correctly because the backTo contains a nested backTo (?backTo={...?backTo=...}).

} else if (!!backTo && navigation.getState().routes.length === 1) {
// If "backTo" is not empty and there is only one route, go back to the specific route defined in "backTo" with a country parameter
Navigation.goBack(`${route.params.backTo}?country=${option.value}` as Route);
} else {
// Otherwise, navigate to the specific route defined in "backTo" with a country parameter
Navigation.navigate(`${route.params.backTo}?country=${option.value}` as Route);
}

} else if (!!backTo && navigation.getState().routes.length === 1) {
    // If "backTo" is not empty and there is only one route, go back to the specific route defined in "backTo" with a country parameter
    Navigation.goBack(appendParam(backTo, 'country', option.value) as Route);
} else {
    // Otherwise, navigate to the specific route defined in "backTo" with a country parameter
    Navigation.navigate(appendParam(backTo, 'country', option.value) as Route);
}

@melvin-bot melvin-bot bot added the Overdue label Aug 21, 2024
@abekkala abekkala added the External Added to denote the issue can be worked on by a contributor label Aug 22, 2024
Copy link

melvin-bot bot commented Aug 22, 2024

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

@melvin-bot melvin-bot bot changed the title Book travel - Report screen changes to profile view when clicking Country field [$250] Book travel - Report screen changes to profile view when clicking Country field Aug 22, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 22, 2024
Copy link

melvin-bot bot commented Aug 22, 2024

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

Copy link

melvin-bot bot commented Aug 22, 2024

@rojiphil @abekkala this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

@rojiphil
Copy link
Contributor

Thanks for the proposal.
The RCA is correct i.e. RHP pages of Address and Country selection map to the workspace profile page in central pane due to which the central pane switches to profile page.
And it makes sense to fix this using backTo
@bernhardoj proposal LGTM
🎀👀🎀 C+ reviewed

Copy link

melvin-bot bot commented Aug 23, 2024

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

@melvin-bot melvin-bot bot added Overdue and removed Help Wanted Apply this label when an issue is open to proposals by contributors labels Aug 26, 2024
Copy link

melvin-bot bot commented Aug 26, 2024

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

Offer link
Upwork job

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Overdue Daily KSv2 labels Aug 26, 2024
@bernhardoj
Copy link
Contributor

PR is ready

cc: @rojiphil

@abekkala
Copy link
Contributor

note: not yet merged

@rojiphil
Copy link
Contributor

rojiphil commented Sep 6, 2024

Oh! The automation did not get triggered here.
Note: Reached production on 06 Sep

@rojiphil
Copy link
Contributor

  • [@rojiphil] The PR that introduced the bug has been identified. Link to the PR: Offending PR
  • [@rojiphil] 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: Added comment
  • [@rojiphil] 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: Not Required. Existing checklist is good enough to capture such issues.
  • [@rojiphil] Determine if we should create a regression test for this bug. : Yes. We can
  • [@rojiphil] 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.

Steps:
1 Press FAB > Book travel
2 Press the Book travel button
3 (Web/Desktop) Verify that the screen behind the RHP overlay doesn't change
4 Press Country
5 (Web/Desktop) Verify that the screen behind the RHP overlay doesn't change
6 (Android/mWeb/iOS) Close the address and book travel page and verify you are still in LHN. After that, go back to the address page
7 Change the country to the United States
8 Press State
9 (Web/Desktop) Verify that the screen behind the RHP overlay doesn't change
10 (Android/mWeb/iOS) Close the address and book travel page and verify you are still in LHN.

@rojiphil
Copy link
Contributor

@abekkala Please help with payment summary here as this reached production 3 weeks ago. I have also added BZ checklist for the issue. Thanks.

@abekkala
Copy link
Contributor

abekkala commented Sep 27, 2024

PAYMENT SUMMARY:

@rojiphil
Copy link
Contributor

@abekkala Accepted offer. Thanks.

@bernhardoj
Copy link
Contributor

@abekkala I think you paid the wrong Contributor

@bernhardoj
Copy link
Contributor

Requested in ND.

@abekkala
Copy link
Contributor

@rojiphil payment sent and contract ended - thank you! 🎉

@abekkala
Copy link
Contributor

@bernhardoj

@abekkala I think you paid the wrong Contributor

yes, I apolize that I tagged the other cont. - I was viewing PR 46333 that Rojiphil linked rather than yours #48002 . I'ved edited.

@abekkala
Copy link
Contributor

PAYMENT SUMMARY:

@JmillsExpensify
Copy link

$250 approved for @bernhardoj

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. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests

6 participants