Skip to content

fix: price calculation#774

Merged
leoslr merged 4 commits intomasterfrom
fix-price-calculation
Feb 16, 2022
Merged

fix: price calculation#774
leoslr merged 4 commits intomasterfrom
fix-price-calculation

Conversation

@leoslr
Copy link
Contributor

@leoslr leoslr commented Feb 15, 2022

Fixes edge cases where we lost precision.

e.g. :
item.unitPrice = 33.33
item.quantity = 1
item.tax.amount = 20
item.discount = 0

The "true" result is 39.996. This function returned 39.99, it now returns 40.00.

Copy link
Contributor

@alexandre-abrioux alexandre-abrioux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Please add a test case for this in packages/data-format/test/rnf_invoice/utils.test.ts.
  • Do you know where this issue occurs exactly? I'm not sure this is the way to go at it, to me it looks like it's more of a display issue rather than a calculation issue.
    Edit: see comment below

@alexandre-abrioux
Copy link
Contributor

@leoslr I understand now why this issue occurs, just saw your message on Slack. I think it might be a mistake to represent fiat currencies with only 2 decimals. But without changing the way every other components work, you might want to take a look at the FixedNumber class. It does exactly what we need as it has a round method. I did some test, you can do something like this:

  const fixedNumber = FixedNumber.from(item.unitPrice)
    // account for floating quantities
    .mulUnsafe(FixedNumber.from(item.quantity * preciselyOne))
    .divUnsafe(FixedNumber.from(preciselyOne))
    .subUnsafe(FixedNumber.from(discount))
    // Artificially offset the decimal to let the multiplication work
    .mulUnsafe(FixedNumber.from(taxPercent * preciselyOne))
    // Remove the decimal offset
    .divUnsafe(FixedNumber.from(preciselyOne))
    // Remove the percentage multiplier
    .divUnsafe(FixedNumber.from(100))
    .addUnsafe(FixedNumber.from(taxFixed))
    .round(0);
  console.log(fixedNumber.toString());

Just tested real quick but i'm sure you can optimize more, like remove the preciselyOne constant when using FixedNumber as its probably not needed in this case.

@leoslr
Copy link
Contributor Author

leoslr commented Feb 15, 2022

@alexandre-abrioux Nice solution with the FixedNumber class, indeed preciselyOne is not needed anymore. It's weird though, we can instantiate a FixedNumber from a floating string but not a floating number.

I also added the test describing the edge case and it passes. (I checked again but indeed, it was not passing before).

Copy link
Contributor

@alexandre-abrioux alexandre-abrioux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

leoslr and others added 2 commits February 15, 2022 19:05
Co-authored-by: Alexandre ABRIOUX <alexandre-abrioux@users.noreply.github.com>
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.002%) to 89.447% when pulling 9cf92ef on fix-price-calculation into c047df3 on master.

@leoslr leoslr merged commit 6773e0c into master Feb 16, 2022
@leoslr leoslr deleted the fix-price-calculation branch February 16, 2022 07:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants