Automattic / woocommerce-subscriptions-core

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

Fix subscription product sales ending before WC products #611

Closed mattallan closed 1 month ago

mattallan commented 2 months ago

Fixes 4646-gh-woocommerce/woocommerce-subscriptions

Description

When the same scheduled sale days are set on a subscription product and a regular product, the sale price for the subscription product expires earlier than the regular product.

After some investigation, I found that WooCommerce sets the "sale from" to be the beginning of the given day, and the "sale to" to be the end of the given day i.e, their code:

$date_on_sale_from = date( 'Y-m-d 00:00:00', strtotime( $date_on_sale_from ) );
$date_on_sale_to = date( 'Y-m-d 23:59:59', strtotime( $date_on_sale_to ) );

In Subscriptions we didn't apply this rule and so sales were ending earlier for subscription products than regular products despite having the same dates.

This PR fixes this issue by making sure the scheduled sale "to" date ends at Y-m-d 23:59:59.

How to test this PR

  1. Create a subscription product and simple WC product with the sale schedule (found in the general tab)image
  2. Put the following snippet into something like WP Console compare the scheduled sale dates for both products:
    
    $product_id = 123123;
    $subscription_product_id = 456456;

echo "---- Simple Product ----" . PHP_EOL; $from = get_post_meta( $product_id, '_sale_price_dates_from', true ); $to = get_post_meta( $product_id, '_sale_price_dates_to', true ); echo '_sale_price_dates_from = ' . gmdate( 'Y-m-d H:i:s', $from ) . ' ( ' . $from . ' )' . PHP_EOL; echo '_sale_price_dates_to = ' . gmdate( 'Y-m-d H:i:s', $to ) . ' ( ' . $to . ' )' . PHP_EOL; echo PHP_EOL . PHP_EOL; echo "---- Subscription Product ----" . PHP_EOL; $from = get_post_meta( $subscription_product_id, '_sale_price_dates_from', true ); $to = get_post_meta( $subscription_product_id, '_sale_price_dates_to', true ); echo '_sale_price_dates_from = ' . gmdate( 'Y-m-d H:i:s', $from ) . ' ( ' . $from . ' )' . PHP_EOL; echo '_sale_price_dates_to = ' . gmdate( 'Y-m-d H:i:s', $to ) . ' ( ' . $to . ' )' . PHP_EOL;


4. Confirm they're the same.
5. Now update your sites timezone, go back to each product and save the sale dates again.
6. Run the above snippet again and confirm both dates are the same.
7. Inside wp-config, set your servers timezone using `date_default_timezone_set( 'Australia/Brisbane' );`
8. Go back to each Edit Product page and save the sale to and from dates.
9. Run the above snippet again and confirm the sale dates stored on the product and subscription are the same.

## Product impact
<!-- What products will this PR ship in? -->

- [x] Added changelog entry (or does not apply)
- [ ] Will this PR affect WooCommerce Subscriptions? yes/no/tbc, add issue ref
- [ ] Will this PR affect WooCommerce Payments? yes/no/tbc, add issue ref
- [ ] <!-- 🚨 Deprecations 🚨 --> Added deprecated functions, hooks or classes to the [spreadsheet](https://docs.google.com/spreadsheets/d/1xw9xszcPMnWsp4C8OKZMsLzZob7tOmWT7qMqmEIq314/edit#gid=0)
mattallan commented 1 month ago

Thanks @james-allan for the review!

We can either remove the setting of the _price and review the uses of it to make sure we aren't breaking anything, or leave these $date_from and date_to for now and open an issue to review the _price meta at a later date.

From my investigation, I think it would be fine to get rid of setting _price since it's updated inside handle_updated_props() which is called by create/update functions in the WC Product CPT class.

That said, I'd like to have a bit more time to look into other opportunities to clean up save_subscription_meta() but I don't think it's worth holding this fix back from the release tomorrow. So, I'm thinking of going with your second option and leaving the dates in there for now and open another issue.