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

Tax - Split scan - Selected tax rate is duplicated in Tax rate list #42642

Closed
6 tasks done
m-natarajan opened this issue May 27, 2024 · 24 comments
Closed
6 tasks done

Tax - Split scan - Selected tax rate is duplicated in Tax rate list #42642

m-natarajan opened this issue May 27, 2024 · 24 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering

Comments

@m-natarajan
Copy link

m-natarajan commented May 27, 2024

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Issue found when validating #40240
Version Number: 1.4.76-0
Reproducible in staging?: y
Reproducible in production?: n
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause internal team
Slack conversation:

Action Performed:

Precondition:

  • Workspace is tax enabled.
  • Workspace currency default and Foreign currency default have different tax rate in workspace tax settings.
  1. Go to staging.new.expensify.com
  2. Go to workspace chat.
  3. Create a split scan.
  4. When the receipt is scanning, click on the split preview.
  5. Click Amount.
  6. Change the currency to foreign currency.
  7. Enter amount and save it.
  8. Click Tax rate.

Expected Result:

The selected tax rate will not be duplicated.

Actual Result:

The selected tax rate is duplicated.

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

Bug6493019_1716821848213.20240527_225309.mp4

View all open jobs on GitHub

Issue OwnerCurrent Issue Owner: @Christinadobrzyn
@m-natarajan m-natarajan added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels May 27, 2024
Copy link

melvin-bot bot commented May 27, 2024

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

@m-natarajan m-natarajan added DeployBlockerCash This issue or pull request should block deployment DeployBlocker Indicates it should block deploying the API labels May 27, 2024
@github-actions github-actions bot added Engineering Hourly KSv2 and removed Daily KSv2 labels May 27, 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.

Copy link

melvin-bot bot commented May 27, 2024

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

@m-natarajan
Copy link
Author

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

@m-natarajan
Copy link
Author

We think that this bug might be related to #vip-vsb

@blimpich
Copy link
Contributor

Looking into this

@blimpich
Copy link
Contributor

I don't think this is worth blocking the deploy, demoting.

@blimpich blimpich 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 May 27, 2024
@blimpich
Copy link
Contributor

@MonilBhavsar I think this is related to #40240. Do you want to take this over? Otherwise I think we can just open this up to contributors.

@cretadn22
Copy link
Contributor

Proposal

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

Duplicated tax rate in the tax rate list

What is the root cause of that problem?

when we update the split transaction, we save the data in splitDraftTransaction. However, we consistently rely on transaction in the TaxPicker Component

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

In the TaxPicker, we need to implement a logic similar to what we've done in the IOURequestStepTaxRatePage

const currentTransaction = isEditingSplitBill && !isEmptyObject(splitDraftTransaction) ? splitDraftTransaction : transaction;

When isEditingSplitBill is true, we should utilize splitDraftTransaction instead of transaction

What alternative solutions did you explore? (Optional)

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

@cretadn22
Copy link
Contributor

@blimpich This seems somewhat connected to #40240, but I don't believe it is a regression

cc @MonilBhavsar

@blimpich
Copy link
Contributor

@cretadn22 if this wasn't caused by #40240 then what did cause this? Please explain why you think this specifically isn't a regression

@bondydaa
Copy link
Contributor

the revert #42670 has been deployed, please retest and confirm this is no longer reproducible.

@blimpich
Copy link
Contributor

cc: @m-natarajan, can you see if this still reproduces?

@kavimuru
Copy link

This bug is not reproducible

@Christinadobrzyn
Copy link
Contributor

I also can't reproduce this issue. I think we can close this. Feel free to let me know if I'm missing anything!

@MonilBhavsar
Copy link
Contributor

MonilBhavsar commented May 29, 2024

I think we should pay @cretadn22 $100(May be $125 i.e. 50% of our usual bounty) for investigating the root cause of this and other related issues.
I think it helped to revert the PR in timely manner and move forward with the deploy. Also it saved me some time fixing the bugs in a follow up PR. cc @Christinadobrzyn @blimpich @bondydaa

Comments:
#42642 (comment)
#42649 (comment)
#42652 (comment)
#42655 (comment)

@MonilBhavsar MonilBhavsar reopened this May 29, 2024
@MonilBhavsar MonilBhavsar removed the Reviewing Has a PR in review label May 29, 2024
@cretadn22
Copy link
Contributor

@MonilBhavsar Thank you for your recognition of my efforts. After proposing solutions for 4 blockers, it appears they were implemented in the subsequent pull request. Could I suggest raising the bounty to $250 for the effort put into investigating and suggest solutions for those blockers? It seems more fitting than the current $125, which is only half the base price for an issue.

@Christinadobrzyn
Copy link
Contributor

checking with @MonilBhavsar on your request @cretadn22

@Christinadobrzyn Christinadobrzyn added Daily KSv2 and removed Weekly KSv2 labels Jun 5, 2024
@MonilBhavsar
Copy link
Contributor

I can't think of a reason why we should not. So I'm fine with paying it. Fine with what others has to say 👍

@blimpich
Copy link
Contributor

blimpich commented Jun 6, 2024

Sounds good to me 👍

@Christinadobrzyn can we pay out $250 to @cretadn22 and then close this issue?

@Christinadobrzyn
Copy link
Contributor

Christinadobrzyn commented Jun 7, 2024

Yep! Payouts due:

Upwork job is here.

@cretadn22 can you accept? TY!

@cretadn22
Copy link
Contributor

@Christinadobrzyn Done 🙏 Thanks so much

@blimpich blimpich closed this as completed Jun 7, 2024
@cretadn22
Copy link
Contributor

Awaiting payment from @Christinadobrzyn

@blimpich blimpich reopened this Jun 7, 2024
@blimpich blimpich added the Awaiting Payment Auto-added when associated PR is deployed to production label Jun 7, 2024
@Christinadobrzyn
Copy link
Contributor

Thanks @cretadn22 - Paid out based on this payment summary - #42642 (comment)

Closing!

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. Daily KSv2 Engineering
Projects
None yet
Development

No branches or pull requests

7 participants