Bring Mailers from Spree and make order shipped email translatable #5763

luisramos0 · 2020-07-14T13:21:09Z

What? Why?

Closes #5686
and addresses point 2 in openfoodfoundation/wishlist#357

Move all needed mailers from spree_core to OFN.

This was we can now translate test email and order shipped email (this email still needs a better template, but that will be for next time).

What should we test?

Here we change the base_mailer so better verify email basics like order confirmation and order cancellation for example. Are they sent correctly? Is the data in the email correct? Is the invoice email sent correctly?
Test the emails that were changed:

test email - is this email sent when the button pressed in the backoffice mail settings?
shipped email - is this email sent when order is marked as shipped? Any errors in the old un-styled template?

Release notes

Changelog Category: Changed
Brought test and shipment emails from Spree. Order Shipped email is now translatable!

…rder_mailer specs

…lookups

sauloperez · 2020-07-17T16:36:31Z

app/views/spree/shipment_mailer/shipped_email.html.haml

+- if @shipment.tracking_url
+  = Spree.t('shipment_mailer.shipped_email.track_link', url: @shipment.tracking_url)
+
+= Spree.t('shipment_mailer.shipped_email.thanks')


Are you sure switching from .text to .HTML won't break things? I never investigated this but I think .text is used as a fallback text-version of the email for specific old mail servers/clients. Again, no idea about how it works.

Spree emails are all text, and ofn's are html. So I think it's fine, from now on these will also be sent in html. Testing will validate that as well.

I think you are referring to something we could do which is to send html and text emails and the email client would use text as a fall back option. This is also good for deliverability but it's something we are not doing and is out of scope for this PR. Does this make sense to you?

Understood, I thought those text ones were just a fallback and not Spree's only templates 👍 Then, this means that those only-text emails saw Eugeni some time ago were a bug or they were missing an OFN template 🙈

The emails seen by Eugeni are this shipment email. It was text in spree and after this PR it's html in OFN. But still without the normal OFN styling. That will need to be addressed in another PR.

RachL · 2020-07-29T16:16:29Z

OMG I'm just discovering this PR!!! Being able to translate this email is such a GOOOD news!!

Thanks @luisramos0 !!

luisramos0 · 2020-07-29T18:27:44Z

I know!! :-)

filipefurtad0 · 2020-07-30T09:17:39Z

Hi @luisramos0,

Triggered the following emails, before and after the PR:

Test
Order shipped
Confirm Account
Welcome to OFN
Reset Password
Send Invoice
Order Confirmation
Order Cancellation

Test emails

Before

After

Order shipped Emails

Before

After

Confirm Account Email - OK, before and after
Welcome to OFN Email - OK, before and after
Reset Password Email - OK, before and after
Send Invoice (pdf per email) - OK, before and after
Order Confirmation emails

These seem to be broken after this PR. I checked this both by clicking "Resend" or "Resend Confirmation" in the backoffice, and in both in this case, the user sees:

After placing a new order, neither the Hub nor the customer receive confirmation email. This is a silent error, no snail seen. Check Bugsnag errors for details:

https://app.bugsnag.com/yaycode/openfoodnetwork-uk/errors/5f228433c37fb80018f09297

Order cancellation emails are also broken, after this PR

The changes introduced by the PR work fine, but order confirmation/cancellation emails are quite a critical, so perhaps best not to introduce these in the code, right? I'm moving the PR back to In Dev for a second look.

Some Updates:

Findings above were double checked on staging-FR
Order confirmation emails from a placed order (staging-UK) arrived for both hub and customer, about 2hrs after this test was made.

Also, a product question maybe @RachL @lin-d-hop : I'm not sure what's the difference between the two buttons (pic below). "Resend Confirmation" triggers the modal asking for confirmation but other than that, is there a difference in function?

RachL · 2020-07-30T09:50:52Z

Also, a product question maybe @RachL @lin-d-hop : I'm not sure what's the difference between the two buttons (pic below). "Resend Confirmation" triggers the modal asking for confirmation but other than that, is there a difference in function?

Yes I've always wondered as well. 🤷

filipefurtad0 · 2020-07-30T10:18:17Z

In that case, maybe we could consider removing one of them, maybe as a papercut - if appropriate.

The "Resend Confirmation" button has a more descriptive naming and already has the associated modal, which is also helpful I think, so we could keep this one. Thoughts?

RachL · 2020-07-30T10:22:10Z

Sounds good 👍 Let's re-test maybe just to see if it is really 100% the same? Maybe you already did?

filipefurtad0 · 2020-07-30T10:36:32Z

Yes, I found no difference - the emails looked identical, but I'd wait for feedback from core Devs as well. Perhaps @luisramos0 and/or reviewers can confirm that the code behind these features is the same?

luisramos0 · 2020-07-30T15:53:13Z

yes, the code is the same behind both resends 👍

Thanks for testing Filipe, I cant replicate that error locally. It's a tricky one related to class loading I think because the "missing method" is there and included as part of one of the helper classes... it should work.

I'll have to come back to this later.

filipefurtad0 · 2020-07-31T14:11:26Z

Thanks for your feedback @luisramos0.

The snail-errors were reproducible in staging environments (UK and FR). But thinking of it now, I am not entirely sure the PR broke the order confirmation emails as well, or they just got queued after the snail errors. Maybe testing in a different order would result in a different outcome. I'll try this and provide some additional feedback.

Thanks as well for confirming the code on the "Resend" button 👍
Opened an Enhancement/Papercut #5850 on this regard.

filipefurtad0 · 2020-07-31T18:54:32Z

I did a quick double-checked now:
Order confirmation emails trigger a bugsnag error as well, and don't arrive the mailbox. This happens also after placing an order immediately after staging the PR, so it appears independent of those errors seen by the user (order cancelling and resend confirmation).

…he required ::CheckoutHelper in OFN The OFN checkoutHelper was not being included and instead the Spree::CheckoutHelper, that doesnt have the necessary helpers, was used

luisramos0 · 2020-08-03T15:30:04Z

ok, this was easier than I thought. Yes, there was a bug in the code. There's Spree::CheckoutHelper in Spree that was hidden our CheckoutHelper. I managed to replicate the problem you describe, I pushed a new simple commit (no need for code review) and I restaged the PR. The problem is now gone. This is ready for testing again 👍

…xt and is now html This template needs to be revisited, this is just a quick fix

luisramos0 · 2020-08-03T15:45:59Z

In staging I got the shipment email like this:

So I added some paragraphs and breaklines to the template so it doesnt happen again.
We need to revisit this email template. I think we need a new issue to redo the shipment email template, like this one: #2910

luisramos0 · 2020-08-03T16:12:34Z

ok, the email now looks like this 👍

RachL · 2020-08-05T16:35:23Z

I've re-tested them all:

Test : I'm not sure how to get the email. Should I turn one of my accounts as super admin?
Order shipped ✅
Confirm Account ✅
Welcome to OFN ✅
Reset Password ✅
Send Invoice ✅
Order Confirmation ✅
Order Cancellation ✅

As all email works perfectly, I'm moving this to ready to go!

RachL · 2020-08-05T16:40:02Z

Nevermind: I found how to test the test email :)

luisramos0 added 7 commits July 14, 2020 13:28

Bring remaing mailers from spree_core

c6cd695

Merge decorators with original classes brought from spree and merge o…

d12495f

…rder_mailer specs

Fix easy rubocop issues

5162964

Fix mailer specs brought from spree_core

e96d9c1

Transpec mailer specs

5835a0e

Bring test and shipment email templates from spree_core

55f160c

Convert test and shipment email templates to haml

f66538d

luisramos0 force-pushed the mailers branch from b45cae9 to a6b7850 Compare July 14, 2020 13:34

Add translation keys for test and shipment emails and make them lazy …

646f48f

…lookups

luisramos0 force-pushed the mailers branch from a6b7850 to 646f48f Compare July 14, 2020 13:35

luisramos0 self-assigned this Jul 14, 2020

luisramos0 changed the title ~~Mailers~~ Bring Mailers from Spree and make order shipped email translatable Jul 14, 2020

Matt-Yorkley approved these changes Jul 16, 2020

View reviewed changes

sauloperez reviewed Jul 17, 2020

View reviewed changes

sauloperez approved these changes Jul 22, 2020

View reviewed changes

filipefurtad0 self-assigned this Jul 29, 2020

filipefurtad0 added pr-staged-uk staging.openfoodnetwork.org.uk and removed pr-staged-uk staging.openfoodnetwork.org.uk labels Jul 29, 2020

filipefurtad0 added the pr-staged-uk staging.openfoodnetwork.org.uk label Jul 30, 2020

filipefurtad0 added pr-staged-uk staging.openfoodnetwork.org.uk pr-staged-fr staging.coopcircuits.fr and removed pr-staged-uk staging.openfoodnetwork.org.uk pr-staged-fr staging.coopcircuits.fr labels Jul 30, 2020

filipefurtad0 mentioned this pull request Jul 31, 2020

Remove obsolete "Resend" button (edit order page) #5850

Closed

filipefurtad0 added the pr-staged-uk staging.openfoodnetwork.org.uk label Jul 31, 2020

filipefurtad0 removed the pr-staged-uk staging.openfoodnetwork.org.uk label Jul 31, 2020

Fix conflict between unused Spree::CheckoutHelper in spree_core and t…

6e57e1a

…he required ::CheckoutHelper in OFN The OFN checkoutHelper was not being included and instead the Spree::CheckoutHelper, that doesnt have the necessary helpers, was used

luisramos0 force-pushed the mailers branch from 6cd5b54 to 6e57e1a Compare August 3, 2020 15:25

Add some paragraphs and breaklines to the tempalte that used to be te…

425901f

…xt and is now html This template needs to be revisited, this is just a quick fix

RachL added the pr-staged-es label Aug 5, 2020

RachL self-assigned this Aug 5, 2020

RachL removed the pr-staged-es label Aug 5, 2020

luisramos0 merged commit f093656 into openfoodfoundation:master Aug 5, 2020

luisramos0 deleted the mailers branch August 5, 2020 17:09

This was referenced Aug 20, 2020

Move lib/spree to OFN #5733

Merged

Fix master build - fix mail interceptor spec #5935

Merged

luisramos0 mentioned this pull request Sep 4, 2020

Multi language Email issues openfoodfoundation/wishlist#357

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bring Mailers from Spree and make order shipped email translatable #5763

Bring Mailers from Spree and make order shipped email translatable #5763

luisramos0 commented Jul 14, 2020 •

edited

Loading

sauloperez Jul 17, 2020

luisramos0 Jul 17, 2020 •

edited

Loading

sauloperez Jul 22, 2020

luisramos0 Jul 22, 2020

RachL commented Jul 29, 2020 •

edited

Loading

luisramos0 commented Jul 29, 2020

filipefurtad0 commented Jul 30, 2020 •

edited

Loading

RachL commented Jul 30, 2020

filipefurtad0 commented Jul 30, 2020

RachL commented Jul 30, 2020

filipefurtad0 commented Jul 30, 2020

luisramos0 commented Jul 30, 2020

filipefurtad0 commented Jul 31, 2020

filipefurtad0 commented Jul 31, 2020

luisramos0 commented Aug 3, 2020

luisramos0 commented Aug 3, 2020

luisramos0 commented Aug 3, 2020

RachL commented Aug 5, 2020

RachL commented Aug 5, 2020

Bring Mailers from Spree and make order shipped email translatable #5763

Bring Mailers from Spree and make order shipped email translatable #5763

Conversation

luisramos0 commented Jul 14, 2020 • edited Loading

What? Why?

What should we test?

Release notes

sauloperez Jul 17, 2020

Choose a reason for hiding this comment

luisramos0 Jul 17, 2020 • edited Loading

Choose a reason for hiding this comment

sauloperez Jul 22, 2020

Choose a reason for hiding this comment

luisramos0 Jul 22, 2020

Choose a reason for hiding this comment

RachL commented Jul 29, 2020 • edited Loading

luisramos0 commented Jul 29, 2020

filipefurtad0 commented Jul 30, 2020 • edited Loading

RachL commented Jul 30, 2020

filipefurtad0 commented Jul 30, 2020

RachL commented Jul 30, 2020

filipefurtad0 commented Jul 30, 2020

luisramos0 commented Jul 30, 2020

filipefurtad0 commented Jul 31, 2020

filipefurtad0 commented Jul 31, 2020

luisramos0 commented Aug 3, 2020

luisramos0 commented Aug 3, 2020

luisramos0 commented Aug 3, 2020

RachL commented Aug 5, 2020

RachL commented Aug 5, 2020

luisramos0 commented Jul 14, 2020 •

edited

Loading

luisramos0 Jul 17, 2020 •

edited

Loading

RachL commented Jul 29, 2020 •

edited

Loading

filipefurtad0 commented Jul 30, 2020 •

edited

Loading