openfoodnetwork icon indicating copy to clipboard operation
openfoodnetwork copied to clipboard

Fully remove SKU on product level

Open drummer83 opened this issue 1 year ago • 15 comments

Description

It is currently possible to set the SKU on product level (on page /products) as well as on variant level (on pages /products and /products/<Product_ID>/variants/<Variant_ID>/edit. However it is unclear which one is used where.

This could be related to #10939 where master variants have been removed - not sure.

Assumption 1: Since it is not possible to edit the product's SKU on page /products/<Product_ID>/edit, removing it from /products page could have been overlooked. In this case always the variant's SKU should be used everywhere - but in some reports the product's SKU is still being used.

~~Assumption 2: It was intentional to keep the SKU field on product level. Then it should be clear where it is used insead of the variant's SKU.~~

Expected Behavior

☑️ Option A: Fully remove SKU on product level

  • Remove the input field for SKU on page /products.
  • Search the code for places where the product's SKU is being used and replace it with the variant's SKU (e.g. Orders and Distributors report).
  • Disadvantage: It's unknown to me if and how much the product's SKU is being used by our users. A migration (maybe including communication to users could be required). But since in most customer facing cases the variant's SKU is already being used, the effect might be small.
  • Disadvantage: Uncomfortable to add SKUs when it needs to be done for every single variant.
  • Advantage: Clearly defined usage of SKU.

🆇 ~~Option B: Fully keep SKU on product level~~

  • Add SKU input field on page /products/<Product_ID>/edit.
  • Clearly indicate where which SKU is being used, e.g. by naming them Product SKU vs. Variant SKU or similar.

🆇 ~~Option C: Keep the input field on product level but always use variant's SKU~~

  • Search the code for places where the product's SKU is being used and replace it with the variant's SKU (e.g. Orders and Distributors report).
  • Proposal: Add logic to copy the product's SKU to all variants, when it's updated. Optional: Ask before overwriting existing variant's SKUs.
  • Advantage: It's quick to add the same SKU or a SKU-prefix to all variants.
  • Disadvantage: It's unknown to me if and how much the product's SKU is being used by our users. A migration (maybe including communication to users could be required). But since in most customer facing cases the variant's SKU is already being used, the effect might be small.

Actual Behaviour

SKU can be set for product and variant: image image

Product's SKU can't be set on the /products/<Product_ID>/edit page: image

Variant's SKU can be set on the /products/<Product_ID>/variants/<Variant_ID>/edit page: image

Variant's SKU is used in order confirmation email: image

Variant's SKU is used in order cycle report email: image

Product's SKU is used in Orders and Distributors report: image

Variant's SKU is used in Order Cycle Supplier Totals report: image image

Other reports have not yet been checked.

Steps to Reproduce

  1. Add different SKUs to the product and it's variant.
  2. Check out with that variant.
  3. Run the Orders and Distributors report.
  4. Run the Order Cycle Supplier Totals report.
  5. See that the different SKUs are being used.

Animated Gif/Screenshot

See above.

Workaround

Severity

bug-s3: a feature is broken but there is a workaround OR bug-s5: we can live with it, only a few users impacted

Your Environment

  • Version used:
  • Browser name and version:
  • Operating System and version (desktop or mobile):

Possible Fix (for option A)

Step 1:

  • hide the field on the old products page (/admin/products)
  • hide the field on the new products page (/admin/products with feature toggle admin_style_v3; see app/views/admin/products_v3) and
  • update Orders and Distributors report to use variant SKU

Step 2: Then once that change has been released and bedded down, we can perform a full cleanup and delete the database columns.

drummer83 avatar Dec 22 '23 15:12 drummer83

Thanks Konrad!

I don't see uses cases for option C. I would go for option A: what do you think @lin-d-hop @kirstenalarsen ?

RachL avatar Dec 22 '23 15:12 RachL

I would definitely go for Option A - people have distinct SKUs on variants

kirstenalarsen avatar Jan 09 '24 07:01 kirstenalarsen

apologies for initial misread!

kirstenalarsen avatar Jan 09 '24 07:01 kirstenalarsen

@openfoodfoundation/core-devs is option A a papercut?

RachL avatar Jan 11 '24 14:01 RachL

Option A: Fully remove SKU on product level

  • Remove the input field for SKU on page /products.
  • Search the code for places where the product's SKU is being used and replace it with the variant's SKU (e.g. Orders and Distributors report).

With a quick search of the codebase, it appears that Konrad has already identified all usages.

Note that there's currently two /products screens. I think it would save a little bit of time to ignore the old one.

But if we're removing the product SKU, I think we should also consider:

  • Go ahead and remove it from the database entirely.

But if we're happy to start with simply:

  1. hide the field on the new products page and
  2. update Orders and Distributors report to use variant SKU

Then I'd say it's a papercut.

dacook avatar Jan 11 '24 21:01 dacook

I would like to work on this.

bouaik avatar Feb 20 '24 20:02 bouaik

Sure, thank you, @bouaik! I have assigned you.

Let's go for option A.

And please check David's comment above for some helpful information.

Thanks again!

drummer83 avatar Feb 20 '24 20:02 drummer83

Hello @bouaik ! I hope this message finds you well. Are you still planning to work on this issue? Let us know if you need more info!

RachL avatar May 03 '24 09:05 RachL

Is anyone working on it? If not I could try to take a look and get more familiar. I do not want to be a only Rubocops person :D

zanetagebka avatar Jun 19 '24 16:06 zanetagebka

Yes you are welcome to @zanetagebka 👍 This is part of the feature #9069 , take a look for more background.

sigmundpetersen avatar Jun 19 '24 18:06 sigmundpetersen

Thanks Zaneta, I've updated the "possible fix" section in the description above to clarify what's expected. I realised we need to remove the field from the old and new product screens, to ensure we're able to proceed with deleting the data later. (I think the old screen will stay for a while).

dacook avatar Jun 19 '24 23:06 dacook

I need to get familiar with it and currently do not have too much spare time, but Im looking at the related ticket.

zanetagebka avatar Jun 24 '24 19:06 zanetagebka

https://github.com/openfoodfoundation/openfoodnetwork/pull/12650

I opened draft PR here. As far as I understood there is currently only need to "hide" this field. I found there is products_v3 as well, do we want to remove it from there as well?

zanetagebka avatar Jul 09 '24 09:07 zanetagebka

Hi @zanetagebka yes we are in the process of putting product_v3 as the default dev environment (see https://github.com/openfoodfoundation/openfoodnetwork/issues/12627). So if you have the time it would be worth doing it on product_v3 as well.

RachL avatar Jul 09 '24 09:07 RachL