-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Redirect internally for 2FA management from bank account step #30746
Conversation
@aimane-chnaif 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] |
I don't think so. You'll need to complete onfido check |
Conflicts. There's function migration refactor of ReimbursementAccountPage merged in main but let's see how it goes, as it caused multiple regressions. |
Conflicts resolved! |
Resolved conflict again! |
sure, I will review and test during weekend |
This kind of bug is usually reported though it's not normal case so let's fix that: Refresh on 2fa flow and click back button -> should go back to bank account step Screen.Recording.2023-11-26.at.8.00.12.PM.mov |
}; | ||
|
||
function Enable2FAPrompt(props) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we're touching this file, let's destructure props which is new common pattern
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay 👍
Looks like this is navigation bug in general. When we refresh the page, we lose the navigation stack and don't know where to redirect back to |
We get that from Or if you think out of scope, create new GH and handle it separately |
Yes, I think we'll be able to update it. I'll push a fix |
I have pushed a fix. @aimane-chnaif Let me know |
clearTwoFactorAuthData(); | ||
Navigation.goBack(ROUTES.SETTINGS_SECURITY); | ||
Navigation.goBack(backTo ? backTo : ROUTES.SETTINGS_SECURITY); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To fix TS lint:
Navigation.goBack(backTo ? backTo : ROUTES.SETTINGS_SECURITY); | |
Navigation.goBack(backTo ? `${backTo}` : ROUTES.SETTINGS_SECURITY); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't using backTo || ROUTES.SETTINGS_SECURITY
the right way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@blazejkustra what's the best practice for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My suggestion would be fine. Otherwise just disable lint for this line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
null coalesce won't work in this case. I think it's fair to use ||
in this case
src/pages/settings/Security/TwoFactorAuth/StepWrapper/StepWrapper.js
Outdated
Show resolved
Hide resolved
src/pages/settings/Security/TwoFactorAuth/Steps/DisabledStep.js
Outdated
Show resolved
Hide resolved
Co-authored-by: Aimane Chnaif <96077027+aimane-chnaif@users.noreply.github.com>
TS check still failing |
Reviewer Checklist
Screenshots/VideosAndroid: Nativeandroid-msafari.movAndroid: mWeb Chromemchrome.moviOS: Nativeios.moviOS: mWeb Safariandroid-msafari.movMacOS: Chrome / Safariweb.movweb2.movMacOS: Desktopdesktop.mov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -8,7 +8,7 @@ import {TwoFactorAuthStep} from '@src/types/onyx/Account'; | |||
* Clear 2FA data if the flow is interrupted without finishing | |||
*/ | |||
function clearTwoFactorAuthData() { | |||
Onyx.merge(ONYXKEYS.ACCOUNT, {recoveryCodes: '', twoFactorAuthSecretKey: '', twoFactorAuthStep: '', codesAreCopied: false}); | |||
Onyx.merge(ONYXKEYS.ACCOUNT, {recoveryCodes: null, twoFactorAuthSecretKey: null, twoFactorAuthStep: null, codesAreCopied: false}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NAB: what's this change for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to remove these data navigating away from 2FA setup. Setting it as null, removes the onyx data.
@cristipaval 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] |
🎯 @aimane-chnaif, thanks for reviewing and testing this PR! 🎉 An E/App issue has been created to issue payment here: #32163. |
🚀 Deployed to production by https://github.com/mountiny in version: 1.4.5-7 🚀
|
Details
Now that 2FA management inside App, redirect user to 2FA page and then back to bank account step, instead of redirecting to oldDot app
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/278552
PROPOSAL:
Tests
Offline tests
None, as bank account and redirect doesn't work
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Screen.Recording.2023-11-02.at.2.11.43.PM.mov
Android: mWeb Chrome
Screen.Recording.2023-11-02.at.2.56.59.PM.mov
iOS: Native
Screen.Recording.2023-11-02.at.2.24.40.PM.mov
iOS: mWeb Safari
Screen.Recording.2023-11-02.at.2.36.01.PM.mov
MacOS: Chrome / Safari
Screen.Recording.2023-11-02.at.12.50.01.PM.mov
MacOS: Desktop
Screen.Recording.2023-11-02.at.12.53.26.PM.mov