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

Fix rounding issues by upgrading decimal maths library #12950

Merged

Conversation

macanudo527
Copy link
Collaborator

@macanudo527 macanudo527 commented Oct 29, 2024

What? Why?

BigDecimal was version locked, but with some small modifications to a test and another small modification to display logic, it doesn't need to be. This allows us to use default gems on Alpine Docker images much easier. It also allows dependabot to automatically upgrade the gem when necessary.

What should we test?

  • Visit http://localhost:3000/admin/products
  • Edit any product's unit to be 500.1, which causes a rounding error.
  • Save and verify that the display unit is 500.1 and not 500.09, which happened when we used trunc to create the value instead of round.

Release notes

BigDecimal will now be updated to the latest version more easily.

Changelog Category (reviewers may add a label for the release notes):

  • User facing changes
  • API changes (V0, V1, DFC or Webhook)
  • Technical changes only
  • Feature toggled

The title of the pull request will be included in the release notes.

Copy link
Member

@dacook dacook left a comment

Choose a reason for hiding this comment

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

I confess I've never needed to understand numbers at this level! I've added my thoughts, but I think this is ok as it is.

spec/lib/reports/sales_tax_totals_by_order_spec.rb Outdated Show resolved Hide resolved
spec/lib/reports/sales_tax_totals_by_order_spec.rb Outdated Show resolved Hide resolved
app/services/variant_units/option_value_namer.rb Outdated Show resolved Hide resolved
Copy link
Member

@mkllnk mkllnk left a comment

Choose a reason for hiding this comment

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

Nice one!

I'm glad we have this in a separate pull request now because there's quite a bit to discuss. Awesome work and I think that this update will fix some rounding bugs in our app as well. I just have two little requests to make the code more readable and robust. Otherwise good to go.

spec/lib/reports/sales_tax_totals_by_order_spec.rb Outdated Show resolved Hide resolved
app/services/variant_units/option_value_namer.rb Outdated Show resolved Hide resolved
@mkllnk mkllnk added the user facing changes Thes pull requests affect the user experience label Oct 30, 2024
@mkllnk mkllnk changed the title Remove Version Restriction for BigDecimal Fix rounding issues by upgrading decimal maths library Oct 30, 2024
@macanudo527 macanudo527 force-pushed the unlock_bigdecimal branch 2 times, most recently from ac4aa32 to 511aff9 Compare November 1, 2024 02:00
Copy link
Collaborator

@rioug rioug left a comment

Choose a reason for hiding this comment

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

Nice, thanks @macanudo527 🙏

@macanudo527 macanudo527 force-pushed the unlock_bigdecimal branch 2 times, most recently from 636868a to b79acf3 Compare November 11, 2024 01:08
@macanudo527
Copy link
Collaborator Author

@mklink is there anything else needed for this one?

Copy link
Member

@dacook dacook left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, as Maikel has been offline for the last week.
I can see that his feedback has been addressed, and this PR has already been added to the testing queue, we are just waiting for someone to verify with manual testing.

@filipefurtad0 filipefurtad0 self-assigned this Nov 22, 2024
@filipefurtad0 filipefurtad0 added the pr-staged-uk staging.openfoodnetwork.org.uk label Nov 22, 2024
@filipefurtad0
Copy link
Contributor

Hey @macanudo527,

Same on my side, apologies for the delay in testing this one.

I think it should close #7504. I've spotted this issue in two places in the app (pics taken before staging):

Products page

image

Variant edit page

image

Interestingly, the first test case seems fixed:

image

While the second one does not:

image

I've pulled this PR to my local env and tested it locally, but still obtained the same result. I'm very tempted to merge this as it addresses the first test case. Any idea why the second case is not working @macanudo527 ?

Adding the feedback needed label.

@filipefurtad0 filipefurtad0 added feedback-needed and removed pr-staged-uk staging.openfoodnetwork.org.uk labels Nov 22, 2024
@mkllnk
Copy link
Member

mkllnk commented Nov 25, 2024

Sorry, can you add more context? Which number did you enter and which number did you expect? The screenshots on their own look correct to me.

@macanudo527
Copy link
Collaborator Author

@filipefurtad0 I think the numbers in the db will always be a little off due to float inaccuracies, but when you round up they will go away for the display_as value. The only way to eliminate that is to store the sizes as decimals, but I don't think that is necessary.

@filipefurtad0 filipefurtad0 added pr-staged-fr staging.coopcircuits.fr and removed feedback-needed labels Nov 26, 2024
@filipefurtad0
Copy link
Contributor

@mkllnk @macanudo527, apologies if the pictures are not clear: They relate to two test cases:

Save and verify that the display unit is 500.1 and not 500.09, which happened when we used trunc to create the value instead of round.

  • for a g variant, editing the Unit field to be 500.1 -> should display 500.1, but displays 500.09

I think the numbers in the db will always be a little off due to float inaccuracies, but when you round up they will go away for the display_as value.

Ok, that makes sense. However, I was not expecting that the UI would still display 500.09 after staging the PR. After typing and saving the value or, creating a new variant with the value 500.1, then it is displayed correctly.

Thanks for clarifying, merging!

@filipefurtad0 filipefurtad0 removed the pr-staged-fr staging.coopcircuits.fr label Nov 26, 2024
@filipefurtad0 filipefurtad0 merged commit a493d70 into openfoodfoundation:master Nov 26, 2024
57 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
user facing changes Thes pull requests affect the user experience
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants