Attributes combinations price #47

TRiV07 · 2020-01-10T14:19:11Z

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

Variants builded from attributes combinations (now we can build only from text attributes with predefined values).

bleroy · 2020-01-13T07:17:58Z

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.

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.
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.
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.

…ardCore.Commerce into triv/attributesPrice

This reverts commit 9a6a3c4.

TRiV07 · 2020-01-16T18:29:23Z

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 · 2020-01-19T02:08:36Z

Abstractions/IPriceVariantsService.cs

+{
+    public interface IPriceVariantsService
+    {
+        Dictionary<string, decimal> GetPriceVariants(ContentItem product);


Should those be amounts rather than decimals?

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

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 · 2020-01-19T02:24:58Z

Models/PriceVariantsPart.cs

+    /// </summary>
+    public class PriceVariantsPart : ContentPart
+    {
+        public Amount BasePrice { get; set; }


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?

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

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).

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

Currency + Dictionary<string, decimal> properties.

Dictionary<string, Amount> property.

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

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).

Agree! Will do.

Services/PredefinedValuesProductAttributeService.cs

Services/PriceVariantProvider.cs

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).

Variants changed to dictionary 'Amount'.

bleroy · 2020-01-30T18:08:27Z

Thanks for all your efforts!

bleroy · 2020-02-02T01:07:15Z

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 · 2020-02-02T01:26:28Z

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 · 2020-02-02T02:44:12Z

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 · 2020-02-02T17:01:03Z

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 · 2020-02-02T20:10:55Z

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 · 2020-02-02T20:14:41Z

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 · 2020-02-02T20:21:09Z

Hahaha. Of course it crashes :)

bleroy · 2020-02-03T02:50:42Z

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 · 2020-02-05T14:06:20Z

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 :)

PricePart have prices for product variants.

9a6a3c4

Variants builded from attributes combinations (now we can build only from text attributes with predefined values).

TRiV07 mentioned this pull request Jan 10, 2020

Product, price, and inventory parts and fields #5

Closed

bleroy requested changes Jan 13, 2020

View reviewed changes

Alexandr Ponomarenko added 3 commits January 16, 2020 20:15

Price Variants Part

68d17fd

Merge branch 'triv/attributesPrice' of https://github.com/TRiV07/Orch…

ea627b5

…ardCore.Commerce into triv/attributesPrice

Revert "PricePart have prices for product variants."

417b3e0

This reverts commit 9a6a3c4.

TRiV07 requested a review from bleroy January 17, 2020 16:05

bleroy requested changes Jan 19, 2020

View reviewed changes

Created interfaces for PredefinedValuesProductAttribute

dc87eb0

TRiV07 requested a review from bleroy January 25, 2020 14:52

bleroy requested changes Jan 25, 2020

View reviewed changes

Removed 'BasePrice' from 'PriceVariantsPart'.

b0576a7

Variants changed to dictionary 'Amount'.

TRiV07 requested a review from bleroy January 30, 2020 16:28

bleroy approved these changes Jan 30, 2020

View reviewed changes

bleroy merged commit ab52edc into OrchardCMS:master Jan 30, 2020

TRiV07 deleted the triv/attributesPrice branch February 21, 2020 17:28

TRiV07 restored the triv/attributesPrice branch February 21, 2020 17:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Attributes combinations price #47

Attributes combinations price #47

TRiV07 commented Jan 10, 2020 •

edited

Loading

bleroy commented Jan 13, 2020

bleroy left a comment

TRiV07 commented Jan 16, 2020

bleroy left a comment

bleroy Jan 19, 2020

TRiV07 Jan 25, 2020

bleroy Jan 25, 2020

bleroy Jan 19, 2020

TRiV07 Jan 25, 2020

bleroy Jan 25, 2020

TRiV07 Jan 27, 2020

bleroy Jan 27, 2020

TRiV07 Jan 28, 2020

bleroy left a comment

bleroy commented Jan 30, 2020

bleroy commented Feb 2, 2020

bleroy commented Feb 2, 2020

bleroy commented Feb 2, 2020

TRiV07 commented Feb 2, 2020

bleroy commented Feb 2, 2020

bleroy commented Feb 2, 2020

bleroy commented Feb 2, 2020

bleroy commented Feb 3, 2020

TRiV07 commented Feb 5, 2020

Attributes combinations price #47

Attributes combinations price #47

Conversation

TRiV07 commented Jan 10, 2020 • edited Loading

bleroy commented Jan 13, 2020

bleroy left a comment

Choose a reason for hiding this comment

TRiV07 commented Jan 16, 2020

bleroy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bleroy left a comment

Choose a reason for hiding this comment

bleroy commented Jan 30, 2020

bleroy commented Feb 2, 2020

bleroy commented Feb 2, 2020

bleroy commented Feb 2, 2020

TRiV07 commented Feb 2, 2020

bleroy commented Feb 2, 2020

bleroy commented Feb 2, 2020

bleroy commented Feb 2, 2020

bleroy commented Feb 3, 2020

TRiV07 commented Feb 5, 2020

TRiV07 commented Jan 10, 2020 •

edited

Loading