Fix stock policy propagation on product combination

[FabienPapet]

Questions	Answers
Branch?	8.0.x
Description?	Product out of stock policy is not propagated in product attributes
Type?	bug fix
Category?	BO
BC breaks?	no
Deprecations?	no
Fixed ticket?	Fixes #28958
Related PRs	If theme, autoupgrade or other module change is needed, provide a link to related PRs here.

[marsaldev]

Seems ok to me 😉

[marsaldev]

Uh oh, the tests needs update ^^"

And I think there is also a BC break here:
https://github.com/PrestaShop/PrestaShop/pull/29132/files#diff-c7671f11a1aa9e93a5db6dcdde0cca09b9a6185af9fb8b2edc3c52fa34903bb7R86

and here (?):
https://github.com/PrestaShop/PrestaShop/pull/29132/files#diff-1d3ebe4c70617961beaa59d7621f5e122b55a8e58e25fdd2aa11345e9af81a77R65

[FabienPapet]

Following this ADR, changes should be OK with BC premise, can another one confirm ?

I'll have an look on failing tests

[zuk3975]

checking strictly against null doesn't add any value, because arguments are already strictly typed, so i IMO its cleaner to read without null checks:

Suggested change

	if ($shopConstraint !== null && $shopConstraint->getShopId() !== null) {
	if ($shopConstraint && $shopConstraint->getShopId()) {

[zuk3975]

Also shouldn't we handle shopGroupId?
Lets say if $shop->constraint is ShopGroupId, then we probably need to check where id_shop IN (:shopIds) (shop ids being the shops in group. You should explore more if thats the case, im just saying it without much exploration)? Or if shop group is not supported, then we should throw exception. Because I think in other productRep methods we load the product by shop constraint, which already handles the error case for shopGroup and then its impossible to pass shop constraint for group. (but in this case you can pass any shop constraint, so the ShopGroupId case isn't handled)

[FabienPapet]

You are right about shopGroupIds, I've aded code to handle them. Considering the 'short if' notation, I prefer keep it that way because I don't like the if($object) notation and I prefer testing boolean results.

[zuk3975]

I think there is a risk for sql injection 🤔

[FabienPapet]

I removed the code as we don't need it yet and it may cause some issues like you mentioned.

Thank you for pointing that out

[zuk3975]

Sorry that I didn't see it earlier (and I know this causes PR to drag longer 😓 ), but it seems there is risk of sql injection in couple places 🤔 (left comments)

[FabienPapet]

Code is now corrected and ready for review

[FabienPapet]

Fixed

Tests have been added

[florine2623]

Hello @FabienPapet ,

Automated tests were launched and were successful.

Tested the basic case in PPV2, it is fixed ✅
Checkout process is working well.

Screen.Recording.2022-11-09.at.14.25.40.mov

Also checked for the parameters: ✅

• Tested with standard products and combination products
• Disable/enable option Allow ordering of out-of-stock products
• In Product Page edition, checked with all the options of When out of stock

Also checked with Product Page V1, it is not correct. ❌

Screen.Recording.2022-11-09.at.15.35.41.mov

I can always add product to cart + proceed to checkout, with these 2 options:

I should not be able to add my product to my cart when I have Deny orders and Allow ordering of out-of-stock products.

Could you check?
Thanks!

[florine2623]

As discussed with @jolelievre ,

This PR is validated for Product Page V2 ✅

Another issue has been raised for the Product Page V1 mentioned in the previous comment : #30312

Thanks all !

[prestonBot]

QA approved, well done! Message to the maintainers: do not forget to milestone it before the merge.

[jolelievre]

Thanks @FabienPapet and @florine2623