fix(profile): register new screens for settings, help, invite #5121

finnian0826 · 2024-03-18T17:18:54Z

Description

Registers new screens and modifies headers so can navigate back.

Test plan

Unit tests updated.

Manual tests:

Feature gate false:

old.mp4

Feature gate true:

new.mp4

Related issues

Fixes ACT-1132

Backwards compatibility

Network scalability

If a new NetworkId and/or Network are added in the future, the changes in this PR will:

N/A

MuckT · 2024-03-18T19:47:06Z

src/account/Settings.tsx

@@ -435,7 +435,7 @@ export const Account = ({ navigation, route }: Props) => {
  }

  return (
-    <SafeAreaView style={styles.container}>
+    <SafeAreaView style={styles.container} edges={['bottom']}>


Does edges need to be conditional also?

codecov · 2024-03-18T20:16:34Z

Codecov Report

Attention: Patch coverage is 90.32258% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 85.69%. Comparing base (3bc5f91) to head (8e63bfc).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5121      +/-   ##
==========================================
+ Coverage   85.56%   85.69%   +0.12%     
==========================================
  Files         729      729              
  Lines       29823    29833      +10     
  Branches     5144     5154      +10     
==========================================
+ Hits        25519    25565      +46     
+ Misses       4068     4033      -35     
+ Partials      236      235       -1

Files	Coverage Δ
src/account/Settings.tsx	`91.70% <100.00%> (+0.14%)`	⬆️
src/account/Support.tsx	`100.00% <100.00%> (ø)`
src/backup/BackupQuiz.tsx	`84.89% <100.00%> (+19.42%)`	⬆️
src/keylessBackup/KeylessBackupProgress.tsx	`99.20% <100.00%> (ø)`
src/navigator/DrawerNavigator.tsx	`93.89% <ø> (ø)`
src/navigator/ProfileMenu.tsx	`98.00% <100.00%> (+6.00%)`	⬆️
src/navigator/Screens.tsx	`100.00% <100.00%> (ø)`
src/pincode/PincodeSet.tsx	`94.69% <100.00%> (ø)`
src/backup/BackupPhrase.tsx	`87.32% <75.00%> (-0.18%)`	⬇️
src/backup/BackupComplete.tsx	`77.41% <33.33%> (ø)`

... and 4 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3bc5f91...8e63bfc. Read the comment docs.

emerge-tools · 2024-03-18T20:58:30Z

1 build increased size

Name	Version	Download	Change	Install	Change	Approval
Celo (test) org.celo.mobile.test	1.80.0 (145)	24.2 MB	⬆️ 893 B	60.4 MB	⬆️ 4.1 kB	N/A

Celo (test) 1.80.0 (145)
org.celo.mobile.test

⚖️ Compare build
📦 Install build
⏱️ Analyze build performance

Total install size change: ⬆️ 4.1 kB
Total download size change: ⬆️ 893 B

Largest size changes

Item	Install Size Change
main.jsbundle	⬆️ 4.1 kB

🛸 Powered by Emerge Tools

satish-ravi · 2024-03-18T21:41:56Z

src/backup/BackupComplete.tsx

+        navigate(
+          getFeatureGate(StatsigFeatureGates.USE_TAB_NAVIGATOR)
+            ? Screens.Settings
+            : Screens.SettingsDrawer,


I believe this will lead to a crash if a feature gate changes when the app is backgrounded. Can you test the follow scenario?

Have the feature on for your wallet, navigate to this screen (or any of the below screens)

Background the app

Change your wallet to be on the false variant and give a few mins for things to be updated

Foreground the app and invoke the action to go to settings

@satish-ravi you're right, I get this error when following this procedure to go back to settings. Is there a way to do navigate to make sure the screens are available, or should it just navigate home?

you can navigate to Screens.DrawerNavigator with initial screen name as Screens.SettingsDrawer, there should be examples of others.

satish-ravi · 2024-03-18T21:42:58Z

src/backup/BackupComplete.tsx

-        navigate(Screens.Settings, { promptConfirmRemovalModal: true })
+        navigate(
+          getFeatureGate(StatsigFeatureGates.USE_TAB_NAVIGATOR)
+            ? Screens.Settings


Wonder if we have to clear the stack before navigating to the new settings screen with back. Previously we landed back on the drawer and there was no way for the user to go back, but wonder if this opens up the possibility on landing on an inconsistent state in the app.

satish-ravi · 2024-03-18T21:44:10Z

src/pincode/PincodeSet.test.tsx

@@ -166,7 +166,7 @@ describe('Pincode', () => {
    })

    expect(updatePin).toHaveBeenCalledWith(mockAccount.toLowerCase(), oldPin, mockPin)
-    expect(navigate).toBeCalledWith(Screens.Settings)
+    expect(navigate).toBeCalledWith(Screens.SettingsDrawer)


better to switch any test that checks settings navigation to run against the true variant (or test both scenarios). Might also be worth wrapping navigating to settings into a helper function given my comments above. I don't think it will be as simple as navigating one or the other screen

src/account/Support.tsx

satish-ravi · 2024-03-19T20:08:47Z

src/navigator/NavigationService.ts

@@ -231,6 +231,12 @@ export function navigateHome(fromModal?: boolean) {
  }, timeout)
 }

+export function navigateToSettings(promptConfirmRemovalModal: boolean) {
+  getFeatureGate(StatsigFeatureGates.USE_TAB_NAVIGATOR)
+    ? navigateClearingStack(Screens.Settings, { promptConfirmRemovalModal })


actually would the user get stuck on the settings screen here? this would reset settings to the first screen. wonder if we should just navigate home, although not sure if it will break stuff. Maybe good to check in with product as well

yeah I was thinking of just navigating to home, I'll check with product and see

I synced with Laura on this and we don't want to navigate home instead of settings in most places. Seems like there's only a handful of places where we navigate to settings. So let's do this:

change keyless backup progress screen to navigate home instead of settings (as the flow can start from either settings or notifications screen)

to solve the edge case of feature gate changing when the user is on the Settings screen and backgrounded, I think we can get away with using the same screen enum. If we do that, then the version of settings screen that the flow started with will already be on the stack, so any navigate(Screen.Settings) would just pop back to that screen, instead of adding the new version of the screen to the stack. This should work as long as there is no flow that starts from somewhere else in the app and ends in settings, which I believe is only the keyless backup progress screen which we've addressed above.

satish-ravi · 2024-03-20T17:11:32Z

src/navigator/Navigator.tsx

+    <Navigator.Screen
+      name={Screens.Settings}
+      component={SettingsScreen}
+      options={isTabNav ? headerWithBackButton : noHeader}


could we just register the back button always? I think this version of the screen should only be used in the tab navigator anyway. The screens I believe are registered at startup and won't be refreshed if the gate changes when app is backgrounded.

MuckT

@finnian0826 Thanks for working to getting this across the finish line! 🚀

satish-ravi · 2024-03-20T21:24:16Z

src/backup/BackupQuiz.tsx

-      return navigatedFromSettings ? (
-        <CancelButton onCancel={onCancel} style={styles.cancelButton} />
+      return settingsScreen ? (
+        <CancelButton onCancel={() => navigateHome()} />


probably fine to go home here but the settingsScreen param now allows us to retain existing behavior right? We can navigate similar to what is done in BackupComplete?

satish-ravi · 2024-03-20T21:24:36Z

src/backup/BackupQuiz.tsx

-  cancelButton: {
-    color: colors.gray4,
-  },


same question as above

satish-ravi · 2024-03-20T21:26:44Z

src/backup/BackupPhrase.tsx

    return (
      <SafeAreaView style={styles.container}>
        <CustomHeader
          style={{ paddingHorizontal: variables.contentPadding }}
          left={
-            navigatedFromSettings ? (
-              <CancelButton style={styles.cancelButton} />


seems like this was just navigating back previously ? could we retain the same behavior here?

satish-ravi · 2024-03-20T21:27:46Z

src/backup/BackupPhrase.tsx

-  cancelButton: {
-    color: colors.gray4,
-  },


any reason to remove this styling?

satish-ravi · 2024-03-20T21:58:40Z

src/backup/BackupComplete.tsx

  const backupCompleted = useSelector(backupCompletedSelector)
  const { t } = useTranslation()

  useEffect(() => {
    const timer = setTimeout(() => {
-      if (navigatedFromSettings) {
-        navigate(Screens.Settings, { promptConfirmRemovalModal: true })
+      if (settingsScreen) {


might be worth leaving a cleanup comment here. I liked your naming on isAccountRemoval, maybe we can do that when we clean the drawer ? Should be also fine to just describe this in the linear issue @MuckT created.

### Description Reverts changes done in #5121 , can just pass a boolean to the that there's a single Settings screen ### Test plan CI, manually trying reset ### Related issues - Part of ACT-1133 ### Backwards compatibility Yes ### Network scalability N/A

…-inc#5121) ### Description Registers new screens and modifies headers so can navigate back. ### Test plan Unit tests updated. **Manual tests:** Feature gate false: https://github.com/valora-inc/wallet/assets/140328381/74b676ea-334a-4c49-ae17-f868ac98b776 Feature gate true: https://github.com/valora-inc/wallet/assets/140328381/91ded2a7-d54f-43a8-8f65-a33ac8a3fbaf ### Related issues - Fixes ACT-1132 ### Backwards compatibility  ### Network scalability If a new NetworkId and/or Network are added in the future, the changes in this PR will: - N/A --------- Co-authored-by: Tom McGuire <Mcgtom10@gmail.com> Co-authored-by: Satish Ravi <satish.ravi@valoraapp.com>

…inc#5336) ### Description Reverts changes done in valora-inc#5121 , can just pass a boolean to the that there's a single Settings screen ### Test plan CI, manually trying reset ### Related issues - Part of ACT-1133 ### Backwards compatibility Yes ### Network scalability N/A

finnian0826 added 3 commits March 18, 2024 11:18

fix(profile): register new screens for settings, help, invite

0675975

fix weird spacing

d553016

add root state schema

e53f345

MuckT reviewed Mar 18, 2024

View reviewed changes

fix tests

8fcafe3

finnian0826 added 2 commits March 18, 2024 14:34

fix stuff

2b81903

Merge branch 'main' into finnian0826/settings-help-invite

c78a616

finnian0826 marked this pull request as ready for review March 18, 2024 20:58

satish-ravi reviewed Mar 18, 2024

View reviewed changes

MuckT reviewed Mar 18, 2024

View reviewed changes

src/account/Support.tsx Outdated Show resolved Hide resolved

finnian0826 added 2 commits March 19, 2024 12:39

feedback

9cbdbda

fix tests

3f73faf

satish-ravi reviewed Mar 19, 2024

View reviewed changes

MuckT and others added 2 commits March 20, 2024 08:20

Merge branch 'main' into finnian0826/settings-help-invite

a9c93c5

feedback

4a89917

satish-ravi reviewed Mar 20, 2024

View reviewed changes

finnian0826 and others added 11 commits March 20, 2024 12:50

fix

0bc2158

root state schema

f54302f

fix again

c06b3ab

cleanup

d7b5a5a

Merge branch 'main' into finnian0826/settings-help-invite

4ec8b34

fixes

4ed9cd9

fix tests

f23858a

fix: set isTabNav prop

8fde1a1

feedback

53cdab2

fixes

c11af50

fix

d634c31

MuckT approved these changes Mar 20, 2024

View reviewed changes

satish-ravi reviewed Mar 20, 2024

View reviewed changes

finnian0826 and others added 6 commits March 20, 2024 15:35

feedback

02af7ab

cleanup

9a88aed

clean up

35d1f96

Merge branch 'main' into finnian0826/settings-help-invite

3a7b957

test: check all setting screen options

e3ec36e

test: update snapshots

3018d6e

satish-ravi approved these changes Mar 20, 2024

View reviewed changes

add cleanup todo

6a8eaef

finnian0826 enabled auto-merge March 20, 2024 22:03

satish-ravi and others added 3 commits March 20, 2024 15:42

improve test coverage

66e8cf1

test(backup-quiz): cancel flow and quiz completion

52daa41

test: remove test stub mistakenly added

8e63bfc

finnian0826 added this pull request to the merge queue Mar 20, 2024

Merged via the queue into main with commit dde91eb Mar 21, 2024
16 checks passed

finnian0826 deleted the finnian0826/settings-help-invite branch March 21, 2024 00:14

satish-ravi mentioned this pull request Apr 25, 2024

refactor(backup): replace settings screen param with boolean #5336

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(profile): register new screens for settings, help, invite #5121

fix(profile): register new screens for settings, help, invite #5121

finnian0826 commented Mar 18, 2024 •

edited

Loading

MuckT Mar 18, 2024 •

edited

Loading

codecov bot commented Mar 18, 2024 •

edited

Loading

emerge-tools bot commented Mar 18, 2024 •

edited

Loading

satish-ravi Mar 18, 2024

finnian0826 Mar 19, 2024

satish-ravi Mar 19, 2024

satish-ravi Mar 18, 2024

satish-ravi Mar 18, 2024

satish-ravi Mar 19, 2024

finnian0826 Mar 19, 2024

satish-ravi Mar 20, 2024

satish-ravi Mar 20, 2024

MuckT left a comment

satish-ravi Mar 20, 2024

satish-ravi Mar 20, 2024

satish-ravi Mar 20, 2024

satish-ravi Mar 20, 2024

satish-ravi Mar 20, 2024

fix(profile): register new screens for settings, help, invite #5121

fix(profile): register new screens for settings, help, invite #5121

Conversation

finnian0826 commented Mar 18, 2024 • edited Loading

Description

Test plan

Related issues

Backwards compatibility

Network scalability

MuckT Mar 18, 2024 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Mar 18, 2024 • edited Loading

Codecov Report

emerge-tools bot commented Mar 18, 2024 • edited Loading

1 build increased size

Celo (test) 1.80.0 (145)org.celo.mobile.test

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MuckT left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

finnian0826 commented Mar 18, 2024 •

edited

Loading

MuckT Mar 18, 2024 •

edited

Loading

codecov bot commented Mar 18, 2024 •

edited

Loading

emerge-tools bot commented Mar 18, 2024 •

edited

Loading

Celo (test) 1.80.0 (145)
org.celo.mobile.test