Fix and Revert "Revert "Display banner to enable 2FA when setting up VBBA""

[MonilBhavsar]

Details

Reverts and updates to the previous PR to fix the functionality

Reverts #13407

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/238668

Tests

Let user have 2FA disabled
Tests with https://github.com/Expensify/Web-Expensify/pull/35988.

1. Checkout that branch
2. Run app
3. Go to Workspace > Connect bank account
4. Add a bank account in a PENDING state, go till Validation step and confirm the 2FA prompt
5. Click on secure account and confirm it redirects to olddot, enables 2FA.
6. Finish the setup and confirm it redirects back to newdot and this time it has no prompt
7. Go to olddot and disable 2FA
8. I manually changed account state, to go through validation state and confirmed the 2FA prompt there

Notes:

1. On iOS safari, you need to go to settings > Safari > Disable block popups
2. On iOS Safari, update settings to request desktop site always to load olddot
3. Change URL_TO_NEWDOT in dev-config on olddot to reflect ngrok url instead of localhost:8080 (localhost doesn't work on android chrome)
4. Change EXPENSIFY_URL in App .env to reflect your ngrok url so that the linking from App to olddot works.

• Verify that no errors appear in the JS console

Offline tests

Doesn't work offline

QA Steps

Prerequisite: User should have 2FA disabled in olddot > Settings > Accounts

1. Go to Workspace > Connect bank account and start the process of setting up bank account in VERIFYING state
2. Go till Verifying state and confirm the 2FA prompt as in screenshot
3. Click on "Secure Account" and confirm you're redirected to olddot and 2FA modal is open.
4. Finish the 2FA setup. Upon clicking Finish, confirm you're redirected back to newdot
5. Confirm this time there is not any 2FA prompt.

Now,

7. Disable 2FA from olddot
8. Go to bank account setup again. Go to Workspace > Connect bank account
9. Wait till Bank account is verified and you go to validation step and see the inputs to enter 3 amounts
10. Confirm 2FA prompt as in screenshot
11. Click on "Secure Account" and confirm you're redirected to olddot and 2FA modal is open.
12. Finish the 2FA setup. Upon clicking Finish, confirm you're redirected back to newdot
13. Confirm this time there is not any 2FA prompt.

• 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 / Chrome
  • iOS / native
  • iOS / 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 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 correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
  • 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 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(themeColors.componentBG)
• 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 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.
• I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

23/01

Screen.Recording.2023-01-23.at.4.29.37.PM.mov

Web

Screen.Recording.2023-01-06.at.10.41.19.AM.mov

Mobile Web - Chrome

Screen.Recording.2023-01-06.at.6.41.56.PM.mov

Mobile Web - Safari

Screen.Recording.2023-01-06.at.5.56.57.PM.mov

Desktop

Screen.Recording.2023-01-06.at.4.12.35.PM.mov

iOS

Screen.Recording.2023-01-06.at.6.01.38.PM.mov

Android

Screen.Recording.2023-01-06.at.7.18.10.PM.mov

[melvin-bot]

@mananjadhav @dangrous One of you needs to 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]

[mananjadhav]

@MonilBhavsar Is the API available on staging to test and review?

[MonilBhavsar]

Not until, we deploy the Web PR. Will notify here, once it is deployed and this PR is unheld

[dangrous]

While we're waiting for the Web-E pull, a quick question - it looks like there are two places where this isn't quite a revert:

• https://github.com/Expensify/App/pull/14029/files#diff-c2f568552061f0bd108cc07b83e68feaf0fc95f3dea86dcd69772e302d390f71R24 differs from https://github.com/Expensify/App/pull/13407/files#diff-c2f568552061f0bd108cc07b83e68feaf0fc95f3dea86dcd69772e302d390f71L25 (and the associated import)
• These changes didn't exist before either

Am I interpreting that right? Totally fine either way but want to make sure I'm not missing anything. Thanks!

[MonilBhavsar]

@dangrous yes correct! This is not a straight revert, in fact a revert with some amendments. Looks like you got the changes that differs from the previous PR that was reverted 😄
Also, Web-E PR was merged, so you'll be able to test by checking out main. Please make sure to run grunt. Thanks!

[dangrous]

Hey! So tested this on newDot web and got through setting up 2FA on oldDot. However when I click the "Finish" button on oldDot after setting it up, instead of redirecting back to the App it goes to https://www.expensify.com.dev/bank-account/ which is a 404 error. Is this known?

The app side of things, though, DOES work - the prompt appears if 2FA is not enabled, and disappears if it is (on a page reload). Haven't tested the other platforms yet due to the above.

[MonilBhavsar]

@mananjadhav you'll be now able to test on staging

[MonilBhavsar]

Hey! So tested this on newDot web and got through setting up 2FA on oldDot. However when I click the "Finish" button on oldDot after setting it up, instead of redirecting back to the App it goes to https://www.expensify.com.dev/bank-account/ which is a 404 error. Is this known?

Hmm, not known. Could you please confirm, you have checked out main, ran grunt on Web-E.
And URL_TO_NEW_DOT in dev config points to a Newdot URL.

[mananjadhav]

Okay thanks. I'll get to this today.

[dangrous]

@MonilBhavsar Apologies, I thought I had run grunt but maybe I didn't. Works like a charm now! I'll wait for @mananjadhav to confirm the testing but looking good and we can merge shortly.

[mananjadhav]

I just testing this and should have the checklist in 30mins-1 hour.

[mananjadhav]

I am a little blocked for the testing, @MonilBhavsar @dangrous. Can you help me what can be done here? I tried adding. multiples non public emails, and refresh. It blocks me later for multiple secondary attempts. Can't get past this step.

[dangrous]

So I think that might be a different flow - are you adding a bank account in a VERIFYING state? I think that way you won't hit that screen. I realize now that that instruction is in the QA Steps but not testing, I'll update the description quickly.

[mananjadhav]

@dangrous I am not sure what you mean by adding the bank account in Verifying State.

Here's the steps that I am following:

1. Go to Settings -> Workspaces
2. Choose any workspace -> Connect bank account
3. Connect via Plaid
4. Followed the steps to add the account
5. Takes me to Company Information
6. Next Personal Information
7. Next Additional Information, and on continue I get stuck on this screen.

[MonilBhavsar]

Ah yes, this needs to be tested internally because of bank accounts, I think

[MonilBhavsar]

No worries!

@dangrous I tried again today, in a different browser and could not reproduce that issue

Screen.Recording.2023-01-20.at.11.24.21.PM.mov

[MonilBhavsar]

However, a question - this flow shouldn't actually work right now for real users - there's no way for a user to scan the QR code to get 2FA set up, except if they have two devices. Is this an issue, and/or is it okay to skip this testing altogether?

Correct, also olddot doesn't work on mobile web, unless one selects "Request a desktop" site, but it would be good to test it as a part of process.

iOS and Android both redirect the user to newDot in the browser, rather than the app itself. This would be a Web-E issue, do we know if it's being worked on?

Yes, in the physical, I think there is an option in android and iOS to open that in the app.

[dangrous]

Okay cool - so how do you want to move forward? Should I fill out the checklist with the caveat that I was unable to confirm the behavior on mobile web, but that it won't be a flow that needs to work for the end user? Or are we able to get someone else to test? What do you think?

I'm hoping the desktop flakiness is just on my end, but would be great to have one more test of that too, since I have no idea why it's happening for me

(Also merge conflicts now, sorry!)

[MonilBhavsar]

I think we should test it. @mountiny @youssef-lr if you could help us test and complete the checklist

[youssef-lr]

Testing now!

[dangrous]

Thanks for jumping in @youssef-lr ! Sorry my testing keeps causing so much trouble

[mountiny]

Have you had success? @youssef-lr I will try but not sure if I will get around to this, gotta deploy web today

[youssef-lr]

Sorry @mountiny, I had some issues in my VM after updating everything, and had to do a destroy+up, it's now working properly and I'm back to testing! Will report back here as soon as I'm done.

[youssef-lr]

Testing on iOS is working as expected! I added a 4th step to the notes in the issue description as I needed to do this for linking to work.

4. Change EXPENSIFY_URL in App .env to reflect your ngrok url so that the linking from App to olddot works.

RPReplay_Final1674494460.2.MP4

Will update this comment with Chrome Android results.

[mountiny]

Oh nice, thanks @youssef-lr

@MonilBhavsar if we got that one from Youssef, seems like we might be able to merge this one, right?

[youssef-lr]

Seems like the issue @dangrous was originally having occurs on Android Chrome, when I'm taken back to NewDot, I find myself logged out.

Screen.Recording.2023-01-23.at.20.06.31.mov

[dangrous]

Hmm @MonilBhavsar any thoughts? On the one hand I'm happy that it's not just me, but on the other hand, now we have to figure out why haha

[mountiny]

Crazy thought, should we add the 2FA set up to NewDot right now, I mean it will have to be added eventually and probably quite soon. These bridges over to oldDot and back will always been a bit wobly.

Or, we can also deploy this to staging, keep an eye on it, let QA test this out wild on all platforms and revert in case it does not work. Since we can see it works for some and less for others it might be due to the fact that this portal between New and Old dot is not that great in dev. If @MonilBhavsar tested this and it worked for him and it worked fine for almost all platforms to the other reviewers, then we should be fine getting this out. Thoughts?

[dangrous]

I might agree with you @mountiny given that the tested flow is not actually a viable flow except on desktop - all mobile implementations, whether mWeb or the app, require the user to request the desktop app and then use a different device to scan the QR code, which is.... very unlikely. So as long as we're sure desktop (definitely works) and desktop web (mostly works?) work, I think we would be okay.

It does bother me that we don't yet know the source of the flakiness, but maybe that's a separate issue?

[mountiny]

It does bother me that we don't yet know the source of the flakiness, but maybe that's a separate issue?

Seems like at the start the QA/Tests/ set up steps were not clear or not everyone had the same environment set up, I feel like that might have contirbuted.

I feel like we should move on, lets merge this and ping QA to specially test this flow on all platforms once in staging to ensure its working. Its not extremely used flow so I feel like this should work out.

[dangrous]

Cool, I think that makes sense for now! @mountiny I'll let you do the honors.

[mountiny]

@MonilBhavsar Are you happy with the plan? Would you be able to contact QA when this would be merged and deployed?

If this worked for you locally on mobile web as well, I believe we can go ahead. Feel free to self-merge once you happy with this!

[MonilBhavsar]

Hey! Sorry for some delay here. back from some ooo.
I am having a sync today with @youssef-lr to figure out what's wrong with android, since it worked for youssef on all platforms (except android)

[MonilBhavsar]

we tested yesterday and it worked for @youssef-lr
I tested again after building everything and it worked for me too. So merging it

2FA.testing.720p.mov

[OSBotify]

🚀 Deployed to staging by https://github.com/MonilBhavsar in version: 1.2.64-1 🚀

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

[OSBotify]

🚀 Deployed to production by https://github.com/thienlnam in version: 1.2.64-7 🚀

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

[OSBotify]

🚀 Deployed to production by https://github.com/thienlnam in version: 1.2.64-7 🚀

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

[OSBotify]

🚀 Deployed to production by https://github.com/thienlnam in version: 1.2.64-7 🚀

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