Implement the ability to expire and delete old carts

Good idea but it needs further work.

I have a cart 37 days old and assigned 2 days as cart expiration term but the cart was not deleted after running cron manually. Not explored this deeply but I think the reason is that the cart expiry setting is not actually saved (I expect to see 2 days here now) and might be not visible in modules/cart/src/Plugin/QueueWorker/CartExpire.php

Also, it would be useful to expire cart not only per day basis but also per hour too.

There's been many adjustments, here is the PR: https://github.com/drupalcommerce/commerce/pull/639. I also have added a test.

Care to give the new patch a try https://patch-diff.githubusercontent.com/raw/drupalcommerce/commerce/pul...

The https://patch-diff.githubusercontent.com/raw/drupalcommerce/commerce/pul... still didn't work for me.

After applying interdiff-639.txt it works. Also, would be it more useful to set hours instead of days for cart to expire. Please, consider interdiff-639-hours.txt because it allows to set more precise expiry setting.

What I've found additionally is that if the cart was viewed on the admin/commerce/orders/{commerce_order} or admin/commerce/orders/{commerce_order}/edit or admin/commerce/orders/carts and the interval between the last cart refresh and current time is more than order type refresh frequency setting then the cart will be refreshed and therefore not removed. The same had happened when trying to get $order->cart->value before getting $order->getChangedTime() in modules/cart/src/Plugin/QueueWorker/CartExpire.php. Actually it had blocked any cart removing.

We need to keep this mind because there might come questions like: Why my carts are not removed in the time specified? Do not touch them and they will be removed.

That PR should get added as a patch here and rerolled. We also now have \Drupal\commerce\Interval that could be used to help determine the run cycle.

Issue needs triage in general.

Working on this

Here the patch rerolled and improved :

- Use Drupal\Commerce\Interval
- We can select now an unit for the expiration time
- #6 is fixed. This was because using orderStorage->loadMultiple() trigger the OrderRefresh and then change the order changed time to now.

Awesome!! Quick review notes. Will test locally as well.

1. +++ b/modules/cart/commerce_cart.module
   @@ -284,3 +335,36 @@ function commerce_cart_views_data_alter(array &$data) {
   +    if ($cart_expiration['enable'] && !empty($cart_expiration['unit']) && !empty($cart_expiration['number'])) {
   

   Should we do a !empty() on checking for enable as well?

2. +++ b/modules/cart/commerce_cart.module
   @@ -284,3 +335,36 @@ function commerce_cart_views_data_alter(array &$data) {
   +      $current_date = new DrupalDateTime('now');
   

   It defaults to 'now'. But maybe we can keep to be explicit

3. +++ b/modules/cart/src/Plugin/QueueWorker/CartExpire.php
   @@ -0,0 +1,94 @@
   +      // We skip the OrderRefresh process to not update the order and so
   +      // its changed time to now.
   +      $order = $this->orderStorage->loadUnchanged($id);
   

   Good catch!

1. Why not. But as the enable option is 0 or 1, this condition is sufficient. I wanted to be more "explicit" with other parameters.

2. To be honest, I'm not really comfortable when working on date :-). My first idea was to provide a DrupalDatetime with a DateTimeZone based on UTC (new DrupalDateTime('now', new DatetimeZone('UTC'))). But instead, for simplicity, I removed the DateTimeZone('UTC'), and then we use the default system timezone. And I let the 'now' parameter to stay explicit. But I have some doubt if we have to use (or not) the UTC TimeZone.

Cool, I'm fine keeping the 'now', just update to empty check, just in case.

To address #6

I do not know how often store managers go to view a cart or edit a cart. If they do so, I think that is valid that it persists longer (resets the countdown to deletion.)

The same had happened when trying to get $order->cart->value before getting $order->getChangedTime() in modules/cart/src/Plugin/QueueWorker/CartExpire.php. Actually it had blocked any cart removing.

This was fixed by using loadUnchanged.

Carts may not expire, however, if the refresh was changed to REFRESH_ALWAYS. Anytime admin/commerce/order/carts is loaded the first page will have their refresh run. But, these are edge cases I can live with. Orders should only refresh when the current user is the owner of the order while in a draft state.

See \Drupal\commerce_order\OrderRefresh::shouldRefresh.

I'm testing this manually locally.

• Patch updated for #11.1
• Also, added a $cart_expiration_default default value to the $order_type->getThirdPartySetting() method (in hook_cron and CartExpire), otherwise the new Interval will throw an exception if the order type has not been saved to set this new third party setting.
• Added a control on the QueueWorker if the cart expiration is still enabled when the queue worker run.

Woo! It worked. Putting to RTBC for bojanz eyes.

So, DrupalCI had some issues. Tests are fine. Back to RTBC.

rerolled patch.

Has anyone been using this on their site to provide feedback?

1. +++ b/modules/cart/config/schema/commerce_cart.schema.yml
   @@ -8,6 +8,19 @@ commerce_order.commerce_order_type.*.third_party.commerce_cart:
   +        enable:
   +          type: integer
   +          label: 'Cart expiration enabled'
   

   This should be changed to be a boolean, I feel like.

2. +++ b/modules/cart/config/schema/commerce_cart.schema.yml
   @@ -8,6 +8,19 @@ commerce_order.commerce_order_type.*.third_party.commerce_cart:
   +        number:
   +          type: integer
   +          label: 'Cart expiration time'
   

   Wondering if this should be something different

   EDIT: that's what we call it in Interval.

Here is an updated patch. enable is now boolean in the schema and updated the test.

1. +++ b/modules/cart/commerce_cart.module
   @@ -255,3 +306,41 @@ function commerce_cart_views_data_alter(array &$data) {
   +  $queue = \Drupal::queue('cart_expirations');
   
   +++ b/modules/cart/src/Plugin/QueueWorker/CartExpire.php
   @@ -0,0 +1,104 @@
   + *  id = "cart_expirations",
   

   "commerce_cart_expiration"

2. +++ b/modules/cart/src/Plugin/QueueWorker/CartExpire.php
   @@ -0,0 +1,104 @@
   + * Removes an expired Cart.
   

   Rework, lower case C.

There's other fixes to go as well.

New patch. The following remains.

1. +++ b/modules/cart/commerce_cart.module
   @@ -210,6 +216,45 @@ function commerce_cart_form_commerce_order_type_form_alter(array &$form, FormSta
   +  $form['commerce_cart']['cart_expiration_unit'] = [
   ...
   +  $form['commerce_cart']['cart_expiration_number'] = [
   

   Per recurring, the interval UX is to have number first and then the unit. And number should just be "interval"

   See https://cgit.drupalcode.org/commerce_recurring/tree/src/Plugin/Commerce/...

2. +++ b/modules/cart/commerce_cart.module
   @@ -210,6 +216,45 @@ function commerce_cart_form_commerce_order_type_form_alter(array &$form, FormSta
   +    '#title' => t('Number of time unit until cart expires'),
   

   Fix wording

3. +++ b/modules/cart/config/schema/commerce_cart.schema.yml
   @@ -8,6 +8,19 @@ commerce_order.commerce_order_type.*.third_party.commerce_cart:
   +        enable:
   +          type: boolean
   +          label: 'Cart expiration enabled'
   

   Remove enable. If it's enabled we save a valid number value. If it is not enabled, we save 0 as the number value.

Just deploy patch #27 on production. Everything is working fine. I will report in a few day once the big queue of abandoned carts will be purged.

Fixes the UI items and removes enabled.

+  $cart_expiration_default = ['unit' => 'day', 'number' => 0];

Why don't we provide a reasonable default, such as 30 days?

+  $form['commerce_cart']['cart_expiration_interval']['cart_expiration_number'] = [
+    '#type' => 'number',
+    '#title' => t('Interval'),
+    '#default_value' => $cart_expiration['number'],
+    '#required' => TRUE,
+    '#min' => 0,
+  ];

Why would we allow a minimum of zero? Shouldn't #min be 1?

+    '#options' => [
+      'hour' => 'Hour',
+      'day' => 'Day',
+      'month' => 'Month',
+    ],

Missing t().

+    $cart_expiration = $order_type->getThirdPartySetting('commerce_cart', 'cart_expiration', [
+      'unit' => 'day',
+      'number' => 0,
+    ]);
+
+    if (!empty($cart_expiration['unit']) && !empty($cart_expiration['number'])) {

Wouldn't it be shorter to provide no default and then do if (!empty($cart_expiration))
Same with the default in CartExpiration.

+      $ids = $order_storage->getQuery()
+        ->condition('type', $order_type->id())
+        ->condition('cart', TRUE)
+        ->condition('changed', $expiration, '<')
+        ->range(0, 50)
+        ->execute();

Let's use "<="

Why don't we provide a reasonable default, such as 30 days?

What if someone wants to disable cart expiration? By removing enabled we would always have it be configured.

Why would we allow a minimum of zero? Shouldn't #min be 1?

Makes sense -- we can save it as 0 if disabled and if empty/0 show 30.

Wouldn't it be shorter to provide no default and then do if (!empty($cart_expiration))
Same with the default in CartExpiration.

Yeah.

----

Also, I wonder if this should exclude orders attached to users. Right now it purges anonymous and authenticated carts. If we turn it on by default, then I definitely think we should. It's probably a good idea anyways to preserve authenticated user carts. Amazon does.

And what about a checkbox "Delete only anonymous cart" checked by default ?

Actually, I don't know if we need to worry about authenticated users or not. If the expiration threshold is high enough, their cart will be refreshed whenever they come back (a day, a week, etc) later. This prevents it from being purged.

Here is an updated patch per bojanz review. Didn't concern about authenticated users, given thoughts in #33

While we're at it: this isn't a task, it's a feature request.

Clean up default values in the form.

+    $cart_expiration = [
+      'unit' => $settings['cart_expiration_interval']['cart_expiration_unit'],
+      'number' => $settings['cart_expiration_interval']['cart_expiration_number'],
+    ];

This shows clearly that the form structure is odd, cart_expiration_interval instead of cart_expiration, and the prefixes on unit and number.

The file for CartExpirationTest is called CartExpirationsTest.

I'll clean that up on commit.

CartExpirationTest doesn't have test coverage for:
1) A deleted order doesn't prevent others from being deleted
2) Cart expiration can be disabled, and the queue worker will respect that and skip deletion
3 Carts that no longer qualify for deletion are skipped.

So no commit tonight. Attaching a patch with the #37 fixes and other nits.

Ryan suggested that we need a more direct phrase for the checkbox. Attaching a screenshot of the current version. Suggestions welcome.

Unassigning.

I'd say just "Delete abandoned carts" for the checkbox, use form #state to reveal the interval field if checked, and label that "Delete after" or such.

Working on test coverage

1) A deleted order doesn't prevent others from being deleted
2) Cart expiration can be disabled, and the queue worker will respect that and skip deletion
3 Carts that no longer qualify for deletion are skipped.

Here are the updated tests. Now we just need to bikeshed the label.

> Delete abandoned carts

Sounds great to me.

Agreed

Changed to Delete abandoned carts.

@mglaman
The tests in #38 and #44 appear to be identical, don't see any new test coverage.

😭😭😭 I realized my work was behind yours, so I did a branch and -3way merge of the patch and apparently lost my tests. I'll add them back in tomorrow.

Looks like there's a trait that contains indentical installCommerceCart() function.

Is this still missing tests?

It looks like 8.x-2.10 and above breaks this patch. I did some digging when I couldn't get it to work and it appears to be caused by the implementation from this issue (https://www.drupal.org/project/commerce/issues/2499645). Since the cron job is running the task under the anonymous account the QueryAccessSubscriber is trying to lock it down to only its carts, instead of allowing the entire order system to be queried.

It doesn't seem like there's any way to differentiate the cron job from a standard anonymous user, so the only fix I can think of is to disable the access checks for the initial abandoned carts entity query.

I guess the failing test also needs accessCheck like

$orders = $this->orderStorage->getQuery()->accessCheck(FALSE)->execute();

Patch doesn't apply anymore on 2.12

Simple reroll from #48 with the missing ->accessCheck(FALSE) added on the test's queries.

Still missing tests #38 about
1) A deleted order doesn't prevent others from being deleted
2) Cart expiration can be disabled, and the queue worker will respect that and skip deletion
3 Carts that no longer qualify for deletion are skipped.

Working on this.

Missing tests mentionned in #38 added

1) A deleted order doesn't prevent others from being deleted
2) Cart expiration can be disabled, and the queue worker will respect that and skip deletion
3 Carts that no longer qualify for deletion are skipped.

Cart expiration was not well disabled for the fail.

Fixing the test (I have recreated the cart1 instead of the cart2 which was previously deleted) for the latest test : A deleted order doesn't prevent others from being deleted.

But this shouldn't pass. I guess we may need a Try / Catch in the QueueWorker.

Looks like this is unrelated fails (because we test now again Core 8.7 ?). Switching to need review.

8.7.x broke a lot of kernel tests.

I've tested the #59 on my local install with 8.6 and the CartExpirationTest is green.

Once tests fixed against 8.7 #3038493: Tests are failing on Drupal 8.7, I will relaunch it.

Test relaunched but it failed because certainly of a missing $this->installEntitySchema() for an entity type (in reference to #3038493: Tests are failing on Drupal 8.7).

Same patch than #59. Local tests are green. To see what the bot says about it.

Missing test were added. And tests are again green :-). Looks like RTBC ?

reroll against 2.13

Some small nitpicks, I think this patch is essentially RTBC, depending what is though of these small questions. Functionality worked as expected, tests look thorough and UI looked correct. I think this qualifies for bojanz review.

1. +++ b/modules/cart/tests/src/Kernel/CartExpirationTest.php
   @@ -0,0 +1,269 @@
   +    // Setting the `changed` attribute doesn't work in save. Manually change.
   

   If changed doesn't work on save, why are we still passing it into the create/save code above?

2. +++ b/modules/cart/tests/src/Kernel/CartExpirationTest.php
   @@ -0,0 +1,269 @@
   +    $this->assertNull($this->reloadEntity($cart2));
   +
   +
   +    // A deleted order doesn't prevent others from being deleted.
   

   Extra blank line

Should we also name all 5 test carts and test more explicitly for the ids that should or shouldn't exist, instead of just checking the number of remaining carts? I don't see any flaws currently, but it seems like being explicit might make it a little more durable in the future.

What if instead of only expiry, it was a way to react on stale carts and allow custom actions via a triggered event and an event subscriber on the other end of it.

There could be:

• Some default event subscriber that delete old/stale carts at a configurable interval.
• Other generic, but identifiable events that fire at configurable intervals.

This could serve multiple purposes. It would clean up old/stale carts as is the point of this issue. It would also serve and as a loose API for developers to fire off marketing related events based on the age of a cart or order.

From a UI standpoint, I'd imagine the "deleter" would be a simple checkbox and number input. Underneath, it would be a simple event trigger and subscriber.

--checkbox-- Delete old carts after --number-input-- minutes/hours/day

Then another form below this that would be an interval entity of sorts where the user could add "events" after the age of the cart is x minutes/hours/days old. The number of events should start empty, yet be unlimited and user configurable. There would be no default subscribers for these events. Developers could hook into them however they want. There should be some sort of "identifier" to an event that is configurable for developers to understand which exact event they are responding to.

Thoughts?

The comments in #73 carry over from some Slack discussions.

From a rough viewpoint, we would need a "Nudge" and "Expire" event. The config schema then becomes a sequence of items that contain an interval and type of event through. Or it's a single event with an "action" string and that string is what is configured in the UI "nudge" or "expire", naming to be bikeshed.

By default, we'd ship with the expiry solution to react on expiration settings. It does blow the scope of the issue and adds complexity. But it allows us to nail expired carts and a simple API for remarketing tools without additional hacks or fear of conflict.

I haven't had time to sit and fully think on the consequences of this. On the surface, it seems "🙌woo 🎉 cart abandonment emails and cleanup?!" but I'm not sure what dragons await us.

Ooh this sounds cool...

With a little work, the Scheduled Message module should be able to plug in here for the notification part, if this is implemented as different states of the cart. That module is set up to use a state from State Machine (haven't yet converted to core Workflow), and a date field of some kind, and lets you trigger a message based on an offset from that date along with the state -- if the state changes, message gets canceled instead of sent (e.g. if the cart converts to an order).

+++ b/modules/cart/src/Plugin/QueueWorker/CartExpiration.php
@@ -0,0 +1,98 @@
+        $order->delete();

I don't think the order should be deleted. The order status should be changed to aborted/timed-out and it should be kept for reporting purposes. Or at least it should be configurable.

I don't think the order should be deleted. The order status should be changed to aborted/timed-out and it should be kept for reporting purposes. Or at least it should be configurable.

If it's configurable then I have no issue, but we are talking about deleting old cart orders, right? I can't think of many uses for old cart orders in reports. Our needs for something like this is to lessen our databasen size so disabling the orders wouldn't help us. We have a Drupal 7 site that we're porting to Drupal 8 soon with millions of orders, many of which are cart orders. Lately they've been complaining about the order admin being unusably slow and it's surely due to the sheer amount of records in the database. We have to solve this when moving to Drupal 8 and deleting all old cart orders is a step in the right direction. We only ever allow 1 cart order (the most recent one) per user anyway so these old cart orders do nothing except clog up the database slowing down the site.

I guess my point is that I feel deletion is the logical default here, but it could by all means be configurable out of the box if people see a need to keep old cart orders.

Since there's been a lot of discussion recently, as well as talks of introducing an event, I am bumping this issue back to "needs review", to indicate that it's not simply waiting for someone to hit the commit button.

I have gone through this issue again, reviewed the current code and all comments.

I recognize that sites have different needs.
a) Email the customer after X days
b) Delete or mark as expired the order after Y days
However, a) and b) are not the same feature. Sites that email customers will still want to clear their old orders if they fail to respond within a given time period. So let's leave emails to Commerce Abandoned Carts.

It is tempting to try and make this issue solve all use cases, but that opens additional UI questions. Meanwhile, our requirements are simple and repeating across projects: Delete old carts so that they don't clog the database. That's why I've decided to proceed with this issue as-is. This feature is off by default. Sites that wish to retain carts and change their state are free to write custom code for that (using this feature as a template), and keep the deletion disabled. Sites that wish to send the cart details to an external system before deletion can respond to OrderEvents::ORDER_PREDELETE (or hook_entity_predelete).

Attaching a rerolled, cleaned up patch. Moved the commerce_cron_logic to a commerce_cart.cron service, to allow it to be swapped out if needed.

Committed!

Big thank you to rthornton for getting this started, to flocondetoile and mglaman for doing most of the work needed to complete the work, and everyone else who pitched in with improvements, rerolls, testing.

@bojanz:

+++ b/modules/cart/src/Cron.php
@@ -0,0 +1,99 @@
+      ->range(0, 50)

Why limiting to 50?

You're right, it's too conservative. Raised it so that we get up to 250 ids, chunk them into groups of 50, then queue them for processing. Let's see how that treats us.

EDIT: Pushed another commit to add the commerce_cart_expiration tag to the entity query, so people can use hook_query_TAG_alter() to lower/increase the number of processed items.