-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Migrate NVPs to their new keys #37556
Changes from 7 commits
50308c7
9ce6a3c
55f816d
4c70029
0b4e8db
b17b23c
e094189
39d33de
3053b96
6746721
a2ada45
f0c5910
fdadc74
1290c36
54c7a4c
be58c4f
a5dcb89
82fdff0
ef87783
440da06
9962ab1
df55f3c
7b08292
df49722
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,64 @@ | ||
import after from 'lodash/after'; | ||
import Onyx from 'react-native-onyx'; | ||
import ONYXKEYS from '@src/ONYXKEYS'; | ||
|
||
const migrations = { | ||
This comment was marked as resolved.
Sorry, something went wrong. |
||
// eslint-disable-next-line @typescript-eslint/naming-convention | ||
nvp_lastPaymentMethod: ONYXKEYS.NVP_LAST_PAYMENT_METHOD, | ||
isFirstTimeNewExpensifyUser: ONYXKEYS.NVP_IS_FIRST_TIME_NEW_EXPENSIFY_USER, | ||
preferredLocale: ONYXKEYS.NVP_PREFERRED_LOCALE, | ||
preferredEmojiSkinTone: ONYXKEYS.PREFERRED_EMOJI_SKIN_TONE, | ||
frequentlyUsedEmojis: ONYXKEYS.FREQUENTLY_USED_EMOJIS, | ||
// eslint-disable-next-line @typescript-eslint/naming-convention | ||
private_blockedFromConcierge: ONYXKEYS.NVP_BLOCKED_FROM_CONCIERGE, | ||
// eslint-disable-next-line @typescript-eslint/naming-convention | ||
private_pushNotificationID: ONYXKEYS.NVP_PRIVATE_PUSH_NOTIFICATION_ID, | ||
tryFocusMode: ONYXKEYS.NVP_TRY_FOCUS_MODE, | ||
introSelected: ONYXKEYS.NVP_INTRO_SELECTED, | ||
hasDismissedIdlePanel: ONYXKEYS.NVP_HAS_DISMISSED_IDLE_PANEL, | ||
}; | ||
|
||
// This migration changes the keys of all the NVP related keys so that they are standardized | ||
export default function () { | ||
return new Promise<void>((resolve) => { | ||
// It's 1 more because activePolicyID is not in the migrations object above as it is nested inside an object | ||
const resolveWhenDone = after(Object.entries(migrations).length + 1, () => resolve()); | ||
|
||
for (const [oldKey, newKey] of Object.entries(migrations)) { | ||
const connectionID = Onyx.connect({ | ||
// @ts-expect-error oldKey is a variable | ||
key: oldKey, | ||
callback: (value) => { | ||
Onyx.disconnect(connectionID); | ||
if (value !== null) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like this approach more because if I do early return I will need to call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Disregard, changing this given I need to pass resolveWhenDone to the promise too |
||
// @ts-expect-error These keys are variables, so we can't check the type | ||
Onyx.multiSet({ | ||
[newKey]: value, | ||
[oldKey]: null, | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I just noticed this in while writing the message above, fixing it. |
||
} | ||
resolveWhenDone(); | ||
}, | ||
}); | ||
} | ||
const connectionID = Onyx.connect({ | ||
key: ONYXKEYS.ACCOUNT, | ||
callback: (value) => { | ||
Onyx.disconnect(connectionID); | ||
// @ts-expect-error we are removing this property, so it is not in the type anymore | ||
if (value?.activePolicyID) { | ||
// @ts-expect-error we are removing this property, so it is not in the type anymore | ||
const activePolicyID = value.activePolicyID; | ||
const newValue = {...value}; | ||
// @ts-expect-error we are removing this property, so it is not in the type anymore | ||
delete newValue.activePolicyID; | ||
Onyx.multiSet({ | ||
[ONYXKEYS.NVP_ACTIVE_POLICY_ID]: activePolicyID, | ||
[ONYXKEYS.ACCOUNT]: newValue, | ||
}); | ||
} | ||
resolveWhenDone(); | ||
}, | ||
}); | ||
}); | ||
} |
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -22,7 +22,7 @@ | |||
import CONST from '@src/CONST'; | ||||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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]} |
||||
|
||||
type NewChatPageWithOnyxProps = { | ||||
/** All reports shared with the user */ | ||||
|
@@ -286,9 +286,8 @@ | |||
NewChatPage.displayName = 'NewChatPage'; | ||||
|
||||
export default withOnyx<NewChatPageProps, NewChatPageWithOnyxProps>({ | ||||
dismissedReferralBanners: { | ||||
Check failure on line 289 in src/pages/NewChatPage.tsx GitHub Actions / typecheck
|
||||
key: ONYXKEYS.ACCOUNT, | ||||
selector: (data) => data?.dismissedReferralBanners ?? {}, | ||||
key: ONYXKEYS.NVP_DISMISSED_REFERRAL_BANNERS, | ||||
}, | ||||
reports: { | ||||
key: ONYXKEYS.COLLECTION.REPORT, | ||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
import type CONST from '@src/CONST'; | ||
|
||
type DismissedReferralBanners = { | ||
[CONST.REFERRAL_PROGRAM.CONTENT_TYPES.MONEY_REQUEST]?: boolean; | ||
[CONST.REFERRAL_PROGRAM.CONTENT_TYPES.START_CHAT]?: boolean; | ||
[CONST.REFERRAL_PROGRAM.CONTENT_TYPES.SEND_MONEY]?: boolean; | ||
[CONST.REFERRAL_PROGRAM.CONTENT_TYPES.REFER_FRIEND]?: boolean; | ||
[CONST.REFERRAL_PROGRAM.CONTENT_TYPES.SHARE_CODE]?: boolean; | ||
}; | ||
|
||
export default DismissedReferralBanners; | ||
Comment on lines
+1
to
+11
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you update the description to also include the background for these changes for this dismissedReferralBanner nvp? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't follow, what background? 😕 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The explanation for why the changes to this nvp, it sees like it was refactored more than others in this PR |
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.
It would really be great to group all these together under an
NVP
key like:Or if not that, at least put all the NVPs next to each other in this file.
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.
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.