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

Record the SES Message ID in the header :ses_message_id #25

Merged
merged 3 commits into from
Feb 13, 2019
Merged

Record the SES Message ID in the header :ses_message_id #25

merged 3 commits into from
Feb 13, 2019

Conversation

hughevans
Copy link
Contributor

@hughevans hughevans commented Feb 12, 2019

Following on from #10. Having the message ID is essential for tracking the deliverability (and other events).

Description of changes:

Instead of overriding message_id as was rejected in #10 I’ve placed the SES Message ID in a new header, so after you deliver a message through ActionMailer you have it available:

message = MyMailer.demo.deliver_now
message.header[:ses_message_id].value
>> "0000000000000000-1111111-2222-3333-4444-555555555555-666666"

This is adding the header after it’s sent only. I think it’s a reasonable conduit for passing this data back to the app in this instance.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@awood45
Copy link
Member

awood45 commented Feb 12, 2019

I actually like this plan. I'll timebox some time to brainstorm alternatives (say, tomorrow), and if I can't think of a better way, I'll merge and release this. I can't see a way that this would create problems, so I'm thinking I'm on board.

@awood45 awood45 self-assigned this Feb 12, 2019
assert_equal resp.context.params[:destinations], message.destinations
data = @mailer.deliver!(message).context.params
body = data[:raw_message][:data].to_s
body.gsub!("\r\nHallo", "ses-message-id: #{message_id}\r\n\r\nHallo")
Copy link
Member

Choose a reason for hiding this comment

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

I'd actually like the test to mimic the behavior pattern you showed in the PR description. Perhaps retrieve the message body and message headers like a user would?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, that’s pushed.

class TestMailer < ActionMailer::Base
layout nil

def deliverable(body:, from:, subject:, to:)
Copy link
Member

Choose a reason for hiding this comment

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

Please use an 'opts' hash rather than keyword args here. We still maintain test compat with older Ruby versions, for customers with LTS of deprecated versions, in this library. Those tests are breaking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries, that’s done.

@awood45 awood45 merged commit a479da4 into aws:master Feb 13, 2019
@awood45
Copy link
Member

awood45 commented Feb 13, 2019

Thank you! I'll release this sometime tomorrow.

@awood45
Copy link
Member

awood45 commented Feb 14, 2019

Released as version 2.1.0

@hughevans hughevans deleted the record-message-id branch February 14, 2019 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants