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.

Sign up for GitHub

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

Jump to bottom

Products may be created with pounds for their weight unit_converter #1

Closed
wants to merge 3 commits into from
Closed

Products may be created with pounds for their weight unit_converter #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1abdd8b
WIP: Products may be created with pounds for their weight unit_converter
zspencer May 20, 2020
9780955
Support imperial units when scaling the unit value
zspencer May 27, 2020
77ae4b4
Presenting a line item or variants options_text uses overriden values
zspencer May 27, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 9 additions & 1 deletion app/assets/javascripts/admin/products/services/variant_unit_manager.js.coffee
View file
Open in desktop
Expand Up @@ -4,7 +4,15 @@ angular.module("admin.products").factory "VariantUnitManager", ->
'weight':
1.0: 'g'
1000.0: 'kg'
1000000.0: 'T'
1000000.0: 'T',
# This appears to be what needs to be set in order for
# products to have a mass value stored in the database
# when they are created. However, it does not appear to
# change the existing product(s), so if the scale value
# is changed, a data migration may be necessary to make sure
# the proper unit X to grams actually works.
# TODO: ^^^ Delete this
Copy link

Choose a reason for hiding this comment

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

afaik this is only used in the UI when you change the unit of the product. All other cases are handled on the server side.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you!

453.592: 'lb'
'volume':
0.001: 'mL'
1.0: 'L'
Expand Down
4 changes: 4 additions & 0 deletions app/models/product_import/unit_converter.rb
View file
Open in desktop
Expand Up @@ -32,6 +32,10 @@ def unit_scales
{
'g' => { scale: 1, unit: 'weight' },
'kg' => { scale: 1000, unit: 'weight' },
# We have _no idea_ what this is doing. It has units?
# And it maybe is connected to something related to shipping?
# TODO: DELETE THIS ^^^
'lb' => { scale: 453.592, unit: 'weight' },
Copy link

Choose a reason for hiding this comment

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

this is unit conversion in the product import tool (menu Products > Import). It's just repeated logic that should be in a service 🙈 This refactoring is not your responsibility as you just landed on the codebase!

Copy link
Member Author

Choose a reason for hiding this comment

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

Apologies, I've fallen behind on this. Thanks for this pointer!

't' => { scale: 1_000_000, unit: 'weight' },
'ml' => { scale: 0.001, unit: 'volume' },
'l' => { scale: 1, unit: 'volume' },
Expand Down
10 changes: 9 additions & 1 deletion lib/open_food_network/option_value_namer.rb
View file
Open in desktop
Expand Up @@ -63,7 +63,15 @@ def option_value_value_unit_scaled
end

def scale_for_unit_value
units = { 'weight' => { 1.0 => 'g', 1000.0 => 'kg', 1_000_000.0 => 'T' },
# TODO: We _think_ this will cause the following weird results:
# 29g of product would use the `oz` measurement
# 445g of product would use the `lb` measurement
# So we probably want to keep the metric and imperial measures
# in their own lanes; perhaps using a configurable value on a per
# shop or producer basis?

units = { 'weight' => { 1.0 => 'g', 1000.0 => 'kg', 1_000_000.0 => 'T',
28.34952 => 'oz', 453.59237 => 'lb'},
Copy link

Choose a reason for hiding this comment

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

I think this is good, I think this is the most important bit 💪

Copy link
Member Author

Choose a reason for hiding this comment

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

Glad we found it :D :D :D

Copy link

Choose a reason for hiding this comment

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

This does, indeed, cause the weird results where a 29g variant will try to use ounces, a 445g variant will use lbs, etc.

Would it be crazy to remove the autoscaling behavior instead of trying to write some conditional-heavy code here? That is, could we use the variant_unit_scale of the product as the scale for all variants, instead of adjusting the scale based on the variant?

I made a (wip) commit in order to test this on Semaphore; there are some test failures coming back related to it, but nothing we can't fix/rewrite. However, I want to see if such a change would be acceptable to the larger community first or if having the variants autoscale is important behavior.

From a code philosophy point of view, it seems to me to be much cleaner to rely on the product's scale as the single place where the scale information lives. And from the user perspective, it seems to me to be easy enough to denote most common cases in, for example, either g or kg, instead of assuming that the software will auto convert.

But @luisramos0 (or maybe you too Zee) will probably have a much better sense for whether such a change would be acceptable.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a challenge I've ran into in other food-related systems (I built a nutrition-based recipe adjustment recommendation app for a startup a few years back). The ability to do implicit conversions is really slick from a user perspective; but it does add significant complexity to the underlying application logic.

The way we resolved that coplexity in the other project was by storing all the data in the database in milli(grams || liters), and performing the conversion at the user interface/ presentation layer.

I don't know enough about the OFN code-base to know how feasible or desirable such a solution is.

'volume' => { 0.001 => 'mL', 1.0 => 'L', 1000.0 => 'kL' } }

# Find the largest available unit where unit_value comes to >= 1 when expressed in it.
Expand Down
23 changes: 19 additions & 4 deletions lib/open_food_network/variant_and_line_item_naming.rb
View file
Open in desktop
Expand Up @@ -18,7 +18,18 @@ def options_text
order("#{Spree::OptionType.table_name}.position asc")
end

values.map(&:presentation).to_sentence(words_connector: ", ", two_words_connector: ", ")
values.map { |option_value| presentation(option_value) }.to_sentence(words_connector: ", ", two_words_connector: ", ")
end

def presentation(option_value)
if option_value.option_type.name == "unit_weight"
if has_attribute?(:display_as) && display_as.present?
return display_as
elsif respond_to?(:variant) && variant.present? && variant.respond_to?(:display_as) && variant.display_as.present?
return variant.display_as
end
end
option_value.presentation
end

def product_and_full_name
Expand Down Expand Up @@ -48,9 +59,13 @@ def name_to_display
end

def unit_to_display
return options_text if !has_attribute?(:display_as) || display_as.blank?

display_as
if has_attribute?(:display_as) && display_as.present?
display_as
elsif respond_to?(:variant) && variant.present? && variant.respond_to?(:display_as) && variant.display_as.present?
variant.display_as
else
options_text
end
end

def update_units
Expand Down