[HOLD for payment 2024-07-26][$500] When amending the amount, if the last digit is a 0, we don't show it.

kavimuru commented 10 months ago

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.29-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: @twisterdotcom Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1705662590072039

Action Performed:

create an expense report
Open the report and edit the amount so that last decimal digit is 0 (e.g. 23.20)
Again open and see the amount

Expected Result:

Should see 0 at the end

Actual Result:

"0" is missing

Workaround:

Platforms:

Which of our officially supported platforms is this issue occurring on?

[ ] Android: Native
[ ] Android: mWeb Chrome
[ ] iOS: Native
[ ] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/43996225/9bb584e7-49a7-4968-ab33-db9e71b281f8

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~0178a9407eab9689fe
Upwork Job ID: 1749451650573455360
Last Price Increase: 2024-01-29
Automatic offers:

abzokhattab | Contributor | 28129136

Issue Owner

Current Issue Owner: @eVoloshchak

melvin-bot[bot] commented 10 months ago

Job added to Upwork: https://www.upwork.com/jobs/~0178a9407eab9689fe

melvin-bot[bot] commented 10 months ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @eVoloshchak (External)

paultsimura commented 10 months ago

This looks like a pretty normal behavior for decimals, isn't that expected?🤔

abzokhattab commented 10 months ago

Proposal

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

if the last digit is a 0 in amount we dont show it

What is the root cause of that problem?

in this function whe dividing over 100 the last digit is removed if its 0 https://github.com/Expensify/App/blob/82b76c1a36cf1a01b00c14510a5812a591e7f2fd/src/libs/CurrencyUtils.ts#L91-L93

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

we can force it to always return two decimal digits after the comma since the max allowed numbers of digits after the comma is two:

function convertToFrontendAmount(amountAsInt: number): string {
    const amount = Math.trunc(amountAsInt) / 100.0;
    return amount.toFixed(2);
}

Or we can just check if the last digit is not then return the amount as in the old calculations but i would prefer not to have edge cases.

cjoshidev commented 10 months ago

Proposal

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

If the trailing digit is 0 then it is getting truncated. Show 0 digits upto 2 decimals for amounts having significant number after the decimal

What is the root cause of that problem?

in this function when dividing over 100 the last digit is removed if its 0 https://github.com/Expensify/App/blob/82b76c1a36cf1a01b00c14510a5812a591e7f2fd/src/libs/CurrencyUtils.ts#L91-L93

in the above function trailing 0 is getting truncated which is the root cause of the problem ``

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

We can make changes in the code such it will keep the trailing zero in case there is any significant digit after decimal and not in case it is a whole number which will be the edge case scenario.

So based on the bug if it is 23.10 it should display the same in amount input without truncating.

and also in my solution for eg. 24.00 will be shown as 24 since trailing zeros after decimals don't have any significant digits.

keep the definition of the function intact without changing its return type.

function convertToFrontendAmount(amountAsInt: number): number {
    // Convert the input integer to a floating-point number by dividing it by 100.0
    const amount = Math.trunc(amountAsInt) / 100.0;
    // Check if the decimal part is zero, then remove it
    if (amount % 1 === 0) {
        return amount;
    }
    // Return the resulting floating-point number
    return Number(amount.toFixed(2));
}

What alternative solutions did you explore? (Optional)

allgandalf commented 10 months ago

I agree with @paultsimura and @cjoshi-zeals, thinking of UX, it would also help users to change the value more conveniently as compared to having to cancel the 0 and input a digit again :)

melvin-bot[bot] commented 10 months ago

📣 @Aditya-svg-code! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

Make sure you've read and understood the contributing guidelines.
Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Format:
```
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>
```

Aditya-svg-code commented 10 months ago

Proposal

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

when dividing the number by 100.0 it's unable to display the 0 after the decimal.

What is the root cause of that problem?

in this function when dividing over 100.0 the unit digit is removed if it is 0. for eg. if amountAsInt = 2370 then 2370/100.0 = 23.7 but we want 23.70

App/src/libs/CurrencyUtils.ts

Lines 91 to 93 in 82b76c1

 function convertToFrontendAmount(amountAsInt: number): number { 
     return Math.trunc(amountAsInt) / 100.0; 
 }

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

When you perform a division operation, the result is often displayed with the minimum required precision. To format the result with a specific number of decimal places, you can use the toFixed() method.

toFixed() method returns a string, so if you need to perform further calculations, you may need to convert it back to a number using parseFloat(). Here's how you can fix the issue.

function convertToFrontendAmount(amountAsInt: number): number {
    const result = Math.trunc(amountAsInt) / 100.0;
    return parseFloat(result.toFixed(2));
}

Additionally, if amountAsInt = 2300 then the previous function was providing the result as 23.0 only, but in my solution it will return 23.00 as the result, usually in bills we should show the numbers after the decimal point even if it is 0, it helps the customer understand the visibility of the payment.

Aditya-svg-code commented 10 months ago

Contributor details : Expensify account email: soni96996@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~015162f5f4b61238ea

melvin-bot[bot] commented 9 months ago

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

twisterdotcom commented 9 months ago

So, this was reported by me (which is why I added the label instead of taking it on), but I disagree that we shouldn't show the last 0. I agree 12.30 is the same as 12.3, but I found it odd that we didn't show the last character. If you asked your mate or your company to send you $12.30, you wouldn't ask them to send you $12.3.

eVoloshchak commented 9 months ago

I agree with @twisterdotcom, this doesn't look consistent https://github.com/Expensify/App/assets/9059945/00e62fe4-c2b1-492e-9c34-d2495b486bb2 Banking apps also do this, if you put 12.3 it will still be displayed as 12.30 on the next step

@abzokhattab, I think this is the best proposal so far, but it has one problem - it changes the return type of convertToFrontendAmount from number to string. We need it to be number to calculate isTaxAmountInvalid

@cjoshi-zeals, your proposal doesn't work for me, could you double-check it please?

and also in my solution for eg. 24.00 will be shown as 24 since trailing zeros after decimals don't have any significant digits

We should display 24.00 in this case. 24 is displayed as 24.00 on the previous screen, those two should be identical

https://github.com/Expensify/App/assets/9059945/faedb657-a3cf-4fa0-8167-985d9d4b5037

@Aditya-svg-code, your proposal also doesn't work for me, could you double-check?

abzokhattab commented 9 months ago

@abzokhattab, I think this is the best https://github.com/Expensify/App/issues/34894#issuecomment-1904253989 so far, but it has one problem - it changes the return type of convertToFrontendAmount from number to string. We need it to be number to calculate isTaxAmountInvalid

converting the output to a number will remove the leading zeros, thus we can handle this case ( isTaxAmountInvalid individually by converting it into a number

we can do that by making the following two functions:

function convertToFrontendAmountAsString(amountAsInt: number): string {
    return convertToFrontendAmount(number).toFixed(2);
}

function convertToFrontendAmount(amountAsInt: number): number {
 return Math.trunc(amountAsInt) / 100.0;
}

then in the isTaxAmountInvalid we need to use the convertToFrontendAmount method and in other occurrences we can use the other method

cjoshidev commented 9 months ago

We should display 24.00 in this case. 24 is displayed as 24.00 on the previous screen, those two should be identical

@eVoloshchak

In that case, we just need to remove the condition for checking if there is no significant digit after the decimal, here is my updated solution

in the below solution will keep the return type of the function the same and will also deliver the desired output.

function convertToFrontendAmount(amountAsInt: number): number {
    // Convert the input integer to a floating-point number by dividing it by 100.0
    const amount = Math.trunc(amountAsInt) / 100.0;

    // Return the resulting floating-point number
    return Number(amount.toFixed(2));
}

Aditya-svg-code commented 9 months ago

I agree with @twisterdotcom, this doesn't look consistent https://github.com/Expensify/App/assets/9059945/00e62fe4-c2b1-492e-9c34-d2495b486bb2 Banking apps also do this, if you put 12.3 it will still be displayed as 12.30 on the next step

@abzokhattab, I think this is the best proposal so far, but it has one problem - it changes the return type of convertToFrontendAmount from number to string. We need it to be number to calculate isTaxAmountInvalid

@cjoshi-zeals, your proposal doesn't work for me, could you double-check it please?

and also in my solution for eg. 24.00 will be shown as 24 since trailing zeros after decimals don't have any significant digits

We should display 24.00 in this case. 24 is displayed as 24.00 on the previous screen, those two should be identical

Screen.Recording.2024-01-24.at.23.57.22.mov @Aditya-svg-code, your proposal also doesn't work for me, could you double-check?

@eVoloshchak I have double checked my solution and the issue is that converting it to the number and the leading zero after decimal is getting removed but if we keep it string then it is working correctly, and for the further calculation we will create a function convertToFrontendAmountAsNumber() that return a number for the calculation.

This is the function which will return string for the bill purpose.

function convertToFrontendAmount(amountAsInt: number): string { 
    const result = Math.trunc(amountAsInt) / 100.0;
    return (result.toFixed(2));              //return the value of result as string
}

This is the function which will return number for the furthur calculation.

 function convertToFrontendAmountAsNumber(amountAsInt: number): number { 
     return Math.trunc(amountAsInt) / 100.0; 
 }

in isTaxAmountInvalid we need to use convertToFrontendAmountAsNumber() to get a number for calculation.

sakluger commented 9 months ago

I agree with @twisterdotcom - dollar amounts should always be reflected with two decimals.

allgandalf commented 9 months ago

Proposal

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

When amending the amount, if the last digit is a 0, it isn't displayed

What is the root cause of that problem?

In initializeAmount, we pass the variable newAmount to the function convertToFrontendAmount

https://github.com/Expensify/App/blob/0d0b0a8c9b073c30a9b54de15066789d17f7c718/src/pages/iou/steps/MoneyRequestAmountForm.js#L121-L122

But here as we pass a numerical value, anything of the format 9.70, 9.700, 9.7000 will be considered as 9.7 by default by javascript variables:

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

Instead of updating the currencyUtil, i propose that we update initializeAmount to fix the amount to 2 decimal places by using toFixed(2) as shown below:

const initializeAmount = useCallback((newAmount) => {
    const frontendAmount = newAmount ? 
        (newAmount.includes('.') ? parseFloat(CurrencyUtils.convertToFrontendAmount(newAmount)).toFixed(2) : CurrencyUtils.convertToFrontendAmount(newAmount)) 
        : '';

Result:

Screencast from 01-26-2024 12:22:18 AM.webm

What alternative solutions did you explore? (Optional)

N/A

allgandalf commented 9 months ago

@abzokhattab, I think this is the best https://github.com/Expensify/App/issues/34894#issuecomment-1904253989 so far, but it has one problem - it changes the return type of convertToFrontendAmount from number to string. We need it to be number to calculate isTaxAmountInvalid

@eVoloshchak , By taking into consideration your comments on above proposals, i have proposed a different solution which doesn't have to deal with the CurrencyUtils, looking forward to your review :)

Proposal

eVoloshchak commented 9 months ago

Instead of updating the currencyUtil, i propose that we update initializeAmount to fix the amount to 2 decimal places by using toFixed(2) as shown below:

@RohanSasne, this adds a small regression, amount is altered (formatted) when you're navigated to the next screen:

Plus button -> Request money
Enter an amount (for instance, 24)
Click 'Next'
Click 'Back'
Notice the amount is displayed as '24.00'

https://github.com/Expensify/App/assets/9059945/f1a66571-f02f-4f90-8b02-b193d7c49503

eVoloshchak commented 9 months ago

I think we should proceed with @abzokhattab's updated proposal There's only a single occurrence when we need amount to have a number type, moving it into a separate function is simple and reliable

🎀👀🎀 C+ reviewed!

melvin-bot[bot] commented 9 months ago

Triggered auto assignment to @dangrous, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

allgandalf commented 9 months ago

@RohanSasne, this adds a small regression, amount is altered (formatted) when you're navigated to the next screen: So, this was reported by me (which is why I added the label instead of taking it on), but I disagree that we shouldn't show the last 0. I agree 12.30 is the same as 12.3, but I found it odd that we didn't show the last character. If you asked your mate or your company to send you $12.30, you wouldn't ask them to send you $12.3.

Hey @eVoloshchak just a small doubt , i guess by the comments from @twisterdotcom , this is what is intended !? in banking transactions, we take $12 dollars as $12.00 :thinking: , so won't it be appropriate to have it here too ?

In the screen recording you attached, there too we display it as 23.00:

Currently also on staging, you can see that when we enter value as 23 it is displayed as 23.00 (which is intended) :) so won't my proposal be a more optimal fix for this bug?

melvin-bot[bot] commented 9 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

dangrous commented 9 months ago

Yeah I think @abzokhattab's is probably the clearest moving forward. @RohanSasne you're not wrong here, it's not quite consistent, but I think if the user has JUST entered a value, we'd want to maintain it as is, making it easier for the user to fix if needed. (@sakluger or @twisterdotcom let me know if you disagree). This is also how Square Cash and Venmo do it, as a reference.

Two notes:

Let's be sure to add a test for this in CurrencyUtilsTest
We might want the new function to return an empty string if there's no value, so we can replace the full amount ? CurrencyUtils.convertToFrontendAmount(amount).toString() : '' line that's used, though I defer to your judgement on that.

melvin-bot[bot] commented 9 months ago

📣 @abzokhattab 🎉 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 📖

sakluger commented 9 months ago

Hi @abzokhattab, how is the PR coming?

abzokhattab commented 9 months ago

Working on it … thanks

dangrous commented 9 months ago

hey @eVoloshchak @abzokhattab how are we looking? Seems like there's one bug we still need to fix, and then some merge conflicts. Can we get this ready for final review / merging by Monday or Tuesday? Thank you!

dangrous commented 9 months ago

Bumping this when you can, can we get this updated and rereviewed on Monday please? Thanks!

abzokhattab commented 9 months ago

Sorry for the delay .. working on the latest comments today

dangrous commented 9 months ago

How are we looking on this @abzokhattab ? Can we get it reviewed today, do you think?

abzokhattab commented 9 months ago

I covered the last requested requirement and its still pending on the review

melvin-bot[bot] commented 8 months ago

This issue has not been updated in over 15 days. @dangrous, @sakluger, @eVoloshchak, @abzokhattab eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

melvin-bot[bot] commented 7 months ago

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

iwiznia commented 7 months ago

Had to revert the PR due to https://github.com/Expensify/App/issues/39599

eVoloshchak commented 7 months ago

@abzokhattab, this bug was caused by a conflict when merging main, the commit at fault is https://github.com/Expensify/App/commit/bd6868207f1e8e03899515721ed6899eb50cf172

melvin-bot[bot] commented 7 months ago

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

dangrous commented 7 months ago

Hrm okay let's see if we can figure out a new way forward that doesn't cause that blocker (or https://github.com/Expensify/App/issues/39537)

allgandalf commented 7 months ago

I updated the condition in my proposal a little now and this can fix the issue without updating any utility function imo:

const initializeAmount = useCallback((newAmount) => {
    const frontendAmount = newAmount ? 
        (newAmount.includes('.') ? parseFloat(CurrencyUtils.convertToFrontendAmount(newAmount)).toFixed(2) : CurrencyUtils.convertToFrontendAmount(newAmount)) 
        : '';

Also regression pointed by @eVoloshchak would also be fixed here, @abzokhattab can you try with this please

abzokhattab commented 7 months ago

I will be providing a PR today. thanks

melvin-bot[bot] commented 7 months ago

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

melvin-bot[bot] commented 7 months ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.4.60-13 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

https://github.com/Expensify/App/pull/35797

If no regressions arise, payment will be issued on 2024-04-15. :confetti_ball:

For reference, here are some details about the assignees on this issue:

@eVoloshchak requires payment through NewDot Manual Requests
@abzokhattab requires payment automatic offer (Contributor)

melvin-bot[bot] commented 7 months ago

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:

[ ] [@eVoloshchak] The PR that introduced the bug has been identified. Link to the PR:
[ ] [@eVoloshchak] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
[ ] [@eVoloshchak] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
[ ] [@eVoloshchak] Determine if we should create a regression test for this bug.
[ ] [@eVoloshchak] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
[ ] [@sakluger] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

dangrous commented 7 months ago

How are we looking on the updated fix for this? Thanks!

abzokhattab commented 7 months ago

working on a fix, thanks for the ping

abzokhattab commented 7 months ago

the PR is ready https://github.com/Expensify/App/pull/40062 please let me know if you have any other comments!

sakluger commented 7 months ago

I removed the payment hold since the original PR caused a regression and we now have a new PR (https://github.com/Expensify/App/pull/40062) in review.

On that new PR, there is no BE reviewer. @eVoloshchak @dangrous is that correct?

sakluger commented 7 months ago

I'm also going to throw this in wave-collect. It could possibly be vsb, but I think collect is more appropriate.

melvin-bot[bot] commented 6 months ago

@dangrous, @sakluger, @eVoloshchak, @abzokhattab Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] commented 6 months ago

@dangrous, @sakluger, @eVoloshchak, @abzokhattab Eep! 4 days overdue now. Issues have feelings too...

Expensify / App