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

Allow hashes to be submitted as headers #185

Closed
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@matthewbarram
Contributor

matthewbarram commented Nov 23, 2014

Hi!

Thanks for this awesome gem! I am finding it super useful. :-) I am pretty new to this whole thing, so please let me know if I have missed something or there is a much better way of doing this.

Goal

Allow hashes to be sent as headers from the adapter gems.

Side Note

I looked at the Mandrill, Sendgrid, PostMark gems and as far as I can see none of them submit header params.

Current Situation

If a hash is submitted when it is processed by the Mail::Header class it will throw an exception. My logic is that if the headers are already formatted hashes, then they do not need to be processed by Mail::Header.

Tests

I have added a test to confirm that hashes are dealt with correctly.

I look forward to your feedback.

Cheers
Matthew

@gabebw

This comment has been minimized.

Contributor

gabebw commented Nov 23, 2014

I looked at the Mandrill, Sendgrid, PostMark gems and as far as I can see none of them submit header params.

Does that mean none of the adapters send hashes that would be handled by this change? If so, why make this change?

@matthewbarram

This comment has been minimized.

Contributor

matthewbarram commented Nov 23, 2014

Hi Gabe

In the case of the Mandrill adapter (the one I am working with), I need to be able to access the headers, so I added the ability to send headers to the mandrill adapter in the same way as all the other params are sent. Which is extracting a hash from the JSON provided by Mandrill.

Once I added sending headers to there adapter, there was always an exception thrown by this gem and this is caused by the string processing of Mail::Header.

Once this is merged (if it is) I will get headers added to the Mandrill adapter (at least).

You can see how I have added headers into the Mandrill Adapter here - https://github.com/matthewbarram/griddler-mandrill/blob/master/lib/griddler/mandrill/adapter.rb#L21 and this implementation will only work with this change.

Converting the headers hash into a string and then converting it back doesn't make much sense to me. Especially since Mandrill (maybe the others) already formats the headers in a well formatted hash.

Cheers
Matthew

@matthewbarram

This comment has been minimized.

Contributor

matthewbarram commented Dec 15, 2014

There is now a PR waiting for this PR to be merged. See wingrunr21/griddler-mandrill/pull/12

@gabebw

This comment has been minimized.

Contributor

gabebw commented Dec 15, 2014

Hey, just so you know, this is a really excellent PR. You laid out the problem & the solution clearly, and added tests. I don't have time to look at this right this second, but I expect to get to it soon.

Thank you!

@joeyAghion

This comment has been minimized.

joeyAghion commented Jan 16, 2015

Thanks @matthewbarram. I was about to attempt the same change.

@gabebw I'd appreciate a merge when possible.

@@ -40,6 +40,7 @@ def self.extract_reply_body(body)
end
def self.extract_headers(raw_headers)
return raw_headers if raw_headers.is_a?(Hash)

This comment has been minimized.

@calebthompson

calebthompson Jan 16, 2015

Contributor

Please avoid trailing conditionals.

it 'handles a hash being submitted' do
header = {"X-Mailer" => "Airmail (271)",
"Mime-Version" => "1.0",
"Content-Type" => "multipart/alternative; boundary=\"546876a7_66334873_aa8\""}

This comment has been minimized.

@calebthompson

calebthompson Jan 16, 2015

Contributor

Please split hash parameters to their own lines, such as:

{
  foo: :bar,
  baz: :bat,
}
@calebthompson

This comment has been minimized.

Contributor

calebthompson commented Jan 16, 2015

The concept is sound, but I still worry a bit that we're just passing along whatever "headers" are provided to us. Passing through Mail::Header gave us some degree of certainty that we were getting valid/useful headers, but this means we're passing info into users' apps that could be wrong.

That said, it's been requested often.

@joeyAghion

This comment has been minimized.

joeyAghion commented Jan 16, 2015

An interesting scenario (that I just encountered): header keys are intended to be case-insensitive, so, e.g., both headers['Message-ID'] and headers['Message-Id'] resolve to the same value as long as headers is a Mail::Header. However, when Mandrill's headers hash is passed through, it must be accessed with an exactly matching key or risk getting nil.

A safer fix might be for griddler to handle either String or Hash inputs, but force them both through the Mail::Header conversion.

@@ -432,6 +432,15 @@ def email_with_params(params)
expect(headers[header_name]).to eq header_value
end
it 'handles a hash being submitted' do
header = {
"X-Mailer" => "Airmail (271)",

This comment has been minimized.

@houndci-bot

houndci-bot Jan 17, 2015

Use 2 spaces for indentation in a hash, relative to the start of the line where the left curly brace is.

header = {
"X-Mailer" => "Airmail (271)",
"Mime-Version" => "1.0"
}

This comment has been minimized.

@houndci-bot

houndci-bot Jan 17, 2015

Indent the right brace the same as the start of the line where the left brace is.

@@ -432,6 +432,15 @@ def email_with_params(params)
expect(headers[header_name]).to eq header_value
end
it 'handles a hash being submitted' do

This comment has been minimized.

@houndci-bot

houndci-bot Jan 17, 2015

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

This comment has been minimized.

@houndci-bot

houndci-bot Jan 17, 2015

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

This comment has been minimized.

@matthewbarram

matthewbarram Jan 17, 2015

Contributor

I am happy to change it, but all the other tests in this file use single quoted string.

This comment has been minimized.

@gabebw

gabebw Jan 17, 2015

Contributor

That's fine then.

@matthewbarram

This comment has been minimized.

Contributor

matthewbarram commented Jan 17, 2015

@calebthompson I agree with the point. I think it would depend on the use case.

I am only wanting to look for a particular header and ignoring the other ones - and I will only save the header that meets my criteria.

@wingrunr21

This comment has been minimized.

Collaborator

wingrunr21 commented May 6, 2015

@calebthompson @gabebw wanted to ping you guys on this. Just had another issue filed regarding headers that I believe is related.

If you are not comfortable with this change I'll need to investigate an alternative solution.

wingrunr21/griddler-mandrill/issues/16

@gabebw

This comment has been minimized.

Contributor

gabebw commented May 13, 2015

Thanks for the comments, everybody. I'm aware of this and I'll have time to look at it on Friday.

@xenapto-administrator

This comment has been minimized.

xenapto-administrator commented Jun 5, 2015

We are using the CloudMailin adapter, with CloudMailin's JSON format. This sends us a headers hash but no raw headers that can be processed by Mail::Header.

For the same reasons as other contributors to this thread, I'd like Griddler to allow a headers hash from the adapter.

@gabebw

This comment has been minimized.

Contributor

gabebw commented Jun 6, 2015

Thanks for your input, everyone. I've updated the code for the newest version of Griddler and merged this as the following commits:

@gabebw gabebw closed this Jun 6, 2015

@gabebw

This comment has been minimized.

Contributor

gabebw commented Jun 7, 2015

I also released Griddler 1.2.0 with this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment