thoughtbot / griddler-cloudmailin

MIT License
2 stars 7 forks source link

Correctly handle emails where CloudMailin has been BCCed #3

Closed dominicsayers closed 9 years ago

dominicsayers commented 9 years ago

This pull request is designed to achieve two things:

  1. To make the CloudMailin adapter make better use of the JSON that CloudMailin sends today.
  2. To handle the case where CloudMailin is processing an email that has been BCCed to it

So as not to break Griddler itself, I've chosen to put the BCCed address in the bcc field of the normalized params.

I've minimized the changes I've made as far as possible. Here's a walkthrough of what I've had to change in adapter.rb :

  1. I've built the to array from [:headers][:To] instead of [:envelope][:to] because the envelope contains only the bare address and not the recipient's name. Also, Cloudmailin uses [:envelope][:to] for the BCCed recipient if any.
  2. For similar reasons, I've used [:headers][:From] instead of [:envelope][:from] because the envelope contains only the email address and not the name of the sender.
  3. The bcc field is only added to the normalized params if the CloudMailin recipient was BCCed.
  4. To establish whether the [:envelope][:to] field is a Bcc: or a To: recipient, we have to compare the email address with all the email addresses in [:headers][:To]. Unfortunately this means calling Griddler::EmailParser for each of them, an expensive but unavoidable step.

In the tests, I've modified the service params to reflect more accurately what CloudMailin actually sends. I've also added tests to show my expectations of how we handle the BCC case.

  1. The real-world [:envelope][:to] and [:envelope][:from] fields are always just a bare email address.
  2. I've added [:headers][:To] and [:headers][:From] to the service params because these are the fields we now use.
  3. In the tests where the recipient is BCCed, you can see that the [:headers][:To] field does not contain the [:envelope][:to] address

Here are two real-world messages from CloudMailin https://gist.github.com/dominicsayers/c40b08b7d581aa54e4d0 and https://gist.github.com/dominicsayers/67f4884b797a0f975396. They are intended to be very similar but were sent from different email clients. You can see that the message sent from Gmail has [:headers][:Bcc] set but the one from the OS X Mail app doesn't. Go figure.

dominicsayers commented 9 years ago

I see Hound has raised a number of issues although I copied the coding style of the existing code. I'm happy to change the code to suit Hound but that would introduce additional change.

dominicsayers commented 9 years ago

I'm closing this for two reasons:

  1. I now realise the Multipart format POSTed by CloudMailin handles attachments better, so I'm changing some of the assumptions here.
  2. A colleague's code review pointed out a flaw in my BCC logic that I need to fix.

I'll submit a revised PR shortly.