Product variants should have unique SKUs (OCC-23)

[sarahelsaig]

Suggested by @chinasqzl here.

This could be handled similarly to variant prices. However inventory management may complicate things. An approach where variants are separate content items and they are linked (similar to how the Content Localization feature works) may be more appropriate. Please discuss.

[sarahelsaig]

I suggest the following:

Product variant SKUs should be derived. For example, if you have a product with a base SKU of "SHIRT", and it has two sets of properties (size being S, M or L; color being black or blue), then you can use productPart.Sku + '-' + value.Classify().ToUpperInvariant to generate the SKU fragments. This way you'd have SKUs like "SHIRT-M-BLUE" or "SHIRT-L-BLACK".

At this point it would be good to convert the base SKU to upper-case during publish just to be clean since it's case-insensitive in the database anyway. Also now dash is an invalid character in the base SKU so that has to be handled.

Then we'd need to update ProductService and ProductServiceExtensions (basically just chop off the parts of the SKU variable after the first dash). Also, add a string GetVariantKey(string sku) method and a (PriceVariantsPart Part, string VariantKey) GetExactVariant(string sku) method that uses the full, generated SKU so we can locate the variant, not just the product.

[porgabi]

This way you'd have SKUs like "SHIRT-M-BLUE" or "SHIRT-L-BLACK".

This part has me a little confused at the moment. PriceVariantsProduct content items come with a singular ProductPart that has an SKU string field on it, but here the variants have no additional data associated with them. So there can be 3 different SKUs for just one of these items, stored on the PriceVariantsPart perhaps? It's unclear to me what the use case is going to be and how this should work in general.

Apart from general clarification, a few more specific questions:

1. Where should the variant-specific SKUs be saved?
2. Should a base SKU still exist for PriceVariantsProduct items?
3. Should these be editable/shown in the dashboard editor?

[0liver]

From what I remember from our planning meeting, the variant SKUs are transient, they are not to be persisted anywhere. We generate them in code on the fly, and interpret them on the fly when an order is placed by parsing them. That's what Dávid's last paragraph is about.

[sarahelsaig]

Oliver is right, you only save the base SKU. The variant SKU should be generated similarly to how the price variant labels are generated and then converted to that upper case format I previously described. It will be used for display (e.g. in the invoice or for shipping) or to look up a specific variant (e.g. for inventory management since you'd want to handle each variant's count separately).

[porgabi]

Thanks, getting closer to understanding the issue at hand!

At this point it would be good to convert the base SKU to upper-case during publish just to be clean since it's case-insensitive in the database anyway. Also now dash is an invalid character in the base SKU so that has to be handled.

Couldn't both of these be handled in the ProductPartDisplayDriver, converting SKUs to be upper-case here and preventing dashes by adding validation for the field? If I understood correctly, the dashboard editor is the only interface where SKUs can be entered. If so, is there still a need to "chop off the parts of the SKU variable after the first dash" in ProductService?

It will be used for display (e.g. in the invoice or for shipping) or to look up a specific variant (e.g. for inventory management since you'd want to handle each variant's count separately).

Makes sense; just to clarify, these display surfaces are to be implemented in the future and do not yet exist, right?

[sarahelsaig]

Couldn't both of these be handled in the ProductPartDisplayDriver

Sure.

If so, is there still a need to "chop off the parts of the SKU variable after the first dash" in ProductService?

Yes, the method that just returns the product should be able to accept the base SKU or the variant SKU.

these display surfaces are to be implemented in the future and do not yet exist, right?

Yes.

[porgabi]

Having added the new methods to IProductService, the test-related classes (DummyProductService and FakeProductService) that are derived from the interface require an implementation of said methods as well. Should these have the proper implementation (just like in ProductService), or should these be throwing a NotImplementedException?

[sarahelsaig]

You can use NotSupportedException, but include some text to make it clear that it's because the tests don't use it.

[DemeSzabolcs]

Also now dash is an invalid character in the base SKU so that has to be handled.

Couldn't we replace the dash with a "more unique" character? I feel like it could be used in the base SKU too. I also have real examples for this, where the admin used dashes in the base SKUs.

But thinking more about this, after all, I don't have a better suggestion, since using other special characters could break things and I think the dash is the most humanly readable.

[0liver]

Where will that character be used? Will it be displayed to the user?

[DemeSzabolcs]

Where will that character be used? Will it be displayed to the user?

In the SKU, so the question is rather: where will the SKU be used?

Currently, it's not visible to the user, only to the admin in the editor. But it can vary, it could be displayed on the frontend too. I can imagine a scenario where a webshop wants to display the SKU on a product page. That's why I changed my mind.

[0liver]

We could one of the non-standard dash characters that no-one should have in their SKUs - ever™. See here:

[sarahelsaig]

Please Oliver, you are giving me a panic attack.

[0liver]

I was waiting for that 🤣

But on a serious note, why not?

OK, let me guess: Once we have a magic dash in our code, even in a constant, it will be prone to being replaced by someone (or maybe even software?) by the real thing. Or the display of such a symbol could break in the UI.

What else?

[sarahelsaig]

Yep, especially if it were to ever enter into the database (e.g. in invoicing) since MS SQL is very prone to "simplifying" characters like that. Also with an future ERP system integration, some of those still don't use Unicode.

Another concern is search. If the SKU is displayed, then product search by SKU is a logical next step. (Probably) works if the SKU is copy-pasted, but not so much if it was written down and then typed in.

Using exotic characters also limits font choice, you will end up with � instead of dash. 😵

It's probably not the best idea to play Unicode tricks on anything serializable or user-facing. It's easier and cleaner to just ban dashes from the base SKU. As I said above, SKUs should be upper-cased anyway to avoid case-sensitivity discrepancies since the DB is case-insensitive so the SKU character set is already somewhat limited.

[0liver]

Thanks. I like to understand the why's.

As for banning the dash from SKUs - it does seem like a harsh limitation.

Some non-authoritative examples that promote the dash

1. Shopify mentions the dash in their first example for SKUs.
2. This page uses dashes all over the place.
3. This e-commerce site says SKUs should be "composed of letters, numbers, underscores, or hyphens".

Counterexamples

This page mentions that SKUs are "usually eight alphanumeric digits" which would exclude the hyphen.

TL;DR;

Both hyphen and underscore seem to be more or less widely used.

Let's choose some other ASCII character.

1. *
2. |
3. /
4. ^
5. +
6. '
7. :
8. Choose your own (e.g. here).

[sarahelsaig]

Actually the dash example in Shopify is exactly a product with variant information (they just identify it numerically). I say just use underscore if your SKU needs partitioning. The idea is exactly that the variant SKU should still be a valid SKU, so if SKUs are considered to be "composed of letters, numbers, underscores, or hyphens" then either hyphen or underscore has to be the reserved variant separator.

[0liver]

That drove the point home. Thanks. Dash be it then!

👍🏼