Don't allow removing the policy owner from workspace members

[pecanoro]

Details

Disable the checkbox and shows a tooltip if the admin tries to remove the owner or themselves from the workspace.

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/179467

Tests / QA

1. In an account that has an existing workspace (or you can create a new one), click on People.
2. Add a couple of extras users to the workspace.
3. Make sure you can select all users to be removed except for the owner of the workspace and yourself.
4. If you hover on web or desktop on the owner or yourself, you should get the tooltip shown in the screenshots.
5. If you test this on a mobile device, a growl should show up instead (as shown in the screenshots)
6. Play with the Select All button and make sure it still works, it should select all users except for the above.
7. Test that removing the selected users work as expected and they get removed.

Tested On

• Web
• Mobile Web
• Desktop
• iOS
• Android

Screenshots

Web

Mobile Web

Desktop

iOS

Android

[Julesssss]

Reminder for us to test this on iPad, as I'm not sure whether isSmallScreenWidth will include large iPads.

[pecanoro]

@tgolen I moved it to a component like you mentioned, but I encountered the following roadblocks and maybe it's just that I am not super strong in React Native:

1. Before I moved the checkbox and the tooltip to a component, I was able to use the TouchableOpacity to trigger the tooltips and growls wherever the user clicked in the row, but I can't do that anymore and I am not sure how to do it since it's a higher level than the new component.
2. I have no idea how to show the growl in mobile web browsers, I don't there is a specific file like native.js for that. If I add it to index.js, I would end up just doing what I was doing before, checking for the dimensions and I would be replicating the logic in native.js but in that file, not great for DRY.

Any advice on how I could solve these problems?

[tgolen]

OK, let's see if I can help here. For the first problem, I would start with changing the onpress handler to be more like this:

- onPress={canBeRemoved ? () => this.toggleUser(item.login) : () => {}}
+ onPress={() => this.toggleUser(item.login)}

Then update toggleuser to be more like this:

    toggleUser(login) {
        const canBeRemoved = this.props.policy.owner !== item.login && this.props.session.email !== item.login;
        if (!canBeRemoved) {
            this.setState({showTooltipForLogin: login});
            return;
        }
        // Add or remove the user if the checkbox is enabled and is clickable.
        if (_.contains(this.state.selectedEmployees, login)) {
            this.removeUser(login);
        } else {
            this.addUser(login);
        }
    }

Then finally update the checkbox component:

- toggleTooltip={!canBeRemoved}
+ toggleTooltip={this.state.showTooltipForLogin === login}

Would that work?

For your second issue, I don't think I am understanding the problem. There shouldn't be anything specific to mobile web for showing a growl and you can just use Growl.show(). Are you saying it's broken for mobile web but works on all other platforms? If so, is it that way in main too? Maybe it's not something specific to your PR.

[pecanoro]

There shouldn't be anything specific to mobile web for showing a growl and you can just use Growl.show(). Are you saying it's broken for mobile web but works on all other platforms? If so, is it that way in main too? Maybe it's not something specific to your PR.

@tgolen Let me explain again, so mobile browsers on phones use index.js and not index.native.js, so it will try to show a tooltip as well, but it won't work. The only way to do this (I think) would be to check for the dimensions. But then I feel I would be repeating most of the code from index.js to index.native.js

[tgolen]

Oh, got it. Yeah, that mobile web issue isn't great. It would be pretty cool to have an index.mobileweb.js ability, but it's a little trickier than that. I think what you'll probably need to do is update index.js to use dimensions like you said, and then on a small screen, just use the index.native.js component (you'll have to import it specifically from index.native). I think that will be OK and should keep the code DRY?

[pecanoro]

Ok! Updated!! Everything is working except for a small bug on Web that I am not sure how to fix, I tried multiple things but nothing works. We only show the tooltip depending on this.state.showTooltipForLogin, but the state is only updated if the user clicks on the row, meaning that we don't show the tooltip on hover unless the user clicks on it before. I am not sure how to bypass this without breaking the mobile version of it. Any ideas?

[tgolen]

Hm, no... I don't have any idea about that. Could you post that in the open source channel on slack and see if someone else might know?

[tgolen]

Those changes to the component look great!

[pecanoro]

I fixed the bug!! I had a revelation this morning to use hoverable to fix the tooltip so it would work not only on press! Mobiles devices don't have hoverables, so win-win. Ready for another review, I replied to your two remaining comments.

[pecanoro]

Updated!

[Julesssss]

It's working well on all platforms, but I noticed that we're showing the Growl on hover when the website window is 'small'. I think this is not expected, right? In this case, shouldn't we just display the growl on touch? This doesn't seem to occur on Desktop.

WebHover.mov

[pecanoro]

That's a consequence of not being able to tell between a mobile browser from a computer browser, I couldn't find any other solution and I tried a lot of things, but if I remove the hoverable, then the tooltip stops working on normal web browsers and desktop.

[pecanoro]

@Julesssss had an idea to fix the weird bug and it worked!! So updated again with the small changes

[Julesssss]

👍

[pecanoro]

Can we merge this? It was a n6 polish issue so I am assuming yes?

[pecanoro]

I double-checked on Slack, n6 polish issues are ok to merge so self-merging since everyone approved.

[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.

[OSBotify]

🚀 Deployed to staging by @pecanoro in version: 1.1.7-25 🚀

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

[OSBotify]

🚀 Deployed to production by @roryabraham in version: 1.1.8-9 🚀

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