-
Notifications
You must be signed in to change notification settings - Fork 86
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
feat(onboarding): Keyless Backup Setup Fail in Onboarding Flow #5537
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5537 +/- ##
=======================================
Coverage 86.43% 86.44%
=======================================
Files 762 762
Lines 31425 31448 +23
Branches 5406 5416 +10
=======================================
+ Hits 27161 27184 +23
Misses 4033 4033
Partials 231 231
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
src/analytics/Properties.tsx
Outdated
@@ -265,6 +265,8 @@ interface KeylessBackupEventsProperties { | |||
[KeylessBackupEvents.cab_progress_completed_continue]: undefined | |||
[KeylessBackupEvents.cab_progress_failed_later]: undefined | |||
[KeylessBackupEvents.cab_progress_failed_manual]: undefined | |||
[KeylessBackupEvents.cab_progress_failed_manual_onboarding]: undefined |
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.
Can we pass an origin instead of having cab_progress_failed_manual
and cab_progress_failed_manual_onboarding
?
[KeylessBackupEvents.cab_progress_failed_manual]: {
origin: 'onboarding' | 'settings'
}
@@ -78,6 +78,7 @@ function KeylessBackupPhoneInput({ route }: Props) { | |||
navigate(Screens.KeylessBackupPhoneInput, { | |||
keylessBackupFlow, | |||
selectedCountryCodeAlpha2: countryCodeAlpha2, | |||
origin: route.params.origin, |
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.
nit: Can we destructure this from the route.params
earlier in the file?
@@ -69,7 +76,7 @@ function KeylessBackupProgress({ | |||
if (route.params.keylessBackupFlow === KeylessBackupFlow.Restore) { | |||
return <Restore /> | |||
} else { | |||
return <Setup /> | |||
return <Setup navigatedFromSettings={navigatedFromSettings} /> |
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.
I wonder if something such as isOnboarding
would be clearer; however, that also gets a bit muddled when someone could technically be onboarding to CAB from settings.
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.
I changed what was being passed so now origin is being passed (to make analytics easier). Now navigatedFromSettings is computed in the Setup component, can change the name if you want but I'm not sure if changing would make any more clear?
1 build had no size change
Celo (test) 1.87.0 (152)
|
@@ -90,6 +91,7 @@ function KeylessBackupPhoneInput({ route }: Props) { | |||
navigate(Screens.KeylessBackupPhoneCodeInput, { | |||
keylessBackupFlow, | |||
e164Number: phoneNumberInfo.e164Number, | |||
origin: route.params.origin, |
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.
nit: suggestion
origin: route.params.origin, | |
origin, |
@@ -69,7 +74,7 @@ function KeylessBackupProgress({ | |||
if (route.params.keylessBackupFlow === KeylessBackupFlow.Restore) { | |||
return <Restore /> | |||
} else { | |||
return <Setup /> | |||
return <Setup origin={route.params.origin} /> |
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.
nit: this can also be destructured with an additional const { origin } = route.params
in the code above.
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.
LGMT just some small nitpicks.
export enum KeylessBackupOrigin { | ||
Onboarding = 'Onboarding', | ||
Settings = 'Settings', | ||
} |
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.
nit: move this to keylessBackup/types since this is used in multiple places. This can potentially cause circular imports
### Description Addressing this comment: #5537 (comment) ### Test plan CI ### Related issues - Fixes #ACT-1222 ### Backwards compatibility Yes ### Network scalability If a new NetworkId and/or Network are added in the future, the changes in this PR will: - [X] Continue to work without code changes, OR trigger a compilation error (guaranteeing we find it when a new network is added)
Description
Added origin param to decide whether CAB flow was entered from Onboarding or Settings. The first screen that this param is passed to is SignInWithEmail, since that is the first CAB screen that is hit during onboarding.
Test plan
Unit tests added.
Manual:
Screen:
Recovery phrase:
recovery.mp4
Skip:
skip.mp4
Related issues
Backwards compatibility
Yes
Network scalability
If a new NetworkId and/or Network are added in the future, the changes in this PR will: