-
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 19 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 |
---|---|---|
|
@@ -16,9 +16,6 @@ const ONYXKEYS = { | |
/** Holds the reportID for the report between the user and their account manager */ | ||
ACCOUNT_MANAGER_REPORT_ID: 'accountManagerReportID', | ||
|
||
/** Boolean flag only true when first set */ | ||
NVP_IS_FIRST_TIME_NEW_EXPENSIFY_USER: 'isFirstTimeNewExpensifyUser', | ||
|
||
/** Holds an array of client IDs which is used for multi-tabs on web in order to know | ||
* which tab is the leader, and which ones are the followers */ | ||
ACTIVE_CLIENTS: 'activeClients', | ||
|
@@ -106,27 +103,52 @@ const ONYXKEYS = { | |
STASHED_SESSION: 'stashedSession', | ||
BETAS: 'betas', | ||
|
||
/** NVP keys | ||
/** NVP keys */ | ||
|
||
/** Boolean flag only true when first set */ | ||
NVP_IS_FIRST_TIME_NEW_EXPENSIFY_USER: 'nvp_isFirstTimeNewExpensifyUser', | ||
|
||
/** Contains the user preference for the LHN priority mode */ | ||
NVP_PRIORITY_MODE: 'nvp_priorityMode', | ||
|
||
/** 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would really be great to group all these together under an
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 commentThe 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. |
||
|
||
/** A unique identifier that each user has that's used to send notifications */ | ||
NVP_PRIVATE_PUSH_NOTIFICATION_ID: 'private_pushNotificationID', | ||
NVP_PRIVATE_PUSH_NOTIFICATION_ID: 'nvp_private_pushNotificationID', | ||
|
||
/** The NVP with the last payment method used per policy */ | ||
NVP_LAST_PAYMENT_METHOD: 'nvp_lastPaymentMethod', | ||
NVP_LAST_PAYMENT_METHOD: 'nvp_private_lastPaymentMethod', | ||
|
||
/** This NVP holds to most recent waypoints that a person has used when creating a distance request */ | ||
NVP_RECENT_WAYPOINTS: 'expensify_recentWaypoints', | ||
|
||
/** This NVP will be `true` if the user has ever dismissed the engagement modal on either OldDot or NewDot. If it becomes true it should stay true forever. */ | ||
NVP_HAS_DISMISSED_IDLE_PANEL: 'hasDismissedIdlePanel', | ||
NVP_HAS_DISMISSED_IDLE_PANEL: 'nvp_hasDismissedIdlePanel', | ||
|
||
/** This NVP contains the choice that the user made on the engagement modal */ | ||
NVP_INTRO_SELECTED: 'introSelected', | ||
NVP_INTRO_SELECTED: 'nvp_introSelected', | ||
|
||
/** This NVP contains the active policyID */ | ||
NVP_ACTIVE_POLICY_ID: 'nvp_expensify_activePolicyID', | ||
|
||
/** This NVP contains the referral banners the user dismissed */ | ||
NVP_DISMISSED_REFERRAL_BANNERS: 'nvp_dismissedReferralBanners', | ||
|
||
/** Indicates which locale should be used */ | ||
NVP_PREFERRED_LOCALE: 'nvp_preferredLocale', | ||
|
||
/** Whether the user has tried focus mode yet */ | ||
NVP_TRY_FOCUS_MODE: 'nvp_tryFocusMode', | ||
|
||
/** Whether the user has been shown the hold educational interstitial yet */ | ||
NVP_HOLD_USE_EXPLAINED: 'holdUseExplained', | ||
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. should this one be 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. No, I did not migrate this one.... I can't find this neither in web-e nor auth, I think this might be a broken feature? I see App sets it and reads it _in onyx), but I don't see anything setting it or loading it in the servers. 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. Can we delete it? 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 am not sure honestly, this seems broken and kind of outside of the scope of this PR to fix it or remove it./ |
||
|
||
/** Store preferred skintone for emoji */ | ||
PREFERRED_EMOJI_SKIN_TONE: 'nvp_expensify_preferredEmojiSkinTone', | ||
|
||
/** Store frequently used emojis for this user */ | ||
FREQUENTLY_USED_EMOJIS: 'nvp_expensify_frequentlyUsedEmojis', | ||
|
||
/** The NVP with the last distance rate used per policy */ | ||
NVP_LAST_SELECTED_DISTANCE_RATES: 'lastSelectedDistanceRates', | ||
|
@@ -150,9 +172,6 @@ const ONYXKEYS = { | |
ONFIDO_TOKEN: 'onfidoToken', | ||
ONFIDO_APPLICANT_ID: 'onfidoApplicantID', | ||
|
||
/** Indicates which locale should be used */ | ||
NVP_PREFERRED_LOCALE: 'preferredLocale', | ||
|
||
/** User's Expensify Wallet */ | ||
USER_WALLET: 'userWallet', | ||
|
||
|
@@ -174,12 +193,6 @@ const ONYXKEYS = { | |
/** The user's cash card and imported cards (including the Expensify Card) */ | ||
CARD_LIST: 'cardList', | ||
|
||
/** Whether the user has tried focus mode yet */ | ||
NVP_TRY_FOCUS_MODE: 'tryFocusMode', | ||
|
||
/** Whether the user has been shown the hold educational interstitial yet */ | ||
NVP_HOLD_USE_EXPLAINED: 'holdUseExplained', | ||
|
||
/** Boolean flag used to display the focus mode notification */ | ||
FOCUS_MODE_NOTIFICATION: 'focusModeNotification', | ||
|
||
|
@@ -192,12 +205,6 @@ const ONYXKEYS = { | |
/** Stores information about the active reimbursement account being set up */ | ||
REIMBURSEMENT_ACCOUNT: 'reimbursementAccount', | ||
|
||
/** Store preferred skintone for emoji */ | ||
PREFERRED_EMOJI_SKIN_TONE: 'preferredEmojiSkinTone', | ||
|
||
/** Store frequently used emojis for this user */ | ||
FREQUENTLY_USED_EMOJIS: 'frequentlyUsedEmojis', | ||
|
||
/** Stores Workspace ID that will be tied to reimbursement account during setup */ | ||
REIMBURSEMENT_ACCOUNT_WORKSPACE_ID: 'reimbursementAccountWorkspaceID', | ||
|
||
|
@@ -291,7 +298,8 @@ const ONYXKEYS = { | |
POLICY_CATEGORIES: 'policyCategories_', | ||
POLICY_RECENTLY_USED_CATEGORIES: 'policyRecentlyUsedCategories_', | ||
POLICY_TAGS: 'policyTags_', | ||
POLICY_RECENTLY_USED_TAGS: 'policyRecentlyUsedTags_', | ||
POLICY_RECENTLY_USED_TAGS: 'nvp_policyRecentlyUsedTags_', | ||
OLD_POLICY_RECENTLY_USED_TAGS: 'policyRecentlyUsedTags_', | ||
POLICY_REPORT_FIELDS: 'policyReportFields_', | ||
WORKSPACE_INVITE_MEMBERS_DRAFT: 'workspaceInviteMembersDraft_', | ||
WORKSPACE_INVITE_MESSAGE_DRAFT: 'workspaceInviteMessageDraft_', | ||
|
@@ -494,6 +502,7 @@ type OnyxCollectionValuesMapping = { | |
[ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS]: OnyxTypes.TransactionViolations; | ||
[ONYXKEYS.COLLECTION.SPLIT_TRANSACTION_DRAFT]: OnyxTypes.Transaction; | ||
[ONYXKEYS.COLLECTION.POLICY_RECENTLY_USED_TAGS]: OnyxTypes.RecentlyUsedTags; | ||
[ONYXKEYS.COLLECTION.OLD_POLICY_RECENTLY_USED_TAGS]: OnyxTypes.RecentlyUsedTags; | ||
[ONYXKEYS.COLLECTION.SELECTED_TAB]: string; | ||
[ONYXKEYS.COLLECTION.PRIVATE_NOTES_DRAFT]: string; | ||
[ONYXKEYS.COLLECTION.NEXT_STEP]: OnyxTypes.ReportNextStep; | ||
|
@@ -590,6 +599,8 @@ type OnyxValuesMapping = { | |
[ONYXKEYS.LOGS]: Record<number, OnyxTypes.Log>; | ||
[ONYXKEYS.SHOULD_STORE_LOGS]: boolean; | ||
[ONYXKEYS.CACHED_PDF_PATHS]: Record<string, string>; | ||
[ONYXKEYS.NVP_ACTIVE_POLICY_ID]: string; | ||
[ONYXKEYS.NVP_DISMISSED_REFERRAL_BANNERS]: OnyxTypes.DismissedReferralBanners; | ||
}; | ||
|
||
type OnyxValues = OnyxValuesMapping & OnyxCollectionValuesMapping & OnyxFormValuesMapping & OnyxFormDraftValuesMapping; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,86 @@ | ||
import after from 'lodash/after'; | ||
import Onyx from 'react-native-onyx'; | ||
import ONYXKEYS from '@src/ONYXKEYS'; | ||
|
||
// These are the oldKeyName: newKeyName of the NVPs we can migrate without any processing | ||
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) => { | ||
// Resolve the migration when all the keys have been migrated. The number of keys is the size of the `migrations` object in addition to the ACCOUNT and OLD_POLICY_RECENTLY_USED_TAGS keys (which is why there is a +2). | ||
const resolveWhenDone = after(Object.entries(migrations).length + 2, () => 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) { | ||
resolveWhenDone(); | ||
return; | ||
} | ||
// @ts-expect-error These keys are variables, so we can't check the type | ||
Onyx.multiSet({ | ||
[newKey]: value, | ||
[oldKey]: null, | ||
}).then(resolveWhenDone); | ||
}, | ||
}); | ||
} | ||
const connectionIDAccount = Onyx.connect({ | ||
key: ONYXKEYS.ACCOUNT, | ||
callback: (value) => { | ||
Onyx.disconnect(connectionIDAccount); | ||
// @ts-expect-error we are removing this property, so it is not in the type anymore | ||
if (!value?.activePolicyID) { | ||
resolveWhenDone(); | ||
return; | ||
} | ||
// @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, | ||
}).then(resolveWhenDone); | ||
}, | ||
}); | ||
const connectionIDRecentlyUsedTags = Onyx.connect({ | ||
key: ONYXKEYS.COLLECTION.OLD_POLICY_RECENTLY_USED_TAGS, | ||
waitForCollectionCallback: true, | ||
callback: (value) => { | ||
Onyx.disconnect(connectionIDRecentlyUsedTags); | ||
if (!value) { | ||
This comment was marked as resolved.
Sorry, something went wrong. |
||
resolveWhenDone(); | ||
return; | ||
} | ||
const newValue = {}; | ||
for (const key of Object.keys(value)) { | ||
// @ts-expect-error We have no fixed types here | ||
newValue[`nvp_${key}`] = value[key]; | ||
// @ts-expect-error We have no fixed types here | ||
newValue[key] = null; | ||
} | ||
Onyx.multiSet(newValue).then(resolveWhenDone); | ||
}, | ||
}); | ||
}); | ||
} |
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.
Looks like this is causing regression. Engagement modal doesn't show after signup
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.
Screen.Recording.2024-03-14.at.10.13.27.pm.mov
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.
Great catch! I was missing renaming this constant in the backend. Sending a PR now but it needs to go out to prod before you can re-check.
Was this the only bug you found? Can you check the rest of things to see if you find anything else broken please?
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.
No issues found on any other nvp keys. But still going to retest
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.
Wait a second, going to push a change...
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.
Done