SKUs' enhancements and multilanguage store

[MatteoPiovanelli-Laser]

Opening this for parts of what's in #99 I am keeping the skus with the multilanguage thing because they intersect

Multi-language store We will need to have localized versions of products, bundles and attributes. This opens up the field to several considerations not unlike those we did in Orchard for Taxonomies and ContentPicker: synchronization, limiting selection to same-language things only... On top of that, there is the issue of SKUs: the way I see this, a SKU should be a unique product identifier (even though there is currently no check for that, but I'll come to that later). However, if a ContentItem of type Product has localized versions, they should in general have the same SKU, because they represent the same product.

SKU As I mentioned above, to me a SKU should be a unique product identifier. However, this unicity is currently not enforced, and has some caveats (e.g. the case of multiple locales). I would create a feature to:

• Add a setting to enforce the unicity of SKUs (should not enforce it by default for retrocompatibility).
• Add the option to generate SKUs automatically (based on templates/tokens).
• Add permissions/options to prevent users to edit SKUs.

[MatteoPiovanelli-Laser]

Did anyone give a thought on advanced SKUs? I was thinking earlier that in some cases, variants may have slightly different skus, as they drive the inventory differently. e.g. the "Brown" variant for a show may have sku = "SHOE1_BROWN" and the "Navy" variant may have sku = "SHOE1_NAVY". Sku generation then becomes more complex

[bleroy]

Ideally, this could work like Autoroute, where you can configure how the slug gets generated, from tokens.

[MatteoPiovanelli-Laser]

I think that the ability for attributes to take part in SKU generation should be in a feature, probably the same feature that allows having separate inventories based on product attributes.

[MatteoPiovanelli-Laser]

I started doing something on this, in a branch. Let me know if it all makes sense or not:

• New feature to handle "advanced" SKUs
  • The feature adds a Site Setting telling whether uniqueness should be enforced (default = false)
  • If we have to check for uniqueness of SKUs, we execute a method from a list of providers to see what products (if any) constitute exceptions to this uniqueness constraint. For example, I implemented the provider to return as exceptions the other localizations of the product.
  • The feature will provide settings to enable and configure automatic generation of SKUs (still to do, and still to figure out exactly how)

Still thinking about:

• Should the feature, if SKUs are to be generated automatically, prevent edits of the SKU from backend?
• Should the feature provide a way to prevent editing SKUs after the product has been published?

[bleroy]

I agree attribute should take part in SKU generation, but this should be done using tokens. Instead of having providers that procedurally generate the SKU, do it the Orchard way and have a setting that uses tokens to format a string. This allows for all your scenarios and more, and doesn't require coding to change the format. Like for the slug, the SKU should still be editable after generation if settings allow for it. Really, Autoroute is the blueprint for this feature. It's pretty much the same design.

[MatteoPiovanelli-Laser]

OK, I agree with you that Autoroute is a great template for this.

The providers I mentioned above are not for SKU generation, but for the uniqueness check: they provide the cases where SKUs may actually not be unique (localization being the one case I could think of and wrote the provider for).

[MatteoPiovanelli-Laser]

The check for uniqueness is currently (in the branch I am working on) implemented as a query for ProductParts whose SKU is the same as the part being processed, . The providers for the exceptions have a method call that returns a list of the IDs of items that may have the same SKU as the product being processed.

var sameSKUProductIds = _contentManager.Query<ProductPart, ProductPartRecord>(VersionOptions.Latest)
    .Where(ppr => ppr.Id != productPart.Id && ppr.Sku == productPart.Sku)
    .List().Select(pp => pp.Id);
//Handle exceptions to uniqueness of SKUs
if (_SKUUniquenessExceptionProviders.Any()) {
    var exceptionIds = _SKUUniquenessExceptionProviders.SelectMany(provider =>
        provider.GetIdsOfValidSKUDuplicates(productPart));
    sameSKUProductIds = sameSKUProductIds.Where(ppid => !exceptionIds.Contains(ppid));
}
if (sameSKUProductIds.Any()) {
    context.Updater.AddModelError("", T("The SKU must be unique."));
}

[MatteoPiovanelli-Laser]

Considerations on the tokenization of the SKU using attributes:

• The combination of attributes in a product is not known until the product is defined/saved/published. Even in the definition of the ContentType we have no information on that. Thus, I don't see a way to meaningfully add attribute-tokens in a setting defined at the site or ContentType level, unless it's a special token that says {Attribute tokens go here}
• The "active" attributes for a product are selected when editing the product, so that is probably the best place to add attribute-tokens to the sku. An attribute-token to me it's something like {AttributeName}, that should be replaced with an attribute value, or a string related to the attribute value. e.g. for a Color attribute, the value may be "Blue" but I may want, in the SKU, to add "BL".
• When saving/publishing the product, upon generating the sku, we don't know what values for each attribute to use to substitute tokens. At this stage we could generate all possible SKUs for all possible variants (attribute combinations on a product), because in this case the sku of ProductPart becomes meaningless by itself.
• Then we may want to query on things, looking for all products that are available in at least one variant.

Basically, where we want inventory and sku to be by-variant, I think we get:

• The Product is an abstract concept. It has attributes, and each attribute has a list of possible values. "That shoe" is a product, with attributes size (possible values 10, 11, 12) and color (Red, Blue, Yellow)
• The Variant is the concrete thing the customer buys. It is obtained by selecting a value for every attribute of a Product. A Yellow "That shoe" on size 11.
  • The inventory is associated to a variant. We have 7 Yellow "That shoe" on size 11.
  • The sku identifies an inventory item, hence it is unique for a variant.

I think I would in the end have an Inventory db table for that, that has:

• SKU, computed for the specific variant
• Quantity available
• ID of the Product ContentItem
• The serialized ordered int[] of the attributes involved in defining the variant
• The serialized ordered string[] of the attribute values that end up defining the variant (see my comment in #60 where I propose a similar table for splitting the inventory by variant)

[bleroy]

That is a big change. Not all products have attributes. I'd like to better understand what exactly will happen at various stages of the product creation and buying workflows: what are all the places where we use the SKU, and how do we use it with those changes?

[MatteoPiovanelli-Laser]

I am doing this in stages, because I am not 100% sold on the big changes I described in my post above, and I am not yet sure about the reworks those would require.

The first stage, for which I may be able to put a PR out tomorrow, sets up a feature that has:

• A setting to enforce checks on the uniqueness of SKUs.
• A setting enabling automatic generation of SKUs:
  • The generation works similarly to Autoroute, but it is based on a site setting, rather than a type setting.
  • The default pattern is "{SKU-{Content.Id}"

That first stage doe not introduce any particular complexity.

The next stages would build on top of that feature and whatever we decide to use to handle an inventory-by-variant, and are for now postponed to whenever we feel that we found a solid design.

[bleroy]

Sounds good, except that I don't like the default pattern: nothing should expose the content id in principle. What about the slug?

[MatteoPiovanelli-Laser]

Sure

[MatteoPiovanelli-Laser]

With that PR merged, the next improvement I would do on SKUs would be to keep the inventory in synch between products with the same SKU.

The ISKUUniquenessHelper provided in the code there basically allows product translations to have the same SKU. I think they should also have the same inventory.

[bleroy]

Sounds reasonable.

[MatteoPiovanelli-Laser]

I am on this right now. I am setting this up so that when the specific feature is activated, the synchronization of the inventories based on the SKU is active by default.

I have a few points where I would like your input:

• How do we want to handle the situation where, when the feature is activated, the inventories of products with the same SKU are not the same? The idea is that the feature will cause the inventory numbers to be in synch, but to begin with they may not be. Tentatively, the path I am planning to follow is to have a notification in the backend whenever two inventories that should be the same are not, with links to edit the products in question.

• I would like to make all changes to ProductPart.Inventory from a service (IProductInventoryService?). The default implementation would give the exact behaviour that is in place right now. The implementation enabled with this feature would take care of synchronizing the inventory across same-SKU products. Does this sound ok? I feel this is better than directly changing the value in controllers/drivers, as it lets me centralize behaviours so that e.g. I can reliably fire events on inventory changes.

• I will probably like to add a way to interact with inventory from the summary admin view. Kind of like there is the "remove one" link right now, but ideally a text box to directly change the value (if the permissions are there). To begin with, this would probably be an additional "add one" link.

• I am going to add an hint near the text box for the inventory in the editor view (and maybe in the summary admin) with links to the editor view of the other products that share the same inventory. The list of products would come from the same service I mentioned above, so that in the future we can possibly change the fact that they are "linked" by their having the same SKU. I am trying to leave the door open to do something with more advanced inventories as we have begun discussing in a couple other issues.

[MatteoPiovanelli-Laser]

also, something I noticed:

Say I have PRODUCT-A, with inventory 100. This is published. I Edit the inventory to 80 and save, creating a draft. The value of the inventory is changed also for the published PRODUCT-A. Same goes for all other product properties (price, sku ....).

I can see reasons for this being as it is, but I can also see cases in favor of versioning the ProductPart.

[bleroy]

If there's a way to coalesce warnings, that may alleviate the problem of too many products throwing them. Is the synchronization a manual operation or is it automatic when you activate the feature? Is it part of a migration? I don't think it can be a migration, because the feature could still be disabled later, products can go out of sync, and then you could turn it on again.

An inventory service sounds fine.

Better summary UI to modify inventory sounds great too. I know I would have used that feature. It should still have a one-click way to remove one. Maybe have up and down arrows next to the textbox.

Links to inventory-sharing products sounds good too.

Unrelated: we should really have a way to quickly find a product by SKU.

Yes, inventory is not versionable iirc, and probably shouldn't be. I'm not sure that's a problem. Price on the other hand, should probably be versionable. Maybe we should have an issue to track this and consider what should and shouldn't be versionable.

[MatteoPiovanelli-Laser]

If there's a way to coalesce warnings, that may alleviate the problem of too many products throwing them. Is the synchronization a manual operation or is it automatic when you activate the feature? Is it part of a migration? I don't think it can be a migration, because the feature could still be disabled later, products can go out of sync, and then you could turn it on again.

After the feature is enabled, the synchronization will happen automatically on inventory changes. At the moment the feature is enabled, I would run a check on products to verify that inventories that should be in synch (same SKU) actually are. If any are not, I would add a notification. My running idea right now is that I would have a single product for each "group" (linking to its editor view), up to a number (say 5). If there's more, I would have a link to a page where I am displaying the full list of issues. This page could be on principle the store settings page. Then, it should be possible to trigger manually the method verifying that the synchronization is fine, and it should also, I think, run every once in a while "by itself" to make sure nothing got messed up.

An inventory service sounds fine.

Doing it.

Better summary UI to modify inventory sounds great too. I know I would have used that feature. It should still have a one-click way to remove one. Maybe have up and down arrows next to the textbox.

Will do something. Up/down arrows should cover most cases, but I'll also think about something more complex. It's probably something that should be permission-ed, if it's not already (I don't remember)

Links to inventory-sharing products sounds good too.

Done it, I think.

Unrelated: we should really have a way to quickly find a product by SKU.

I agree. A feature based on Orchard.Search that adds a search box to the Product's list and sets the search up? I am honestly not familiar with Search. I would also like things like "products with inventory less than N", "Sort products by price" and so on...

[MatteoPiovanelli-Laser]

As a side note, to make the inventory synchronization work I am also fixing a few weird things with bundles: basically, when a ContentItem has both BundlePart and ProductPart, the inventory is not always being updated correctly. For example, when clicking the "remove one" link for the bundle, only the inventory of the products it contains are being updated. Moreover, when updating a product's inventory form its editor view, bundles containing that product are not being updated.

Since I am doing the IProductInventoryService I am cleaning that up as well.

[MatteoPiovanelli-Laser]

Better summary UI to modify inventory sounds great too. I know I would have used that feature. It should still have a one-click way to remove one. Maybe have up and down arrows next to the textbox.

2 things:

• What did you use to minify inventory.js?
• I am not adding the "Add One" link for bundles, because it sounds wrong to have it.

[bleroy]

I would also like things like "products with inventory less than N", "Sort products by price" and so on...

That sounds more like reports to me, for which there's a feature in the module already.

What did you use to minify inventory.js?

I don't remember. Probably a manual operation using Mads Kristensen's VS extension. Ideally, we'd have an automated task to do that.

I am not adding the "Add One" link for bundles, because it sounds wrong to have it.

Sounds reasonable.

Then, it should be possible to trigger manually the method verifying that the synchronization is fine, and it should also, I think, run every once in a while "by itself" to make sure nothing got messed up.

I'm uncomfortable with all this. Having code lingering around, and even running regularly, that deals with what should be a point-in-time state doesn't seem right. I'd much prefer a manual upgrade step, maybe even in a separate feature. The reasoning here is that if I'm a user who started with the actual feature on, I have no use for that code and shouldn't be affected by it in any way.

[MatteoPiovanelli-Laser]

How about this:

I add to InventoryBySKUSiteSettingsPart a InventoriesAreAllInSynch flag that is set to true when we checked all inventories and verified that they are fine. I add a method CheckForInventoryConflicts that gives me the list of product groups whose inventories don't match. If InventoriesAreAllInSynch == false, an INotificationProvider tells the back-end user there may be issues and prompts them to go to the ecommerce settings page. At the time the feature is enabled, the flag is set to false, and CheckForInventoryConflicts is run once. If no conflicts are found, the flag is set to true. In the ecommerce settings page, I have a text telling that the feature is on (this is already in the code I have). I'll add a button "Check for inventory conflicts" that will cause the CheckForInventoryConflicts method to be called and its results displayed:

• If no conflicts are found, the flag is set to true, and I display a message telling everything is good.
• I conflicts are found, the flag is set to false, and I display the list of products with inventory conflicts, with links to their edit views to fix things.

So the method will run only once at the time the feature is enabled, and then can be re-run manually whenever the store administrator wishes. Ideally, it will find no conflicts to begin with. Less ideally, it will be run once automatically, and it will be run again manually as the back-end user goes to fix things (twice here: once to find the list of issues and a second time once they are all fixed to set the flag).

If ways are found to put things out of synch after that, the back-end user will not be notified automatically.

[bleroy]

Sounds reasonable.

[MatteoPiovanelli-Laser]

See #113 where I implemented what is described above.

[MatteoPiovanelli-Laser]

Since #113 is in, I'll close this issue as well.