Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added notification emails and basic scheduling logic #648

Draft
wants to merge 48 commits into
base: trunk
Choose a base branch
from

Conversation

peterfabian
Copy link

@peterfabian peterfabian commented Aug 5, 2024

First POC that contributes towards #645

Description

This is a WIP to add notifications to Subscriptions Core.

Currently, it

  • adds simple email templates,
  • adds email-related classes,
  • registers these classes,
  • adds simple scheduling logic for actions that should trigger the email with pre-defined offset (3 days)

Mostly, it's inspired by code that's already in Subscriptions Core.

Notification Types Added

  • Auto Renewal
    • scheduled {{time_offset}} ahead of $subscription->get_date( 'next_payment' ) for automatically renewing subscriptions ($subscription->is_manual() is false)
  • Manual Renewal
    • scheduled {{time_offset}} ahead of $subscription->get_date( 'next_payment' ) manual subscriptions ($subscription->is_manual() is true)
  • Trial Expiry
    • scheduled {{time_offset}} ahead of $subscription->get_date( 'trial_end' ) manual subscriptions
  • Subscription End
    • scheduled {{time_offset}} ahead of $subscription->get_date( 'end' ) manual subscriptions

Open Questions

Some questions that crossed my mind while implementing this:

  • We need to decide where to put filters/actions to allow extensibility
  • I could have added the schedules to WCS_Action_Scheduler, but it seemed more appropriate to create a new class for notifications (WCS_Action_Scheduler_Customer_Notifications ), otherwise the scheduling logic might be a bit complex. It should be quite easy to change this, though.
  • The scheduling logic currently uses the state of the subscription object instead of deciding depending on which date/status is being updated. It was easier to grasp for me, given my inexperience with Subscriptions, but again, it should be possible to convert this to an approach similar to WCS_Action_Scheduler that relies entirely on Subscription status and which date is being updated (combining the logic of get_scheduled_action_hook, update_status and update_date methods).
  • Should we send a payment notification for trial expiry (in addition to trial expiry notification)?
  • Which conditions we want to check at schedule time vs at the time of sending the notification?
  • Do we want to unschedule notifications when store manager disables them?
    • Most likely not, as it's a lot of work to schedule/unschedule all of them. If they're disabled, this will create clutter in the db, though.

Side Notes

  • I had to downgrade to node 16 to build it
  • I had problems committing as phpcs was giving me errors. I updated ControlStructureSpacingSniff.php (converted last argument to array for ExtraSpaceBeforeCloseParenthesis and ExtraSpaceAfterCloseParenthesis sniffs), otherwise phpcs was failing. Perhaps there's a better way to solve this, but I didn't want to spend too much time investigating what's going on and what array should be passed where it gets a string. Updating squizlabs/php_codesniffer to 3.10.2 didn't help either.

TODO

  • Add trigger to send the emails automatically
    • Add order note when an email is sent
    • Respect when the site is staging site and other exclusions
  • Surface the day offset setting in the WC settings
  • Add trigger to send a notification manually from order/subscription edit screen
  • etc etc

How to test this PR

  1. Create a couple of different types of subscriptions: simple, variable, with free trial, with defined end-date/length, with free trial and length
  2. Buy these subscriptions, update them, renew them manually and automatically, cancel them.
  3. See if the notifications are being (un)scheduled as expected.
  • 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
  • Added deprecated functions, hooks or classes to the spreadsheet

@peterfabian peterfabian self-assigned this Aug 5, 2024
Copy link
Member

@xristos3490 xristos3490 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing work, @peterfabian! Loved how you incorporated the WCS_Scheduler structure and the date types! Everything seem to be falling into place 🙌

Left some initial thoughts on the structure before I start doing some hands-on testing.

includes/class-wcs-core-autoloader.php Outdated Show resolved Hide resolved
unset( $date_types_to_schedule['start'] );

//TODO: filter?
$this->date_types_to_schedule = array_keys( $date_types_to_schedule );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me! An internal filter would be helpful for quickly managing active notifications "types"

If we do add one, we need to reuse it within each email's trigger function to handle existing records before applying that filter. It's a small "gap" but has a simple solution.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might also concern the global toggle of this feature. Since it is initially disabled, we need a reliable method to control its access. Adding a filter here could introduce some complexity, but there is a crucial difference between this class and the WCS_Action_Scheduler, in that this class requires a toggle to enable or disable the feature. 🤔

A follow-up question here would be related to how we handle this "global toggle." Should that be a checkbox? Or should we consider an empty value in the "days input" to indicate that it is disabled? I lean more toward the latter option.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I'm looking at it, I can see the emails in Woo have their own toggle for enabling/disabling, maybe we can reuse that (and if we provide a global disable, we can just set this toggle for all notifications?)

Copy link
Member

@xristos3490 xristos3490 Aug 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we probably don't need a filter here at this point.

I also think that we could pass the date type into the days-getter-filter and allow developers to disable specific types (e.g., by returning 0.)

Related question: Should we schedule notifications for disabled emails (based on the UI toggle)? I think we still should. This is different from disabling by returning 0 from the filter above, as we cannot schedule "0 days before the event".

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we schedule notifications for disabled emails (based on the UI toggle)? I think we still should.

Yes, I think we should and the code indeed does that.

@james-allan james-allan self-requested a review August 19, 2024 00:16
if (
! (
$subscription->has_status( 'active' )
|| $subscription->has_status( 'pending-cancel' ) //TODO: do we want to create notifications when user cancelled the subscription?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we want to create notifications when user cancelled the subscription?

I think we'll need to test that to confirm but I don't think we would.

For some background the WCS_Scheduler hooks into both subscription date and status updates.

When a subscription date is updated, we schedule the related AS event.
When a subscription status is changed, we unschedule any events that are no longer applicable to that status and schedule events that are. eg putting a subscription on-hold cancels the payment event. Reactivating a subscription reschedules the next payment date.

I think this notification class should follow a similar principle.

So to answer the question then I don't think we do need to create notifications on the cancelled event because when a customer cancels the subscriptions, we calculate the end date and schedule the end date. The scheduling of that date is what should trigger the notification to be scheduled, not the status transition itself.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for elaborating. My todo comment was added to clarify whether we want to notify the customer about the upcoming expiry if they manually canceled the subscription (and thus should be aware that it's expiring).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My todo comment was added to clarify whether we want to notify the customer about the upcoming expiry if they manually canceled the subscription (and thus should be aware that it's expiring)

Oh right. I think the answer is yes, even if the customer initiated the cancellation, we'd still want to send the pre-cancel email. The reason is that they may have cancelled the subscription weeks or months earlier and so notification of it being finally cancelled.

$dt->sub( new DateInterval( "PT{$this->get_hours_offset( $subscription )}H" ) );

return $dt->getTimestamp();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be missing the rest of the context here but I'm wondering if we should just use the number of seconds and timestamps?

I noticed that the setting (interval and period) is being converted into hours and I'm wondering if it would be easier to convert it into seconds and then just do a straight subtraction.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a good idea to me. Working with seconds allows us more granular control.
Also, since both the subscription's get_date() and the AS work in UTC by default, we should be aligned. Right?

Copy link
Author

@peterfabian peterfabian Sep 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I generally prefer to work with standard DateTime classes when available to avoid some of the pitfalls mentioned here https://gist.github.com/timvisee/fcda9bbdff88d45cc9061606b4b923ca, but I suppose using timestamps would be easier here and should be ok.


if ( $subscription->get_date( 'next_payment' ) ) {
$this->schedule_payment_notification( $subscription );
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be right but wanted to double check. When a subscription's date is updated you reschedule all dates?

eg if the next payment date is updated, it would pass the in_array() check and then all subscription dates would be rescheduled.

Copy link
Author

@peterfabian peterfabian Sep 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When a subscription's date is updated you reschedule all dates?

Yes. If the scheduling is correct, nothing is written to the db, but the code currently checks whether the scheduling is correct for all 3 types of notifications (and fixes it if it isn't).

* @param string $date_type Can be 'trial_end', 'next_payment', 'end', 'end_of_prepaid_term' or a custom date type
*/
public function delete_date( $subscription, $date_type ) {
$this->update_date( $subscription, $date_type, '0' );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noting that the 3rd param isn't used in update_date(). It might still work though. Just wanted to flag as I skimmed :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean it might still work? 😄
I'm old-school, so if a method is declared with 3 arguments, I provide 3 arguments.

Jokes aside, I removed the call as I updated the logic when notifications get scheduled.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also reading my comment and wondering what I meant by "it might still work". 🤔 😆

I think I took that a function called delete_date() calling a function update_date() with 0 as a param to mean that passing 0 would cause the update_date() call to behave in a sort of delete mode.

I was flagging that the third param in this version of update_date() isn't used and therefore '0' couldn't cause it to work in a delete mode.

I think I was noting that I would need to come back to this. After looking at it a little deeper I don't know if I can tell there is a problem here. When a date is deleted, it will eventually call schedule_all_notifications() and that function only schedules notifications if a date exists. eg

if ( $subscription->get_date( 'next_payment' ) ) {
   $this->schedule_payment_notification( $subscription );
}

Because the date is deleted, that if condition would fail and it wouldn't schedule an event. That might be correct, but wouldn't we need to unschedule a related event if a relevant date is deleted?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way to test this I guess would be to have a subscription with a next payment date and a pre-renewal event scheduled. Then call $subscription->delete_date( 'next_payment' ) and see if it correctly unschedules the event.

Copy link
Contributor

@james-allan james-allan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks pretty good @peterfabian. I haven't had a chance to test it today, however I've given it a skim in terms of reviewing the general approach taken.

I left some initial comments. :)

Just a heads up. I noticed in the PR array(). We have switched to use the shorthand syntax ([]) however haven't updated all existing uses. Our current approach is to use [] in newer code.


break;
case 'pending-cancel':
// Unschedule all notifications?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you would want to unschedule all, I think the expiration one is still one you'd want.

To allow easier batch processing, notifications are updated whenever subscription is updated after the settings changed, if the previous update of the subscription was before the settings changed.
Copy link
Contributor

@james-allan james-allan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @peterfabian and the rest of the team working on this! 💯

Really nice work here. It's a monster of a PR to review.

I left a few inline comments but I'm happy if you want to turn those into issues so we can move forward with smaller issues and bug fixes.

A couple of general things I noticed that I thought were worth raising.

  1. $subscription->get_time() returns a subscription date as a timestamp. I noticed in a couple of places where a datetime object is being created from the get_date() mysql formatted value, and then converting into a timestamp via getTimestamp(). I'm curious if that's by design or if the approach taken in get_time() isn't correct or there's a preference with it. I ask because in general it would be my preference to use the established helpers to get subscription dates as a timestamp format rather than have multiple places where date formats are converted just from a DRY principle.
  2. Function block comments. I noticed there's a few functions missing function block comments. Understandably so given the size of the PR. I left a few inline comments before realising it's more widespread. Just wanted to flag it incase you respond to those inline comments. But as I mentioned, I'm happy to move forward and address that separately rather than hold up this PR on such a minor technicality. :)

return array_map( 'wcs_sanitize_subscription_status_key', $allowed_statuses );
}

public function get_total_pending_count(): int {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: This function is missing a function comment.

}

/**
* Start the background process for coupon data conversion.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Start the background process for coupon data conversion.
* Start the background process for batch processing subscription notifications updates.

Looks like a copy-paste.

}

/**
* Stop the background process for coupon data conversion.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Stop the background process for coupon data conversion.
* Stop the background process for batch processing subscription notifications updates.

/**
* Subscriptions Email Notifications Class
*
* Some details to enlighten your exploration of this code.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Some details to enlighten your exploration of this code.

Were there supposed to be details that enlighten my exploration ? 😄


add_filter( 'woocommerce_order_actions', [ __CLASS__, 'add_notification_actions' ], 10, 1 );

// TODO this is a bit ugly...
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. We generally don't like using closures/anonymous functions especially attached to callbacks but we can address this separately if you're eager to merge this big PR.


$this->heading = __( 'Subscription Expiring Notification', 'woocommerce-subscriptions' );
// translators: placeholder is {blogname}, a variable that will be substituted when email is sent out
$this->subject = sprintf( _x( '[%s] Subscription is about to expire', 'default email subject for cancelled emails sent to the admin', 'woocommerce-subscriptions' ), '{blogname}' );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$this->subject = sprintf( _x( '[%s] Subscription is about to expire', 'default email subject for cancelled emails sent to the admin', 'woocommerce-subscriptions' ), '{blogname}' );
$this->subject = sprintf( _x( '[%s] Subscription is about to expire', 'default email subject for upcoming subscription expiration emails sent to the customer', 'woocommerce-subscriptions' ), '{blogname}' );

$order_note_msg = sprintf( __( '%1$s was successfully sent to %2$s.', 'woocommerce-subscriptions' ), $this->title, $this->recipient );
} else {
/* translators: 1: Notification type, 2: customer's email. */
$order_note_msg = sprintf( __( 'Attempt to send %1$s to %2$s failed successfully.', 'woocommerce-subscriptions' ), $this->title, $this->recipient );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$order_note_msg = sprintf( __( 'Attempt to send %1$s to %2$s failed successfully.', 'woocommerce-subscriptions' ), $this->title, $this->recipient );
$order_note_msg = sprintf( __( 'Attempt to send %1$s to %2$s failed.', 'woocommerce-subscriptions' ), $this->title, $this->recipient );

A successful failure. 🤔

echo wp_kses(
sprintf(
// translators: %1$s: name of the blog.
_x( 'Your subscription for XYZ on %1$s will be renewed automatically. Thanks for being a loyal customer with us!', 'In customer renewal invoice email', 'woocommerce-subscriptions' ),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm assuming xyz is a placeholder for now but is that going to be the product or something else?

Just curious because for subscriptions with multiple items, it could get unwieldy.

echo wp_kses(
sprintf(
// translators: %1$s: name of the blog, %2$s: link to checkout payment url, note: no full stop due to url at the end
_x( 'Your subscription for XYZ on %1$s is about to expire. To keep receiving the goodies, renew it over here: %2$s', 'In customer renewal invoice email', 'woocommerce-subscriptions' ),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I apologise if I've missed it, but we may need to consider a different word other than "renew".

WooCommerce Subscriptions uses renew to describe the regular scheduled payments for a subscription. Subscriptions renew every month for example, but they only expire at their predetermined end date.

Subscriptions expire (end automatically after x periods). eg $14/month for 12 months. After the 12 months, the subscription will automatically expire and the customer would need to resubscribe - not renew.

Also, linking the customer to the subscription's checkout payment URL wouldn't work. That would send them to a URL like this /checkout/order-pay/9269/?pay_for_order=true&key=wc_order_U168PYBA4ixPl which is an empty pay for order page.

Screenshot 2024-09-19 at 5 10 29 pm

TBH I'm not sure what the CTA would be for an upcoming subscription expiration because until it's expired, there's not a lot they can do. Once it is expired, they can resubscribe.

echo wp_kses(
sprintf(
// translators: %1$s: name of the blog, %2$s: link to checkout payment url, note: no full stop due to url at the end
_x( 'Your subscription for XYZ on %1$s is about to expire. To keep receiving the goodies, renew it manually over here: %2$s', 'In customer renewal invoice email', 'woocommerce-subscriptions' ),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar feedback to above. Expire in WooCommerce Subscriptions means end. We may prefer to use "expire" in this customer facing way because that makes sense to customers -- in which case this feedback can be ignored. 😅

Perhaps something like

"You subscription for xyz on %1$s is nearly due for renewal. To keep receiving..."

The CTA is also not likely to work here either because the renewal order wouldn't have been created yet and so they would need to wait until the renewal is created in order to pay for the renewal. There's a renewal invoice email that would be sent for that purpose.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants