From d1fa5c4649fc6f924dcdd5ce2700417909733645 Mon Sep 17 00:00:00 2001 From: Iain McNulty Date: Mon, 14 Oct 2024 13:23:40 +0100 Subject: [PATCH 1/5] format: Update rubocop to match current style --- .rubocop.yml | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/.rubocop.yml b/.rubocop.yml index 763fa0c..1ec0288 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -11,4 +11,12 @@ Style/Documentation: Metrics/BlockLength: Exclude: - - 'spec/**/*' \ No newline at end of file + - 'spec/**/*' + +Style/StringLiterals: + Enabled: true + EnforcedStyle: double_quotes + +Layout/SpaceInsideHashLiteralBraces: + Enabled: true + EnforcedStyle: no_space From e25d90feef95d6cd8af4b029305aaf699ff2e9af Mon Sep 17 00:00:00 2001 From: Iain McNulty Date: Mon, 14 Oct 2024 13:24:00 +0100 Subject: [PATCH 2/5] format: Format mailer_spec according to rubocop config --- spec/mail/notify/mailer_spec.rb | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/spec/mail/notify/mailer_spec.rb b/spec/mail/notify/mailer_spec.rb index 00d852b..c19bd6e 100644 --- a/spec/mail/notify/mailer_spec.rb +++ b/spec/mail/notify/mailer_spec.rb @@ -6,7 +6,8 @@ RSpec.describe Mail::Notify::Mailer do describe "#view_mail" do it "sets the message template id" do - message_params = {template_id: "template-id", to: "test.name@email.co.uk", subject: "Test subject"} + message_params = {template_id: "template-id", to: "test.name@email.co.uk", + subject: "Test subject"} message = TestMailer.with(message_params).test_view_mail @@ -14,7 +15,8 @@ end it "sets the message subject" do - message_params = {template_id: "template-id", to: "test.name@email.co.uk", subject: "Test subject"} + message_params = {template_id: "template-id", to: "test.name@email.co.uk", + subject: "Test subject"} message = TestMailer.with(message_params).test_view_mail @@ -23,7 +25,8 @@ end it "sets the message to address" do - message_params = {template_id: "template-id", to: "test.name@email.co.uk", subject: "Test subject"} + message_params = {template_id: "template-id", to: "test.name@email.co.uk", + subject: "Test subject"} message = TestMailer.with(message_params).test_view_mail @@ -32,7 +35,8 @@ end it "sets the subject on personalisation" do - message_params = {template_id: "template-id", to: "test.name@email.co.uk", subject: "Test subject"} + message_params = {template_id: "template-id", to: "test.name@email.co.uk", + subject: "Test subject"} message = TestMailer.with(message_params).test_view_mail @@ -40,7 +44,8 @@ end it "sets the body on personalisation" do - message_params = {template_id: "template-id", to: "test.name@email.co.uk", subject: "Test subject"} + message_params = {template_id: "template-id", to: "test.name@email.co.uk", + subject: "Test subject"} message = TestMailer.with(message_params).test_view_mail @@ -145,7 +150,8 @@ 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"} + message_params = {template_id: "template-id", to: "test.name@email.co.uk", + subject: "My subject"} message = TestMailer.with(message_params).test_template_mail From b70f6982e38d7c66d860353f6b4f7c2a3fd615f3 Mon Sep 17 00:00:00 2001 From: Iain McNulty Date: Mon, 14 Oct 2024 13:29:59 +0100 Subject: [PATCH 3/5] fix: Call except with * positional arguments personalisation, reply_to_id and reference were not being excluded from the headers in view_mail and template_mail becuase they were passed in an array to Hash#except. Reference: https://rubyapi.org/3.3/o/hash#method-i-except --- lib/mail/notify/mailer.rb | 11 +++++-- spec/mail/notify/mailer_spec.rb | 54 +++++++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 2 deletions(-) diff --git a/lib/mail/notify/mailer.rb b/lib/mail/notify/mailer.rb index cd871fd..c1ca595 100644 --- a/lib/mail/notify/mailer.rb +++ b/lib/mail/notify/mailer.rb @@ -38,7 +38,7 @@ def template_mail(template_id, options) message.personalisation = options[:personalisation] || {} - headers = options.except([:personalisation, :reply_to_id, :reference]) + headers = options.except(:personalisation, :reply_to_id, :reference) headers[:subject] = "Subject managed in Notify" unless options[:subject] @@ -74,12 +74,19 @@ def view_mail(template_id, options) message.reference = options[:reference] subject = options[:subject] - headers = options.except([:personalisation, :reply_to_id, :reference]) + headers = options.except(:personalisation, :reply_to_id, :reference) # 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. + # Calling the #mail method is not idempotent. It modifies state by setting instance variables on the message. Specifically it sets @_message. + # mail generates message headers for the options passed in. + # each time it is called with the same headers it adds another header field. + # + # original_message = message.dup body = mail(headers).body.raw_source + # @_message = original_message + # The 'view mail' works by sending a subject and body as personalisation options, these are # then used in the Notify template to provide content. message.personalisation = {subject: subject, body: body} diff --git a/spec/mail/notify/mailer_spec.rb b/spec/mail/notify/mailer_spec.rb index c19bd6e..2a9f784 100644 --- a/spec/mail/notify/mailer_spec.rb +++ b/spec/mail/notify/mailer_spec.rb @@ -14,6 +14,33 @@ expect(message.template_id).to eql("template-id") end + it "does not set reply_to_id as a header" do + message_params = {template_id: "template-id", to: "test.name@email.co.uk", + subject: "Test subject", reply_to_id: "123"} + + message = TestMailer.with(message_params).test_view_mail + + expect(message.header[:reply_to_id]).to be_nil + end + + it "does not set reference as a header" do + message_params = {template_id: "template-id", to: "test.name@email.co.uk", + subject: "Test subject", reference: "ref-123"} + + message = TestMailer.with(message_params).test_view_mail + + expect(message.header[:reference]).to be_nil + end + + it "does not set personalisation as a header" do + message_params = {template_id: "template-id", to: "test.name@email.co.uk", + subject: "Test subject", personalisation: "Dear sir"} + + message = TestMailer.with(message_params).test_view_mail + + expect(message.header[:personalisation]).to be_nil + end + it "sets the message subject" do message_params = {template_id: "template-id", to: "test.name@email.co.uk", subject: "Test subject"} @@ -131,6 +158,33 @@ expect(message.template_id).to eql("template-id") end + it "does not set reply_to_id as a header" do + message_params = {template_id: "template-id", to: "test.name@email.co.uk", + subject: "Test subject", reply_to_id: "123"} + + message = TestMailer.with(message_params).test_template_mail + + expect(message.header[:reply_to_id]).to be_nil + end + + it "does not set reference as a header" do + message_params = {template_id: "template-id", to: "test.name@email.co.uk", + subject: "Test subject", reference: "ref-123"} + + message = TestMailer.with(message_params).test_template_mail + + expect(message.header[:reference]).to be_nil + end + + it "does not set personalisation as a header" do + message_params = {template_id: "template-id", to: "test.name@email.co.uk", + subject: "Test subject", personalisation: "Dear sir"} + + message = TestMailer.with(message_params).test_template_mail + + expect(message.header[:personalisation]).to be_nil + end + it "sets the message to address" do message_params = {template_id: "template-id", to: "test.name@email.co.uk"} From 4ede654013dffbf919db27c7ad7d0c36ff011df0 Mon Sep 17 00:00:00 2001 From: Iain McNulty Date: Mon, 14 Oct 2024 13:40:51 +0100 Subject: [PATCH 4/5] format: LineLength --- lib/mail/notify/mailer.rb | 43 ++++++++++++++++++++------------------- 1 file changed, 22 insertions(+), 21 deletions(-) diff --git a/lib/mail/notify/mailer.rb b/lib/mail/notify/mailer.rb index c1ca595..395ab54 100644 --- a/lib/mail/notify/mailer.rb +++ b/lib/mail/notify/mailer.rb @@ -3,13 +3,14 @@ module Mail module Notify ## - # The Mail Notify base Mailer class, overridden in Rails applications to provide the additional - # Notify behaviour along with the application behaviour. + # The Mail Notify base Mailer class, overridden in Rails applications to + # provide the additional Notify behaviour along with the application + # behaviour. class Mailer < ActionMailer::Base ## - # Set a default from address, will only be used in previews if a from address is not supplied - # by subclasses + # Set a default from address, will only be used in previews if a from + # address is not supplied by subclasses default from: "preview@notifications.service.gov.uk" @@ -25,12 +26,17 @@ class Mailer < ActionMailer::Base # # Add any additional headers in the options hash. # - # 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. + # 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. def template_mail(template_id, options) raise ArgumentError, "You must specify a Notify template ID" if template_id.blank? - raise ArgumentError, "You must specify a to address" if options[:to].nil? || options[:to].blank? + + if options[:to].nil? || options[:to].blank? + raise ArgumentError, + "You must specify a to address" + end message.template_id = template_id message.reply_to_id = options[:reply_to_id] @@ -42,9 +48,9 @@ def template_mail(template_id, options) headers[:subject] = "Subject managed in Notify" unless options[:subject] - # 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 - # sending or previewing + # 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 sending or previewing mail(headers) do |format| format.text { nil } format.html { nil } @@ -76,19 +82,14 @@ def view_mail(template_id, options) subject = options[:subject] headers = options.except(:personalisation, :reply_to_id, :reference) - # 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. - # Calling the #mail method is not idempotent. It modifies state by setting instance variables on the message. Specifically it sets @_message. - # mail generates message headers for the options passed in. - # each time it is called with the same headers it adds another header field. - # - # original_message = message.dup + # 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. body = mail(headers).body.raw_source - # @_message = original_message - - # The 'view mail' works by sending a subject and body as personalisation options, these are - # then used in the Notify template to provide content. + # The 'view mail' works by sending a subject and body as + # personalisation options, these are then used in the Notify template + # to provide content. message.personalisation = {subject: subject, body: body} mail(headers) do |format| From 9f97605cfc75344aa8c116403ab115aebb69798f Mon Sep 17 00:00:00 2001 From: Iain McNulty Date: Mon, 14 Oct 2024 13:47:41 +0100 Subject: [PATCH 5/5] fix: Prevent multiple calls to #mail method producing multiple fields Calling #mail multiple times produces multiple Mail::Field the #view_mail method calls mail twice. First to generate a body which it can pass to the #personalisation= method. The second is to generate the final Mail object. We can prevent undesireable side effects of multiple calls to mail by duplicating the message before it's mutated. --- lib/mail/notify/mailer.rb | 21 ++++++++++++++++++++- spec/mail/notify/mailer_spec.rb | 10 ++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/lib/mail/notify/mailer.rb b/lib/mail/notify/mailer.rb index 395ab54..86dc433 100644 --- a/lib/mail/notify/mailer.rb +++ b/lib/mail/notify/mailer.rb @@ -84,9 +84,28 @@ def view_mail(template_id, options) # 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. + # the Notify API. + # + # Calling the #mail method is not idempotent. It + # modifies state by setting instance variables on the message. + # Specifically, it sets @_message. mail generates message headers for + # the options passed in. Each time it is called with the same headers + # it adds another header field. This results in something like this + # + # mail({custom_header => 123}) + # message.header['custom_header'] + # #=> Mail::Field + # + # mail({custom_header => 123}) + # message.header['custom_header'] + # #=> [Mail::Field..., Mail::Field...] + # + original_message = message.dup + body = mail(headers).body.raw_source + @_message = original_message + # The 'view mail' works by sending a subject and body as # personalisation options, these are then used in the Notify template # to provide content. diff --git a/spec/mail/notify/mailer_spec.rb b/spec/mail/notify/mailer_spec.rb index 2a9f784..791ed8d 100644 --- a/spec/mail/notify/mailer_spec.rb +++ b/spec/mail/notify/mailer_spec.rb @@ -14,6 +14,16 @@ expect(message.template_id).to eql("template-id") end + it "sets a custom value as a header" do + message_params = {template_id: "template-id", to: "test.name@email.co.uk", + subject: "Test subject", custom_header: "custom"} + + 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') + end + it "does not set reply_to_id as a header" do message_params = {template_id: "template-id", to: "test.name@email.co.uk", subject: "Test subject", reply_to_id: "123"}