Product option types vs Product variant_unit_* fields

I am posting on “Using OFN” as I found this as a user in Portugal. We can move to product or tech if appropriate after we get better clarity over the topic.

Product Option Types is a spree feature that enables us to define dimensions in which the product can have variants, the spree guide examples are size and color.

Additionally to Option Types in OFN we added fields to the product model: variant_unit, variant_unit_scale and variant_unit_name.
It looks like this was done to create option types that are numeric (like weight and volume). These are much more common variant dimensions than color or size in OFN.

As far as I understand we have the menu and page to add product option types, but we don’t have a way to add these option types to the products directly.
Instead we have a static list of options (built in code of VariantUnitManager): g, kg, T, ml, L, KL and Items
In the database, we put this value in product.variant_unit and at the same time we create the option type for it AND we also create an option value for it based on what we input in the field “Value”.
So, as an example, when I create a new product with “Unit Size” "Volume (L) and value “1” I see in the database that this product gets a product.variant_unit = volume, an new option type is also created “unit_volume” and an option value of 1 is also created.
All this mixed up with the fact that two variants are immediately created for the new product: the master and the “standard” variant (the standard variant is created by OFN code) and both these variants get a variant.unit_value which ALSO comes from the input field “Value”, i.e., 1.

It’s definitely not easy to understand…
After VariantOverrides, this is the most “confusing” part of OFN code/model I know.

Are there related topics to this? I guess we need to clear this up or at least understand it :smiley:

are option_types and unintended side effect that we didn’t anticipate when we implemented variant_unit, variant_unit_scale and variant_unit_name? I don’t remember any code using option types for this feature.

As far as I understood, these variant_* attributes are meant to be shared between variants of the same product and are tightly realted to unit_value and unit_description.

Also, I think I was the last one to touch this part of the code in https://github.com/openfoodfoundation/openfoodnetwork/pull/2845 and I can’t assure you I didn’t introduce any bug.

It looks like it’s how it’s meant to be, we manage variant_units but keep option types for them.
Option types may be necessary because Spree code requires them. The spree user guide says a product must have at least one option type…

On 28/Nov/2013 product.variant_unit was added in this commit
And on 13/Dec/2013 code to replicate it to option types was added in this commit

So, looks like we manage the variant units and scales to be able to have all the weight and volume features and we keep option types for this. SO, that means the option types page should never be available. This page should not be accessible, correct? https://staging.openfoodfrance.org/admin/option_types

The related mystery question remains: why do we have a standard variant? any clue?https://github.com/openfoodfoundation/openfoodnetwork/commit/3f01a459ac317a5fef7ac6b753bfcf6eca074bf8#diff-e6a3a7e1c82ac34e5e72899c9ceb109c

Currently the variant edit page is a defaced spree view.
In deface we:

  • show the OFN specific fields unit_description and unit_value
  • hide the option type and option value that the original spree view is showing (as you change the unit_value and unit_description the option_values are not updated, the original values on creation are kept there. this re-enforces the idea that option types are just created for compatibility with spree)

I have created a new issue to add fields display_name and display_as to the variant edit page. Currently, these values can only be edited in the bulk product edit page.

I wonder if there’s any chance we could get input from @oeoeaio on this . .

The creation of the ofn specific fields was done to get food specific and shared option arrangements to enable consistent handling of units - this was done in creation of the bulk product edit and ‘quick’ new product pages. Requiring every enterprise to create option types and then option values before they could create products was not feasible (I remember it took me ages to work out how to make spree system support the food products i was setting up when we first started), and we wanted to prevent massive proliferation of inconsistent and non-comparable option types like kg KG kilogram etc. It was this restriction that enables some of the logic that switches units up and down e.g. g kg T throughout the system.

I have no problem with hiding the confusing spree fields that are not needed. There are many many confusing things that could be hidden!

There is a reason why we have a standard variant but i really can’t remember what it is! Maybe with network 2.0 it can / will go away . .

ah, thanks a lot for the context @Kirsten

Now that I understand the challenge you describe of managing these units and the code itself, I think the solution is not easy to understand but it’s not as bad as I thought initially (re “most “confusing” part of OFN code” above).
The code itself (in the frontend in particular) needs to be improved.

I think if we put the missing fields in the variant edit page (the issue I’ve created) it will be all right.
Further investigation could clarify if we really need the code that generates option types and if option types are even working currently.

I dont think 2.0 or network will change these things…

I think all this needs to be iterated as the first implementation was a bit rough and somewhat incosistent. Shall we have a dedicated initiative for it as we do for features? there are a few bugs related to it.

we could do that. but my opinion right now is that, if we fix the bug I created, there’s nothing here that deserves much attention/change.
I am not aware of the rest of the bugs in this area apart from the one I have created.

I know there are some. I feel like it’s a rather “rough” area of the product and I’m sure we might have issues created already. In any case, if they are severe enough, these bugs should get fixed like any other if we improve our bug priorization process.

One year later and a few updates:

  • the page to edit option_types was removed and there is no connection between UI and this table now.
  • the option_types and option_values tables are still there and the unit values in variants are still copied to them

So I moved this to discourse topic Tech now as this topic is now tech debt with no user facing impact…

I am back to this post because this structure is related to the source of this bug: https://github.com/openfoodfoundation/openfoodnetwork/issues/5461

Meanwhile, I found a few more things:

  • these tables are still used for some of the things, like showing the names of the variants AND line items in VariantAndLineItemNaming
  • there is an extra table! Apart from spree_option_types and spree_option_values AND spree_option_values_variants to connect option_values to variants, there is an EXTRA table spree_option_values_line_items to connect line items to option_values. I assume this table must be used to keep history on the option_values in the variant, for example, if the option_value of the variant changes the option_value in the line items of past orders will remain the same.
  • spree_option_values_variants is a table from Spree, spree_option_values_line_items was added in OFN :see_no_evil: https://github.com/openfoodfoundation/openfoodnetwork/commit/039fcb80eb3b43d0f09a7bb3bde2107651f62347

SO, my current question is: why do we need all these 4 tables if we are ALSO keeping units in the product (variant_unit_name) and variant (unit_value, etc)? I wonder if it is to keep history (changes in variant units wont affect past orders).

I haven’t dug into all that you found but it’s very likely that was the reason. @maikel might know about. No ideas if he was around back then.

I have no idea what the initial reason was. Keeping information for past orders sounds like a good think but that should probably be saved in line items instead of variants.

yes, spree_option_values_line_items is connected to line items.