-
Notifications
You must be signed in to change notification settings - Fork 9.4k
Fix for the issue #25675 #25711
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix for the issue #25675 #25711
Conversation
Merge from Magento develop in forked repo
Hi @molneek. Thank you for your contribution
For more details, please, review the Magento Contributor Guide documentation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @molneek, thank you for your contribution!
Could you please drop a few words about the root reason of the issue and how the applied solution solves it? Unfortunately, it's not clear without performing an additional investigation.
Please, also note, due to Magento Definition of Done the changes should be covered by tests. Could you please cover your fix by an automated test?
Thank you!
Hi @dmytro-ch, thank you for review. The root reason for the issue is that during checking a product inventory this method calls several times before item is saved to a DB and after. But the method uses item id to collect inventory information. And when an item is not saved to a DB but checked inventory for the product, we collect incorrect data, that in next checking will be added to current qty and corrupt the result. Also, this fix does not require additional automated tests, as all Magento current tests are good with this solution. And this solution fixes the process of corrupting data and does not change the method signature |
@molneek thank you for the details! |
@magento give me test instance |
Hi @dmytro-ch. Thank you for your request. I'm working on Magento instance for you |
Hi @dmytro-ch, here is your new Magento instance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@molneek, unfortunately, there is no test that covers the aforementioned case.
Please create at least a simple unit test (e.g. \Magento\CatalogInventory\Test\Unit\Model\Quote\Item\QuantityValidator\QuoteItemQtyListTest
) which will make sure the quantity calculation logic is performed correctly, according to the scenario you've just described above.
It can be easily accomplished by calling the \Magento\CatalogInventory\Model\Quote\Item\QuantityValidator\QuoteItemQtyList::getQty
method with different sets of parameters and asserting the result.
Thank you!
Hello @molneek, I'm closing this PR now due to inactivity. |
Hi @molneek, thank you for your contribution! |
Hi @molneek. Thank you for your contribution
For more details, please, review the Magento Contributor Guide documentation. |
Hello @dmytro-ch, I've added a unit test as you asked, please verify this merge request one more time |
@dmytro-ch I close this PR due to open the new one for the 2.4 version #26650 |
Hi @molneek, thank you for your contribution! |
Description (*)
This pull request should fix issue #25675
Fixed Issues (if relevant)
Manual testing scenarios (*)
Described here #25675