search

openfoodfoundation / openfoodnetwork

Connect suppliers, distributors and consumers to trade local produce.
https://www.openfoodnetwork.org
GNU Affero General Public License v3.0
1.12k stars 724 forks source link

Product Model refactor #9069

Closed lin-d-hop closed 4 weeks ago

lin-d-hop commented 2 years ago

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:

Progress

Prep

Associations:

Unit Sizes:

Group Buy feature:

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 commented 1 year ago

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. :shrug:

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. :question:

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.

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 commented 1 year ago

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 :)

:raised_hands:

mkllnk commented 1 year ago

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?

Total: 43 days

RachL commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Nice summary! I think getting rid of all the complexity of OptionTypes and OptionValues will make this a lot easier (https://github.com/openfoodfoundation/openfoodnetwork/pull/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 :tada: but I haven't pushed it yet as it's forked from https://github.com/openfoodfoundation/openfoodnetwork/pull/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 :grimacing:

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 :+1:

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 :man_shrugging:

mkllnk commented 1 year ago
kirstenalarsen commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 https://github.com/openfoodfoundation/openfoodnetwork/issues/9069#issuecomment-1582584641, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAWGXSFFY6XNBMN5W46NQ3LXKHHVBANCNFSM5SSIKEDQ . You are receiving this because you were mentioned.Message ID: @.***>

rioug commented 5 months ago

@kirstenalarsen Estimation on what's left TODO :

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 :

kirstenalarsen commented 5 months ago

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 commented 4 months ago
  • 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 commented 3 months ago

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 commented 4 weeks ago

Closing 🎉