Codecov Report

Merging #37842 (75f3d31) into trunk (34dd147) will decrease coverage by 0.0%.
The diff coverage is 56.2%.

❗ Current head 75f3d31 differs from pull request most recent head 9f6190c. Consider uploading reports for the commit 9f6190c to get more accurate results

@@            Coverage Diff             @@
##             trunk   #37842     +/-   ##
==========================================
- Coverage     51.6%    51.5%   -0.0%     
+ Complexity   17444    17280    -164     
==========================================
  Files          440      430     -10     
  Lines        80297    80039    -258     
==========================================
- Hits         41401    41230    -171     
+ Misses       38896    38809     -87     

Thanks for working on this, @Abdalsalaam.

However, on further review, we're not sure it makes sense to decrease this value when a sale is refunded, as it is used to measure the gross (not net) number of units sold. On the other hand, it does seem valid to reduce it on the following occasions:

• If an order is deleted or cancelled (because in these cases, the sale is effectively annulled).
• If an order is placed then subsequently one or more line item quantities are amended (as in most cases, it seems probable this would be the result of a mistake being made when the order was initially placed).

Thinking of some of the scenarios in the original issue (these are places where total_sales may be used):

• Shortcode for displaying product with best_selling="1"
• Sort by popularity on the product pages
• Product widget when order by sales is selected

We fully appreciate different merchants may hold different opinions on this, but it does not seem a given that a product should not be regarded as best-selling, or unpopular, simply because it suffers a high number of refunds.

I hope that makes sense, but let me know if you have any questions or concerns about this (that is, our stance on refunds, and how they should or should not impact total_sales specifically).

Hi @Abdalsalaam!

I was doing a further pass and realized we didn't address anything relating to order deletion. For consistency, it would seem to me like the following should generally be true:

• Trashing an order should act like a cancellation, and total_sales counts should be increased accordingly.
• Un-trashing an order should have the opposite effect (just like when a cancelled order is set back to a completed status).

However, there are exceptions:

• If it was already cancelled before being trashed, we probably would not want to make a further adjustment to any of the total_sales counts.
• Similarly, if it is subsequently untrashed we wouldn't want to adjust total_sales if it is going back to a cancelled status.

With regards to permanent deletion, the following seems like a sensible policy:

• If the order was already in the trash when it is permanently deleted, nothing more needs to happen (the total_sales adjustment will already have happened if it was needed).
• However, if an order is permanently deleted without passing through the trash (ie, force deletion), then we would potentially need to make an adjustment to total_sales at this point. Of course, as in the earlier cases, it again would depend on the order's previous status.

What do you think?

Hi @Abdalsalaam!

I was doing a further pass and realized we didn't address anything relating to order deletion. For consistency, it would seem to me like the following should generally be true:

* Trashing an order should act like a cancellation, and `total_sales` counts should be increased accordingly.

* Un-trashing an order should have the opposite effect (just like when a cancelled order is set back to a completed status).

However, there are exceptions:

* If it was already cancelled before being trashed, we probably would not want to make a further adjustment to any of the `total_sales` counts.

* Similarly, if it is subsequently _untrashed_ we wouldn't want to adjust `total_sales` if it is going back to a cancelled status.

With regards to permanent deletion, the following seems like a sensible policy:

* If the order was already in the trash when it is permanently deleted, nothing more needs to happen (the `total_sales` adjustment will already have happened if it was needed).

* However, if an order is permanently deleted _without_ passing through the trash (ie, force deletion), then we would potentially need to make an adjustment to `total_sales` at this point. Of course, as in the earlier cases, it again would depend on the order's previous status.

What do you think?

@barryhughes How can we add tests for (untrash) scenario? wp_untrash_post function will work only with CPT

wp_untrash_post function will work only with CPT

True!

However, we can work at a higher level of abstraction rather than directly with the WP Posts API. The WC_Object_Data_Store_Interface includes a delete() method, and this is implemented by both the CPT and HPOS data-stores (and it supports trashing as well as permanent deletion).

Does that help at all?

@barryhughes yes that helps, i'll check what i can do! thank you

Just a friendly note that code freeze for the 7.8.0 release happens on Monday next week (2023-05-22).

This means that, realistically, if we want to include this fix in the 7.8.0 release then the PR will need to be updated and ready for a fresh review early this week (of course, it can also wait until some future point—I just wanted to give you an idea of the timelines we are working to re 7.8.0).

@barryhughes thanks for letting me know, I need a help on writing the test using WC_Object_Data_Store_Interface, is there an example or documented code? I couldn't find a way copatile with HPOS & CPT to "untrash" the order.

Hi @Abdalsalaam,

There's a good example over here that does this by calling the order's delete method:

$order    = OrderHelper::create_order();
$order_id = $order->get_id();
$order->delete( true );

Some levels below that, the data-store's delete method is invoked:

$this->data_store->delete( $this, array( 'force_delete' => $force_delete ) );

In the test I linked to, we explicitly enable the HPOS data-store however, for this PR, i don't think that step is strictly necessary, as we already have a CI process that runs all unit tests against HPOS ("Run tests against PR in an environment with COT enabled").

Apologies, I misread your question. For untrashing, this test may be a better example. You should be able to get the underlying data-store from an order object as follows:

$order->get_data_store();

@barryhughes I added a test for CPT

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

@barryhughes thanks a lot for your review! that was very helpful!

With the most recent rebase (done to incorporate #39136), all checks are passing except for building the live branch (expected in this context).

Testing instructions updated to cover some of the key evolutions in how this works.

@barryhughes thank you for your feedback and help on it

For the record, the code in this pull request incorrectly uses the before_delete_post hook, which doesn't work in the context of HPOS; it should use woocommerce_before_delete_order instead.

However by the time this pull request was merged Abstract_WC_Order_Data_Store_CPT::delete wasn't firing the woocommerce_trash/delete_order and woocommerce_before_trash/delete_order hooks, so strictly speaking it should have used both of before_delete_post and woocommerce_before_delete_order, depending on whether HPOS was active or not.

All of this is fixed as part of #37050: it introduces the firing of woocommerce_trash/delete_order and woocommerce_before_trash/delete_order hooks as part of Abstract_WC_Order_Data_Store_CPT::delete (see 77af853), and it hooks to woocommerce_before_delete_order instead of before_delete_post (see 387c91c).