[$250] Workspace Chat - Second approver unapproval shows 'Waiting for first approver' in next steps

[lanitochka17]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: 9.0.44-8
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): [email protected]
Issue reported by: Applause - Internal Team

Action Performed:

1. Navigate to staging.new.expensify.com/
2. Create a new workspace and enable the workflow feature
3. Set the expense submission mode to "Manual
4. Add 3 members to the workspace:

• An employee
• Approver A (first approver)
• Approver B (second approver)

5. Configure the approval workflow:

• Employee submits to Approver A
• Approver A sends it to Approver B

6. As the employee, submit an expense in the workspace chat
7. Approve the expense as Approver A
8. As Approver B, approve the expense, then unapprove it
9. Observe the next step in the workflow

Expected Result:

After Approver B unapproves the expense, the message should revert to: "Waiting for you to approve expense(s)."

Actual Result:

After Approver B unapproves the expense, the next step for Approver B says:"Waiting for Approver A to approve expense(s)." Meanwhile, on Approver A's side, the next step says:
"Waiting for Approver B to approve expense(s)."

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

Add any screenshot/video evidence

Bug6624607_1728057683525.unapprove.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021843453908006071230
• Upwork Job ID: 1843453908006071230
• Last Price Increase: 2024-10-22
• Automatic offers:
• truph01 | Contributor | 104801974

[melvin-bot]

Triggered auto assignment to @kadiealexander (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]

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

[lanitochka17]

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

[Nodebrute]

Edited by proposal-police: This proposal was edited at 2024-10-04 22:46:01 UTC.

Proposal

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

Second approver unapproval shows 'Waiting for first approver' in next steps

What is the root cause of that problem?

We use this function to get next approver ID

App/src/libs/actions/IOU.ts

Line 7011 in 10b8547

function getNextApproverAccountID(report: OnyxEntry<OnyxTypes.Report>) {

and then in when Approver B will be last in the approver chain so when we try to (currentUserEmail) + 1) ...nextApproverEmail will undefined.

App/src/libs/actions/IOU.ts

Line 7021 in 10b8547

const nextApproverEmail = approvalChain.length === 1 ? approvalChain.at(0) : approvalChain.at(approvalChain.indexOf(currentUserEmail) + 1);

so it will fallback to this which will be Approver A. We'll see outdated approver until we'll refresh the page or switch report

App/src/libs/actions/IOU.ts

Lines 7022 to 7024 in 10b8547

	if (!nextApproverEmail) {
	return submitToAccountID;
	}

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

We can fallback here to currentUserEmail

    const nextApproverEmail = approvalChain.length === 1 ? approvalChain.at(0) : approvalChain.at(approvalChain.indexOf(currentUserEmail) + 1) || currentUserEmail;

This will fix the problem but it will show Approver B until we'll get the data from backend so to change it to 'You' optimistically we need to add clickToCopyText here. No matter which solution we pick this change is required.

    clickToCopyText: approverAccountID === currentUserAccountID ? currentUserEmail : '',

Screen.Recording.2024-10-05.at.3.51.50.AM.mov

What alternative solutions did you explore? (Optional)

We can also check currentIndex === approvalChain.length - 1 then it should fall back to current user

const currentIndex = approvalChain.indexOf(currentUserEmail);
    const nextApproverEmail = approvalChain.length === 1 
    ? approvalChain.at(0) 
    : (currentIndex === approvalChain.length - 1 
        ? currentUserEmail 
        : approvalChain.at(currentIndex + 1))

Note: This is just pseudocode. We can use different combinations of this logic. It's also possible to return the current user ID early if the conditions are met..

Alternate Solution 2
We can pass third param here indicating that we are unapproving the iou isFromUnapprove

App/src/libs/actions/IOU.ts

Line 7200 in 10b8547

const optimisticNextStep = NextStepUtils.buildNextStep(expenseReport, CONST.REPORT.STATUS_NUM.SUBMITTED);

and then we can pass it down to

App/src/libs/NextStepUtils.ts

Line 71 in 10b8547

const approverAccountID = getNextApproverAccountID(report);

and in that function, we can use to check

 const currentIndex = approvalChain.indexOf(currentUserEmail);
    const nextApproverEmail = approvalChain.length === 1 
    ? approvalChain.at(0) 
    : (isFromUnapprove 
        ? currentUserEmail 
        : (currentIndex === approvalChain.length - 1 
            ? currentUserEmail 
            : approvalChain.at(currentIndex + 1)));

Alternative 3
Or we can check if isFromUnapprove in this function then we can return current user account ID

App/src/libs/actions/IOU.ts

Line 7011 in 10b8547

function getNextApproverAccountID(report: OnyxEntry<OnyxTypes.Report>) {

[Nodebrute]

Proposal Updated
Added poc

[truph01]

Edited by proposal-police: This proposal was edited at 2024-10-05 03:04:11 UTC.

Proposal

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

After Approver B unapproves the expense, the next step for Approver B says:"Waiting for Approver A to approve expense(s)." Meanwhile, on Approver A's side, the next step says:
"Waiting for Approver B to approve expense(s)."

What is the root cause of that problem?

• When unapproving expense, we call:

App/src/libs/actions/IOU.ts

Line 7192 in b9d9686

function unapproveExpenseReport(expenseReport: OnyxEntry<OnyxTypes.Report>) {

• In the text Waiting for ${user} to approve expense(s). comes from:

App/src/libs/actions/IOU.ts

Line 7200 in b9d9686

const optimisticNextStep = NextStepUtils.buildNextStep(expenseReport, CONST.REPORT.STATUS_NUM.SUBMITTED);

• In buildNextStep function, we get the user data from:

App/src/libs/actions/IOU.ts

Line 7011 in b9d9686

function getNextApproverAccountID(report: OnyxEntry<OnyxTypes.Report>) {

• The getNextApproverAccountID is only defined to handled case approve, not unapprove. As a result, the nextApproverEmail in:

App/src/libs/actions/IOU.ts

Line 7021 in b9d9686

const nextApproverEmail = approvalChain.length === 1 ? approvalChain.at(0) : approvalChain.at(approvalChain.indexOf(currentUserEmail) + 1);

is undefined so it fallbacks to submitToAccountID, so the message Waiting for first approver is displayed instead of Waiting for second(you) approver

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

• We should update function getNextApproverAccountID so that it can handled case unapprove as well and because both approver in approvalChain:
  

  App/src/libs/actions/IOU.ts

  Line 7014 in b9d9686

  const approvalChain = ReportUtils.getApprovalChain(policy, ownerAccountID, report?.total ?? 0);

  
  and policy admin can unapprove any expense, so we need to make sure:

1. If the current user is in approvalChain: Return current user ID.
2. If the current user is admin but is not in approvalChain: Return current manager ID. When the current user is not the last approver in the chain what BE does (and also what I have done in optimistic data) is the managerID of the iou report will be set to the next approver, based on comment.

• Based on the above, the detailed code changes are:

1. In there, introduce new param:

function getNextApproverAccountID(report: OnyxEntry<OnyxTypes.Report>, isUnapproved = false) {

2. In there, add:

    if (isUnapproved) {
        if (approvalChain.includes(currentUserEmail)) {
            return userAccountID;
        } else {
            return report?.managerID;
        }
    }

3. In there, call NextStepUtils.buildNextStep with extra param isUnapprove is true, then pass down to getNextApproverAccountID as we defined above.

4. In there add:

                        clickToCopyText: approvers.at(0),

with:

    const approverAccountID = getNextApproverAccountID(report, isUnapprove);
    const approvers = getLoginsByAccountIDs([approverAccountID ?? -1]);

5. BE: There are pusher bugs in BE we need to fix to sync the next step message between two users.

What alternative solutions did you explore? (Optional)

[truph01]

Proposal updated

[melvin-bot]

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

[melvin-bot]

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

[trjExpensify]

multi-step approvals is a control-only feature. Moving.

[melvin-bot]

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

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[melvin-bot]

@allroundexperts, @kadiealexander 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

[truph01]

Proposal updated

[truph01]

@allroundexperts Can you review the above proposals?

[melvin-bot]

@allroundexperts, @kadiealexander 8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it!

[melvin-bot]

@allroundexperts @kadiealexander this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[melvin-bot]

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

[truph01]

@allroundexperts Your selected solution leads to regression if the current user is an admin, not an approver. When the admin unapproves the expense, it should display "Waiting for first approver", but with that solution, it display "Waiting for your approver" since that solution fallback to the current user email. Please correct me if I am wrong.

[melvin-bot]

@Julesssss @abekkala @allroundexperts @kadiealexander this issue is now 4 weeks old, please consider:

• Finding a contributor to fix the bug
• Closing the issue if BZ has been unable to add the issue to a VIP or Wave project
• If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

[melvin-bot]

@Julesssss, @abekkala, @allroundexperts, @kadiealexander Whoops! This issue is 2 days overdue. Let's get this updated quick!

[truph01]

@allroundexperts Can you check my comment above?

[melvin-bot]

@Julesssss, @abekkala, @allroundexperts, @kadiealexander Still overdue 6 days?! Let's take care of this!

[Julesssss]

Bumping @allroundexperts for your review.

The solutions are similar and I'm finding it difficult to figure out how/when the proposals were edited. It looks like this proposal includes the detail of needing to fix on backend. Though I think @truph01 is correct in pointing out that small difference in text from Waiting for first approver | Waiting for your approver

To be honest the backend issue is unlikely to be fixed.

[allroundexperts]

I'll look into this again today.

[allroundexperts]

Okay. I think its a really small difference that @truph01 has pointed out, but it does lead to a regression (although its super minor). I'm a little confused if that difference is enough to select @truph01's proposal over @Nodebrute's proposal.

@Julesssss I'd leave this final decision to you.

[truph01]

There are only two cases to consider: when the current user is an approver and when they are not. The previously selected proposal results in a regression for the second case, so I believe it's more than a minor issue. Up to now, I haven't encountered a scenario where a proposal with a regression would be chosen, regardless of how minor that regression may be.
Finally, let @Julesssss leave the decision.

[Julesssss]

Yeah, I see no reason not to move forward with @truph01's proposal here.

@truph01 in your PR please make sure to add tests for both of these cases.

[melvin-bot]

📣 @truph01 🎉 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 📖

[truph01]

PR is ready

[melvin-bot]

This issue has not been updated in over 15 days. @Julesssss, @abekkala, @allroundexperts, @kadiealexander, @truph01 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!

[abekkala]

note to self: PR has not merged yet

[melvin-bot]

@Julesssss, @abekkala, @allroundexperts, @kadiealexander, @truph01, this Monthly task hasn't been acted upon in 6 weeks; closing.

If you disagree, feel encouraged to reopen it -- but pick your least important issue to close instead.

[allroundexperts]

This is ongoing. @truph01 Can you please resolve conflicts on your PR?