Automattic / woocommerce-subscriptions-core

Subscriptions core package for WooCommerce
Other
80 stars 29 forks source link

Properly migrate subscription dates from postmeta to wc_orders_meta to fix syncing issues and prevent infinite `order.updated` webhooks being sent #548

Closed mattallan closed 6 months ago

mattallan commented 7 months ago

Fixes #547

Description

This PR is fixing an issue where trying to sync/migrate our _scheduled_{date_type} metadata from the wp_postmeta table over to HPOS wc_orders_meta table does not work and results in a continuously attempting to sync subscription data each time it's read into memory.

This issue severely impacts stores that are using the order.updated webhook because when building the webhook payload, a subscription object is read into memory, attempts to sync data and fails, which then results in a new webhook payload being scheduled. Because the subscription remains out of the sync, the next webhook payload will repeat the same process resulting in an infinite loop of the order.updated webhook being sent.


When WooCommerce attempts to sync/migrate postmeta over to the orders_meta table, it calls migrate_post_record() which uses two methods to sync the data:

  1. migrate_meta_data_from_post_order(), and
  2. set_order_prop()

The first method calls OrdersTableDataStore::get_diff_meta_data_between_orders( $order, $post_order ) which returns an array of meta keys/value that are different between the two objects. This function strictly ignores any meta that is listed in $internal_meta_keys which means the following subscription meta won't be synced by this function:

The second method set_order_prop() attempts to call $subscription->set_{meta_key}() and then tries WCS_Orders_Table_Subscription_Data_Store->set_{meta_key}() to sync meta.

Out of the above internal meta keys, the only meta keys that don't have setters are:

Given this, the solution I've opted to go with in this PR is to add these setters.

How to test this PR

Get into the infinite loop

  1. Check out trunk of this repo.
  2. Install the latest Woo and enable HPOS with syncing enabled.
  3. Open https://public.requestbin.com/r and spin up a fresh endpoint to send webhooks to
  4. Go to WC > Settings > Advanced > Webhooks and create an order.updated webhook
  5. Purchase a subscription product
  6. Inside the wc_orders_meta table, search for the subscription ID and delete the value stored next to _schedule_next_payment (can be any other date value)
  7. Trigger the order.updated event for the parent order for this subscription by running something like:
$parent_order = wc_get_order( 123 );
$parent_order->save();
  1. Visit Tools > Action Scheduler and manually run the woocommerce_deliver_webhook_async action for the parent order.
  2. Notice that after running the action, another woocommerce_deliver_webhook_async for the same parent order is scheduled.
  3. Check the wc_orders_meta table and confirm the _schedule_next_payment is still empty.
  4. Keep running the webhook action and notice it's creating a new webhook to be sent off again.

Confirm the fix

  1. Checkout this branch
  2. Re-run woocommerce_deliver_webhook_async again
  3. Notice a new async webhook event is not longer scheduled
  4. Check the wc_orders_meta table and confirm the _schedule_next_payment has been synced/migrated from the postmeta table

Product impact

prettyboymp commented 6 months ago

I was able to test these changes on the site with issues today and all of the subscriptions that previously reported differences between the post and order data were able to properly sync after applying the changeset. Thank you!

james-allan commented 6 months ago

I was able to test these changes on the site with issues today and all of the subscriptions that previously reported differences between the post and order data were able to properly sync after applying the changeset. Thank you!

That's fantastic news, thanks for letting us know @prettyboymp. 😃

mattallan commented 6 months ago

Thanks @james-allan for the additions to the docblock and @prettyboymp for confirming the fix on your end!! Merging this!