Allow US units on products/variants #5888
Conversation
|
Hrrrm, that test was passing on my build.... is there any chance that's a spurious failure? |
I restarted the build, and it's passing now 👍 |
app/assets/javascripts/admin/products/services/variant_unit_manager.js.coffee
Show resolved
Hide resolved
There was a problem hiding this comment.
Thanks @andrewpbrett, this looks really promising! Could you add some more test coverage here on the Ruby side, for the changes in option_value_namer.rb and variant_and_line_item_naming.rb?
|
I put a couple tests in there to start; I'm still sort of sussing out the conventions around testing and what we want to write as unit tests... feedback welcome in that department :) |
andrewpbrett
force-pushed
the
imperial-auto-units
branch
from
db0a88b
to
c2d3650
Compare
|
On the bulk order management page, all adjustments to the weight of a line item currently have to be done in grams. It would be nice if the weights here were scaled in the same way that the line item's variant is scaled, to avoid having to convert things manually to grams. I don't know how much this functionality is used though, or if it's worth holding off on releasing the ability to have US units on products/variants until we can also have US units on this screen. |
|
I started by just giving this PR a try and play with the app. I notice that the calculator WEIGHT is always per kg. We need the calculators to be per lb right? It will be very weird to use this as is, products in lbs and ship fee per kg. I think can be done in an separate issue/PR. I have verified that ship method is calculated correctly, it will convert the weight calculator price and use it for lbs. The problem I found is that the weight calculator is not working for Enterprise Fees. It think it takes lbs to be grams I think. Editing order quantities in the backoffice will update the ship fees according to weight in lbs correctly 👍 In BOM, changing quantities again, also works well 👍 But, like the calculator in Kgs, it does work well and I can change the final weight volume of the 1lb product from 453.999 to 1000 and it will take 1000g for the calculation of the fee 👍 Quick summary of what I see in terms of features: imperial units are available to be used and display correctly in the shopfront 🎉 I think these 3 problems can be addresses in future PRs 🎉 Now I need to look at the code and then think about test coverage. I may be able to do this second part of the review this weekend. |
|
To answer your previous comment about BOM Andy: one improvement couldd be to change the title of the column from "Weight/Volume (g)" to "Weight/Volume (in grams)" so that users don't miss it. |
There was a problem hiding this comment.
Hi Andy, I have created a PR on top of your branch, I fixed some rubocop issues as I was going through the code:
https://github.com/andrewpbrett/openfoodnetwork/pull/1/files
I have also moved option_value_namer and variant_and_line_item_naming to app/services where they belong. There's a lot to clean up in this space, this is just a minor improvement so we keep the boy scout rule and do a little refactoring in every change.
I m going to approve this PR as is (even better if you merge my PR into your branch :-)).
Anyway, imo this is good enough 🎉 I am approving it.
app/assets/javascripts/admin/products/services/option_value_namer.js.coffee
Show resolved
Hide resolved
|
Hi Luis, I merged your PR, thanks for the improvements. I'm going to work this week on the shipping calculator being in pounds and hopefully some of the other code cleanliness improvements we've talked about above, but I agree they can be in a separate branch and we can start having people test these changes as they are 🎉 |
See: https://community.openfoodnetwork.org/t/hubs-managers-can-choose-the-adapted-weight-and-measure-units-for-their-shops-given-their-own-local-situation/1289/11 We're not entirely sure what needs to be changed in order for this to accurately work with shipping and other parts of the eCommerce platform. We are assuming that so long as we canonically store the weight scale in grams, that the shipping calculation will be able to do what it needs to. So if we put in values for "oz" as grams, we may not need to do much else in order to let product(s) be sold by the pound (or ounce). Next steps appear to be: - [ ] When looking at an order as a customer, do we want to show pounds instead of grams? (See: http://localhost:3000/orders/R125684626) - [ ] Compile a list of tests that are worth writing (because we have no confidence that we know what we are supposed to be doing in order for this feature to be "ready" to be used by people.) - [ ] Write a test that demonstrates when we create a product with a variant in pound that the product's shipping weight is correctly calculated? - [ ] Do we want to think about i18n?
We are pretty sure this is not the correct final implementation, but we wanted to get some tests failing so we can start to fix them.
This changes how we display the description of weight, but it doesn't change the `Spree::OptionValue`s that are being created when someone adds a product to their cart. This takes us closer by making the UI look more correct; but it feels odd compared to settiong the `Spree::OptionValue` to the correct unit on creation. But on the other hand, that could possibly make things worse for the shipping calculation bits.
There was a problem hiding this comment.
This is awesome! thanks @andrewpbrett and @luisramos0 👏 !
LGTM although we could add specific unit tests for variant_and_line_item_naming.rb. I'd make me feel safer if we ever refactor this logic. I'm happy to do that in a separate PR as according to Simplecov the methods we touched get covered already.
|
Thanks for the review @sauloperez! And let me know if you want to collaborate on the refactoring PR. |
|
Hi @andrewpbrett , Thanks for pointing me out to the discourse post - very helpful indeed. I went through it and performed the following test cases: i) expect lb and oz units to appear in
ii) expect unit conversion to work correctly, in bulk product edit page From a previously created product in (1) Kg Interchanging from kg to lb and to oz works:
Converting between lb and oz works as well, ex:
iii) expect product import to accept new units in the field
This currently fails for oz and lb - I verified it works correctly for kg and g currently, so I'll open a new issue on this regard. iv) expect to be able to place orders containing items with units of mixed measurement systems Works fine and units appear displayed correctly. v) expect enterprise fee weight calculator to work correctly with lb and oz Verified that enterprise fee weight calculator only includes the kg unit:
kg product: 1kg
All good here, I think. What did you find not to be working before @luisramos0 ? vi) expect shipping fee weight calculator to work correctly with lb and oz Verified that shipping fee weight calculator only includes the kg unit [9.9792 kg ( = 2.2 lb) + 4.536 kg ( = 16 oz) ] x 2 UK-pounds per kg = 31,0304
Discrepancy in values comes from rounding. Altogether:
vii) edit orders on Bulk Order Management - changing the amount in grams and recalculating fees on the orders to y The Weight/Volume column shows the amount in grams:
After reducing all the items to half:
And editing the order one sees:
There is a credit owed, respective to the reduction in the amounts in both the enterprise fees and the shipment fees. Asides from the import issue - test case iii) - I think this is good to go. Ounces (oz) and pounds (lb) are introduced in OFN! 🎉 |
filipefurtad0
added
feedback-needed
and removed
pr-staged-uk
|
Thanks Filipe. |
|
You are right @luisramos0: I've changed the enterprise fee to 2.5 UK-pounds/kg. It looks correct as well:
|
|
awesome, I tested again with sample data locally and enterprise fees are working! I am not sure what happened, it doesnt matter 👍 There's a PR for the shipping and payment methods calculators to support pounds so, it will soon be just BOM on the list of problematic things for imperial units 🎉 |
|
Just to be sure that I understood correctly: Both enterprise fee weight calculator and shipping weight only includes the kg unit (so I as an enterprise user can not choose an imperial unit for enterprise fee or shipping weight, for products I created with imperial units), but the units are converted so the amounts are calculated correctly? And has been an issue created already for BOM? |
|
Hello, @jaycmb that depends if you are talking before or after #5998 I think we need a new issue for the BOM case. |
|
Another concern that came up again in today´s product curation meeting: |
|
yes, that's been discussed before 👍 I am not sure where... |
|
That's what I was envisioning as well. I was picturing long term having each enterprise able to choose from a list of units available (I was thinking a multi-tick-box UI, maybe even separated by imperial/metric). Then a shop could even choose to ignore units they never use, like grams or ounces. And also an instance could have in its configuration the default choices for a new shop on that instance. I think the behavior Luis describes is correct for what should happen if a product is in a unit that's no longer in the list of units available - namely, nothing should happen until the user explicitly changes them. Here's the issue I created for it: #6082 |
|
ok 👍 sounds good, but I think this is out of scope for the original wishlist item: https://community.openfoodnetwork.org/t/hubs-managers-can-choose-the-adapted-weight-and-measure-units-for-their-shops-given-their-own-local-situation/1289 I'd say that instead of an issue #6082 it could be a new wishlist item on discourse to define how we are going to build this extra config and also ask how important is it/prioritize: it needs some inception I think. |
|
Got it; I'll close the issue for now and move its contents here: https://community.openfoodnetwork.org/t/allow-enterprises-to-choose-what-available-units-for-products-variants/2060 |
|
@luisramos0 not sure if I understand the concern: "what happens if the user selects imperial, creates products with imperial units and then switches to metric?" But that raises another questions, since we now discuss basically 2 different features:
Was 2) tested and is this possible currently? @luisramos0 point on #6082 being a new wishlist item: for a more sophisticated solution yes, that needs some inception work.
(Think of for example a user in France, who won´t ever sell in oz just doesn´t wanna be bothered by those weird imperial units ;) (S) he should be able to hide them) |
|
Agreed that something like a multi-select is the first priority. For your question about conversion: Right now if I have a product variant that is 1 kg and change the product's units to pounds, it will automatically change the variant to be ~2.2 lb. However, as @tschumilas points out, there's some "going further" work we could do here to encourage/allow different shops that might have different units to share a hub and have all their units show up the same without each farm having to recalculate. |
|
Seriously - I had NO idea on this - if I enter a product in pounds, and I change that product's units to grams, it will auto-calculate? OMG - I can't wait to try this. Mega feature for Canada!!!! |
What? Why?
No issue has been created yet, but see Discourse for background.
The change addresses the issue that for many farms in the United States, not having an easy way to sell products by the pound/ounce is enough to dissuade them from using OFN. There is a currently a workaround to use "items" and a custom string but this makes it much easier to use.
The Discourse post from @kirstenalarsen discusses four points; the first two are fairly straightforward and addressed by this PR.
Point 3 is certainly possible, and it would be great if enterprises/instances could hide certain units if they never use them so that the dropdown has fewer options. I think it'd take a fair amount of refactoring, however, and possibly making a model out of Scale or Unit, but I don't know a ton about how instances are configured yet and how that config trickles down and is read in the codebase.
Point 4 seems like it might add more complexity than it's worth, however, and I agree that it's beyond "minimum viable feature".
Other items to address:
As @luisramos0 noted further down the Discourse thread, it would be really nice to do some refactoring here... we have:
We could consider adding additional non-metric units for volume (pints, quarts, gallons, liquid ounces); this would strengthen the case/need for an instance/enterprise to be able to choose which units are available.
I noticed that, for example, if a variant's unit_value is '1.0' (1g) and you change the product's variant_unit_scale from '1.0' (g) to '453.6' (lb), the variant's unit_value gets converted... this was surprising to me when I did it but I suppose it makes sense. And it's probably rare enough that someone would want to change the units on a product once they can create the product with the desired units in the first place. As an aside, it's taken me a while to get the hang of the admin interface for products/variants/units... if it's not on the design roadmap yet, I have some ideas for making it more clear!
What should we test?
The test suite is (obviously) green as of 5680cc9 but there are still tests I'd like to add, particularly around the new javascript functions to test for bad/unexpected input from other files.
We should also do some extensive sanity/smoke tests, particularly around shipping calculations, and likely add some automated tests to the suite there too.
For example, @luisramos0 asked: "do shipping methods with weight calculators work for orders where the weight was changed in the bulk order management? Example, if I order two items of 0.25kg and then the hub manager uses Bulk Order Management to say the final weight was 400g, the shipping fee with cost 10eur per kilo will have to update from 5eur to to 4eur. This must work across units and scales." I think they're working properly, but I'm a little new to the codebase to know what working properly really looks like yet.
Release notes
"Pounds and ounces are now available as units for products and variants"
Changelog Category: Added
How is this related to the Spree upgrade?
Unrelated
Discourse thread
https://community.openfoodnetwork.org/t/hubs-managers-can-choose-the-adapted-weight-and-measure-units-for-their-shops-given-their-own-local-situation/1289/11
Dependencies
None that I know of.
Documentation updates
Probably! Will investigate and add soon.