thoughtbot / griddler

Simplify receiving email in Rails (deprecated)
http://griddler.io/
MIT License
1.38k stars 199 forks source link

the adapter for mailgun does not parse the several parts of the recipient email addresses correctly #106

Closed mepster closed 10 years ago

mepster commented 10 years ago

Thanks bradpauly for doing the mailgun adapter (here: https://github.com/thoughtbot/griddler/pull/84 ). Could I submit a small bug report? It has to do with sending emails to "full" email addresses. The bug occurs with 1 or multiple recipients but I will do 2 recipients here:

If you send an email to (two recipients, each with a "full" address): Monkey Q. Baby mep@stone-house.org Foo B. Baz mp7@stone-house.org

the current mailgun adapter returns an email.to (inside the EmailProcessor self.process(email) method) of: [{:token=>"mp7", :host=>"stone-house.org", :email=>"mp7@stone-house.org", :full=>"mp7@stone-house.org", :name=>nil}, {:token=>"mep", :host=>"stone-house.org", :email=>"mep@stone-house.org", :full=>" mep@stone-house.org", :name=>nil}]

The problem is that the :name is nil, and :full is not correct (:full should be the whole thing with the human-readable name plus the email address as submitted).

Instead, we want email.to to be like this: [{:token=>"mep", :host=>"stone-house.org", :email=>"mep@stone-house.org", :full=>"\"Monkey Q. Baby\" mep@stone-house.org", :name=>"\"Monkey Q. Baby\""}, {:token=>"mp7", :host=>"stone-house.org", :email=>"mp7@stone-house.org", :full=>"\"Foo B. Baz\" mp7@stone-house.org", :name=>"\"Foo B. Baz\""}]

You can get that result with a single line change in mailgun_adapter.rb. Change line 28 from: params[:recipient].split(',') to: params["To"].split(',').map(&:strip)

Note, I also added a :strip because, if you notice in the output of the current version of the mailgun adapter, there is a leading space before the email address in :full=>" mep@stone-house.org" .

Thanks!

P.S. Note, this bug is only evident when you set config.to = :hash in Griddler.configure .

bradpauly commented 10 years ago

@mepster Sorry about the bug, I'll try to take a look this week.

mepster commented 10 years ago

no problem! Thanks for doing that adapter. I think my suggested fix will do it, you can have a look.

Cheers,

Mike

On 12/30/13, 7:00 PM, Brad Pauly wrote:

@mepster https://github.com/mepster Sorry about the bug, I'll try to take a look this week.

— Reply to this email directly or view it on GitHub https://github.com/thoughtbot/griddler/issues/106#issuecomment-31381727.

calebhearth commented 10 years ago

@mepster It looks like you've already got a fix in mind. Do you want to add a failing test and the code to get it passing?

calebhearth commented 10 years ago

Ping @mepster @bradpauly

bradpauly commented 10 years ago

@calebthompson thanks for the ping, taking a look right now.

bradpauly commented 10 years ago

@mepster after looking into this some more I think it might be worth taking an approach similar to what @r38y did for https://github.com/thoughtbot/griddler/pull/108

The reason being that mailgun support has told me not to rely on the 'To' param. It isn't in their docs as something they pass even though they currently are. 'message-headers' is in the docs so maybe falling back to that if 'To' isn't sent.

For reference this is what I see in my log files when sending to multiple To addresses.

https://gist.github.com/bradpauly/250e0a28fc7d5c71faf4

calebhearth commented 10 years ago

Merged.