Refactor NewDot and HybridApp onboarding flow #49586

blazejkustra · 2024-09-23T08:35:45Z

Details

HybridApp PR: Adjust OldDot to new fix of NewDot onboarding

While working on #49499, I noticed how complicated the current onboarding logic is:

Current onboarding issues

First of all there are two onboarding flows:
- HybridApp onboarding that displays Explanation Modal - it is triggered from the top file Expensify.tsx with some conditional logic that is unnecessary complicated
  - Once HybridApp onboarding is completed, NewDot onboarding is triggered conditionally, this time from ExplanationModal.tsx
- NewDot onboarding that displays Onboarding Modal - it is triggered from BottomTabNavigator but only if HybridApp module isn't available (also the logic is quite complex)
There is no single source of truth on how onboarding should behave, instead it is scattered across the codebase making this refactor quite hard, these files affect onboarding:
- Expensify.tsx
- ExplanationModal.tsx
- OnboardingModalNavigator.tsx
- BottomTabBar.tsx
Currently we are relying on the backend to change the value for NVP_ONBOARDING, which obviously works in most cases, but when being offline this causes NVP_ONBOARDING to never change even after onboarding completion.
- But after optimistic update the modal is unmounted right away due to the logic in OnboardingModalNavigator.tsx that returns null when NewDot onboarding is completed
- Currently the animation only works because the request takes more time than dismissModal
NAVIGATORS.ONBOARDING_MODAL_NAVIGATOR is always mounted while the screens inside are rendered conditionally - which is anti-pattern - that's why after consulting with @adamgrzybowski we decided to hide or display the whole navigator from AuthScreens

This PR aims to bring all the logic into one place useOnboardingFlow which controls the whole onboarding flow, making it much more simple.

Fixed Issues

$ #49499
PROPOSAL: N/A

Tests

Sign in as a new user, verify that onboarding modal is displayed
- Verify that the animation is smooth when the modal is dismissed
- After completing the onboarding verify that you cannot access onboarding anymore (by linking to it)
- Try different cases, like refreshing when completing onboarding flow
Sign in again, verify onboarding isn't displayed
Create an account on OldDot, and then login on NewDot, verify onboarding isn't displayed
Verify deeplinking, with/without finished onboarding
Try to break the onboarding, QAs are great at it 😅

Hybrid App Tests (No QA)

Explanation modal

Create an account in NewDot, and finish the onboarding
Login on HybridApp, transition from OldDot to NewDot
Verify onboarding modal is NOT shown
Verify explanation modal is NOT shown

NewDot Onboarding

Create an account in NewDot without finishing onboarding
Login on HybridApp, transition from OldDot to NewDot
Verify onboarding modal is shown

HybridApp explanation modal

Create an account in HybridApp
Transition from OldDot to NewDot
Verify explanation modal is shown

Offline tests

Verify the onboarding works well when offline, after completing it and refreshing the page user should no longer see the modal

QA Steps

Same as tests

PR Author Checklist

Screenshots/Videos

iOS: Native

Simulator.Screen.Recording.-.iPhone.taki.SE.-.2024-09-30.at.16.28.40.mp4

MacOS: Chrome / Safari

Screen.Recording.2024-09-30.at.16.29.13.mov

src/libs/actions/Report.ts

src/libs/Navigation/AppNavigator/createCustomBottomTabNavigator/BottomTabBar.tsx

allgandalf · 2024-09-24T16:13:27Z

This branch has conflicts @blazejkustra can you take a look

… is available

…labs/expensify-app-fork into fix/onboarding-offline

allgandalf · 2024-09-27T07:09:58Z

However, onboarding changed so much that I wouldn't rely on the Manual Tests section from these 😅

Umm, fair point, I will see testrails just in case we have better coverage there.

My email would be: whogandalf@gmail.com

mountiny · 2024-09-27T07:54:52Z

Thread for better discussion here

github-actions · 2024-09-27T09:11:05Z

🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪

Android 🤖	iOS 🍎
https://ad-hoc-expensify-cash.s3.amazonaws.com/android/49586/index.html	https://ad-hoc-expensify-cash.s3.amazonaws.com/ios/49586/index.html

Desktop 💻	Web 🕸️
https://ad-hoc-expensify-cash.s3.amazonaws.com/desktop/49586/NewExpensify.dmg	https://49586.pr-testing.expensify.com

👀 View the workflow run that generated this build 👀

lanitochka17 · 2024-09-27T17:57:51Z

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Onboarding - App opens /r/undefined when opening onboarding steps after onboarding is done

Version Number: 9.0.40-4
Reproducible in staging?: N
Reproducible in production?: N
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Issue reported by: Applause - Internal Team

Action Performed:

Go to https://49586.pr-testing.expensify.com/
Log in with a new gmail account
Select any option on the onboarding modal
Copy https://49586.pr-testing.expensify.com/onboarding/personal-details
link
Complete the onboarding
Go to https://49586.pr-testing.expensify.com/onboarding/personal-details

Expected Result:

App will return to any report because the onboarding is complete

Actual Result:

App redirects to https://49586.pr-testing.expensify.com/r/undefined
when navigating to onboarding personal details steps after onboarding is complete

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Screenshots/Videos

Bug6617639_1727458929014.20240928_013704.mp4

Upwork Automation - Do Not Edit

lanitochka17 · 2024-09-27T21:03:59Z

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Sign in – Blank page opens when log in as a user with 2FA

Version Number: 9.0.40-4
Reproducible in staging?: N
Reproducible in production?: N
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Issue reported by: Applause - Internal Team

Action Performed:

Go to https://49586.pr-testing.expensify.com/
Log in as a Gmail user with set up 2FA

Expected Result:

Inbox opens

Actual Result:

Blank page opens

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Screenshots/Videos

Bug6617650_1727459574481.And.1.mp4

blazejkustra · 2024-09-30T13:09:07Z

Onboarding - App opens /r/undefined when opening onboarding steps after onboarding is done

This is connected to a bigger regression that was introduced in AuthScreens withOnyx migration.

After reverting locally it works fine:

Screen.Recording.2024-09-30.at.15.07.23.mov

blazejkustra · 2024-09-30T13:09:37Z

Actually it was just reverted, so I'm going to focus on the other error now 👀

blazejkustra · 2024-09-30T13:48:49Z

As for the other bug: Sign in – Blank page opens when log in as a user with 2FA

It's reproducible on the newest main (only on bad internet speed), so I wouldn't say it's connected to this refactor.

Screen.Recording.2024-09-30.at.15.39.59.mov

blazejkustra · 2024-09-30T13:51:06Z

The issue isn't as visible on the main because of the above revert, but it hasn't been fully resolved either.

Regardless, this PR isn't the source of these bugs, so you can continue reviewing, @allgandalf. 🚀

allgandalf

This code looks great, thanks for all the hard work and the refactor, code is so much clearer now, I only have a few comments, I will start with the checklist in the meantime

src/Expensify.tsx

src/libs/onboardingSelectors.ts

allgandalf · 2024-09-30T14:31:41Z

src/libs/onboardingSelectors.ts

+    // Backend might return strings instead of booleans
+    if (typeof completedHybridAppOnboarding === 'string') {


sorry, but why exactly this ? There might be a fixed type right on the BE ? This is a safe typecheck but still thinking 🤔

This is copied over from src/libs/actions/Welcome/index.ts, BE sends completedHybridAppOnboarding as a 'true' | 'false' sometimes, and we need to handle them accordingly 😄

allgandalf · 2024-09-30T14:37:56Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android: Native
- Android: mWeb Chrome
- iOS: Native
- iOS: mWeb Safari
- MacOS: Chrome / Safari
- MacOS: Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
- I verified that all the inputs inside a form are aligned with each other.
- I added Design label and/or tagged @Expensify/design so the design team can review the changes.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

MacOS: Chrome / Safari

Screen.Recording.2024-09-30.at.8.14.54.PM.mov

Screen.Recording.2024-09-30.at.8.22.14.PM.mov

Screen.Recording.2024-09-30.at.8.20.14.PM.mov

MacOS: Desktop

Screen.Recording.2024-09-30.at.8.36.09.PM.mov

Android: Native

Screen.Recording.2024-09-30.at.8.43.27.PM.mov

Android: mWeb Chrome

Screen.Recording.2024-09-30.at.8.45.37.PM.mov

iOS: Native

Screen.Recording.2024-09-30.at.8.37.47.PM.mov

iOS: mWeb Safari

Screen.Recording.2024-09-30.at.8.28.40.PM.mov

allgandalf

Great work @blazejkustra 🙇 , This looks great to me, lets get the refactor through 🚀

blazejkustra · 2024-09-30T15:39:46Z

All yours @mountiny @carlosmiceli 🙌

mountiny

Changes look good to me, thanks!

mountiny · 2024-09-30T16:27:10Z

We are going to skip the Changed files ESLint check / Changed files ESLint check action because this is already a complicated refactor so additionally migrating to useOnyx would add too much scope and risk of regressions

melvin-bot · 2024-09-30T16:27:55Z

@mountiny looks like this was merged without a test passing. Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

mountiny · 2024-09-30T16:28:08Z

Not emergency, explained above

OSBotify · 2024-09-30T16:38:37Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

github-actions · 2024-09-30T22:57:23Z

🚀 Deployed to staging by https://github.com/mountiny in version: 9.0.42-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

github-actions · 2024-10-02T00:20:14Z

🚀 Deployed to production by https://github.com/jasperhuangg in version: 9.0.42-3 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

Refactor completeOnboarding function to update onboarding status

00f6568

mountiny reviewed Sep 23, 2024

View reviewed changes

src/libs/actions/Report.ts Outdated Show resolved Hide resolved

Set hasCompletedGuidedSetupFlow to false

67ef4d2

blazejkustra mentioned this pull request Sep 23, 2024

[HOLD for payment 2024-10-09] [$250] Onboarding - Onboarding Modal opens when returning back online #49499

Closed

6 tasks

allgandalf reviewed Sep 23, 2024

View reviewed changes

src/libs/actions/Report.ts Show resolved Hide resolved

blazejkustra added 2 commits September 23, 2024 18:25

Clean navigation to onboarding logic

4ae48e5

Remove unused imports

eda9b65

blazejkustra commented Sep 23, 2024

View reviewed changes

src/libs/Navigation/AppNavigator/createCustomBottomTabNavigator/BottomTabBar.tsx Outdated Show resolved Hide resolved

blazejkustra requested a review from allgandalf September 23, 2024 16:43

blazejkustra added 9 commits September 24, 2024 09:32

Merge branch 'main' into fix/onboarding-offline

1ab2303

Add useful comments

f07f55e

Remove onboarding logic from bottom tabs

647622b

Adjust onboardinf flow with hybrid app considerations

a94e8b5

Fix the logic and remove console.log

bb1cd88

Add comments for selectors

2bad563

Refactor onboarding logic and improve logging

11b901d

Remove unused import

c1aecb0

Add comments to useOnboardingFlow hook

e4dd280

blazejkustra and others added 5 commits September 24, 2024 18:13

Send the completeHybridAppOnboarding request only if HybridApp module…

4ce6479

… is available

Merge branch 'main' into fix/onboarding-offline

13c9c7d

Remove unused useOnyx

baf8497

Fix imports

e2a855c

Init HybridApp with information about HybridApp onboarding

ab69af2

mountiny assigned blazejkustra Sep 25, 2024

blazejkustra added 3 commits September 26, 2024 14:37

Make the hybrid app flow weven easier

cdfe1c7

Remove unsued variables

15a0eda

Merge branch 'fix/onboarding-offline' of github.com:software-mansion-…

3794824

…labs/expensify-app-fork into fix/onboarding-offline

mateuuszzzzz approved these changes Sep 26, 2024

View reviewed changes

blazejkustra changed the title ~~Refactor completeOnboarding function to update onboarding status when offline~~ Refactor NewDot and HybridApp onboarding flow Sep 26, 2024

Merge branch 'main' into fix/onboarding-offline

6d70f43

Merge branch 'main' into fix/onboarding-offline

907dcc3

Merge branch 'main' into fix/onboarding-offline

03c94dc

Merge branch 'main' into fix/onboarding-offline

075af14

allgandalf reviewed Sep 30, 2024

View reviewed changes

blazejkustra requested a review from allgandalf September 30, 2024 14:43

allgandalf approved these changes Sep 30, 2024

View reviewed changes

mountiny approved these changes Sep 30, 2024

View reviewed changes

mountiny merged commit fbe4025 into Expensify:main Sep 30, 2024
17 of 22 checks passed

melvin-bot bot added the Emergency label Sep 30, 2024

mountiny removed the Emergency label Sep 30, 2024

github-actions bot mentioned this pull request Sep 30, 2024

Deploy Checklist: New Expensify 2024-09-30 #49960

Closed

49 tasks

blazejkustra mentioned this pull request Oct 1, 2024

[HOLD for payment 2024-10-10] Android – Sign in – Able to dismiss the Onboarding modal using back button #49990

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor NewDot and HybridApp onboarding flow #49586

Refactor NewDot and HybridApp onboarding flow #49586

blazejkustra commented Sep 23, 2024 •

edited

Loading

allgandalf commented Sep 24, 2024

allgandalf commented Sep 27, 2024

mountiny commented Sep 27, 2024

github-actions bot commented Sep 27, 2024

lanitochka17 commented Sep 27, 2024

lanitochka17 commented Sep 27, 2024 •

edited

Loading

blazejkustra commented Sep 30, 2024

blazejkustra commented Sep 30, 2024

blazejkustra commented Sep 30, 2024 •

edited

Loading

blazejkustra commented Sep 30, 2024 •

edited

Loading

allgandalf left a comment

allgandalf Sep 30, 2024

blazejkustra Sep 30, 2024 •

edited

Loading

allgandalf commented Sep 30, 2024 •

edited

Loading

allgandalf left a comment

blazejkustra commented Sep 30, 2024

mountiny left a comment

mountiny commented Sep 30, 2024

melvin-bot bot commented Sep 30, 2024

mountiny commented Sep 30, 2024

OSBotify commented Sep 30, 2024

github-actions bot commented Sep 30, 2024

github-actions bot commented Oct 2, 2024

		// Backend might return strings instead of booleans
		if (typeof completedHybridAppOnboarding === 'string') {

Refactor NewDot and HybridApp onboarding flow #49586

Refactor NewDot and HybridApp onboarding flow #49586

Conversation

blazejkustra commented Sep 23, 2024 • edited Loading

Details

Fixed Issues

Tests

Hybrid App Tests (No QA)

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

allgandalf commented Sep 24, 2024

allgandalf commented Sep 27, 2024

mountiny commented Sep 27, 2024

github-actions bot commented Sep 27, 2024

lanitochka17 commented Sep 27, 2024

Onboarding - App opens /r/undefined when opening onboarding steps after onboarding is done

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

lanitochka17 commented Sep 27, 2024 • edited Loading

Sign in – Blank page opens when log in as a user with 2FA

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

blazejkustra commented Sep 30, 2024

blazejkustra commented Sep 30, 2024

blazejkustra commented Sep 30, 2024 • edited Loading

blazejkustra commented Sep 30, 2024 • edited Loading

allgandalf left a comment

Choose a reason for hiding this comment

allgandalf Sep 30, 2024

Choose a reason for hiding this comment

blazejkustra Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

allgandalf commented Sep 30, 2024 • edited Loading

Reviewer Checklist

Screenshots/Videos

allgandalf left a comment

Choose a reason for hiding this comment

blazejkustra commented Sep 30, 2024

mountiny left a comment

Choose a reason for hiding this comment

mountiny commented Sep 30, 2024

melvin-bot bot commented Sep 30, 2024

mountiny commented Sep 30, 2024

OSBotify commented Sep 30, 2024

github-actions bot commented Sep 30, 2024

github-actions bot commented Oct 2, 2024

blazejkustra commented Sep 23, 2024 •

edited

Loading

lanitochka17 commented Sep 27, 2024 •

edited

Loading

blazejkustra commented Sep 30, 2024 •

edited

Loading

blazejkustra commented Sep 30, 2024 •

edited

Loading

blazejkustra Sep 30, 2024 •

edited

Loading

allgandalf commented Sep 30, 2024 •

edited

Loading