Multiple calls to ActionMailer::Base#mail produces multiple Mail::Fields in the headers #162
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
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.
7 tasks
|
Thank you so much for identifying this and offering a path to a solution! 👍 Would you mind opening an issue - I can do it, but it would be better coming from you? I'll have a look into this today. |
mec
reviewed
Oct 18, 2024
| - 'spec/**/*' | ||
|
|
||
| Style/StringLiterals: |
There was a problem hiding this comment.
We actually use Standard so this config shouldn't really be here, even though Standard uses Rubocop under the hood.
mec
reviewed
Oct 18, 2024
| @@ -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) | |||
There was a problem hiding this comment.
This is spot on and your tests demonstrate it nicely! 👌
mec
reviewed
Oct 18, 2024
| # #=> [Mail::Field..., Mail::Field...] | ||
| # | ||
| original_message = message.dup | ||
|
|
||
| body = mail(headers).body.raw_source |
There was a problem hiding this comment.
I think we can just not pass headers to the call to mail here and leave it to the final call that will be returned.
There was a problem hiding this comment.
Digging into it more, I can't see why we are handling headers other than to and subject at all, they will never be used as we cannot pass them onto Notify via the API.
I would be interested to know what headers are being duplicated for you and are showing up in your tests.
I've refactored the two methods to only use to and subject on the end of #165.
mec
added a commit
that referenced
this pull request
Oct 18, 2024
The call to `except` here should not be an array as there is no array of these symbol to exclude from the hash. This issue was kindly identified by @inulty-dfe in #162.
mec
added a commit
that referenced
this pull request
Oct 18, 2024
With a view mail we have to call `mail` twice, once to get the body of the email from the view and once again to override the format contents. We were passing in any custom headers for both calls which was resulting in duplicate headers in the message. By calling `mail` without the headers, we can get the body and not modify the headers, leaving that for the second call. Interestingly adding custom headers is pointless, as they will never end up in the email from Notify - we may want to stop messing with the headers at all - I think it is a hangover from the v1 implementation. This was kindly identified by @inulty-dfe in #162 who are testing the header Mail::Fields that are potentially duplicated.
mec
added a commit
that referenced
this pull request
Oct 18, 2024
The call to `except` here should not be an array as there is no array of these symbol to exclude from the hash. This issue was kindly identified by @inulty-dfe in #162.
mec
added a commit
that referenced
this pull request
Oct 18, 2024
With a view mail we have to call `mail` twice, once to get the body of the email from the view and once again to override the format contents. We were passing in any custom headers for both calls which was resulting in duplicate headers in the message. By calling `mail` without the headers, we can get the body and not modify the headers, leaving that for the second call. Interestingly adding custom headers is pointless, as they will never end up in the email from Notify - we may want to stop messing with the headers at all - I think it is a hangover from the v1 implementation. This was kindly identified by @inulty-dfe in #162 who are testing the header Mail::Fields that are potentially duplicated.
mec
added a commit
that referenced
this pull request
Oct 18, 2024
The call to `except` here should not be an array as there is no array of these symbol to exclude from the hash. This issue was kindly identified by @inulty-dfe in #162.
mec
added a commit
that referenced
this pull request
Oct 18, 2024
With a view mail we have to call `mail` twice, once to get the body of the email from the view and once again to override the format contents. We were passing in any custom headers for both calls which was resulting in duplicate headers in the message. By calling `mail` without the headers, we can get the body and not modify the headers, leaving that for the second call. Interestingly adding custom headers is pointless, as they will never end up in the email from Notify - we may want to stop messing with the headers at all - I think it is a hangover from the v1 implementation. This was kindly identified by @inulty-dfe in #162 who are testing the header Mail::Fields that are potentially duplicated.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
When upgrading the mail-notify gem to v2.0, our test suite began to fail. Our test suite asserts that accessing the mail headers returns a single
Mail::Field. When we upgraded we began seeing an array ofMail:Field.After some debugging we see that the mail-notify gem has introduced a potential unintended bug.
In an effort to begin the conversation and offer a path to a solution I have produced this PR. I don't think it's near ready and it should be used as a starting point to explore a better solution. And should be more thoroughly tested.
ActionMailer::Base#mail
Use of @_message instance variable in AM::B