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.47k stars 2.82k forks source link

[$250] Header - No unapprove option for first approver #50292

Open lanitochka17 opened 1 week ago

lanitochka17 commented 1 week 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.45-2 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: N/A Email or phone of affected tester (no customers): biruknew45+1399@gmail.com Issue reported by: Applause - Internal Team

Action Performed:

  1. Go to https://staging.new.expensify.com/
  2. Create a new workspace and enable workflow
  3. Set the delay submission to "Manual."
  4. Add 3 members to the workspace:
    • Employee
    • Approver A (first approver)
    • Approver B (second approver)
  5. Configure the approval workflow:
    • Employee submits to Approver A
    • Approver B is the second approver
  6. As the employee, submit an expense in the workspace chat
  7. As Approver A, approver the expense
  8. As approver A, click on the preview and then go to the header

Expected Result:

The first approver should have the option to unapprove the expense, similar to the second approver

Actual Result:

There is no option for the first approver to unapprove the expense in the header, but the second approver has

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/d8b5a7d2-e110-4df2-ba7a-a8fbbf8c0623

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021843475137047948222
  • Upwork Job ID: 1843475137047948222
  • Last Price Increase: 2024-10-08
Issue OwnerCurrent Issue Owner: @alitoshmatov
melvin-bot[bot] commented 1 week ago

Triggered auto assignment to @VictoriaExpensify (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 1 week ago

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

cretadn22 commented 1 week ago

Proposal

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

The first approver does not have an option to unapprove.

What is the root cause of that problem?

When there are multiple approvers in the workflow, if the first approver approves the request, the iouReport.managerID will be updated to the next approver. If the last approver approves the request, the iouReport.managerID will remain set to the last approver

https://github.com/Expensify/App/blob/99f280b0edae75ff59614a1c5e98c47b39800aa5/src/pages/ReportDetailsPage.tsx#L205

As a result, ReportUtils.isReportManager(report) will return false since the current manager ID is now set to the next approver

Additionally, ReportUtils.isReportApproved(report) will return false because the request still requires approval from other approvers.

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

We need to update the condition to allow users to unapprove requests in two separate cases:

Draft implementation ``` const [currentUserAccountID = -1] = useOnyx(ONYXKEYS.SESSION, {selector: (value) => value?.accountID}); const approvalChain = ReportUtils.getApprovalChain(policy, report?.ownerAccountID ?? -1, report?.total ?? 0); let previousApprover approvalChain.forEach((email, index) => { const personalDetail = PersonalDetailsUtils.getPersonalDetailByEmail(email); if (personalDetail?.accountID === report.managerID) { previousApprover = PersonalDetailsUtils.getPersonalDetailByEmail(approvalChain[index - 1] ?? '')?.accountID } }) const isLastApproverAndReportApproved = ReportUtils.isReportManager(report) && ReportUtils.isReportApproved(report) const isPreviousApproverAndReportNotApproved = currentUserAccountID === previousApprover && !ReportUtils.isReportApproved(report) const canUnapproveRequest = ReportUtils.isExpenseReport(report) && (isLastApproverAndReportApproved || isPolicyAdmin || isPreviousApproverAndReportNotApproved) && !PolicyUtils.isSubmitAndClose(policy); ```

After implementing the fix on the front end, we also need to update the API to ensure everything functions correctly

What alternative solutions did you explore? (Optional)

VictoriaExpensify commented 6 days ago

Yeah Approver A should have the ability to unapproved the expense, but I think they should only have this option up until it is approved by Approver B - believe that's how Classic logic works. Checking on this.

melvin-bot[bot] commented 6 days ago

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

melvin-bot[bot] commented 6 days ago

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

truph01 commented 6 days ago

@alitoshmatov Currently, if an expense is awaiting approval from approver B, and approver A attempts to unapprove the expense, the backend throws an error: 'Only managers and admins can unapprove.'

I am not sure if the feature that "Allow approver A to unapprove" can be supported by BE, can you or any internal member confirm?

melvin-bot[bot] commented 2 days ago

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