Find LHN lastActorDetails from global personal details rather than the participants list #14008

yuwenmemon · 2023-01-05T02:11:23Z

Details

We were unnecessarily grabbing personal details from the list of participants in the report. So if a user was removed from the report this would mean we would not be able to find their personal details. At the same time we already had the global personalDetails map in scope. So, just use that instead.

Fixed Issues

$ #13858

Tests

Sign in with User A
Go to Settings > Workspaces > New workspace
Go to Manage members and invite User B
Sign out and sign in with User B
Send a message to the invited workspace room
Sign out and sign in with User A again
Observe the last message on LHN with the user's name (e.g. User B: Hello)
Go to Settings > Workspace > Manage members
Remove User B
Make sure the last message in LHN still has the user's name (e.g. User B: Hello)

Verify that no errors appear in the JS console

Offline tests

Open up your workspace members like from the steps above
Go offline
Remove the user, you will see them optimistically crossed off:

Make sure you still see their name next to their message in the announce room:

QA Steps

Same as both sets of test steps above.

PR Author Checklist

Screenshots/Videos

Web

Kapture.2023-01-04.at.18.08.06.mp4

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

…e participants list

… or phone number

melvin-bot · 2023-01-05T02:11:51Z

@eVoloshchak @joelbettner One of you needs to 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]

yuwenmemon · 2023-01-05T17:07:53Z

src/libs/SidebarUtils.js

    }

-    const lastActorDetails = personalDetailMap[report.lastActorEmail] || null;
+    const lastActorDetails = personalDetails[report.lastActorEmail] || null;


This is the actual change that fixes the issue - the rest of the changes are just renaming variables in an attempt to make them more clear.

Hmm...I'm a little fuzzy on why we are doing some of this.

Why do we bother building the participantPersonalDetailList if we already have all that information in the personalDetails?

If we build participantPersonalDetailList because the getPersonalDetailsForLogins method gets personal details that might not currently be in personalDetails, shouldn't we use participantPersonalDetailList for getting the lastActorDetails as well? We know report.lastActorEmail will be in participantPersonalDetailList, how do we know that report.lastActorEmail will always be in personalDetails?

Why do we bother building the participantPersonalDetailList if we already have all that information in the personalDetails?

Answered in the above comment!

If we build participantPersonalDetailList because the getPersonalDetailsForLogins method gets personal details that might not currently be in personalDetails, . . .

Ah, that's not correct - take a look at the implementation for getPersonalDetailsForLogins - all that method is doing is pulling the Personal Details we want from the pesonalDetails object in Onyx. So, all Personal Details in participantPersonalDetailList will be in personalDetails.

yuwenmemon · 2023-01-09T15:54:41Z

@eVoloshchak @joelbettner bump!

eVoloshchak · 2023-01-09T18:33:07Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android / native
- Android / Chrome
- iOS / native
- iOS / Safari
- MacOS / Chrome / Safari
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

Screen.Recording.2023-01-09.at.20.23.14.mov

Mobile Web - Chrome

Screen_Recording_20230109-202728_Chrome.mp4

Mobile Web - Safari

Screen.Recording.2023-01-09.at.20.24.54.mov

Desktop

Screen.Recording.2023-01-09.at.20.22.18.mov

iOS

Screen.Recording.2023-01-09.at.20.25.54.mov

Android

Screen_Recording_20230109-202849_New.Expensify.mp4

eVoloshchak

Looks good

joelbettner · 2023-01-10T16:04:58Z

src/libs/SidebarUtils.js

    }

-    const lastActorDetails = personalDetailMap[report.lastActorEmail] || null;
+    const lastActorDetails = personalDetails[report.lastActorEmail] || null;


Hmm...I'm a little fuzzy on why we are doing some of this.

Why do we bother building the participantPersonalDetailList if we already have all that information in the personalDetails?

If we build participantPersonalDetailList because the getPersonalDetailsForLogins method gets personal details that might not currently be in personalDetails, shouldn't we use participantPersonalDetailList for getting the lastActorDetails as well? We know report.lastActorEmail will be in participantPersonalDetailList, how do we know that report.lastActorEmail will always be in personalDetails?

src/libs/SidebarUtils.js

github-actions · 2023-01-10T18:30:24Z

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries

Name	Duration
App start nativeLaunch	9.548 ms → 20.375 ms (+10.827 ms, +113.4%) 🟡
App start runJsBundle	183.500 ms → 187.313 ms (+3.813 ms, +2.1%)
App start TTI	665.539 ms → 668.690 ms (+3.151 ms, ±0.0%)
App start regularAppStart	0.014 ms → 0.020 ms (+0.006 ms, +43.6%) 🟡
Open Search Page TTI	610.897 ms → 608.184 ms (-2.713 ms, ±0.0%)

Show details

Name	Duration
App start nativeLaunch	Baseline Mean: 9.548 ms Stdev: 1.811 ms (19.0%) Runs: 8 8 8 8 8 8 8 8 8 8 8 8 9 9 9 9 9 9 9 10 10 10 10 11 11 11 11 12 12 14 15 Current Mean: 20.375 ms Stdev: 2.934 ms (14.4%) Runs: 18 18 18 18 18 18 18 18 18 18 18 18 18 19 19 19 19 20 20 20 20 21 21 21 22 23 23 25 25 26 27 28
App start runJsBundle	Baseline Mean: 183.500 ms Stdev: 18.873 ms (10.3%) Runs: 156 157 160 161 162 167 168 170 170 172 175 175 175 176 179 180 181 181 181 183 185 186 189 194 197 200 206 209 209 217 222 229 Current Mean: 187.313 ms Stdev: 23.462 ms (12.5%) Runs: 157 158 158 163 166 167 168 169 169 172 172 177 177 178 180 181 184 185 186 186 190 192 193 195 196 209 209 214 222 232 243 246
App start TTI	Baseline Mean: 665.539 ms Stdev: 26.968 ms (4.1%) Runs: 624.186211001128 625.9633660018444 627.7246930003166 628.3107540011406 630.005052998662 637.178422998637 637.4107169993222 644.2519830018282 644.9481839984655 646.763723000884 656.8954380005598 661.1250649988651 664.7233420014381 665.953832000494 666.3582439981401 666.4707800000906 668.2485640011728 668.7255369983613 669.3052579984069 670.3070060014725 672.8782920017838 673.5119390003383 675.6728870011866 679.416329998523 685.4120070002973 693.0115340016782 698.3758860006928 698.5442560017109 699.4922260008752 715.348207000643 735.1867320016026 Current Mean: 668.690 ms Stdev: 25.141 ms (3.8%) Runs: 635.466099999845 636.3564519993961 637.4385760016739 640.1012419983745 642.4612429998815 643.3540330007672 645.5563080012798 648.2214910015464 651.9208070002496 653.6977810002863 655.7080360017717 655.7665040008724 661.6492840014398 663.7121809981763 665.5634460002184 667.5443120002747 667.7035339996219 667.8172640018165 669.9580800011754 673.7886449992657 675.6929229982197 677.6386680006981 678.9272839985788 689.2848570011556 691.708207000047 691.9873180016875 707.5169759988785 710.4619529992342 722.9076250009239 730.7813480012119
App start regularAppStart	Baseline Mean: 0.014 ms Stdev: 0.001 ms (9.7%) Runs: 0.012696001678705215 0.01285799965262413 0.012939997017383575 0.012940000742673874 0.012940000742673874 0.01297999918460846 0.013020001351833344 0.013060998171567917 0.013060998171567917 0.013223998248577118 0.01326499879360199 0.013387002050876617 0.01342800259590149 0.013631999492645264 0.013752996921539307 0.013875000178813934 0.013875000178813934 0.013875998556613922 0.014079000800848007 0.01416100189089775 0.014201000332832336 0.014485999941825867 0.014607999473810196 0.014649000018835068 0.01505500078201294 0.016683001071214676 0.017090000212192535 0.017212003469467163 0.01753699779510498 Current Mean: 0.020 ms Stdev: 0.001 ms (5.9%) Runs: 0.01798500120639801 0.018718000501394272 0.01879899948835373 0.018839001655578613 0.018919996917247772 0.019124001264572144 0.019164998084306717 0.01924699917435646 0.019328001886606216 0.019369002431631088 0.019734997302293777 0.01985599845647812 0.01989699900150299 0.020344998687505722 0.020426001399755478 0.020589999854564667 0.02075200155377388 0.02087400108575821 0.02103700116276741 0.02103700116276741 0.02103700116276741 0.021118000149726868 0.021646998822689056 0.021769002079963684 0.021891999989748 0.02197299897670746 0.02278599888086319
Open Search Page TTI	Baseline Mean: 610.897 ms Stdev: 19.974 ms (3.3%) Runs: 561.7410479970276 576.6168209984899 580.4978839978576 584.9719239994884 589.7025560028851 591.9163819998503 594.6308190003037 595.359294001013 595.4777429997921 598.8819989971817 601.5528559982777 602.8341070003808 606.4155279994011 609.5951340012252 609.710287000984 611.6698819994926 613.2278649993241 613.9125169999897 614.7076019980013 617.0923670008779 617.6140959970653 618.9636639989913 621.9052729979157 622.2401540018618 624.2747809998691 625.2430020011961 626.0749109983444 626.7878829985857 635.7853600010276 637.7852380014956 638.8066009990871 639.4334319978952 654.1752530001104 Current Mean: 608.184 ms Stdev: 24.985 ms (4.1%) Runs: 559.1820069998503 569.4121089987457 573.1193040013313 576.4040130004287 577.5786130018532 577.6765139997005 586.8538010008633 587.4666750021279 589.8522539995611 589.9945480003953 594.4962980002165 595.3876550011337 600.8546959981322 602.0924890004098 602.7616369985044 605.2913010008633 609.1844900026917 611.0963139981031 612.8032229989767 614.803752001375 620.1295580007136 621.3036700002849 621.9776610024273 622.7216800004244 623.2455240003765 626.4792890027165 629.2442220002413 636.1221110001206 637.0990809984505 637.8517259992659 643.5307209976017 645.9418540000916 668.1043299995363

OSBotify · 2023-01-10T18:34:50Z

🚀 Deployed to staging by @joelbettner in version: 1.2.52-4 🚀

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

OSBotify · 2023-01-11T13:18:53Z

🚀 Deployed to staging by @joelbettner in version: 1.2.52-4 🚀

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

OSBotify · 2023-01-11T14:35:47Z

🚀 Deployed to staging by @joelbettner in version: 1.2.52-4 🚀

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

OSBotify · 2023-01-12T13:37:48Z

🚀 Deployed to production by @Julesssss in version: 1.2.52-4 🚀

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

yuwenmemon added 3 commits January 4, 2023 17:38

Find LHN lastActorDetails from global personal details rather than th…

115aa11

…e participants list

Even if we don't have personal details allow for displaying the email…

2aa9b02

… or phone number

Use null if we don't have a personal detail

4d2c081

yuwenmemon self-assigned this Jan 5, 2023

yuwenmemon requested a review from a team as a code owner January 5, 2023 02:11

melvin-bot bot requested review from eVoloshchak and joelbettner and removed request for a team January 5, 2023 02:11

yuwenmemon commented Jan 5, 2023

View reviewed changes

eVoloshchak approved these changes Jan 9, 2023

View reviewed changes

joelbettner suggested changes Jan 10, 2023

View reviewed changes

joelbettner reviewed Jan 10, 2023

View reviewed changes

src/libs/SidebarUtils.js Show resolved Hide resolved

joelbettner approved these changes Jan 10, 2023

View reviewed changes

joelbettner merged commit bc1deae into main Jan 10, 2023

joelbettner deleted the yuwen-participants branch January 10, 2023 17:59

OSBotify mentioned this pull request Jan 10, 2023

Deploy Checklist: New Expensify 2023-01-10 #14188

Closed

15 tasks

melvin-bot bot mentioned this pull request Jan 12, 2023

[HOLD for payment 2023-01-19] Removing a member from the workspace removes their name from the LHN as well #13858

Closed

6 tasks

yuwenmemon referenced this pull request Jan 19, 2023

Move create option to new utils and remove non-report logic

c0131c3

Find LHN lastActorDetails from global personal details rather than the participants list #14008

Find LHN lastActorDetails from global personal details rather than the participants list #14008

Uh oh!

Conversation

yuwenmemon commented Jan 5, 2023

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Uh oh!

melvin-bot bot commented Jan 5, 2023

Uh oh!

yuwenmemon Jan 5, 2023

Choose a reason for hiding this comment

Uh oh!

joelbettner Jan 10, 2023

Choose a reason for hiding this comment

Uh oh!

yuwenmemon Jan 10, 2023

Choose a reason for hiding this comment

Uh oh!

yuwenmemon commented Jan 9, 2023

Uh oh!

eVoloshchak commented Jan 9, 2023

Reviewer Checklist

Screenshots/Videos

Uh oh!

eVoloshchak left a comment

Choose a reason for hiding this comment

Uh oh!

joelbettner Jan 10, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Jan 10, 2023

Performance Comparison Report 📊

Significant Changes To Duration

Meaningless Changes To Duration

Uh oh!

OSBotify commented Jan 10, 2023

Uh oh!

OSBotify commented Jan 11, 2023

Uh oh!

OSBotify commented Jan 11, 2023

Uh oh!

OSBotify commented Jan 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants