[Product Refactor] Primary Taxon

[Matt-Yorkley]

What? Why?

Part of #9069

Migrates primary_taxon from Product to Variant.

Review Note

I hit a bit of an issue described here: https://openfoodnetwork.slack.com/archives/C01T75H6G0Z/p1691581195059389

I've added a temporary solution which resolves it (see last two commits) by applying the filtering products by taxon via the first taxon of the first variant and it avoids having to remove the ordering feature for now. I think we can come back to this later.

What should we test?

Setting the product category when creating a new product or editing a variant, filtering and ordering by taxon in the shopfront.

Displaying the taxon (product category) in the products and inventory report.

Release notes

Changelog Category: User facing changes

The title of the pull request will be included in the release notes.

[mkllnk]

Nice work. Looks like we may need to rethink the shopfront ordering at some point.

Another couple of thoughts:

• A column ignore would prepare us for the column removal and test that Rails really doesn't try to access it.
• Resolving style issues or at least updating the Rubocop todo helps others keeping the code quality high.

[dacook]

This looks good. I have a few questions and think we might be able to simplify the factory.

But I think the only essential change is to fix the linter errors. I'll have a go at that now.

[drummer83]

Hi @Matt-Yorkley,
There's a conflict here.
Moving to In Dev for resolution.
Thanks!

[mkllnk]

Rebased and conflicts resolved.

[mkllnk]

I resolved the conflicts but there are quite a few specs failing now. Back to In Dev for @Matt-Yorkley. We need to review these pull requests quicker to avoid conflicts. Otherwise it will be stuck in the review-test-rebase loop forever.

[dacook]

Great, I reviewed the last commits, all looks good to me

[dacook]

Ready to test with no conflicts!

[dacook]

As it's user-facing change, it would be good to provide a little more detail in the PR name. I'm not sure exactly how it will affect users, but I guess they will see the category selection when editing Variants instead of Products? Does the tester have a suggestion? 🥺

[drummer83]

Hi @Matt-Yorkley,
Thanks for continuing your work on these network 2.0 preparations. 💪

I have tested the following.

Super admin:

• Create new taxons ✔️
• Edit taxons ✔️
• Delete taxons ✔️ (unless Super admin: Deleting the last taxon or header taxon using right click 'remove' does not work #9387)

Admin:

• Create new product ✔️
• Create new variant ✔️ (price is not checked to be a number anymore - it's set to 0.00)
• Duplicate product ✔️ (on demand setting is not duplicated)
• Edit variant ✔️
• Delete variant ✔️
• Import products ✔️ (working fine for the first import, second import fails due to Tax category error with file import #11492, but no error for product category is shown - so I assume it would pass)
• Define sorting of products by category in shop front ✔️
• Products & Inventory Reports ✔️
• Filter in product list ❌
  • All variants are shown if one variant meets the product category (this may be part of your compromise and we could live with it).
  • If there are n variants of the same product with the same product category and the filter is set to that category, then the product (including all variants) is shown n times in the product list.
    

Shopper:

• Filter for categories ✔️
  • The filter ignores deleted variants properly.
• Sorted products by category ❌
  • Deleted variants are taken into account when defining the sorting in the shop. This can lead to incorrect results.
    
    

Conclusion

I am not sure how edgy the two failing test cases are. They both are regressions, I think. After our discussion on Tuesday about the trade off between quality and invested time I don't know if we want to merge, create issues and maybe fix later or if we want fix this before merging. So I'm adding the feedback needed label and ask @openfoodfoundation/train-drivers-product-owners for a decision.

Thanks anyway for this great effort! 🙏

[RachL]

Thx @drummer83 for these thorough testing notes!

I couldn't make it on Tuesday so I'm not sure I have all the info. Are those last bits tricky to fix @Matt-Yorkley ? I'm thinking we could merge and fix later: it will take a while for users to see they can change the category per variant and even in that case, scenarios where they will use it are rare - but I could be wrong.

[drummer83]

I thought it's a good idea to test deeply due to the regression we currently have with the product import and tax categories. Didn't want that to happen again. 😉

I agree that use cases for different categories for variants of the same product are rare and will probably not be used for some time.
However, as soon as users filter the product list for categories, they will see the multiple entries. This is "not nice" but still everything is working. No strong opinion on my side but regarding the fixing of the issues these multiple entries should be the priority.

[RachL]

Ah so the multiple entries are happening all the time, not only when variants of the same product have different categories. In that case yes I agree it should be fixed in priority.

[filipefurtad0]

Hey @rioug ,

There were some issues staging the PR, it seems the first attempt fails to fully deploy; this happened for both:

staging-FR

Here, staging was eventually successful on the second run, but I noticed that some products are gone, from the products page. Edited: somehow, this was related to filters, although there was no selection, clearing filters displayed all products/variants.

cat-o-mat was present, just before staging, and like in previous tests. Still is, after clearing filters.

I'll stage master to make sure these products appear again, and this is indeed introduced by this PR.

staging-UK

I thought this could be an issue with the need of re-running the migration (for which, we'd have to delete a pre-existing version). But I could not find any of the migrations from this PR:

openfoodnetwork=> select * from schema_migrations WHERE version LIKE '%2023080%';
    version     
----------------
 20230807145022
 20230809172206
 20230809194304
 20230809195519
 20230809201542
(5 rows)

I'll still test staging master before - since the PR was recently rebase, maybe a migration which is in master could be needed? I'm not sure, can't hurt trying.

[filipefurtad0]

(cont.)

staging-FR

As for the tests itself, we had two relevant scenarios:

1. Seems to be fixed: filtering by category, displays one entry only, for a product with three variants:

2. I've repeated the test above. Before deleting a variant which sets the product in the first position:

After deleting it, the cat-o-mat product lowers it's position accordingly:

staging-UK

Deploying master works fine; still can't deploy this PR:
https://github.com/openfoodfoundation/openfoodnetwork/actions/runs/8522752128/job/23343614002#step:4:177

Worth trying deployment on staging-AU:

Deployment seems to have worked with no issues.

Summary

The PR fixes the previous issues 🎉
However, I'm not sure about the deployment issues for staging-UK.. Master stages just fine, so I find this a bit concerning. What do you think? This is the only blocker for merging.

Added the feedback-needed label.

[rioug]

However, I'm not sure about the deployment issues for staging-UK.. Master stages just fine, so I find this a bit concerning. What do you think? This is the only blocker for merging.

It looks like the DB on staging-UK already has primary_taxon_id added on spree_variants table, which is weird. It shouldn't be an issue on production though.

[rioug]

The staging-UK DB somehow got out of sync.
I tested the migration on a copy of au prod DB and it's fine, I am going to merge.