Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Is it possible to resend the order confirmation email?
Comment | File | Size | Author |
---|---|---|---|
#81 | 2912598-81.patch | 26.28 KB | simgui8 |
#78 | 2912598-78.patch | 26.22 KB | lisastreeter |
| |||
#71 | 2912598-71.patch | 26.1 KB | lisastreeter |
| |||
#64 | 2912598-64.patch | 31.45 KB | lisastreeter |
#63 | 2912598-63.patch | 21.44 KB | simgui8 |
|
Comments
Comment #2
agoradesign CreditAttribution: agoradesign commentedI'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
Comment #3
agoradesign CreditAttribution: agoradesign commentedIn 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
Comment #4
sorabh.v6Comment #5
mglamansorabh.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?
Comment #6
sorabh.v6Hi, @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.phpThanks
Comment #7
agoradesign CreditAttribution: agoradesign commented@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
Comment #8
mglamanThis should be an admin operation only. On the order management form for admins. Not for end users.
Comment #9
sorabh.v6@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
Comment #10
bojanz CreditAttribution: bojanz at Centarro commented#7 is the only way to do it.
Comment #11
sorabh.v6I have started working as per #7. I will post patch soon.
Comment #12
sorabh.v6Hi 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
Comment #13
bojanz CreditAttribution: bojanz at Centarro commentedJust 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.
Comment #14
sorabh.v6@bojanz What fields I should add to Confirmation form?
Comment #15
sorabh.v6Work as per #13 included in the patch. Please review.
Thanks
Comment #16
Hygglo CreditAttribution: Hygglo commentedI 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).
Comment #17
mglamanThe order receipt service.
$order_receipt
The order receipt service.
The order receipt service.
$this->t()
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.
Comment #18
lammensj CreditAttribution: lammensj commentedRerolled against commerce 8.2.
Comment #19
sorabh.v6As per #17, we still need tests for this.
Comment #20
sorabh.v6Comment #21
sorabh.v6Created test for resend confirmation email. I could not test this on my machine but I hope it works.
Comment #23
sorabh.v6Comment #26
alexpottWe 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:
$this->t()
is preferred overt()
.This is an out-of-scope change.
The order to send receipt for.
"we" don't do much :)Returns TRUE if the mail sent successfully, FALSE if not.
is better.Looking at MailManager::mail() it does 't work this way. You need to do something like:
Relatedly core makes the same mistake - see #2933916: EmailAction incorrectly uses the result of $this->mailManager->mail()
The JavascriptTestTrait is not necessary for this test.
After this let's just make sure that saving has not sent a receipt by doing:
Comment #27
alexpottInstead 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.
Comment #28
rakesh.gectcr[#26] is done.
To do, WIP [#27]
Comment #29
alexpottThis 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.
Comment #31
rakesh.gectcrtest is getting passed at my local
Comment #33
rakesh.gectcrWorking on it
Comment #34
alexpottThe fail in #31 is a random fail in commerce. Fixed by #2930209: Random fail in PaymentMethodStorageTest.
Comment #35
alexpottI 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.
There are 2 returns in this method that return nothing - they should return FALSE.
Comment #36
postback CreditAttribution: postback commentedApplying 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?
Comment #37
mattjones86This wouldn't apply for me on 2.7. New patch attached.
Comment #38
mattjones86Actually 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?
Comment #39
mattjones86Comment #41
simgui8 CreditAttribution: simgui8 as a volunteer and commented#37 and #38 apply successfully for me on 2.7
Comment #42
mattjones86Yeah please ignore #39... I shouldn't have posted that. If only I could delete it!
Comment #43
s.messaris CreditAttribution: s.messaris commentedI 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?
Comment #44
simgui8 CreditAttribution: simgui8 as a volunteer and commentedThis patch resend the confirmation email this way :
if user is known (except anonymous)
-in the user preffered language
else
-the admin current language
Comment #45
agoradesign CreditAttribution: agoradesign commentedDid a quick look at patches #38 and #44:
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
Comment #46
simgui8 CreditAttribution: simgui8 as a volunteer and commentedJust 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?
Comment #47
agoradesign CreditAttribution: agoradesign commentedSure? Because this is what's inside the patch:
And that looks good imho
Comment #48
simgui8 CreditAttribution: simgui8 as a volunteer and commentedWith #38, I have followed $langcode until doMail from MailManager.php
$langcode is always the expected value (the client preferred language) from:
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?
Comment #49
agoradesign CreditAttribution: agoradesign commentedI 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!?!?
Comment #50
simgui8 CreditAttribution: simgui8 as a volunteer and commentedAfter further testing: the behavior in #48 seems expected.
From MailManager::mail doc
From my understanding, when a user creates an order
When it's from a form (the actual case), it could be ether
Comment #51
simgui8 CreditAttribution: simgui8 as a volunteer and commentedHere is a reroll of #44 using \Drupal::messenger()->addStatus with a note about customer preferred language
Comment #52
eli-on-drupal CreditAttribution: eli-on-drupal at Mindgrub Technologies commentedComment #54
mglamanI'm so confused
What is this patch doing that destroys the tests?
Comment #55
simgui8 CreditAttribution: simgui8 as a volunteer and commentedWell, 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
Comment #56
eli-on-drupal CreditAttribution: eli-on-drupal at Mindgrub Technologies commentedComment #57
mglamanThere's no interdiff, but I'm assuming this is the magic?
Comment #58
simgui8 CreditAttribution: simgui8 as a volunteer and commentedThis is a reroll for 2.10 (dev 2018-10-17)
Comment #59
agoradesign CreditAttribution: agoradesign commentedI've tried the patch from #58. Works for me
Comment #60
agoradesign CreditAttribution: agoradesign commentedPS: 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
Comment #61
simgui8 CreditAttribution: simgui8 as a volunteer and commentedAdded 'Resend order confirmation email' permission.
Comment #63
simgui8 CreditAttribution: simgui8 as a volunteer and commentedShould fix previous fail (test was missing new permission)
Comment #64
lisastreeter CreditAttribution: lisastreeter at Centarro commentedI'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.
$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.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 codebcc
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.Comment #65
lisastreeter CreditAttribution: lisastreeter at Centarro commentedComment #66
simgui8 CreditAttribution: simgui8 as a volunteer and commentedThank you Lisa
After trying patch from #64
Mail fail sending :
when sending to customer with non-english preferred language :
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)
Comment #67
bojanz CreditAttribution: bojanz at Centarro commentedThis 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.
We already have an OrderRouteProvider, we can generate this route from there (especially since we already have a link template defined).
Language is defined but not used. A user is loaded manually even though orders have a getCustomer() method.
Comment #68
simgui8 CreditAttribution: simgui8 as a volunteer and commentedLanguage 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?
Comment #69
simgui8 CreditAttribution: simgui8 as a volunteer and commentedHere 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.
is removed
Maybe someone can pitch in to fix this part (did not find my way through it).
Comment #70
bojanz CreditAttribution: bojanz at Centarro commentedWe have committed both #2984098: Order receipt themed differently for orders entered in admin and #3017080: Create a service for sending emails to customers, we can now revisit and reroll this patch.
Comment #71
lisastreeter CreditAttribution: lisastreeter at Centarro commentedThis 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.
Comment #72
lisastreeter CreditAttribution: lisastreeter at Centarro commentedComment #73
simgui8 CreditAttribution: simgui8 as a volunteer and commentedTested on multilingual setup.
#71 works great!
Thank you lisa! for your work (and bojanz/mglaman/berdir/smessaris/everyone)
Comment #74
bojanz CreditAttribution: bojanz at Centarro commentedI love how much smaller this patch has gotten over the initial attempts.
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).
5. This is also a leftover, the latest code had bcc directly in params, the if is not needed.
6. Code incorrectly indented, doesn't pass phpcs.
We should introduce a getResendReseiptFormRoute() helper like we do for routes elsewhere.
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.
8. Interface not found, you're missing \Mail.
Comment #75
lisastreeter CreditAttribution: lisastreeter at Centarro commentedRemoving 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.
Comment #76
bojanz CreditAttribution: bojanz at Centarro commentedWe should convert the isset() check in MailHandler into a !empty check, that way callers don't need to care about passing an empty value.
Comment #77
lisastreeter CreditAttribution: lisastreeter at Centarro commented@bojanz Thanks. Made the change in MailHandler. Test is happy now.
Patch reroll for review in comment #
Comment #78
lisastreeter CreditAttribution: lisastreeter at Centarro commentedAnother try.
Comment #79
agoradesign CreditAttribution: agoradesign commentedyou forgot the status change
Comment #80
lisastreeter CreditAttribution: lisastreeter at Centarro commented@agoradesign Thanks! I always wait until the tests pass, first :)
Comment #81
simgui8 CreditAttribution: simgui8 as a volunteer and commentedJust a little touch up to include #3031195
Comment #82
simgui8 CreditAttribution: simgui8 as a volunteer and commentedAfter tested the following :
Theming is good, language is good, logs are good!
Comment #83
agoradesign CreditAttribution: agoradesign commented@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"
Comment #84
bojanz CreditAttribution: bojanz at Centarro commentedI'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.
Comment #85
bojanz CreditAttribution: bojanz at Centarro commentedI 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.
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.
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.
6. It is unclear how multiple addresses could be passed. The right answer is "comma separated", so let's document that.
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.
9. The docblock description should be "Gets the resend-receipt-form 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).
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.
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.
Comment #87
bojanz CreditAttribution: bojanz at Centarro commentedAddressed #85 and committed. Thanks, everyone!
Comment #88
simgui8 CreditAttribution: simgui8 as a volunteer and commentedThis is so awesome!
Thank you bojanz for the detailed follow up and everyone who worked on this!
Comment #90
petednz CreditAttribution: petednz at Fuzion commentedthx all. found it under Operations (rhs) of the commerce/orders page
Comment #91
simgui8 CreditAttribution: simgui8 as a volunteer and commentedWhen 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.