Fix Invite member checkbox selection behavior

[luacmartins]

Details

Allows users to select every member of a workspace and displays an inline error if they select themselves of the Workspace owner.

Fixed Issues

$ #13769

Tests

1. Create a Workspace + > New Workspace
2. Invite a few users to it Settings > Workspace > Manage members > Invite
3. In the Manage members page, use the Select all checkbox to select all users
4. Verify that all users, except for yourself and the workspace owner are selected
5. Verify that you can manually select yourself or the workspace owner and that you see an error
6. Verify that tapping the Remove button doesn't work if you have an error
7. Unselect yourself
8. Verify that you can remove the other selected members

• Verify that no errors appear in the JS console

Offline tests

N/A

QA Steps

Same as tests

• 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.
• I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

web.mov

Mobile Web - Chrome

chrome.mov

Mobile Web - Safari

safari.mov

Desktop

desktop.mov

iOS

ios.mov

Android

android.mov

[melvin-bot]

@sketchydroide @rushatgabhane 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]

[rushatgabhane]

So for consistency, I think that we should allow the user to select any checkbox they want, but show an error when they try to submit it. That will remove the growl and UI/UX inconsistencies.

I agree with this solution for the problem because it resonates with allowing a user to input anything (just like form).

[rushatgabhane]

Possibly non-blocking

Problem: A user that cannot be removed is selected. When the error message is off-screen, or we scroll away from the error message.
On clicking remove nothing happens and there is no feedback on what's wrong.

Solution: scroll to the first error message on clicking remove.

screen-20230119-031936.mp4

[rushatgabhane]

Bug: Pressing select all unchecks non-removable users.

Steps:

1. Select yourself
2. Press select all

Expected: All users including yourself are checked
Actual: All users excluding yourself are checked

screen-20230119-033138.mp4

[luacmartins]

@rushatgabhane when I changed the logic for the Select all button, I actually thought of that behavior more as a feature than a bug. IMO the Select all button should still just select all removable members (and leave non-removable members unselected). Pressing it while we have non-removable members selected also de-selects those members which fixes errors for the user.

We can discuss this behavior in Slack though!

[luacmartins]

Asked in Slack

[luacmartins]

@rushatgabhane from the Slack thread, it seems like we will keep the current behavior, i.e. Select all should deselect non-removable members as well and fix the error for the user. So the above is not a bug, but the expected behavior.

[luacmartins]

@rushatgabhane's comment was addressed above. This is ready for review!

[rushatgabhane]

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-20.at.11.20.06.PM.mov

Mobile Web - Chrome

WhatsApp.Video.2023-01-20.at.23.22.58.mp4

Mobile Web - Safari

Screen.Recording.2023-01-20.at.11.23.18.PM.mov

Desktop

Screen.Recording.2023-01-20.at.11.26.25.PM.mov

iOS

Screen.Recording.2023-01-20.at.11.30.20.PM.mov

Android

WhatsApp.Video.2023-01-20.at.23.31.57.mp4

[rushatgabhane]

@sketchydroide LGTM!

#14347 (review)

[rushatgabhane]

So the above is not a bug, but the expected behavior.

Thanks for confirming @luacmartins

[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	690.869 ms → 699.453 ms (+8.585 ms, +1.2%)
Open Search Page TTI	607.523 ms → 608.678 ms (+1.155 ms, ±0.0%)
App start runJsBundle	196.613 ms → 197.625 ms (+1.012 ms, +0.5%)
App start regularAppStart	0.021 ms → 0.015 ms (-0.006 ms, -28.5%) 🟢
App start nativeLaunch	20.333 ms → 20.281 ms (-0.052 ms, ±0.0%)

Show details

Name	Duration
App start TTI	Baseline Mean: 690.869 ms Stdev: 29.522 ms (4.3%) Runs: 635.2817299999297 638.9912730000215 643.3533369998913 650.3096529999748 650.7924770000391 655.5727669999469 670.247689000098 670.2904610000551 671.4651560001075 672.126086999895 674.4335739999078 682.3604840000626 683.313950000098 689.5313069999684 691.7631739999633 693.7668800000101 694.2675360001158 695.35848299996 696.6899939998984 705.2861379999667 706.3816549999174 711.6630810000934 714.9444450000301 717.9339030000847 720.3082910000812 722.1657600000035 726.5726580000482 728.651996999979 731.699006000068 735.0686510000378 736.335020000115 Current Mean: 699.453 ms Stdev: 43.178 ms (6.2%) Runs: 629.54091900005 645.084186000051 648.2185480000917 648.4540669999551 649.1249120000284 653.4879280000459 669.4644949999638 671.452646000078 671.5783180000726 673.1511750000063 682.532036999939 683.1141399999615 683.7751949999947 686.0070279999636 689.5544300000183 690.7170329999644 691.8172850001138 692.9011909998953 693.5641240000259 694.7286209999584 700.2821889999323 709.6216029999778 719.7624870000873 724.3367409999482 726.8011060000863 727.2118649999611 729.1371039999649 751.5358299999498 766.4757739999332 776.5850579999387 795.5230239999946 806.9694300000556
Open Search Page TTI	Baseline Mean: 607.523 ms Stdev: 25.376 ms (4.2%) Runs: 572.7152509998996 573.5843510001432 575.0385330000427 577.4955240001436 579.3475340001751 583.4147540000267 583.5160729999188 586.1807049999479 588.8463540000375 589.5588790001348 593.5635179998353 593.5978190000169 595.9767249999568 597.2777509998996 597.2943520001136 597.6359049999155 597.644125000108 608.8845220000949 609.168008999899 610.364135999931 613.1656910001766 620.6225590000395 621.1937259999104 624.1593019999564 625.6417239999864 631.4543059999123 639.0970059998799 641.3813889999874 644.9012050000019 647.6060799998231 658.9264739998616 661.475626999978 Current Mean: 608.678 ms Stdev: 25.037 ms (4.1%) Runs: 571.7552499999292 571.7595619999338 574.3596189999953 578.2134200001601 581.6168619999662 585.2422700000461 585.266317000147 590.7610679999925 591.1824950000737 594.039511000039 595.3495279999916 596.9188650001306 597.0337730001193 599.1270749999676 599.9895430000033 601.3839930000249 602.2039399999194 605.8827319999691 610.2574060000479 610.3354079998098 612.5041100000963 613.5907789999619 617.5799970000517 621.5651449998841 622.3636479999404 627.4508060000371 631.1977949999273 634.6840409999713 637.9302580000367 639.1903490000404 651.9034830001183 663.5398760000244 670.1964529999532
App start runJsBundle	Baseline Mean: 196.613 ms Stdev: 21.685 ms (11.0%) Runs: 159 162 166 167 168 172 173 177 183 183 184 185 194 195 198 200 201 203 207 208 208 210 211 215 215 216 217 221 221 228 248 Current Mean: 197.625 ms Stdev: 22.541 ms (11.4%) Runs: 161 165 171 173 176 178 179 179 183 183 185 185 186 186 191 194 195 196 197 202 202 204 208 210 212 217 218 227 232 233 238 258
App start regularAppStart	Baseline Mean: 0.021 ms Stdev: 0.002 ms (9.6%) Runs: 0.018148000119253993 0.018757999874651432 0.018798999954015017 0.018880000105127692 0.019165000063367188 0.01920600002631545 0.0194089999422431 0.019409000175073743 0.0197350000962615 0.01989799994044006 0.02010100008919835 0.020182000007480383 0.020223000086843967 0.020385999931022525 0.020426000002771616 0.020549000008031726 0.020629999926313758 0.020995999919250607 0.02099600015208125 0.021036999998614192 0.02140299999155104 0.021525000222027302 0.021564999828115106 0.021646999986842275 0.022176000056788325 0.02343699987977743 0.023681000107899308 0.024861000012606382 0.02510600001551211 0.026733000064268708 Current Mean: 0.015 ms Stdev: 0.001 ms (4.7%) Runs: 0.013794000027701259 0.01395700010471046 0.014119999948889017 0.01424099993892014 0.014283000025898218 0.014322999864816666 0.0143630001693964 0.014404000015929341 0.014404000015929341 0.014525999780744314 0.014770000008866191 0.014771000016480684 0.014811000088229775 0.014973999932408333 0.014973999932408333 0.015014000004157424 0.015054999850690365 0.01509599993005395 0.01509599993005395 0.015177000081166625 0.015177999855950475 0.015420999843627214 0.015461999922990799 0.015625 0.015625 0.01566500007174909 0.015705999918282032 0.015949999913573265 0.016235999995842576 0.016967999981716275
App start nativeLaunch	Baseline Mean: 20.333 ms Stdev: 2.181 ms (10.7%) Runs: 17 18 18 18 18 19 19 19 19 19 19 19 19 20 20 20 20 20 20 20 21 21 22 22 22 22 23 25 25 26 Current Mean: 20.281 ms Stdev: 1.972 ms (9.7%) Runs: 18 18 18 18 18 18 18 19 19 19 19 19 19 20 20 20 20 20 20 21 21 21 21 21 22 22 22 22 22 24 24 26

[OSBotify]

🚀 Deployed to staging by https://github.com/sketchydroide in version: 1.2.59-0 🚀

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

[OSBotify]

🚀 Deployed to production by https://github.com/luacmartins in version: 1.2.59-1 🚀

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

[s77rt]

This PR caused a regression here #17203. The style that was being passed to <CheckboxWithTooltip /> was being used in a wrapper View here. But after this PR when you switched to <Checkbox /> the style was getting applied directly to the Checkbox itself.

[sobitneupane]

In this issue #20719 , we decided to not allow users to select an admin from the members checklist.