Add a column id_shop in product_sale

[lmeyer1]

Questions	Answers
Branch?	develop
Description?	Distinct best sales for each shop. Add column id_shop to table PREFIX_product_sale and adapt all queries
Type?	bug fix
Category?	CO
BC breaks?	no
Deprecations?	no
Fixed ticket?	Fixes #29538.
Related PRs	A PR in autoupdate module will follow.
How to test?	The list of best sales is now distinct in each shop and reflects the orders passed in the current shop.
Possible impacts?	Three functions in class ProductSaleCore change their signature.

These queries must be run in order to test this PR:

ALTER TABLE `ps_product_sale` ADD `id_shop` INT(10) UNSIGNED NOT NULL AFTER `id_product`;
ALTER TABLE `ps_product_sale` DROP PRIMARY KEY, ADD PRIMARY KEY (`id_product`, `id_shop`) USING BTREE;
TRUNCATE TABLE `ps_product_sale`;
INSERT INTO ps_product_sale (`id_product`, `id_shop`,`quantity`, `sale_nbr`, `date_upd`)
  SELECT od.product_id, od.id_shop, SUM(od.product_quantity), COUNT(od.product_id), NOW()
  FROM ps_order_detail od GROUP BY od.product_id, od.id_shop;

[jolelievre]

If this code is only used in FO then only handling a single shop case would suffice But it maybe be also used in BO where the context can also be: All shops, and Shop groups

So those use cases should be anticipated, so you should use Shop::getContextListShopID() with the list of affected IDs and use this in a IN condition instead of a strict equality on one shop only.

This applies also for the read queries you modified, although be aware that when Shop::addSqlAssociation is used this is already partially done already.

[lmeyer1]

OK, I will look into this. Thanks

[lmeyer1]

• In getNbSales(), Shop::addSqlAssociation will handle the problem.
• If addProductSale() is called, there must always be added only one line for the shop the order belongs to. Shop::getContextListShopID() will not help, if someone is modifying an order while being in all-shop-context. We must add an argument id_shop.
• For removeProductSale() it is the same case.
• getNbrSales() is used only in ProductSaleCore and must also take id_shop as an argument

[kpodemski]

I'm afraid there are BC Breaks.

If you want to avoid it, you need to:

• add new arguments at the end of the methods
• don't make them required, get shop from the Context::getContext() if the shop is not provided

[lmeyer1]

@kpodemski

I'm afraid there are BC Breaks

You are right, I didn't think about it before.

I think add new arguments at the end of the methods and don't make them required will not be enough to avoid BC break.
If the method is used from the BO, Context::getContext()->shop->id could return the wrong id_shop, if we are in All shop context or a shop group.

Thus, it does not seem possible to me to keep consistency AND to avoid a BC break.

[jolelievre]

Hi @lmeyer1

Thus, it does not seem possible to me to keep consistency AND to avoid a BC break.

I think it's possible, only when your $shopId optional parameter is empty you need to get the contextual list via Shop::getContextListShopID() then you can loop through this list and insert what you need.
When a $shopId is specified your list is simply [$shopId] this way the code is simpler because it's always based on a list.

For getNbrSales it's the same principle but you'll have to use an IN (shopIds) condition.

[lmeyer1]

@jolelievre, @kpodemski
I now changed like suggested.

But for these two functions addProductSale and removeProductSale working in shop group or all shop context is hurting the statistics. In both cases, we either add or remove one sale for each shop. This is duplicating sales or removing them duplicated.

We could argue, that it would be better do nothing, if $isShop is null, because in this case we can assess exactly the error: one sale has not be added or removed. When adding or removing on an array of shops, this array could be big, and the error introduced would be big too. (Suppose we have 20 shops, we would save 20 sales instead of 1).

[kpodemski]

@lmeyer1, you are right. Unfortunately, I don't see any way to keep "BC Break-free" different way

We could:

• introduce a new method addProductSaleForShop
• deprecate or leave the old one
• change the use of addProductSale for addProductSaleForShop
• but still, there will be a problem with those who run addProductSale because id_shop will be NULL, and we won't get any information about it in methods getting sales

Maybe we should wait for PS 9.0 and change it then?