Stop using MY_PERSONAL_DETAILS and only use PERSONAL_DETAILS in Onyx #9560

puneetlath · 2022-06-24T15:29:49Z

Details

Today, we have two different Onyx keys that store the same information. MY_PERSONAL_DETAILS which stores only the personal details of the logged in user. And PERSONAL_DETAILS which stores the personal details of the logged in user and other users they interact with in the app.

This PR makes it so that we are no longer storing duplicate data by getting rid of the MY_PERSONAL_DETAILS key. Any component that needs the personal details of the current user instead gets the information from the PERSONAL_DETAILS key. In addition, I've added {isCurrentUser: true} to the personal details object for the current logged in user in order to make it easy to find/reference.

Fixed Issues

https://github.com/Expensify/Expensify/issues/215037

Tests

Clear existing storage and sign in. Make sure that no MY_PERSONAL_DETAILS key exists.
Verify that the PERSONAL_DETAILS key in Onyx has details for the logged in user
Verify that the details for the current user has {isCurrentUser: true} as part of the object.
Verify that the details for the current user has timezone as part of the object and that it updates successfully in Onyx when changed in personal settings
Verify that the details for the current user has localCurrency as part of the object.
Request money from a group. Make sure everyone's personal details show up as normal without error.
Send money to a group. Make sure everyone's personal details show up as normal without error.
Click into a chat. Make sure everyone's personal details show up as normal without error. (i.e. in the chat header and when clicking on their name/pic)
Request a call from Concierge. Make sure your personal details show up as normal without error.
Send a message. Make sure your personal details show up as normal without error as part of the message.
Look at the LHN. Make sure everyone's personal details show up as normal without error.
Go to your profile settings. Make sure your personal details show up as normal without error. Make sure you're able to change them without issue.

Verify that no errors appear in the JS console

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Request money from a group. Make sure everyone's personal details show up as normal without error.
Send money to a group. Make sure everyone's personal details show up as normal without error.
Click into a chat. Make sure everyone's personal details show up as normal without error. (i.e. in the chat header and when clicking on their name/pic)
Request a call from Concierge. Make sure your personal details show up as normal without error.
Send a message. Make sure your personal details show up as normal without error as part of the message.
Look at the LHN. Make sure everyone's personal details show up as normal without error.
Go to your profile settings. Make sure your personal details show up as normal without error. Make sure you're able to change them without issue.

Verify that no errors appear in the JS console

Screenshots

Web

Mobile Web

Desktop

iOS

Android

…L_DETAILS

… user

…_PERSONAL_DETAILS

…SONAL_DETAILS

…DETAILS

…ONAL_DETAILS

…ETAILS

…Y_PERSONAL_DETAILS

…AL_DETAILS

…L_DETAILS

…_PERSONAL_DETAILS

…SONAL_DETAILS

…lection.js

marcaaron

Happy we are getting rid of this 👍 🙇‍♂️

Had a few initial comments not a full review.

src/components/IOUConfirmationList.js

Beamanator

Another interesting thing I've noticed: A few components directly subscribe to ONYXKEYS.PERSONAL_DETAILS via withOnyx, but some use withPersonalDetails.

Do you think we could standardize this as well?

Beamanator · 2022-07-05T11:32:36Z

src/components/IOUConfirmationList.js

 import ROUTES from '../ROUTES';
 import networkPropTypes from './networkPropTypes';
 import {withNetwork} from './OnyxProvider';
+import personalDetailsPropType from '../pages/personalDetailsPropType';


NAB - should be plural since there's more than one prop type in this file - though this isn't your fault since you didn't create the original file :D

Suggested change

import personalDetailsPropType from '../pages/personalDetailsPropType';

import personalDetailsPropTypes from '../pages/personalDetailsPropTypes';

Good call. Updating.

Beamanator · 2022-07-05T11:37:25Z

src/pages/RequestCallPage.js


-    /** The personal details of the person who is logged in */
-    myPersonalDetails: personalDetailsPropType.isRequired,
+    /** Personal details of all the users */


In AdditionalDetailsStep.js you have this at the end of the same comment: , including current user

Probably should be consistent - I think it makes sense to keep , including current user in these comments

puneetlath · 2022-07-15T23:45:16Z

Ok, everything is updated! To recap:

got rid of MY_PERSONAL_DETAILS from Onyx
created a withCurrentUserPersonalDetails HOC which passes one currentUserPersonalDetails prop
any component that needs the current user's personal details is wrapped with this HOC

Please give it another review 🙏🏾

marcaaron

LGTM 👍

marcaaron · 2022-07-16T00:11:03Z

src/libs/Navigation/AppNavigator/AuthScreens.js

+            return;
+        }
+
+        const timezone = lodashGet(val, currentUserEmail, 'timezone', {});


NAB, it seems like there is a very remote chance that currentUserEmail won't have a value at this point. It depends entirely on the session.email data being set before the personalDetails and ~~both would be set when the user logs in~~ actually not 100% sure when they would be set.

The subscriptions here feel out of place though and should be probably be moved into PersonalDetails (but not necessary to do that now).

marcaaron · 2022-07-16T00:30:16Z

Gonna merge this so we can unblock some other open PRs. Tested and code is 👍

OSBotify · 2022-07-16T00:35:29Z

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

Beamanator · 2022-07-21T10:38:37Z

src/libs/Navigation/AppNavigator/AuthScreens.js

+            return;
+        }
+
+        const timezone = lodashGet(val, currentUserEmail, 'timezone', {});


Looks like this needed to be lodashGet(val, [currentUserEmail, 'timezone'], {});

Beamanator · 2022-07-21T10:41:10Z

src/libs/DateUtils.js

+    key: ONYXKEYS.PERSONAL_DETAILS,
    callback: (val) => {
-        timezone = lodashGet(val, 'timezone', CONST.DEFAULT_TIME_ZONE);
+        timezone = lodashGet(val, currentUserEmail, 'timezone', CONST.DEFAULT_TIME_ZONE);


Same with this one :)

Beamanator · 2022-07-21T10:41:53Z

src/pages/settings/Profile/ProfilePage.js

+            selectedTimezone: lodashGet(this.props.currentUserPersonalDetails.timezone, 'selected', CONST.DEFAULT_TIME_ZONE.selected),
+            isAutomaticTimezone: lodashGet(this.props.currentUserPersonalDetails.timezone, 'automatic', CONST.DEFAULT_TIME_ZONE.automatic),


Here maybe we can put the timezone prop with selected & automatic

Beamanator · 2022-07-21T10:51:56Z

Fixing ^ issues in #10043

OSBotify · 2022-07-25T17:21:44Z

🚀 Deployed to production by @luacmartins in version: 1.1.85-8 🚀

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

puneetlath · 2022-07-26T18:14:23Z

Thanks for catching those @Beamanator!

mananjadhav · 2023-04-27T09:46:49Z

I am tagging this PR to highlight an issue fixed here. All conditions in ternary expressions or left-hand operands on conditional renders, should be boolean. This PR is one of the PRs that uses conditional render with string operands, hence I am tagging it here for the contributors to check.

We've also updated the item in the checklist with this PR to avoid this issue in the future.

puneetlath · 2023-04-27T13:52:34Z

Thanks for pointing that out!

Refactor AuthScreens.js to use PERSONAL_DETAILS instead of MY_PERSONA…

1a05abf

…L_DETAILS

puneetlath self-assigned this Jun 24, 2022

puneetlath added 20 commits June 24, 2022 15:58

Flag wich details in the personalDetails object belong to the current…

8b67fd9

… user

Update AuthScreen.js to look for new isCurrentUser flag

98092d7

Update Report.js to use PERSONAL_DETAILS instead of MY_PERSONAL_DETAILS

09fe20d

Replace semicolon that was supposed to be comma

413872f

Refactor ReportActionCompose.js to use PERSONAL_DETAILS instead of MY…

dc7170b

…_PERSONAL_DETAILS

Refactor RequestCallPage.js to use PERSONAL_DETAILS instead of MY_PER…

630e90e

…SONAL_DETAILS

Refactor DateUtils.js to use PERSONAL_DETAILS instead of MY_PERSONAL_…

c6a6d82

…DETAILS

Refactor EnablePayments.js to use PERSONAL_DETAILS instead of MY_PERS…

9aa4772

…ONAL_DETAILS

Refactor IOUModal.js to use PERSONAL_DETAILS instead of MY_PERSONAL_D…

a65ca25

…ETAILS

Refactor IOUCurrencySelection.js to use PERSONAL_DETAILS instead of M…

c3ba3b0

…Y_PERSONAL_DETAILS

Refactor SidebarLinks.js to use PERSONAL_DETAILS instead of MY_PERSON…

b1a1eb7

…AL_DETAILS

Refactor ProfilePage.js to use PERSONAL_DETAILS instead of MY_PERSONA…

dc7a183

…L_DETAILS

Remove no longer used currentUserPersonalDetailsPropsTypes

b035b1d

Refactor InitialSettingsPage.js to use PERSONAL_DETAILS instead of MY…

5f9bf56

…_PERSONAL_DETAILS

Refactor IOUConfirmationList.js to use PERSONAL_DETAILS instead of MY…

b005095

…_PERSONAL_DETAILS

No longer save to MY_PERSONAL_DETAILS key in fetchPersonalDetails method

22e1150

Refactor PersonalDetails.js to use PERSONAL_DETAILS instead of MY_PER…

085ed98

…SONAL_DETAILS

Remove the MY_PERSONAL_DETAILS onyx key

380dbaf

Merge branch 'main' into puneet-personaldetails-refactor

3d37ef3

Get rid of unnecessary personal details references from IOUCurrencySe…

67e1fd5

…lection.js

puneetlath marked this pull request as ready for review June 28, 2022 17:59

puneetlath requested review from a team and marcaaron as code owners June 28, 2022 17:59

melvin-bot bot requested review from joelbettner and removed request for a team June 28, 2022 17:59

marcaaron reviewed Jun 28, 2022

View reviewed changes

src/components/IOUConfirmationList.js Outdated Show resolved Hide resolved

Beamanator reviewed Jul 5, 2022

View reviewed changes

Create HOC for withPersonalDetails

8aec239

puneetlath added 3 commits July 15, 2022 19:34

dont flag the current user details in personal details

ccb9d27

Add more values to personal details prop type

0deaba4

Merge branch 'main' into puneet-personaldetails-refactor

972c310

puneetlath requested review from luacmartins, marcaaron and mountiny July 15, 2022 23:45

marcaaron approved these changes Jul 16, 2022

View reviewed changes

marcaaron merged commit 2dfef0f into main Jul 16, 2022

marcaaron deleted the puneet-personaldetails-refactor branch July 16, 2022 00:30

marcaaron mentioned this pull request Jul 18, 2022

💥 Crash 💥 .buildOptimisticReportAction #9945

Closed

OSBotify mentioned this pull request Jul 18, 2022

Deploy Checklist: New Expensify 2022-07-18 #9970

Closed

79 tasks

This was referenced Jul 19, 2022

fix: typo in personaldetails prop #9966

Merged

Use currentUserPersonalDetails key #9987

Merged

mountiny mentioned this pull request Jul 19, 2022

Remove call to non-existent fetchData method #9994

Merged

93 tasks

marcaaron mentioned this pull request Jul 19, 2022

Messages are being lost and fail to send #10000

Closed

5 tasks

Beamanator reviewed Jul 21, 2022

View reviewed changes

This was referenced Jul 21, 2022

Web: Report comment date & time always shows Today, at current time (not when comment was sent) #10042

Closed

[CP Stag] Fix lodashGet usage #10043

Merged

Puneet-here mentioned this pull request Aug 9, 2022

[HOLD for payment 2022-08-08] [HOLD for payment 2022-08-01] [$500] At #admins and #announce rooms, users own timezone displayed - reported by @Puneet-here #9238

Closed

alexxxwork mentioned this pull request Apr 21, 2023

[HOLD for payment 2023-05-05] [HOLD for payment 2023-04-20] [$1000] Text strings must be rendered within a <Text> component #16683

Closed

	import personalDetailsPropType from '../pages/personalDetailsPropType';
	import personalDetailsPropTypes from '../pages/personalDetailsPropTypes';

		selectedTimezone: lodashGet(this.props.currentUserPersonalDetails.timezone, 'selected', CONST.DEFAULT_TIME_ZONE.selected),
		isAutomaticTimezone: lodashGet(this.props.currentUserPersonalDetails.timezone, 'automatic', CONST.DEFAULT_TIME_ZONE.automatic),

Stop using MY_PERSONAL_DETAILS and only use PERSONAL_DETAILS in Onyx #9560

Stop using MY_PERSONAL_DETAILS and only use PERSONAL_DETAILS in Onyx #9560

Uh oh!

Conversation

puneetlath commented Jun 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Uh oh!

marcaaron left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Beamanator left a comment

Choose a reason for hiding this comment

Uh oh!

Beamanator Jul 5, 2022

Choose a reason for hiding this comment

Uh oh!

puneetlath Jul 11, 2022

Choose a reason for hiding this comment

Uh oh!

Beamanator Jul 5, 2022

Choose a reason for hiding this comment

Uh oh!

puneetlath commented Jul 15, 2022

Uh oh!

marcaaron left a comment

Choose a reason for hiding this comment

Uh oh!

marcaaron Jul 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcaaron commented Jul 16, 2022

Uh oh!

OSBotify commented Jul 16, 2022

Uh oh!

Beamanator Jul 21, 2022

Choose a reason for hiding this comment

Uh oh!

Beamanator Jul 21, 2022

Choose a reason for hiding this comment

Uh oh!

Beamanator Jul 21, 2022

Choose a reason for hiding this comment

Uh oh!

Beamanator commented Jul 21, 2022

Uh oh!

OSBotify commented Jul 25, 2022

Uh oh!

puneetlath commented Jul 26, 2022

Uh oh!

mananjadhav commented Apr 27, 2023

Uh oh!

puneetlath commented Apr 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

puneetlath commented Jun 24, 2022 •

edited

Loading

marcaaron Jul 16, 2022 •

edited

Loading