Refactor `PersonalDetails_Update` with a few new commands by Beamanator · Pull Request #9696 · Expensify/App

Beamanator · 2022-07-05T13:42:17Z

Details

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/211813

Tests

Same tests as: https://github.com/Expensify/Web-Expensify/pull/34219:

`UpdateProfile`

Go to profile, update name & timezone, save
Make sure everything updates correctly (& in multiple devices!)
Verify you see this in the App logs:

[info] [Report] Handled onyxApiUpdate event sent by Pusher - [{"onyxMethod":"merge","key":"personalDetails","value":{"alexbeaman@expensifail.com":{"firstName":"TheE","lastName":"OriginalFake","pronouns":"__predefined_callMeByMyName"}}}]
[info] [Report] Handled onyxApiUpdate event sent by Pusher - [{"onyxMethod":"merge","key":"personalDetails","value":{"alexbeaman@expensifail.com":{"timezone":{"automatic":false,"selected":"Africa/Cairo"}}}}]

`OpenProfile`

Make sure your timezone is set and is ready to update automatically
In Onyx, manually change your timezone to a valid timezone, in a different time zone
Open profile page
Verify Tz updates back to your current timezone everywhere!
Verify you see this in the App logs: [info] [Report] Handled onyxApiUpdate event sent by Pusher - [{"onyxMethod":"merge","key":"personalDetails","value":{"alexbeaman@expensifail.com":{"timezone":{"automatic":true,"selected":"Africa/Cairo"}}}}]

`DeleteUserAvatar`

Delete your personal user avatar
Verify you now get a default avatar set on all devices!
Verify you see this in the App logs: [info] [Report] Handled onyxApiUpdate event sent by Pusher - [{"onyxMethod":"merge","key":"personalDetails","value":{"alexbeaman@expensifail.com":{"avatar":"https://d2k5nsl2zxldvw.cloudfront.net/images/avatars/avatar_7.png"}}}]

Verify that no errors appear in the JS console

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Same as "Tests" section, but don't worry about checking the logs

Verify that no errors appear in the JS console

Screenshots

Web & Desktop

Updating timezone by opening profile:

Screen.Recording.2022-07-19.at.4.38.22.PM.mov

Deleting profile image:

Screen.Recording.2022-07-19.at.4.40.31.PM.mov

Updating profile info

Screen.Recording.2022-07-19.at.4.42.18.PM.mov

Mobile Web

iOS

Android

…ersonalDetailsUpdate

Beamanator · 2022-07-18T14:57:53Z

The App PR we were waiting on has been merged into main & into this branch! Ready for review 👍

…ersonalDetailsUpdate

Gonals · 2022-07-19T14:21:06Z

Seems like Travis is not happy 😁

Gonals · 2022-07-19T14:21:18Z

Can we get a few screenshots?

Gonals · 2022-07-19T14:21:49Z

This should be on HOLD until the Web counterpart merges, right?

Beamanator · 2022-07-19T14:22:58Z

Put on hold :D Dang travis!!

And ya.... Videos are good... But... annoying to make... but will do :D

mountiny · 2022-07-28T15:21:00Z

@Beamanator So I see it in your videos as well, but it is expected we wont update the details for the user automatically? For example, I feel like it is confusing that I have updated the name and then I go back to the settings but the name is not updated yet, I need to refresh or close settings and go back to the profile settings which updates the name in UI as well

mountiny · 2022-07-28T15:23:47Z

+        optimisticData: [{
+            onyxMethod: CONST.ONYX.METHOD.MERGE,
+            key: ONYXKEYS.PERSONAL_DETAILS,
+            value: {
+                [currentUserEmail]: {
+                    firstName,
+                    lastName,
+                    pronouns,
+                    timezone,
+                },
+            },
+        }],


@Beamanator should this optimistic data actually update it in the UI automatically? Is it that we do not rerender the RHS when we go back from the settings?

In profile settings:

Clicking back to the settings, still old name

I think that should be fixed because as user I would be perplexed what is correct.

Beamanator · 2022-07-28T15:55:45Z

@mountiny I also saw that prop type timezone error (you mentioned here) but not for the account I was testing with - I think it's a bug in how we default a timezone when that account doesn't have a timezone set in an NVP - but I'm not 100% sure, didn't have enough time to investigate today

about why personal details aren't updating even after going away from the page & back, that's really weird I will test that out tomorrow 🤔 thanks for the thorough testing!

mountiny · 2022-07-28T16:08:53Z

No, thank you for working on this!

Beamanator · 2022-07-29T11:08:32Z

Ok @vitHoracek it looks like this is the fix you're looking for, right? I'm about to commit these changes

Screen.Recording.2022-07-29.at.1.06.42.PM.mov

mountiny · 2022-07-29T11:11:15Z

@Beamanator YAAASS 😍

Beamanator · 2022-07-29T11:11:55Z

+                    displayName: getDisplayName(currentUserEmail, {
+                        firstName,
+                        lastName,
+                    }),


FYI this will need to be added in Web-E so that all clients get updated with this displayName, I plan to add this requirement in this issue: https://github.com/Expensify/Expensify/issues/220321

Ok, but we should not get any problems when sending this. It will get updated for other users when their refresh, but it is not included in the Pusher update you saying?

Right right exactly @mountiny - nothing will break, only the client making the change will get these updates automatically, others will get it after refresh 👍

Beamanator · 2022-07-29T11:14:17Z

Great!! This will not update on all clients yet (see my previous comment) but I'll fix that in a following Web-E PR 👍

Beamanator · 2022-07-29T13:02:40Z

Actually before we merge this, I'm going to add firstName, lastName, and pronouns as their own params to UpdateProfile so that it will be fewer PRs to change from details to the individual params

…ersonalDetailsUpdate

Beamanator · 2022-07-29T13:06:27Z

Ok now I think we're good to go again

mountiny

I have one more outstanding question there.

Otherwise I think we are good to go with how the timezones work now, but we should update the test steps. Also I would copy them over from the Web-E so it is easier for QA to follow!

Great job Alex, this one is now easy

mountiny · 2022-07-29T15:28:07Z

+    const oldTimezoneData = myPersonalDetails.timezone || {};
+    const newTimezoneData = {
+        automatic: lodashGet(oldTimezoneData, 'automatic', true),
+        selected: moment.tz.guess(true),


Discussed with Alex in DM, I think this function does not work as expected with the VPNs and it does not set you to the timezone of the area. However, that is how we have it now and I would argue it is better as in the chat you want others to see your actual timezone and not the VPN timezone.

mountiny · 2022-07-29T15:38:21Z

-            hasSelfSelectedPronouns: !_.isEmpty(this.props.currentUserPersonalDetails.pronouns) && !this.props.currentUserPersonalDetails.pronouns.startsWith(CONST.PRONOUNS.PREFIX),
-            selectedTimezone: lodashGet(this.props.currentUserPersonalDetails, 'timezone.selected', CONST.DEFAULT_TIME_ZONE.selected),
-            isAutomaticTimezone: lodashGet(this.props.currentUserPersonalDetails, 'timezone.automatic', CONST.DEFAULT_TIME_ZONE.automatic),
+            hasSelfSelectedPronouns: !(currentUserDetails.pronouns || '').startsWith(CONST.PRONOUNS.PREFIX),


@Beamanator I think this change is not gonna result in the same boolean right?

if pronouns is empty, we will get false for hasSelfSelectedPronouns here:

!_.isEmpty(this.props.currentUserPersonalDetails.pronouns)

but if pronouns is empty with the new line, that would be startsWith(PREFIX) is false and negating the entire line will give true.

Nonetheless, even if it is correct, it is hard to understand 😄

Dude good shout, this would probably cause weird behavior if pronouns is empty 🙃 👍

I can revert part of the refactor here, just keeping the currentPersonalDetails simplification

@Beamanator Sounds good!

…ersonalDetailsUpdate

mountiny

Thanks for working on thins and for all the changes!

Beamanator · 2022-08-01T10:49:09Z

Added necessary check back (for has personal pronouns), brought tests here from the Web-E PR, and changed the timezone checks to not test by using a VM

mountiny · 2022-08-01T11:00:14Z

Since Alberto approved the changes before, let's

OSBotify · 2022-08-01T11:07:15Z

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

OSBotify · 2022-08-01T22:39:00Z

🚀 Deployed to staging by @mountiny in version: 1.1.87-0 🚀

platform	result
🤖 android 🤖	failure ❌
🖥 desktop 🖥	failure ❌
🍎 iOS 🍎	failure ❌
🕸 web 🕸	failure ❌

Beamanator · 2022-08-03T10:43:17Z

+        PersonalDetails.updateProfile(
+            this.state.firstName.trim(),
+            this.state.lastName.trim(),
+            this.state.pronouns.trim(),
+            {
                automatic: this.state.isAutomaticTimezone,
                selected: this.state.selectedTimezone,
            },
-        }, true);
+        );


This tiny change gave rise to #10215

Note: The main issue is opacity being incorrect, not the Growl being missing

OSBotify · 2022-08-04T20:51:03Z

🚀 Deployed to production by @yuwenmemon in version: 1.1.87-9 🚀

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

Beamanator added 3 commits July 5, 2022 15:33

New command OpenProfilePage

faea1c4

New command UpdateProfile

6057115

New command DeleteUserAvatar (WIP)

50ba9ec

Beamanator self-assigned this Jul 5, 2022

Beamanator commented Jul 6, 2022

View reviewed changes

Comment thread src/libs/actions/PersonalDetails.js

Beamanator added 3 commits July 15, 2022 15:02

Merge branch 'main' of github.com:Expensify/App into beaman-refactorP…

d5836f2

…ersonalDetailsUpdate

Remove 'page' from command

fd71758

Clean up logic for deleting user avatar

9f8d7f0

Beamanator changed the title ~~[WIP] Refactor PersonalDetails_Update~~ [HOLD #9560] Refactor PersonalDetails_Update Jul 15, 2022

Clean up logic and lint

0472768

Beamanator marked this pull request as ready for review July 15, 2022 16:36

Beamanator requested a review from a team as a code owner July 15, 2022 16:36

melvin-bot Bot requested review from Gonals and removed request for a team July 15, 2022 16:36

Beamanator added 3 commits July 15, 2022 19:10

Fix new openProfile function name

35a24ed

Use onyx consts, stringify data

70ce913

Stringify data sent to server & fix default avatar

f8beb46

Beamanator changed the title ~~[HOLD #9560] Refactor PersonalDetails_Update~~ Refactor PersonalDetails_Update Jul 18, 2022

Beamanator added 3 commits July 18, 2022 15:52

Merge branch 'main' of github.com:Expensify/App into beaman-refactorP…

816ed7a

…ersonalDetailsUpdate

Clean up logic getting my personal details

3ebc476

Update state if avatar prop changes

1c23ade

Beamanator added 2 commits July 19, 2022 16:09

Merge branch 'main' of github.com:Expensify/App into beaman-refactorP…

0a05600

…ersonalDetailsUpdate

Remove unused import

259e877

Beamanator changed the title ~~Refactor PersonalDetails_Update~~ [HOLD Web-Expensify#34219] Refactor PersonalDetails_Update Jul 19, 2022

Remove dupe import

0ec78f2

mountiny requested changes Jul 28, 2022

View reviewed changes

Update displayName in onyx when name changes

3792cc8

Beamanator dismissed Gonals’s stale review via 3792cc8 July 29, 2022 11:10

Beamanator commented Jul 29, 2022

View reviewed changes

Beamanator requested a review from mountiny July 29, 2022 11:14

Beamanator added 2 commits July 29, 2022 15:02

Merge branch 'main' of github.com:Expensify/App into beaman-refactorP…

21655be

…ersonalDetailsUpdate

Send all personal details as own params

d37051b

mountiny reviewed Jul 29, 2022

View reviewed changes

Beamanator added 2 commits August 1, 2022 11:50

Merge branch 'main' of github.com:Expensify/App into beaman-refactorP…

c2f5de5

…ersonalDetailsUpdate

Add back important check

40b05a4

mountiny approved these changes Aug 1, 2022

View reviewed changes

mountiny merged commit fd7c59a into main Aug 1, 2022

mountiny deleted the beaman-refactorPersonalDetailsUpdate branch August 1, 2022 11:00

OSBotify mentioned this pull request Aug 1, 2022

Deploy Checklist: New Expensify 2022-08-01 #10169

Closed

95 tasks

kbecciv mentioned this pull request Aug 2, 2022

Android - Settings-Time zone is not updating after Saved changes #10217

Closed

Beamanator commented Aug 3, 2022

View reviewed changes

Beamanator mentioned this pull request Aug 3, 2022

Fix timezone not showing correct timezone after save #10231

Merged

47 tasks

Conversation

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

Uh oh!

Details

Fixed Issues

Tests

UpdateProfile

OpenProfile

DeleteUserAvatar

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Screenshots

Web & Desktop

Mobile Web

iOS

Android

Uh oh!

Uh oh!

Beamanator commented Jul 18, 2022

Uh oh!

Gonals commented Jul 19, 2022

Uh oh!

Gonals commented Jul 19, 2022

Uh oh!

Gonals commented Jul 19, 2022

Uh oh!

Beamanator commented Jul 19, 2022

Uh oh!

mountiny commented Jul 28, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Beamanator commented Jul 28, 2022

Uh oh!

mountiny commented Jul 28, 2022

Uh oh!

Beamanator commented Jul 29, 2022

Uh oh!

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

Uh oh!

Uh oh!

Beamanator Jul 29, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Beamanator commented Jul 29, 2022

Uh oh!

Beamanator commented Jul 29, 2022

Uh oh!

Beamanator commented Jul 29, 2022

Uh oh!

mountiny left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mountiny left a comment

Choose a reason for hiding this comment

Uh oh!

Beamanator commented Aug 1, 2022

Uh oh!

mountiny commented Aug 1, 2022

Uh oh!

OSBotify commented Aug 1, 2022

Uh oh!

OSBotify commented Aug 1, 2022

Uh oh!

Choose a reason for hiding this comment

Beamanator commented Jul 5, 2022 •

edited

Loading

`UpdateProfile`

`OpenProfile`

`DeleteUserAvatar`

mountiny commented Jul 29, 2022 •

edited

Loading

Beamanator Jul 29, 2022 •

edited

Loading