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] Onboarding - Changing the size of the window dismisses the onboarding modal #48930

Closed
2 of 6 tasks
IuliiaHerets opened this issue Sep 10, 2024 · 15 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

@IuliiaHerets
Copy link

IuliiaHerets commented Sep 10, 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: v9.0.31-4
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers): dave0123seife@gmail.com
Issue reported by: Applause Internal Team

Action Performed:

  1. Resize the height of the window to be a very small mobile viewport
  2. Sign in as a new user
    3 When the onboarding modal shows resize to normal window size

Expected Result:

Resizing the of window does not dismisses the onboarding modal

Actual Result:

Resizing the of window dismisses the onboarding modal

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Bug6598412_1725953865275.Screen_Recording_2024-09-10_at_10.22.23_in_the_morning.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021834487171313044824
  • Upwork Job ID: 1834487171313044824
  • Last Price Increase: 2024-09-13
  • Automatic offers:
    • jjcoffee | Reviewer | 103951138
    • c3024 | Contributor | 103951139
Issue OwnerCurrent Issue Owner: @jjcoffee
@IuliiaHerets IuliiaHerets added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Sep 10, 2024
Copy link

melvin-bot bot commented Sep 10, 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.

@IuliiaHerets
Copy link
Author

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

@c3024
Copy link
Contributor

c3024 commented Sep 11, 2024

Proposal

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

Logging in on a narrow window and then enlarging it to a non-narrow window dismisses the onboarding modal

What is the root cause of that problem?

When we login in the narrow view, the routes in state only have BottomTabNavigator and OnboardingModalNavigator
Screenshot 2024-09-11 at 10 40 34 PM
and when we enlarge it we find the template state from the path

const {adaptedState: templateState} = getAdaptedStateFromPath(pathFromCurrentState, linkingConfig.config);

and here REPORT screen is added to the template state
if (!isNarrowLayout) {
routes.push({
name: SCREENS.REPORT,

We extract the central pane from the template state and insert it into state if there is no top most central pane route in it
const templateCentralPaneRoute = templateState.routes.find((route) => isCentralPaneName(route.name));
const topmostCentralPaneRouteExtracted = getTopmostCentralPaneRoute(state);
const templateCentralPaneRouteExtracted = getTopmostCentralPaneRoute(templateState as State<RootStackParamList>);
// If there is no templateCentralPaneRoute, we don't have anything to add.
if (!templateCentralPaneRoute) {
return;
}
// If there is no topmostCentralPaneRoute in the state and template state has one, we need to add it.
if (!topmostCentralPaneRoute) {
insertRootRoute(state, templateCentralPaneRoute);
return;
}

and we insert it before modalRoutes
state.routes = [...nonModalRoutes, routeToInsert, ...modalRoutes]; // eslint-disable-line

but here
const nonModalRoutes = state.routes.filter((route) => route.name !== NAVIGATORS.RIGHT_MODAL_NAVIGATOR && route.name !== NAVIGATORS.LEFT_MODAL_NAVIGATOR);
const modalRoutes = state.routes.filter((route) => route.name === NAVIGATORS.RIGHT_MODAL_NAVIGATOR || route.name === NAVIGATORS.LEFT_MODAL_NAVIGATOR);

only Left and Right Modal Navigators are included in the modal routes and Onboarding Navigator is not so it is moved to an index before the REPORT route.

So, the order of navigators will become
BottomTabNavigator > Onboarding Navigator > Report and the Onboarding Modal gets dismissed.

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

Exclude and include the NAVIGATORS.ONBOARDING_MODAL_NAVIGATOR in these two lines respectively.

const nonModalRoutes = state.routes.filter((route) => route.name !== NAVIGATORS.RIGHT_MODAL_NAVIGATOR && route.name !== NAVIGATORS.LEFT_MODAL_NAVIGATOR);
const modalRoutes = state.routes.filter((route) => route.name === NAVIGATORS.RIGHT_MODAL_NAVIGATOR || route.name === NAVIGATORS.LEFT_MODAL_NAVIGATOR);

What alternative solutions did you explore? (Optional)

@VictoriaExpensify
Copy link
Contributor

2024-09-13_16-55-03 (2)

Recreated the issue and I agree that this seem like a problem we should fix

@VictoriaExpensify
Copy link
Contributor

It doesn't look like this is being resolved elsewhere

@VictoriaExpensify VictoriaExpensify added the External Added to denote the issue can be worked on by a contributor label Sep 13, 2024
@melvin-bot melvin-bot bot changed the title Onboarding - Changing the size of the window dismisses the onboarding modal [$250] Onboarding - Changing the size of the window dismisses the onboarding modal Sep 13, 2024
Copy link

melvin-bot bot commented Sep 13, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 13, 2024
Copy link

melvin-bot bot commented Sep 13, 2024

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

@jjcoffee
Copy link
Contributor

@c3024's proposal LGTM!

🎀👀🎀 C+ reviewed

Copy link

melvin-bot bot commented Sep 13, 2024

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

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 13, 2024
Copy link

melvin-bot bot commented Sep 13, 2024

📣 @jjcoffee 🎉 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

Copy link

melvin-bot bot commented Sep 13, 2024

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

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Sep 14, 2024
@c3024
Copy link
Contributor

c3024 commented Sep 21, 2024

Deployed to production on 18-Sep. Payment due on 25-Sep.

@jjcoffee
Copy link
Contributor

  • The PR that introduced the bug has been identified. Link to the PR: [Onboarding - Stage 1] Onboarding Flow #37733
  • 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: [Onboarding - Stage 1] Onboarding Flow #37733 (comment)
  • 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 - fairly specific case that was missed
  • 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 Proposal

  1. Reduce the browser window to a narrow size.
  2. Login with a new account.
  3. Wait till the onboarding modal opens up.
  4. Enlarge the screen till the central pane (report screen) shows up.
  5. Verify that the onboarding modal is not dismissed.

Do we agree 👍 or 👎

@jjcoffee
Copy link
Contributor

@VictoriaExpensify This one's due payment now as it was deployed last week.

@VictoriaExpensify
Copy link
Contributor

Payment summary:
Contributor: @c3024 paid $250 via Upwork
Contributor+: @jjcoffee paid $250 via Upwork
Upwork job

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
No open projects
Status: Done
Development

No branches or pull requests

5 participants