search

Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.32k stars 2.75k forks source link

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

Open m-natarajan opened 1 month ago

m-natarajan commented 1 month ago

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 https://github.com/Expensify/App/pull/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?

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 OwnerCurrent Issue Owner: @getusha
melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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[bot] commented 1 month ago

:wave: 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 commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

neonbhai commented 1 month ago

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:

https://github.com/Expensify/App/blob/c46990ba7e8e7cf21da72bce5ade7472f7cf7019/src/components/SelectionListWithModal/index.tsx#L44-L45

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 commented 1 month ago

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: https://github.com/Expensify/App/blob/73f97c930795b4c68512d0e25ee253f7ce7d4515/src/components/SelectionList/BaseListItem.tsx#L75-L77

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: https://github.com/Expensify/App/blob/73f97c930795b4c68512d0e25ee253f7ce7d4515/src/components/SelectionListWithModal/index.tsx#L45 to:

        if (!turnOnSelectionModeOnLongPress || !isSmallScreenWidth || item?.isDisabled || item.isSelected || item?.isDisabledCheckbox) {
Tony-MK commented 1 month ago

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.

https://github.com/Expensify/App/blob/ec38b0508184b9ef00c368c7c5f7e583edfb0bef/src/pages/workspace/WorkspaceMembersPage.tsx#L348-L350

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 commented 1 month ago

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

getusha commented 1 month ago

@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[bot] commented 1 month ago

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

daledah commented 1 month ago

@getusha Can you re-check the solution, it does not work. We don't have isCheckBoxDisabled

neonbhai commented 1 month ago

ah I meant to write isDisabledCheckbox. Solution stays same!

getusha commented 1 month ago

@getusha Can you re-check the https://github.com/Expensify/App/issues/46620#issuecomment-2261675223, it does not work. We don't have isCheckBoxDisabled

Yes, i figured it could be a typio

daledah commented 1 month ago

I don't believe this is just a typo, as the issue appears consistently throughout the entire proposal. Let's have @cead22 review and make a decision on this matter.

In my opinion, it would be more appropriate for other contributors if the selected proposal not only identifies the solution but also ensures it is correctly implemented, rather than rushing to comment (to get the first comment) without proper testing.

neonbhai commented 1 month ago

I think we should go for the proposals which proposes the idea first. This is noted in https://github.com/Expensify/App/issues/28535 and https://github.com/Expensify/App/issues/27592#issuecomment-1722295827

cead22 commented 1 month ago

I agree with @getusha and @neonbhai on this one. I think it's hard to argue that isCheckBoxDisabled was referring something other than isDisabledCheckbox

Before moving forward with the proposal, what are the use cases for which we should have item.isDisabled with a value different than item.isDisabledCheckbox?

neonbhai commented 1 month ago

what are the use cases for which we should have item.isDisabled with a value different than item.isDisabledCheckbox

we could have this situation if a user was invited to a workspace offline, or if a member is pending delete [ref: here]. But for disabled items, all presses are disabled on the Pressable in BaseListItem here: https://github.com/Expensify/App/blob/2336ba827e9752d951a322db3a0c61ee40849d8e/src/components/SelectionList/BaseListItem.tsx#L88 so a check for item.isDisabled is not needed here. We could still add this as a sanity check.

cc: @cead22

melvin-bot[bot] commented 1 month ago

📣 @neonbhai 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

neonbhai commented 1 month ago

Raising PR soon!

melvin-bot[bot] commented 1 month ago

@cead22, @bfitzexpensify, @neonbhai, @getusha Eep! 4 days overdue now. Issues have feelings too...

bfitzexpensify commented 4 weeks ago

How is this one going @neonbhai?

neonbhai commented 3 weeks ago

PR is ready for review!

cc: @getusha

melvin-bot[bot] commented 6 days ago

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 commented 2 days ago

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

cead22 commented 17 hours ago

@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: