Check if policy room is archived because of merging #14779

mountiny · 2023-02-03T00:06:27Z

Details

Finding out if the archive workspace is coming from merged account is not easy. The archived workspace of the old account (by old its meant an account which got merged into the current "new" account) is still associated with the old accountID, hence the normal condition to figure out the name of the workspace chat is not enough since the new account does not technically "own" the report, its just shared with it.

Hence we need to add further checks, we will check if the room is archived and if the last report action in that chat report is a closed report action with MERGED archiveReason. This ensures that for a member of the workspace the chat report is correctly names Workspace name as that is what is expected.

Then the other case is that Admin sees this

Fixed Issues

$ #14292

Tests

Verify that no errors appear in the JS console

Create 3 new accounts
As user A, log in and create a new workspace
Invite userB and userC to the workspace
UserA as Admin, send a message to the UserB and UserC workspace chats, something like Here is admin
Log in as userB and send a message to the workspace chat: Message from userB
Log in as userC and send a message to the workspace chat: Message from userC
Go to oldDot and log in as userB
Go to Account setting and merge the UserC to the UserB account
Log in to the UserB in NewDot
Verify you can see UserC's workspace (archived) chat in the LHN, navigate to it
Verify you can see a message from UserC saying: Message from userC
Verify there is a Closed report action with correct merged archive reason message.
Log in as admin - UserA
Verify you can see the old archived workspace chat
Verify the Report name says its UserC archived chat

Offline tests

No specific Offline tests

QA Steps

Verify that no errors appear in the JS console

Same as tests. @trjExpensify would help with the QA here.

PR Author Checklist

Screenshots/Videos

Web

As an admin:

As a new account

Mobile Web - Chrome

As admin

As member

Mobile Web - Safari

As an admin

As a member

Desktop

As an admin

As a member

iOS

There are some native iOS build issues which prevent me from testing. This change is platform agnostic however so we can continue without these

Android

There are some native build issues which prevent me from testing. This change is platform agnostic however so we can continue without these

mountiny · 2023-02-07T00:18:17Z

src/libs/ReportActionsUtils.js

@@ -7,7 +7,6 @@ import moment from 'moment';
 import * as CollectionUtils from './CollectionUtils';
 import CONST from '../CONST';
 import ONYXKEYS from '../ONYXKEYS';
-import * as ReportUtils from './ReportUtils';


This is to resolve a cyclic dependency between these two utility libraries

mountiny · 2023-02-07T00:19:13Z

src/libs/ReportUtils.js

@@ -70,7 +71,7 @@ let doesDomainHaveApprovedAccountant;
 Onyx.connect({
    key: ONYXKEYS.ACCOUNT,
    waitForCollectionCallback: true,
-    callback: val => doesDomainHaveApprovedAccountant = val.doesDomainHaveApprovedAccountant,
+    callback: val => doesDomainHaveApprovedAccountant = val.doesDomainHaveApprovedAccountant || false,


I have been running to this value not being defined locally and causing the app not to load so casting this to false in case val is not defined helped. Should be no harm in this change

mountiny · 2023-02-07T00:21:03Z

src/pages/home/report/ReportActionItemSingle.js

@@ -51,13 +52,14 @@ const showUserDetails = (email) => {
 };

 const ReportActionItemSingle = (props) => {
+    const actorEmail = props.action.actorEmail.replace(CONST.REGEX.MERGED_ACCOUNT_PREFIX, "");


the merged accounts will have MERGED_<number>@ prefix on the actorEmail so lets remove that so we can load the details correctly

MelvinBot · 2023-02-07T00:24:07Z

@thesahindia @chiragsalian 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]

mountiny · 2023-02-07T00:26:08Z

@thesahindia i am not sure if this flow is easy to test for you, maybe not. At the moment its on hold too.

src/libs/ReportUtils.js

Co-authored-by: Chirag Chandrakant Salian <[email protected]>

chiragsalian · 2023-02-07T19:25:36Z

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

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

chiragsalian

Code LGTM. Your test step 15 says - "Verify the Report name says its UserC' workspace chat"
but the report name for me is not userc workspace chat, the report name is UserC (archived) for me.

I see the same in your screenshot too so maybe the test step is a bit confusing.

mountiny · 2023-02-08T15:56:08Z

@chiragsalian thanks for catching that, that was a mistake in the test steps indeed.

Thanks for review and testing!

OSBotify · 2023-02-08T16:00:36Z

✋ 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-08T16:30:03Z

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries

Name	Duration
App start TTI	726.828 ms → 754.244 ms (+27.416 ms, +3.8%)
App start regularAppStart	0.015 ms → 0.016 ms (+0.001 ms, +6.7%)
App start nativeLaunch	20.194 ms → 20.067 ms (-0.127 ms, -0.6%)
Open Search Page TTI	621.031 ms → 616.817 ms (-4.214 ms, -0.7%)
App start runJsBundle	204.219 ms → 198.821 ms (-5.397 ms, -2.6%)

Show details

Name	Duration
App start TTI	Baseline Mean: 726.828 ms Stdev: 35.939 ms (4.9%) Runs: 658.2123850001954 676.5936730001122 678.7924339999445 679.1554359998554 690.0985869998112 692.0427549998276 694.604679999873 697.6640739999712 698.096429000143 704.1744820000604 707.1159219997935 711.2164219999686 713.8737220000476 714.0971929999068 716.0404849997722 717.2775960001163 725.813955000136 729.8287450000644 733.0916360002011 733.756816000212 737.5490500000305 738.3026290000416 750.6332749999128 751.6864299997687 756.7684240001254 767.5273710000329 769.2185599999502 773.7291120002046 776.2977640000172 776.9169399999082 778.9212050000206 809.3827780000865 Current Mean: 754.244 ms Stdev: 26.211 ms (3.5%) Runs: 702.3576790001243 714.0174160003662 721.0313699999824 721.5817299997434 723.9298679996282 728.4901569997892 728.8631640002131 730.2010019998997 732.978582999669 736.8924759998918 744.0393829997629 744.5088740000501 748.211462999694 752.5015669995919 753.5671439999714 755.0405379999429 757.2135579995811 758.5323360003531 761.9906080001965 763.5267479997128 765.7997059999034 766.4066180000082 768.4807120002806 771.5197320003062 771.8138420004398 781.0512589998543 785.7610210003331 787.7654940001667 790.0265800002962 798.6407080003992 814.8146090004593
App start regularAppStart	Baseline Mean: 0.015 ms Stdev: 0.001 ms (6.7%) Runs: 0.013549999799579382 0.013632000423967838 0.013874999713152647 0.014159999787807465 0.014281999785453081 0.014282000251114368 0.014444999862462282 0.01444500032812357 0.014607000164687634 0.014851000159978867 0.014933999627828598 0.014973999932408333 0.014973999932408333 0.014973999932408333 0.014973999932408333 0.015056000091135502 0.015381000004708767 0.015381000004708767 0.015381000004708767 0.015422000084072351 0.015583999920636415 0.015666000079363585 0.015666000079363585 0.015910000074654818 0.016031000297516584 0.016112999990582466 0.01643899967893958 0.01660200022161007 0.01713100029155612 0.017455999739468098 0.017741000279784203 Current Mean: 0.016 ms Stdev: 0.001 ms (6.1%) Runs: 0.014201000332832336 0.014322999864816666 0.01444500032812357 0.015259000472724438 0.015259000472724438 0.015300000086426735 0.015461999922990799 0.015705999918282032 0.015747000463306904 0.015868999995291233 0.015990999527275562 0.016154000535607338 0.016195000149309635 0.016358000226318836 0.016398999840021133 0.016479000449180603 0.016479000449180603 0.0165200000628829 0.01664199959486723 0.016642999835312366 0.016805000603199005 0.016885999590158463 0.016886000521481037 0.017008000053465366 0.0170499999076128 0.017130999825894833 0.0172520000487566 0.0174150001257658 0.0176189998164773 0.01769999973475933 0.018554000183939934
App start nativeLaunch	Baseline Mean: 20.194 ms Stdev: 2.206 ms (10.9%) Runs: 17 17 18 18 18 18 18 18 18 19 19 19 19 19 20 20 20 20 21 21 21 21 21 22 22 22 23 23 24 25 25 Current Mean: 20.067 ms Stdev: 2.015 ms (10.0%) Runs: 18 18 18 18 18 18 18 19 19 19 19 19 19 19 19 20 20 20 20 20 21 21 21 21 22 22 22 23 25 26
Open Search Page TTI	Baseline Mean: 621.031 ms Stdev: 24.392 ms (3.9%) Runs: 589.9401040002704 591.5053300000727 591.5987149998546 595.668864000123 597.509237000253 598.4250090001151 599.1718350006267 599.3907059999183 600.5254319999367 600.6760660000145 603.9876310001127 611.8271079999395 612.7338060000911 613.1569419996813 614.7669679997489 615.8184819999151 616.2693290002644 622.3262950000353 625.4632160002366 626.3381750001572 626.5071209999733 629.2371420003474 631.7105709998868 631.8006189996377 635.6918540000916 636.4898680001497 640.4843350001611 640.8234049999155 644.897501999978 650.8403320000507 684.5398769997992 692.8549810005352 Current Mean: 616.817 ms Stdev: 20.772 ms (3.4%) Runs: 585.7672119997442 586.8811850007623 595.2187099996954 595.7302250005305 596.2812510002404 596.6669110003859 600.628987999633 600.947592000477 601.0626639993861 605.7521979995072 605.7685960000381 606.1525879995897 606.7213549995795 607.8879800001159 608.415648999624 609.6753739994019 611.988404000178 613.6121009998024 615.5475260000676 619.3532710000873 620.4672039998695 620.6109620006755 622.3496099999174 624.3233239995316 625.0341389998794 628.3010660000145 635.2456459999084 638.2788899997249 638.6785080004483 643.2104899995029 643.8374429997057 671.0672610001639 673.4937749998644
App start runJsBundle	Baseline Mean: 204.219 ms Stdev: 20.174 ms (9.9%) Runs: 171 172 174 177 180 183 183 185 187 189 195 195 196 201 201 201 204 205 211 215 218 218 219 219 220 222 225 228 229 229 238 245 Current Mean: 198.821 ms Stdev: 9.809 ms (4.9%) Runs: 179 183 188 188 188 188 193 194 194 195 196 196 198 198 198 200 200 201 203 203 204 206 206 208 210 211 213 226

OSBotify · 2023-02-09T00:30:49Z

🚀 Deployed to staging by https://github.com/mountiny in version: 1.2.68-0 🚀

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

trjExpensify · 2023-02-09T19:22:40Z

Quick update: Actively working on the internalQA of this at the moment.

trjExpensify · 2023-02-09T20:00:18Z

I executed this with two scenarios:

two members are on the policy at the time of merging accounts
one member has been removed from the policy and one member remains on the policy at the time of merging accounts

Scenario #1 is what's laid out in the OP (though I merged userB into userC). The only bug I caught there was that the MERGED_0@ prefix in the email is visible from both the admin and the merged accounts view on chat messages from the old account:

For good measure, I then deleted the workspace and the archiveReason updated for all to note that the workspace has been deleted. 👍

Scenario #2, I executed this full test script for workspace chats. I ran into a couple of additional bugs in this case:

archivedReason did not update to note that userB has merged accounts with userC, it remained with the archiveReason being that userB is no longer a member of the policy (for everyone)

archivedReason did not update after the workspace was deleted either.

OSBotify · 2023-02-10T00:31:38Z

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

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

allroundexperts · 2024-01-21T22:51:21Z

src/pages/home/report/ReportActionItemSingle.js

@@ -51,13 +52,14 @@ const showUserDetails = (email) => {
 };

 const ReportActionItemSingle = (props) => {
+    const actorEmail = props.action.actorEmail.replace(CONST.REGEX.MERGED_ACCOUNT_PREFIX, '');


Not directly related, but we should have created a utility function that did this replacement. Later on, at some places we forgot to do this replacement which caused #32127.

Check if policy room is archived because of merging

07fc435

mountiny self-assigned this Feb 3, 2023

mountiny added 4 commits February 3, 2023 09:17

import CollectionUtils

46c0c44

Merge branch 'main' into vit-fixAccountMergingGimmick

80d264f

Remove unused global

5e06a1f

Merge branch 'main' into vit-fixAccountMergingGimmick

6036b74

mountiny changed the title ~~Check if policy room is archived because of merging~~ [HOLD Auth#7495] Check if policy room is archived because of merging Feb 6, 2023

mountiny added 6 commits February 6, 2023 22:30

Refactor and clean up

aec314f

Clean up

ba64ef9

Resolve dependency cycle

680608c

Resolve the dependency cycle

829ac76

Return the last visible message directly

210068e

Check for the Merged prefix and remove it from the email if needed

81cd5e9

mountiny commented Feb 7, 2023

View reviewed changes

Polish a comment

6b541f1

mountiny marked this pull request as ready for review February 7, 2023 00:23

mountiny requested a review from a team as a code owner February 7, 2023 00:23

melvin-bot bot requested review from chiragsalian and thesahindia and removed request for a team February 7, 2023 00:24

mountiny added the InternalQA This pull request required internal QA label Feb 7, 2023

mountiny added 2 commits February 7, 2023 00:26

Fix style

9f305a8

Merge main to resolve merge conflict

2f2c0c2

chiragsalian requested changes Feb 7, 2023

View reviewed changes

src/libs/ReportUtils.js Outdated Show resolved Hide resolved

src/libs/ReportUtils.js Outdated Show resolved Hide resolved

mountiny and others added 2 commits February 7, 2023 18:58

Update src/libs/ReportUtils.js

9482f61

Co-authored-by: Chirag Chandrakant Salian <[email protected]>

simplify the logic

96753fb

chiragsalian approved these changes Feb 7, 2023

View reviewed changes

mountiny changed the title ~~[HOLD Auth#7495] Check if policy room is archived because of merging~~ Check if policy room is archived because of merging Feb 8, 2023

mountiny merged commit 363c2fd into main Feb 8, 2023

mountiny deleted the vit-fixAccountMergingGimmick branch February 8, 2023 15:56

OSBotify mentioned this pull request Feb 8, 2023

Deploy Checklist: New Expensify 2023-02-08 #14977

Closed

6 tasks

mountiny assigned trjExpensify Feb 8, 2023

mountiny mentioned this pull request Feb 9, 2023

Remove the merged prefix from display name of merged accounts #14997

Closed

54 tasks

MelvinBot mentioned this pull request Feb 10, 2023

Incorrect workspace chat name after merging accounts #14292

Closed

6 tasks

allroundexperts reviewed Jan 21, 2024

View reviewed changes

allroundexperts mentioned this pull request Jan 21, 2024

[$500] Workspace - Incorrect primary login displayed after merge with other account #32127

Closed

6 tasks

getusha mentioned this pull request Jun 5, 2024

[HOLD for payment 2024-06-05] [$250] Merge account - Email for merged account is prefixed with 0@ in archived workspace chat #40977

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check if policy room is archived because of merging #14779

Check if policy room is archived because of merging #14779

mountiny commented Feb 3, 2023 •

edited

Loading

mountiny Feb 7, 2023

mountiny Feb 7, 2023

mountiny Feb 7, 2023

MelvinBot commented Feb 7, 2023

mountiny commented Feb 7, 2023

chiragsalian commented Feb 7, 2023 •

edited

Loading

chiragsalian left a comment

mountiny commented Feb 8, 2023 •

edited

Loading

OSBotify commented Feb 8, 2023

github-actions bot commented Feb 8, 2023

OSBotify commented Feb 9, 2023

trjExpensify commented Feb 9, 2023

trjExpensify commented Feb 9, 2023

OSBotify commented Feb 10, 2023

allroundexperts Jan 21, 2024

Check if policy room is archived because of merging #14779

Check if policy room is archived because of merging #14779

Conversation

mountiny commented Feb 3, 2023 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

mountiny Feb 7, 2023

Choose a reason for hiding this comment

mountiny Feb 7, 2023

Choose a reason for hiding this comment

mountiny Feb 7, 2023

Choose a reason for hiding this comment

MelvinBot commented Feb 7, 2023

mountiny commented Feb 7, 2023

chiragsalian commented Feb 7, 2023 • edited Loading

Reviewer Checklist

Screenshots/Videos

chiragsalian left a comment

Choose a reason for hiding this comment

mountiny commented Feb 8, 2023 • edited Loading

OSBotify commented Feb 8, 2023

github-actions bot commented Feb 8, 2023

Performance Comparison Report 📊

Significant Changes To Duration

Meaningless Changes To Duration

OSBotify commented Feb 9, 2023

trjExpensify commented Feb 9, 2023

trjExpensify commented Feb 9, 2023

OSBotify commented Feb 10, 2023

allroundexperts Jan 21, 2024

Choose a reason for hiding this comment

mountiny commented Feb 3, 2023 •

edited

Loading

chiragsalian commented Feb 7, 2023 •

edited

Loading

mountiny commented Feb 8, 2023 •

edited

Loading