Closed mattallan closed 1 year ago
I have finished testing (see PR description for all of the tests I've done) and have pushed up the changelogs for this one.
There was some issues I discovered with untrashing subscriptions when HPOS was enabled.
To fix this I had to override the untrash_order()
order data store function.
This one is ready for review now :)
Thanks for the review and suggestions @shendy-a8c. Let me know if this is ready for merge or if there's more testing/feedback you need to provide.
@mattallan Code diff looks good but I was still testing.
I tested in several store configurations, following your testing steps. Here are my notes and questions:
After trashing parent order and then restoring (untrashing) it, I'm a confused about the subscription's status. Is it pending, cancelled, or draft?
Looking at the posts table, post_status is draft
.
Looking at the Status column, it says pending.
Per order notes, subscription was cancelled when parent order was trashed. Subscription's meta _wp_trash_meta_status
was set to wc-cancelled
expectedly, so when parent order was restored, I thought subscription would be back to cancelled or at least if it is expected to be in draft / pending, it will be reflected in the order notes but it's not.
Is the current behavior I observe expected?
After trashing a parent order, there is a way to undo.
Clicking undo, will be restored but the status is cancelled, not draft like if parent order is restored. Also, the status change is reflected in the order notes.
Is that expected?
Same questions as above, ie when parent order is restored, subscription becomes draft, and subscription becomes cancelled after trashing parent order and then clicking undo. Everything else looks good.
I see no undo after trashing parent order. No problem. Just noting the different UX. Subscription's status is cancelled after parent order is restored. This behavior is different from when HPOS is off.
The status change from trash to cancelled is shown in order notes, which is nice.
Confirmed that subscription's meta _wp_trash_meta_status
is set both in table wp_postmeta
and wp_wc_orders_meta
.
When parent order is deleted, even though subscription is deleted as well, woocommerce_subscription_deleted
action is not triggered. That is not expected, right?
When I stepped through the code, it stopped here, so woocommerce_delete_order
which is a couple lines below isn't triggered.
From the comment:
// If this datastore method is called while the posts table is authoritative, refrain from deleting post data.
Since this store is using orders table, I don't expect posts table to be 'authoritative' here.
Debugging this line, I see the value of $order->get_data_store()->get_current_class_name()
is WCS_Orders_Table_Subscription_Data_Store
and self::class
is Automattic\WooCommerce\Internal\DataStores\Orders\OrdersTableDataStore
.
Maybe when that if clause was written, it was only posts vs orders data store and not expecting other order type data store.
Not sure what's the workaround here. Maybe override delete()
?
The subscription is deleted though. I can't find it in wp_posts
or wp_wc_orders
.
Same as above. Every thing looks good but woocommerce_subscription_deleted
is not triggered.
Also, I couldn't find subscription's meta _wp_trash_meta_status
in wp_postmeta
table, which is expected because sync is off.
Works well. When parent order is trashed, restored, or deleted, subscription is also the same. Both woocommerce_subscription_trashed
and woocommerce_subscription_deleted
are triggered correctly.
I see similar behavior with non-HPOS store though, ie subscription becomes draft when parent order is restored but subscription becomes cancelled if I click 'undo'.
Also, when parent is trashed, subscription's meta _wp_trash_meta_status
is not set in wp_wc_orders_meta
table. Is that expected?
It does not break any behavior because posts tables are used here and _wp_trash_meta_status
meta is set in wp_postmeta
table but I would expect data is sync'd between posts and orders table when sync is on.
I see the same behaviors with when sync is on. No difference at all.
When parent is trashed, subscription's meta _wp_trash_meta_status
is not set in wp_wc_orders_meta
table, which is OK when sync is off.
Sorry for long comment!
Woow!! Thanks @shendy-a8c for the thorough testing on this PR! There's a lot to go through here.
After trashing parent order and then restoring (untrashing) it, I'm a confused about the subscription's status. Is it pending, cancelled, or draft?
Yeah there's some underlying bug with the status of a subscription after untrashing it. The behaviour on trunk with HPOS-off (without any changes in this PR) is that the subscription goes to draft after untrashing it. Since this is pre-existing behaviour I don't think we should worry too much about similar behaviour still existing with these changes. If there's slight inconsistencies we can look at solving them in a separate PR.
Clicking undo, will be restored but the status is cancelled, not draft like if parent order is restored. Also, the status change is reflected in the order notes.
Is that expected?
This is interesting! I haven't actually clicked that undo button before but it looks like it's working as we expect the "untrash" to work. Good to know!
Subscription's status is cancelled after parent order is restored. This behavior is different from when HPOS is off.
Another interesting find! If I'm understanding this correctly, it looks like untrashing the parent order correctly sets the subscription status to cancelled when HPOS is enabled (working)? But when HPOS is off, the subscription goes to draft/pending (not working).
Since this issue exists on the latest released version of subscriptions and isn't introduced by this PR, I think we work on a fix separately after this HPOS project has been completed.
When parent order is deleted, even though subscription is deleted as well, woocommerce_subscription_deleted action is not triggered. That is not expected, right?
Nice catch! We will need to fix this one up.
I like your suggestion with overriding the delete()
method to workaround this self::class()
check not working properly for classes that extend the OrdersTableDataStore
class.
It's kind of annoying but it's also a good thing because now we can use _subscription
named hooks for deleting :)
I have just pushed up those changes to fix the issue where woocommerce_subscription_deleted
action was not triggered on stores using HPOS + Order Tables, see 5c99e50.
Now that we have subscription-specific hooks for trashing and deleting, I have updated the subscriptions manager to use these new hooks. See 17f62dc
I have gone through the codebase and made sure there are no instances of us using order trashed/deleted hooks and checking if it's a subscription. In these cases, we should use the new subscription hooks.
My latest changes only affect HPOS + Order Tables so I've done some more testing:
woocommerce_subscription_deleted
is fired@shendy-a8c am I correct in thinking this was the only main issue with this PR being merged? Let me know if I missed something else :)
Making a note of some things I missed:
HPOS on with WC 7.4-dev, use posts table, sync on. when parent is trashed, subscription's meta _wp_trash_meta_status is not set in wp_wc_orders_meta table. Is that expected?
I'm not sure about this one. WC handles the data syncing so I'm wondering if they've specifically not synced this meta 🤔 This meta probably isn't important to sync so I could see it being expected behaviour.
Fixes #221 Fixes #195
Description
This PR updates all of the WP Post-related hooks we were using to hook onto the trashing, deleting and untrashing of orders and subscriptions in the
WC_Subscriptions_Manager
class to now use the CRUD/HPOS equivalent hooks so that this functionality continues to work on stores that are using custom order tables.This PR consists of the following changes:
woocommerce_loaded
add_action()
's calls for when HPOS is enabled to hook onto the new WC order hooksmaybe_trash_subscription()
&maybe_cancel_subscription()
functions to use CRUD methods1. Moved actions into a new function on
woocommerce_loaded
In order to load the new hooks we need to run
wcs_is_custom_order_tables_usage_enabled()
but this function only works after WC has been loaded.While testing this PR I was getting errors due to some WC classes/instances not existing yet when trying to check if HPOS is enabled.
To fix this, I moved these hooks into their own function which is attached to the WC loaded do action.
2. New Add Actions
Here are all of the
_post
hooks changing in this PR and their new HPOS equivalent hooks:wp_trash_post
is nowwoocommerce_before_trash_order
untrashed_post
is nowwoocommerce_untrash_order
before_delete_post
is nowwoocommerce_before_delete_order
trashed_post
is nowwoocommerce_trash_order
deleted_post is now
woocommerce_delete_order`To avoid breaking backwards compatibility, I made it so these new action hooks are only used on stores with HPOS enabled.
3. Updating functions to use CRUD methods
Most of the changes in these functions are pretty self-explanatory but there are some I wanted to make note of.
order_data_store::untrash_order()
method only exists in theOrderTablesDataStore
class so I made it so we only use this function if it exists, otherwise, fall back to using thewp_untrash_post()
function.When fixing
fix_trash_meta_status()
to work in CRUD, I couldn't useupdate_meta_data()
as it was creating duplicate rows in the database, so I had to use theread_meta()
andupdate_meta()
approach which writes this data directly to the DB and works in both HPOS and none-HPOS environments.How to test this PR
There are 4 main flows that need to be tested in this PR:
_wp_trash_meta_status
towc-cancelled
if the subscription previously had an active status.My own testing on this PR:
HPOS off:
_wp_trash_meta_status
is set on subscriptionwoocommerce_subscription_trashed
action hook is triggered after trashwoocommerce_subscription_deleted
action hook is triggered after deleteHPOS on, syncing off:
_wp_trash_meta_status
is set on subscriptionwoocommerce_subscription_trashed
action hook is triggered after trashwoocommerce_subscription_deleted
action hook is triggered after deleteHPOS on, syncing on:
_wp_trash_meta_status
is set on subscriptionwoocommerce_subscription_trashed
action hook is triggered after trashwoocommerce_subscription_deleted
action hook is triggered after deleteWC 6.8 (HPOS off):
_wp_trash_meta_status
is set on subscriptionwoocommerce_subscription_trashed
action hook is triggered after trashwoocommerce_subscription_deleted
action hook is triggered after deleteProduct impact