search

Automattic / woocommerce-subscriptions-core

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

Refactor WC_Subscriptions_Tracker to support HPOS #374

Closed Jinksi closed 1 year ago

Jinksi commented 1 year ago

Fixes #147

Description

This PR replaces SQL queries and other non-CRUD WC functions present in the WC_Subscriptions_Tracker class with HPOS-compatible alternatives.

This class hooks into the WC woocommerce_tracker_data filter to calculate and send subscription-related data about a store (opted-in only) to tracking.woocommerce.com/v1/, once per week by default.

You can prepare and view the data to be sent without sending by running WC_Tracker::get_tracking_data() in the WP Console plugin or similar.

Note on data types

~The data type of some values in the output has changed from str to float/int.~

As we talked about below, all values will be returned as a string. https://github.com/Automattic/woocommerce-subscriptions-core/pull/374#issuecomment-1385320556

This data is processed by wp_json_encode() in WC_Tracker::send_tracking_data() which keeps the dtypes intact (keeps an int an int).

In practice, the new approach outputting numbers as ~int/float~ string is more consistent and predictable. ~However, this may be a concern for backwards compatibility purposes, depending on what the recipient of this data is expecting.~

The values are received by gh-woocommerce/woocommerce-tracking-api-service and converted to a string before storing (WC_Tracker_API_V1::save_tracking_data()).

Todo

How to test this PR

  1. Enable WC Tracking in WC Settings → Advanced → WooCommerce.com → Enable Tracking.
  2. Ensure your store has multiple subscriptions. Ideally, some are created in the past, including switch, renewal and resubscribes.
  3. Start on trunk with HPOS disabled.
  4. Run WC_Tracker::get_tracking_data() with the WP Console plugin, noting what is output under 'extensions'=>'wc_subscriptions'
  5. Checkout this branch, enable HPOS + syncing (wait for the sync to complete).
  6. Run WC_Tracker::get_tracking_data() again. Compare the output of 'extensions'=>'wc_subscriptions' with what was output on trunk, ensuring they are identical. Note: run WC_Tracker::get_tracking_data()['extensions']['wc_subscriptions'] for direct access to relevant data.

Example before (trunk):

"subscription_orders" => [
    "switch_gross" => "0",
    "switch_count" => "3",
    "renewal_gross" => "200",
    "renewal_count" => "16",
    "resubscribe_gross" => "10",
    "resubscribe_count" => "1",
    "initial_gross" => 45,
    "initial_count" => 5,
  ],

After (this PR):

"subscription_orders" => [
    "switch_gross" => 0.0,
    "switch_count" => 3,
    "renewal_gross" => 200.0,
    "renewal_count" => 16,
    "resubscribe_gross" => 10.0,
    "resubscribe_count" => 1,
    "initial_gross" => 45.0,
    "initial_count" => 5,
  ],

After wp_json_encode():

"subscription_orders": {
    "switch_gross": 0,
    "switch_count": 3,
    "renewal_gross": 200,
    "renewal_count": 16,
    "resubscribe_gross": 10,
    "resubscribe_count": 1,
    "initial_gross": 45,
    "initial_count": 5
  }

Product impact

Jinksi commented 1 year ago

Thanks @mattallan!

Thoughts on casting some of these values to strings? 🙈

I agree, we should make these data types consistent. It is currently a bit of a mix of string/int on trunk, example below.

Example output showing mix of data types from `trunk` ```php "subscriptions" => [ "first" => "2022-02-17 05:28:05", "last" => "2023-01-11 06:27:48", "wc-pending" => 0, "wc-active" => "1", "wc-on-hold" => "13", "wc-cancelled" => "2", "wc-switched" => 0, "wc-expired" => 0, "wc-pending-cancel" => 0, ], "subscription_orders" => [ "switch_gross" => "0", "switch_count" => "4", "renewal_gross" => "420.2", "renewal_count" => "21", "resubscribe_gross" => "22", "resubscribe_count" => "1", "initial_gross" => 223.2, "initial_count" => 13, ], ```

Since the current/trunk implementation prefers returning strings for 'truthy' values, I'd say we should send all numeric values as a string. Either way (string or int/float), it will be an improvement to be explicit and consistent.

Jinksi commented 1 year ago

Something I noticed is WooCommerce Core hasn't updated their tracker class to support HPOS yet, I wonder what approach they will use.

WC does not help guide us towards a consistent approach here either; here are the product counts from WC_Tracker::get_tracking_data():

"products" => [
    "total" => "11",
    "external" => 0,
    "grouped" => 0,
    "simple" => 7,
    "subscription" => 2,
    "variable" => 1,
    "Variable Subscription" => 1,
    "yith_bundle" => 0,
  ],
mattallan commented 1 year ago

WC does not help guide us towards a consistent approach here either; here are the product counts

Oh wow, it's so strange how inconsistent this data is 😭 I'm generally okay with it being a bit mix-and-match as long as it doesn't impact how the tracking data is used internally.

Happy with whatever you think is the right approach!

Jinksi commented 1 year ago

@mattallan I think strings are a safe bet, enforced in a6e7012.

I've found the data recipient at gh-woocommerce/woocommerce-tracking-api-service, where it is converted to a string before storing (WC_Tracker_API_V1::save_tracking_data()).

haszari commented 1 year ago

I think strings are a safe bet, enforced in https://github.com/Automattic/woocommerce-subscriptions-core/commit/a6e7012004d59cbc6ed86b3de5f9c287775de8e3.

I agree, the data is ultimately sent as json (i.e. a string / text format) so if some numbers are rendered to string early I don't think that's a problem at all 😁

Jinksi commented 1 year ago

the data is ultimately sent as json

Although, a value's dtype before encoding to json will be recovered when decoding.

So if the recipient was assuming a value to be of type float or int after decoding and we had changed it to a string, 🐛 😞