Block Onboarding modal dismiss on android #42818

skyweb331 · 2024-05-30T07:42:34Z

Details

Block Onboarding modal dismiss by native android back button and swipe behavior to force user complete the onboarding steps

Fixed Issues

$ #40963
2. PROPOSAL: #40963 (comment)

Tests

Login as a new user or a user who does not complete the onboarding steps on Android
User should see the onboarding modal.
Android Back button should not dismiss the modal
Android Swipe should not dismiss the modal

Verify that no errors appear in the JS console

Offline tests

Same as above but user should be logged in before offline.

QA Steps

Login as a new user or a user who does not complete the onboarding steps on Android
User should see the onboarding modal.
Android Back button should not dismiss the modal
Android Swipe should not dismiss the modal

Verify that no errors appear in the JS console

PR Author Checklist

I linked the correct issue in the ### Fixed Issues section above
I wrote clear testing steps that cover the changes made in this PR
- I added steps for local testing in the Tests section
- I added steps for the expected offline behavior in the Offline steps section
- I added steps for Staging and/or Production testing in the QA steps section
- I added steps to cover 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 tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
I included screenshots or videos for tests on all platforms
I ran the tests on all platforms & verified they passed on:
- Android: Native
- Android: mWeb Chrome
- iOS: Native
- iOS: mWeb Safari
- MacOS: Chrome / Safari
- MacOS: Desktop
I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
I followed proper code patterns (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
  - If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
- 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 followed the guidelines as stated in the Review Guidelines
I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
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
I verified that if a function's arguments changed that all usages have also been updated correctly
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.

Screenshots/Videos

Android: Native

Screen.Recording.2024-06-18.at.1.44.24.AM.mov

Android: mWeb Chrome

iOS: Native

Screen.Recording.2024-06-18.at.1.44.24.AM.mov

iOS: mWeb Safari

MacOS: Chrome / Safari

MacOS: Desktop

melvin-bot · 2024-05-30T07:42:39Z

@sobitneupane 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]

sobitneupane

You have not blocked the swiping mentioned in the 2nd step of your proposal. Is it not required?

sobitneupane · 2024-06-06T08:49:14Z

src/libs/Navigation/AppNavigator/Navigators/OnboardingModalNavigator/index.tsx

@@ -17,7 +18,7 @@ import * as Report from '@userActions/Report';
 import CONST from '@src/CONST';
 import ONYXKEYS from '@src/ONYXKEYS';
 import SCREENS from '@src/SCREENS';
-import Overlay from './Overlay';


What is the need for this change? Let's revert it back if it is not required.

This is just a code clean up... If you don't agree, I will revert. just let me know.

Yup. Let's revert it.

IT IS A ESLINT RULE NOW :)

I think this is related to eslint linked with your vsc not the app.

Oh, is it??? My VSC is really smart...
I will revert it.

This is not my IDE rule...EXPENSIFY RULE!!!

skyweb331 · 2024-06-06T15:58:30Z

@sobitneupane
Swipe was already blocked here

App/src/libs/Navigation/getOnboardingModalScreenOptions/index.native.ts

Line 8 in f2018b8

gestureEnabled: false,

sobitneupane

@skyweb331 We are blocking the user to go back to "What do you want to do today?" page from "What's your name?" page as well. Can we remove the back handler for initial page only? We can swipe back to "What do you want to do today?" page in IOS from "What's your name?" page.

vidma_recorder_edited_07062024_132215.mp4

skyweb331 · 2024-06-07T09:53:12Z

@sobitneupane
I got it. will submit the PR soon.

skyweb331 · 2024-06-13T05:29:23Z

@sobitneupane
So I blocked dismiss behavior on only first step of onboarding. Please check this.

sobitneupane · 2024-06-17T08:40:45Z

@skyweb331 I could close the onboarding flow on IOS.

Screen.Recording.2024-06-17.at.14.23.50.mov

skyweb331 · 2024-06-17T09:20:06Z

@sobitneupane
I tried to block it on android only... will work on IOS now...

sobitneupane · 2024-06-17T09:26:13Z

It was already blocked in IOS AFAIK. The changes that you introduced might have caused the issue.

skyweb331 · 2024-06-18T09:05:21Z

@sobitneupane Please review the latest update. I fixed it by reverting some of my changes...

Screen.Recording.2024-06-18.at.1.44.24.AM.mov

sobitneupane

Screenshots/Videos

Android: Native

vidma_recorder_edited_19062024_141429.mp4

Android: mWeb Chrome

iOS: Native

Screen.Recording.2024-06-19.at.13.46.06.mov

iOS: mWeb Safari

Screen.Recording.2024-06-19.at.13.48.20.mov

MacOS: Chrome / Safari

Screen.Recording.2024-06-19.at.13.55.47.mov

MacOS: Desktop

sobitneupane · 2024-06-19T08:33:10Z

@skyweb331 Can you please update the screen recording on IOS/native and Android/native.

sobitneupane

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.

skyweb331 · 2024-06-19T08:44:45Z

@sobitneupane You mean, update video in PR description?
#42818 (comment)

skyweb331 · 2024-06-19T08:50:56Z

@sobitneupane I updated the PR description with this video
https://github.com/Expensify/App/assets/25519849/17f75b36-4a92-474e-a1d6-0ce2e76236c9

melvin-bot · 2024-06-19T09:08:34Z

We did not find an internal engineer to review this PR, trying to assign a random engineer to #40963 as well as to this PR... Please reach out for help on Slack if no one gets assigned!

github-actions · 2024-06-19T11:54:50Z

🧪🧪 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/42818/index.html	❌ FAILED ❌
	The QR code can't be generated, because the iOS build failed
Desktop 💻	Web 🕸️
https://ad-hoc-expensify-cash.s3.amazonaws.com/desktop/42818/NewExpensify.dmg	https://42818.pr-testing.expensify.com

👀 View the workflow run that generated this build 👀

rlinoz

Thanks!

OSBotify · 2024-06-19T12:15:07Z

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

OSBotify · 2024-06-19T21:52:13Z

🚀 Deployed to staging by https://github.com/rlinoz in version: 1.4.86-0 🚀

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

OSBotify · 2024-06-19T22:16:55Z

🚀 Deployed to staging by https://github.com/rlinoz in version: 1.4.86-0 🚀

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

OSBotify · 2024-06-19T22:50:54Z

🚀 Deployed to staging by https://github.com/rlinoz in version: 1.4.86-0 🚀

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

kbecciv · 2024-06-20T15:55:47Z

This PR is failing because of issue #40963

The issue is reproducible in: Android app

1718883135213.42818_Android.mp4

skyweb331 · 2024-06-20T16:39:44Z

@kbecciv According to the video, back button works when you change button mode on Android system settings?
I think, this is a really rare case and it is impossible to block newly appeared back button while NewDot is running.
cc @sobitneupane

sobitneupane · 2024-06-21T06:24:06Z

I think, this is a really rare case

I agree with @skyweb331. I don't think anyone would just turn on/off the navigation bar in the middle of onboarding flow. This is a super rare case.

rlinoz · 2024-06-21T13:18:12Z

Yeah, I don't think that is a case we need to take in consideration.

rlinoz · 2024-06-21T13:19:56Z

@kbecciv if that is the only problem with the PR would you mind checking it off?

kbecciv · 2024-06-21T21:03:01Z

Yes, this is only problem, checking off

OSBotify · 2024-06-21T21:21:32Z

🚀 Deployed to production by https://github.com/AndrewGable in version: 9.0.0-9 🚀

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

skyweb331 requested a review from a team as a code owner May 30, 2024 07:42

melvin-bot bot requested review from sobitneupane and removed request for a team May 30, 2024 07:42

skyweb331 force-pushed the onboarding-dismiss branch from 406d448 to 2a2d2f5 Compare June 6, 2024 04:17

sobitneupane reviewed Jun 6, 2024

View reviewed changes

sobitneupane reviewed Jun 7, 2024

View reviewed changes

skyweb331 force-pushed the onboarding-dismiss branch 2 times, most recently from aa28e8b to 4f25ca2 Compare June 12, 2024 04:48

skyweb331 force-pushed the onboarding-dismiss branch from fb010f0 to e8eaf03 Compare June 17, 2024 17:56

disable back button behavior for android on onboarding

d4deb6b

skyweb331 force-pushed the onboarding-dismiss branch from e8eaf03 to d4deb6b Compare June 18, 2024 08:40

sobitneupane reviewed Jun 19, 2024

View reviewed changes

sobitneupane approved these changes Jun 19, 2024

View reviewed changes

melvin-bot bot requested a review from rlinoz June 19, 2024 09:09

rlinoz added the Ready To Build label Jun 19, 2024

rlinoz approved these changes Jun 19, 2024

View reviewed changes

rlinoz merged commit e0ff35b into Expensify:main Jun 19, 2024
25 of 28 checks passed

github-actions bot mentioned this pull request Jun 19, 2024

Deploy Checklist: New Expensify 2024-06-19 #44047

Closed

59 tasks

melvin-bot bot mentioned this pull request Jun 21, 2024

[HOLD for payment 2024-06-28] [$250] Android - Onboarding - Onboarding modal can be dismissed by tapping back button or swiping #40963

Closed

6 tasks

Block Onboarding modal dismiss on android #42818

Block Onboarding modal dismiss on android #42818

Conversation

skyweb331 commented May 30, 2024 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented May 30, 2024

sobitneupane left a comment • edited Loading

Choose a reason for hiding this comment

sobitneupane Jun 6, 2024 • edited Loading

Choose a reason for hiding this comment

skyweb331 Jun 6, 2024

Choose a reason for hiding this comment

sobitneupane Jun 6, 2024

Choose a reason for hiding this comment

skyweb331 Jun 6, 2024

Choose a reason for hiding this comment

sobitneupane Jun 7, 2024

Choose a reason for hiding this comment

skyweb331 Jun 7, 2024

Choose a reason for hiding this comment

skyweb331 Jun 9, 2024

Choose a reason for hiding this comment

skyweb331 commented Jun 6, 2024

sobitneupane left a comment

Choose a reason for hiding this comment

skyweb331 commented Jun 7, 2024

skyweb331 commented Jun 13, 2024

sobitneupane commented Jun 17, 2024

skyweb331 commented Jun 17, 2024

sobitneupane commented Jun 17, 2024

skyweb331 commented Jun 18, 2024

sobitneupane left a comment

Choose a reason for hiding this comment

Screenshots/Videos

sobitneupane commented Jun 19, 2024

sobitneupane left a comment • edited Loading

Choose a reason for hiding this comment

Reviewer Checklist

skyweb331 commented Jun 19, 2024

skyweb331 commented Jun 19, 2024

melvin-bot bot commented Jun 19, 2024

github-actions bot commented Jun 19, 2024

rlinoz left a comment

Choose a reason for hiding this comment

OSBotify commented Jun 19, 2024

OSBotify commented Jun 19, 2024

OSBotify commented Jun 19, 2024

OSBotify commented Jun 19, 2024

kbecciv commented Jun 20, 2024

skyweb331 commented Jun 20, 2024

sobitneupane commented Jun 21, 2024

rlinoz commented Jun 21, 2024

rlinoz commented Jun 21, 2024

kbecciv commented Jun 21, 2024

OSBotify commented Jun 21, 2024

skyweb331 commented May 30, 2024 •

edited

Loading

sobitneupane left a comment •

edited

Loading

sobitneupane Jun 6, 2024 •

edited

Loading

sobitneupane left a comment •

edited

Loading