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

Fix Bcc header missing with emails from conductor and test helpers #35992

Merged
merged 1 commit into from Jul 26, 2019

Conversation

jduff
Copy link
Contributor

@jduff jduff commented Apr 16, 2019

Summary

Bcc header was not being included with emails created by the actionmailbox conductor or the create_inbound_email_from test helpers. This change marks the Bcc header to be included when the email is encoded in both of those cases, using the include_in_headers features from the mail gem found here

@@ -28,6 +28,9 @@ def new_mail
end

def create_inbound_email(mail)
# Bcc header is not encoded by default
mail[:bcc].include_in_headers = true if mail[:bcc]
Copy link
Contributor

@kaspth kaspth Apr 16, 2019

Choose a reason for hiding this comment

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

Since this line isn't in create_and_extract_message_id! will that mean Action Mailbox can't capture any BCC fields in emails?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there an option we can pass to Mail.new in new_mail instead? Besides new_mail seems like the more suitable place for this.

Copy link
Contributor Author

@jduff jduff Apr 16, 2019

Choose a reason for hiding this comment

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

Thanks for taking a look @kaspth!

Since this line isn't in create_and_extract_message_id! will that mean Action Mailbox can't capture any BCC fields in emails?

The only reason this is a problem in these two places is because a Mail object is initialized and then serialized with to_s before passing it along, that doesn't occur with the other services afaik since the raw data forwarded by the service is stored without creating a Mail object first (I only tested with Mailgun which included the Bcc header correctly).

Is there an option we can pass to Mail.new in new_mail instead? Besides new_mail seems like the more suitable place for this.

The only reference I found to include_in_headers is within the Bcc field itself so I don't think it can be passed through Mail.new. Putting this in new_mail does seem suitable, I'll update the PR with that change.

@jduff
Copy link
Contributor Author

jduff commented Apr 24, 2019

@kaspth are there any other changes that I should make to this before it can be merged?

@rafaelfranca
Copy link
Member

Hey @jduff can you rebase this? We applied aab9f67 but I think we still need the change in the test helper.

@jduff
Copy link
Contributor Author

jduff commented Jul 26, 2019

@rafaelfranca I just rebased and pushed it up. I think the commit you referenced updated tests so if you want me to drop the tests I added as well let me know.

@rafaelfranca rafaelfranca merged commit a40da82 into rails:master Jul 26, 2019
rafaelfranca added a commit that referenced this pull request Jul 26, 2019
Fix Bcc header missing with emails from conductor and test helpers
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants