Openfoodnetwork: Nb of units wrongly captured if , used instead of . and make BOM crash

[myriamboure]

Description

As a user of a non anglosaxon country, I use , as decimal separator.
I have created a product "per kg" (like zuchini) and I set up 2 variants, one with nb of unit = 1 (1 kg), the other with nb of unit = 0,5 (displayed 500g)
[I need to use kg and not g if I want for example a shipping fee based on weight, which is the case here]

When I fill "0,5" and save the system transform it to "0 ,5" with a space between the 0 and the ,
See video recording:
https://www.useloom.com/share/3e1147769fbb4ba0a978bec881697461

Actually you will see the behavior is very unfriendly, when I try first from 1 to write 0,5 it doesn't work, back to 1. Then I put 0.5 and it saves correctly. Then I put 0,5 and it works correctly and transform 0,5 to 0.5 as expected. Then I write 0 ,5 and it save but with wrong input without fixing it. Then I write to fix it 0,5 and it doesn't work, save back to 0 ,5... it's a mess !

When 0 ,5 is saved (which happened to a use on French production), when the product is ordered, in BOM it is displayed with a 0 weight/volume, an error message "certain variants don't have a nb of units" and the whole BOM feature is broken for all users.

Expected Behavior

In product catalog:

• If I type in nb of unit with decimals "x,x" it saves and transform it to x.x
• If I type a nb of unit with a space it should display an error message and prevent saving

Actual Behaviour

• Decimal nb of units are wrongly handled if , is used and not .
• if there is a space no error message is displayed and saving happens

Steps to Reproduce

1. Go to products, create a product per kg with a variant with nb unit 1
2. Save, then change with all the cases described above (0,5/0.5/0 ,5) in various orders. See things bugging.
3. When 0 ,5 is saved order that product in an OC.
4. Check the BOM page : you can't modify anymore the volume/weight of any product and you see that for this product volume/weight is to 0

Context

BOM was broken on French production due to a product filled with nb of unit = 0 ,5

Severity

S3, there is a workaround but for non anglosaxon users this will happen again for sure ! User naturally use , for decimals.

Your Environment

• Version used: 1.29
• Browser name and version: Chrome
• Operating System and version (desktop or mobile): Ubuntu 18
• OFN Platform instance where you discovered the bug, and which version of the software they are using : French production, reproduced on Katuma staging.

Original bug report and investigation (for memory): https://github.com/openfoodfoundation/openfoodnetwork/issues/3655

[RachL]

@myriamboure FYI the 0 quantity was noticed by Sally here : https://github.com/openfoodfoundation/openfoodnetwork/issues/1202

[luisramos0]

Tech Analysis:
we are handling decimal separators well in prices because the javascript code is just sending the values (either 12,0 or 12.0) to the server, and the server code is ready to handle the different types of numbers.

here, in the units, we have the scale conversion happening on the browser in javascript before the number is sent to the server. The javascript code is not prepared to handle different numeric representations. We need to write that code in JS or just send the numbers without handling scale to the server...

In the products page this is where the magic happens:
https://github.com/openfoodfoundation/openfoodnetwork/blob/b1d4461c77f959f66e9155618c491d31076db1e5/app/assets/javascripts/admin/bulk_product_update.js.coffee#L231-L239

The same problem happens in the variants edit page if you edit the weight of a variant with something like 150,10.
the problem is the same but in a different place in the JS code that is also not able to handle commas as decimal separator and sends NaN to the server:
https://github.com/openfoodfoundation/openfoodnetwork/blob/b1d4461c77f959f66e9155618c491d31076db1e5/app/assets/javascripts/admin/products/controllers/variant_units_controller.js.coffee#L8-L10

[luisramos0]

I see the code in bulk_product_update.js.coffee#packVariant is unit tested here.
The unit tests help understand what's going on.

In the unit value field you can write something like "2 (bottle)"

and the code will interpert this into separate fields in the database spree_variants:

the scale is kept at the product level, spree_products:

and the option type connection to the variant is in spree_option_values_variants
and the value is in spree_option_values in the field name:

this is wild!!! there are 4 db tables involved and some very smart JS code...

Option 1: The easiest solution for this current separator issue would be to send the values to the server and get the server to handle localized numbers here as well BUT for this we would have to move this smart JS code to the server... some seriously risky work but an opportunity to clean up...
Option 2: The other alternative I see is to make JS code know how to handle localized numbers. That will require the addition of a new JS library to do the work... this is easier BUT we will keep most of the mess as is.

[luisramos0]

above I discuss bulk product update page, this issue will have to be resolved in the BOM as well here

[kristinalim]

@luisramos0 I agree that this is a rather scary issue to fix...

For me, Option 2 sounds good - doing the number localization in the code layer closest to the user, and purely for presentation. So, this would involve an Angular directive for fields and spans in the UI, a helper method for emails, and a parser for user-input numbers in product and inventory import. IMO, this would simplify development because we have to consider only one format for the rest of the code.

[luisramos0]

thanks for your feedback Kristina!

[sauloperez]

considering this was opened by the end of March, can't invest some more time and fix things properly and remove the redundancy at db level? I understand we want to solve things quickly but in this case, is not a life or death situation and IMHO we tend to postpone these clean ups a bit too much some times. I don't have all the details now but option 1 might not exclude improving the presentation logic on the frontend, right?

It comes without saying that this should not be done in one shot.

[luisramos0]

yes Pau, great point! this is not urgent but it is important and thus, a good opportunity to do it correctly.

so, I'm going to move to a product question now: do we need this smart logic in this unit field? @kirstenalarsen @lin-d-hop @RachL

The unit column in the bulk update page is basically taking the value and breaking it into two fields unit_value and unit_description (split using the space as a separator). These fields are shown as separate in the variant edit page:

Would it not be easier for users to have these fields separated in the bulk update page?
If we split the two fields explicitly for the user, we just need to make sure that if the option type is Items (not weight or volume), the unit_description is not editable, only the unit_value.
Is this a possibility?

The reason for my question is that it looks like we will need to change this part of the code, and as we are doing that, there's the opportunity to make things better for the user.

[RachL]

Would it not be easier for users to have these fields separated in the bulk update page?

YES! It would be really great IMO to harmonize this. A field dedicated to a special aspect should have the same "scope" across all pages. That being said, it does not mean we are forced to display both fields on bulk edit product page. This page should show only most regularly-used fields. But I guess we can keep the approach of letting the user choose the number of column for this. Even if it can become very ugly...

[sauloperez]

:100: no need for that fanciness that becomes difficult to manage and brings dubious benefits

[luisramos0]

ok, I moved this out of the "top 5 bugs" for now. it's still a top bug to fix and we can still fix it soon but not as part of this "task force".

[luisramos0]

I added sizeM label to make it obvious why this has still not been prioritised.