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

lanitochka17 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!

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): biruknew45+1371@gmail.com Issue reported by: Applause - Internal Team

Action Performed:

Navigate to staging.new.expensify.com/
Create a new workspace and enable the workflow feature
Set the expense submission mode to "Manual
Add 3 members to the workspace:
- An employee
- Approver A (first approver)
- Approver B (second approver)
Configure the approval workflow:
- Employee submits to Approver A
- Approver A sends it to Approver B
As the employee, submit an expense in the workspace chat
Approve the expense as Approver A
As Approver B, approve the expense, then unapprove it
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
[x] Android: mWeb Chrome
[ ] iOS: Native
[ ] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/8ac70554-3261-47b9-8688-f4a5e9ae61c8

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

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

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

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

Nodebrute commented 1 month ago

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 https://github.com/Expensify/App/blob/10b8547c3fdad76e9a6f1986edef5b94507e1ae6/src/libs/actions/IOU.ts#L7011

and then in when Approver B will be last in the approver chain so when we try to (currentUserEmail) + 1) ...nextApproverEmail will undefined. https://github.com/Expensify/App/blob/10b8547c3fdad76e9a6f1986edef5b94507e1ae6/src/libs/actions/IOU.ts#L7021 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 https://github.com/Expensify/App/blob/10b8547c3fdad76e9a6f1986edef5b94507e1ae6/src/libs/actions/IOU.ts#L7022-L7024

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 : '',

https://github.com/user-attachments/assets/7b9bd70c-13f8-422e-a42c-94448ecc5dbf

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 https://github.com/Expensify/App/blob/10b8547c3fdad76e9a6f1986edef5b94507e1ae6/src/libs/actions/IOU.ts#L7200

and then we can pass it down to https://github.com/Expensify/App/blob/10b8547c3fdad76e9a6f1986edef5b94507e1ae6/src/libs/NextStepUtils.ts#L71

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 https://github.com/Expensify/App/blob/10b8547c3fdad76e9a6f1986edef5b94507e1ae6/src/libs/actions/IOU.ts#L7011

Nodebrute commented 1 month ago

Proposal Updated Added poc

truph01 commented 1 month ago

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:

https://github.com/Expensify/App/blob/b9d9686d23200813b57ff98bbe69a070fb31d28d/src/libs/actions/IOU.ts#L7192

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

https://github.com/Expensify/App/blob/b9d9686d23200813b57ff98bbe69a070fb31d28d/src/libs/actions/IOU.ts#L7200

In buildNextStep function, we get the user data from:

https://github.com/Expensify/App/blob/b9d9686d23200813b57ff98bbe69a070fb31d28d/src/libs/actions/IOU.ts#L7011

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

https://github.com/Expensify/App/blob/b9d9686d23200813b57ff98bbe69a070fb31d28d/src/libs/actions/IOU.ts#L7021

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: https://github.com/Expensify/App/blob/b9d9686d23200813b57ff98bbe69a070fb31d28d/src/libs/actions/IOU.ts#L7014 and policy admin can unapprove any expense, so we need to make sure:

If the current user is in approvalChain: Return current user ID.
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:

In there, introduce new param:

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

In there, add:

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

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

                        clickToCopyText: approvers.at(0),

with:

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

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

Proposal updated

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

trjExpensify commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 4 weeks ago

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

melvin-bot[bot] commented 4 weeks ago

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

truph01 commented 4 weeks ago

Proposal updated

truph01 commented 4 weeks ago

@allroundexperts Can you review the above proposals?

melvin-bot[bot] commented 3 weeks ago

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

melvin-bot[bot] commented 3 weeks ago

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

allroundexperts commented 3 weeks ago

Thanks for the proposals everyone.

@truph01 Can you please explain what you mean by the following statement?

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

Are you suggesting that we need backend fixes with this as well?

@Nodebrute Just to confirm, are you not suggesting any backend change?

truph01 commented 3 weeks ago

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

During testing, I noticed a few pusher issues—such as when a report is unapproved on one device, the "next steps" status doesn't update on the second device. However, these are separate issues and aren't directly related to this one.

Are you suggesting that we need backend fixes with this as well?

As mentioned, resolving this specific issue does not require any backend fixes.

melvin-bot[bot] commented 3 weeks ago

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

melvin-bot[bot] commented 2 weeks ago

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

kadiealexander commented 2 weeks ago

@allroundexperts bump on this! Reassigning for someone to babysit while I'm OOO for the next two weeks.

melvin-bot[bot] commented 2 weeks ago

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

abekkala commented 2 weeks ago

@kadiealexander please leave a status summary before going ooo and reassigning. I'm not sure where this issue is currently at?

@allroundexperts can you give me a quick status on this one?

allroundexperts commented 2 weeks ago

@allroundexperts can you give me a quick status on this one?

This one is pending a reply from me @abekkala.

allroundexperts commented 2 weeks ago

@truph01 Given that backend changes are not strictly related to this issue, I can't find much of a difference between your and @Nodebrute's proposal. Both are pointing towards the usage of currentUserEmail as a fallback. As such, we can go with @Nodebrute's proposal.

🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 2 weeks ago

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

truph01 commented 2 weeks ago

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

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

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

truph01 commented 1 week ago

@allroundexperts Can you check my comment above?

melvin-bot[bot] commented 1 week ago

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

Julesssss commented 6 days ago

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

I'll look into this again today.

allroundexperts commented 5 days ago

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

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

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[bot] commented 4 days ago

📣 @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 commented 1 day ago

PR is ready

Expensify / App