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

[HOLD for payment 2025-01-13] Tax rate - System message does not show the correct tax rate when category has tax rate #54515

Closed
5 of 8 tasks
IuliiaHerets opened this issue Dec 24, 2024 · 19 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Engineering Weekly KSv2

Comments

@IuliiaHerets
Copy link

IuliiaHerets commented Dec 24, 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.78-0
Reproducible in staging?: Yes
Reproducible in production?: No
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: Yes, reproducible on both
If this was caught during regression testing, add the test name, ID and link from TestRail: Exp
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause Internal Team
Device used: Windows 11
App Component: Money Requests

Action Performed:

Precondition:

  • Rules and Taxes are enabled.
  • There are three tax rates - A, B and C.
  • Tax rate A belongs to Workspace currency default.
  • Tax rate B belongs to Foreign currency default.
  1. Go to staging.new.expensify.com
  2. Go to workspace settings > Categories.
  3. Click Advertising category.
  4. Click Default tax rate.
  5. Select Tax rate C.
  6. Go to workspace chat.
  7. Submit an expense in foreign currency, and with "Advertising" category from Step 3.
  8. Go to transaction thread.
  9. Click Category.
  10. Click on Advertising category to unselect it.
  11. Note that tax field shows Tax rate B, but the system message shows "changed the tax rate to Tax rate A".
  12. Click Category.
  13. Select any other category except Advertising.
  14. Note that tax field shows Tax rate B, but the system message shows "changed the tax rate to Tax rate A".

Expected Result:

In Step 11 and 14, the system message should correctly reflect the current state of selected tax rate.

Actual Result:

In Step 11 and 14, the system message shows that the tax rate is changed to Tax rate A, when the tax field shows that Tax rate B is selected.

Workaround:

Unknown

Platforms:

  • Android: Standalone
  • Android: HybridApp
  • Android: mWeb Chrome
  • iOS: Standalone
  • iOS: HybridApp
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Bug6701713_1735034366618.bandicam_2024-12-24_17-54-15-171.1.mp4

View all open jobs on GitHub

Issue OwnerCurrent Issue Owner: @sakluger
@IuliiaHerets IuliiaHerets added DeployBlockerCash This issue or pull request should block deployment Bug Something is broken. Auto assigns a BugZero manager. labels Dec 24, 2024
Copy link

melvin-bot bot commented Dec 24, 2024

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

Copy link

melvin-bot bot commented Dec 24, 2024

Triggered auto assignment to @sakluger (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 Dec 24, 2024

💬 A slack conversation has been started in #expensify-open-source

@melvin-bot melvin-bot bot added the Daily KSv2 label Dec 24, 2024
@github-actions github-actions bot added Engineering Hourly KSv2 and removed Daily KSv2 labels Dec 24, 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.

@sakluger
Copy link
Contributor

Discussing in Slack.

@nkdengineer
Copy link
Contributor

nkdengineer commented Dec 25, 2024

Proposal

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

In Step 11 and 14, the system message shows that the tax rate is changed to Tax rate A, when the tax field shows that Tax rate B is selected.

What is the root cause of that problem?

We always pass the defaultTaxRate param as policy?.taxRates?.defaultExternalID then the system message is updated wrongly

const categoryTaxCode = getCategoryDefaultTaxRate(taxRules, category, policy?.taxRates?.defaultExternalID);

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

We should use getDefaultTaxCode function that already handled the case foreign currency to get the correct default tax rate.

const categoryTaxCode = getCategoryDefaultTaxRate(taxRules, category, getDefaultTaxCode(policy, transaction, getCurrency(transaction)));

const categoryTaxCode = getCategoryDefaultTaxRate(taxRules, category, policy?.taxRates?.defaultExternalID);

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

We need another test case for getCategoryTaxCodeAndAmount when the transaction currency is different with the policy currency and verify that the default tax rate will be the Foreign tax rate if it exists.

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.

@bernhardoj
Copy link
Contributor

I'm fixing it here

@jasperhuangg
Copy link
Contributor

The PR fixes this issue mostly in the front-end, but we still need a back-end PR to fully solve this issue, so I'm adding the Web blocker label.

@melvin-bot melvin-bot bot added the Overdue label Dec 27, 2024
@sakluger
Copy link
Contributor

I'm going to add reviewing for now since the front-end PR is merged and awaiting deploy. @jasperhuangg what's the plan for the backend fix?

@melvin-bot melvin-bot bot removed the Overdue label Dec 27, 2024
@sakluger sakluger added the Reviewing Has a PR in review label Dec 27, 2024
@jasperhuangg
Copy link
Contributor

cc @MonilBhavsar I remember it was mentioned you would work on the back-end fix, correct?

@MonilBhavsar
Copy link
Contributor

This one does not need a backend change. #54579 should fix it!

@jasperhuangg
Copy link
Contributor

jasperhuangg commented Dec 27, 2024

@MonilBhavsar #54579 (comment) what about this comment? Or is that going to be a separate concern handled elsewhere?

@bernhardoj
Copy link
Contributor

bernhardoj commented Dec 28, 2024

@jasperhuangg I think #54513 is the only one that needs a BE fix. This issue is fully FE.

@jasperhuangg jasperhuangg removed the DeployBlocker Indicates it should block deploying the API label Dec 30, 2024
@sakluger
Copy link
Contributor

sakluger commented Jan 2, 2025

@bernhardoj, just to confirm, no additional payment is required for this issue, right?

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Hourly KSv2 labels Jan 6, 2025
@melvin-bot melvin-bot bot changed the title Tax rate - System message does not show the correct tax rate when category has tax rate [HOLD for payment 2025-01-13] Tax rate - System message does not show the correct tax rate when category has tax rate Jan 6, 2025
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Jan 6, 2025
Copy link

melvin-bot bot commented Jan 6, 2025

Reviewing label has been removed, please complete the "BugZero Checklist".

Copy link

melvin-bot bot commented Jan 6, 2025

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.80-6 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2025-01-13. 🎊

Copy link

melvin-bot bot commented Jan 6, 2025

@grgia @sakluger @grgia The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

@bernhardoj
Copy link
Contributor

@sakluger yes, no payment here.

@sakluger
Copy link
Contributor

sakluger commented Jan 8, 2025

Perfect, thank you for confirming!

@sakluger sakluger closed this as completed Jan 8, 2025
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 Weekly KSv2
Projects
None yet
Development

No branches or pull requests

7 participants