Provide a way to suppress the receipt email when admins place orders

Reviving this, let's create an MR, we also need tests.

Let's unset the data flag to make it more like a per request flag instead of a global flag that wouldn't allow resending the order receipt manually.
We can unset the flag from the order receipt subscriber.
I'm also wondering if we should update the order type method to check for the order flag as well, but the method doesn't accept an order, so that would be a breaking change.

One more note here because something similar came up in one of the projects I've been working on recently. In their case, some orders would be created programmatically, and in such a case, all following messaging is supposed to be suppressed. So in that case, the flag would have to be permanent.

So my proposal here is to change from using Boolean values to have a helpful CONST defined on the MailerHelper and have the possibility to set skip_order_receipt to PERMANENT, and in such a case, do not reset the value. We could also use that value to hide the action link. Furthermore commerce_shipping could take advantage and suppress shipment confirmation if needed (TBD).

I'm not particularly in favor of making the flag permanent, as you'd never be able to send the email from the UI. Plus you might need to use the order receipt resend form to send the order to another email (once that feature lands, See ✨ Allow specifying an email to send the order receipt to Active ).

If you're explicitly opening the form to resend a receipt, we shouldn't prevent the user from doing that. By MailerHelper are you referring to the OrderReceiptMail service?

Also if we make the flag a non boolean and it can support more than 2 states, we should probably make it an enum.

But maybe we'd need a more global flag that'd be respected by commerce_shipping too for the shipment confirmation (instead of having a separate flag for this).

So in this case, we'd be more looking at a flag that can be set for programmatically placed orders I suppose, rather than what we're trying to solve here (i.e. the place transition applied from the UI);

So... I've been thinking about this further, and wondering if it isn't cleaner to update the shouldSendReceipt() method to accept an order.

From there, we could either dispatch an event... Or just check the order flag.
It'd then be possible (just like today actually) to swap the OrderType class to add additional logic to allow for the use case described by zaporylie to disable the order receipt permanently for a given order.

It isn't possible today actually by just swapping the class, since the order isn't passed.

If we were to support persistent receipt notification suppression, I think the action link must be hidden and the checkbox in the transition form, at the very least, disabled. While I don't want to bring another field to the order base table, I think that would actually be the cleanest approach. notify - tinyint with enum-based values could do the job:
SUPPRESS_PERMANENTLY = -1
SUPPRESS_TEMPORARILY = 0
ALLOW = 1 (default)

We could also just go for a Boolean field type, given that SUPPRESS_TEMPORARILY would possibly never have to be stored in the database (changed to ALLOW on Order::preSave), but I don't love the fact that it would likely mean we still have to use int for setter/getter and then process it to bool on preSave (-1 => 0, 0 => 1, 1 => 1). and again on OrderStorage::preLoad (0 => -1, 1 => 1). But given tinyint and bool are of the same storage size using signed tinyint would just be cleaner.

I was actually exploring the enum route:

enum OrderReceiptOption: string {
    case SEND = 'send';              // default
    case SKIP_ONCE = 'skip_once';    // suppress just this time
    case SKIP_ALWAYS = 'skip_always';// permanently suppress for this order
}

And still live go the order data:
Additionally, I think I want to pass the order to OrderType::shouldSendReceipt(), I'll post a patch soon.

Opened an MR. We have tests coverage for the order receipt subscriber logic running on the place post transition, and added basic test coverage for the added extra methods from the OrderTest kernel test.

The SKIP_ONCE case shouldn't be persistent, so had to figure out how to handle this properly, via a class variable used for holding the "current" in memory receipt policy.

Attaching a screenshot of the checkbox that is added from the state transition modal:

Also, the order receipt resend operation / form isn't accessible in case SKIP_ALWAYS is set.

SKIP_ONCE and SKIP_ALWAYS could be renamed to SKIP_REQUEST and SKIP_PERMANENT perhaps?

@rszrama, @zaporylie: Any opinion about the naming?

Cool, I really like the implementation! I think it establishes a good pattern for others to follow when customizing their stores, too. We'll need to document it. 😅

So these are our current OrderReceipt policies:

/**
 * Enum for supported order receipt policies.
 */
enum OrderReceiptPolicy: string {
  // The order receipt should be sent normally (default policy).
  case Send = 'send';

  // Skip sending the order receipt only for this request.
  // Used for temporary overrides, e.g., admin checkbox on "place".
  case SkipOnce = 'skip_once';

  // Forbid sending an order receipt for this order.
  // Persistent across requests.
  case Forbid = 'forbid';
}

Perhaps we should rename "Send" to "AllowOnce" and not make it persistent and have only Forbid persist?

I wonder if we need a "Default" policy that indicates the order type setting are going to be respected instead?

These are hopefully our final policies:

<?php

namespace Drupal\commerce_order;

/**
 * Enum for supported order receipt policies.
 *
 * This allows overriding the default behavior when checking whether the order
 * receipt should be sent for a given order.
 */
enum OrderReceiptPolicy: string {
  // Respects the order type setting.
  // This is functionally equivalent to not having a receipt policy override.
  case Default = 'default';

  // The order receipt should be sent only for this request.
  // Used for temporary overrides, e.g., admin checkbox on "place".
  case AllowOnce = 'allow_once';

  // Skips sending the order receipt only for this request.
  // Used for temporary overrides, e.g., admin checkbox on "place".
  case SkipOnce = 'skip_once';

  // Forbids sending an order receipt for this order permanently.
  // Persistent across requests.
  case Forbid = 'forbid';
}

I was discussing this on Slack with @zaporylie, and we're wondering if we shouldn't make this more generic to apply also to the Shipment confirmation.

With that said, the "transient" policies (AllowOnce, SkipOnce) are transients and can't really be used for the shipments. But I guess the Forbid policy could be somehow carried on if we made the OrderReceiptPolicy an OrderCommunicationPolicy instead?

OrderCommunicationPolicy might imply it can be used for SMS communication as well for example? Which isn't a bad thing.

The risk with going with OrderReceiptPolicy is the need to implement duplicated similar logic elsewhere...

> We could potentially skip "Default", and rename OrderReceiptPolicy to OrderReceiptPolicyOverride? Or even stick with OrderReceiptPolicy but just omit the default.

I think it's better to skip Default or use it as default instead of NULL.

> With that said, the "transient" policies (AllowOnce, SkipOnce) are transients and can't really be used for the shipments. But I guess the Forbid policy could be somehow carried on if we made the OrderReceiptPolicy an OrderCommunicationPolicy instead?

After going through the code, I am leaning towards defining CommunicationPolicyInterface/Trait for addressing this. I don't see any benefit in particular to keep it order-oriented.

Re MR - I left some in-line comments. Major issue IMHO is the change to the interface method's signature, as that breaks the contract. Therefore I would prefer if we keep the order-type level method and fallback to that when policy returns null, or default in case we decide to keep it.

I don't want to mess up the MR that is now fully functional so I'll branch out and test some refactoring there.

Re MR - I left some in-line comments. Major issue IMHO is the change to the interface method's signature, as that breaks the contract. Therefore I would prefer if we keep the order-type level method and fallback to that when policy returns null, or default in case we decide to keep it.

Yes it does break the contract. However, the likelyhood of someone decorating the order type is really low. I didn't want to touch this method at first for that reason, what I dislike if we don't change the method is we now need to copy the same code potentially in multiple places and we don't have a single entry point for determining whether the receipt should be sent for an order.

After going through the code, I am leaning towards defining CommunicationPolicyInterface/Trait for addressing this. I don't see any benefit in particular to keep it order-oriented.

Then we need to discuss this further, and we definitely won't be including this change in the next release. There is definitely a risk in scope creeping here which will delay this change further.

It now sounds weird to have an "AllowOnce" communication policy (at least to me).

> I didn't want to touch this method at first for that reason, what I dislike if we don't change the method is we now need to copy the same code potentially in multiple places and we don't have a single entry point for determining whether the receipt should be sent for an order.

I think this calls for a method on the Order class itself. I would leave it as is for now until the need for reusing logic in other places emerges.

> CommunicationPolicyInterface sounds really really generic. We only control the order receipt atm.

It does, and given that we only use it in the receipt-aware scenarios, I unified it to ReceiptPolicy (ReceiptPolicyTrait/SupportsReceiptPolicy).

I see the benefit of the trait / interface if the methods / code is reusable. It is not the case here. I think the trait + interface would make sense in case we come up with code that is generic enough to be reused by other entity types or emails (such as the shipment confirmation for example, or if we extend it to any kind of order communication).

Otherwise, I really don't see the benefit of the interface if the only entity implementing it is the order?

So perhaps we should just remove the "Default" policy, and skip the order type signature change (haven't looked at the suggested approach for the alternative suggested approach).

But I think the extra interface / trait doesn't make sense once again unless it is reusable by other entity types.