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 2023-10-10] [$500] Don't allow decimals in request amounts if the currency doesn't support decimals #26551

Closed
1 of 6 tasks
kbecciv opened this issue Sep 2, 2023 · 38 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item.

Comments

@kbecciv
Copy link

kbecciv commented Sep 2, 2023

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:

  1. Open anything that involves money. (Request money, split bill etc).
  2. Write any amount in decimal in a currency that disallows decimals (i.e. ISK or MGA)
  3. Click next

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?

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

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
  • Upwork Job URL: https://www.upwork.com/jobs/~018f0f0811f6fe308d
  • Upwork Job ID: 1699169106621186048
  • Last Price Increase: 2023-09-25
  • Automatic offers:
    • esh-g | Contributor | 26858302
@kbecciv kbecciv added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Sep 2, 2023
@esh-g
Copy link
Contributor

esh-g commented Sep 2, 2023

Proposal

Please re-state the problem we are trying to solve

Some 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 ONYXKEYS.CURRENCY_LIST.
Example:

VND: { 
  ISO4217: "704",
  decimals: 0,
  name: "Vietnam Dong",
  symbol: "₫"
}

So, if we select this currency the decimals are removed as decimals : 0.

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 MoneyRequestUtils.validateAmount() method.

  1. function MoneyRequestAmountForm({amount, currency, isEditing, forwardedRef, onCurrencyButtonPress, onSubmitButtonPress}) {
    const {isExtraSmallScreenHeight} = useWindowDimensions();
    const {translate, toLocaleDigit, numberFormat} = useLocalize();
    const textInput = useRef(null);
    const selectedAmountAsString = amount ? CurrencyUtils.convertToFrontendAmount(amount).toString() : '';
    const [currentAmount, setCurrentAmount] = useState(selectedAmountAsString);

    In the MoneyRequestAccountForm we should get the decimals allowed for the currency in the body of the component like this:
const decimals = CurrencyUtils.getCurrencyDecimals(currency);
  1. function validateAmount(amount) {
    const decimalNumberRegex = new RegExp(/^\d+(,\d+)*(\.\d{0,2})?$/, 'i');
    return amount === '' || (decimalNumberRegex.test(amount) && calculateAmountLength(amount) <= CONST.IOU.AMOUNT_MAX_LENGTH);
    }

    Modify the MoneyRequestUtils.validateAmount() to take two arguments instead of one: amount and decimals. Then, use a dynamic regex to validate the amount according to the decimals like this:
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);
}
  1. if (!MoneyRequestUtils.validateAmount(newAmountWithoutSpaces)) {

    Modify the use of the validateAmount() function and pass the decimals we got in step 1 to the function like this:
if (!MoneyRequestUtils.validateAmount(newAmountWithoutSpaces, decimals)) {
  1. We can also add a function to strip the decimals places if the currentAmount if it doesn't match the decimals when the currency is changed. Like we have these functions here:
    */
    function stripCommaFromAmount(amount) {
    return amount.replace(/,/g, '');
    }
    /**
    * Strip spaces from the amount
    *
    * @param {String} amount
    * @returns {String}
    */
    function stripSpacesFromAmount(amount) {
    return amount.replace(/\s+/g, '');
    }
function stripDecimalsFromAmount(amount) {
    return amount.replace(/\.\d*/, '');
}

Then add a useEffect to MoneyRequestAccountForm to use the function on currency change:

useEffect(() => {
        if (!MoneyRequestUtils.validateAmount(currentAmount, decimals)) {
            setCurrentAmount(MoneyRequestUtils.stripDecimalsFromAmount(currentAmount));
        }
    }, [currency]);

@melvin-bot
Copy link

melvin-bot bot commented Sep 2, 2023

Triggered auto assignment to @sakluger (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@melvin-bot
Copy link

melvin-bot bot commented Sep 2, 2023

Bug0 Triage Checklist (Main S/O)

  • This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • This bug is not a duplicate report (check E/App issues and #expensify-bugs)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • This bug is reproducible using the reproduction steps in the OP. S/O
    • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
    • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
  • This issue is filled out as thoroughly and clearly as possible
    • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
  • I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

@sakluger
Copy link
Contributor

sakluger commented Sep 5, 2023

This is by design. Some currencies do not support decimals.

  • ISK: This currency has been this way in our system for at least four years(I found a 4 year old internal PR where ISK had 0 decimal places)
  • MGA: As of 2021 the unit is effectively obsolete, since one iraimbilanja is worth less than USD $0.005 and the coins have fallen into disuse. https://en.wikipedia.org/wiki/Malagasy_ariary

@sakluger sakluger closed this as completed Sep 5, 2023
@sakluger sakluger added NewFeature Something to build that is a new item. and removed Bug Something is broken. Auto assigns a BugZero manager. labels Sep 5, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 5, 2023

Current assignee @sakluger is eligible for the NewFeature assigner, not assigning anyone new.

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Sep 5, 2023
@sakluger sakluger changed the title Web - Amount is being rounded in few currencies Don't allow decimals in request amounts if the currency doesn't support decimals Sep 5, 2023
@sakluger
Copy link
Contributor

sakluger commented Sep 5, 2023

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.

@sakluger sakluger reopened this Sep 5, 2023
@sakluger
Copy link
Contributor

sakluger commented Sep 5, 2023

I think this has to be internal - in order to determine if a currency supports decimals, we need to access a private repo.

@sakluger sakluger added the Internal Requires API changes or must be handled by Expensify staff label Sep 5, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 5, 2023

Job added to Upwork: https://www.upwork.com/jobs/~018f0f0811f6fe308d

@melvin-bot
Copy link

melvin-bot bot commented Sep 5, 2023

Triggered auto assignment to Contributor Plus for review of internal employee PR - @parasharrajat (Internal)

@melvin-bot
Copy link

melvin-bot bot commented Sep 5, 2023

Triggered auto assignment to @grgia (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@sakluger
Copy link
Contributor

sakluger commented Sep 5, 2023

@parasharrajat
Copy link
Member

@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?

@esh-g
Copy link
Contributor

esh-g commented Sep 6, 2023

@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 1 or 0.01. But still if we want to strip only the places with respect to the decimals provided we can use this function:

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*/, '');
}

@parasharrajat
Copy link
Member

This component is a little tricky when it comes down to localization. Some languages use , for decimal. So let's make sure we are manipulating the base amount not the represenational value.

I will check your updated proposal in sometime.

@esh-g
Copy link
Contributor

esh-g commented Sep 6, 2023

@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

@esh-g
Copy link
Contributor

esh-g commented Sep 13, 2023

Bump @parasharrajat

@aldo-expensify aldo-expensify added the Reviewing Has a PR in review label Sep 25, 2023
@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 25, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 25, 2023

📣 @parasharrajat Please request via NewDot manual requests for the Reviewer role ($500)

@melvin-bot
Copy link

melvin-bot bot commented Sep 25, 2023

📣 @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
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@melvin-bot
Copy link

melvin-bot bot commented Sep 25, 2023

📣 @priyankrawat We're missing your Upwork ID to automatically send you an offer for the Reporter role.
Once you apply to the Upwork job, your Upwork ID will be stored and you will be automatically hired for future jobs!

@esh-g
Copy link
Contributor

esh-g commented Sep 25, 2023

Here is the PR: #28137
@parasharrajat @aldo-expensify

@priyankrawat
Copy link

Though I've already added my upwork details, I've also sent a proposal to the above link as mentioned by the bot.

@priyankrawat
Copy link

📣 @priyankrawat We're missing your Upwork ID to automatically send you an offer for the Reporter role. Once you apply to the Upwork job, your Upwork ID will be stored and you will be automatically hired for future jobs!

Hi guys, any updates? Thanks!

@melvin-bot
Copy link

melvin-bot bot commented Oct 2, 2023

Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:

  • when @esh-g got assigned: 2023-09-25 15:07:41 Z
  • when the PR got merged: 2023-10-02 12:32:31 UTC
  • days elapsed: 4

On to the next one 🚀

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Oct 3, 2023
@melvin-bot melvin-bot bot changed the title [$500] Don't allow decimals in request amounts if the currency doesn't support decimals [HOLD for payment 2023-10-10] [$500] Don't allow decimals in request amounts if the currency doesn't support decimals Oct 3, 2023
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Oct 3, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 3, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Oct 3, 2023

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.

  • External issue reporter
  • Contributor that fixed the issue
  • Contributor+ that helped on the issue and/or PR

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:

  • Merged PR within 3 business days of assignment - 50% bonus
  • Merged PR more than 9 business days after assignment - 50% penalty

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Oct 10, 2023
@sakluger
Copy link
Contributor

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)
Contributor+: @parasharrajat $750 (payable via Manual Request after BZ checklist is completed)

Above payments include efficiency bonus 🎉
Upwork job: https://www.upwork.com/jobs/~018f0f0811f6fe308d

@melvin-bot melvin-bot bot added the Overdue label Oct 16, 2023
@sakluger
Copy link
Contributor

@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. 🙇

@melvin-bot melvin-bot bot removed the Overdue label Oct 16, 2023
@parasharrajat
Copy link
Member

parasharrajat commented Oct 16, 2023

New features does not need BZ checklist. Feel free to close it, I can request it later.

@sakluger
Copy link
Contributor

Good call out, my bad! Closing this one out!

@parasharrajat
Copy link
Member

Payment requested as per #26551 (comment)

@JmillsExpensify
Copy link

$750 payment approved for @parasharrajat based on this comment.

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 Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item.
Projects
None yet
Development

No branches or pull requests

8 participants