[No QA] [ECUK In-App 3DS] Add BIOMETRICS_TEST scenario screens & routes + add test biometrics button to the troubleshoot menu#79475
Conversation
b3af9c4 to
ecc621d
Compare
BIOMETRICS_TEST scenario screens & routesBIOMETRICS_TEST scenario screens & routes + add test biometrics button to the troubleshoot menu
BIOMETRICS_TEST scenario screens & routes + add test biometrics button to the troubleshoot menuBIOMETRICS_TEST scenario screens & routes + add test biometrics button to the troubleshoot menu
70ab22a to
6cd18f7
Compare
|
@DylanDylann 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] |
src/components/MultifactorAuthentication/NoEligibleMethodsDescription.tsx
Outdated
Show resolved
Hide resolved
src/components/MultifactorAuthentication/ValidateCodeResendButton.tsx
Outdated
Show resolved
Hide resolved
ab160dd to
7916466
Compare
7916466 to
a1e4ba7
Compare
rafecolton
left a comment
There was a problem hiding this comment.
Overall feedback:
- Add lots of comments explaining both what (if not extremely obvious) and especially why
- Seems like some duplication of magic code screens, I wonder it can be DRY'd
- Wondering if the MFA context needs to be included in this PR - discussing in Slack here
src/components/MultifactorAuthentication/NoEligibleMethodsDescription.tsx
Outdated
Show resolved
Hide resolved
| onBackButtonPress={onGoBackPress} | ||
| shouldShowBackButton | ||
| /> | ||
| <FullPageOfflineBlockingView> |
There was a problem hiding this comment.
This is a little bit different from the mocks and my understanding of how we were going to design this. I understand that we need to open an RHP / modal in order to do the biometrics and show success/failure screens. However, I expected that there would be no starting screen. Basically, when you press the button on the row in the settings page, it would do what the "Biometrics test" page does in your recording. There might be an invisible RHP page (implementation detail) but no need for the user to tap an additional button to initiate the test. That also result in the FaceID prompt displaying over top of the settings page, rather than over top of the "Biometrics test" page.
Am I understanding that right or did we change the decision somewhere? Is it possible to do it the way I've described above?
There was a problem hiding this comment.
@JakubKorytko I have no context for this, will you reply?
There was a problem hiding this comment.
Screen.Recording.2026-01-19.at.12.42.29.mov
resolving, invisible screen added
There was a problem hiding this comment.
Unresolving so future reviewers can see the video. This is very cool! 😍
| inputCode?: TranslationPaths; | ||
| }; | ||
|
|
||
| function MultifactorAuthenticationValidateCodePage() { |
There was a problem hiding this comment.
This seems to be reimplementing a validate code page that I'm fairly certain we use elsewhere in the app - can we reuse that content from anywhere else for DRY purposes?
There was a problem hiding this comment.
Hmm, you might be right that we can reuse the ValidateCodeForm component. But that carries the risk that its implementation is missing something necessary for MFA.
I've talked to Kuba about this, and we want to add the current code to make MFA faster, and then we can revisit this issue.
It might be a good idea to create a refactor issue on Github.
There was a problem hiding this comment.
Yes, we do have something like this page, but those pages do much more than we need and we would have to adjust already big components in order to make them fit our case. More reasonable approach would be to refactor all those validateCodeForm components
There was a problem hiding this comment.
We can consider using ValidateCodeActionContent component here
There was a problem hiding this comment.
Discussed in Slack here. If you feel there are other components that can be easily reused, please make a follow-up issue. If not, the we can leave it. Any change that requires a major refactor of an unrelated flow is not worth making IMO, as it has a high risk of introducing bugs and will require extensive testing. Not to mention validate code flows are extremely important.
There was a problem hiding this comment.
We can consider using ValidateCodeActionContent component here
@DylanDylann @rafecolton @JakubKorytko
After revisiting the topic, I remembered what originally led us to creating a completely new ValidateCodePage. It was because of the fallback flow. I have completely forgotten about it's removal (or existence 😅).
At the time, this approach made a lot of sense. We supported a fallback to magic code + 2FA/SMS when biometric authentication failed, which meant we had to handle those 2 code-based factors (I mean 2FA/SMS) anyway, so it made sense to just keep everything MFA-related in one place and just add MFA magic code handling there too.
The main arguments for this approach were:
- We kept the handling of all code-based MFA validation factors in one place - whether it was magic code, SMS OTP, or authenticator code. Since we had to introduce a new page anyway, it felt more reasonable to include magic code validation there (especially that we wanted a singular call to BE with 2 factors so it had to behave differently) rather than modifying the well-established
ValidateCodeActionContentcomponent. ValidateCodeActionContentis a “one code -> one action” component, which was problematic for the fallback flow. In that flow, we first had to collect both factors and only then make a single BE call. Supporting this would have required significant changes toValidateCodeActionContent(specificallyValidateCodeForm), which didn’t seem like a good idea.
Now that this flow is no longer in place, whenever a magic code is required it always involves a single code triggering a single action. In this context, reusing ValidateCodeActionContent does seem reasonable, although it would still require some BE adjustments, making it a moderate-sized refactor, which would be better to do as a followup without rushing.
There was a problem hiding this comment.
Interesting, thanks for the explanation. What BE adjustments would it require? I'm not sure whether it's worth it to make BE adjustments and do a "moderate-sized refactor" even if we give ourselves plenty of time. But I'm open to it 😄 and curious to learn more.
There was a problem hiding this comment.
@jakubkalinski0 bump, perhaps move this convo to slack?
- Improve MFA component naming and styles - Reuse ValidateCodeCountdown in MFA ValidateCodeResendButton - Move ValidateCodeCountdown from pages/signin/ to components/ for reusability - Refactor MultifactorAuthenticationValidateCodeResendButton to use ValidateCodeCountdown instead of duplicating timer logic - Update imports in BaseValidateCodeForm (signin and ValidateCodeActionModal) - Adjust BiometricsTestPage - Rename constants in NoEligibleMethodsDescription for clarity: - base -> baseTranslationPath - tPaths -> translationPaths - Add dedicated mfaBlockingViewAnimation style instead of reusing emptyLHNAnimation - Update PromptContent to use the new MFA-specific animation style - Add TODO comments for mocked configs in NotificationPage - Add TODO comment for mockedConfig in TriggerCancelConfirmModal
35184d0 to
fd89257
Compare
| inputCode?: TranslationPaths; | ||
| }; | ||
|
|
||
| function MultifactorAuthenticationValidateCodePage() { |
There was a problem hiding this comment.
We can consider using ValidateCodeActionContent component here
| inputCode?: TranslationPaths; | ||
| }; | ||
|
|
||
| function MultifactorAuthenticationValidateCodePage() { |
There was a problem hiding this comment.
Discussed in Slack here. If you feel there are other components that can be easily reused, please make a follow-up issue. If not, the we can leave it. Any change that requires a major refactor of an unrelated flow is not worth making IMO, as it has a high risk of introducing bugs and will require extensive testing. Not to mention validate code flows are extremely important.
| onBackButtonPress={onGoBackPress} | ||
| shouldShowBackButton | ||
| /> | ||
| <FullPageOfflineBlockingView> |
There was a problem hiding this comment.
Unresolving so future reviewers can see the video. This is very cool! 😍
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/rafecolton in version: 9.3.5-0 🚀
|
|
@jakubkalinski0 @rafecolton is it no QA PR, right? |
|
@IuliiaHerets correct |
|
🚀 Deployed to production by https://github.com/Beamanator in version: 9.3.5-7 🚀
|
BIOMETRICS_TEST scenario screens & routes + add test biometrics button to the troubleshoot menuBIOMETRICS_TEST scenario screens & routes + add test biometrics button to the troubleshoot menu
Explanation of Change
This PR adds the components & pages used for the
BIOMETRICS_TESTMultifactor Authentication Scenario.At the current state, the screens simply navigate between each other - the correct logic will be added as a part of this PR: #79482
This allows us to test the UI, animations, and overall appearance.
It also adds the test biometrics button to the Troubleshoot menu so we can start the navigation flow.
For now, the native biometrics flow is available on the web, though it will be context-restricted in the future.
This is because the same components & pages with different images & text will be used for the passkeys later on, so their appearance can be tested on the web now.
co-author: @jakubkalinski0
Fixed Issues
$ #79373
$ #79377
PROPOSAL: N/A
Tests
Offline tests
N/A,
D - Full Page Blocking UI Patternfor this project.QA Steps
Same as tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Web.mp4