Add filter hook to allow currently hardcoded 10 minute checkout draft stock reservation to be filtered

[ninetyninew]

Submission Review Guidelines:

• I have followed the WooCommerce Contributing Guidelines and the WordPress Coding Standards.
• I have checked to ensure there aren't other open Pull Requests for the same update/change.
• I have reviewed my code for security best practices.
• Following the above guidelines will result in quick merges and clear and detailed feedback when appropriate.

Changes proposed in this Pull Request:

Closes #45245

Note: We originally committed a filter hook for the hardcoded 10 minute reserve stock on block checkout entry, however due to reviews since then we have opted for a filter in Automattic\WooCommerce\Checkout\Helpers in the ReserveStock::reserve_stock_for_order().

The proposed newer filter was then reviewed and recommended we amend to a minutes based filter, and hence this is now a woocommerce_order_hold_stock_minutes filter hook.

Testing instructions below updated.

Snippet for testing:

function test_woocommerce_order_hold_stock_minutes( $minutes, $order ) {

	return 0;

}
add_filter( 'woocommerce_order_hold_stock_minutes', 'test_woocommerce_order_hold_stock_minutes', 10, 2 );

Test based on WooCommerce hold stock minutes setting:

1. First determine that stock is being reserved without the snippet
2. Ensure WooCommerce > Settings > Inventory > Manage stock enabled and set to hold stock for 60 minutes
3. Create a simple product with manage stock on and stock quantity 10
4. Add the product to block cart with quantity 1
5. Place order
6. View order
7. In order notes and/or by checking wp_wc_reserve_stock database table see that stock reserved for 60 minutes (it may also show an earlier 10 minutes note due to the checkout entry draft hardcoded 10 minute stock reserve)
8. Add the snippet
9. Repeat the above steps, this time there shouldn't be any stock reservation (you can also test with a minute level by returning one in the snippet instead of 0)

Test based on block checkout entry hold stock minutes (this uses a hardcoded 10 minutes not the hold stock setting):

1. First determine that stock is being reserved for checkout drafts without the snippet
2. Ensure WooCommerce > Settings > Inventory > Manage stock enabled and set to hold stock for 60 minutes (this gets ignored for this test due to the hardcoded 10 minutes)
3. Create a simple product with manage stock on and stock quantity 10
4. Add the product to block cart with quantity 1
5. Go to the checkout page but don't place the order (generates a draft order)
6. View the draft order
7. In order notes and/or by checking wp_wc_reserve_stock database table see that stock reserved for 10 minutes
8. Add the snippet
9. Ensure you empty cart and trash then delete the draft order just to ensure you are back to a clean slate, repeat the above steps, this time there shouldn't be any stock reservation (you can also test with a minute level by returning one in the snippet instead of 0)

Changelog entry

• Automatically create a changelog entry from the details below.

Significance

• Patch
• Minor
• Major

Type

• Fix - Fixes an existing bug
• Add - Adds functionality
• Update - Update existing functionality
• Dev - Development related task
• Tweak - A minor adjustment to the codebase
• Performance - Address performance issues
• Enhancement - Improvement to existing functionality

Message

Add woocommerce_order_hold_stock_minutes filter hook to allow the number of minutes stock in an order should be reserved for to be filtered.

Comment

[github-actions]

Hi ,

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

[opr]

Hi @ninetyninew thanks for contributing to WooCommerce. I have some quick feedback on this.

We should ensure the new filter is documented (similar to how it is done here) and maybe a better name would be woocommerce_draft_order_hold_stock_minutes.

You could also move it to a separate variable to make it clearer what the documentation refers to.

Finally, the changelog entry on the PR body is incorrectly formatted, but I'll update it for you so it auto-generates one. I'd also change it to an Add rather than a tweak since we're adding new functionality here.

I'd like to hear @mikejolley's input on why 10 minutes was chosen, and why it's not editable. It was added in this PR: woocommerce/woocommerce-blocks#5546

@ninetyninew could you also share your use-case on this PR as to why you want to filter this?

[ninetyninew]

@opr I've hopefully made those changes and committed them as required.

My use case for this is for when you have a layer of custom stock management on top of WooCommerce's standard stock functionality, it is useful to be able to stop the reservation by suppling zero minutes, previously you could turn off the stock reservation with woocommerce_hold_stock_minutes, but now with Cart/Checkout blocks this isn't possible due to the forced 10 minutes, I have read @mikejolley's ticket on why it is 10 minutes, but I think you should be allowed to supply your own value for this (including 0):

"we put a hold on stock for all items in the cart to prevent things going out of stock while the customer enters their details"

If you've set the proposed new filter to 0 and it does go out of stock in that scenario, what would happen?

[mikejolley]

I'd like to hear @mikejolley's input on why 10 minutes was chosen, and why it's not editable. It was added in this PR: woocommerce/woocommerce-blocks#5546

@opr just seemed like a sensible number to allow customer to idle on checkout before invalidating everything. The time gets updated whenever there is an API push during field entry.

If this was somehow set too low, the checkout would error when placing an order if something went out of stock. There would be race conditions.

[opr]

Thanks for the info Mike, makes sense.

If this was somehow set too low, the checkout would error when placing an order if something went out of stock. There would be race conditions.

If we communicate this in the hook docks I think it's OK then.

[ninetyninew]

@opr Let me know if anything further needs amending here, I saw yesterday the workflows flagged a lint error which I believe I have rectified in the latest commit.

[opr]

Maybe we should check that the result is: not empty, is an integer (or can be cast to one) and is not negative, if any of these are true it should default back to 10. Do you agree?

[ninetyninew]

@opr I think casting to a integer and a check for negative is fine, however the !empty would stop being able to filter to 0, which for our particular scenario (to stop stock reservation) is why we need this filter.

[opr]

Yep, you're right 👍🏼

[ninetyninew]

@opr The int casting is already there, i've just amended to include the negative check in the latest commit if you'd like to check over it.

[opr]

@ninetyninew thanks for your patience so far, after more testing I have found that if the filter returns 0, then the value is defaulted to 60 in reserve_stock_for_order due to this check: https://github.com/woocommerce/woocommerce/blame/8eb8a09c8881bf12a471fd5d650d539308914ae2/plugins/woocommerce/src/Checkout/Helpers/ReserveStock.php#L69

therefore, to disable stock reservation on draft orders, you need to use two filters: woocommerce_hold_stock_minutes and woocommerce_draft_order_hold_stock_minutes.

Looping in @woocommerce/proton to get your thoughts on this. I personally don't think this is the greatest DX, especially if you only want to set draft order stock reservation time to 0 leaving regular order's reservation time alone.

Instead of this, do you think a better approach would be adding a filter to reserve_stock_for_order that allows extensions to decide if the stock should be reserved or not. It could be added as a condition here. The filter would receive the $order so it could determine the status in the callback.

[ninetyninew]

@opr Makes sense, and less complicated. Happy to do that, would I do that here or does it require creation of a new PR? Also what would the suggested filter name be? Maybe woocommerce_reserve_stock_for_order?

[opr]

Thanks for working on this @ninetyninew - it's working fine for me now and the stock is not reserved. Please can you close and reopen the PR to get CI running?

[Konamiman]

I have a suggestion for an improvement that would make the filter more flexible: what if instead of accepting an returning a boolean, it accepts the value of $minutes (from either the function argument or the woocommerce_hold_stock_minutes option) and returns a new minutes value? If it returns 0 it means that no stock reservation should be made. This implies that the name of the hook would have to be changed, for example to woocommerce_order_hold_stock_minutes.

Thus the begninning of the function body, not including the filter docblock, would look like this (I haven't tested it):

$minutes = $minutes ? $minutes : (int) get_option( 'woocommerce_hold_stock_minutes', 60 );
$minutes = apply_filters( 'woocommerce_order_hold_stock_minutes', $minutes, $order );
if ( ! $minutes || ! $this->is_enabled() ) {
	return;
}

[ninetyninew]

@Konamiman @opr I find this a little harder to understand than our proposed filter, due to the already existing woocommerce_hold_stock_minutes setting, could that new filter lead to confusion with what woocommerce_hold_stock_minutes is doing maybe? Whereas the woocommerce_reserve_stock_for_order seems quite self explanatory: there is a hold stock minutes setting, but you can exclude an order from it.

Not quite sure on this, but I think that it may also cause a potential issue, as the checkout draft $minutes are hardcoded to 10 before it gets here, the filter you are proposing would allow that 10 minute period to be changed to something lower and result in the race conditions @mikejolley mentioned earlier.

[opr]

@ninetyninew thanks for sharing your thoughts! I think @Konamiman's suggestion is great and makes a lot of sense, but I also agree that naming is tricky. I think we should go with the new suggestion but with a clearer/easier name.

the filter you are proposing would allow that 10 minute period to be changed to something lower and result in the race conditions @mikejolley mentioned earlier.

If the stock-hold is set to 0, it's effectively the same as returning false in your current filter. Values between 1-10 would not result in race conditions.

[ninetyninew]

Thanks for the feedback, I have now made the changes as recommended, committed them and updated the testing instructions. Can you please re-review @opr @Konamiman

Note: As a $minutes based hook, I can't think of better name than this, when writing the previous reply I was mistakenly forgetting hat woocommerce_hold_stock_minutes isn't a filter in itself and is a setting, so with this new proposed filter being the only filter hook related to what it's doing (and woocommerce_hold_stock_minutes being a setting), I think the name works.

[Konamiman]

@ninetyninew It looks good now, thanks. Just one additional small nitpick: it would be good to explicitly state in the docblock of the filter that a value of zero minutes will disable the stock reservation for the order entirely (that was the original goal of the change after all).

[ninetyninew]

@Konamiman No problem, I have now made and committed the doc block change.

[ninetyninew]

@Konamiman Thanks! I noticed 1 thing failed here, do I need to do anything? I haven't raised a PR for a long time so unfamiliar with the latest processes.

[Konamiman]

@ninetyninew Don't worry, it's Github trying to create a build from the PR branch but that sometimes fails and it's not related to the code changes introduced.