-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Comments
Triggered auto assignment to @muttmuure ( |
ProposalPlease 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 App/src/pages/iou/MoneyRequestAmountForm.tsx Line 217 in 6b6ed03
but we are displaying to the user the App/src/pages/iou/MoneyRequestAmountForm.tsx Line 107 in 6b6ed03
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 App/src/pages/iou/MoneyRequestAmountForm.tsx Line 217 in 6b6ed03
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 |
I reported this bug from another PR. Could I take over this issue as C+? |
ProposalPlease 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. App/src/libs/TransactionUtils.ts Lines 664 to 670 in 525ad6f
Lines 85 to 92 in 525ad6f
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:
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:
We'll update the places where these are used to pass the currency as well. Polishing can also be done. |
ProposalPlease 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 The divisor does not work properly with large amounts with currencies with small decimals. App/src/libs/TransactionUtils.ts Lines 667 to 670 in 525ad6f
In this case, it calculates the
Finally, the What changes do you think we should make to solve the problem?Change the
POC.mov |
@muttmuure Whoops! This issue is 2 days overdue. Let's get this updated quick! |
@muttmuure Eep! 4 days overdue now. Issues have feelings too... |
@muttmuure 6 days overdue. This is scarier than being forced to listen to Vogon poetry! |
@muttmuure 10 days overdue. I'm getting more depressed than Marvin. |
Catching up from OOO |
@muttmuure Can you have a look? |
Working through higher priorities right now |
@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! |
@muttmuure Should this issue be handled externally? |
Thanks for your patience, looking at this now |
Job added to Upwork: https://www.upwork.com/jobs/~0119a2144ed665fa18 |
OK cool, that seems like something we should fix. If we can handle it externally, that is my preference |
This seems to be due for payments. |
cc @muttmuure ^ |
@muttmuure Can you have a look when possible? |
Triggered auto assignment to @anmurali ( |
@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 |
@DylanDylann is paid |
Waiting for @ShridharGoel to accept the offer |
@ShridharGoel can you accept the offer so we can process the payments and close this one please? |
Not overdue. Waiting for @ShridharGoel |
Paid. |
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%
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?
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
Issue Owner
Current Issue Owner: @The text was updated successfully, but these errors were encountered: