Stock management regression fixes #28620

vedanshujain · 2020-12-17T17:00:20Z

All Submissions:

Have you followed the WooCommerce Contributing guideline?
Does your code follow the WordPress' coding standards?
Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Changes proposed in this Pull Request:

Stock managment fixes:

Use $already_reduced_stock instead of also considering $refunded_item_quantity while deleting orders. This will bring back part of Stock quantity restocked twice when deleting Order line item after refund #27504 again, but for now this seems to be the best solution for countering Manually deleting product from an order no longer adjusts stock #28605. It needs discussion whether deleting a line item completely should also undo any refund related changes on it or not.
Also mark stock_reduced flag on order if any of the line item has any _reduced_stock flag. This will allow for stock restoring logic to work properly when order is cancelled.
Only adjust line item stock when order is in processing, completed or on-hold status state, because we need to reduce stocks on these status only. Stock adjustments in refunds or when changing statuses is already taken care of by their specific hooks.

Closes #28605 #28505 #28585 . Reopen #27504 for more consideration

How to test the changes in this Pull Request:

Follow steps in Manually deleting product from an order no longer adjusts stock #28605 Stock reduced when 'Pending Payment' order is set to 'Cancelled' #28505 and Stock is decreased when you hit Update for already Cancelled order #28585

Other information:

Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your changes, as applicable?
Have you successfully run tests with your changes locally?

Changelog entry

Fix - Adjust stock items only for statuses which reduces the stock.

This loops and tests stock changes through ALL supported WooCommerce statuses. Hopefully this will make our inventory tracking more robust.

1. Use $already_reduced_stock instead of also considering $refunded_item_quantity while deleting orders. This will bring back part of #27504 again, but for now this seems to be the best solution for countering #28605. It needs discussion whether deleting a line item completely should also undo any refund related changes on it or not. 2. Also mark `stock_reduced` flag on order if any of the line item has any `_reduced_stock` flag. This will allow for stock restoring logic to work properly when order is cancelled. 3. Only adjust line item stock when order is in `processing`, `completed` or `on-hold` status state, because we need to reduce stocks on these status only. Stock adjustments in refunds or when changing statuses is already taken care of by their specific hooks.

roykho

With these new changes, are the older unit tests still needed? https://github.com/woocommerce/woocommerce/blob/master/tests/php/includes/admin/class-wc-admin-functions-test.php#L72-L288

Otherwise your PR tested well.

peterfabian

Hi, mostly it seems to be working fine, I just found a couple of issues/questions:

I'm not sure if this is in scope, but these cases seems to not work for me as I would expect:

from wp admin, adding a product to an order does not reduce the stock (and any subsequent changes also do nothing to the stock level for me)
reinstantiating an order with refunds

Complete list of what I tested:

create order from the front end---ok
reduce stock from wp admin by edit button---ok
increase stock from wp admin by the edit button---ok
refund some stock---ok
delete line item---ok
add the line item back---stock level not ok
new order from front end---ok
refund 2 items---ok
Cancel the order---ok
back to On hold---not ok (it decreases based on the full stock amount, not the refunded amount)
editing qty of item in the order---ok (fixes the overall count to correct count, i.e. fixes the problem in the previous step)
removing line item---ok

Maybe we should update the notice when deleting an item from an order, too?

tests/php/includes/wc-stock-functions-tests.php

peterfabian · 2020-12-18T12:03:54Z

tests/php/includes/wc-stock-functions-tests.php

+		$this->assertEquals( $before_transition, $product->get_stock_quantity() );
+
+		// Changing status from UI also calls this method in metadata save hook. This has impact on stock levels, so simulate it here as well.
+		wc_save_order_items( $order, $order->get_items() );


Nice that you noticed this.

General remark: I wonder if this is somewhat fragile for a test, as we might miss something like this easily.

Yeah, I missed this initially but only noticed it when tests were passing but there was clearly a bug while doing the same steps via interface. Adding this made the tests fail when I was expecting them to, so I agree that code around these parts is bit fragile, and is a good candidate for refactoring efforts.

peterfabian · 2020-12-18T12:05:55Z

tests/php/includes/wc-stock-functions-tests.php

+	public function test_status_transition_stock_reduce_to_stock_reduce() {
+		foreach ( $this->order_stock_reduce_statuses as $order_status_from ) {
+			foreach ( $this->order_stock_reduce_statuses as $order_status_to ) {
+				$this->transition_order_status_and_assert_stock_quantity( $order_status_from, $order_status_to, 9, 9 );


Just to confirm I understand, this is supposed to test that the stock isn't reduced twice, right? Do you think it would make sense to add it to the assert message to be clear about the intent?

Looks like adding it in assert messages is a bit confusing because assert can fail whenever expected and actual stock level would not match, which can happen for a lot of reason than just stock being reduced twice (for example, there could be a bug which increases the stock, and assert message will say stock is not reduced twice).

But I agree that its a bit unclear at the moment, so I have added expectations in doc comments for all these tests in 5171108. Let me know if these makes sense.

peterfabian · 2020-12-18T12:08:16Z

tests/php/includes/wc-stock-functions-tests.php

+	 * @var array List of statuses which have no impact on inventory.
+	 */
+	public $order_stock_no_effect_statuses = array(
+		'wc-failed',


Should failed not restore stock eventually? I'm just not sure the assumptions on all those statuses are aligned.
Checking https://docs.woocommerce.com/document/managing-orders/ it seems that order can be in failed status also because

The hold stock window expired without a response

So I wonder if there isn't any potential problem there. But probably not, since failed comes after pending in the diagram and it does not reduce the stock level, only puts the stock on hold, so there is nothing to restore. Okay, nevermind :) I'll leave this here just in case someone would wonder about the same thing.

vedanshujain · 2020-12-21T10:48:09Z

@peterfabian

from wp admin, adding a product to an order does not reduce the stock (and any subsequent changes also do nothing to the stock level for me)

This is expected, we removed stock changes unless status is also changed to prevent ghost stock issues. See #26642.

reinstantiating an order with refunds

This is also kinda expected, because as part of this PR I am reverting some of the related changes to have more discussion. See from the PR description:

Use $already_reduced_stock instead of also considering $refunded_item_quantity while deleting orders. This will bring back part of #27504 again, but for now this seems to be the best solution for countering #28605. It needs discussion whether deleting a line item completely should also undo any refund related changes on it or not.

claudiosanches

Seems to be working as expected, I followed all steps to reproduce and I agree with your comments in #28620 (comment)

I'm not merging now because it's still missing unit test to run on Travis (in localhost those works fine) after I fixed some coding standards.

addressed feedback comments

vedanshujain · 2020-12-21T17:06:45Z

Merging because manually tested the failed e2e tests (able to delete refund). there is another one about setup wizard failing, but it does not looks related.

vedanshujain added 2 commits December 17, 2020 22:28

Add unit tests for all status change ops.

84d60e5

This loops and tests stock changes through ALL supported WooCommerce statuses. Hopefully this will make our inventory tracking more robust.

vedanshujain marked this pull request as ready for review December 17, 2020 17:06

vedanshujain requested review from a team, peterfabian and roykho and removed request for a team December 17, 2020 17:06

roykho previously approved these changes Dec 17, 2020

View reviewed changes

peterfabian previously requested changes Dec 18, 2020

View reviewed changes

vedanshujain added 2 commits December 21, 2020 15:42

Merge branch 'master' into fix/28505

b4b03f3

Better docs for test functions to specify expectations.

5171108

vedanshujain dismissed roykho’s stale review via 5171108 December 21, 2020 10:35

vedanshujain requested a review from claudiosanches December 21, 2020 10:48

vedanshujain added the release: highlight Issues that have a high user impact and need to be discussed/paid attention to. label Dec 21, 2020

Fixed coding standards

8568a9b

claudiosanches approved these changes Dec 21, 2020

View reviewed changes

vedanshujain merged commit 800c6a6 into master Dec 21, 2020

vedanshujain deleted the fix/28505 branch December 21, 2020 17:06

woocommercebot added release: add changelog Mark all PRs that have not had their changelog entries added. [auto] release: add testing instructions PRs that have not had testing instructions added to the wiki. [auto] labels Dec 21, 2020

vedanshujain removed the release: add changelog Mark all PRs that have not had their changelog entries added. [auto] label Dec 21, 2020

vedanshujain added this to the 4.9.0 milestone Dec 21, 2020

juliaamosova added testing instructions added and removed release: add testing instructions PRs that have not had testing instructions added to the wiki. [auto] labels Dec 22, 2020

This was referenced Dec 23, 2020

Stock reduced when 'Pending Payment' order is set to 'Cancelled' #28505

Closed

Stock is decreased when you hit Update for already Cancelled order #28585

Closed

Stock quantity restocked twice when deleting Order line item after refund #27504

Closed

This was referenced Jan 10, 2021

Updating a Failed order reduces stock #28772

Closed

Removing item from order doesn't restore stock #28756

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Stock management regression fixes #28620

Stock management regression fixes #28620

Uh oh!

vedanshujain commented Dec 17, 2020 •

edited

Loading

Uh oh!

roykho left a comment

Uh oh!

peterfabian left a comment

Uh oh!

Uh oh!

peterfabian Dec 18, 2020

Uh oh!

vedanshujain Dec 21, 2020

Uh oh!

peterfabian Dec 18, 2020

Uh oh!

vedanshujain Dec 21, 2020

Uh oh!

peterfabian Dec 18, 2020 •

edited

Loading

Uh oh!

vedanshujain commented Dec 21, 2020

Uh oh!

claudiosanches left a comment •

edited

Loading

Uh oh!

vedanshujain commented Dec 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Stock management regression fixes #28620

Stock management regression fixes #28620

Uh oh!

Conversation

vedanshujain commented Dec 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

All Submissions:

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

Other information:

Changelog entry

Uh oh!

roykho left a comment

Choose a reason for hiding this comment

Uh oh!

peterfabian left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

peterfabian Dec 18, 2020

Choose a reason for hiding this comment

Uh oh!

vedanshujain Dec 21, 2020

Choose a reason for hiding this comment

Uh oh!

peterfabian Dec 18, 2020

Choose a reason for hiding this comment

Uh oh!

vedanshujain Dec 21, 2020

Choose a reason for hiding this comment

Uh oh!

peterfabian Dec 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vedanshujain commented Dec 21, 2020

Uh oh!

claudiosanches left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vedanshujain commented Dec 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

vedanshujain commented Dec 17, 2020 •

edited

Loading

peterfabian Dec 18, 2020 •

edited

Loading

claudiosanches left a comment •

edited

Loading