Skip to content

Limit stock changes for order items to status methods for consistency. #26642

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.

Sign up for GitHub

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

Jump to bottom
Merged
peterfabian merged 4 commits into from
Jul 24, 2020
Merged

Limit stock changes for order items to status methods for consistency. #26642

peterfabian merged 4 commits into from
Jul 24, 2020
+130 −33

Conversation

@vedanshujain
Copy link
Contributor

@vedanshujain vedanshujain commented May 29, 2020 •
edited
Loading

All Submissions:

Changes proposed in this Pull Request:

Stock changes should be applied only when changing statuses.

Note that this means if a plugin/extension is reducing/adding stock on completed orders, then they would have to also call wc_maybe_adjust_line_item_product_stock manually.

Closes #26607.

How to test the changes in this Pull Request:

  1. Start creating a new order in the admin view, and add a product with managed stock.
  2. Without this patch, you will immediately see that product stock is reduced as soon as you add a product. With this patch, stock will be reduced when order is saved and moved into processing or completed status.

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

Tweak - Limit stock changes for order items to status methods for consistency.

vedanshujain
vedanshujain commented Jun 1, 2020
View reviewed changes
includes/class-wc-ajax.php
}

$response = array();
if ( ! isset( $_POST['order_id'] ) ) {
Copy link
Contributor Author

@vedanshujain vedanshujain Jun 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to reviewer: I have refactored this method in two to add maybe_add_order_item so that I can unit test this better. Only real changes are deletion of these lines: https://github.com/woocommerce/woocommerce/pull/26642/files#diff-133db5662c51f5686d87611121a05a3bL918-L923. Other changes are rearranging of some lines to make unit testing easier.

@vedanshujain vedanshujain added this to the 4.3.0 milestone Jun 5, 2020
rrennick
rrennick approved these changes Jun 9, 2020
View reviewed changes
Copy link
Contributor

@rrennick rrennick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vedanshujain Thanks for getting this one. Looks good. Approving with a note that there are a few PHPCS items reported in class-ajax.php that we should fix:

FILE: includes/class-wc-ajax.php
-------------------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 4 WARNINGS AFFECTING 4 LINES
-------------------------------------------------------------------------------------------------------------------------------------------------------------------
  548 | WARNING | [ ] Using the WPCS native whitelist comments is deprecated. Please use the PHPCS native "phpcs:ignore Standard.Category.SniffName.ErrorCode"
      |         |     annotations instead. Found: // WPCS: sanitization ok.
      |         |      (WordPress.Security.ValidatedSanitizedInput.DeprecatedWhitelistCommentFound)
 1585 | WARNING | [x] Equals sign not aligned correctly; expected 1 space but found 4 spaces (Generic.Formatting.MultipleStatementAlignment.IncorrectWarning)
 1864 | WARNING | [x] Equals sign not aligned with surrounding assignments; expected 6 spaces but found 7 spaces
      |         |     (Generic.Formatting.MultipleStatementAlignment.NotSameWarning)
 1865 | WARNING | [x] Equals sign not aligned with surrounding assignments; expected 1 space but found 2 spaces
      |         |     (Generic.Formatting.MultipleStatementAlignment.NotSameWarning)

@peterfabian peterfabian added the release: highlight Issues that have a high user impact and need to be discussed/paid attention to. label Jun 11, 2020
@peterfabian
Copy link
Contributor

peterfabian commented Jun 11, 2020

Flagging with high impact as this might affect the workflow for people creating orders for their customers manually.

@peterfabian
Copy link
Contributor

peterfabian commented Jun 11, 2020 •
edited
Loading

When reading more about the background in #21754, it seems to me that this solution would break workflow for those people, no?

(emphasis mine)

When following the above process, and saving the order (keeping order in Payment Pending Status), stock is reduced (per order notes, outside of the screenshot).

That feature was removed in WooCommerce 3.5 and order stock is automatically updated only after the order is set to processing (after payment received).

While auto-updating is good and saves a step, this takes the ability to change stock on the order level out of the hands of store owners.

In this case, a store owner needs to make sure products do not go out of stock while waiting for payment (which, happens after items are received in this case), but could happen often in Wholesale arrangements where more time is needed than is covered in hold stock options.

The previous change was added explicitly to handle this differently, so I'm a bit unsure if this should go in as is. Thus moving back to needs feedback. Happy to hear thoughts from @woocommerce/proton

@peterfabian peterfabian added needs: author feedback The issue/PR needs a response from any of the parties involved in the issue. and removed status: approved labels Jun 11, 2020
@peterfabian peterfabian removed this from the 4.3.0 milestone Jun 15, 2020
@peterfabian peterfabian mentioned this pull request Jun 18, 2020
Closed
@vedanshujain
Copy link
Contributor Author

vedanshujain commented Jun 30, 2020

@peterfabian This PR still preserves the changes introduced in #22329 (as a response to #21754), with the caveat that, to reduce stock:

  1. The order needs to be saved
  2. The order needs to be in one of the statuses that reduce the stock, like on-hold, processing, completed etc.

We can change this to remove caveat 2, and make sure that stock is reduced as soon as you save the order in any status. However, I would recommend against it so that order status and stock reduction logic is consistent and easier to understand for all our users.

For example, in the current master, stocks are reduced if you create an order with payment-pending status. However, if you move the status to completed, and then move it back to payment-pending, the stock reduction will be undone.

In any case, we have to wait till order is saved to reduce the stock, otherwise, there is no way to recover this stock change.

@vedanshujain vedanshujain added status: needs review and removed needs: author feedback The issue/PR needs a response from any of the parties involved in the issue. labels Jul 1, 2020
@vedanshujain
Limit stock changes for order items to status methods for consistency.
1b3f1d7 
Methods `wc_maybe_increase_stock_levels` and `wc_maybe_reduce_stock_levels` already reduce/increase stock levels when statuses are changed, so no need to do this here.
@vedanshujain
Add tests
836d764
@vedanshujain
Disabled flaky test
6a8f9a4
@vedanshujain
Applied coding standards
8ded6a3
@juliaamosova juliaamosova mentioned this pull request Jul 7, 2020
Closed
@rrennick
Copy link
Contributor

rrennick commented Jul 7, 2020

@peterfabian Do you have any followup on this one?

@vedanshujain vedanshujain requested a review from peterfabian July 16, 2020 09:05
@peterfabian peterfabian added this to the 4.4.0 milestone Jul 24, 2020
peterfabian
peterfabian approved these changes Jul 24, 2020
View reviewed changes
Copy link
Contributor

@peterfabian peterfabian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great fix, I agree that it should work fine, we just need to highlight it in the release post, as it changes the workflow for some people.

Also, as per comments, the testing instructions should also include on-hold status that can be used when waiting for payment from the end customer.

@peterfabian peterfabian merged commit 03230ec into master Jul 24, 2020
@peterfabian peterfabian deleted the fix/26607 branch July 24, 2020 11:23
@woocommercebot 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 Jul 24, 2020
@Konamiman Konamiman removed the release: add changelog Mark all PRs that have not had their changelog entries added. [auto] label Jul 29, 2020
@vedanshujain vedanshujain mentioned this pull request Jul 30, 2020
Closed
3 tasks
@MarkHaynes MarkHaynes mentioned this pull request Aug 21, 2020
Closed
@negapo negapo mentioned this pull request Sep 23, 2020
Closed
5 tasks
vedanshujain added a commit that referenced this pull request Sep 23, 2020
@vedanshujain
Remove protection as we already diff and update only if needed.
c69e1f5 
In #26642 we removed adding reduced_stock meta when adding new order item to prevent ghost entries, but in inadvertently exposed an underlying bug where _reduced_stock meta was getting set to 0 if its emtpy.

We were then checking the presence of this meta, but also not reducing the stock in case it was not set.
@vedanshujain vedanshujain mentioned this pull request Sep 23, 2020
Merged
6 tasks
@vedanshujain vedanshujain mentioned this pull request Dec 21, 2020
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@peterfabian peterfabian peterfabian approved these changes

+1 more reviewer

@rrennick rrennick rrennick approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

release: add testing instructions PRs that have not had testing instructions added to the wiki. [auto] release: highlight Issues that have a high user impact and need to be discussed/paid attention to.

Projects

None yet

Milestone

4.4.0

Development

Successfully merging this pull request may close these issues.

Adding an item to a newly created order in the Admin panel without saving the order results in stock being reduced that is unrecoverable.

6 participants

@vedanshujain @peterfabian @rrennick @Konamiman @woocommercebot