search

AmpersandHQ / magento2-disable-stock-reservation

This module disables the inventory reservation logic introduced as part of MSI in Magento 2.3.3
GNU Lesser General Public License v3.0
211 stars 60 forks source link

Change SourceDeductionService class in the Refund observer #92

Closed markfischmann closed 11 months ago

markfischmann commented 1 year ago

Change a class in the refund observer to use the module's own SourceDeductionService instead of the native one. This causes refunds to not change stock status in cataloginventory_stock_item, cataloginventory_stock_status and inventory_source_item if the product goes from "out of stock" to "in stock" following a refund.

Instead of using the vendor/ampersand/magento2-disable-stock-reservation/src/Model/SourceDeductionService/PatchedSourceDeductionService.php class, it used the vendor/magento/module-inventory-source-deduction-api/Model/SourceDeductionService.php class.

zeeshantpp commented 1 year ago

@convenient @markfischmann can we also review and merge this please?

convenient commented 1 year ago

Hey @markfischmann is it expected that this does not work unless it is paired with #93 ?

I've pushed a test and we can see in the 2.4.3 instance and higher we're still getting

1) CheckoutCest: Product goes back in stock when order is refunded
 Test  tests/acceptance/CheckoutCest.php:productGoesBackInStockWhenOrderIsRefunded
 Step  Assert equals 1,0,"Product did not go to is_in_stock=1 after a refund"
 Fail  Product did not go to is_in_stock=1 after a refund
Failed asserting that 0 matches expected 1.

I've created a PR and branch https://github.com/AmpersandHQ/magento2-disable-stock-reservation/pull/107 which combines #92 and #93 and we can see that 2.4.5 and higher are going green with the combination.

This testing infra is new, so there may be some bug or issue i've not gotten to the bottom of yet, but just thought i'd check first.

markfischmann commented 1 year ago

@convenient To be completely honest, I can't remember exactly, but on our production website running 2.4.3-p1 and recently 2.4.3-p3, there have not been any issues with both PRs working together.

I made two seperate PRs because there were two seperate issues, but I need them both to have a working system, so I guess we can say that they should be combined.

convenient commented 1 year ago

Thanks @markfischmann Hmmm very strange that the combined tests are only passing for 2.4.5 and higher, would prefer to continue to support 2.4.3 if at all possible.

I'll give this a go through the code in xdebug I guess and see why Product did not go to is_in_stock=1 after a refund is still occurring :/ seeing as you say you needed it in 2.4.3 to get it working.

Did you do any other customisations that may need included do you think?

markfischmann commented 1 year ago

@convenient I wonder if it's not due to the fact that you're using the Refund API, which might possibly be different than the Credit Memo refund thing. I'll try it out during the day when I have a bit of spare time and I'll get back to you when I have more information.

convenient commented 1 year ago

thanks @markfischmann that may be the case :) using the API for tests for ease of writing them

markfischmann commented 1 year ago

@convenient I've checked a bit around, it seems that the RefundOrder API does not go through the same process as when creating a credit memo from the admin panel.

I don't believe the API does anything with the "return_to_stock_items" parameter, and I don't see anything relating to a "back_to_stock" order item attribute.

The "back_to_stock" attribute is used by the Inventory thing processor (Observer vendor/magento/module-sales-inventory/Observer/RefundOrderInventoryObserver.php) and the Ampersand module (Observer app/code/Ampersand/DisableStockReservation/Observer/RestoreSourceItemQuantityOnRefundObserver.php) to dictate which product's qty should go back in stock or not.

When using the credit memo process via the admin panel, we go through the following class vendor/magento/module-sales/Controller/Adminhtml/Order/CreditmemoLoader.php that sets the "back_to_stock" attribute to the order item. The API does not go through this class, which is why the tests are failing I guess...

Currently testing the code on a 2.4.3 version of Magento.

convenient commented 1 year ago

I did not mean to close these as invalid, apologies I made an error, I'm continuing the work in https://github.com/AmpersandHQ/magento2-disable-stock-reservation/pull/107 thanks

convenient commented 11 months ago

Hello @markfischmann

I am closing this PR and have cherry picked the relevant commits into https://github.com/AmpersandHQ/magento2-disable-stock-reservation/releases/tag/1.2.9

In https://github.com/AmpersandHQ/magento2-disable-stock-reservation/pull/120 I was able to reproduce your bug on the affected version of Magento, but the only change necessary to correct the is_in_stock handling was the interface fix, I didnt need any of the DecrementQtyForMultipleSourceItem fixes.

Please read through the notes on https://github.com/AmpersandHQ/magento2-disable-stock-reservation/pull/120, I tested

Your fix to the interface definitely fixed the issue, but i couldnt see any other problems which necessitated DecrementQtyForMultipleSourceItem.

There's a lot more test infra and coverage now, so maybe if you can point out any other requirement to replicate this issue in your case I can try and repro but based on my testing, this issue is resolved.

Thanks for much for your patience on this, with all the varying versions of Magento we're trying to support with this module it's a bit tricky to ensure the change is appropriate, and that we have the necessary test coverage.

If you have any more details please raise a new issue and thanks for your contribution.