Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 10 commits into from
Aug 5, 2020
27 changes: 27 additions & 0 deletions app/mailers/spree/base_mailer.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
# frozen_string_literal: true

module Spree
class BaseMailer < ActionMailer::Base
# Inline stylesheets
include Roadie::Rails::Automatic

# Define layout
layout 'mailer'

def from_address
Spree::Config[:mails_from]
end

def money(amount)
Spree::Money.new(amount).to_s
end
helper_method :money

protected

def roadie_options
# This lets us specify assets using relative paths in email templates
super.merge(url_options: { host: URI(main_app.root_url).host })
end
end
end
14 changes: 0 additions & 14 deletions app/mailers/spree/base_mailer_decorator.rb

This file was deleted.

68 changes: 68 additions & 0 deletions app/mailers/spree/order_mailer.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
# frozen_string_literal: true

module Spree
class OrderMailer < BaseMailer
helper HtmlHelper
helper CheckoutHelper
helper SpreeCurrencyHelper
helper OrderHelper
include I18nHelper

def cancel_email(order_or_order_id, resend = false)
@order = find_order(order_or_order_id)
I18n.with_locale valid_locale(@order.user) do
mail(to: @order.email,
from: from_address,
subject: mail_subject(t('spree.order_mailer.cancel_email.subject'), resend))
end
end

def confirm_email_for_customer(order_or_order_id, resend = false)
@order = find_order(order_or_order_id)
I18n.with_locale valid_locale(@order.user) do
subject = mail_subject(t('spree.order_mailer.confirm_email.subject'), resend)
mail(to: @order.email,
from: from_address,
subject: subject,
reply_to: @order.distributor.contact.email)
end
end

def confirm_email_for_shop(order_or_order_id, resend = false)
@order = find_order(order_or_order_id)
I18n.with_locale valid_locale(@order.user) do
subject = mail_subject(t('spree.order_mailer.confirm_email.subject'), resend)
mail(to: @order.distributor.contact.email,
from: from_address,
subject: subject)
end
end

def invoice_email(order_or_order_id, pdf)
@order = find_order(order_or_order_id)
attach_file("invoice-#{@order.number}.pdf", pdf)
I18n.with_locale valid_locale(@order.user) do
mail(to: @order.email,
from: from_address,
subject: mail_subject(t(:invoice), false),
reply_to: @order.distributor.contact.email)
end
end

private

# Finds an order instance from an order or from an order id
def find_order(order_or_order_id)
order_or_order_id.respond_to?(:id) ? order_or_order_id : Spree::Order.find(order_or_order_id)
end

def mail_subject(base_subject, resend)
resend_prefix = (resend ? "[#{t(:resend).upcase}] " : '')
"#{resend_prefix}#{Spree::Config[:site_name]} #{base_subject} ##{@order.number}"
end

def attach_file(filename, file)
attachments[filename] = file if file.present?
end
end
end
64 changes: 0 additions & 64 deletions app/mailers/spree/order_mailer_decorator.rb

This file was deleted.

13 changes: 13 additions & 0 deletions app/mailers/spree/shipment_mailer.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
# frozen_string_literal: true

module Spree
class ShipmentMailer < BaseMailer
def shipped_email(shipment, resend = false)
@shipment = shipment.respond_to?(:id) ? shipment : Spree::Shipment.find(shipment)
subject = (resend ? "[#{Spree.t(:resend).upcase}] " : '')
base_subject = Spree.t('shipment_mailer.shipped_email.subject')
subject += "#{Spree::Config[:site_name]} #{base_subject} ##{@shipment.order.number}"
mail(to: @shipment.order.email, from: from_address, subject: subject)
end
end
end
11 changes: 11 additions & 0 deletions app/mailers/spree/test_mailer.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
# frozen_string_literal: true

module Spree
class TestMailer < BaseMailer
def test_email(user)
recipient = user.respond_to?(:id) ? user : Spree.user_class.find(user)
subject = "#{Spree::Config[:site_name]} #{Spree.t('test_mailer.test_email.subject')}"
mail(to: recipient.email, from: from_address, subject: subject)
end
end
end
19 changes: 19 additions & 0 deletions app/views/spree/shipment_mailer/shipped_email.html.haml
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
= Spree.t('shipment_mailer.shipped_email.dear_customer')

= Spree.t('shipment_mailer.shipped_email.instructions')

= "============================================================"
= Spree.t('shipment_mailer.shipped_email.shipment_summary')
= "============================================================"
- @shipment.manifest.each do |item|
= item.variant.sku
= item.variant.product.name
= item.variant.options_text
= "============================================================"

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

= Spree.t('shipment_mailer.shipped_email.thanks')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@luisramos0 luisramos0 Jul 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🙈

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

4 changes: 4 additions & 0 deletions app/views/spree/test_mailer/test_email.html.haml
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
= t('test_mailer.test_email.greeting')
= "================"

= t('test_mailer.test_email.message')
74 changes: 74 additions & 0 deletions spec/mailers/order_mailer_spec.rb
Original file line number Diff line number Diff line change
@@ -1,8 +1,82 @@
# frozen_string_literal: true

require 'spec_helper'

describe Spree::OrderMailer do
include OpenFoodNetwork::EmailHelper

context "basic behaviour" do
let(:order) { build(:order_with_totals_and_distribution) }

context ":from not set explicitly" do
it "falls back to spree config" do
message = Spree::OrderMailer.confirm_email_for_customer(order)
expect(message.from).to eq [Spree::Config[:mails_from]]
end
end

it "doesn't aggressively escape double quotes in confirmation body" do
confirmation_email = Spree::OrderMailer.confirm_email_for_customer(order)
expect(confirmation_email.body).to_not include("&quot;")
end

it "confirm_email_for_customer accepts an order id as an alternative to an Order object" do
expect(Spree::Order).to receive(:find).with(order.id).and_return(order)
expect {
confirmation_email = Spree::OrderMailer.confirm_email_for_customer(order.id)
}.to_not raise_error
end

it "cancel_email accepts an order id as an alternative to an Order object" do
expect(Spree::Order).to receive(:find).with(order.id).and_return(order)
expect {
cancel_email = Spree::OrderMailer.cancel_email(order.id)
}.to_not raise_error
end
end

context "only shows eligible adjustments in emails" do
let(:order) { create(:order_with_totals_and_distribution) }

before do
order.adjustments.create(
label: "Eligible Adjustment",
amount: 10,
eligible: true
)

order.adjustments.create!(
label: "Ineligible Adjustment",
amount: 0,
)
end

let!(:confirmation_email) { Spree::OrderMailer.confirm_email_for_customer(order) }
let!(:cancel_email) { Spree::OrderMailer.cancel_email(order) }

specify do
expect(confirmation_email.body).to_not include("Ineligible Adjustment")
end

specify do
expect(cancel_email.body).to_not include("Ineligible Adjustment")
end
end

context "displays line item price" do
let(:order) { create(:order_with_totals_and_distribution) }

specify do
confirmation_email = Spree::OrderMailer.confirm_email_for_customer(order)
expect(confirmation_email.body).to include("3.00")
end

specify do
cancel_email = Spree::OrderMailer.cancel_email(order)
expect(cancel_email.body).to include("3.00")
end
end

describe "order confimation" do
let(:bill_address) { create(:address) }
let(:distributor_address) { create(:address, address1: "distributor address", city: 'The Shire', zipcode: "1234") }
Expand Down
36 changes: 36 additions & 0 deletions spec/mailers/shipment_mailer_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
# frozen_string_literal: true

require 'spec_helper'

describe Spree::ShipmentMailer do
let(:shipment) do
order = build(:order)
product = build(:product, name: %{The "BEST" product})
variant = build(:variant, product: product)
line_item = build(:line_item, variant: variant, order: order, quantity: 1, price: 5)
shipment = build(:shipment)
allow(shipment).to receive_messages(line_items: [line_item], order: order)
allow(shipment).to receive_messages(tracking_url: "TRACK_ME")
shipment
end

context ":from not set explicitly" do
it "falls back to spree config" do
message = Spree::ShipmentMailer.shipped_email(shipment)
expect(message.from).to eq [Spree::Config[:mails_from]]
end
end

# Regression test for #2196
it "doesn't include out of stock in the email body" do
shipment_email = Spree::ShipmentMailer.shipped_email(shipment)
expect(shipment_email.body).to_not include(%{Out of Stock})
end

it "shipment_email accepts an shipment id as an alternative to an Shipment object" do
expect(Spree::Shipment).to receive(:find).with(shipment.id).and_return(shipment)
expect {
shipped_email = Spree::ShipmentMailer.shipped_email(shipment.id)
}.to_not raise_error
end
end
21 changes: 21 additions & 0 deletions spec/mailers/test_mailer_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
# frozen_string_literal: true

require 'spec_helper'

describe Spree::TestMailer do
let(:user) { create(:user) }

context ":from not set explicitly" do
it "falls back to spree config" do
message = Spree::TestMailer.test_email(user)
expect(message.from).to eq [Spree::Config[:mails_from]]
end
end

it "confirm_email accepts a user id as an alternative to a User object" do
expect(Spree.user_class).to receive(:find).with(user.id).and_return(user)
expect {
test_email = Spree::TestMailer.test_email(user.id)
}.to_not raise_error
end
end