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

Inconsistent rounding truncates smallest digit for mL units #12874

Open
filipefurtad0 opened this issue Sep 24, 2024 · 4 comments · May be fixed by #12970
Open

Inconsistent rounding truncates smallest digit for mL units #12874

filipefurtad0 opened this issue Sep 24, 2024 · 4 comments · May be fixed by #12970
Assignees
Labels
bug-s3 The bug is stopping a critical or non-critical feature but there is a usable workaround.

Comments

@filipefurtad0
Copy link
Contributor

filipefurtad0 commented Sep 24, 2024

Description

Some places in the app are rounding mL units incorrectly.

Expected Behavior

2mL -> should display as 2mL
125mL -> should display as 125mL

Actual Behaviour

2mL -> display as 0mL
125mL -> display as 130mL

Steps to Reproduce

  1. Login as admin and create a mL product with unit value of 2
  2. Take the necessary steps so it appears on the shopfront
  3. As a customer, add this item to the cart and complete an order.
  4. Notice the incorrect rounding on the order confirmation screen, summary step table (maybe this error occurs somewhere else as well...)

Animated Gif/Screenshot

For 0.2 mL -> displayed as 0mL:

Checkout Summary Step

image

Order confirmation Page

image

For 125 mL -> displayed as 130 mL

image

Workaround

Reports, shopfront and cart display the correct rounding.

Severity

bug-s3: a feature is broken but there is a workaround

Your Environment

  • Version used: v4.6.6
  • Browser name and version: all
  • Operating System and version (desktop or mobile): all

Possible Fix

See #12787 (comment)

@github-project-automation github-project-automation bot moved this to All the things 💤 in OFN Delivery board Sep 24, 2024
@filipefurtad0 filipefurtad0 added the bug-s3 The bug is stopping a critical or non-critical feature but there is a usable workaround. label Sep 24, 2024
@sigmundpetersen sigmundpetersen changed the title Inconsistent rounding truncates smalles digit for mL units Inconsistent rounding truncates smallest digit for mL units Oct 9, 2024
@bouaik
Copy link
Contributor

bouaik commented Nov 1, 2024

can i work on this

@sigmundpetersen
Copy link
Contributor

Yes thank you @bouaik , I'll assign you 👍

@sigmundpetersen sigmundpetersen moved this from All the things 💤 to In Progress ⚙ in OFN Delivery board Nov 1, 2024
@bouaik
Copy link
Contributor

bouaik commented Nov 4, 2024

The issue is mainly form the wrong calculation of unit_presentation in spree_line_items, is't calculated from final_weight_volume which is a decimal with 2 numbers after the decimal point

Explication of the issue

image

Also the problem exist for mg units

Notice

I noticed that in line_item model we calculated the unit_value like this
https://github.com/openfoodfoundation/openfoodnetwork/blob/master/app/models/spree/line_item.rb#L224-L228

and the final_weight_volume like this
https://github.com/openfoodfoundation/openfoodnetwork/blob/master/app/models/spree/line_item.rb#L281-L287

Since we already have variant.value in unit_value method, why are we calculating final_weight_volume = variant.value * quantity then getting unit_value = final_weight_volume / quantity ?

Possible fix

I'm suggesting we change the column type to decimal(10, 3) or float if we anticipate smaller units in the future. Also, let's refactor the logic to avoid recalculating unit_value

What do you think guys ?
@rioug

@rioug
Copy link
Collaborator

rioug commented Nov 5, 2024

Hi @bouaik,

It makes sense to update the column to decimal(10, 3). As for the calculations , I am sure I am following what you are saying, I'll just point out that the variant.unit_value might change after the line item is created. So that's probably why final_weight_volume is calculated with the variant.unit_value, and also why 'unit_value' is derived from 'final_weight_volume'. But feel free to give it a go if you think it can be refactored.

@bouaik bouaik linked a pull request Nov 9, 2024 that will close this issue
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug-s3 The bug is stopping a critical or non-critical feature but there is a usable workaround.
Projects
Status: In Progress ⚙
Development

Successfully merging a pull request may close this issue.

4 participants