From 99db95544ce8da0ca3179ddeb98c3f599f9e2281 Mon Sep 17 00:00:00 2001
From: meyric <meyric@dxw.com>
Date: Fri, 18 Oct 2024 14:40:20 +0100
Subject: [PATCH] Refactor mailer methods

We only keep a Mail::Message for use in previews, it is never actually
sent, email sending is handled by Notify, not your 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.
---
 lib/mail/notify/mailer.rb       | 25 +++++++++++++++----------
 spec/mail/notify/mailer_spec.rb | 26 ++++++++++++++++++++------
 2 files changed, 35 insertions(+), 16 deletions(-)

diff --git a/lib/mail/notify/mailer.rb b/lib/mail/notify/mailer.rb
index 0cd46bd..1472064 100644
--- a/lib/mail/notify/mailer.rb
+++ b/lib/mail/notify/mailer.rb
@@ -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.
@@ -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
@@ -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?
@@ -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
diff --git a/spec/mail/notify/mailer_spec.rb b/spec/mail/notify/mailer_spec.rb
index d72c68b..63804e9 100644
--- a/spec/mail/notify/mailer_spec.rb
+++ b/spec/mail/notify/mailer_spec.rb
@@ -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",
@@ -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
 
@@ -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"}
 
@@ -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",
@@ -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