FIX: out-of-stock options for configurable product visible on frontend as sellable #12936
Conversation
|
Hello dear @magento-engcom-team ! |
|
Hi @coderimus thank you for this pull request. We aim to get around this pull request during the next few days and will let you know if we need anything else from your side. |
|
Hi @adrian-martinez-interactiv4 ! |
|
Hi @coderimus , I'll check it tomorrow, and update you as soon as possible. I apologize for the delay. |
|
Hi again, @coderimus , I've trying to reproduce the issue with no luck, I've tried with cache enabled and disabled, indexes in Update On Save mode, even with config setting Display Out of Stock Products set to Yes and No values, always with the same result: when I update the simple product to remove its stock, it dissapears from product page; when I restore its stock, it shows up again. I'm using attribute swatches, as marked in the testing scenario. Following is the process I've been doing, with the result, please check if I am missing something:
I can't reproduce the issue, am I missing something? |
|
@adrian-martinez-interactiv4 thank you for your time spent on this review and such detail description. I think that together we found one more aspect of this problem. This issue can be reproduced for a product having more than one item for options and options qty should be bigger than 1 and using swatches. In your example, you have 1 option with 2 products. If you would have 2 options with several products you could reproduce it.
Screen #2 check error: My settings stock settings: Please, find time if it is suitable for you and try to reproduce it with my instructions. Looking forward to your response, |
adrian-martinez-interactiv4
added
the
Reproduced on 2.2.x
|
Hi again @coderimus , I've been able to reproduce the issue following your steps, applying your changes seems to solve the problem and mark the option as disabled in frontend, I'm starting a deeper review over the proposed changes. |
There was a problem hiding this comment.
@coderimus Please, review changes and let me know your thoughts, thank you!
| * @return \Magento\Catalog\Model\ResourceModel\Product\Collection $collection | ||
| */ | ||
| public function addStockDataToCollection($collection, $isFilterInStock) | ||
| public function addStockDataToCollection($collection, $isFilterInStock = true, $websiteId = null) |
There was a problem hiding this comment.
There is no need to make already mandatory param $isFilterInStock optional, already existing calls to this methods include explicitly this flag. I think the idea is preset a default value for this field to simplify calls, but by now I would preserve this field as mandatory without default value, to force methods define flag value because this class is not marked as api and default value could change at any time, modifying the behaviour of classes using this method with the default value.
| @@ -202,9 +202,10 @@ public function getProductCollection($lastEntityId = 0, $limit = 1000) | |||
| * @SuppressWarnings(PHPMD.UnusedFormalParameter) | |||
| * @return Status | |||
| */ | |||
| public function addStockStatusToSelect(\Magento\Framework\DB\Select $select, \Magento\Store\Model\Website $website) | |||
| public function addStockStatusToSelect(\Magento\Framework\DB\Select $select, $websiteId = null) | |||
There was a problem hiding this comment.
\Magento\Store\Model\Website $website => $websiteId = null.
- There is no need to make already mandatory param $website optional, see other review comments for further explanation.
- It's preferable to used typed parameters for methods, instead a generic variable. In this case, with this change, you are allowing to pass to this method as second parameter any kind of undesired variables or objects. New private method getWebsiteId also does not check $websiteId param type, so you could end with potential runtime errors when trying to build the query with the unknown-typed $websiteId param.
Please, consider reverting $websiteId param to its previous definition and adapt $websiteId = $this->getWebsiteId($websiteId); to $websiteId = $this->getWebsiteId($website->getId());, or use \Magento\Store\Model\StoreManager::getWebsite method to get a valid website from a websiteId.
| */ | ||
| private function getWebsiteId($websiteId) | ||
| { | ||
| if ($websiteId === null) { |
There was a problem hiding this comment.
While it is not mandatory, please consider putting the operand that cannot be assigned on the left whenever you write a condition, to reduce possible errors.
There was a problem hiding this comment.
Dependant of other review comments resolution.
| private function getWebsiteId($websiteId) | ||
| { | ||
| if ($websiteId === null) { | ||
| $websiteId = $this->getStockConfiguration()->getDefaultScopeId(); |
There was a problem hiding this comment.
Avoid using deprecated \Magento\CatalogInventory\Model\ResourceModel\Stock\Status::getStockConfiguration method; please consider injecting $stockConfiguration as optional param in the constructor, and instantiate it in the constructor if needed. Don't hesitate to ask if you need more info about this.
| { | ||
| $collection->addAttributeToFilter( | ||
| ProductInterface::STATUS, | ||
| Status::STATUS_ENABLED | ||
| ); | ||
|
|
||
| $stockFlag = 'has_stock_status_filter'; | ||
| if (!$collection->hasFlag($stockFlag)) { | ||
| if (!$collection->hasFlag($stockFlag) || !$collection->getFlag($stockFlag)) { |
There was a problem hiding this comment.
I see no advantage in adding || !$collection->getFlag($stockFlag), since the value of this flag is irrelevant as long it is set, because its presence should be only used to mark the collection has already added the stock data to collection, to prevent query errors if \Magento\CatalogInventory\Model\ResourceModel\Stock\Status::addStockDataToCollection is called twice with the same collection before the collection is loaded. Also, this processor logic is very similar to
\Magento\CatalogInventory\Helper\Stock::addIsInStockFilterToCollection, if we preserved this here, we'd have to adapt also this method.
There was a problem hiding this comment.
if (!$collection->hasFlag($stockFlag)) {
| * @return Collection | ||
| */ | ||
| public function process(Collection $collection) | ||
| public function process(Collection $collection, $isFilterInStock = true) |
There was a problem hiding this comment.
Please, adapt \Magento\ConfigurableProduct\Test\Unit\Model\Product\Type\Collection\SalableProcessorTest to cover new parameter cases
| if ($salableOnly) { | ||
| $collection = $this->salableProcessor->process($collection); | ||
| } | ||
| $collection->setFlag('has_stock_status_filter',false); |
There was a problem hiding this comment.
Please remove this line: $collection->setFlag('has_stock_status_filter',false);
| $collection = $this->salableProcessor->process($collection); | ||
| } | ||
| $collection->setFlag('has_stock_status_filter',false); | ||
| $collection = $this->salableProcessor->process($collection, false); |
There was a problem hiding this comment.
$collection = $this->salableProcessor->process($collection, $salableOnly);
| $this->salableProcessor | ||
| ->expects($this->once()) | ||
| ->method('process') | ||
| ->with($productCollection) |
There was a problem hiding this comment.
process() method is usually called with its second parameter as false, but test uses default method second param value, which value is true.
Change ->with($productCollection) with ->willReturnMap([ [$productCollection, false, $productCollection] ]);
| ->expects($this->once()) | ||
| ->method('process') | ||
| ->with($productCollection) | ||
| ->will($this->returnValue($productCollection)); |
There was a problem hiding this comment.
->will($this->returnValue($productCollection)): Remove this line after applying previous change, as return values are specified in map depending on method call params.
|
@adrian-martinez-interactiv4 thank you for this super detail and informative review. I will apply all requested changes and will follow them for all my new contributions! I will update PR with them tomorrow. |
|
@adrian-martinez-interactiv4 sorry for the delay in reply. Had some blockers. Previously, it was private function loadUsedProducts(\Magento\Catalog\Model\Product $product, $cacheKey, $salableOnly = false)
{
$dataFieldName = $salableOnly ? $this->usedSalableProducts : $this->_usedProducts;
if (!$product->hasData($dataFieldName)) {
$usedProducts = $this->readUsedProductsCacheData($cacheKey);
if ($usedProducts === null) {
$collection = $this->getConfiguredUsedProductCollection($product);
if ($salableOnly) {
$collection = $this->salableProcessor->process($collection);
}
$usedProducts = array_values($collection->getItems());
$this->saveUsedProductsCacheData($product, $usedProducts, $cacheKey);
}
$product->setData($dataFieldName, $usedProducts);
}
return $product->getData($dataFieldName);
}as you can see the I also did this public function process(Collection $collection, $isFilterInStock = true)
{
$collection->addAttributeToFilter(
ProductInterface::STATUS,
Status::STATUS_ENABLED
);
$stockFlag = 'has_stock_status_filter';
if (!$collection->hasFlag($stockFlag) || !$collection->getFlag($stockFlag)) {
$stockStatusResource = $this->stockStatusFactory->create();
$stockStatusResource->addStockDataToCollection($collection, $isFilterInStock);
$collection->setFlag($stockFlag, true);
}
return $collection;
}I used this flag just to add stock status. This issue is quite tricky in debugging and as a result difficult to explain all details briefly. Please, if you will have time for detail test, pull my changes and try to debug mentioned methods. Sorry if my description of changes is not so informative. Thank you so much, |
adrian-martinez-interactiv4
force-pushed
the
bugfix/configurable-product-collection-salable-only-filter
branch
from
f2abac5
to
edf2dbf
Compare
|
Hi @coderimus , I've been reviewing why proposed changes didn't work, and I think I have found the main problem with this issue. The method If you are comfortable with changes, you can squash commits and push force your branch so you can get full credit for this PR, as it belongs to you. |
adrian-martinez-interactiv4
changed the title
coderimus
force-pushed
the
bugfix/configurable-product-collection-salable-only-filter
branch
from
edf2dbf
to
b349218
Compare
|
@coderimus thank you for contributing. Please accept Community Contributors team invitation here to gain extended permissions for this repository. |
…sible on frontend as sellable #12936
|
@coderimus now that this is accepted and merged, would you consider to backport it to 2.3 if needed? Thank you in advance |
|
@adrian-martinez-interactiv4 thank you! Sure, I will prepare the backport for this issue to 2.3 on Monday next week and will add you as a reviewer! :) |
…rable product visible as sellable #13417
Accepted Public Pull Requests: - #13251: Translate attribute label with default translation helper function (by @cmuench) - #12765: #12717 - Catalog Products List widget is not displayed on Storefront (by @RostislavS) - #13417: [Backport 2.3] #12936 out-of-stock options for configurable product visible as sellable (by @coderimus) Fixed GitHub Issues: - #12717: Catalog Products List widget is not displayed on Storefront (reported by @alena-marchenko) has been fixed in #12765 by @RostislavS in 2.3-develop branch Related commits: 1. d30696e 2. 55a3ded
|
We're having a similar issue that is at least as important to fix but isn't solved by this patch. In previous comments of @coderimus: Though when 1 option type is used let's say 'size' with S, M, L, XL. And we make sure that option M has qty = 0 and is out of stock + the Inventory setting to display products out of stock is enabled, that option 'M' disappears from the source code like described in #12936 (comment) by @adrian-martinez-interactiv4 This can't be intended behaviour. We have to be able to see all the options AND make sure that the option that is out of stock either is crossed out or is hidden, but at least outputted in the source code which is not the case with this patch. |
|
@duckchip, I am also having the same problem with 2.2.5. If I have a set of configurable products with simple attributes (like size), and some of them are out of stock, then the text swatches disappear. This happens regardless of my Store > Configuration > Catalog > Inventory being set to "Display Out of Stock Products : Yes" Is this part of the same issue, or something different? |
|
@duckchip @scrivvles We're currently having similar issues to you two. I've taken a look and found where this stems from, it is indeed related to the PR for this issue, more specifically the code which gets the collection of products when The false parameter results in the following function not setting the By not setting that flag, whenever the collection is processed via the Magento\ConfigurableProduct\Model\Product\Type\Collection\SalableProcessor::process() method, out of stock products are then removed. This happens because the aforementioned process method calls the This results in a select query on the Collection where products stock status is @scrivvles I too believe that we should be able to set @dmanners @coderimus @adrian-martinez-interactiv4 Keen to hear your thoughts. It could pretty easily be fixed with a conditional to check whether the above configuration is set to No, if it is then within the SalableProcessor class pass the I'd be happy to create a PR for this. |
|
@josh-carter thank you so much for this detailed review. As I understood, the main idea of the As you can find the This 3rd parameter is a So it means that this param should be always false and we have to have all sub products and if we will not have force Also, you can see that this method getSalableUsedProductCollections is also deprecated which means that by calling getUsedProducts we need to have all subproducts. But with one condition in mind described in the next paragraph. And the main part here: the loadUsedProductCollection method is already sensitive to the stock showing setting. If you have "Display out of stock products" to Yes you will have collection consists of those methods. If setting eq to No only products which are in stock will be displayed. Possible solutionFrom my point of view, the problem is in the Swatch module and how it operates with the received collection. Or the described problem is caused be another place in the code after the call to the getUsedProducts method. Need to be investigated. From my point of view, we are not able to have any settings related condition because it will break the original flow. @dmanners @adrian-martinez-interactiv4 please, when you will have time check this comment. With a great respect, |
|
@coderimus I think we need to move away from the The problem is from So this could still be seen as "not the problem" if i'm making sense, because really this is still fine, the collection doesn't have any Once the We see the check for the flag, which isn't set in the So is there actually a problem? And if so, where? Personally i think there is a problem, or maybe a lack of clarity. When i read the configuration for For me I think there are a couple of options: "Fix" This is something i've personally implemented yesterday for a client following on from this issue. Another option could be to add some more clarification to the configuration comment, just something simple saying that out of stock swatches/options wont be displayed. Maybe theres a possibility that we could add a further configuration option somewhere to define some sort of further out of stock product display. ConclusionI'd say right now its pretty vague as to whether everything is working how it "should". Whether what we're talking about is actually a fix or whether its technically a feature. For now I'll wait to see what the others say. |
|
@dmanners @adrian-martinez-interactiv4 @coderimus Found this issue today which is related to our convo - #10454 |
|
For what it's worth, I agree with you completely @josh-carter This is a classic example of POLA - if everyone interprets something a certain way without any additional prompting or clarification, then that is the way it should work. (Or it should be modified). I also would love to see your fix if you don't mind sharing it. |
|
@scrivvles Linked it on that issue i've mentioned, the "fix" is at - https://github.com/interjar/configurable-child-visibility But to be honest, i aim to get this fixed or at least cleared up in the core too so hopefully that fix extension will become redundant! |
This PR is for configurable product out-of-stock option fix with several improvements for generally used methods. Please, check Description and Code changes explanation sections for more detail information.
Description
If one of the configurable product options will be out-of-stock but has status enabled, it will be visible on the frontend (product page with swatches).
But, when a customer selects it and tries to add to the cart the next message will be shown: "You need to choose options for your item." And item will not be added to the cart.
The main problem here that the out-of-stock and not salable option is shown as normal one without any notification before adding to the cart and when a customer tries to add it to the cart this generates useless load on the system.
Fixed Issues (if relevant)
The most relevant issue is #5948
Despite the fact that this issue was closed, it still can be reproduced on the Magento2 2.2.2.
Manual testing scenarios
Accepted criteria: out-of-stock option should be shown as not-selectable and there is no ability to add it to the cart.
Code changes explanation
Magento\ConfigurableProduct\Model\Product\Type\Configurable::loadUsedProducts(\Magento\Catalog\Model\Product $product, $cacheKey, $salableOnly = false)method had 2 issues:$salableOnlyvariable never used since 100.2.0 because of deprecated methodgetSalableUsedProductsand, as a result, the stock status filter was not added to the collection object$salableOnlyfilter will betrueand$this->salableProcessor->process($collection)will be executed, it will not get any effect because items were previously loaded in$collection = $this->getConfiguredUsedProductCollection($product);It means that collection will not be updated with the real stock status data.Items (
$this->_items) were loaded before by$this->getItems()method in$collection->addMediaGalleryData()and$collection->addTierPriceData().This is why I have moved
$collection->addMediaGalleryData()and$collection->addTierPriceData()to another method and execute them after all other collection modifications.Also, I removed the
if($salableOnly) {...}condition because there is no need to skip it. With other provided changes product collection will contain all products with correct stock statuses. There will not be skipped products which is equal to current behaviour.The
$collection->setFlag('has_stock_status_filter',false);line was added to add stock status filter.Magento\ConfigurableProduct\Model\Product\Type\Collection\SalableProcessor::process(Collection $collection, $isFilterInStock = true)has 3 improvements:This method now has second param
$isFilterInStockwith default value equals totrue. It makes it more flexible because now you can just add stock status to collection or filter it by status equals to 1 (in stock)!$collection->hasFlag($stockFlag) || !$collection->getFlag($stockFlag)this change allow develoeprs to check what value has collcetion flag. This was added because thehasFlag()' method checks only if current key exists -array_key_exists($flag, $this->_flags);`$stockStatusResource->addStockDataToCollection($collection, $isFilterInStock);now uses$isFilterInStockvalue as parametrMagento\CatalogInventory\Model\ResourceModel\Stock\Statushas next changesaddStockStatusToSelect,addStockDataToCollection,addIsInStockFilterToCollectionnow have$websiteIdwithnullas default value and$this->getWebsiteId($websiteId)method for website condition checkContribution checklist