Make sure closed accounts don't show in workspace members

[puneetlath]

Details

Before this change, if you:

1. invite an employee to a workspace
2. that employee closes their account
3. you go back to the workspace members page

The closed account would still show up as a workspace member. Now, they get removed from the workspace members list. This is an update the logic implemented here (cc @s77rt)

Fixed Issues

$ #15267

Tests

1. In one window, log into account 1 (e.g. admin@test.com)
2. Create a workspace if you don't already have one
3. Invite a user to the workspace (e.g. employee@test.com)
4. Close the right hand modal
5. In another window, log into employee@ and close the account
6. In the admin@ account, go back to the workspace members page
7. The employee whose account was closed should no longer appear as a workspace member (it may take a second to update)

• Verify that no errors appear in the JS console

Offline tests

N/A. This requires interacting with the API, so no special offline tests.

QA Steps

1. In one window, log into account 1 (e.g. admin@test.com)
2. Create a workspace if you don't already have one
3. Invite a user to the workspace (e.g. employee@test.com)
4. Close the right hand modal
5. In another window, log into employee@ and close the account
6. In the admin@ account, go back to the workspace members page
7. The employee whose account was closed should no longer appear as a workspace member (it may take a second to update)

• Verify that no errors appear in the JS console

PR Author Checklist

• I linked the correct issue in the ### Fixed Issues section above
• I wrote clear testing steps that cover the changes made in this PR
  • I added steps for local testing in the Tests section
  • I added steps for the expected offline behavior in the Offline steps section
  • I added steps for Staging and/or Production testing in the QA steps section
  • I added steps to cover 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 tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
• I included screenshots or videos for tests on all platforms
• I ran the tests on all platforms & verified they passed on:
  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop
• I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
• I followed proper code patterns (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
    • If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
  • 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 followed the guidelines as stated in the Review Guidelines
• I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
• 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
• I verified that if a function's arguments changed that all usages have also been updated correctly
• 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 author checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

Screen.Recording.2023-02-21.at.2.14.33.PM.mov

Mobile Web - Chrome

android.chrome.mov

Mobile Web - Safari

ios.mov

Desktop

Screen.Recording.2023-02-21.at.2.19.09.PM.mov

iOS

ios.mov

Android

android.mov

[s77rt]

Thanks for fixing this, we didn't have a valid use case for the use of errors field back then so we opted to filter based on the role field instead.
As per using the errors field, I'm not sure if it's bulletproof, what if we are adding a user where the role is not set yet and have pendingAction? It will be overwritten and cause flickering effect

Usually on these cases I would go for:

1. Errors are empty
2. Pending action is empty
3. Not offline (not sure about this condition, as if we are offline the request will be queued anyway)

[puneetlath]

Pending action is empty

What's the scenario where there's a pending action and we wouldn't prefer to overwrite with the API response?

Not offline (not sure about this condition, as if we are offline the request will be queued anyway)

Same question here. I can't see this being a problem since the API call won't happen until the user comes online

[s77rt]

1. Add member (pendingAction is add, request is still ongoing)
2. Open members page again (call openWorkspaceMembersPage)
3. Member will be removed since request (on 1) is still ongoing
4. Request is done, member added again

Writing this I remembered something, even it seems as a valid scenario it actually will never happen since we queue all read requests to be executed after write requests are done.

[puneetlath]

Yes and also, when would we ever clear the pendingAction if we didn't include those users in this call?

[MelvinBot]

@Santhosh-Sellavel @MonilBhavsar 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]

[s77rt]

when would we ever clear the pendingAction if we didn't include those users in this call?

On the request response (or success/failure data), but if we fail to get that, members will be stuck on onyx. Using the errors field seems the way to go as it fixes original issue and this one.

[puneetlath]

Sweet, thanks for helping validate that!

[Santhosh-Sellavel]

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.

Screenshots/Videos

All Devices

Added Account

Added_State.mov

Closed Account

Close_Account_Test.mov

[Santhosh-Sellavel]

@puneetlath After performing a close account, the account is not removed automatically from the page even after waiting some time. It's only removed after closing and opening the Manage Members page. Is this fine?

Check the Closed Account video in the Review Checklist for more!

[Santhosh-Sellavel]

Mostly Looks good just one doubt!

[puneetlath]

Yes, that is because the back-end doesn't currently send a pusher update when the account is closed. So until you come back to the page, which then calls the API to get the full members list, the page won't update.

I'm going to look into a doing a back-end pusher update as well, but that's separate from this PR. Thanks for testing!

[Santhosh-Sellavel]

So we are good now then, thanks!

[puneetlath]

yep! over to you @MonilBhavsar

[Santhosh-Sellavel]

LGTM!

[puneetlath]

@MonilBhavsar bump!

[MonilBhavsar]

sorry, was ooo

[OSBotify]

✋ 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]

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries

Name	Duration
App start TTI	709.100 ms → 730.273 ms (+21.173 ms, +3.0%)
App start runJsBundle	197.032 ms → 204.188 ms (+7.155 ms, +3.6%)
App start regularAppStart	0.014 ms → 0.015 ms (+0.001 ms, +6.1%)
App start nativeLaunch	19.968 ms → 19.968 ms
Open Search Page TTI	614.596 ms → 608.029 ms (-6.567 ms, -1.1%)

Show details

Name	Duration
App start TTI	Baseline Mean: 709.100 ms Stdev: 30.072 ms (4.2%) Runs: 660.7054359996691 661.8844839995727 661.9909770004451 671.7697339998558 683.9898699996993 685.8601479995996 689.8617009995505 690.6438880003989 693.904210999608 695.3767609996721 695.9630330000073 697.2385910004377 700.2552370000631 700.8913620002568 701.9430980002508 702.0425540003926 706.4564559999853 710.4086889997125 710.4231310002506 710.7415349995717 719.9321849998087 723.1362439999357 725.723063999787 730.396759999916 742.282247999683 744.379639999941 748.7882650000975 751.1952579999343 776.1978129995987 778.61323000025 Current Mean: 730.273 ms Stdev: 31.764 ms (4.3%) Runs: 675.1364559996873 686.133942999877 694.0297600002959 694.8922760002315 700.3639590004459 701.649675000459 701.6819049995393 702.7556220004335 709.9549439996481 710.517664000392 713.1296760002151 713.6071760002524 717.7104630004615 720.2881640000269 724.0930300001055 724.5551439998671 726.9102560002357 727.0325499996543 727.1399180004373 727.441863999702 735.4279680000618 739.8064299998805 750.0734050003812 750.5878119999543 757.3617300000042 759.879966000095 761.630289000459 764.9320090003312 765.1261449996382 767.547040999867 801.9517890000716 815.377368000336
App start runJsBundle	Baseline Mean: 197.032 ms Stdev: 24.915 ms (12.6%) Runs: 157 164 167 168 171 173 174 178 179 183 185 186 186 187 189 192 196 198 198 200 206 207 209 217 219 222 224 234 237 241 261 Current Mean: 204.188 ms Stdev: 22.672 ms (11.1%) Runs: 173 173 173 177 178 178 179 180 187 189 190 194 195 198 198 199 206 208 209 212 213 216 217 217 218 219 229 235 236 237 241 260
App start regularAppStart	Baseline Mean: 0.014 ms Stdev: 0.001 ms (5.8%) Runs: 0.011881999671459198 0.012409999966621399 0.012776999734342098 0.012817000038921833 0.01285799965262413 0.012938999570906162 0.013061000034213066 0.013101999647915363 0.013143000192940235 0.013143000192940235 0.013183999806642532 0.01338799949735403 0.013426999561488628 0.013426999561488628 0.013590999878942966 0.013753000646829605 0.01387499924749136 0.013875000178813934 0.013916000723838806 0.013957000337541103 0.0139979999512434 0.014078999869525433 0.014079000800848007 0.014118999242782593 0.01411999948322773 0.014201000332832336 0.014322999864816666 0.014649000018835068 0.015176999382674694 0.015217999927699566 0.015300000086426735 Current Mean: 0.015 ms Stdev: 0.001 ms (5.3%) Runs: 0.013062000274658203 0.013427999801933765 0.013468999415636063 0.013630999252200127 0.013712000101804733 0.013753999955952168 0.013794000260531902 0.013875000178813934 0.014078999869525433 0.014119000174105167 0.014119000174105167 0.014159999787807465 0.014159999787807465 0.014241999946534634 0.014281999319791794 0.014322999864816666 0.014403999783098698 0.014405000023543835 0.014649000018835068 0.014728999696671963 0.01476999931037426 0.014810999855399132 0.014851000159978867 0.014892000705003738 0.014932999387383461 0.015096000395715237 0.015135999768972397 0.015421999618411064 0.015625 0.015747000463306904 0.016112999990582466 0.016316999681293964
App start nativeLaunch	Baseline Mean: 19.968 ms Stdev: 2.293 ms (11.5%) Runs: 17 18 18 18 18 18 18 18 18 18 18 19 19 19 19 19 19 19 19 20 21 21 22 22 22 22 23 23 23 25 26 Current Mean: 19.968 ms Stdev: 2.132 ms (10.7%) Runs: 17 17 18 18 18 18 18 19 19 19 19 19 19 19 19 19 19 20 20 20 20 20 20 21 22 22 23 23 24 25 25
Open Search Page TTI	Baseline Mean: 614.596 ms Stdev: 27.796 ms (4.5%) Runs: 578.5076099997386 582.5236010001972 583.3530279994011 583.9928789995611 584.4923099996522 584.8234870005399 590.5298669999465 591.9637050004676 593.4019769998267 594.1313070002943 595.5913500003517 597.1639809999615 600.1677249995992 604.219076000154 604.8865149999037 609.8323570005596 613.100260999985 616.2082520006225 617.2978509999812 618.906739000231 620.2533369995654 620.3675950001925 622.5219330005348 622.5799160003662 625.325032999739 630.265788000077 632.4490149999037 632.6490080002695 633.7196039995179 655.0711269993335 670.20080600027 685.3391120005399 685.8301189998165 Current Mean: 608.029 ms Stdev: 23.036 ms (3.8%) Runs: 566.1128340000287 571.1520189996809 580.703003000468 582.3876950005069 585.4930020002648 586.0388179998845 586.6693529998884 588.2158199995756 590.3350840006024 594.4980880003422 594.875284999609 595.9556480003521 596.857219000347 600.8832609998062 602.2615970000625 607.1772059993818 608.7119140001014 611.7959390003234 612.2531329998747 613.0261639999226 613.5629480006173 614.8771970001981 615.6251229997724 617.1387529997155 617.6142579996958 622.2181400004774 625.0567229995504 625.4621990006417 628.5883790003136 632.2393800001591 652.8586430000141 655.6262209992856 668.6872960003093

[OSBotify]

🚀 Deployed to staging by https://github.com/MonilBhavsar in version: 1.2.77-0 🚀

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

[OSBotify]

🚀 Deployed to staging by https://github.com/MonilBhavsar in version: 1.2.77-0 🚀

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

[OSBotify]

🚀 Deployed to production by https://github.com/yuwenmemon in version: 1.2.77-4 🚀

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