Automattic / woocommerce-subscriptions-core

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

HPOS syncing from the post record doesn't sync subscription dates #355

Closed james-allan closed 1 year ago

james-allan commented 1 year ago

Describe the bug

When the post record becomes out of sync with the HPOS table record, WC will attempt to sync the subscription using the post record as the authoritative table.

This doesn't work with some subscription-specific data. eg dates.

To Reproduce

  1. Enable HPOS and syncing (see screenshot below).
  2. Go into the database and manually alter the next payment date of a subscription.
  3. View the subscription via the admin screen or customer my account page.
  4. The next payment date should update to reflect the change you made manually. On trunk currently, it won't.

image

Expected behavior

The subscription should sync all data from the post record when it gets out of sync.

Actual behavior

Next payment dates and possibly other data points don't sync.

Product impact

Additional context

From the brief investigation I've done, this is caused by a bug in OrdersTableDataStore::set_order_prop() which is called from OrdersTableDataStore::migrate_post_record()

james-allan commented 1 year ago

From the brief investigation I've done, this is caused by a bug in OrdersTableDataStore::set_order_prop() which is called from OrdersTableDataStore::migrate_post_record()

Because there is no individual setter for things like subscription dates eg $subscription->set_next_payment_date(), OrdersTableDataStore::set_order_prop isn't able to set these properties when the post and order table record is out of sync.

We can get a round this by adding private functions in the datastore for setting these properties. Eg `set_next_payment_date().

james-allan commented 1 year ago

After spending some time yesterday investigating this issue, I now think this falls into a wontfix category.

The only subscription properties that aren't resynced are subscription specific date properties. Namely:

While these are hugely important properties to remain in sync for a subscription to function correctly, for any of these to get out of sync, someone would need to alter the database directly or call a function like update_post_meta( '_schedule_next_payment', ... ) to change the date manually rather than than call the appropriate API $subscription->update_dates( ... )

However, anyone that updates a subscription date directly in that way would be causing major desync issues on their site anyway and syncing that data would only contribute to shadow supporting it.

We rely on folks to call the $subscription->update_dates() function in order to create/update the scheduled actions to reflect those dates. We also require $subscription->update_dates() to be called so that the date relationships are validated (you can't have a next payment after the end date etc).

With this in mind, if this issue's sole purpose is to make sure HPOS records are resynced if someone, somehow changes the CPT post meta record for a subscription date, then they would have had desynced scheduled actions anyway and I don't think that scenario is worth supporting.

With that in mind, I'm going to close off this issue, but feel free to leave a comment if you have a different opinion on this.