inventory icon indicating copy to clipboard operation
inventory copied to clipboard

#3062 Added reservation quantity check to frontend

Open greezlu opened this issue 2 years ago • 13 comments

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)

  1. Fixes magento/inventory#3062
  2. Fixes magento/inventory#3060
  3. Fixes magento/magento2#31117

Manual testing scenarios (*)

  1. Create a new product.
  2. Create url key for product access from frontend.
  3. Set product stock quantity to 1.
  4. Create a new order with this product without shipping it.
  5. Go to the product view page on frontend.
  6. 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)

greezlu avatar May 13 '22 11:05 greezlu

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.

Djohn12 avatar May 30 '22 08:05 Djohn12

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?

greezlu avatar May 30 '22 08:05 greezlu

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 avatar May 30 '22 08:05 Djohn12

@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

greezlu avatar May 30 '22 08:05 greezlu

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 :)

Djohn12 avatar May 30 '22 10:05 Djohn12

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

Djohn12 avatar May 30 '22 10:05 Djohn12

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.

greezlu avatar May 30 '22 10:05 greezlu

Thanks for the feedback !

I'll try playing with that collection this afternoon then, I'll keep you in touch

Djohn12 avatar May 30 '22 11:05 Djohn12

After some debugging in magento/module-inventory-catalog/Model/ResourceModel/AddStockDataToCollection.php :

  • Since we SUM() the reservations I believe we have to use a GROUP BY instruction in order to get distinct rows for each products.
  • Since SUM() will return NULL on product not having any reservations yet, the comparison > 0 always returned FALSE. I added an IFNULL() 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 ?

Djohn12 avatar May 30 '22 12:05 Djohn12

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

Djohn12 avatar May 31 '22 08:05 Djohn12

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 avatar Jun 01 '22 14:06 Djohn12

@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:

  1. Single product default stock.
  2. Single product custom stock.
  3. Collection default stock.
  4. 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.

greezlu avatar Jun 02 '22 20:06 greezlu

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

GetStockReserved.tar.gz InventoryIndexerPatch.tar.gz

ant0x64 avatar Jun 29 '22 14:06 ant0x64

@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

marwan-corals avatar Dec 04 '22 10:12 marwan-corals

Don't think this will ever be reviewed.

greezlu avatar Feb 27 '23 20:02 greezlu