Attachments: Support file data in attachment struct + mailgun attachment support #292

gitviola · 2017-05-23T07:18:59Z

As discussed in #286 @sbrink and I were working on an improved version of the attachment support:

Changes

supports |> put_attachment %Attachment{filename: "event.ics", data: "raw content..", content_type: "text/calendar"}
sets Attachment.data when using |> put_attachment "/hi.jpg" or |> put_attachment %Plug{..}
supports attachments in the mailgun and test adapter
updates the mandrill adapter to use the attachment.data

Todo

tests for the mailgun adapter (attachment support)

randycoulman · 2017-05-31T16:20:56Z

How close is this PR to being ready to merge? I'm working on a project that needs to generate and attach vCards to e-mails, so being able to create an attachment from data rather than a file would be super helpful.

We use the SendGrid adapter, so I'm starting to work on a PR to add attachment support for that adapter today. I'd love it if I could base my work off of the code in this PR.

Thanks for all your work on this library!

paulcsmith

Thank you so much for you work on this!

The description says:

# Supports...
|> put_attachment %Attachment{filename: "event.ics", data: "raw content..", content_type: "text/calendar"}

But I don't see documentation for that or a test, but maybe I missed it. If docs or tests are missing, could you add those? I think once that's in I can merge this and release a new RC.

Thank you so much!

paulcsmith · 2017-06-07T18:30:36Z

lib/bamboo/adapters/mandrill_adapter.ex

@@ -112,7 +112,7 @@ defmodule Bamboo.MandrillAdapter do
      %{
        name: att.filename,
        type: att.content_type,
-        content: Base.encode64(File.read!(att.path))
+        content: Base.encode64(att.data)


While we're in this PR, could you rename att to attachment?

paulcsmith · 2017-06-07T18:30:44Z

lib/bamboo/adapters/test_adapter.ex

@@ -52,4 +52,7 @@ defmodule Bamboo.TestAdapter do
  def clean_assigns(email) do
    %{email | assigns: :assigns_removed_for_testing}
  end
+
+  @doc false
+  def supports_attachments?, do: true


Good catch!

paulcsmith · 2017-06-07T18:31:10Z

lib/bamboo/attachment.ex

-    Attachment.new("/path/to/attachment.png", filename: "image.png")
-    Attachment.new("/path/to/attachment.png", filename: "image.png", content_type: "image/png")
-    Attachment.new(params["file"]) # Where params["file"] is a %Plug.Upload
+    Attachment.new_from_file("/path/to/attachment.png")


Can you change these to Bamboo.Attachment so it's clear this is coming from Bamboo?

Also I think new is better here since there isn't a second function. Alternatively you could add new_from_data so there were two different functions. I'd probably prefer just new though

paulcsmith · 2017-06-07T18:36:27Z

lib/bamboo/email.ex

@@ -220,7 +220,10 @@ defmodule Bamboo.Email do
      #...
    end
  """


I think adding docs for sending data would be awesome here

paulcsmith · 2017-06-07T18:37:23Z

lib/bamboo/attachment.ex

-    Attachment.new("/path/to/attachment.png", filename: "image.png")
-    Attachment.new("/path/to/attachment.png", filename: "image.png", content_type: "image/png")
-    Attachment.new(params["file"]) # Where params["file"] is a %Plug.Upload
+    Attachment.new_from_file("/path/to/attachment.png")


Also I think new is better here since there isn't a second function. Alternatively you could add new_from_data so there were two different functions. I'd probably prefer just new though

randycoulman · 2017-06-07T18:53:03Z

I tried this branch on something I was working on, and I noticed there were a number of test failures that will need to be resolved. It doesn't look like CircleCI is running builds right now, so there's no indication of a problem here.

From the test failures, I couldn't tell if the code was doing the right thing and the tests needed to be changed, or if the tests were right and the code was wrong. I suspect the latter, but I'm not confident enough in that to be able to suggest fixes.

paulcsmith · 2017-06-07T18:55:45Z

Yeah I'm not sure what's up with Circle. I'll work on that. Busy with client work now, but hopefully in the next few days. In the meantime I can run it locally before merging to make sure it all works ok. @schurig are you seeing failures locally?

gitviola · 2017-06-08T13:37:14Z

I'll do it asap! I remember that I fixed all tests. But I can't confirm at the moment.

gitviola · 2017-06-13T21:28:05Z

I haven't found a way to test the attachments for the mailgun adapter. Can anyone help me with it?

It uses multipart as content type and I can't get the attachment by calling assert_receive {:fake_mailgun, struct} in the test.

paulcsmith · 2017-07-10T17:11:37Z

@schurig I just came to check in and didn't realize you left that comment. Sorry about that. I would remove the changes from the MailgunAdapter and it's tests and instead do a separate PR for those. That way we can merge in the attachments changes and work on the adapters separately. Does that sound good?

gitviola · 2017-07-13T07:35:24Z

@paulcsmith that sounds good!!

gitviola · 2017-07-13T08:59:02Z

@paulcsmith ready for a 2nd review.

paulcsmith

Just one small comment. Looks awesome. Thank you!

paulcsmith · 2017-07-13T14:32:31Z

lib/bamboo/email.ex

+      #...
+    end
+  """
+  def put_attachment(%__MODULE__{attachments: attachments} = email, %Attachment{filename: _filename, data: _data} = attachment) do


I believe you can shorten the pattern match of the attach meant to %Attachment{} since you wouldn't be able to initialize that struct without those fields

You actually can. Then the values are set to nil because we don't specify anything as enforced keys:

iex(1)> %Bamboo.Attachment{} %Bamboo.Attachment{content_type: nil, data: nil, filename: nil, path: nil}

But you're right - since we don't check the actual value it doesn't make sense for that check. But it would only make sense to put pass an Attachment struct with both keys (filename and data) specified in there. Do you think we should throw an error if one of it is empty?

Maybe we have 2 functions. One for the happy path and another for attachments that are missing data and filename

def put_attachment(_email, %Attachment{filename: nil, data: nil} = attachment) do raise "You must pass a valid attachment, instead got: #{inspect attachment}" end def put_attachment(%__MODULE__{attachments: attachments} = email, %Attachment{} = attachment) do # happy path! end

I think this would cover the common error case. We can add different error cases later if people run into things often. Thoughts?

@paulcsmith I've made the error messages more specific since it would still jump into the happy path if one of the params is nil. What do you think?

paulcsmith

Just one small comment. I like the two error messages 👍

paulcsmith · 2017-07-13T19:25:57Z

lib/bamboo/email.ex

+    raise "You must provide a filename for the attachment."
+  end
+  def put_attachment(%__MODULE__{attachments: attachments} = email, %Attachment{data: nil}) do
+    raise "The attachment must contain data."


Can you include the attachment in both error message so that it's easier to see what they passed in?

raise "The attachment must contain data, instead got: #{inspect attachment}"

Good point! Done. Also cleaned up a bit. There were some warnings about not used variables.

paulcsmith · 2017-07-13T20:30:14Z

Wonderful work! Thanks for your first contribution to Bamboo!

randycoulman mentioned this pull request Jun 1, 2017

Support attachments in SendGridAdapter #294

Merged

Ghouli mentioned this pull request Jun 2, 2017

Add attachment support fewlinesco/bamboo_smtp#35

Merged

paulcsmith reviewed Jun 7, 2017

View reviewed changes

add attachment support

831f7dd

paulcsmith reviewed Jul 13, 2017

View reviewed changes

raise error on missing attachment fields

ff23a3e

paulcsmith reviewed Jul 13, 2017

View reviewed changes

make error messages more specific, cleanup

65eef6b

paulcsmith merged commit d09da6a into beam-community:master Jul 13, 2017

gitviola deleted the develop branch July 13, 2017 20:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Attachments: Support file data in attachment struct + mailgun attachment support #292

Attachments: Support file data in attachment struct + mailgun attachment support #292

gitviola commented May 23, 2017

randycoulman commented May 31, 2017

paulcsmith left a comment

paulcsmith Jun 7, 2017

paulcsmith Jun 7, 2017

paulcsmith Jun 7, 2017

paulcsmith Jun 7, 2017

paulcsmith Jun 7, 2017

paulcsmith Jun 7, 2017

randycoulman commented Jun 7, 2017

paulcsmith commented Jun 7, 2017

gitviola commented Jun 8, 2017

gitviola commented Jun 13, 2017

paulcsmith commented Jul 10, 2017

gitviola commented Jul 13, 2017

gitviola commented Jul 13, 2017

paulcsmith left a comment

paulcsmith Jul 13, 2017

gitviola Jul 13, 2017 •

edited

paulcsmith Jul 13, 2017

gitviola Jul 13, 2017

paulcsmith left a comment

paulcsmith Jul 13, 2017

gitviola Jul 13, 2017

paulcsmith commented Jul 13, 2017

@@ @@ -220,7 +220,10 @@ defmodule Bamboo.Email do @@
                     #...
                   end
                 """

Attachments: Support file data in attachment struct + mailgun attachment support #292

Attachments: Support file data in attachment struct + mailgun attachment support #292

Conversation

gitviola commented May 23, 2017

Changes

Todo

randycoulman commented May 31, 2017

paulcsmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

randycoulman commented Jun 7, 2017

paulcsmith commented Jun 7, 2017

gitviola commented Jun 8, 2017

gitviola commented Jun 13, 2017

paulcsmith commented Jul 10, 2017

gitviola commented Jul 13, 2017

gitviola commented Jul 13, 2017

paulcsmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gitviola Jul 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulcsmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulcsmith commented Jul 13, 2017

gitviola Jul 13, 2017 •

edited