Automattic / woocommerce-subscriptions-core

Subscriptions core package for WooCommerce
Other
85 stars 31 forks source link

Update cache when subscription is trashed, deleted, or untrashed #383

Closed shendy-a8c closed 1 year ago

shendy-a8c commented 1 year ago

Fixes #310

Description

When HPOS on, remove a subscription from cache when it's trashed or deleted, and add it to cache if it's restored / untrashed.

Changes in this PR:

I notice that untrashing does not update cache correctly when HPOS is off or when HPOS is on but authoritative table is set to posts table. I created a separate issue for that bug: https://github.com/Automattic/woocommerce-subscriptions-core/issues/385.

How to test this PR

  1. Enable HPOS.
  2. Purchase a subscription product.
  3. Trash that subscription's parent order. Make sure that subscription does not show up on My Account > Subscriptions.
  4. Untrash the parent order. Make sure the subscription re-appears on My Account > Subscriptions.
  5. Trash and delete the subscription's parent order. Make sure that subscription does not show up on My Account > Subscriptions.

Product impact

james-allan commented 1 year ago

Hey @shendy-a8c when I tested this by trying to trash subscriptions from the admin list table I noticed that the woocommerce_before_trash_subscription hook isn't running.

This means that trashing and deleting subscriptions doesn't led to the subscription being removed from the users' subscription cache. :(

The issue is caused by WC core and how they handle the bulk actions for trashing and deleting subscriptions. They have hard coded the datastore that gets used to handle those requests [code ref] and so the order data store is used to trash and delete subscriptions.

I'm going to file a separate issue for that.

shendy-a8c commented 1 year ago

when I tested this by trying to trash subscriptions from the admin list table I noticed that the woocommerce_before_trash_subscription hook isn't running.

Do you mean woocommerce_trash_subscription hook that isn't firing? I see woocommerce_before_trash_subscription is firing just fine.

The issue is caused by WC core and how they handle the bulk actions for trashing and deleting subscriptions. They have hard coded the datastore that gets used to handle those requests [code ref] and so the order data store is used to trash and delete subscriptions.

I'm going to file a separate issue for that.

Not rushing but I wonder if you have filed that issue.

The reason I ask is because I see https://github.com/Automattic/woocommerce-subscriptions-core/pull/389 was created in response to that bug but I think the ideal fix should be in WC core, right? https://github.com/Automattic/woocommerce-subscriptions-core/pull/389 would only solve the problem when subscription is trashed / deleted / untrashed from the subscription admin list page but I see trashing subscription's parent order also does not trigger woocommerce_trash_subscription hook and https://github.com/Automattic/woocommerce-subscriptions-core/pull/389 won't solve that problem.

shendy-a8c commented 1 year ago

Do you mean woocommerce_trash_subscription hook that isn't firing?

Actually, after more testing, I find that woocommerce_trash_subscription hook is called as expected after e1ce020 a typo from this commit.

I don't see the problem from trashing, untrashing, or deleting subscription's parent order from bulk menu.

Do I miss something?