Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
1 of 6 tasks
izarutskaya opened this issue Aug 12, 2024 · 47 comments
Closed
1 of 6 tasks
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@izarutskaya
Copy link

izarutskaya commented Aug 12, 2024

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.19.2
Reproducible in staging?: Y
Reproducible in production?: N
Found when validating PR : #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:

  • boss owner (admin) submits to mini owner
  • mini owner (admin) submits boss owner
  • manager (employee) submits to mini owner forwards to mini owner over limit forwards (limit 100) to boss owner
  • employee 1 (employee) submits to manager
  • employee 2(employee) submits to manager
  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?

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Bug6570173_1723484236470.Recording__666.mp4

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: @
@izarutskaya izarutskaya added DeployBlockerCash This issue or pull request should block deployment Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. DeployBlocker Indicates it should block deploying the API labels Aug 12, 2024
Copy link

melvin-bot bot commented Aug 12, 2024

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.

Copy link

melvin-bot bot commented Aug 12, 2024

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

@github-actions github-actions bot added Engineering Hourly KSv2 and removed Daily KSv2 labels Aug 12, 2024
Copy link
Contributor

👋 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
Copy link
Author

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

@izarutskaya
Copy link
Author

Production

Recording.668.mp4

@Beamanator Beamanator added Daily KSv2 and removed DeployBlockerCash This issue or pull request should block deployment DeployBlocker Indicates it should block deploying the API Hourly KSv2 labels Aug 12, 2024
@Beamanator
Copy link
Contributor

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 #44940 (comment)

@melvin-bot melvin-bot bot added the Overdue label Aug 15, 2024
Copy link

melvin-bot bot commented Aug 16, 2024

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

@adelekennedy
Copy link

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

@melvin-bot melvin-bot bot removed the Overdue label Aug 19, 2024
@mananjadhav
Copy link
Collaborator

I can take a look at this tomorrow.

@melvin-bot melvin-bot bot added the Overdue label Aug 22, 2024
Copy link

melvin-bot bot commented Aug 23, 2024

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

@FitseTLT
Copy link
Contributor

FitseTLT commented Sep 9, 2024

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).

Copy link

melvin-bot bot commented Sep 9, 2024

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

@mananjadhav
Copy link
Collaborator

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

@FitseTLT
Copy link
Contributor

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

2024-09-10.16-21-05.mp4

@trjExpensify
Copy link
Contributor

Precondition: create a control workspace

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

@mananjadhav
Copy link
Collaborator

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.

Copy link

melvin-bot bot commented Sep 12, 2024

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

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 12, 2024
Copy link

melvin-bot bot commented Sep 12, 2024

📣 @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
Copy link

PR is in progress

@adelekennedy
Copy link

Deployed to prod today!

@mananjadhav
Copy link
Collaborator

@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
Copy link

adelekennedy commented Oct 9, 2024

Payouts due:

  • Contributor: $500 @FitseTLT (Upwork) @FitseTLT will you confirm you received the payment? I know we've been experiencing an Upwork bug, in this case i released payment first and then closed the contract
  • Contributor+: $500 @mananjadhav (NewDot)

Upwork job is here.

@FitseTLT
Copy link
Contributor

FitseTLT commented Oct 9, 2024

@adelekennedy I received the payment Thx!

@AngadManroy
Copy link

is the issue still live?

Copy link

melvin-bot bot commented Oct 13, 2024

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

@adelekennedy adelekennedy added the Awaiting Payment Auto-added when associated PR is deployed to production label Oct 14, 2024
@garrettmknight
Copy link
Contributor

$250 approved for @mananjadhav

@mananjadhav
Copy link
Collaborator

@garrettmknight The amount is $500.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
Archived in project
Development

No branches or pull requests

10 participants