[ECUK In-App 3DS] Adds new error handling system to Multifactor Authentication#82297
Conversation
…stead of route params Remove outcome path generation (outcomePaths.ts, mapMultifactorAuthenticationOutcomes.ts) and related types. OutcomePage now determines success/failure and specific error outcome entirely from MFA context state instead of route params. Split OUTCOME screen into OUTCOME_SUCCESS and OUTCOME_FAILURE as two separate routes pointing to the same component.
… name Eliminates redundant MULTIFACTOR_AUTHENTICATION_SCENARIO_CONFIG lookups by resolving the scenario config during INIT and storing it directly in state. Consumers now access scenario properties (OUTCOMES, MODALS, allowedAuthenticationMethods) directly from state.scenario.
…teScreenWithDefaults factory
Use IllustrationName with lazy loading in base components, make subtitle optional in FailureScreenBase, and create all default screens from DefaultUserInterface config.
…s screens Move screen defaults (successScreen, defaultFailureScreen, failureScreens) into DefaultUserInterface config. Scenarios can override per-reason failure screens via JSX elements. Simplify OutcomePage to resolve screens from scenario config. Remove OUTCOMES types and FailureScreenResolver.
…th httpStatus propagation Replace single defaultFailureScreen with defaultClientFailureScreen (4xx) and defaultServerFailureScreen (5xx). Propagate httpStatus and message through the error flow from parseHttpRequest to ErrorState. OutcomePage now routes to server failure screen when httpStatus starts with 5.
… single barrel export
…ase and SuccessScreenBase
…solution in FailureScreenBase
…ps separately for type inference
…screen and move Description components to components/
|
Hey, I noticed you changed If you want to automatically generate translations for other locales, an Expensify employee will have to:
Alternatively, if you are an external contributor, you can run the translation script locally with your own OpenAI API key. To learn more, try running: npx ts-node ./scripts/generateTranslations.ts --helpTypically, you'd want to translate only what you changed by running |
…at UNKNOWN_RESPONSE as server error
|
@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] |
|
🚧 @rafecolton has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
rafecolton
left a comment
There was a problem hiding this comment.
Nicely done! Tested and it works great, including magic code error.
src/components/MultifactorAuthentication/components/OutcomeScreen/OutcomeScreenBase.tsx
Show resolved
Hide resolved
src/components/MultifactorAuthentication/components/AuthenticationMethodDescription.tsx
Outdated
Show resolved
Hide resolved
src/components/MultifactorAuthentication/config/scenarios/BiometricsTest.tsx
Outdated
Show resolved
Hide resolved
| cancelButtonText: 'common.cancel', | ||
| }, | ||
| }, | ||
| failureHeaderTitle: 'multifactorAuthentication.verificationFailed', |
There was a problem hiding this comment.
Why is this here and not part of the failure screens?
There was a problem hiding this comment.
This is a simplification to meet the requirement:
- For transaction approve/deny failures, use red phone illustration and Transaction failed title and subtitle
- Use Transaction failed title (upper left) for transaction approve/deny failures (retain Oops, something went wrong subtitle and humpty dumpty illustration)
Of course, we can do this through the screenshots configuration, but then it would require adding the following to the approve transaction scenario:
defaultClientFailureScreen: <DefaultClientFailureScreen headerTitle="multifactorAuthentication.transactionFailed" />,
defaultServerFailureScreen: <DefaultServerFailureScreen headerTitle="multifactorAuthentication.transactionFailed" />,
failureScreens: {
[CONST.MULTIFACTOR_AUTHENTICATION.REASON.GENERIC.NO_ELIGIBLE_METHODS]: <NoEligibleMethodsFailureScreen headerTitle="multifactorAuthentication.transactionFailed" />,
[CONST.MULTIFACTOR_AUTHENTICATION.REASON.GENERIC.UNSUPPORTED_DEVICE]: <UnsupportedDeviceFailureScreen headerTitle="multifactorAuthentication.transactionFailed" />,
},There was a problem hiding this comment.
Hm that is an interesting point. We actually should not show the transaction review screen at all if the device is not supported or has no eligible methods (cc @JakubKorytko). It would create a bad UX - one of these two outcomes depending on how we implement it:
- Immediately fail the transaction when the user taps
Approve- requires the customer to redo the transaction - Let them close the modal and delete the review request from onyx locally - less bad but kinda pointless
For the other scenarios, it's fine to show those errors.
Regarding supplying headerTitle to the defaultClientFailureScreen and defaultServerFailureScreen values - I get what you're saying that we will save a little bit of config by supplying the default failure value once vs multiple times. However, we're already going to need a custom component for each of server/client failure for transaction review in order to supply the title, subtitle, and illustration. So I don't think saving ourselves a single config option is that valuable of an optimization. I'm more inclined to keep this top-level config clean and simple and let each outcome screen be explicit about its headerTitle value.
src/components/MultifactorAuthentication/config/scenarios/DefaultUserInterface.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Are there any efficiency implications of storing it this way? e.g. If it is static, do we save having to calculate the element by storing it this way?
JakubKorytko
left a comment
There was a problem hiding this comment.
Really good job, LGTM, small NABs not worth blocking
src/components/MultifactorAuthentication/components/AuthenticationMethodDescription.tsx
Outdated
Show resolved
Hide resolved
src/components/MultifactorAuthentication/config/scenarios/DefaultUserInterface.tsx
Show resolved
Hide resolved
# Conflicts: # src/components/MultifactorAuthentication/Context/Main.tsx # src/components/MultifactorAuthentication/Context/State.tsx # src/components/MultifactorAuthentication/config/scenarios/DefaultUserInterface.ts # src/components/MultifactorAuthentication/config/types.ts # src/libs/actions/MultifactorAuthentication/processing.ts
Resolve merge conflicts between the type refactor and outcome callback branches: fix stale OutcomePaths import, add missing MultifactorAuthenticationScenarioResponse import, map ProcessResult to ScenarioResponse correctly, and wire default callback into customConfig. Rename all httpCode/httpStatus variants to httpStatusCode for consistency.
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
The subtitle translation key requires {authType} params, but
SuccessScreenBase called translate(subtitle) without them, causing
a crash: "Cannot read property 'authType' of undefined".
|
🚧 @rafecolton has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
chuckdries
left a comment
There was a problem hiding this comment.
Tests pass flawlessly on Android ad-hoc, but the "ReactElement in state" thing is throwing me for a loop and I'm hoping you can help me understand exactly how FailureScreenBase works at all. It calls useMultifactorAuthenticationState, which means it must be rendered as a child of MultifactorAuthenticationStateProvider, but then it itself is stored within the state of that provider. That means it is created, by necessity, outside of that subtree, but it must be okay because it's always actually rendered within that subtree?
|
|
||
| type MultifactorAuthenticationTranslationParams = { | ||
| authType?: AuthTypeName; | ||
| authType?: string; |
There was a problem hiding this comment.
Was widening this type necessary? I'm not strongly against it - I don't know that narrowing this set of strings really provided much safety in the first place, but I see that AuthTypeName is still defined and used elsewhere, so why not use it here?
There was a problem hiding this comment.
Good catch - we should probably leave it given it is a fixed list
There was a problem hiding this comment.
Unfortunately, I had to change this because there's a string that we paste at the end of the translation. Previously, we didn't translate authType into different languages. But after completing the translations, we send the result of translate(AUTH_TYPE_TO_TRANSLATION_KEY(authType)) here, and it returns a string.
rafecolton
left a comment
There was a problem hiding this comment.
Left one comment on something I think should be changed, plus I agree with Chuck's comment about AuthTypeName. Tested well on iOS so I'm going to merge this - we're going to need to make some more changes when adding error screens for the transaction review flows, so we can do the minor refactors then.
| cancelButtonText: 'common.cancel', | ||
| }, | ||
| }, | ||
| failureHeaderTitle: 'multifactorAuthentication.verificationFailed', |
There was a problem hiding this comment.
Hm that is an interesting point. We actually should not show the transaction review screen at all if the device is not supported or has no eligible methods (cc @JakubKorytko). It would create a bad UX - one of these two outcomes depending on how we implement it:
- Immediately fail the transaction when the user taps
Approve- requires the customer to redo the transaction - Let them close the modal and delete the review request from onyx locally - less bad but kinda pointless
For the other scenarios, it's fine to show those errors.
Regarding supplying headerTitle to the defaultClientFailureScreen and defaultServerFailureScreen values - I get what you're saying that we will save a little bit of config by supplying the default failure value once vs multiple times. However, we're already going to need a custom component for each of server/client failure for transaction review in order to supply the title, subtitle, and illustration. So I don't think saving ourselves a single config option is that valuable of an optimization. I'm more inclined to keep this top-level config clean and simple and let each outcome screen be explicit about its headerTitle value.
src/components/MultifactorAuthentication/config/scenarios/DefaultUserInterface.tsx
Show resolved
Hide resolved
|
|
||
| type MultifactorAuthenticationTranslationParams = { | ||
| authType?: AuthTypeName; | ||
| authType?: string; |
There was a problem hiding this comment.
Good catch - we should probably leave it given it is a fixed list
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
🚧 @rafecolton has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
React works in such a way that JSX is really just a configuration that describes which component function to call during rendering. sucessScreen: <SucessScreenBase title="abc"/>is exactly the same as: Therefore, this component will only be called in the react renderer call tree, in the place where the successScreen value is inserted in the OutcomePage. |
|
🚀 Deployed to staging by https://github.com/rafecolton in version: 9.3.21-0 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.3.21-4 🚀
|
Explanation of Change
This PR replaces the route-parameter-based MFA outcome system with a context-driven, component-based architecture.
What changed:
Routing simplification — Replaced the single parameterized route (
/outcome/:outcomeType) with two static routes (/outcome/success,/outcome/failure). Outcome determination now happens entirely withinOutcomePageby reading MFA context state, eliminating the need for outcome path generation (outcomePaths.ts,mapMultifactorAuthenticationOutcomes.ts— both deleted).Client/server error split — Extended
ErrorStatewithhttpStatusandmessagefields propagated from the HTTP response.OutcomePagenow distinguishes between client errors (4xx →defaultClientFailureScreen) and server errors (5xx orUNKNOWN_RESPONSE→defaultServerFailureScreen), showing appropriate UI for each.Component-based screen resolution — Replaced
OUTCOMESconfiguration objects with React elements stored directly in scenario config. Each scenario can overridesuccessScreen,defaultClientFailureScreen,defaultServerFailureScreen, and per-reasonfailureScreens. AcreateScreenWithDefaultsfactory enables layered composition — specialized screens extend base screens by overriding only the props that differ.Shared base components — Extracted
OutcomeScreenBase,FailureScreenBase, andSuccessScreenBaseas reusable layout components. Default screens (DefaultClientFailureScreen,DefaultServerFailureScreen,DefaultSuccessScreen, etc.) are created via the factory and exported fromDefaultUserInterface.Scenario config stored in state — Full scenario config (including UI screens, modals, allowed methods) is resolved at
INITand stored in context state, replacing the previous pattern of storing only the scenario name and doing repeated lookups.Error resilience — Added try/catch for unhandled errors in the MFA process flow, and
AuthenticationMethodDescriptioncomponent with a fallback for missingauthType.Fixed Issues
$ #80537
PROPOSAL:
Tests
Unsupported device error (Web):
Success and failure screens (Mobile):
Server error (Web or Mobile):
Offline tests
N/A — MFA does not have offline-specific behavior; network failure is covered by the server error test above.
QA Steps
Same as Tests.
NOTE: This feature is behind a beta, so if it fails QA, it doesn't need a revert or a deploy blocker. Instead, please tag the PR reviewers in Slack and we'll decide how to move forward.
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
Web: Unsupported device
Mobile: Server failure
Mobile: Client failure
Mobile: Sucess