Migrate NVPs to their new keys#37556
Conversation
| import ONYXKEYS from '@src/ONYXKEYS'; | ||
| import type * as OnyxTypes from '@src/types/onyx'; | ||
| import type {DismissedReferralBanners} from '@src/types/onyx/Account'; | ||
| import type DismissedReferralBanners from '@src/types/onyx/DismissedReferralBanners'; |
There was a problem hiding this comment.
| import type DismissedReferralBanners from '@src/types/onyx/DismissedReferralBanners'; |
There was a problem hiding this comment.
Also, please apply these changes in this file:
- dismissedReferralBanners: DismissedReferralBanners;
+ dismissedReferralBanners: OnyxEntry<OnyxTypes.DismissedReferralBanners>;
- shouldShowReferralCTA={!dismissedReferralBanners[CONST.REFERRAL_PROGRAM.CONTENT_TYPES.START_CHAT]}
+ shouldShowReferralCTA={!dismissedReferralBanners?.[CONST.REFERRAL_PROGRAM.CONTENT_TYPES.START_CHAT]}|
This is on hold, but please review it |
|
|
||
| /** Contains the users's block expiration (if they have one) */ | ||
| NVP_BLOCKED_FROM_CONCIERGE: 'private_blockedFromConcierge', | ||
| NVP_BLOCKED_FROM_CONCIERGE: 'nvp_private_blockedFromConcierge', |
There was a problem hiding this comment.
It would really be great to group all these together under an NVP key like:
ONYXKEYS: {
NVP: {
BLOCKED_FROM_CONCIERGE: 'nvp_private_blockedFromConcierge',
},
};
Or if not that, at least put all the NVPs next to each other in this file.
There was a problem hiding this comment.
Moving them to be together mostly because I don't know how to make my IDE refactor this automatically and I don't want to have to update 420 files.
| import Onyx from 'react-native-onyx'; | ||
| import ONYXKEYS from '@src/ONYXKEYS'; | ||
|
|
||
| const migrations = { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| Onyx.multiSet({ | ||
| [newKey]: value, | ||
| [oldKey]: null, | ||
| }); |
There was a problem hiding this comment.
It's been a while since I've done the migrations, but do you know if it's a bad thing that this doesn't wait for the multiSet() promise to be resolved before calling resolveWhenDone()? I think it could technically mean that the migration finishes before the actual changes are committed to storage. If the app then tries to load in that state, it could cause issues (even fatal errors maybe).
There was a problem hiding this comment.
Yes, I just noticed this in while writing the message above, fixing it.
| key: oldKey, | ||
| callback: (value) => { | ||
| Onyx.disconnect(connectionID); | ||
| if (value !== null) { |
There was a problem hiding this comment.
NAB: I'm kind of surprised that there aren't lint warnings to switch these to early returns. I guess it's because there is no return statement, but I think it would be nice to follow the style of early returns so the code style is consistent.
There was a problem hiding this comment.
I like this approach more because if I do early return I will need to call resolveWhenDone twice
There was a problem hiding this comment.
Disregard, changing this given I need to pass resolveWhenDone to the promise too
| waitForCollectionCallback: true, | ||
| callback: (value) => { | ||
| Onyx.disconnect(connectionIDRecentlyUsedTags); | ||
| if (!value) { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Co-authored-by: Tim Golen <tgolen@gmail.com>
tgolen
left a comment
There was a problem hiding this comment.
Thanks for grouping the NVPs and switching to early returns!
grgia
left a comment
There was a problem hiding this comment.
code LGTM, my remaining comments are NAB
mkhutornyi
left a comment
There was a problem hiding this comment.
LGTM2! Just waiting for backend PR (fixing #37556 (comment)) to hit production
|
@hayata-suenaga 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] |
|
🎯 @mkhutornyi, thanks for reviewing and testing this PR! 🎉 An E/App issue has been created to issue payment here: #38470. |
|
@mkhutornyi the reviewer checklist failed... maybe it is outdated or something? |
|
It's success but don't merge yet. I still need to verify signup flow after backend deploy. |
|
seems like the PR has been already reviewed removing myself from the reviewer list 😄 |
|
@mkhutornyi can you retest now please? |
|
@marcochavezf 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] |
|
✋ 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/iwiznia in version: 1.4.56-0 🚀
|
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 1.4.56-8 🚀
|
Held on https://github.com/Expensify/Web-Expensify/pull/41082Held on https://github.com/Expensify/Web-Expensify/pull/41300Details
Fixed Issues
https://github.com/Expensify/Expensify/issues/372366
Tests
Offline tests
No
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.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 so 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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop