-
-
Notifications
You must be signed in to change notification settings - Fork 710
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
Nb of units wrongly captured if , used instead of . and make BOM crash #3660
Comments
@myriamboure FYI the 0 quantity was noticed by Sally here : #1202 |
Tech Analysis: 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: openfoodnetwork/app/assets/javascripts/admin/bulk_product_update.js.coffee Lines 231 to 239 in b1d4461
The same problem happens in the variants edit page if you edit the weight of a variant with something like 150,10. openfoodnetwork/app/assets/javascripts/admin/products/controllers/variant_units_controller.js.coffee Lines 8 to 10 in b1d4461
|
I see the code in bulk_product_update.js.coffee#packVariant is unit tested here. 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 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... |
above I discuss bulk product update page, this issue will have to be resolved in the BOM as well here |
@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. |
thanks for your feedback Kristina! |
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. |
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? 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. |
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... |
💯 no need for that fanciness that becomes difficult to manage and brings dubious benefits |
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". |
I added sizeM label to make it obvious why this has still not been prioritised. |
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:
Actual Behaviour
Steps to Reproduce
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
Original bug report and investigation (for memory): #3655
The text was updated successfully, but these errors were encountered: