Use Onyx.clear to preserve state when logging out #13384

yuwenmemon · 2022-12-06T23:29:59Z

Details

Instead of setting default values after clearing Onyx, preserve the keys we want to keep using the new keysToPreserve param of Onyx.clear()

Fixed Issues

$ #13062

Tests/QA Steps/Offline Tests

Log into NewDot
Switch your language to spanish
Go offline (You can only do this on Web/Desktop as the "Force Offline" setting won't apply here)
Log out
Make sure that your offline status is preserved, and that the language of the login menu is Spanish and there's no flicker of English or the Log In button being enabled.

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

Kapture.2022-12-09.at.13.37.48.mp4

Mobile Web - Chrome

Kapture.2022-12-09.at.15.11.14.mp4

Mobile Web - Safari

Kapture.2022-12-09.at.15.00.18.mp4

Desktop

Kapture.2022-12-09.at.13.45.00.mp4

iOS

Kapture.2022-12-09.at.14.49.45.mp4

Android

Kapture.2022-12-09.at.15.08.36.mp4

…fline

yuwenmemon · 2022-12-09T22:57:26Z

Having trouble completing testing because of the error being reported here: https://expensify.slack.com/archives/C01GTK53T8Q/p1670528976295469

But looks like it's getting resolved soon so will be able to take this out of Draft soon enough!

…fline

melvin-bot · 2022-12-09T23:29:24Z

@parasharrajat @MariaHCD One of you needs to 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]

…s default param

neil-marcellini

Awesome, this works really well now. I do have a few comments but once those are resolved it will be good to go. @parasharrajat please go ahead and complete the checklist.

web.mov

neil-marcellini · 2022-12-12T17:51:14Z

src/libs/actions/SignInRedirect.js

-    const preferredLocale = currentPreferredLocale;
-    const isOffline = currentIsOffline;
-    const shouldForceOffline = currentShouldForceOffline;
+    const keysToPreserve = [];


NAB: Add a comment above keysToPreserve explaining why we preserve some keys and how that works.

tests/ui/UnreadIndicatorsTest.js

tests/unit/NetworkTest.js

yuwenmemon · 2022-12-13T17:37:43Z

Is there any workaround for this?

Hmmmm... I don't think so no 😞. Looks like it's being tracked here.

neil-marcellini

Looking good but there's a lint error right now. Since @parasharrajat can't test right now should I go ahead and test, or should we wait? I think we would still pay him as we usually do of course.

…fline

yuwenmemon · 2022-12-13T18:24:54Z

RE: the linter that change is being fixed here: https://expensify.slack.com/archives/C01GTK53T8Q/p1670954885116019

…fline

MariaHCD · 2022-12-14T07:43:59Z

@parasharrajat We can have your IP whitelisted once you fill out this form: https://expensify.slack.com/archives/C01GTK53T8Q/p1670957921425999

…fline

yuwenmemon · 2022-12-16T18:07:04Z

@neil-marcellini @MariaHCD bump! If @parasharrajat is still having issues or otherwise busy would one of you be able to fill out the reviewer checklist in their absence?

parasharrajat · 2022-12-16T18:09:52Z

I just got access so let me try to test it out.

MariaHCD · 2022-12-19T06:51:40Z

@parasharrajat Let us know if you're still having problems with testing and I can fill out the reviewer checklist for this.

parasharrajat · 2022-12-19T15:09:27Z

@MariaHCD Please do that. I am not sure what to do I am blocked because my IP changed and I am back to waiting.

MariaHCD · 2022-12-19T15:30:19Z

Thanks for letting us know, @parasharrajat! I'll fill out the checklist.

MariaHCD · 2022-12-19T15:30:35Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

Screen.Recording.2022-12-19.at.7.42.29.PM.mov

Mobile Web - Chrome

XRecorder_19122022_194641.mp4

Mobile Web - Safari

Simulator.Screen.Recording.-.iPhone.14.-.2022-12-19.at.20.06.19.mp4

Desktop

Screen.Recording.2022-12-19.at.7.47.54.PM.mov

iOS

Simulator.Screen.Recording.-.iPhone.14.-.2022-12-19.at.20.10.50.mp4

Android

Screen.Recording.2022-12-19.at.8.35.04.PM.mov

MariaHCD · 2022-12-19T16:39:38Z

We should be good to go here. Not sure if @parasharrajat should give this a final review? Otherwise, feel free to merged @neil-marcellini @yuwenmemon

OSBotify · 2022-12-19T18:03:50Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

github-actions · 2022-12-19T18:22:39Z

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries

Name	Duration
TTI	789.537 ms → 829.369 ms (+39.832 ms, +5.0%)
runJsBundle	179.500 ms → 199.750 ms (+20.250 ms, +11.3%)
regularAppStart	0.017 ms → 0.015 ms (-0.002 ms, -13.3%)
nativeLaunch	9.548 ms → 9.323 ms (-0.226 ms, -2.4%)

Show details

Name	Duration
TTI	Baseline Mean: 789.537 ms Stdev: 29.595 ms (3.7%) Runs: 725.2486200002022 749.4885769998655 752.7305250000209 765.0546519998461 765.4161939998157 768.2565589998849 768.6021929997951 769.4196219998412 770.0307160001248 770.4089879998937 770.4924329998903 772.2560399998911 777.6827790001407 777.9116170001216 779.52493100008 780.7843729997985 781.5327900000848 783.5569090000354 783.9457259997725 793.8752830000594 798.7051209998317 804.0088390000165 809.0751229999587 825.6629770002328 825.966810000129 828.8565190001391 829.396643999964 830.9126969999634 832.0594299999066 838.8705250001512 845.9080860000104 Current Mean: 829.369 ms Stdev: 30.464 ms (3.7%) Runs: 772.117928000167 780.058157000225 794.7579319998622 798.3549970001914 799.2986460002139 800.3755979998969 803.9883719999343 810.8911729999818 811.8349950001575 815.9466869998723 817.7808389998972 819.2992070000619 819.6227640002035 823.0331660001539 825.9805069998838 829.5269619999453 830.7574829999357 830.9847650001757 832.2393459999003 834.8672420000657 841.8585660001263 847.3268659999594 848.4993380000815 854.0720359999686 874.8631290001795 878.3395589999855 878.7718500001356 879.9685120000504 896.2863280000165
runJsBundle	Baseline Mean: 179.500 ms Stdev: 14.394 ms (8.0%) Runs: 149 157 157 157 162 167 167 169 170 174 177 177 178 178 178 178 179 183 186 188 189 191 192 192 192 193 200 201 202 202 Current Mean: 199.750 ms Stdev: 24.335 ms (12.2%) Runs: 160 160 172 173 174 175 178 183 183 184 184 186 190 190 192 198 198 199 200 202 204 208 214 214 219 221 225 227 234 241 249 255
regularAppStart	Baseline Mean: 0.017 ms Stdev: 0.002 ms (9.2%) Runs: 0.014648000244051218 0.01509599993005395 0.015217999927699566 0.015298999845981598 0.01554399961605668 0.015584000386297703 0.01570600038394332 0.015787999611347914 0.016112999990582466 0.016112999990582466 0.01615400006994605 0.016316000372171402 0.01643799990415573 0.016561000142246485 0.016642000060528517 0.01668200036510825 0.01672299997881055 0.01696799974888563 0.01733399974182248 0.01733400020748377 0.017455999739468098 0.0176189998164773 0.01766000036150217 0.017862999811768532 0.0179449999704957 0.0185139998793602 0.018636000342667103 0.019450000021606684 0.019694000016897917 0.020223000086843967 0.020304000005126 0.0204670000821352 Current Mean: 0.015 ms Stdev: 0.001 ms (5.9%) Runs: 0.013346000108867884 0.013671000022441149 0.013671999797224998 0.013875999953597784 0.014038000255823135 0.01407900033518672 0.01407900033518672 0.014119999948889017 0.014241999946534634 0.014403999783098698 0.014444999862462282 0.014525999780744314 0.014567000325769186 0.014648000244051218 0.014689000323414803 0.0147299999371171 0.014770000241696835 0.014851999934762716 0.015056000091135502 0.015136000234633684 0.015298999845981598 0.015300000086426735 0.015381000004708767 0.015422000084072351 0.015503000002354383 0.015786999836564064 0.0157880000770092 0.016601999755948782 0.016642000060528517 0.01680499967187643
nativeLaunch	Baseline Mean: 9.548 ms Stdev: 1.364 ms (14.3%) Runs: 8 8 8 8 8 8 8 9 9 9 9 9 9 9 9 9 9 9 10 10 10 10 10 10 11 11 11 11 11 12 14 Current Mean: 9.323 ms Stdev: 1.280 ms (13.7%) Runs: 7 8 8 8 8 8 8 8 8 8 9 9 9 9 9 9 9 10 10 10 10 10 10 10 10 10 10 11 12 12 12

OSBotify · 2022-12-20T06:46:37Z

🚀 Deployed to staging by @neil-marcellini in version: 1.2.42-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2022-12-21T22:57:29Z

🚀 Deployed to production by @yuwenmemon in version: 1.2.42-2 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

yuwenmemon added 3 commits December 6, 2022 14:40

Preserve offline state when signing out

51e15ea

Use keyStatesToPreserve instead

3f696ad

Merge branch 'main' of github.com:Expensify/App into yuwen-preserveOf…

bf1c4e9

…fline

yuwenmemon self-assigned this Dec 6, 2022

yuwenmemon mentioned this pull request Dec 6, 2022

Update Onyx.clear to accept passed in default values Expensify/react-native-onyx#213

Closed

yuwenmemon added 2 commits December 6, 2022 15:58

Semi colon

9fef987

Merge branch 'main' of github.com:Expensify/App into yuwen-preserveOf…

b707787

…fline

neil-marcellini self-requested a review December 7, 2022 18:09

Merge branch 'main' of github.com:Expensify/App into yuwen-preserveOf…

e3a5174

…fline

yuwenmemon mentioned this pull request Dec 8, 2022

Update Onyx.clear to accept passed in keys to preserve, and use multiSet to clear storage Expensify/react-native-onyx#214

Merged

yuwenmemon added 3 commits December 8, 2022 17:26

keyStatesToPreserve -> keysToPreserve

e968a16

Merge branch 'main' of github.com:Expensify/App into yuwen-preserveOf…

6ed7781

…fline

Also preserve active clients

748c7a7

yuwenmemon changed the title ~~[HOLD ONYX#213] Use Onyx.clear to preserve state when logging out~~ [HOLD ONYX#214] Use Onyx.clear to preserve state when logging out Dec 9, 2022

Bump Onyx version

f405f1f

Merge branch 'main' of github.com:Expensify/App into yuwen-preserveOf…

c372860

…fline

yuwenmemon marked this pull request as ready for review December 9, 2022 23:28

yuwenmemon requested a review from a team as a code owner December 9, 2022 23:28

yuwenmemon changed the title ~~[HOLD ONYX#214] Use Onyx.clear to preserve state when logging out~~ Use Onyx.clear to preserve state when logging out Dec 9, 2022

melvin-bot bot requested review from MariaHCD and parasharrajat and removed request for a team December 9, 2022 23:29

Make sure Network test uses Onyx call that would set keysToPreserve a…

ab14ca4

…s default param

yuwenmemon force-pushed the yuwen-preserveOffline branch from 480d199 to ab14ca4 Compare December 9, 2022 23:49

MariaHCD previously approved these changes Dec 12, 2022

View reviewed changes

neil-marcellini reviewed Dec 12, 2022

View reviewed changes

Add comment

3a49c3f

yuwenmemon dismissed MariaHCD’s stale review via 3a49c3f December 12, 2022 19:37

yuwenmemon dismissed neil-marcellini’s stale review via 390ea5f December 13, 2022 17:35

Remove unneeded ONYX connections

7d0a7e7

neil-marcellini reviewed Dec 13, 2022

View reviewed changes

Merge branch 'main' of github.com:Expensify/App into yuwen-preserveOf…

6401f5e

…fline

Merge branch 'main' of github.com:Expensify/App into yuwen-preserveOf…

9e43b33

…fline

neil-marcellini previously approved these changes Dec 14, 2022

View reviewed changes

yuwenmemon added 2 commits December 14, 2022 12:57

Merge branch 'main' of github.com:Expensify/App into yuwen-preserveOf…

fbbb00d

…fline

Upgrade to onyx version 1.0.31

6227a80

yuwenmemon dismissed neil-marcellini’s stale review via 6227a80 December 14, 2022 20:58

neil-marcellini approved these changes Dec 15, 2022

View reviewed changes

MariaHCD approved these changes Dec 19, 2022

View reviewed changes

neil-marcellini merged commit 7128d27 into main Dec 19, 2022

neil-marcellini deleted the yuwen-preserveOffline branch December 19, 2022 17:58

OSBotify mentioned this pull request Dec 20, 2022

Deploy Checklist: New Expensify 2022-12-20 #13723

Closed

34 tasks

yuwenmemon mentioned this pull request Dec 21, 2022

Revert "Use Onyx.clear to preserve state when logging out" #13758

Merged

51 tasks

hungvu193 mentioned this pull request Feb 24, 2023

[HOLD #34289] [$2000] The app crashes when the user is logged into multiple tabs and logs out of one of the tabs #15321

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Onyx.clear to preserve state when logging out #13384

Use Onyx.clear to preserve state when logging out #13384

yuwenmemon commented Dec 6, 2022 •

edited

yuwenmemon commented Dec 9, 2022

melvin-bot bot commented Dec 9, 2022

neil-marcellini left a comment

neil-marcellini Dec 12, 2022

yuwenmemon commented Dec 13, 2022 •

edited

neil-marcellini left a comment

yuwenmemon commented Dec 13, 2022

MariaHCD commented Dec 14, 2022

yuwenmemon commented Dec 16, 2022

parasharrajat commented Dec 16, 2022

MariaHCD commented Dec 19, 2022

parasharrajat commented Dec 19, 2022 •

edited

MariaHCD commented Dec 19, 2022

MariaHCD commented Dec 19, 2022 •

edited

MariaHCD commented Dec 19, 2022

OSBotify commented Dec 19, 2022

github-actions bot commented Dec 19, 2022

OSBotify commented Dec 20, 2022

OSBotify commented Dec 21, 2022

Use Onyx.clear to preserve state when logging out #13384

Use Onyx.clear to preserve state when logging out #13384

Conversation

yuwenmemon commented Dec 6, 2022 • edited

Details

Fixed Issues

Tests/QA Steps/Offline Tests

PR Author Checklist

Screenshots/Videos

yuwenmemon commented Dec 9, 2022

melvin-bot bot commented Dec 9, 2022

neil-marcellini left a comment

Choose a reason for hiding this comment

neil-marcellini Dec 12, 2022

Choose a reason for hiding this comment

yuwenmemon commented Dec 13, 2022 • edited

neil-marcellini left a comment

Choose a reason for hiding this comment

yuwenmemon commented Dec 13, 2022

MariaHCD commented Dec 14, 2022

yuwenmemon commented Dec 16, 2022

parasharrajat commented Dec 16, 2022

MariaHCD commented Dec 19, 2022

parasharrajat commented Dec 19, 2022 • edited

MariaHCD commented Dec 19, 2022

MariaHCD commented Dec 19, 2022 • edited

Reviewer Checklist

Screenshots/Videos

MariaHCD commented Dec 19, 2022

OSBotify commented Dec 19, 2022

github-actions bot commented Dec 19, 2022

Performance Comparison Report 📊

Significant Changes To Duration

Meaningless Changes To Duration

OSBotify commented Dec 20, 2022

OSBotify commented Dec 21, 2022

yuwenmemon commented Dec 6, 2022 •

edited

yuwenmemon commented Dec 13, 2022 •

edited

parasharrajat commented Dec 19, 2022 •

edited

MariaHCD commented Dec 19, 2022 •

edited