Use queue for sending emails

I wonder if it's worth incorporating: https://www.drupal.org/project/queue_unique →

@nicxvan
Thanks for taking a look!

It looks like that Queue Unique requires a manual intervention in settings.php.

Anyway, I think double queuing is prevented. Because as soon as a mail for an order gets queued, the order gets flagged as queued in the "commerce_abandoned_carts" table. And when selecting abandoned carts, orders that have already been queued are filtered out.

Code that flags an order as queued, from \Drupal\commerce_abandoned_carts\AbandonedCarts::queueSendMails():

// If not in test mode, flag that for this order a mail has been queued.
if (!$this->config->get('testmode')) {
  $this->connection->merge('commerce_abandoned_carts')
    ->keys(['order_id' => $record->order_id])
    ->fields([
      'order_id' => $record->order_id,
      'status' => static::QUEUED,
      'timestamp' => $this->time->getRequestTime(),
    ])
    ->execute();
}

Code that makes sure that already queued orders are not selected, from \Drupal\commerce_abandoned_carts\AbandonedCarts::getAbandonedOrders():

$select->leftJoin('commerce_abandoned_carts', 'a', 'o.order_id = a.order_id');
  // (...)
  ->isNull('a.status')

So the only way double queuing could theoretically happen (when not in test mode):

• A module that somehow can act on a merge query that gets executed - but generates a fatal error, causing the query to not get executed.
• The server gets shut down exactly at the time that the queue item got created, but the merge query did not get executed yet (extremely unlikely).

Double queuing does happen when operating in test mode, but I think that's correct. Because you are still testing then what happens.

@megachriz

The module looks much better with this patch (it was like the D7 module before the changes)!
But at the same time, several things are missing:

1. Support for PHP 8 features (it would be great to add it now because the entire module is almost the commerce_abandoned_carts_cron() function)
2. The mail_abandoned_cart log template was introduced but never used
3. In getAbandonedOrders(), it would be better to use "o.type <> recurring" to detect commerce_recurring, so we don't need to join the commerce_order_item table
4. In getAbandonedOrders() to determine if an order is still a cart, it is better to use "o.cart = 1"(it's more commerce 2.x way) instead of condition('o.state ', 'draft')
5. We must consider the commerce_cart[cart_expiration][number] setting if set, i.e. history_limit must be less than commerce_cart[cart_expiration][number] otherwise all carts can be deleted before sending emails (don't forget about multiple order types?)
6. Probably consider using the advancedqueue module (maybe at least when it's installed?). It supports allow_duplicates and max_retries, seems to be what we need for this module (we added core queue and advancedqueue support to this module https://www.drupal.org/project/commerce_email → )

Also, the patch from MR works fine!

@vmarchuk
Thanks for your review! Would it be a good idea to implement your suggestions in follow-ups? I think that the improvements that you suggest would also need automated tests.

The plan for Commerce Abandoned Carts that I have in mind is the following:

1. Make the current 2.0.x Drupal 11 compatible : 📌 Automated Drupal 11 compatibility fixes for commerce_abandoned_carts Needs review (needs manual testing on Drupal 11)
2. Release 2.0.1
3. Make the code from this issue Drupal 11 compatible (and manual test it on Drupal 11).
4. Commit it to 2.1.x.
5. Create a dev release for 2.1.x.
6. Follow-ups for your suggested improvements

Given my workload, I expect it does take me a few weeks to reach the last step.

Alternatively, I could merge the changes from this issue to 2.1.x now and make it Drupal 11 compatible later. If you would like to help with the improvements that you suggest now.

Let me know what you think about it!

By the way, advancedqueue is an interesting one. I would also like to know more about it for the Feeds module, specifically for this issue: 📌 Add DelayedRequeueException feature to feeds queue - D9.1 Active . Feeds is my #1 priority module on drupal.org.

Based on an other discussion I had with @nicxvan recently I was also thinking about if instead of committing this to 2.1.x if it would be better to commit it to 3.0.x instead. Because of the large amount of changes. I'm not sure if there are things in there that you could call BC breaks however. Perhaps only that the setting for a custom from address was removed.

Yeah this is a tough one.

If there is a breaking change it should definitely be a new major, I didn't see anything specifically breaking.
You can always do a minor and not mark it as supported yet until you get see some usage on it.

I didn't see any handling for removing the config you are removing though.

This is the config being removed:

Instead of setting a custom from address, the from address from the Store gets used instead.

It is a thing I'm not completely sure about if it is a good idea. I made that change a few years ago and I think I had a good reason for it at the time to remove the setting. Perhaps to reduce complexity?

In ✨ Use email settings from store for sending mails Closed: duplicate I proposed to remove the from_email and from_name settings.

Sorry, I didn't mean not to remove those, but usually if you remove config you have an update hook to remove the settings from config on sites that had that configuration right?

I did testing updating from CAC 2.0.0 to the code from this branch and I saw that the config automatically got updated. So I think that there's no need to write an update function for removing config for a removed config property. Unless I made an error while testing this.

Yeah I think that looks good!

I think core has more stringent requirements, but those probably don't make sense here.

I think this looks fine as a minor release from the outside looking in.

I just released version 2.0.1 → ! That one has Drupal 11 support, but does not include the improvements from this issue.

A dev release for 2.1.x has been made. Let's switch version and then see if we can merge this to it. Then create follow-ups for Drupal 11 support and improvements suggested by @vmarchuk.

Changes are merged, but I leave this issue open until followups are created for suggestions made in #9.

I already opened 📌 Add Drupal 11 support for 2.1.x branch Needs review . And I already merged the changes provided there to get the branch tests fixed. But still needs manual testing.

Hello @megachriz,

I eventually got a fix for options 1-3 from a comment ✨ Use queue for sending emails Needs review . I don't think 4-5 are really needed now and can be done later if possible.
Do you think I can open a new branch in this issue and add a fix to it, or is it better to create a new issue?

Thanks.

@vmarchuk
Cool. I think that a new issue for each of them would be preferable. This way we could focus on one thing at a time. Is it is easy to separate them out? (Note: sometimes I don't separate out work because I changed so much that it is harder to see which exact lines of code belongs to what, as has happened in this issue.)

@megachriz
OK, thanks for the reply. I will create at least a few separate issues to address all the issues from the comment ✨ Use queue for sending emails Needs review .