search

Automattic / woocommerce-subscriptions-core

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

With HPOS + compatibility mode enabled, manually created subscriptions don't have a start date in the new order tables #514

Closed mattallan closed 9 months ago

mattallan commented 9 months ago

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

Description

When a store has both HPOS and compatibility mode enabled, there is some odd behaviour where new subscriptions are getting created with _scheduled_start meta stored as 0 in the wp_wc_orders_meta table and the correct start date in the wp_postmeta.

WP Posts Table WC Orders Table
image image

With HPOS enabled and compatibility mode disabled, the correct _scheduled_start is stored in wp_wc_orders_meta so this issue just impacts those with the compatibility mode feature.

The approach I've taken in this PR is to fix this by updating the persist_order_to_db() function inside our Subscriptions OrdersTable datastore class so that if the schedule_start prop needs to be updated and it's empty, use the subscription's created date instead.

We do similar behavior already in init_order_record() however, this function doesn't write the dates to the database as it's surrounded by $subscription->set_object_read( false );.

How to test this PR

Not sure if this is necessary but to flick between the different HPOS and compatibility mode settings, rather than waiting for the order data sync to finish, I just wipe my orders and subscriptions from the DB using the following queries:

TRUNCATE TABLE `wp_wc_orders`;
TRUNCATE TABLE `wp_wc_orders_meta`;
TRUNCATE TABLE `wp_wc_order_addresses`;
TRUNCATE TABLE `wp_wc_order_operational_data`;

DELETE FROM wp_posts WHERE post_type = 'shop_subscription' OR post_type = 'shop_order';
DELETE pm
FROM wp_postmeta pm
LEFT JOIN wp_posts wp ON wp.ID = pm.post_id
WHERE wp.ID IS NULL;

DELETE FROM wp_usermeta WHERE meta_key = '_wcs_subscription_ids_cache';
  1. Enable HPOS and sync (compatibility mode) in WC > Settings > Advanced > Features
  2. Go to WC > Subscriptions and click Add Subscription
  3. Enter any details but do not change the status from "Pending"
  4. Click Update
  5. Using phpMyAdmin or some other DB tool:
    1. On trunk verify that _scheduled_start is set to 0 in the wc_orders_meta table but it is set to the correct value in the wp_postmeta table for that same subscription
    2. On this branch create a new subscription and verify that _scheduled_start has the same value in wc_orders_meta and wp_postmeta tables

Other tests:

Product impact

james-allan commented 9 months ago

@mattallan I was testing this and trying to understand what the origins of this issue is. When I run the following code using wp posts tables, the subscription doesn't have a start date stored in the database. 

$subscription = new WC_Subscription();
$subscription = $subscription->save();
Screenshot 2023-09-28 at 5 14 27 pm

On this branch when using wc orders table, it does have a start date so I'm thinking that the issue here isn't that the subscriptions doesn't have a date saved in the hpos tables, I believe the issue is that it does have a start date in the wp-posts tables when compatibility mode is on.

I'll have to dig into it some more tomorrow because I don't understand what is causing the post table to get a start date set when hpos and sycning is enabled.

james-allan commented 9 months ago

I think I've found the cause of the discrepancy here.

When you save a subscription using HPOS, in persist_order_to_db() it gets a 0 start date. After it is saved in the orders table with a 0 start date, the init_order_record() function runs and in our Subscriptions datastore it sets the start date if it's empty. After this, the subscription is then back-ported to the posts table and it gets this version of the subscription with a start date set and so it is saved.

So I think to untangle this, there'd need to be some work done to understand how to separate this "on load" we set a start date but don't save it.

Regarding this comment I made above. If you run this code snippet on just wp-posts tables, it doesn't save a start date. 

$subscription = new WC_Subscription();
$subscription = $subscription->save();

To resolve that I think we could update the WCS_Subscription_Data_Store_CPT::update_post_meta() function to behave similarly to the change made in this PR to persist_order_to_db() change and if the start date is empty, set it to the date created.

Whether this is the "right" approach to set start dates even if it's not specified. At first I thought that would be the wrong approach but looking at wcs_create_subscription() it does does this exact same sort of thing. It sets the start date to the date created or current time when creating a subscription.

So imo there are currently just discrepancies between loading and saving subscriptions in the different datastores.

Method HPOS WP posts
Read Does default the start date Does default the start date
Save In this PR, it does now default the start date Doesn't default the start date

The problem arises in this HPOS with syncing scenario because there's a read in between the two saves. IMO all the reads and writes need to be consistant.

james-allan commented 9 months ago

@mattallan I submitted a small change in https://github.com/Automattic/woocommerce-subscriptions-core/pull/514/commits/59983198b646ed813310e695a3daf24a07749ea3 brings the saving of subscription post meta inline with the existing changes on this branch so the hpos and post table experience is the same.

Let me know what you think. :)

mattallan commented 9 months ago

I submitted a small change in https://github.com/Automattic/woocommerce-subscriptions-core/commit/59983198b646ed813310e695a3daf24a07749ea3 brings the saving of subscription post meta inline with the existing changes on this branch so the hpos and post table experience is the same.

Thanks @james-allan, I tested non-HPOS sites and confirmed the start date is now added. I also added a changelog entry to this PR.

Merging