search

Automattic / woocommerce-subscriptions-core

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

Backfill subscription dates and cache metadata to the postmeta table when syncing #538

Closed mattallan closed 7 months ago

mattallan commented 7 months ago

Fixes https://github.com/woocommerce/woocommerce-subscriptions/issues/4572

Description

With custom order tables (HPOS) and compatibility mode (data syncing) enabled, some subscription metadata is not correctly backfilled to the postmeta table, namely all of our _schedule_{date_type} meta and our _subscription_{order_type}_order_ids_cache meta. Here's a full list:

Here's a screenshot from trunk of the subscriptions meta in the WC orders meta and WP posts meta tables after a subscription is synced. Notice how the dates are set to 0 and notice how the cache meta is completely missing:

wp_wc_orders_meta wp_postmeta
image image

Why is this meta not backfilled?

During the backfill process, the OrdersTableDataStore::backfill_post_record() function calls $post_order->set_props( $order->get_data() ); which gets all of the data stored on the order object and tries to call the setter methods for each prop. Because we don't have setters for our dates and cache metadata this data is not set.

There's a bit more to it related to Abstract_WC_Order_Data_Store_CPT->update_order_meta_from_object() and get_internal_data_store_key_getters() but that's the crux of it

How to test this PR

  1. Go WC > Settings > Advanced > Features and enable HPOS without compatibility mode image
  2. Purchase a subscription product
  3. Populate the renewal order IDs cache meta by processing a renewal order for this subscription
  4. Open the HPOS settings again and enable compatibility mode and save the settings
  5. If the sync process is slow to kick-off here are some steps to manually run it:
    1. Run the wc_schedule_pending_batch_processes pending scheduled action
    2. Then run the new wc_run_batch_process scheduled action
  6. Open your DB and search wp_postmeta for the subscription ID
  7. Confirm the dates and cache meta is correctly synced from wc_orders_meta to wp_postmetaimage

Product impact

james-allan commented 7 months ago

I've run into a bit of a strange issue while testing this PR.

When the post record is backported, the related order cache meta keys and values are being duplicated.

For example:

Screenshot 2023-11-08 at 11 32 12 am

What is causing that is an issue in WooCommerce Core. I've submitted a PR to WC here: https://github.com/woocommerce/woocommerce/pull/41281

In short WC core's backporting functions are failing to locate the existing related order cache meta because it's comparing a serialised version of the array to the array a:0:{} !=== array()here it doesn't match and so they call add_post_meta() adding a new row. That PR will fix it for us too.

I need to look into whether this PR is contributing to that and if this duplicate meta will cause any issues.

james-allan commented 7 months ago

I need to look into whether this PR is contributing to that and if this duplicate meta will cause any issues.

It was getting difficult to keep straight in my head all the different testing versions, scenarios and results so I've included a table to illustrate the issues.

WC Core Subscriptions Core Result
NO RELATED ORDERS
trunk develop 🐛🐛 0 dates and no related order cache meta
https://github.com/woocommerce/woocommerce/pull/41281 develop 🐛 👍 0 dates but good related order cache meta
trunk This PR 👍 🐛 Good dates, dupped related order caches
https://github.com/woocommerce/woocommerce/pull/41281 This PR 👍👍 Dates and related order caches are good
RELATED ORDERS
trunk develop 🐛🐛 0 dates and duplicate related order caches
https://github.com/woocommerce/woocommerce/pull/41281 develop 🐛 👍 0 dates but good related order cache meta
trunk This PR 👍 🐛 Good dates, dupped related order caches
https://github.com/woocommerce/woocommerce/pull/41281 This PR 👍👍 Dates and related order caches are good

With this table of results in mind, we have 3 options.

  1. Remove the related order cache handling in this PR. Those issues will be fixed with the WC core PR. While we wait for the WC core PR the related order caches won't be copied to the post record.
  2. Leave this PR as is. While we wait for the WC release, it will result in duplicate related order caches.
  3. Leave this PR as is. Resolve the duplicate related order cache issue by ensuring the related order caches cannot be duplicated. ie hook onto something like add_{$meta_type}_metadata, and return false if that meta already exists or have a cleanup function that runs after backfill has happened to make sure it deletes the duplicates. We could use your existing set_renewal_order_ids_cache() setters to run this.
mattallan commented 7 months ago

I noticed the unit tests were failing because of the version of yoast/phpunit-polyfills we were using. I've just bumped this from 1.0.3 to 1.1.0 in b487239 and it's looking like it has fixed the issues.

I think we can merge this though despite the issues raised in https://github.com/Automattic/woocommerce-subscriptions-core/pull/538#issuecomment-1801122335. I'm working on some changes separately that I'll submit a PR for today

Thanks @james-allan !! Once the checks pass I'll go ahead and merge this.