-
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-06-20] [HOLD for payment 2024-06-18] [$250] mWeb - IOU - Creating a split expense with IQD currency & 10 digit amount unexpected error shown #42084
Comments
Triggered auto assignment to @trjExpensify ( |
@trjExpensify 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 |
We think that this bug might be related to #vip-split |
ProposalPlease re-state the problem that we are trying to solve in this issue.Creating a new expense with a 10 digit gives an unexpected error. What is the root cause of that problem?This issue not only happens with IQD currency but with any currency that has no decimals, such as JPY. We are validating the amount length here, App/src/libs/MoneyRequestUtils.ts Lines 58 to 64 in f8a30a1
and the default max length is 10. Line 1557 in f8a30a1
We calculate the amount length using App/src/libs/MoneyRequestUtils.ts Lines 43 to 46 in f8a30a1
In However, in the BE, all amounts are in cent, so the 10-digit length will be 12 digit length, and BE will return an error for it. The wrong length calculation logic is changed in #34849 What changes do you think we should make in order to solve the problem?Always multiply the amount with 100 to convert it to cents instead of depending on the decimals allowed on a currency, just like what we did for the first time in #5243. |
Whoops, slipped through the cracks. I agree there should be front-end validation to match here. |
Job added to Upwork: https://www.upwork.com/jobs/~01894cc4ac52447ce7 |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @ikevin127 ( |
This comment was marked as resolved.
This comment was marked as resolved.
@bernhardoj's proposal makes sense to me. The root cause was identified, together with the offending PR and the proposed solution looks solid. 🎀👀🎀 C+ reviewed |
Triggered auto assignment to @marcochavezf, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
@marcochavezf, @trjExpensify, @ikevin127 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues! |
|
Thanks, the analysis makes sense to me too |
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.78-5 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 2024-06-11. 🎊 For reference, here are some details about the assignees on this issue:
|
BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
|
@trjExpensify False alarm, the initial PR was reverted - now we're working on the second one. |
Cool, removing the payment hold. Thanks for the heads up! |
2nd PR was just merged, hopefully this will do without any other regressions🤞 |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.81-11 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 2024-06-18. 🎊 For reference, here are some details about the assignees on this issue:
|
BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.82-4 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 2024-06-20. 🎊 For reference, here are some details about the assignees on this issue:
|
BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
|
Regression Test Proposal
Do we agree 👍 or 👎. |
Summary:
|
Not sure where 4 days ago is coming from, but if we're talking about these two PRs (1, 2), I agree it's due for payment now. Confirming that as follows:
Let me know if I'm missing something, and I'll settle up with you both once confirmed. |
Cool, settled up! |
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Version Number: 1.4.73
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4554848
Issue reported by: Applause - Internal Team
Action Performed:
Expected Result:
If 10 digit amount are not allowed, user must not be allowed to create expense. If user allowed to create expense, then unexpected error must not be shown
Actual Result:
Creating a split expense with IQD currency & 10 digit amount unexpected error displayed
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
Bug6479391_1715607502707.iqd.mp4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @trjExpensifyThe text was updated successfully, but these errors were encountered: