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.33k stars 2.76k forks source link

[$250] Workspace - Approver user becomes Hidden when error message to delete approver appears #47729

Closed lanitochka17 closed 3 days ago

lanitochka17 commented 3 weeks 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!


Version Number: 9.0.22-0 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4874319&group_by=cases:section_id&group_id=229065&group_order=asc Email or phone of affected tester (no customers): gocemate+a1018@gmail.com Issue reported by: Applause - Internal Team

Action Performed:

Preparation :

  1. Create a workspace
  2. Navigate to the Invite members page in workspace
  3. Invite a member
  4. Navigate to more features
  5. Enable workflows and approvals
  6. Add the recently added member as the approver

Steps:

  1. As the workspace admin, navigate to members
  2. Click on the member that is the approver
  3. Click on Remove
  4. Verify that you see the message: "${memberName} is an approver in this workspace. When you unshare this workspace with them, we’ll replace them in the approval workflow with the workspace owner, ${ownerName}"
  5. Go back to Workflow> Add approvals> Approval

Expected Result:

Display name and email address of Approver should be visible

Actual Result:

Approver becomes Hidden when error message to delete approver appears

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/58037c81-27ec-49ef-8f37-cb55c23c7fd7

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~012277cda3e4453431
  • Upwork Job ID: 1825989387075424165
  • Last Price Increase: 2024-08-20
Issue OwnerCurrent Issue Owner: @c3024
melvin-bot[bot] commented 3 weeks ago

Triggered auto assignment to @garrettmknight (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.

lanitochka17 commented 3 weeks ago

@garrettmknight FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

daledah commented 3 weeks ago

Edited by proposal-police: This proposal was edited at 2023-10-22T10:00:00Z.

Proposal

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

Approver becomes Hidden when error message to delete approver appears

What is the root cause of that problem?

When we delete the approver, we will get an error. And that error message is still saved in Onyx when we go to the workflow approver page

Then that approver will be filtered out here

https://github.com/Expensify/App/blob/72c8cf7679f2ce80e39e8fd14c55bc48b5f80937/src/pages/workspace/workflows/approvals/WorkspaceWorkflowsApprovalsApproverPage.tsx#L121

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

  1. We will not filter out members with errors by adding includeMemberWithErrors = true here
        const policyMemberEmailsToAccountIDs = PolicyUtils.getMemberAccountIDsForWorkspace(policy?.employeeList, true);
  1. I think we should fix this issue here too.

We need to build the function dismissError like we did here and pass in props onDismissError in here

                    <SelectionList
                        ...
                        onSelectRow={setPolicyApprover}
                        shouldSingleExecuteRowSelect
                        showScrollIndicator
+                      onDismissError={dismissError}
                    />

What alternative solutions did you explore? (Optional)

We can consider the solution: we can hide the remove button when the user deletes the approver, only show the warning and not call API

daledah commented 3 weeks ago

Updated proposal

melvin-bot[bot] commented 3 weeks ago

Job added to Upwork: https://www.upwork.com/jobs/~012277cda3e4453431

melvin-bot[bot] commented 3 weeks ago

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

c3024 commented 3 weeks ago

When removing the approver member, the confirmation modal states that the approver will be automatically replaced by the owner.

Screenshot 2024-08-21 at 12 05 11 PM

However, when we proceed to remove the member, the backend returns an error saying that the approver cannot be removed unless they are replaced by another member.

Screenshot 2024-08-21 at 12 06 55 PM

These two messages seem conflicting.

I believe we should correct the warning in the modal to clearly state that the approver cannot be removed

Screenshot 2024-08-21 at 12 05 11 PM

as @daledah suggested here:

We can consider the solution: hide the remove button when the user attempts to delete the approver, only show the warning, and avoid calling the API.

We should also implement the remaining solution of not filtering users with errors and adding the onDismiss function in @daledah's proposal to make the fix complete and fix #47731 also.

@daledah's proposal LGTM!

🎀 👀 🎀 C+ Reviewed

melvin-bot[bot] commented 3 weeks ago

Triggered auto assignment to @arosiclair, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

arosiclair commented 3 weeks ago

When removing the approver member, the confirmation modal states that the approver will be automatically replaced by the owner.

However, when we proceed to remove the member, the backend returns an error saying that the approver cannot be removed unless they are replaced by another member.

It sounds like this might not be fully implemented yet. Reaching out in #wave-collect about it

arosiclair commented 3 weeks ago

This is related to https://github.com/Expensify/App/issues/43508.

Added this to the #wave-collect and marking this Internal for now since it likely requires backend changes to address the behavior described in the confirmation modal.

arosiclair commented 2 weeks ago

A couple more issues for push notifications and QBO errors landed on my plate so I don't think I have the bandwidth to look into this now. Re-assigning to @marcochavezf since we talked about it. Lemme know if you need anything from me

melvin-bot[bot] commented 2 weeks ago

@garrettmknight, @marcochavezf, @c3024 Whoops! This issue is 2 days overdue. Let's get this updated quick!

garrettmknight commented 2 weeks ago

@marcochavezf you're assigned - are you gonna take this one? If not, feel free to drop off.

marcochavezf commented 2 weeks ago

I already have my plate full, dropping it off and assigning the Hot Pick label

melvin-bot[bot] commented 1 week ago

@garrettmknight this issue was created 2 weeks ago. Are we close to a solution? Let's make sure we're treating this as a top priority. Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

garrettmknight commented 1 week ago

Awaiting a volunteer - changing to weekly

melvin-bot[bot] commented 1 week ago

@garrettmknight Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

arosiclair commented 3 days ago

Per this other issue, it looks like this bug was fixed!