allow importing cancelled date, and fix handling of end_date

[dkoo]

Adds support for a new cancelled_date column to the importer CSV, so that you can set a cancelled date for subscriptions with an "ended"-type status.

Also resolves #201, in which end_date inputs are ignored for a couple of reasons:

1. WC_Subscription::update_dates() will reject and discard end_date and cancelled_date params if the subscription does not have an ended-type status. On master, end_date will never get set because update_dates is called before the status is set, meaning imported subscriptions will always have a pending status at the time dates are set. Fixed this by setting status earlier, in the params passed to wcs_create_subscription, instead of in a separate $subscription->update_status() call after updating the dates.
2. Setting end_date on a subscription without a cancelled_date or with a cancelled_date before the given end_date will result in a fatal error. Fixed this by looking for a cancelled_date input (and throwing an error if the given cancelled_date is not before end_date, and automatically setting cancelled_date to match end_date if not given.

Lastly, adds a woocommerce_subscription_imported_via_csv action hook that fires after a subscription has been successfully created and its data updated to match the input CSV data. This could be useful for other devs to extend and customize the importer's behavior after subscriptions are imported.

To test: create an import CSV with a cancelled_date column in Y-m-d H:i:s format. Test with:

• an empty value and no end_date (expected result: import should succeed with no warnings/errors)
• an empty value and an end_date (expected result: import should succeed a warning, and cancelled_date should get set to the same value as end_date)
• a value that occurs before the end_date value (expected result: import should succeed with no warnings/errors)
• a value that occurs after end_date (expected result: import should fail with an error)
• a cancelled_date and/or end_date with a subscription_status value of wc-pending or wc-active (expected result: import should fail with an error)

To test the new hook, add add_action( 'woocommerce_subscription_imported_via_csv', 'callback_name', 10, 2 ); somewhere and make sure it passes the expected arguments (the WC_Subscription object of the imported subscription, and the results array from the import).

[james-allan]

Hey @dkoo. Thanks for the PR.

I think the changes generally look good. I left some initial review comments. Let me know what you think. :)

[james-allan]

Suggested change

	do_action( 'woocommerce_subscription_imported_via_csv', $subscription, $result );
	do_action( 'woocommerce_subscription_imported_via_csv', $subscription, $result, $data );

Passing the data variable (the csv row) might assist with folks who have custom data that they would like to set.

[dkoo]

Great idea—added in 4bba344

[james-allan]

IIRC this switch statement purposefully doesn't have break statements and it is expected to flow through the lower cases. eg cancelled being the top case means that the cancelled date is also validated against the end date, next payment and trial end checks. ie the cancelled date must occur after the next payment date (which is a condition under the end date case).

I've never been a fan of this code though as it's super confusing, hard to follow and is not intuitive at all. I wonder if the csv importer would benefit just not validating the dates relationships at all? 🤔

It calls $subscription->update_dates() which validates them anyway and throws exceptions if they don't pass the validation rules.

The csv importer/exporter calls update_dates in a try catch block so would be able to catch those exceptions and handle them appropriately anyway.

[james-allan]

I just realised that removing this section would essentially break an important part of test mode as folks would use test mode to verify that the dates are valid and update_dates() isn't called while in test mode. 🤔

hmm. How important were these changes for the goal of this PR? It seems from the PR description that you were mostly set on allowing users to set cancelled dates and for the end date issue with regard to when the status is set.

So, can these proposed changes be removed from this PR and we can look into doing a bit more of an overhaul of the validation of dates at a later time?

[dkoo]

@james-allan you're right—these changes are a bit overzealous. I've reduced their scope to only add validation for checking cancelled_date vs. end_date, so the behavior for validating other dates should be intact.

[dkoo]

@james-allan thanks for reviewing and testing these changes! Your feedback helped me narrow down the actual cause of the issue where end_date wasn't getting set upon import. Here's what's actually happening in master:

• When creating a subscription via wcs_create_subscription, the importer is not passing status, so all subscriptions are created with a pending status at first.
• By being in pending status, when calling $subscription->update_dates(), if next_payment_date is passed with an empty value, the next payment date is auto-calculated.
• However, when passing end_date and/or cancelled_date to a subscription in pending status, these dates are ignored.
• Conversely, when passing next_payment_date to $subscription->update_dates() with an empty value to a subscription in active status (as was initially implemented in this PR), it actually clears out the next payment date rather than auto-calculating it.

So the fix is to retain the pending status when calling wcs_create_subscription for subscriptions that we want to import with an eventual active, pending, or on-hold status. This way the next_payment_date gets auto-calculated for those if needed. But for all other statuses, wcs_create_subscription should be called with the actual status so that any cancelled_date and end_date values get set. Then, later on, we call update_status again to make sure that pending subscriptions get updated to their intended active or on-hold statuses.

Let me know if this is working for you, now!

[james-allan]

when calling $subscription->update_dates(), if next_payment_date is passed with an empty value, the next payment date is auto-calculated.

This isn't my understanding of how this works. If you don't pass all dates, it will populate the a full set of dates from the incoming changes and the subscription's current dates so it can validate all eventual dates together (link to that code), is this what you meant by automatically calculating?

The only thing I can think of where dates are automatically set is when you update the status. So, for example, setting a subscription to active will automatically calculate a next payment date if the existing one is in the past or not set at all. I don't think calling update_dates() will ever automatically calculate any dates though. Let me know if I've misunderstood or missed something. :)

I do think this is the root cause behind #201 though. In the old importer code, the subscription would always go through a status transition. eg start as pending and be updated to the eventual status (expired). Whenever a subscription is cancelled or expired, we always set the end date, irrespective of what was previously set. IIRC creating subscriptions via the REST API had a similar problem. In that case we added a new REST param to enable users to set or update the status. Setting the status bypasses any additional business logic and just sets the status of the subscription without any additional calculations etc.

However, when passing end_date and/or cancelled_date to a subscription in pending status, these dates are ignored.

I don't think this is the case. I can create a 'pending' subscription with a cancelled date and end date with the following code:

$subscription = wcs_create_subscription( [
	'customer_id'      => 1,
	'billing_period'   => 'month',
	'billing_interval' => 1,
] );
			
$subscription->update_dates( [ 
	'cancelled_date' => '2024-02-12 08:00:00',
        'end_date'       => '2025-12-25 10:00:00'
] );

In that example the subscription is created correctly as pending with a cancelled and end date.

---

In any case, I think these changes are good now. It resolves #201 because when an imported subscription is cancelled, or expired, it gets created with that status avoiding a status transition which was causing it to override the previously set end date. 👍

This PR also allows importing subscriptions with a cancelled date. 👍

There's a new filter to allow third-parties to set any additional data from the csv row.

[dkoo]

This isn't my understanding of how this works. If you don't pass all dates, it will populate the a full set of dates from the incoming changes and the subscription's current dates so it can validate all eventual dates together (link to that code), is this what you meant by automatically calculating?

The only thing I can think of where dates are automatically set is when you update the status. So, for example, setting a subscription to active will automatically calculate a next payment date if the existing one is in the past or not set at all. I don't think calling update_dates() will ever automatically calculate any dates though. Let me know if I've misunderstood or missed something. :)

I was misunderstanding how the logic works on the WCS side, but that's more or less what looked like was happening in the import function. So if I understand correctly now, what happened was the end dates were set correctly on the pending subscriptions, but then they got reset when transitioning to cancelled a bit later. And then the reset from pending to active is actually what resets the next_payment dates. That makes sense why the fix now works with setting pending for subscriptions we want to eventually be active, but not for ones we want to end up in a non-active state.

Thanks for clarifying, testing, and approving!