[$250] Workspace - "Remove members" and "Make admin" options are present for owner

[m-natarajan]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Issue found when validating #45732
Version Number: 9.0.15-4
Reproducible in staging?: y
Reproducible in production?: unable to check new feature
Issue reported by: applause internal team

Action Performed:

1. Launch New Expensify app.
2. Go to workspace settings > Members.
3. Long press on the owner.
4. Tap Select.
5. Tap on the dropdown.

Expected Result:

Owner should not be able to be selected (production behavior).

Actual Result:

Owner can be selected and there are "Remove members" and "Make members" options which are not applicable to the owner.

Workaround:

unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0110a7f901ae8b6a28
• Upwork Job ID: 1818787780851878074
• Last Price Increase: 2024-07-31
• Automatic offers:
• neonbhai | Contributor | 103438189

Issue Owner

Current Issue Owner: @getusha

[melvin-bot]

Triggered auto assignment to @cead22 (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

[melvin-bot]

Triggered auto assignment to @bfitzexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[github-actions]

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[m-natarajan]

We think that this bug might be related to #wave-collect - Release 1

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0110a7f901ae8b6a28

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @getusha (External)

[neonbhai]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Workspace - "Remove members" and "Make admin" options are present for owner

What is the root cause of that problem?

We allow the select modal to be present for options which have selection disabled. This results in user being able to select rows for which selection has been disabled

What changes do you think we should make in order to solve the problem?

We should use isDisabled and isCheckBoxDisabled properties here when checking selection mode should be triggered:

App/src/components/SelectionListWithModal/index.tsx

Lines 44 to 45 in c46990b

	const handleLongPressRow = (item: TItem) => {
	if (!turnOnSelectionModeOnLongPress || !isSmallScreenWidth) {

if (!turnOnSelectionModeOnLongPress || !isSmallScreenWidth || item?.isDisabled || item?.isCheckBoxDisabled) {
    return;
}

This will also fix the issue in WorkspaceTaxesPage and ReportParticipantsPage which have the same issue present.

[daledah]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Owner can be selected and there are "Remove members" and "Make members" options which are not applicable to the owner.

What is the root cause of that problem?

We always call onLongPressRow?.(item) without checking whether that option can be selected or not:

App/src/components/SelectionList/BaseListItem.tsx

Lines 75 to 77 in 73f97c9

	onLongPress={() => {
	onLongPressRow?.(item);
	}}

What changes do you think we should make in order to solve the problem?

We should use:

                onLongPress={isDisabled || item.isDisabledCheckbox ? undefined : () => onLongPressRow?.(item)}

What alternative solutions did you explore? (Optional)

We can update the condition:

App/src/components/SelectionListWithModal/index.tsx

Line 45 in 73f97c9

if (!turnOnSelectionModeOnLongPress || !isSmallScreenWidth) {

to:

        if (!turnOnSelectionModeOnLongPress || !isSmallScreenWidth || item?.isDisabled || item.isSelected || item?.isDisabledCheckbox) {

[Tony-MK]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Workspace - "Remove members" and "Make admin" options are present for owner

What is the root cause of that problem?

The problem is that the WorkspaceMembersPage does not disabled the policy owner option.

Hence, the policy owner option can always be selected and the "Remove members" and "Make admin" options are present for the owner.

App/src/pages/workspace/WorkspaceMembersPage.tsx

Lines 348 to 350 in ec38b05

	isDisabled:
	!!details.isOptimisticPersonalDetail ||
	(isPolicyAdmin && (policyEmployee.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE || !isEmptyObject(policyEmployee.errors))),

What changes do you think we should make in order to solve the problem?

We should disable the owner option being able to be selected by adding the following condition, accountID === policy?.ownerAccountID, to check if the account owns the policy for isDisabled.

The full condition will look like this.

isDisabled:
    accountID === policy?.ownerAccountID
    !!details.isOptimisticPersonalDetail || 
    (isPolicyAdmin && (policyEmployee.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE || !isEmptyObject(policyEmployee.errors))), 
....

[getusha]

@neonbhai why do we need item?.isDisabled in the condition?

[getusha]

@neonbhai's proposal looks good to me, seems like a very straightforward issue. and we don't want to fully disable it.
🎀 👀 🎀 C+ Reviewed!

[melvin-bot]

Current assignee @cead22 is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[bfitzexpensify]

How is this one going @neonbhai?

[neonbhai]

PR is ready for review!

cc: @getusha

[melvin-bot]

This issue has not been updated in over 15 days. @cead22, @bfitzexpensify, @neonbhai, @getusha eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[getusha]

Seems like automation failed on this one. @cead22 could you please check? thanks!

[cead22]

@bfitzexpensify can you handle payment for this issue please? Automation failed here, but I think @neonbhai should be paid via https://www.upwork.com/jobs/~0110a7f901ae8b6a28, and I'm not sure about @getusha after a brief look at the code

Below is the bug zero checklist for reference, in case we need to complete it. Let me know if I tagged the wrong people on some of the items.

Thanks

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@getusha] The PR that introduced the bug has been identified. Link to the PR:
• [@getusha] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@getusha] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@getusha] Determine if we should create a regression test for this bug.
• [@getusha] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@bfitzexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[melvin-bot]

@cead22, @bfitzexpensify, @neonbhai, @getusha Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[cead22]

Bump @neonbhai @bfitzexpensify

[bfitzexpensify]

Back from OOO, handling this now

Payment summary:

@neonbhai to be paid $250 for contributor work - offer sent via Upwork
@getusha to be paid $250 for C+ work - via manual request

@getusha can you complete the checklist in #46620 (comment)? Thanks!

[melvin-bot]

@cead22, @bfitzexpensify, @neonbhai, @getusha Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[getusha]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

[@getusha] The PR that introduced the bug has been identified. Link to the PR: #46096
[@getusha] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: #46096 (comment)
[@getusha] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: N/a
[@getusha] Determine if we should create a regression test for this bug. No. IMO it's not related to an important flow and isn't impactful
[@getusha] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

[bfitzexpensify]

Thanks! Let's close this out

[garrettmknight]

$250 approved for @getusha