Is it possible to resend the order confirmation email?

CommentFileSizeAuthor
#81 2912598-81.patch26.28 KBsimgui8
#78 2912598-78.patch26.22 KBlisastreeter
#77 interdiff-2912598-71-76.txt9.47 KBlisastreeter
#77 2912598-76.patch26.14 KBlisastreeter
#71 2912598-71.patch26.1 KBlisastreeter
#69 2912598-69.patch31.06 KBsimgui8
#64 interdiff-2912598-63-64.txt29.17 KBlisastreeter
#64 2912598-64.patch31.45 KBlisastreeter
#63 2912598-63.patch21.44 KBsimgui8
#61 2912598-61.patch21.46 KBsimgui8
#58 2912598-58.patch20.94 KBsimgui8
#55 2912598-55.patch20.88 KBsimgui8
#51 2912598-51.patch20.58 KBsimgui8
#44 2912598-44.patch22.36 KBsimgui8
#39 2912598-39.patch8.33 KBmattjones86
#39 2912598-39.patch0 bytesmattjones86
#38 2912598-38.patch19.85 KBmattjones86
#37 2912598-37.patch18.15 KBmattjones86
#31 interdiff-2912598-28-31.txt1.08 KBrakesh.gectcr
#31 2912598-31.patch16.89 KBrakesh.gectcr
#28 interdiff-2912598-21-28.txt10.17 KBrakesh.gectcr
#28 2912598-28.patch16.25 KBrakesh.gectcr
#21 resend_confirmation_mail-2912598-21-interdiff.txt2.35 KBsorabh.v6
#21 resend_confirmation_mail-2912598-21.patch18.5 KBsorabh.v6
#18 resend_confirmation_mail-2912598-18.patch15.99 KBlammensj
#15 resend_confirmation_mail-2912598-15.patch15.91 KBsorabh.v6
#15 resend_confirmation_mail-2912598-15-interdiff.txt15.48 KBsorabh.v6
#12 resend_confirmation_mail-2912598-12.patch9.86 KBsorabh.v6
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Hygglo created an issue. See original summary.

agoradesign’s picture

I'm not 100% sure, as it may have been added shortly and I haven't seen it so far - but I'm quite sure, that the answer is: no

But the request makes absolutely sense to me. I'd vote for introducing this, it shouldn't be difficult to achieve. I could imagine that this is already on the feature list of @bojanz, but maybe not having the highest priority atm

agoradesign’s picture

In the meantime, you could help yourself and implement this in a custom module. All you have to do is to look at the OrderReceiptSubscriber class and duplicate that logic into a custom controller

sorabh.v6’s picture

Assigned: Unassigned » sorabh.v6
mglaman’s picture

sorabh.v6 please detail your thoughts of the work you plan to do, there was no discussion on implementation. I want to avoid wasted efforts. We need to figure out what is the best implementation.

Is it an order operation you can only access via dropbutton on orders, or tab when looking at the order?

Is it a button on the order view page, like transitions?

sorabh.v6’s picture

Hi, @mglaman I am planning to create order operation that we can add in User orders view.
I found the event which sends the confirmation email, its 'commerce_order.place.post_transition' and I think if I find a way to trigger the same event using a menu link, then I can resend the confirmation email using the same event subscriber class. Do I have to create a new event subscriber instead?

Currently, I have implemented operation link for Resend Email. But its functionality is pending because I am trying to send confirmation email using commerce_order.place.post_transition which is in OrderRecieptSubscriber.php

Only local images are allowed.

Thanks

agoradesign’s picture

@sorabh.v6

no, no, no.... please don't do that... it would be absolutely wrong to trigger the placement event again for that!

My approach would be to refactor the current event subscriber, move the logic into a service class, having a sendOrderReceipt(OrderInterface $order) function. This service must be then injected and used by the event subscriber and can be used on any other place --> so clicking your button must trigger a logic that also uses that service

mglaman’s picture

This should be an admin operation only. On the order management form for admins. Not for end users.

sorabh.v6’s picture

@mglaman I may be wrong but what would happen if I bought a product and missed the online receipt. If only admin has the right to resend the email, then I would need to send an email to admin first and then admin will resend the receipt. But if the end user has the right as well to resend the receipt email then it will be easy for end user.

@agoradesign Your approach is lengthy but if it's the right way to do this then I will do your way :)

Thanks

bojanz’s picture

#7 is the only way to do it.

sorabh.v6’s picture

I have started working as per #7. I will post patch soon.

sorabh.v6’s picture

Hi Guys,

I worked as per #7 and created a service and then used that service in the controller for 'resend email' operation link. I checked it, it's working on my test site. Request you to review and post feedback.

Thanks

bojanz’s picture

Status: Active » Needs work

Just a heads up that you'll want to update this for #2913595: Order receipt email has encoding issues.

You also need to make the OrderReceiptSubscriber use the service you're introducing here, and make sure the service is named generically enough (perhaps just "OrderReceipt"?)

Also, don't we want a confirmation form for this? Feels odd to send an email every time the url is hit.

sorabh.v6’s picture

@bojanz What fields I should add to Confirmation form?

sorabh.v6’s picture

Assigned: sorabh.v6 » Unassigned
Status: Needs work » Needs review
FileSize
15.48 KB
15.91 KB

Work as per #13 included in the patch. Please review.

Thanks

Hygglo’s picture

I wish that the notifications for tickets worked....

I'm getting this error message:

Warning: Missing argument 1 for Drupal\Core\Entity\ContentEntityBase::access(), called in /modules/contrib/commerce/modules/order/src/OrderListBuilder.php on line 124 and defined in Drupal\Core\Entity\ContentEntityBase->access() (line 623 of core/lib/Drupal/Core/Entity/ContentEntityBase.php).

mglaman’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests
  1. +++ b/modules/order/src/EventSubscriber/OrderReceiptSubscriber.php
    @@ -20,68 +15,20 @@ class OrderReceiptSubscriber implements EventSubscriberInterface {
    +   * The OrderReceipt object.
    

    The order receipt service.

  2. +++ b/modules/order/src/EventSubscriber/OrderReceiptSubscriber.php
    @@ -20,68 +15,20 @@ class OrderReceiptSubscriber implements EventSubscriberInterface {
    +   * @param \Drupal\commerce_order\OrderReceipt $order_receipt_resend
    ...
    +  public function __construct(OrderReceipt $order_receipt_resend) {
    +    $this->orderReceipt = $order_receipt_resend;
    

    $order_receipt

  3. +++ b/modules/order/src/EventSubscriber/OrderReceiptSubscriber.php
    @@ -20,68 +15,20 @@ class OrderReceiptSubscriber implements EventSubscriberInterface {
    +   *   Object of OrderReceipt class.
    

    The order receipt service.

  4. +++ b/modules/order/src/Form/ResendReceiptForm.php
    @@ -0,0 +1,97 @@
    +   * The OrderReceipt object.
    ...
    +   *   Object of OrderReceipt class.
    

    The order receipt service.

  5. +++ b/modules/order/src/Form/ResendReceiptForm.php
    @@ -0,0 +1,97 @@
    +      drupal_set_message(t('Email confirmation resent successfully.'));
    ...
    +      drupal_set_message(t('Email confirmation could not be resent, please contact site administrator.'));
    

    $this->t()

  6. +++ b/modules/order/src/OrderReceipt.php
    @@ -0,0 +1,146 @@
    +class OrderReceipt {
    

    We need an interface so this can be swapped. If we do not provide an interface then no one can provide a custom implementation.

We have kernel test coverage of the receipt event subscriber and that test passes (yay!) However, I feel we should have a matching Functional test to assert the links and tabs.

lammensj’s picture

Rerolled against commerce 8.2.

sorabh.v6’s picture

As per #17, we still need tests for this.

sorabh.v6’s picture

Assigned: Unassigned » sorabh.v6
sorabh.v6’s picture

Created test for resend confirmation email. I could not test this on my machine but I hope it works.

Status: Needs review » Needs work

The last submitted patch, 21: resend_confirmation_mail-2912598-21.patch, failed testing. View results

sorabh.v6’s picture

Status: Needs work » Needs review

The last submitted patch, 15: resend_confirmation_mail-2912598-15.patch, failed testing. View results

The last submitted patch, 12: resend_confirmation_mail-2912598-12.patch, failed testing. View results

alexpott’s picture

Status: Needs review » Needs work
Issue tags: -Needs tests
  1. +++ b/modules/order/src/Form/ResendReceiptForm.php
    @@ -0,0 +1,97 @@
    +   * @param \Drupal\Core\Routing\CurrentRouteMatch $current_route_match
    +   *   The current route match.
    +   */
    +  public function __construct(OrderReceipt $order_receipt_resend, CurrentRouteMatch $current_route_match) {
    ...
    +    $this->order = $current_route_match->getParameter('commerce_order');
    ...
    +      $container->get('current_route_match')
    ...
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function buildForm(array $form, FormStateInterface $form_state) {
    

    We don't need to pass in the order here. It will be passed into the buildForm function as part of the upcasting process. Just do something like:

    +  public function buildForm(array $form, FormStateInterface $form_state, OrderInterface $commerce_order = NULL) {
    +    $this->order = $commerce_order;
    
  2. +++ b/modules/order/src/Form/ResendReceiptForm.php
    @@ -0,0 +1,97 @@
    +      drupal_set_message(t('Email confirmation resent successfully.'));
    ...
    +      drupal_set_message(t('Email confirmation could not be resent, please contact site administrator.'));
    

    $this->t() is preferred over t().

  3. +++ b/modules/order/src/OrderListBuilder.php
    @@ -106,7 +106,6 @@ class OrderListBuilder extends EntityListBuilder {
    -
    

    This is an out-of-scope change.

  4. +++ b/modules/order/src/OrderReceipt.php
    @@ -0,0 +1,146 @@
    +   * @param \Drupal\commerce_order\Entity\OrderInterface $order
    +   *   The order we resend receipt for.
    

    The order to send receipt for. "we" don't do much :)

  5. +++ b/modules/order/src/OrderReceipt.php
    @@ -0,0 +1,146 @@
    +   * @return bool
    +   *   Returns Boolean values.
    

    Returns TRUE if the mail sent successfully, FALSE if not. is better.

  6. +++ b/modules/order/src/OrderReceipt.php
    @@ -0,0 +1,146 @@
    +    if ($this->mailManager->mail('commerce_order', 'receipt', $to, $langcode, $params)) {
    +      return TRUE;
    +    }
    +    else {
    +      return FALSE;
    +    }
    

    Looking at MailManager::mail() it does 't work this way. You need to do something like:

        $message = $this->mailManager->mail('commerce_order', 'receipt', $to, $langcode, $params);
        return (bool) $message['result'];
    

    Relatedly core makes the same mistake - see #2933916: EmailAction incorrectly uses the result of $this->mailManager->mail()

  7. +++ b/modules/order/tests/src/Functional/ResendOrderReceiptTest.php
    @@ -0,0 +1,79 @@
    +  use JavascriptTestTrait, AssertMailTrait;
    

    The JavascriptTestTrait is not necessary for this test.

  8. +++ b/modules/order/tests/src/Functional/ResendOrderReceiptTest.php
    @@ -0,0 +1,79 @@
    +    $order->save();
    

    After this let's just make sure that saving has not sent a receipt by doing:

    +    // Ensure that saving the order has not sent the receipt.
    +    $mails = $this->getMails();
    +    $this->assertEquals(0, count($mails));
    
alexpott’s picture

+++ b/modules/order/tests/src/Functional/ResendOrderReceiptTest.php
@@ -0,0 +1,79 @@
+    $this->drupalGet($order->toUrl('resend-receipt'));

Instead of just going straight to the resend form it'd be great if we visited the listing page to click on the link there so we test that code too.

rakesh.gectcr’s picture

Status: Needs work » Needs review
FileSize
16.25 KB
10.17 KB

[#26] is done.
To do, WIP [#27]

alexpott’s picture

+++ a/modules/order/src/OrderListBuilder.php
@@ -121,13 +122,6 @@
-    if ($entity->access('update') && $entity->hasLinkTemplate('resend-receipt')) {
-      $operations['resend_email'] = [
-        'title' => $this->t('Resend Email'),
-        'weight' => 20,
-        'url' => $entity->toUrl('resend-receipt'),
-      ];
-    }

This wasn't supposed to be removed from the patch -
afaics. Still nice proof it's not tested.

Plus the new OrderReceipt object is missing. A neat trick to do is to run the new test before uploading to check that the patch at least works.

Status: Needs review » Needs work

The last submitted patch, 28: 2912598-28.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

rakesh.gectcr’s picture

Status: Needs work » Needs review
FileSize
16.89 KB
1.08 KB

test is getting passed at my local

Status: Needs review » Needs work

The last submitted patch, 31: 2912598-31.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

rakesh.gectcr’s picture

Working on it

alexpott’s picture

The fail in #31 is a random fail in commerce. Fixed by #2930209: Random fail in PaymentMethodStorageTest.

alexpott’s picture

  1. +++ b/modules/order/commerce_order.routing.yml
    @@ -45,3 +45,15 @@ entity.commerce_order.user_view:
    +  requirements:
    +    _permission: 'administer commerce_order'
    

    I think that we should have an access control function here and deny access if the following conditions are met - $order_type->shouldSendReceipt() returns FALSE or $order->getEmail() returns FALSE. Thereby we won't display the link or form unnecessarily.

  2. +++ b/modules/order/src/OrderReceipt.php
    @@ -87,12 +85,13 @@ class OrderReceiptSubscriber implements EventSubscriberInterface {
    +  public function sendOrderReceipt(OrderInterface $order) {
    

    There are 2 returns in this method that return nothing - they should return FALSE.

  3. Still need to test the link on the order list
postback’s picture

Applying this patch, which works fine, I realised something else: it seems the e-mail sent out picks up the site's default language.

The sendOrderReceipt function calls the $customer->getPreferredLangcode() and that's passed along into the $this->mailManager->mail() call, but not into the $this->renderer->executeInRenderContext() call. So, the HTML being rendered, used for the mail's body, isn't in the customer's preferred language.

Would this be a matter of moving the setting of the $langcode variable higher up the sendOrderReceipt() function and adding an additional #langcode element to the $build array?

mattjones86’s picture

This wouldn't apply for me on 2.7. New patch attached.

mattjones86’s picture

Actually I see there is a bug where it renders with the admin theme rather than the default frontend theme... this new patch should resolve that too.

EDIT: I guess the tests fail because they have no active theme?

mattjones86’s picture

Status: Needs work » Needs review
FileSize
0 bytes
8.33 KB

Status: Needs review » Needs work

The last submitted patch, 39: 2912598-39.patch, failed testing. View results

simgui8’s picture

#37 and #38 apply successfully for me on 2.7

mattjones86’s picture

Yeah please ignore #39... I shouldn't have posted that. If only I could delete it!

s.messaris’s picture

I came upon the problem mentioned in #38 and have opened an issue in https://www.drupal.org/project/commerce/issues/2984098, where I have a working patch. I would love some feedback on that. Maybe my approach could help here as well?

simgui8’s picture

This patch resend the confirmation email this way :

if user is known (except anonymous)
-in the user preffered language
else
-the admin current language

agoradesign’s picture

Did a quick look at patches #38 and #44:

  • The stuff added to the OrderListBuilder in #44 is not needed. The language to send the e-mail is chosen within OrderReceipt class anyway, and ther is already the preferred language considered.
  • I'm unsure about needing to care about what theme to choose. I guess that no store owner wants to send plain text e-mails and therefore is relying on swiftmailer module (on top of mailsystem), where you can already configure the theme to use for your e-mails. This would rather interfere with that, if you'll have the crazy idea choose an extra theme for e-mails in general e.g..
  • If we nevertheless decide that we want to keep that theme changing logic explained above, you're calling \Drupal:config() instead of injecting the service.
  • In ResendReceiptForm, line 63, t() instead of $this->t() is used
  • food for thought: regarding the route requirements, shouldn't we rather orientate on the pattern used for unlock, where the entity permission is asked?
    It's a matter of taste and commerce_order isn't consistent here -> reassign is using also the administer commerce_order permission here like proposed here
simgui8’s picture

The stuff added to the OrderListBuilder in #44 is not needed. The language to send the e-mail is chosen within OrderReceipt class anyway, and ther is already the preferred language considered.

Just retested #38 with different order assigned to clients with different preferred languages
The language to send the e-mail is the admin preferred language.

Shouldn't it be the client preferred language?

agoradesign’s picture

Sure? Because this is what's inside the patch:

+    // Replicated logic from EmailAction and contact's MailHandler.
+    if ($customer = $order->getCustomer()) {
+      $langcode = $customer->getPreferredLangcode();
+    }
+    else {
+      $langcode = $this->languageManager->getDefaultLanguage()->getId();
+    }

And that looks good imho

simgui8’s picture

With #38, I have followed $langcode until doMail from MailManager.php
$langcode is always the expected value (the client preferred language) from:

+    // Replicated logic from EmailAction and contact's MailHandler.
+    if ($customer = $order->getCustomer()) {
+      $langcode = $customer->getPreferredLangcode();
+    }
+    else {
+      $langcode = $this->languageManager->getDefaultLanguage()->getId();
+    }

But I still receive the email in the current page language.

When using url
/admin/commerce/orders/1206/resend
The language of the page and email is the admin preferred language

When using french url
/fr/admin/commerce/orders/1206/resend
The language of the page and email is french

When using english url
/en/admin/commerce/orders/1206/resend
The language of the page and email is english

Same result without using swiftmail.

That's why I have replaced $entity->toUrl with Url::fromRoute with $language in #44

I am the only one in this situation?

agoradesign’s picture

I haven't tested theses cases yet, but I guess, that the translations don't respect the language parameter from the e-mail manager. So they'll always use the current language!?!?

simgui8’s picture

After further testing: the behavior in #48 seems expected.

From MailManager::mail doc

Finding out what language to send the email with needs some consideration. If you send email to a user, her preferred language should be fine, so use \Drupal\Core\Session\AccountInterface::getPreferredAdminLangcode(). If you send email based on form values filled on the page, there are two additional choices if you are not sending the email to a user on the site. You can either use the language used to generate the page or the site default language. See Drupal\Core\Language\LanguageManagerInterface::getDefaultLanguage(). The former is good if sending email to the person filling the form, the later is good if you send email to an address previously set up (like contact addresses in a contact form).

From my understanding, when a user creates an order

her preferred language should be fine

When it's from a form (the actual case), it could be ether

the language used to generate the page or the site default language

simgui8’s picture

Here is a reroll of #44 using \Drupal::messenger()->addStatus with a note about customer preferred language

eli-on-drupal’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 51: 2912598-51.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

mglaman’s picture

I'm so confused

1) Drupal\Tests\commerce_order\Kernel\TimestampEventTest::testOnPlaceTransition
Drupal\Core\Entity\EntityStorageException: The theme  does not exist.

What is this patch doing that destroys the tests?

simgui8’s picture

Well, s.messaris (#43) have a patch here for theme switching theme using MailSystem's mail manager service.

It doesn't trigger the "theme does not exist." test error.

This is mostly a copy/paste of his patch over #51

eli-on-drupal’s picture

Status: Needs work » Needs review
mglaman’s picture

+++ b/modules/order/src/OrderReceipt.php
@@ -0,0 +1,197 @@
+    if (method_exists($this->mailManager, 'getMailTheme')) {
+      $mail_theme = $this->mailManager->getMailTheme();
+      if ($mail_theme && $mail_theme != $current_active_theme->getName()) {
+        $this->themeManager->setActiveTheme($this->themeInitialization->initTheme($mail_theme));
+      }
+    }
...
+    finally {
+      // Revert the active theme, this is done inside a finally block so it is
+      // executed even if an exception is thrown during theming the mail.
+      if ($mail_theme && $mail_theme != $current_active_theme->getName()) {
+        $this->themeManager->setActiveTheme($current_active_theme);
+      }
+    }

There's no interdiff, but I'm assuming this is the magic?

simgui8’s picture

This is a reroll for 2.10 (dev 2018-10-17)

agoradesign’s picture

I've tried the patch from #58. Works for me

agoradesign’s picture

PS: I'd add a dedicated permission for this - allowing the administer permission just for allowing the resend is a bit too much in some cases

simgui8’s picture

Added 'Resend order confirmation email' permission.

Status: Needs review » Needs work

The last submitted patch, 61: 2912598-61.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

simgui8’s picture

Should fix previous fail (test was missing new permission)

lisastreeter’s picture

I've reviewed the code in the latest patch and made some changes based on all the comments and existing design patterns for the order "unlock" functionality. I also added test coverage.

  • I changed the resend confirmation form to use ContentEntityConfirmFormBase as its base class.
  • I shortened the confirmation message to not mention the email language. It seems like that's better left for documentation.
  • I added a form handler to Order for the resend form so that it can be easily swapped out in custom code.
  • Since the resend option should not be available for cart/draft orders, I switched the form routing from checking for a permission to checking for entity access for "resend_receipt". The access control handler checks that the order has been placed and that the user has the permission.
  • I added an interface for the EmailReceipt service class so that it can be easily swapped out in custom code.
  • For resending email, we should not check $order_type->shouldSendReceipt(). Also, I don't think the automatic bcc field is necessary for resending email. I refactored this code out of the EmailReceipt service class and into the order receipt EventSubscriber.
  • I also added some optional parameters for the `sendOrderReceipt()` method in the EmailReceipt service.
    • key allows you to set the key string that's passed into the MailManager mail() method so that automatic vs. resend order receipt emails can be identified when using hook_mail_alter() in custom code
    • bcc allows you to specify a bcc string for the email, so that you can include bcc for the automatic emails but not for the resend emails.
    • build_overrides has no direct function for the functionality provided by this patch, but I think it improves the DX by allowing you to override elements of the $build array that's used to render the body of the email. For example, you could override the theme and provide additional data to be passed to the theme.
  • I merged the ResendOrderReceiptTest functional test into the existing OrderAdminTest functional test.
  • I added test coverage in the OrderAccessControlHandlerTest kernel test.
  • I added test coverage in the OrderReceiptTest kernel test, specific to the OrderReceipt service.
lisastreeter’s picture

Status: Needs work » Needs review
simgui8’s picture

Thank you Lisa

After trying patch from #64

Mail fail sending :
when sending to customer with non-english preferred language :

Error sending email ... with reply-to undefined

Also note that when the receipt is sent successfully (english preferred language)
the receipt is themed in the admin preferred language.
as before #44 (more info in #45 to #48)

bojanz’s picture

Status: Needs review » Needs work

This issue is trying to do too many things at once.
We need to fix the language bugs in another issue (#2984098: Order receipt themed differently for orders entered in admin?), before introducing the resend functionality.

The last-minute introduction of a permission also feels like something that should not be done in this issue.
Do we want a permission for each operation that the merchant might make?
Is there a use case for a merchant who doesn't have access to edit an order (and modify its state), but still needs to resend the receipt?

In general, Lisa's improvements are going in the right direction, things look good at a glance.

+entity.commerce_order.resend_receipt_form:
+  path: '/admin/commerce/orders/{commerce_order}/resend-receipt'
+  defaults:
+    _entity_form: 'commerce_order.resend-receipt'
+  requirements:
+    _entity_access: 'commerce_order.resend_receipt'

We already have an OrderRouteProvider, we can generate this route from there (especially since we already have a link template defined).

+    if ($entity->access('resend_receipt') && $entity->hasLinkTemplate('resend-receipt-form')) {
+      $language = [];
+      $user = User::load($entity->get('uid')->target_id);
+      if ($user && !$user->isAnonymous()) {
+        $language = ['language' => \Drupal::languageManager()->getLanguage($user->getPreferredLangcode())];
+      }
+      $operations['resend_receipt'] = [
+        'title' => $this->t('Email receipt'),
+        'weight' => 20,
+        'url' => $entity->toUrl('resend-receipt-form'),
+      ];
+    }

Language is defined but not used. A user is loaded manually even though orders have a getCustomer() method.

simgui8’s picture

Language is a leftover from #63
The idea was to pass language to the confirmation form with 'url' => Url::fromRoute
This was working only when resending receipt.

In every attempt I have made from commerce or custom module, whatever langcode passed to MailManager had no effect.

No success trying to move the language bugs to #2984098 using :
-languageManager()->setConfigOverrideLanguage($language)
-or a custom language Negociator

At this point, since trying to fix MailManager langcode seems counter intuitive, and because {% trans with {'langcode': 'xx'} %} will work fine...

maybe doing nothing and focusing on lisastreeter improvements is the way to go!

any thoughts?

simgui8’s picture

Here is a reroll or #64

The language leftover from #63 is removed.
There is no need to investigate here the reason why whatever langcode passed to MailManager has no effect.
Fresh multilingual Drupal install with default config have the same problem.
Plus, there is a workaround passing language to template.

The last-minute introduction of a permission

is removed

We already have an OrderRouteProvider, we can generate this route from there (especially since we already have a link template defined).

Maybe someone can pitch in to fix this part (did not find my way through it).

bojanz’s picture

lisastreeter’s picture

This patch uses the new commerce mail service for email resend. A single service is used for both the receipt sent by the order-event-subscriber and for resends. Also, an attempt to use OrderRouteProvider instead of routing.yml. Seems to be working and tests are passing locally.

lisastreeter’s picture

Status: Needs work » Needs review
simgui8’s picture

Tested on multilingual setup.

#71 works great!

Thank you lisa! for your work (and bojanz/mglaman/berdir/smessaris/everyone)

bojanz’s picture

Status: Needs review » Needs work

I love how much smaller this patch has gotten over the initial attempts.

+  /**
+   * Sends an order receipt email.
+   *
+   * @param \Drupal\commerce_order\Entity\OrderInterface $order
+   *   The order to resend receipt for.
+   * @param string $key
+   *   A key to identify the email sent.
+   * @param string $bcc
+   *   The email address or addresses for the email Bcc header.
+   * @param array $build_overrides
+   *   Override values for the structured array describing the data to be rendered.
+   *
+   * @return bool
+   *   Returns TRUE if the mail sent successfully, FALSE if not.
+   */
+  public function send(OrderInterface $order, $key = 'order_receipt', $bcc = '', $build_overrides = []) {
+    $to = $order->getEmail();

1. The method is already documented on the interface, you need inheritdoc here.
2. I am not sure it makes sense for the method to accept $key, it means that the mail ID is different between the initial send and the resend, and an alter hook might only alter one of them.
3. $build_overrides is documented, passed, but then never used. Probably a leftover from an earlier patch.
4. I would explicitly pass $to, before $bcc, to allow people to send the order receipt email to another address if desired (the store or whatever).

+    // Do not set the Bcc header if there is no bcc parameter.
+    if (!empty($bcc)) {
+      $params['bcc'] = $bcc;
+    }

5. This is also a leftover, the latest code had bcc directly in params, the if is not needed.

+  public function getRoutes(EntityTypeInterface $entity_type) {
+  	$collection = parent::getRoutes($entity_type);
+
+	  if ($entity_type->hasLinkTemplate('resend-receipt-form')) {
+	    $entity_type_id = $entity_type->id();
+	    $route = new Route($entity_type->getLinkTemplate('resend-receipt-form'));
+	    $route
+	      ->addDefaults([
+	        '_entity_form' => "{$entity_type_id}.resend-receipt",
+	        '_title_callback' => '\Drupal\Core\Entity\Controller\EntityController::title',
+	      ])
+	      ->setRequirement('_entity_access', "{$entity_type_id}.resend_receipt")
+	      ->setOption('parameters', [
+	        $entity_type_id => ['type' => 'entity:' . $entity_type_id],
+	      ]);
+
+	    // Entity types with serial IDs can specify this in their route
+	    // requirements, improving the matching process.
+	    if ($this->getEntityTypeIdKeyType($entity_type) === 'integer') {
+	      $route->setRequirement($entity_type_id, '\d+');
+	    }
+
+      $collection->add("entity.{$entity_type_id}.resend_receipt_form", $route);
+    }
+
+    return $collection;
+  }

6. Code incorrectly indented, doesn't pass phpcs.
We should introduce a getResendReseiptFormRoute() helper like we do for routes elsewhere.

+  public function __construct(EntityRepositoryInterface $entity_repository, EntityTypeBundleInfoInterface $entity_type_bundle_info = NULL, TimeInterface $time = NULL, OrderReceiptMailInterface $order_receipt_mail) {

7. There is no reason for these two params to be NULL, since we are explicitly setting them. PhpStorm just complains. I know core does this, but that's cause they want to keep the params optional in inheriting classes, and we don't have that use case here.

+class OrderReceiptResendForm extends ContentEntityConfirmFormBase {
+
+  /**
+   * The order receipt mail.
+   *
+   * @var \Drupal\commerce_order\OrderReceiptMailInterface
+   */
+  protected $orderReceiptMail;

8. Interface not found, you're missing \Mail.

lisastreeter’s picture

+ // Do not set the Bcc header if there is no bcc parameter.
+ if (!empty($bcc)) {
+ $params['bcc'] = $bcc;
+ }
5. This is also a leftover, the latest code had bcc directly in params, the if is not needed.

Removing the if broke the order receipt test. Now:

$this->assertEquals('', $the_email['headers']['Bcc']); works.

But:

$this->assertEmpty(isset($the_email['headers']['Bcc'])); does not work.

I'm fine with changing the test to make it work, but I wonder whether it matters whether we send an empty 'bcc' header vs. not sending one at all.

bojanz’s picture

We should convert the isset() check in MailHandler into a !empty check, that way callers don't need to care about passing an empty value.

lisastreeter’s picture

@bojanz Thanks. Made the change in MailHandler. Test is happy now.

Patch reroll for review in comment #

lisastreeter’s picture

FileSize
26.22 KB

Another try.

agoradesign’s picture

Status: Needs work » Needs review

you forgot the status change

lisastreeter’s picture

@agoradesign Thanks! I always wait until the tests pass, first :)

simgui8’s picture

Just a little touch up to include #3031195

simgui8’s picture

After tested the following :

  • anonymous order in english
  • anonymous order in some other language
  • customer logs in during checkout
  • manually created order for an existing customer
  • resend some existing orders (customers with english preferred language)
  • resend some existing orders (customers with other than english preferred language)


Theming is good, language is good, logs are good!

agoradesign’s picture

@lisastreeter: oooh ok...now I've learned something new :) Always thought that the tests won't run until you set the status to "needs review"

bojanz’s picture

I'm going to leave this issue for the next release because I need to time to understand $build_overrides. It feels weird, but Lisa told me previously that a client project is depending on it. At minimum we need better code docs for it, with an example. The test also doesn't help cause it only overrides #order_entity, but that makes no sense, the order is already a param to the method.

bojanz’s picture

Assigned: Unassigned » bojanz
Status: Needs review » Needs work

I discussed this will Lisa, and we agreed that $build_overrides are unnecessary and can be kept out of the final commit.

Here's my final review. Assigning the issue to myself to address the points pre-commit.

In general, we need to clean up the tests. OrderReceiptTest now tests both the service and the event subscriber, which is not ideal. We need to create a Kernel/Mail/OrderReceiptMailTest which tests the service. This leaves the original OrderReceiptTest looking thin, so we can merge OrderReceiptMultilingualTest into it.

+    if ($this->orderReceiptMail->send($order, $order->getEmail())) {
+      $this->messenger()->addMessage($this->t("Order receipt resent successfully."));
+    }
+    else {
+      $this->messenger()->addWarning($this->t('Order receipt could not be resent, please contact site administrator.'));
+    }

1. I dislike doing the main action in the IF. It makes more sense to assign the return value to a $result first.
2. Ryan had a good point, that we can't know whether the sending was successfully, the mail server might accept the email, but still not send it. Or the customer's mail agent might reject it. So best to shorten it to "Order receipt resent.".
3. Found an unexpected problem. MailManager::doMail() actually sets an error message if the sending fails. That means that we don't need to set our own.

+  public function send(OrderInterface $order, $to, $bcc = '', $build_overrides = []);

4. We should allow $to to be empty, and default it to the order email. That means that OrderReceiptResendForm doesn't need to pass both $order and $order->getEmail().
5. If a parameter can be empty, it should default to NULL, not an empty string.

+   * @param string $bcc
+   *   The email address or addresses for the email Bcc header.

6. It is unclear how multiple addresses could be passed. The right answer is "comma separated", so let's document that.

+    if ($entity->access('resend_receipt') && $entity->hasLinkTemplate('resend-receipt-form')) {
+      $operations['resend_receipt'] = [
+        'title' => $this->t('Email receipt'),
+        'weight' => 20,
+        'url' => $entity->toUrl('resend-receipt-form'),
+      ];
+    }

7. The hasLinkTemplate() check is not needed, it's not done by any of the operations above this one.
8. The title should be "Resend receipt". The "Resend" language is used across systems.

+  /**
+   * Gets the resend receipt form route.
+   *
+   * @param \Drupal\Core\Entity\EntityTypeInterface $entity_type
+   *   The entity type.
+   *
+   * @return \Symfony\Component\Routing\Route|null
+   *   The generated route, if available.
+   */

9. The docblock description should be "Gets the resend-receipt-form route."

+  protected function getResendReceiptFormRoute(EntityTypeInterface $entity_type) {
+    $entity_type_id = $entity_type->id();
+    $route = new Route($entity_type->getLinkTemplate('resend-receipt-form'));
+    $route
+      ->addDefaults([
+        '_entity_form' => "commerce_order.resend-receipt",
+        '_title_callback' => '\Drupal\Core\Entity\Controller\EntityController::title',
+      ])
+      ->setRequirement('_entity_access', "commerce_order.resend_receipt")
+      ->setRequirement('commerce_order', '\d+')
+      ->setOption('parameters', [
+        'commerce_order' => ['type' => 'entity:commerce_order'],
+      ])
+      ->setOption('_admin_route', TRUE);
+
+    return $route;
+  }

10. $entity_type_id defined but never used.
11. Several strings are enclosed in double quotes, even though there's no need (no embedded variables).

+    // Resending an order email requires the same permissions as 'view' with
+    // an additional check to ensure the necessary permission.
+    elseif ($operation == 'resend_receipt') {
+      // Emails cannot be sent for draft/cart orders.
+      if (empty($entity->getPlacedTime())) {
+        return AccessResult::forbidden();
+      }

12. The docblock is incorrect, since we're no longer checking an additional permission, we're checking whether the order was placed.
13. Missing @var for $entity causes getPlacedTime() to get highlighted in PhpStorm. In general, let's check whether getState()->getId() is "draft", that's the check we use elsewhere.
14. The AccessResult needs to be cached per entity, since it depends on an entity field.

     /** @var \Drupal\commerce_order\Entity\OrderInterface $entity */
     if ($result->isNeutral() && $operation == 'view') {
-      if ($account->id() == $entity->getCustomerId()) {
+      if ($additional_operation == 'resend_receipt') {
+        $result = AccessResult::allowedIfHasPermissions($account, ['administer commerce_order']);
+      }
+      elseif ($account->id() == $entity->getCustomerId()) {

At this point the admin permission was already checked (by the parent class), so there's no need to do it again.
Instead, we want to check $account->id() == $entity->getCustomerId() && empty($additionl_operation), which ensures that "view own" people (aka customers) can't access any of the operations.

  • bojanz committed c9f047d on 8.x-2.x authored by lisastreeter
    Issue #2912598 by simgui8, lisastreeter, mattjones86, sorabh.v6, rakesh....
bojanz’s picture

Version: 8.x-2.0 » 8.x-2.x-dev
Assigned: bojanz » Unassigned
Status: Needs work » Fixed

Addressed #85 and committed. Thanks, everyone!

simgui8’s picture

This is so awesome!

Thank you bojanz for the detailed follow up and everyone who worked on this!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

petednz’s picture

thx all. found it under Operations (rhs) of the commerce/orders page

simgui8’s picture

When viewing an order, there is a resend receipt button since commerce 8.x-2.29
https://www.drupal.org/node/3261876

Orders view 'actions': probably since this issue is fixed.

Maybe compare what you have with a fresh D9/Commerce2 install.