inventory
inventory copied to clipboard
#3062 Added reservation quantity check to frontend
Description (*)
Adding product reservation quantity check to the frontend. To prevent situations where product is in "In Stock" status but you can't add it to a cart.
Fixed Issues (if relevant)
- Fixes magento/inventory#3062
- Fixes magento/inventory#3060
- Fixes magento/magento2#31117
Manual testing scenarios (*)
- Create a new product.
- Create url key for product access from frontend.
- Set product stock quantity to 1.
- Create a new order with this product without shipping it.
- Go to the product view page on frontend.
- The product should be in "Out of stock" status.
Questions or comments
Without this additional check product will be in "In Stock" status but you will get an error trying to add such product to your cart.
Also, this check should avoid all other product types except Simple.
Contribution checklist (*)
- [x] Pull request has a meaningful description of its purpose
- [x] All commits are accompanied by meaningful commit messages
- [x] All new or changed code is covered with unit/integration tests (if applicable)
- [x] All automated tests passed successfully (all builds are green)
Hi,
Thanks for that contribution @greezlu Are you still making changes to your code ?
I tried applying your patch on my local env and it does seem to fix the issue on product page effectively but products are still marked as In stock on category pages even when salable is 0.
Hi, @Djohn12 Yeah, turns out it not that simple) I'm still working on it. But I think I'm close.
Should I tag you after I finish?
I believe I can take some time today to assist you if I can help ?
Need to fix this issue for a client.
Do you have any trail to follow to make it work on category pages ? Any point you might need assistance to look into ?
@Djohn12 Actually yes, you can help.
I found different way to approach. There's select queries to load product(s) which needs to be modified. I've done it incorrectly, may be you can fix it. Main idea is the same: check reservations for simple products. But to do it on a db query stage. Here are patches, where you can find those queries. Good luck.
For collections:
magento/module-inventory-catalog
github-issue-3062-1.patch.zip
For single product:
magento/module-inventory-indexer
github-issue-3062-2.patch.zip
Thanks for all that information @greezlu !
Looking into your zip patches and the stack trace for Magento Catalog page I'm not sure updating the collection will do the trick though.
As far as I can tell, the only check to render the "add to cart" CTA on catalog pages depends on the call $_product->isSaleable()
in magento/module-catalog/view/frontend/templates/product/list.phtml
That call goes through
-
vendor/magento/module-catalog/Model/Product::isSalable()
- that method internally calls
vendor/magento/module-catalog/Model/Product::isAvailable()
- which ends calling
magento/module-catalog/Model/Product/Type/AbstractType::isSalable($product)
- which is defined as following (as per Magento 2.4.3-p2) :
/**
* Check is product available for sale
*
* @param \Magento\Catalog\Model\Product $product
* @return bool
*/
public function isSalable($product)
{
$salable = $product->getStatus() == \Magento\Catalog\Model\Product\Attribute\Source\Status::STATUS_ENABLED;
if ($salable && $product->hasData('is_salable')) {
$salable = $product->getData('is_salable');
}
return (bool)(int)$salable;
}
I don't see anything allowing for the template update after that call.
I'm thinking about modifying magento/module-catalog/Model/Product/Type/AbstractType::isSalable($product)
to make it call magento/module-inventory-indexer/Model/IsProductSalable::getIsSalable(string $sku, int $stockId)
you already fixed for product pages.
I reckon this could also be part of a Plugin/Observer call, not sure what would be the best implementation.
Would you consider it a viable solution ?
Let me know if what you think :)
To add more information :
The magento/module-checkout/Controller/Cart/Add.php
Action called on "add to cart" action does call magento/module-inventory-indexer/Model/IsProductSalable::getIsSalable(string $sku, int $stockId)
to check for product salability.
I feel it'd be right to make that same check when displaying product availability on catalog pages and get consistent results
I'm not sure updating the collection will do the trick though.
Block of the list.phtml
template loads products collection. You see call isSalable()
for each of them. So I think it will.
Seems right to me to make that same check to display product availability on catalog pages as well.
It's not the same. You're trying to add only one product at a time to the cart. Your performance don't mind additional check. But doing it for each product in a list is quite expensive.
I reckon this could also be part of a Plugin/Observer call, not sure what would be the best implementation.
I believe that there can be some place for plugin or observer to do similar trick as I did before. But you will end up with performance downgrade due to possible amount of listed products. And I will not investigate such option because of that reason.
I think my patches found right spot to change query. It just needs to be done right.
Thanks for the feedback !
I'll try playing with that collection this afternoon then, I'll keep you in touch
After some debugging in magento/module-inventory-catalog/Model/ResourceModel/AddStockDataToCollection.php
:
- Since we
SUM()
the reservations I believe we have to use aGROUP BY
instruction in order to get distinct rows for each products. - Since
SUM()
will returnNULL
on product not having any reservations yet, the comparison> 0
always returnedFALSE
. I added anIFNULL()
condition for that case.
Here's my new version of your patch on magento/module-inventory-catalog/Model/ResourceModel/AddStockDataToCollection.php
, this does seem to do the trick on category pages
github-issue-3062-category-page.zip
I believe we just need to apply the same adjustments on magento/module-inventory-indexer/Model/ResourceModel/GetStockItemData.php
?
Do you already know where the calls of GetStockItemData
would be used in favor of AddStockDataToCollection
?
Hi,
I believe my version of the patch was malformed, here is the version we can apply using composer github-issue-3062-category-page.zip
Hi @greezlu,
Any update on this ? Did check out my changes ? Don't want to put any pressure, just not sure you've been notified of my last messages :sweat_smile:
Have a great day
@Djohn12 Yeah, I like your solution. I tested and it works fine. Feel free to find any new ways to test it. Basically there's four scenarios to test:
- Single product default stock.
- Single product custom stock.
- Collection default stock.
- Collection custom stock.
So feel free to test any of them.
Do you already know where the calls of GetStockItemData would be used in favor of AddStockDataToCollection ?
Magento\InventoryIndexer\Model\IsProductSalable:47
Final patches
For collections:
magento/module-inventory-catalog
github-issue-3062-1.patch.zip
For single product:
Module: magento/module-inventory-indexer
github-issue-3062-2.patch.zip
Disabling cache for single product
Single product stock data is being cached by additional "class layer" preference in di.xml
for GetStockItemDataInterface
.
I think that such cache will make our changes not visible, so I disabled those preferences. Feel free to remove it. But I'll remove it from PR.
As I understood why this problem still exists, Magento tries to rewrite the Catalog Inventory module regarding Multi Inventory Sources but hasn't done it yet. So this problem isn't prioritized but imho
Attached the module that fixed this issue in more clean way
@antondkv we got errors on shipment creation related to ReindexOnReserved,
DDL statements are not allowed in transactions at /var/www/vhosts/tatayab.com/dev/vendor/magento/framework/DB/Adapter/Pdo/Mysql.php:530)"} [] after debugging it turned out its due to the table below
RENAME TABLE inventory_stock_3 TO inventory_stock_3_outdated,inventory_stock_3_replica TO inventory_stock_3,inventory_stock_3_outdated TO inventory_stock_3_replica
I think the solution is not elegant as reindexing the entire stock on the change of one SKU is not efficient at all
Don't think this will ever be reviewed.