Attributes combinations price

[TRiV07]

PricePart have prices for product variants.
Variants builded from attributes combinations (now we can build only from text attributes with predefined values).

[bleroy]

Thanks for the contribution. This is a great start. I have a few demands however, to bring it more inline with the rest of the module and with what I had in mind for this feature.

1. The PricePart is intended to be the simplest possible part with a fixed price on it. If we have attribute-based prices, that should be moved to a separate part (and possibly a separate feature), so people can choose to attach the part to their products or not. Same for the price service, we should have one for the basic price, and a separate one that understands your new stuff. This will also allow for better separation of logic in the drivers.
2. I don't think we need to add to BuildVariantPrices, as the intent behind AddPrices was already to have a way to add other arbitrary prices to a product. However, I'm not happy with the way I did it. I think there's a need for additional metadata with each price, so pricing strategies can later make more subtle decisions than the current one, which is just selecting the lowest price. So I like your version better. Let's keep only one though (yours), and refactor other code as necessary to use it.
3. Don't add those new methods to IProductAttributeService. Create a separate interface and service implementation. Also have one on the relevant attribute, something like IPredefinedValuesProductAttribute. The way it's done currently, that would be text only, as it's the only one with predefined values. I think we should think this over so that it allows for more than mapping Cartesian products of predefined values. What I had in mind was something that would allow arbitrary logic, such as using an admin-provided formula to map numeric values to price adjustments, etc. The guiding principle here should be that mapping product and attributes to prices will be highly business-specific, so we need to remain generic. We can have an implementation such as yours with the base module, but the extensibility must still be baked in.

[bleroy]

See my comment for details.

[TRiV07]

Moved all logic to separate part: PriceVariantsPart.
PriceService working as before, just added another one implementation of IPriceProvider.

[bleroy]

Thanks for the changes. I have a few additional requests, but this is getting very close. Cheers!

[bleroy]

Should those be amounts rather than decimals?

[TRiV07]

I think better to have decimal here b/c currency must be defined in base price. Variant just have other price value, not currency.

[bleroy]

Right, about the base price, see my other comment... I think each variant should stand on its own and not rely on the base price.

[bleroy]

Since this has BasePrice, do you envision people using PricePart or this one, or both at the same time? If both at the same time, why is BasePrice necessary here?

[TRiV07]

People must use only one PricePart or PriceVariantsPart. Otherwise we PriceVariantsPart will depend on PricePart. Let me know you thoughts.

[bleroy]

I think this is problematic.

One scenario is a business that starts with simple price. Then they add attributes to their existing products, that they want to individually price. If you have to choose between those two parts, you have no migration path, and you'll have to enter all those prices again.

Another scenario is one where you don't have a base price at all, only mandatory attributes with each a different price.

For at least those two reasons, but also for separation of concerns, I think the base price should be removed (and clearly, prices per attribute can't depend on the simple price part).

[TRiV07]

All clear. So let me know best way to save variants:

1. Currency + Dictionary<string, decimal> properties.
2. Dictionary<string, Amount> property.

As for me first one is better b/c all variants must have same currency.

[bleroy]

I'm not sure why all variants should have the same currency actually. I could see a scenario where variants are geographically based, like a bookseller who sells translations of the same item. Just to be safe, I'd go for #2 because it offers that flexibility at a low cost (unless I'm missing something).

[TRiV07]

Agree! Will do.

[bleroy]

Thanks for the new changes. We're getting super-close. I think the last thing is to get rid of the base price (see my comment about why).

[bleroy]

Thanks for all your efforts!

[bleroy]

Hey @TRiV07 Alexandr! I've been playing with your work today, and one thing felt a little off: if I add the new part to an existing product, all the variant prices are initially zero. I think the initial value should be unspecified (and when that's the case, the variants price provider would not add a price to the product variant), which should be enough for the simple price provider to take over and apply the simple price (or whatever other price provider exists). Is this something you'd be willing to add? Thanks!

[bleroy]

In fact I've found a couple bugs that I'm fixing now, one of which is the variants price provider adding a 0 price when an attribute value is not found. The other is generating the variant key for text attributes not set to a predefined value.

[bleroy]

Another question: when we are generating the Cartesian product key for a specific combination of attribute values, I'm wondering if we don't have a nondeterminism in the order of the attributes that ould lead to problems, potentially. What do you think?

[TRiV07]

In fact I've found a couple bugs that I'm fixing now, one of which is the variants price provider adding a 0 price when an attribute value is not found. The other is generating the variant key for text attributes not set to a predefined value.

Thank you for bug fixing! I believe every variant must have price, that's why used BasePrice before, and generating at least zero price for every variant, b/c product can be used without PricePart.

Another question: when we are generating the Cartesian product key for a specific combination of attribute values, I'm wondering if we don't have a nondeterminism in the order of the attributes that ould lead to problems, potentially. What do you think?

Correct, this is potentially problem, and we need to sort somehow attributes, maybe attributes can be sorted by Id, and predefined values of attribute just by name.

[bleroy]

So if you look at my latest changes, I added priorities on prices. This way, the simple price part's provider can add its price at the lowest zero priority, and variants are added at priority 1. This makes it possible for the simple price strategy to select the variant price even when it's higher than the variant price (which wasn't the case before). Another consequence is that the simple price (or whatever other price you may have at priority zero) can act as a fallback when variants provide no price. I really think by default, variant prices should be unspecified. This way, the default is whatever other price the product has. It's the right default.

Let's sort by attribute name (which should be of the form $"{PartName}.{AttributeName}") when we build the keys to avoid that determinism issue then.

[bleroy]

Oh, and one last thing: if the site administrator didn't configure the type to have some base price, and then doesn't specify a variant price for all variants, then the product variant has no price and can't be bought. I think that's a better outcome for what is likely a mistake than giving away the product for free. I need to check exactly what happens in this case, and that the product can't be added, not that we get some sort of exception...

[bleroy]

Hahaha. Of course it crashes :)

[bleroy]

There, I fixed it. Now if you attempt to add to the cart a product without a price, you get an error message and the product isn't added.

[TRiV07]

So if you look at my latest changes, I added priorities on prices. This way, the simple price part's provider can add its price at the lowest zero priority, and variants are added at priority 1. This makes it possible for the simple price strategy to select the variant price even when it's higher than the variant price (which wasn't the case before). Another consequence is that the simple price (or whatever other price you may have at priority zero) can act as a fallback when variants provide no price. I really think by default, variant prices should be unspecified. This way, the default is whatever other price the product has. It's the right default.

Let's sort by attribute name (which should be of the form $"{PartName}.{AttributeName}") when we build the keys to avoid that determinism issue then.

Will do :)