-
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 2023-10-10] [$500] Don't allow decimals in request amounts if the currency doesn't support decimals #26551
Comments
ProposalPlease re-state the problem we are trying to solveSome currencies are automatically rounded off and decimals are removed while requesting money. What is the root cause of this problem?Some currencies don't allow decimals places to be present in their denominations. We are already getting the decimals allowed by a currency in the VND: {
ISO4217: "704",
decimals: 0,
name: "Vietnam Dong",
symbol: "₫"
} So, if we select this currency the decimals are removed as What changes should be made to fix this?We should not allow the user to enter decimals places for currencies that don't allow it. This can be done by modifying the
const decimals = CurrencyUtils.getCurrencyDecimals(currency);
function validateAmount(amount, decimals) {
const regexString = decimals === 0
? `^(\\d+(,\\d+)?)?$` // don't allow decimal point if decimals === 0
: `^(\\d+(,\\d+)*(\\.\\d{0,${decimals}})?)?$`; // Allow the decimal point and the desired number of digits after the point
const decimalNumberRegex = new RegExp(regexString, 'i');
return amount === '' || (decimalNumberRegex.test(amount) && calculateAmountLength(amount) <= CONST.IOU.AMOUNT_MAX_LENGTH);
}
if (!MoneyRequestUtils.validateAmount(newAmountWithoutSpaces, decimals)) {
function stripDecimalsFromAmount(amount) {
return amount.replace(/\.\d*/, '');
} Then add a useEffect(() => {
if (!MoneyRequestUtils.validateAmount(currentAmount, decimals)) {
setCurrentAmount(MoneyRequestUtils.stripDecimalsFromAmount(currentAmount));
}
}, [currency]); |
Triggered auto assignment to @sakluger ( |
Bug0 Triage Checklist (Main S/O)
|
This is by design. Some currencies do not support decimals.
|
Current assignee @sakluger is eligible for the NewFeature assigner, not assigning anyone new. |
Reopening as a NewFeature. I updated the title and OP to reflect that we should not even allow amounts to be entered with decimals if the selected currency doesn't support decimals. |
I think this has to be internal - in order to determine if a currency supports decimals, we need to access a private repo. |
Job added to Upwork: https://www.upwork.com/jobs/~018f0f0811f6fe308d |
Triggered auto assignment to Contributor Plus for review of internal employee PR - @parasharrajat ( |
Triggered auto assignment to @grgia ( |
cc @aldo-expensify since we discussed in Slack: https://expensify.slack.com/archives/C049HHMV9SM/p1693939667624119?thread_ts=1690213457.772999&cid=C049HHMV9SM |
@esh-g proposal looks good but instead of point 4, we should strip decimals when currency is changed. What will be your plan for that? |
@parasharrajat I have updated the proposal step 4 to reflect the changes you asked for. Please let me know if it looks good. I went with stripping the decimals without checking the decimal places allowed because I don't think we support currencies with denominations other than function stripDecimalsFromAmount(amount, decimals) {
if (!decimals) {
// If 'decimals' is not provided or equals 0, strip all decimals
return amount.replace(/\.\d*/, '');
} else {
// If 'decimals' is provided, strip digits after the specified number of decimal places
const decimalPattern = `\\.\\d{0,${decimals}}`;
const regex = new RegExp(decimalPattern);
return amount.replace(regex, '');
}
} Let me know if I should add this function instead of the one used in the proposal which is: function stripDecimalsFromAmount(amount) {
return amount.replace(/\.\d*/, '');
} |
This component is a little tricky when it comes down to localization. Some languages use I will check your updated proposal in sometime. |
@parasharrajat I have tested for localzation as well. Here is a video of what I think is expected with the current proposal: Screen.Recording.2023-09-06.at.10.05.20.PM.mov |
Bump @parasharrajat |
📣 @parasharrajat Please request via NewDot manual requests for the Reviewer role ($500) |
📣 @esh-g 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app! Offer link |
📣 @priyankrawat We're missing your Upwork ID to automatically send you an offer for the Reporter role. |
Here is the PR: #28137 |
Though I've already added my upwork details, I've also sent a proposal to the above link as mentioned by the bot. |
Hi guys, any updates? Thanks! |
Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:
On to the next one 🚀 |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.76-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 2023-10-10. 🎊 After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.
For reference, here are some details about the assignees on this issue:
As a reminder, here are the bonuses/penalties that should be applied for any External issue:
|
Hi @priyankrawat, sorry but we don't pay out a reporting bonus for New Features. You can see in the original Slack post where we discussed that this is a new feature rather than a bug. The Github automation incorrectly said you were eligible for the reporting bonus because the GH issue was originally labeled as bug. Sorry for the confusion 🙇. For the other payments, I'm going to apply the speed bonus because the delay was due to BE, otherwise it would have been merged on time. Summarizing payouts for this issue: Contributor: @esh-g $750 (paid via Upwork) Above payments include efficiency bonus 🎉 |
@parasharrajat could you please complete the BZ checklist and let me know when you've sent the request for payment? I'd love to close this one out. 🙇 |
New features does not need BZ checklist. Feel free to close it, I can request it later. |
Good call out, my bad! Closing this one out! |
Payment requested as per #26551 (comment) |
$750 payment approved for @parasharrajat based on this comment. |
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!
Action Performed:
Expected Result:
We should return an error stating that decimals are not supported in the selected currency.
Actual Result:
No error is returned and the amount is rounded.
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: 1.3.62.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
Notes/Photos/Videos: Any additional supporting documentation
Bug.1.mov
Recording.4180.mp4
Expensify/Expensify Issue URL:
Issue reported by: @priyankrawat
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1690213457772999
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: