Product Model refactor

[lin-d-hop]

Description

The Product model in OFN is a bit of a mess. Originally we just used products. Later we started making use of variants. When we did so we hacked a bit such that all products are variants, but some data is only stored in products. One product can have one or more variants. This means that when a variant needs to update, sometimes this is fine, and sometime it has the potential to impact a number of other variants. This can create all kinds of unexpected behaviours. In order to progress with any functionality that can update products, we'll be doing ourselves a huge favour if we investigate and refactor the hacked existing model to make it more fit for purpose.

Our future product model will:

• Not make a split between products and variants. Everything is the same and all necessary information is held within the entity
• Clarity on how variant groups will be identified without the current product/variant split. Some kind of model for a 'product group' will be necessary that will enable the current product/variant relationship to be displayed to the user.
• Enable multiple prices for products. For now we don't need to know what criteria will exist on each price. Note that our current model already separates price into the spree_prices table, identified by a variant_id. This is likely sufficient at this stage until we understand how we want to interact with prices going forward.
• Require minimal changes to the shopfront and backoffice to be implemented. We're going to feature parity.
• Avoid making changes to inventory. That is out of scope for now if possible. We're going for feature parity on inventory too.

Progress

Prep

• Remove permalink Remove :permalink attribute from Product #11137
• Remove meta_description Remove unused product meta description #10566
• Remove available_on Remove :available_on attribute from Product #11136
• Add timestamps to variants Add timestamps to variants table #11138
• Deal with multiple/singular Taxons [Product Refactor] Remove multiple taxons #11346

Associations:

• tax_category_id [Product Refactor] Tax category #11156
• shipping_category_id [Product Refactor] Shipping Category #11221
• primary_taxon_id [Product Refactor] Primary Taxon #11369
• supplier_id [Product Refactor] Move supplier to Variant #12562

Unit Sizes:

• variant_unit, variant_unit_scale, variant_unit_name

Group Buy feature:

• [ ] group_buy, group_buy_unit_size

Images?

Acceptance Criteria & Tests

As this is a spike, at the end of this work we need to know:

1. A proposal for the new product model including how we will handle price at this stage and how we will continue to understand groups of products that will display as variants currently do in BO and shopfront
2. A rough plan for how the work will be delivered. How might we best break this down into phases?
3. A time estimate for each phase.
4. Any gotchas and caveats spotted along the way.

[mkllnk]

Knifflig, my German brain says, tricky. So we need to identify the product attributes that have to move to variants to make them their own independent product/variant. In the UX, creating a variant can become cloning a product/variant and putting it into the same product group. Unfortunately, we do use the term product group already, at least the database has a model to group products and list them in a custom order. I reckon we have to clean up and rename. After this confused intro, I will try to organise my thoughts by things that we still need to discuss:

New terms

I'm not sure about this but I think that we ideally rename Product to ProductGroup and rename Variant to Product. But it will make the term Product very confusing because it can mean the old Spree Product or the new OFN Product. Dilemma. 🤷

Network and Product Groups

For now it would be simplest to keep the current spree_products as Product Groups. We may want to keep that concept just to arrange the OFN shop front. Otherwise we'll need some new UX.

Thinking of the Network, we could actually get rid of product groups aka Spree::Product. For example, in Network 2.0 we have product relationships. One apple comes from a bag of apples, the apple pie has one apple as ingredient. We can group these on the shop page to list all child products under their parent product and then the product group is just another product with derived products in it. So you can offer generic apples and have derived Pink Lady and Granny Smith products. I'm not sure if that fits in with the general idea of derived / related products though. What kind of product relationships do we want to allow? Would there be any problems with this approach? We can leave this decision for later though when we implement Network 2.0. ❓

Product/Variant attributes

Current attributes

  create_table "spree_products", id: :serial, force: :cascade do |t|
    t.string "name", limit: 255, default: "", null: false
    t.text "description"
    t.datetime "available_on"
    t.datetime "deleted_at"
    t.string "permalink", limit: 255
    t.text "meta_description"
    t.string "meta_keywords", limit: 255
    t.integer "tax_category_id"
    t.integer "shipping_category_id"
    t.datetime "created_at", null: false
    t.datetime "updated_at", null: false
    t.integer "supplier_id"
    t.boolean "group_buy"
    t.float "group_buy_unit_size"
    t.string "variant_unit", limit: 255
    t.float "variant_unit_scale"
    t.string "variant_unit_name", limit: 255
    t.text "notes"
    t.integer "primary_taxon_id", null: false
    t.boolean "inherits_properties", default: true, null: false
  end

  create_table "spree_variants", id: :serial, force: :cascade do |t|
    t.string "sku", limit: 255, default: "", null: false
    t.decimal "weight", precision: 8, scale: 2
    t.decimal "height", precision: 8, scale: 2
    t.decimal "width", precision: 8, scale: 2
    t.decimal "depth", precision: 8, scale: 2
    t.datetime "deleted_at"
    t.boolean "is_master", default: false
    t.integer "product_id"
    t.integer "position"
    t.string "cost_currency", limit: 255
    t.float "unit_value"
    t.string "unit_description", limit: 255, default: ""
    t.string "display_name", limit: 255
    t.string "display_as", limit: 255
    t.datetime "import_date"
  end

Attribute changes in spree_products

Here's a quick draft of what to do with the current spree_products table.

• t.string "name" keep as product group name.
• t.text "description" keep as product group description.
• t.datetime "available_on" move to variant. I didn't know this existed. Need to change admin UI or always set for all variants of a product group.
• t.datetime "deleted_at" keep in product group.
• t.string "permalink" seems unused. Delete?
• t.text "meta_description" seems unused. Delete?
• t.string "meta_keywords" used in shop product search. Move to variant.
• t.integer "tax_category_id" move to variant.
• t.integer "shipping_category_id" move to variant.
• t.datetime "created_at" keep. Rails default.
• t.datetime "updated_at" keep. Rails default.
• t.integer "supplier_id" copy to variant to make product group optional in theory. Duplication for now but brings flexibility later, e.g. grouping apples from different suppliers.
• t.boolean "group_buy" move to variant.
• t.float "group_buy_unit_size" move to variant.
• t.string "variant_unit" move to variant.
• t.float "variant_unit_scale" move to variant.
• t.string "variant_unit_name" move to variant.
• t.text "notes" move to variant? Technically not used.
• t.integer "primary_taxon_id" move to variant.
• t.boolean "inherits_properties" keep in product group.

Master variants

Spree had the concept of master variants to hold variant information when you had a single product without variants. We abandoned that concept and it should be unused. We stumbled across some old usage over the years though and there may still be use of the master variant which results in bugs. We best remove the concept entirely to be sure and simplify the code.

Summary

The above work areas may not be complete. I can create a plan for them once we discussed what's in scope. To achieve feature parity, we may end up with a bit of data duplication but that's just a UX constraint which can evolve later.

[lin-d-hop]

Thanks @mkllnk!! Awesome that you are getting your head into this!

Re New Terms - I definitely agree with the desire to rename things in the DB and it would be good to include some of that renaming within scope because I can see that these changes will make things unbearably confusing for newcomers otherwise.

Re Keeping spree_products as Shopfront UX product groups. This is a good idea I think.
I don't think we'll be able to get rid of that later. I think that under Network model we'll want to separate the shopfront UX from the parent child product groupings. Otherwise we'll struggle to properly utilise the Network feature as the UX will limit people. This is similar to what happens now, in which people use variants incorrectly to manipulate shopfront UX. We want to avoid that if we want data integrity in the Networked Product model.

Re Master Variants. Sounds like simplifying this code complexity would be a good idea to me. Perhaps we can t-shirt size this before confirming it is in scope, but I'd like it to be.

Re Attributes. These all make sense to me on first reading :)

🙌

[mkllnk]

Here's a task list in the order I would do it in. The estimates a based on gut feel but let me know if you need me to spend more time working on it to be more precise. I'm expecting a lot of work in rewriting SQL queries. We have to rewrite a lot of the product import and reports for this. So maybe I'm still optimistic?

• Remove unused Spree database tables #10234
• Remove master variants. 5 days.
• Delete unused columns from spree_products #10565
  • permalink
  • meta_description
• Move attributes from spree_products to spree_variants (20 days):
  • available_on
  • meta_keywords
  • tax_category_id
  • shipping_category_id
  • group_buy
  • group_buy_unit_size
  • variant_unit
  • variant_unit_scale
  • variant_unit_name
  • primary_taxon_id
• Copy supplier_id from spree_products to spree_variants. 3 days.
• Rename spree_products to product_groups. 7 days.
• Rename spree_variants to products. 7 days.

Total: 43 days

[RachL]

@mkllnk @lin-d-hop for first release - is this work going to have any impact on the UI? We had a chat with @filipefurtad0 about this today and were wondering how this work will impact the current test suite.

[mkllnk]

We are aiming not to change the UI at all at this point. There may be slight differences in error messages which need adjustments.

[lin-d-hop]

Thanks for completing this estimate @mkllnk
Definitely bigger than I was expecting so I'm really glad you took the time on this.
I think the plan will be to delay this until we are closer to tackling network feature, this will be one of the next steps.
I'll move this back to All the Things for now :)

[Matt-Yorkley]

Nice summary! I think getting rid of all the complexity of OptionTypes and OptionValues will make this a lot easier (#10812).

Finishing off the removal of master variants is definitely a great first step and I think that should come first. It was mostly done in the old Spree upgrades so 99% of the codebase pretends that master variants don't exist, but they're still there in weird ways that leave a bunch of dead code and phantom objects. I've got a branch already that removes master variants 🎉 but I haven't pushed it yet as it's forked from #10812. In the process I also found loads of places in the test suite where old specs are (incorrectly) using master variants in weird ways that are not at all reflective of how the app currently works 😬

I wasn't sure whether or not we want to (at least for now) keep an SKU field on product as well as variant. Currently the product stores it's own SKU on the master variant, but we can just migrate that to a column on the product table and the interface will be unchanged (for now).

Deferring renaming the classes until later on is a good idea. For now it's simpler to just think of moving responsibilities out of the Spree::Product and into Spree::Variant 👍

Most of these attributes should be fairly straightforward to migrate, but I'm not sure about the intricacies of the group_buy feature and how that fits together..?

I think the changes required in reports will probably be minimal, as most of the time it's cases like a line item asking the variant for the tax category, and the variant asking the product, and some small changes to those interfaces should be enough (eg: the variant just telling what it's tax category is instead of asking the product first).

You're right about product import, that's one of the main places that will need code changes. I think afterwards the product import code will be vastly simpler. Almost all of it's complexity is around the different permutations of potential updates or creates across different products and variants, and most of that complexity and those permutations will no longer be present? Most of it's code is just a long series of workarounds for the horrible task of trying to map a one-dimensional dataset (a row in a spreadsheet) to a multi-dimensional dataset (products with multiple variants) in a way that doesn't cause database inconsistencies and that can make some amount of sense to the user. I guess that's kind of the point of the whole exercise; moving towards a flattened ontology for the object that represents "the thing you put in the cart" so that it's a simpler, self-contained, non-composite object, so that it will be much easier to build any features like product importing, or mass-create and mass-update via the API, or simpler handling of networked / linked products.

I'm trying to think about how to pull it off with minimal changes to the UI, but I think there might need to be some changes. Perhaps a phase one could make minimal UI changes and keep the UX logic fairly intact, and a potential second phase could bring in differences? For example if each variant can have it's own tax_category (and products don't store the tax_category at all?) then logically the dropdown for setting that would have to move onto the variant in the bulk edit products page. And maybe it would move into the variant edit page?

The other issue I'm considering is around images, and whether we might want to have an image per variant? Also whether at some point we might want to allow completely stand-alone variants, eg ones that don't have a product at all. But maybe those are considerations for the future...

[Matt-Yorkley]

Hmmm I have a question about the available_on field as well: it doesn't change which products are shown in the shopfront, and it doesn't change which products are listed in the OC edit page, so what is it actually doing? Maybe that's a bug? I would assume if a product had it's available_on date set in the future it shouldn't be shown in the shopfront? It also indicates there's no test coverage on that, if it's the desired behaviour...

[Matt-Yorkley]

Also as a side note, the Spree::Variant object doesn't have created_at or updated_at timestamps, which seems insane to me. We will definitely want to add them at some point, not least because you can't do any caching without them...

[Matt-Yorkley]

On the meta_keywords attribute, it's partially used for searching/filtering results in the shopfront. I'm not sure it's necessary to move it to the variant in that case (or at least not currently). It doesn't get used for anything else and it isn't vital to defining what the variant's main attributes are..?

Likewise with notes, it's a weird attribute that seems to have almost no use anywhere in the codebase, I don't think it needs to migrate on to all variants 🤷‍♂️

[mkllnk]

• available_on - I recently discovered that as well. Must be a leftover from Spree. Should be removed.
• timestamps - Yes, we will need those.
• meta_keywords - If we still want to use it for searching and filtering then it would be nice to have it in the variant because we can then avoid searching in the product (group). We do want stand-alone variants as well.
• notes are relatively new, I think. They are just for reporting, reading, no logic attached.

[kirstenalarsen]

If not too much of a hassle, could we keep available_on? If is easier to clean-up / get rid of it and put something in later if/as needed that's ok, I'm just forseeing use cases in conversations we are having @Matt-Yorkley

[Matt-Yorkley]

@kirstenalarsen I think the available_on thing is potentially a very nice and useful feature, but the issue is that currently it doesn't actually work or do the things that the feature is supposed to do. I'm pretty sure there's no tests on it and if there were, they would fail.

So I think we should either a) fix it up a bit so it's an actual feature that actually works as intended, or b) delete it for now and potentially add it back later as an actual feature that actually works. Otherwise, it's kind of a mostly-broken non-feature that's effectively just junk code and additional complexity in it's current state.

[kirstenalarsen]

cool, get rid of it and if we actually need it we can build it. thanks:)

…

On Thu, 8 Jun 2023 at 23:29, Matt-Yorkley ***@***.***> wrote: @kirstenalarsen <https://github.com/kirstenalarsen> I think the available_on thing is *potentially* a very nice and useful feature, but the issue is that currently it doesn't actually work or do the things that the feature is supposed to do. I'm pretty sure there's no tests on it and if there were, they would fail. So I think we should either *a)* fix it up a bit so it's an actual feature that actually works as intended, or *b)* delete it for now and potentially add it back later as an actual feature that actually works. Otherwise, it's kind of a mostly-broken non-feature that's effectively just junk code and additional complexity in it's current state. — Reply to this email directly, view it on GitHub <#9069 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAWGXSFFY6XNBMN5W46NQ3LXKHHVBANCNFSM5SSIKEDQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[rioug]

@kirstenalarsen
Estimation on what's left TODO :

• finish moving supplier to variant 2 days
• variant_unit, variant_unit_scale, variant_unit_name, maybe 4 days
• move group_buy, group_buy_unit_size to variant 2 days

Total : 8 days, this strictly dev days, it doesn't account for code/review testing.

Maikel mentioned renaming the tables, It's probably not strictly necessary to move forward but I think it's better do it now, his estimations :

• Rename spree_products to product_groups. 7 days.
• Rename spree_variants to products. 7 days.

[kirstenalarsen]

Thanks @rioug - we most definitely do not have capacity to do the renaming at the moment - I'm scrabbling around under the sofa to find coins that let us do the first bit. Go hard please and let's try and get the essentials done before I have to call it and shelve until more $

[dacook]

• t.boolean "inherits_properties" keep in product group.

This flag actually depends on the supplier, which is now associated with the Variant, so I think we need to consider if this gets moved also.

[rioug]

After discussion today it has been decided we would not move Group Buy to the variant for now : https://community.openfoodnetwork.org/t/product-model-refactor/2773/4

[RachL]

Closing 🎉