Skip to content

Commit

Permalink
Refactor mailer methods
Browse files Browse the repository at this point in the history
We only keep a Mail::Message for use in previews, it is never actually
sent, email sending is handled by Notify, not the host application.

The Notify API does not accept additional custom headers, so passing
custom headers to the message via the call to `mail` serves no purpose
as those headers will never be used (other than in the previews, where
they get shown).

This work refactors the two mailer methods so that only the `to` and
`subject` headers are set on the Mail::Message returned.
  • Loading branch information
mec committed Oct 18, 2024
1 parent 72254a5 commit a4db21f
Show file tree
Hide file tree
Showing 3 changed files with 37 additions and 16 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ The format is based on [Keep a Changelog]
included in the preview email headers for no reason - thanks to @inulty-dfe
- Fixed a bug in `view_mail` that meant custom headers in the preview email were
duplicated - thanks to @inulty-dfe
- Custom headers passed in the options to `view_mail` and `template_mail` will
no longer be passed to the preview email.

## [2.0.0] - 2024-04-01

Expand Down
25 changes: 15 additions & 10 deletions lib/mail/notify/mailer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,13 @@ class Mailer < ActionMailer::Base
# - template_id
# - to address
#
# Can include personalisation.
#
# Add any additional headers in the options hash.
# The optional arguments are:
#
# - subject
# - personalisation
# - reply_to_id
# - reference
#
# A default subject is supplied as ActionMailer requires one, however it will never be used as
# the subject is assumed to be managed in the Notify template.
Expand All @@ -35,12 +39,9 @@ def template_mail(template_id, options)
message.template_id = template_id
message.reply_to_id = options[:reply_to_id]
message.reference = options[:reference]

message.personalisation = options[:personalisation] || {}

headers = options.except(:personalisation, :reply_to_id, :reference)

headers[:subject] = "Subject managed in Notify" unless options[:subject]
headers = {to: options[:to], subject: options[:subject] || "Subject managed in Notify"}

# We have to set the html and the plain text content to nil to prevent Rails from looking
# for the content in the views. We replace nil with the content returned from Notify before
Expand All @@ -60,9 +61,13 @@ def template_mail(template_id, options)
# - to address
# - subject
#
# Personalisation will dropped as all content comes from the view provided by Rails.
# The optional arguments are:
#
# - personalisation
# - reply_to_id
# - reference
#
# Add any additional headers in the options hash.
# Personalisation will be dropped as all content comes from the view provided by Rails.

def view_mail(template_id, options)
raise ArgumentError, "You must specify a Notify template ID" if template_id.blank?
Expand All @@ -74,13 +79,13 @@ def view_mail(template_id, options)
message.reference = options[:reference]

subject = options[:subject]
headers = options.except(:personalisation, :reply_to_id, :reference)
headers = {to: options[:to], subject: options[:subject]}

# We have to render the view for the message and grab the raw source, then we set that as the
# body in the personalisation for sending to the Notify API.
#
# We do not pass the headers as the call to `mail` will keep adding headers resulting in
# duplication when we have to call it again later.
# potential duplication when we have to call it again later.
body = mail.body.raw_source

# The 'view mail' works by sending a subject and body as personalisation options, these are
Expand Down
26 changes: 20 additions & 6 deletions spec/mail/notify/mailer_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@
expect(message.header[:reference]).to be_nil
end

it "sets custom headers only once" do
it "only includes to and subject headers" do
message_params = {
template_id: "template-id",
to: "test.name@email.co.uk",
Expand All @@ -143,8 +143,8 @@

message = TestMailer.with(message_params).test_view_mail

expect(message.header["custom-header"]).to be_a(Mail::Field)
expect(message.header["custom-header"].value).to eq("custom header value")
expect(message.header["custom-header"]).to be_nil
expect(message.header["template-id"]).to be_nil
end
end

Expand Down Expand Up @@ -175,6 +175,21 @@
expect(message.header[:subject].value).to eql("Subject managed in Notify")
end

context "when passed a subject" do
it "uses that subject" do
message_params = {
template_id: "template-id",
to: "test.name@email.co.uk",
subject: "Test subject"
}

message = TestMailer.with(message_params).test_template_mail

expect(message.header[:subject]).to be_a Mail::Field
expect(message.header[:subject].value).to eql("Test subject")
end
end

it "sets the subject if one is passed, even though it will not be used" do
message_params = {template_id: "template-id", to: "test.name@email.co.uk", subject: "My subject"}

Expand Down Expand Up @@ -251,7 +266,7 @@
expect(message.header[:reference]).to be_nil
end

it "sets custom headers only once" do
it "only includes to and subject headers" do
message_params = {
template_id: "template-id",
to: "test.name@email.co.uk",
Expand All @@ -261,8 +276,7 @@

message = TestMailer.with(message_params).test_view_mail

expect(message.header["custom-header"]).to be_a(Mail::Field)
expect(message.header["custom-header"].value).to eq("custom header value")
expect(message.header["custom-header"]).to be_nil
end
end

Expand Down

0 comments on commit a4db21f

Please sign in to comment.