Make onboarding continue from last visited onboarding page by tsa321 · Pull Request #48137 · Expensify/App

tsa321 · 2024-08-28T02:33:38Z

Details

Fixed Issues

$ #46104
PROPOSAL: #46104 (comment)

Tests

Sign in as a new user -> Join
Verify that the onboarding modal is displayed.
Select Manage my team's expenses.
Change the URL to a random URL after .com, for example, /aaa (for desktop, enter the URL in the browser as new-expensify:/aaa and press Enter).
Verify that the onboarding modal work page is displayed.
Change the URL after .com to /settings/profile (for desktop, enter the URL in the browser as new-expensify:/settings/profile and press Enter).
Verify that the onboarding modal work page is displayed.
Fill in the business name and click Continue.
Complete the onboarding flow.
Verify that the explanation modal appears properly.

Verify that no errors appear in the JS console

Offline tests

QA Steps

Sign in as a new user -> Join
Verify that the onboarding modal is displayed.
Select Manage my team's expenses.
Change the URL to a random URL after .com, for example, /aaa (for desktop, enter the URL in the browser as new-expensify:/aaa and press Enter).
Verify that the onboarding modal work page is displayed.
Change the URL after .com to /settings/profile (for desktop, enter the URL in the browser as new-expensify:/settings/profile and press Enter).
Verify that the onboarding modal work page is displayed.
Fill in the business name and click Continue.
Complete the onboarding flow.
Verify that the explanation modal appears properly.

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

android-native-d.mp4

Android: mWeb Chrome

android-mweb_d.mp4

iOS: Native

ios-native_d.mp4

iOS: mWeb Safari

ios-msfari_d.mp4

MacOS: Chrome / Safari

macos-web-d.mp4

MacOS: Desktop

macos-desktop_d.mp4

Signed-off-by: Tsaqif <tsaiinkwa@yahoo.com>

melvin-bot · 2024-08-28T02:33:43Z

@mollfpr Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

tsa321 · 2024-08-28T02:38:39Z

@mollfpr the previous PR was reverted due to lint and type-check errors caused by the deletion of ROUTES in baseOnboardingPersonalDetails by a recent PR. Let's continue with this PR...

mollfpr

The changes still look good. However, after retesting it I think these found will be taken as a regression.

The Onboarding_Purpose is shown briefly after refreshing the page.
The back button from Onboarding_Personal_Details doesn't take to the Onboarding_Work after refreshing the page.

Screen.Recording.2024-08-29.at.12.24.58.mp4

I agree the above issue might be out of the scope of this PR, unfortunately, it's introduced in this PR.

So what do you guys think, should we fix it in this PR?

cc @chiragsalian @tsa321

mollfpr · 2024-08-29T04:51:35Z


-            Navigation.dismissModal();
+            // Navigate to HOME instead of dismissModal, because there is bug in small screen
+            // where the onboarding puropose page will be disaplayed briefly


Suggested change

// where the onboarding puropose page will be disaplayed briefly

// where the onboarding purpose page will be displayed briefly

tsa321 · 2024-08-30T02:54:34Z

@mollfpr, I can actually reproduce the first issue in staging as well. However, due to the optimizations in the production build, the purpose page is displayed for a shorter time compared to development.

bug-d.mp4

tsa321 · 2024-08-30T02:57:09Z

The changes still look good. However, after retesting it I think these found will be taken as a regression.
1. The `Onboarding_Purpose` is shown briefly after refreshing the page.

This also happens in main/staging. Here is the video. To fix this will require changes in the Navigation/Routes-related files, I am afraid it would break other navigation function

2. The back button from `Onboarding_Personal_Details` doesn't take to the `Onboarding_Work` after refreshing the page.

This also happens in the main. When a user refreshes the personal details page after selecting work - flow, the 'Back' button in the header will navigate back to the purpose page.
To fix this also requires changes to the Navigation-related files, specifically to getAdaptedStateFromPath. I’m just concerned that it might break other navigations.

So, I believe these two bugs already exist in the main branch.
Should we open new issues for these two?

cc @chiragsalian

chiragsalian · 2024-08-30T20:40:01Z

If the bug exists in main branch then we don't need to solve it here.
But yes we should open issues for them. Otherwise QA will unnecessarily create blockers for known issues.
With that said, is that something you would be interested in solving in this PR or separate ones @tsa321? If it this PR we can increase the payment because the scope has increased.
Let me know what you prefer.

tsa321 · 2024-08-31T06:24:28Z

@chiragsalian I would like to try to fix the issues here. I will inform you again once I am able to fix the issues.

…oing back navigation Signed-off-by: Tsaqif <tsaiinkwa@yahoo.com>

Signed-off-by: Tsaqif <tsaiinkwa@yahoo.com>

tsa321 · 2024-09-03T07:37:49Z

                            shouldSaveDraft
                            maxLength={CONST.TITLE_CHARACTER_LIMIT}
                            spellCheck={false}
-                            autoFocus


I am removing this line because the autofocus is handled by useAutoFocusInput(); as it is in the personal details page. This line is causing issues with the work page animation (the animation doesn't run) and navigation problems on iOS.

tsa321 · 2024-09-03T07:43:33Z

I have fixed the two issues mentioned above.
Could you please retest this PR, @mollfpr? Thank you!

mollfpr

LGTM 👍

Just need to fix the minor suggestion and we're good to go.

While testing back and forth, I found an issue where the page redirects to the personal details page when pressing the browser back button on the onboarding work page. After that, I test the same scenario on staging and it's also reproducible, so the issue is not coming from this PR.

Screen.Recording.2024-09-04.at.21.08.46.mp4

mollfpr · 2024-09-04T13:55:02Z

+                {
+                    name: SCREENS.ONBOARDING.WORK,
+                    params: currentRoute?.params,
+                    path: '/onboarding/works',


The path is work singular. I think it's better we use getRoute().

Suggested change

path: '/onboarding/works',

path: `/${ROUTES.ONBOARDING_WORK.route}`,

@mollfpr actually we don’t need the path field, I forgot to remove it.

Signed-off-by: Tsaqif <tsaiinkwa@yahoo.com>

mollfpr · 2024-09-05T17:47:49Z

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

Android: Native

48137.Android.mp4

Android: mWeb Chrome

48137.mWeb-Chrome.mp4

iOS: Native

48137.iOS.mov

iOS: mWeb Safari

48137.mWeb-Safari.mov

MacOS: Chrome / Safari

48137.Web.mp4

MacOS: Desktop

48137.Desktop.mp4

mollfpr

LGTM 👍

The Onboarding_Purpose is shown briefly after refreshing the page.

The back button from Onboarding_Personal_Details doesn't take to the Onboarding_Work after refreshing the page.

I confirm the above issue is fixed!

chiragsalian

LGTM

OSBotify · 2024-09-06T17:43:54Z

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

IuliiaHerets · 2024-09-10T20:48:40Z

This PR is failing for Desktop because of issue #48937

suneox · 2024-11-11T15:19:40Z

+function goBack() {
+    adaptOnboardingRouteState();
+    Navigation.isNavigationReady().then(() => {
+        Navigation.goBack();


Coming from the BZ of #50177
This condition is missing handling for the case where the user navigates back using the hardware button on Android and the onboarding route state does not initialize properly. The solution for this issue is to reset the onboarding state when setting up the router.

Make onboarding continue from last visited onboarding page

e6e271d

Signed-off-by: Tsaqif <tsaiinkwa@yahoo.com>

tsa321 requested a review from a team as a code owner August 28, 2024 02:33

melvin-bot Bot requested review from mollfpr and removed request for a team August 28, 2024 02:33

mollfpr reviewed Aug 29, 2024

View reviewed changes

tsa321 added 2 commits September 3, 2024 14:12

Fix onboarding navigation issues, briefly visible purpose and wrong g…

b862e13

…oing back navigation Signed-off-by: Tsaqif <tsaiinkwa@yahoo.com>

Fix lint errors

514d56f

Signed-off-by: Tsaqif <tsaiinkwa@yahoo.com>

tsa321 commented Sep 3, 2024

View reviewed changes

Merge branch 'main' into makeOnboardContinue

d425363

tsa321 requested a review from mollfpr September 3, 2024 12:23

mollfpr reviewed Sep 4, 2024

View reviewed changes

Remove path parameter on work route

5eed809

Signed-off-by: Tsaqif <tsaiinkwa@yahoo.com>

tsa321 requested a review from mollfpr September 4, 2024 16:26

Merge main and resolve conflict

16862d2

Signed-off-by: Tsaqif <tsaiinkwa@yahoo.com>

mollfpr approved these changes Sep 5, 2024

View reviewed changes

melvin-bot Bot requested a review from chiragsalian September 5, 2024 18:01

chiragsalian approved these changes Sep 6, 2024

View reviewed changes

chiragsalian merged commit 9a6bcfa into Expensify:main Sep 6, 2024

chiragsalian mentioned this pull request Sep 6, 2024

[$500] Onboarding - Changing the URL during onboarding takes back to the first flow #46104

Closed

6 tasks

github-actions Bot mentioned this pull request Sep 9, 2024

Deploy Checklist: New Expensify 2024-09-09 #48791

Closed

IuliiaHerets mentioned this pull request Sep 10, 2024

[$250] Desktop - Onboarding - Modal doesn't open in desktop when navigating to incorrect URL #48937

Closed

6 tasks

abdulrahuman5196 mentioned this pull request Sep 17, 2024

[CVP] Bottom Up - iOS Safari - first name field is focused, but keyboard not open on the "What's your name?" page #48333

Closed

6 tasks

suneox reviewed Nov 11, 2024

View reviewed changes

	// where the onboarding puropose page will be disaplayed briefly
	// where the onboarding purpose page will be displayed briefly

	path: '/onboarding/works',
	path: `/${ROUTES.ONBOARDING_WORK.route}`,

Uh oh!

Conversation

tsa321 commented Aug 28, 2024

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Uh oh!

melvin-bot Bot commented Aug 28, 2024

Uh oh!

tsa321 commented Aug 28, 2024

Uh oh!

mollfpr left a comment

Choose a reason for hiding this comment

Uh oh!

mollfpr Aug 29, 2024

Choose a reason for hiding this comment

Uh oh!

tsa321 commented Aug 30, 2024

Uh oh!

tsa321 commented Aug 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chiragsalian commented Aug 30, 2024

Uh oh!

tsa321 commented Aug 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tsa321 Sep 3, 2024

Choose a reason for hiding this comment

Uh oh!

tsa321 commented Sep 3, 2024

Uh oh!

mollfpr left a comment

Choose a reason for hiding this comment

Uh oh!

mollfpr Sep 4, 2024

Choose a reason for hiding this comment

Uh oh!

tsa321 Sep 4, 2024

Choose a reason for hiding this comment

Uh oh!

mollfpr commented Sep 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviewer Checklist

Screenshots/Videos

Uh oh!

mollfpr left a comment

Choose a reason for hiding this comment

Uh oh!

chiragsalian left a comment

Choose a reason for hiding this comment

Uh oh!

OSBotify commented Sep 6, 2024

Uh oh!

IuliiaHerets commented Sep 10, 2024

Uh oh!

suneox Nov 11, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

tsa321 commented Aug 30, 2024 •

edited

Loading

tsa321 commented Aug 31, 2024 •

edited

Loading

mollfpr commented Sep 5, 2024 •

edited

Loading