Add Onyx key to allow remotely resetting client data #38196
Conversation
melvin-bot
bot
requested review from
alitoshmatov
and removed request for
a team
|
@cubuspl42 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] |
|
Hey @cubuspl42! For the testing here you can use the JS console to manually set the key in Onyx. I think it should have the same effect as triggering it from the local dev. But let me know if it's unclear or creates any issues. Thanks so much for your help 🙇 |
src/ONYXKEYS.ts
Outdated
| @@ -291,6 +291,9 @@ const ONYXKEYS = { | |||
| /** Indicates whether an forced upgrade is required */ | |||
| UPDATE_REQUIRED: 'updateRequired', | |||
|
|
|||
| /** Indicates whether an forced reset is required a.k.a. clear Oynx data without signing the user out */ | |||
There was a problem hiding this comment.
Do you think it would be a good idea to include the intention for this feature in the comment? If it's meant to be debug-only, maybe we could highlight it
There was a problem hiding this comment.
Given the confusion experienced here I think it's a great idea to update this and explain the intention!
It's a server side way to force clients to reload in case some backend code leaves the app in a broken state.
There was a problem hiding this comment.
Please let me know if the change looks good to you 🙇
There was a problem hiding this comment.
Looks much better! The comment doesn't mention that it's a server-to-client pipe, but maybe that's obvious.
|
So I understand that I'm meant to ignore the "Tests" section, as it's internal, and just use the global |
|
@cubuspl42 That's correct. Please use the global Onyx JS object to test that the "reload" behavior works ok. I am not sure if it will be easy to do on native (might have to use Flipper and/or mWeb local debugger). But let me know if you run into any issues and we can have the next reviewer test with the local code provided that will work with the dev environment. |
|
Would you merge |
Reviewer Checklist
Screenshots/VideosAndroid: NativeaddResetRequiredKey-android-compressed.mp4Android: mWeb ChromeaddResetRequiredKey-android-web-compressed.mp4iOS: NativeaddResetRequiredKey-ios-compressed.mp4iOS: mWeb SafariaddResetRequiredKey-ios-web-compressed.mp4MacOS: Chrome / SafariaddResetRequiredKey-web-converted.mp4MacOS: DesktopaddResetRequiredKey-desktop-converted.mp4 |
There was a problem hiding this comment.
On Native, I used this snippet to test the change:
// eslint-disable-next-line rulesdir/prefer-actions-set-data
Onyx.set(ONYXKEYS.RESET_REQUIRED, true).then((result) => {
// eslint-disable-next-line no-console
console.log("Reset successful");
}).catch((error) => {
console.error("Reset failed", error);
});
|
@srikarparsi 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] |
|
🎯 @cubuspl42, thanks for reviewing and testing this PR! 🎉 An E/App issue has been created to issue payment here: #40170. |
|
✋ 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/srikarparsi in version: 1.4.63-0 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 1.4.63-21 🚀
|
| @@ -77,6 +78,38 @@ Onyx.connect({ | |||
| }, | |||
| }); | |||
|
|
|||
| const KEYS_TO_PRESERVE: OnyxKey[] = [ | |||
| ONYXKEYS.ACCOUNT, | |||
There was a problem hiding this comment.
@marcaaron Was there any specific reason not to include ONYXKEYS.USER here? We're considering adding it in #49087, but wanted to check first if there are any good arguments against doing so.
There was a problem hiding this comment.
Can't think of a reason! Thanks for asking! The array of keys also predates this PR so we could also ask the author of the original PR that introduced this change.
Details
Gives a way to reset a client via an Onyx key flag
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/359211
Tests
../script/psysh.shOnyx::pushUpdatesToClients([257], ['onyxMethod' => 'set', 'key' => 'resetRequired', 'value' => true]);Offline tests
❌
QA Steps
Internal. See linked Web-Expensify PR
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))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
Full disclosure there is some kind of bug on main preventing me from testing in mobile sized views.
Android: mWeb Chrome
Full disclosure there is some kind of bug on main preventing me from testing in mobile sized views.
iOS: Native
Full disclosure there is some kind of bug on main preventing me from testing in mobile sized views.
iOS: mWeb Safari
Full disclosure there is some kind of bug on main preventing me from testing in mobile sized views.
MacOS: Chrome / Safari
2024-04-04_13-21-45.mp4
MacOS: Desktop