Problem/Motivation
Currently, there's nothing preventing multiple prices for the same quantity & product belonging to the same price list.
We should ensure this isn't possible by adding a unique key on the following columns:
'type',
'purchasable_entity',
'price_list_id',
'quantity',
Let's define a storage schema handler for price list items, and write an update hook to install the new schema.
Unfortunately, that means having to write a query for deleting duplicate prices, patch to follow.
| Comment | File | Size | Author |
|---|---|---|---|
| #6 | 3419737-6.patch | 2.48 KB | jsacksick |
| #2 | 3419737-2.patch | 5.79 KB | jsacksick |
Comments
Comment #2
jsacksick commentedComment #4
jsacksick commentedCommitted.
Comment #5
simonbaeseCan you please explain how to handle the same entry with different currencies? In our current implementation, we can not define a unique key with
We would also need
We select which price is applicable for the current market with a hook on the commerce pricelist item query tag. Not sure if this use case is not accounted for or if our implementation approach is wrong.
If you do not want to change the behavior, could please introduce a hook to alter the key definition?
Comment #6
jsacksick commentedOof! Sorry for that, what about the attached patch? If you're ok with it, I'll tag a new release urgently...
Comment #7
jsacksick commentedBtw, I'm thinking that it's probably risky to just delete duplicate prices... Perhaps we should display a warning and we should let people cleanup their prices...
Comment #9
jsacksick commentedSorry went ahead and committed a slightly different patch, I'm now skipping the duplicate prices deletion... Perhaps I should have let the update fail in case the schema could not be updated, right now I'm catching the exception and displaying a message.
I think it was urgent to fix before people mass upgrade to it...
Comment #10
simonbaeseThanks for the quick reply. I understand the motivation to prevent duplicates in the price list item table. But this approach may be too restrictive, since the
PriceListItemis a fieldable entity and the query inPriceListRepository::loadItems()allows alterations.We can refactor our implementation to satisy an extended
PriceListItemStorageSchema. But users with less technical experience probably would struggle with this.Comment #11
jsacksick commentedWell, this is an important feature to me... Having several prices for the same quantity tier within the same pricelist is a bug to me...
This causes a random price to be resolved, and that was causing a price history feature I built on a project to be "confused" as it was detecting a new price constantly.
I added the currency code as suggested. I don't really see the case for allowing that...
Comment #12
simonbaeseAs already said, I understand the case for the "vanilla" state of the module. I am giving feedback that the module offers extendability and this extendability is disrupted in a minor release. If you are extending the functionality in a price history module, why not enforce the schema from there?
Comment #14
lawxen commentedSame sku && quantity with different price by conditional field of price_list_item like "Time period".
This is our use case. The commit confilct with it.
Comment #15
lawxen commentedFor these possible usage like #14 Do you sussgest to add the feature back? @jsacksick
Comment #16
jsacksick commentedWhy not defining a different pricelist then?
No plan of doing that for now.
Regarding the comment #12 that I somehow missed:
True, could have done that, maybe a less disruptive change. But now that this change is in, it's also going to be disruptive to revert.
Comment #17
lawxen commentedIf do this:
A lot of new price list wiil be created.
Group module need be introduced to group the dispersed price list. But this increases the complexity of the system.