Fix group member order changing randomly when group members have phone numbers #15509

abdulrahuman5196 · 2023-02-27T16:53:34Z

Fix group member order changing randomly when group members have phone numbers

Details

Group members order changes randomly when it has more than 1 user with phone number. This is due to report.participants getting sorted using .sort() function is ReportUtils -

App/src/libs/ReportUtils.js

Line 1384 in 4d3390b

    
           return !isUserCreatedPolicyRoom(report) && _.isEqual(newParticipantList, report.participants.sort());

Fixed Issues

$ #14886
PROPOSAL: #14886 (comment)

Tests

Have a group chat with combination of members with phone number and email/name logins(eg: ["test@expensify.com", "test1@expensify.com", "+17512345678", "+14512345678" ]
Note the order of group members shown in the chat list page.
Open the group chat
Open the members list page of group
Click on a member with phone number login and choose message option in the user detail page
Now the order at step 2 should be maintained

Verify that no errors appear in the JS console

Offline tests

Have a group chat with combination of members with phone number and email/name logins(eg: ["test@expensify.com", "test1@expensify.com", "+17512345678", "+14512345678" ]
Note the order of group members shown in the chat list page.
Open the group chat
Open the members list page of group
Click on a member with phone number login and choose message option in the user detail page
Now the order at step 2 should be maintained

QA Steps

Have a group chat with combination of members with phone number and email/name logins(eg: ["test@expensify.com", "test1@expensify.com", "+17512345678", "+14512345678" ]
Note the order of group members shown in the chat list page.
Open the group chat
Open the members list page of group
Click on a member with phone number login and choose message option in the user detail page
Now the order at step 2 should be maintained

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

Untitled.4.mp4

Mobile Web - Chrome

mweb.chrome.mp4

Mobile Web - Safari

mweb.safari.mov

Desktop

desktop.mov

iOS

ios.mov

Android

Untitled.5.mp4

Fix group member order changing randomly when group members have phone numbers

MelvinBot · 2023-02-27T18:25:58Z

@luacmartins @sobitneupane 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]

sobitneupane

Screenshots/Videos

Web

Screen.Recording.2023-02-28.at.13.19.57.mov

Mobile Web - Chrome

Screen.Recording.2023-02-28.at.13.25.57.mov

Mobile Web - Safari

Screen.Recording.2023-02-28.at.13.32.49.mov

Desktop

Screen.Recording.2023-02-28.at.13.24.27.mov

iOS

Screen.Recording.2023-02-28.at.13.29.46.mov

Android

Screen.Recording.2023-02-28.at.13.29.04.mov

sobitneupane

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.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

MelvinBot · 2023-02-28T18:47:26Z

Congrats, that’s your 5th PR merged! 🎉 Do you know about the ContributorPlus role? It’s an opportunity to earn more in the Expensify Open Source community. Keep up the great work - thanks!

OSBotify · 2023-02-28T18:50:54Z

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

github-actions · 2023-02-28T19:20:52Z

Performance Comparison Report 📊

Significant Changes To Duration

Name	Duration
Open Search Page TTI	613.197 ms → 672.339 ms (+59.142 ms, +9.6%) 🔴

Show details

Name	Duration
Open Search Page TTI	Baseline Mean: 613.197 ms Stdev: 17.689 ms (2.9%) Runs: 579.1903890001122 587.1886390000582 587.4364020000212 589.1018070001155 590.2661950001493 596.5021570001263 597.9469810000155 600.7362879998982 602.9370120000094 603.2497570000123 603.3238939999137 607.0350349999499 608.1463619999122 608.8496499999892 610.6724450001493 612.7646079999395 613.6227619999554 615.0855709998868 617.1716310000047 619.9409190001898 622.060220999876 625.0010180000681 625.7524829998147 627.4044600001071 628.303222999908 628.3404540000483 631.6531569999643 634.7415359998122 638.7890630001202 642.2221270001028 653.6701660000253 Current Mean: 672.339 ms Stdev: 22.649 ms (3.4%) Runs: 627.457154000178 628.9751800000668 629.7384849996306 636.4877119995654 639.8376470003277 647.7595220003277 653.3678390001878 656.0998949999921 664.6434329999611 665.4555259998888 668.3303220001981 668.7228190000169 670.5687669999897 672.3857420003042 674.5861010001972 674.6444509997964 675.993489000015 676.1642660000362 676.2593999998644 677.897135999985 681.4646820002235 681.9036470004357 683.8844000003301 684.8621420003474 685.8539229999296 692.1452229996212 696.0324309999123 697.829101999756 703.698243000079 703.9073489997536 708.185099999886 709.7147630001418

Meaningless Changes To Duration

Show entries

Name	Duration
App start TTI	741.707 ms → 745.258 ms (+3.552 ms, ±0.0%)
App start regularAppStart	0.015 ms → 0.015 ms (+0.000 ms, +2.3%)
App start nativeLaunch	19.742 ms → 19.656 ms (-0.086 ms, ±0.0%)
App start runJsBundle	205.226 ms → 200.548 ms (-4.677 ms, -2.3%)

Show details

Name	Duration
App start TTI	Baseline Mean: 741.707 ms Stdev: 28.568 ms (3.9%) Runs: 682.3056820000056 697.906915999949 699.8418850000016 702.1909290000331 703.6011640001088 710.3422469999641 716.9773210000712 723.4828689999413 725.2134950000327 733.2183809999842 734.3849069999997 735.045361999888 737.3515920001082 739.3208580000792 741.7227789999451 742.6237459999975 742.6700939999428 742.8264999999665 745.1516919999849 749.5835460000671 754.2702880001161 754.6230019999202 757.4128550000023 759.679002000019 763.8946360000409 765.2935399999842 766.0759320000652 786.723660999909 787.4506230000407 788.9599109999835 802.7623739999253 Current Mean: 745.258 ms Stdev: 27.110 ms (3.6%) Runs: 704.3380710000638 705.7565049999394 707.5821779998951 716.5408219997771 718.0727329999208 718.981732999906 720.1243040000554 725.2783349999227 728.6389200000558 732.7901369999163 733.0897860000841 733.1154289999977 735.343634000048 735.4055729999673 736.3031080001965 741.2138729998842 741.2436929999385 742.8260309998877 744.2749290000647 745.0261800000444 748.2911789999343 750.2562150000595 750.4144810000435 755.424362000078 768.5222700000741 772.169380000094 772.1991040001158 773.932833999861 787.4432469999883 792.5858950000256 793.8655119999312 817.213633999927
App start regularAppStart	Baseline Mean: 0.015 ms Stdev: 0.001 ms (5.7%) Runs: 0.013387000188231468 0.013510000193491578 0.013589999871328473 0.013672000030055642 0.0138349998742342 0.013876000186428428 0.013916000025346875 0.013916000025346875 0.01395700010471046 0.014201000100001693 0.01432300009764731 0.01436399994418025 0.014404000015929341 0.014405000023543835 0.014567000092938542 0.014649000018835068 0.014690000098198652 0.014770000008866191 0.014851999934762716 0.014932999853044748 0.015015000011771917 0.015339999925345182 0.015503000002354383 0.01566599984653294 0.015705999918282032 0.015705999918282032 0.01570700015872717 0.015746999997645617 0.015787999844178557 0.01607199991121888 0.01639799983240664 Current Mean: 0.015 ms Stdev: 0.001 ms (5.2%) Runs: 0.013752999948337674 0.013794000027701259 0.014038000022992492 0.014282000018283725 0.014283000025898218 0.014445000095292926 0.014566999860107899 0.014566999860107899 0.014607999939471483 0.014649000018835068 0.014689000090584159 0.014730000169947743 0.014810999855399132 0.0148930000141263 0.015015000011771917 0.015015000011771917 0.01513600000180304 0.015257999999448657 0.015299999853596091 0.015339999925345182 0.015379999997094274 0.015461999922990799 0.015542999841272831 0.015705999918282032 0.015746999997645617 0.015868999995291233 0.01607300015166402 0.016805000137537718 0.017171000130474567
App start nativeLaunch	Baseline Mean: 19.742 ms Stdev: 1.565 ms (7.9%) Runs: 18 18 18 18 18 18 18 18 18 19 19 19 19 19 19 19 20 20 20 20 21 21 21 21 21 21 21 22 22 22 24 Current Mean: 19.656 ms Stdev: 1.864 ms (9.5%) Runs: 17 17 18 18 18 18 18 18 18 18 19 19 19 19 19 19 19 19 19 20 20 20 20 21 21 21 22 22 22 23 24 24
App start runJsBundle	Baseline Mean: 205.226 ms Stdev: 19.206 ms (9.4%) Runs: 174 174 184 185 185 187 188 191 193 195 196 199 199 201 202 204 206 207 207 210 210 210 212 213 218 221 221 229 233 253 255 Current Mean: 200.548 ms Stdev: 15.170 ms (7.6%) Runs: 173 174 180 186 187 188 188 189 190 190 192 193 196 197 198 199 201 203 205 206 206 208 208 209 211 212 214 219 225 233 237

github-actions · 2023-02-28T19:20:55Z

@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker.

luacmartins · 2023-02-28T20:53:49Z

@abdulrahuman5196 @sobitneupane wow it looks like _.sortBy is a lot slower than sort. We might have to revert this and look for another solution.

abdulrahuman5196 · 2023-02-28T21:08:54Z

@luacmartins i am not sure if this is due to _.sortBy().

Even in expensify-open-source i saw a post mentioning false latency increase message in PR.

abdulrahuman5196 · 2023-02-28T21:14:07Z

Slack thread on the same - https://expensify.slack.com/archives/C01GTK53T8Q/p1677275385841379

@luacmartins

And to note this flow will only be initiated when we open a report. Search doesn't have anything to do with this.

AFAIK this should be false positive, I think we should confirm before reverting the change

luacmartins · 2023-02-28T21:21:27Z

Thanks for the link! I bumped that thread, let's see what happens

luacmartins · 2023-03-01T15:48:06Z

Yea it seems like this was caused by another PR. I'll remove the deploy blocker label.

OSBotify · 2023-03-02T23:17:03Z

🚀 Deployed to staging by https://github.com/luacmartins in version: 1.2.78-0 🚀

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

OSBotify · 2023-03-06T15:38:41Z

🚀 Deployed to production by https://github.com/mountiny in version: 1.2.78-0 🚀

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

Issue Expensify#14886

f2be136

Fix group member order changing randomly when group members have phone numbers

abdulrahuman5196 marked this pull request as ready for review February 27, 2023 18:25

abdulrahuman5196 requested a review from a team as a code owner February 27, 2023 18:25

melvin-bot bot requested review from luacmartins and sobitneupane and removed request for a team February 27, 2023 18:25

abdulrahuman5196 mentioned this pull request Feb 27, 2023

[HOLD for payment 2023-03-13] [$1000] Group member orders keeps on changing randomly when it has more then 1 user with phone number as primaryLogin #14886

Closed

6 tasks

sobitneupane reviewed Feb 28, 2023

View reviewed changes

sobitneupane approved these changes Feb 28, 2023

View reviewed changes

luacmartins approved these changes Feb 28, 2023

View reviewed changes

luacmartins merged commit 05c84c8 into Expensify:main Feb 28, 2023

github-actions bot added the DeployBlockerCash This issue or pull request should block deployment label Feb 28, 2023

luacmartins removed the DeployBlockerCash This issue or pull request should block deployment label Mar 1, 2023

OSBotify mentioned this pull request Mar 2, 2023

Deploy Checklist: New Expensify 2023-03-02 #15618

Closed

35 tasks

kbecciv mentioned this pull request Mar 3, 2023

[$1000] Group Details - The order of the group members changes #15644

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix group member order changing randomly when group members have phone numbers #15509

Fix group member order changing randomly when group members have phone numbers #15509

abdulrahuman5196 commented Feb 27, 2023 •

edited

Loading

MelvinBot commented Feb 27, 2023

sobitneupane left a comment

sobitneupane left a comment

MelvinBot commented Feb 28, 2023

OSBotify commented Feb 28, 2023

github-actions bot commented Feb 28, 2023

github-actions bot commented Feb 28, 2023

luacmartins commented Feb 28, 2023

abdulrahuman5196 commented Feb 28, 2023

abdulrahuman5196 commented Feb 28, 2023

luacmartins commented Feb 28, 2023

luacmartins commented Mar 1, 2023

OSBotify commented Mar 2, 2023

OSBotify commented Mar 6, 2023

Fix group member order changing randomly when group members have phone numbers #15509

Fix group member order changing randomly when group members have phone numbers #15509

Conversation

abdulrahuman5196 commented Feb 27, 2023 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

MelvinBot commented Feb 27, 2023

sobitneupane left a comment

Choose a reason for hiding this comment

Screenshots/Videos

sobitneupane left a comment

Choose a reason for hiding this comment

Reviewer Checklist

MelvinBot commented Feb 28, 2023

OSBotify commented Feb 28, 2023

github-actions bot commented Feb 28, 2023

Performance Comparison Report 📊

Significant Changes To Duration

Meaningless Changes To Duration

github-actions bot commented Feb 28, 2023

luacmartins commented Feb 28, 2023

abdulrahuman5196 commented Feb 28, 2023

abdulrahuman5196 commented Feb 28, 2023

luacmartins commented Feb 28, 2023

luacmartins commented Mar 1, 2023

OSBotify commented Mar 2, 2023

OSBotify commented Mar 6, 2023

abdulrahuman5196 commented Feb 27, 2023 •

edited

Loading