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 2024-08-15] [$250] Can't enter maximum tax amount #42601

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

[HOLD for payment 2024-08-15] [$250] Can't enter maximum tax amount #42601

m-natarajan opened this issue May 24, 2024 · 48 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 External Added to denote the issue can be worked on by a contributor

Comments

@m-natarajan
Copy link

m-natarajan commented May 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: 1.4.75-0
Reproducible in staging?: y
Reproducible in production?: y
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: @DylanDylann
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1716540690746529

Action Performed:

Pre requisite: Have a control workspace with tax enabled and with a rate of 12%

  1. As a employee click "submit expense" in the workspace
  2. Enter amount đ20000 (currency: VND)
  3. Select tax rate: 12%
  4. Edit tax amount to đ2143

Expected Result:

The error message : "Maximum tax amount is ₫2,143" should appear only when we enter a mount greater than "đ2143"

Actual Result:

The error message : "Maximum tax amount is ₫2,143" appear when we enter exactly "đ2143"

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

Screen.Recording.2024-05-24.at.15.46.55.mov
Recording.121.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0119a2144ed665fa18
  • Upwork Job ID: 1800124610279105511
  • Last Price Increase: 2024-06-10
  • Automatic offers:
    • DylanDylann | Reviewer | 102728051
    • ShridharGoel | Contributor | 102728052
Issue OwnerCurrent Issue Owner: @
@m-natarajan m-natarajan added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels May 24, 2024
Copy link

melvin-bot bot commented May 24, 2024

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

@dragnoir
Copy link
Contributor

dragnoir commented May 24, 2024

Proposal

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

Wrong rrror for max tax amount

What is the root cause of that problem?

on MoneyRequestAmountForm, when we check the new entered tax amount, we do a calculation between the tax amount and the entered value. If there's an error like "Maximum tax amount is...." we use this function

setFormError(['iou.error.invalidTaxAmount', {amount: formattedTaxAmount}]);

but we are displaying to the user the formattedTaxAmount not the exact taxAmount

const formattedTaxAmount = CurrencyUtils.convertToDisplayString(Math.abs(taxAmount), currency);

So the error message in this issue is "Maximum tax amount is 2143" where it should be "Maximum tax amount is 2142,86"

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

use taxAmount and not formattedTaxAmount here

setFormError(['iou.error.invalidTaxAmount', {amount: formattedTaxAmount}]);

so the error message will display the exact tax amount that should not be surpassed.

We can also edit the error message to inform the user that he need to inter a lower value from formattedTaxAmount

@DylanDylann
Copy link
Contributor

I reported this bug from another PR. Could I take over this issue as C+?

@ShridharGoel
Copy link
Contributor

Proposal

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

Tax amount doesn't use decimals count data because of which faulty values get used.

What is the root cause of that problem?

Some currencies like VND & JPY use 0 decimals.

While calculating the tax amount and also while converting to frontend amount, we are not taking that into consideration and always using 2 decimals.

/**
* this is the formulae to calculate tax
*/
function calculateTaxAmount(percentage: string, amount: number) {
const divisor = Number(percentage.slice(0, -1)) / 100 + 1;
return Math.round(amount - amount / divisor) / 100;
}

/**
* Takes an amount in "cents" as an integer and converts it to a floating point amount used in the frontend.
*
* @note we do not support any currencies with more than two decimal places.
*/
function convertToFrontendAmount(amountAsInt: number): number {
return Math.trunc(amountAsInt) / 100.0;
}

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

Update the calculateTaxAmount method to take decimals in currency into consideration:

function calculateTaxAmount(percentage: string, amount: number, currency: string) {
    const divisor = Number(percentage.slice(0, -1)) / 100 + 1;
    const taxAmount = (amount - amount / divisor) / 100;
    const decimals = getCurrencyDecimals(currency);
    return parseFloat(taxAmount.toFixed(decimals));
}

To be discussed: 12% of 20000 is 2142.86 so we can discuss if we want to use 2143 as the max tax or do we want to lower it to 2142. I think 2143 should be used.

Also, the tax amount is set to 2142.86 by default in the tax form while editing, but it should have been 2143.

This can be fixed by updating convertToFrontendAmount to take currency decimals into account:

function convertToFrontendAmount(amountAsInt: number, currency: string): number {
    const decimals = getCurrencyDecimals(currency);
    return toFixedNumber((Math.trunc(amountAsInt) / 100.0), decimals);
}

We'll update the places where these are used to pass the currency as well.

Polishing can also be done.

@melvin-bot melvin-bot bot added the Overdue label May 26, 2024
@Tony-MK
Copy link
Contributor

Tony-MK commented May 26, 2024

Proposal

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

Can't enter the maximum tax amount

What is the root cause of that problem?

The problem is the calculateTaxAmount function is not correct.

The divisor does not work properly with large amounts with currencies with small decimals.

function calculateTaxAmount(percentage: string, amount: number) {
const divisor = Number(percentage.slice(0, -1)) / 100 + 1;
return Math.round(amount - amount / divisor) / 100;
}

In this case, it calculates the transactionTaxAmount as 2,143 which should be 2,400.

return CurrencyUtils.convertToBackendAmount(TransactionUtils.calculateTaxAmount(taxPercentage, transactionTaxAmount));

Finally, the calculateTaxAmount function is used in the getTaxAmount function seen above in the IOURequestStepTaxAmountPage.

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

Change the calculateTaxAmount function to the one below to give the correct output in all situations for the getTaxAmount function.

function calculateTaxAmount(percentage: string, amount: number) {
    return  Math.round(amount * (Number(percentage.slice(0, -1)) / 100)) / 100;
}
POC.mov

Copy link

melvin-bot bot commented May 27, 2024

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

Copy link

melvin-bot bot commented May 29, 2024

@muttmuure Eep! 4 days overdue now. Issues have feelings too...

Copy link

melvin-bot bot commented May 31, 2024

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

Copy link

melvin-bot bot commented Jun 4, 2024

@muttmuure 10 days overdue. I'm getting more depressed than Marvin.

@muttmuure
Copy link
Contributor

Catching up from OOO

@melvin-bot melvin-bot bot removed the Overdue label Jun 5, 2024
@ShridharGoel
Copy link
Contributor

@muttmuure Can you have a look?

@muttmuure
Copy link
Contributor

Working through higher priorities right now

Copy link

melvin-bot bot commented Jun 7, 2024

@muttmuure @DylanDylann 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 melvin-bot bot added the Overdue label Jun 10, 2024
@DylanDylann
Copy link
Contributor

@muttmuure Should this issue be handled externally?

@melvin-bot melvin-bot bot removed the Overdue label Jun 10, 2024
@muttmuure
Copy link
Contributor

Thanks for your patience, looking at this now

@muttmuure muttmuure added the External Added to denote the issue can be worked on by a contributor label Jun 10, 2024
@melvin-bot melvin-bot bot changed the title Can't enter maximum tax amount [$250] Can't enter maximum tax amount Jun 10, 2024
Copy link

melvin-bot bot commented Jun 10, 2024

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

@muttmuure
Copy link
Contributor

OK cool, that seems like something we should fix. If we can handle it externally, that is my preference

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jun 10, 2024
@ShridharGoel
Copy link
Contributor

This seems to be due for payments.

@francoisl
Copy link
Contributor

cc @muttmuure ^

@ShridharGoel
Copy link
Contributor

@muttmuure Can you have a look when possible?

@mountiny mountiny added Daily KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Reviewing Has a PR in review Monthly KSv2 labels Aug 15, 2024
@mountiny mountiny added Bug Something is broken. Auto assigns a BugZero manager. and removed Bug Something is broken. Auto assigns a BugZero manager. labels Aug 15, 2024
Copy link

melvin-bot bot commented Aug 15, 2024

Triggered auto assignment to @anmurali (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 melvin-bot bot removed the Overdue label Aug 15, 2024
@mountiny mountiny changed the title [$250] Can't enter maximum tax amount [HOLD for payment 2024-08-15] [$250] Can't enter maximum tax amount Aug 15, 2024
@mountiny
Copy link
Contributor

@anmurali Matt is ooo this week and this failed with automation after a deploy, this is ready for payment

$250 to @ShridharGoel and to @DylanDylann

@anmurali
Copy link

@DylanDylann is paid
@ShridharGoel - your offer is here

@melvin-bot melvin-bot bot added the Overdue label Aug 19, 2024
@muttmuure
Copy link
Contributor

Waiting for @ShridharGoel to accept the offer

@melvin-bot melvin-bot bot added Daily KSv2 Overdue and removed Overdue Daily KSv2 labels Aug 19, 2024
@francoisl
Copy link
Contributor

@ShridharGoel can you accept the offer so we can process the payments and close this one please?

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Aug 23, 2024
@DylanDylann
Copy link
Contributor

Not overdue. Waiting for @ShridharGoel

@melvin-bot melvin-bot bot removed the Overdue label Aug 26, 2024
@anmurali
Copy link

Paid.

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 External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests

9 participants