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.56k stars 2.9k forks source link

[$500] Approve button not disappear after approving report offline if advanced approval set #47264

Closed izarutskaya closed 1 month ago

izarutskaya commented 3 months 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.19.2 Reproducible in staging?: Y Reproducible in production?: N Found when validating PR : https://github.com/Expensify/App/pull/44940 Logs: https://stackoverflow.com/c/expensify/questions/4856 Issue reported by: Applause-Internal team

Action Performed:

Precondition: create a control workspace, set "Manually approve all expenses" over to $10 and set advanced approval in OD:

  1. Log in as an employee 1 and submit an expense below $100, e.g. $25
  2. Log in as an manager
  3. Disable internet connection
  4. Approve the report

Expected Result:

The approve button dsiappears

Actual Result:

The approve button is still displayed until user returns online

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

https://github.com/user-attachments/assets/5b3f27ae-2414-42d4-9b3b-e62419645ce4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01682c8c3ddb795078
  • Upwork Job ID: 1829413690213012581
  • Last Price Increase: 2024-09-06
  • Automatic offers:
    • FitseTLT | Contributor | 103937090
Issue OwnerCurrent Issue Owner: @
melvin-bot[bot] commented 3 months ago

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

melvin-bot[bot] commented 3 months ago

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

github-actions[bot] commented 3 months 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.
izarutskaya commented 3 months ago

We think this issue might be related to the #wave-control

izarutskaya commented 3 months ago

Production

https://github.com/user-attachments/assets/fc7a8c83-a0e7-40f9-a214-034a3fdb4ea9

Beamanator commented 3 months ago

I'd say this isn't a blocker since advanced approvals in NewDot are quite new - I'll get @rushatgabhane and @mananjadhav to look into this ASAP (tomorrow hopefully) πŸ™ - see https://github.com/Expensify/App/pull/44940#issuecomment-2284943045

melvin-bot[bot] commented 3 months ago

@marcaaron, @adelekennedy Whoops! This issue is 2 days overdue. Let's get this updated quick!

adelekennedy commented 3 months ago

@Beamanator any updates on this from @mananjadhav and @rushatgabhane? Should I assign them here?

mananjadhav commented 2 months ago

I can take a look at this tomorrow.

melvin-bot[bot] commented 2 months ago

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

marcaaron commented 2 months ago

@mananjadhav Any update?

melvin-bot[bot] commented 2 months ago

@mananjadhav @marcaaron @adelekennedy 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!

melvin-bot[bot] commented 2 months ago

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

adelekennedy commented 2 months ago

@mananjadhav were you able to get to this one?

mananjadhav commented 2 months ago

I couldn't get to this one. I'll try to get to this by tomorrow.

adelekennedy commented 2 months ago

Great! If you don't have time we'll reassign to get it moving forward!

mananjadhav commented 2 months ago

@adelekennedy I haven't got time to take a look at the issue. We can open up for External contributors and I can help with the review if needed.

melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

Current assignee @mananjadhav is eligible for the External assigner, not assigning anyone new.

melvin-bot[bot] commented 2 months ago

@mananjadhav, @marcaaron, @adelekennedy Whoops! This issue is 2 days overdue. Let's get this updated quick!

adelekennedy commented 2 months ago

Still seeing if someone has time to pick this up

melvin-bot[bot] commented 2 months 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 months ago

Upwork job price has been updated to $500

garrettmknight commented 2 months ago

Let's get some action on this one.

FitseTLT commented 2 months ago

Edited by proposal-police: This proposal was edited at 2024-09-06 19:07:57 UTC.

Proposal

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

Approve button not disappear after approving report offline if advanced approval set

What is the root cause of that problem?

When there is an advanced approval chain the report status will not change to approved status https://github.com/Expensify/App/blob/812fc316a007b711d62a986582d19440c2c1dad8/src/libs/actions/IOU.ts#L6922 but instead stays at submitted and only the manager id will be changed to the next approver but we are not updating the managerID of the expense report optimistically here https://github.com/Expensify/App/blob/812fc316a007b711d62a986582d19440c2c1dad8/src/libs/actions/IOU.ts#L6938-L6948

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

We need to set the manager id optimistically to next approver id which we are already finding to build our next step

function getNextApproverAccountID(report: OnyxEntry<Report>) {
    const {policyID = '', ownerAccountID = -1} = report ?? {};
    const policy = PolicyUtils.getPolicy(policyID);
    const approvalChain = ReportUtils.getApprovalChain(policy, ownerAccountID, report?.total ?? 0);
    const submitToAccountID = PolicyUtils.getSubmitToAccountID(policy, ownerAccountID);

    if (approvalChain.length === 0) {
        return submitToAccountID;
    }

    const nextApproverEmail = approvalChain.length === 1 ? approvalChain[0] : approvalChain[approvalChain.indexOf(currentUserEmail) + 1];
    if (!nextApproverEmail) {
        return submitToAccountID;
    }

    return getAccountIDsByLogins([nextApproverEmail])[0];
}
    const managerID = isLastApprover(approvalChain) ? expenseReport?.managerID : getNextApproverAccountID(expenseReport) ?? expenseReport?.managerID;

or simply we can also do

    const currentUserApprovalIndex = approvalChain.indexOf(currentUserEmail);
    const managerID = isLastApprover(approvalChain) || currentUserApprovalIndex === -1 ? expenseReport?.managerID : getAccountIDsByLogins([approvalChain[currentUserApprovalIndex + 1]])[0];

and set the managerID of the expense report optimistically here https://github.com/Expensify/App/blob/812fc316a007b711d62a986582d19440c2c1dad8/src/libs/actions/IOU.ts#L6942-L6946

            managerID,

What alternative solutions did you explore? (Optional)

adelekennedy commented 2 months ago

@mananjadhav are you able to review proposals here or should we reassign?

rushatgabhane commented 2 months ago

i can take a look ^

mananjadhav commented 2 months ago

I’ll review in an hour.

mananjadhav commented 2 months ago

Reviewing the proposal now.

mananjadhav commented 2 months ago

@FitseTLT I understand adding the managerID to the optimistic steps but when I was trying to access the props last time this is what I found.

{
    "isCurrentUserManager": true,
    "isOpenExpenseReport": false,
    "isApproved": false,
    "iouSettled": false,
    "isArchivedReport": false
}

Essentially the canApproveIOU was returning true and if you see currentManagerID is already true based on the iouReport.managerID. So I am not sure how this is getting fixed. Can you please clarify?

https://github.com/Expensify/App/blob/9affb1384815fa8155fe23c523127c1e72b1120d/src/libs/actions/IOU.ts#L6812-L6833

And I tried setting the managerID with the currentUserApprovalIndex logic you posted and it didn't work for me.

FitseTLT commented 2 months ago

Essentially the canApproveIOU was returning true and if you see currentManagerID is already true based on the iouReport.managerID. So I am not sure how this is getting fixed. Can you please clarify?

Yeah. @mananjadhav This is exactly the reason my solution fixes it(BTW I have already tested my solution works perfectly). The reason the approve button exists after pressing the approve button is because the current user is still the manager. There are two cases for the approve action btw:

  1. For simple approve flow as soon as you press the button the iou report will be changed into approved state (so the approve button will disappear)
  2. But for our case here, advanced approval flow, the iou report state will not change because there is unfinished approval flow and there is still another approver to approve it and the iou report state will change to approved only when the last approver in the approval chain approves it. So 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 and the approve button will disappear because the current user will not be manager anymore (isCurrentUserManager is false).
melvin-bot[bot] commented 2 months ago

@mananjadhav @marcaaron @adelekennedy this issue is now 4 weeks old, please consider:

Thanks!

mananjadhav commented 2 months ago

@FitseTLT Do you mind uploading the video of the fix you've applied? I'll take a look again at my end too.

FitseTLT commented 2 months ago

Here is a test branch https://github.com/FitseTLT/App/tree/fix-approve-button-in-offline

https://github.com/user-attachments/assets/087fffaf-1b3d-4ef3-bc48-3e2ab31aa737

trjExpensify commented 2 months ago

Precondition: create a control workspace

This is an advanced approvals bug with the prerequisite being a control workspace, moving to #wave-control.

mananjadhav commented 2 months ago

Thanks for sharing the branch. I tested the flow and it worked fine. I was attempting a different change from yours and hence it didn't work for me.

I think @FitseTLT's proposal looks good to me.

πŸŽ€ πŸ‘€ πŸŽ€ C+ reviewed.

melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

πŸ“£ @FitseTLT πŸŽ‰ 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 πŸ“–

adelekennedy commented 2 months ago

PR is in progress

adelekennedy commented 1 month ago

Deployed to prod today!

mananjadhav commented 1 month ago

@adelekennedy this should be ready for payout. I couldn't exactly pinpoint the offending PR. Closest I think this should've been a part of https://github.com/Expensify/App/pull/44940/files.

Meanwhile I don't know if a regression test exists for the whole advance approval workflow. @adelekennedy can you confirm this? I think it should exists and in that case we don't need a regression test here.

adelekennedy commented 1 month ago

Payouts due:

Upwork job is here.

FitseTLT commented 1 month ago

@adelekennedy I received the payment Thx!

AngadManroy commented 1 month ago

is the issue still live?

melvin-bot[bot] commented 1 month ago

πŸ“£ @AngadManroy! πŸ“£ Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

  1. Make sure you've read and understood the contributing guidelines.
  2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Screen Shot 2022-11-16 at 4 42 54 PM Format:
    Contributor details
    Your Expensify account email: <REPLACE EMAIL HERE>
    Upwork Profile Link: <REPLACE LINK HERE>
garrettmknight commented 1 month ago

$250 approved for @mananjadhav

mananjadhav commented 1 month ago

@garrettmknight The amount is $500.