Inline attachments #159

theodowling · 2017-10-16T17:46:16Z

Hi there

I needed inline-attachment support for a project, so I added it for myself, and thought it might make a good PR.

Inline-Attachment support has been added to Sendgrid, Mandrill, Mailgun, Postmark and SparkPost adapters.

Update: SMTP adapter also supported

At its core you just need to specify type: :inline when creating an Attachment, and all the rest will happen automatically.

Attachment.new("/data/file.png", type: :inline)

And the ContentID of the attachment will default to filename so in the above case, you can refer to the inline-attached image by using:

<img src="cid:file.png">

I've tested using all of the adapters listed above. Please let me know if you have any comments, or need me to change anything.

Kind regards
Theo

…dicate new addition

princemaple · 2017-10-16T20:18:21Z

Hi Theo,

I'm on my phone.
I just want to quickly say thank you for the great effort!

I've scanned through the PR, it's mostly good. I'll go through it again on my desktop soon. Probably just nits so I might simply merge this and tweak the styles / implementation details myself.

Po

princemaple

Hi @theodowling

This is my first round review on the PR.
The comments are for both myself and @stevedomin to think about. You are welcome to address the nitpicks if you'd like to. If not, I'll clean it up before I merge.

@stevedomin the main things I want to talk about are type: :inline|:attachment vs inline: true|false, and whether to special case inline attachments and perform checks on content types, etc.

@theodowling out of curiosity, have you looked into SMTP inline attachments?

princemaple · 2017-10-16T21:03:31Z

lib/swoosh/adapters/mailgun.ex

@@ -86,10 +88,23 @@ defmodule Swoosh.Adapters.Mailgun do

  defp prepare_attachments(body, %{attachments: []}), do: body
  defp prepare_attachments(body, %{attachments: attachments}) do
-    Map.put(body, :attachments, Enum.map(attachments, &prepare_file(&1)))
+    normal_attachments = Enum.filter(attachments, fn(%Swoosh.Attachment{type: type}) -> type == :attachment end)
+    inline_attachments = Enum.filter(attachments, fn(%Swoosh.Attachment{type: type}) -> type == :inline end)


Enum.split_with

Matching struct type in private functions is not necessary

Agreed. Thanks for recommending Enum.split_with. I'll fix this on my side.

princemaple · 2017-10-16T21:05:36Z

lib/swoosh/adapters/mailgun.ex

    {:multipart,
     params
     |> Map.drop([:attachments])
+     |> Map.drop([:inline])


Map.drop([:attachments, :inline])

Thanks. I'll update this.

princemaple · 2017-10-16T21:10:48Z

lib/swoosh/adapters/mandrill.ex

+
+    inline_attachments = Enum.map(inline_attachments, fn %Swoosh.Attachment{content_type: type, path: path, filename: filename} ->
+      content = path |> File.read! |> Base.encode64
+      %{type: type, name: "#{filename}", content: content}


I'd extract this common part into a separate function

"#{filename}" is just filename?

This has also been updated. Thanks.

princemaple · 2017-10-16T21:12:59Z

lib/swoosh/attachment.ex

@@ -3,7 +3,7 @@ defmodule Swoosh.Attachment do
  Struct representing an attachment in an email.
  """

-  defstruct filename: nil, content_type: nil, path: nil
+  defstruct filename: nil, content_type: nil, path: nil, type: nil


Maybe just inline: false (default to attachment)?

Should we do something special about inline attachments? e.g. validating it's an image?

I'm ok with using type. Slightly more future-proof.

@princemaple: regarding validation of inline attachments, do you know if the spec says anything about accepted file type for inline attachments?

I actually failed to search for the inline attachment related specs. :(

princemaple · 2017-10-16T21:15:29Z

lib/swoosh/email.ex

@@ -464,23 +464,30 @@ defmodule Swoosh.Email do
  as an argument. If you give a path we will detect the MIME type and determine the filename
  automatically.

+  You can also send an inline-attachment used for embedding images in the body of emails by specifying `type: "inline"`


Currently it's :inline not "inline"
(might change, see comment above)

theodowling · 2017-10-17T06:38:07Z

Thanks @princemaple - I'll wait for you and @stevedomin to discuss the handling of type: :inline|:attachment vs inline: true|false. I myself wasn't sure which approach to take.

I'll also commit some small fixes/changes on the back of your comments above.

I haven't looked at the SMTP implementation, but I can do that also today.

stevedomin

@theodowling thank you so much for this, great work!

Pointed out a few things in comment but otherwise this is good to go.

stevedomin · 2017-10-18T08:45:37Z

lib/swoosh/attachment.ex

@@ -3,7 +3,7 @@ defmodule Swoosh.Attachment do
  Struct representing an attachment in an email.
  """

-  defstruct filename: nil, content_type: nil, path: nil
+  defstruct filename: nil, content_type: nil, path: nil, type: nil


I'm ok with using type. Slightly more future-proof.

@princemaple: regarding validation of inline attachments, do you know if the spec says anything about accepted file type for inline attachments?

stevedomin · 2017-10-18T08:47:19Z

mix.lock

@@ -1,20 +1,20 @@
-%{"bunt": {:hex, :bunt, "0.2.0", "951c6e801e8b1d2cbe58ebbd3e616a869061ddadcc4863d0a2182541acae9a38", [:mix], [], "hexpm"},


New version of Hex?

More like an older version. @theodowling Please exclude the mix.lock change :) You might want to upgrade with mix local.hex

Thanks for the heads-up. I've reverted change to mix.lock

stevedomin · 2017-10-18T08:49:16Z

lib/swoosh/email.ex

@@ -464,23 +464,30 @@ defmodule Swoosh.Email do
  as an argument. If you give a path we will detect the MIME type and determine the filename
  automatically.

+  You can also send an inline-attachment used for embedding images in the body of emails by specifying `type: "inline"`


theodowling · 2017-10-18T11:44:36Z

Thanks for the Approval.
I'm still working on adding inline-attachment support to SMTP adapter. Will push that as soon as I get it working 😄

princemaple · 2017-10-18T11:58:21Z

@theodowling thanks for looking into it!

stevedomin · 2017-10-18T12:28:04Z

@theodowling nice work on the SMTP attachment!

princemaple

Hi @theodowling

Thanks a lot for this PR.
This is my final round of review. It's 100% nitpicks.
Feel free to ignore them if you can't be bothered :) , I'll fix them before/after merge.

@stevedomin a final 👍 ?

princemaple · 2017-10-21T00:41:02Z

lib/swoosh/adapters/mailgun.ex

@@ -86,13 +88,17 @@ defmodule Swoosh.Adapters.Mailgun do

  defp prepare_attachments(body, %{attachments: []}), do: body
  defp prepare_attachments(body, %{attachments: attachments}) do
-    Map.put(body, :attachments, Enum.map(attachments, &prepare_file(&1)))
+    {normal_attachments, inline_attachments} = Enum.split_with(attachments, fn(%Swoosh.Attachment{type: type}) -> type == :attachment end)


separate lines

{..} = ...

Remove Swoosh.Attachment and unnecessary braces, fn %{...} -> ... end is sufficient

princemaple · 2017-10-21T00:42:17Z

lib/swoosh/adapters/mandrill.ex

-      "content" => &1.path |> File.read! |> Base.encode64
-    }))
+    normal_attachments = prepare_attachments_structure(Enum.filter(attachments, fn(attachment) -> attachment.type == :attachment end))
+    inline_attachments = prepare_attachments_structure(Enum.filter(attachments, fn(attachment) -> attachment.type == :inline end))


Enum.split_with (might as well just copy paste the change from mailgun)

princemaple · 2017-10-21T00:44:57Z

lib/swoosh/adapters/mandrill.ex

+  end
+
+  defp prepare_attachments_structure(attachments) do
+    Enum.map(attachments, fn %Swoosh.Attachment{content_type: type, path: path, filename: filename} ->


remove Swoosh.Attachment

princemaple · 2017-10-21T00:46:20Z

lib/swoosh/adapters/postmark.ex

-      "ContentType" => &1.content_type,
-      "Content" => &1.path |> File.read! |> Base.encode64
-    }))
+    Map.put(body, "Attachments", Enum.map(attachments, fn(attachment) -> prepare_attachment(attachment) end))


Remove unnecessary braces in fn arguments

princemaple · 2017-10-21T00:46:47Z

lib/swoosh/adapters/postmark.ex

+  defp prepare_attachment(attachment) do
+    case attachment.type do
+      :inline -> %{"Name" => attachment.filename,
+                    "ContentType" => attachment.content_type,


indentation is off

princemaple · 2017-10-21T00:47:34Z

lib/swoosh/adapters/sendgrid.ex

@@ -106,9 +106,12 @@ defmodule Swoosh.Adapters.Sendgrid do

  defp prepare_attachments(body, %{attachments: []}), do: body
  defp prepare_attachments(body, %{attachments: attachments}) do
-    attachments = Enum.map(attachments, fn %{content_type: type, path: path, filename: filename} ->
+    attachments = Enum.map(attachments, fn %Swoosh.Attachment{content_type: content_type, path: path, filename: filename, type: type} ->


Remove Swoosh.Attachment

princemaple · 2017-10-21T00:48:05Z

lib/swoosh/adapters/sparkpost.ex

@@ -57,6 +57,8 @@ defmodule Swoosh.Adapters.SparkPost do
    html_body: html,
    attachments: attachments
  } = email) do
+    normal_attachments = Enum.filter(attachments, fn(attachment) -> attachment.type == :attachment end)
+    inline_attachments = Enum.filter(attachments, fn(attachment) -> attachment.type == :inline end)


theodowling · 2017-10-21T06:49:31Z

Hi @princemaple, thanks for the comments. I've made the changes as requested.
Please let me know if there is anything else we need to update/change. I'm happy to help out 👍

stevedomin · 2017-10-22T17:17:26Z

I'm 👍, feel free to merge if you feel it's good to go @princemaple

princemaple · 2017-10-22T22:47:45Z

@theodowling thank you very much for the great work. Merging now!

blackham · 2021-04-30T04:12:48Z

You would do well to update your documentation to mention the bit about the cid name being the filename for inline attachments. Even better to provide a simple example. (searching github bugs is a poor replacement for docs)

princemaple · 2021-04-30T04:21:42Z

@blackham It's not very helpful to comment on a PR closed over 3 years ago.

It'd be better you filed an issue to tell us what troubled you, and how we can improve. Basically what you did with the comment. Even better you could send a PR to address the issue.

blackham · 2021-04-30T22:45:33Z

Sorry it was late. Let me try and explain and give you example code you might be able to use in the doc. Also sorry for commenting on an old bug.

It would have saved me some time last night if the text "Inline attachments by default use their filename (or basename of the path if filename is not specified) as cid, in relevant adapters." found in https://hexdocs.pm/swoosh/Swoosh.Attachment.html also gave a very basic example.

Just this would have been enough for the light bulb to go off.
<img src='cid:image.png'>

Or better, I could have skipped most everything else in the docs and just looked at this example:

   new()
    |> to({user.name, user.email})
    |> from({"Red Skull", "[email protected]"})
    |> subject("OTP QR Code")
    |> html_body("<h1>Hello #{user.name}</h1> Here is your QR Code <img src='cid:qrcode.png'>")
    |> text_body("Hello #{user.name}. Please find your QR Code attached.\n")
    |> attachment(
      Swoosh.Attachment.new(
        {:data, Avengers.Accounts.OTP.qrcode_png(user)},                                                                                     
        filename: "qrcode.png",
        content_type: "image/png",
        type: :inline)
    )
    |> Avengers.Mailer.deliver

Maybe OTP (One Time Password) isn't the best in that example. Using "OTP" for anything but Open Telecom Platform in Elixir might be against some peoples religion. But TwoFA.qrcode_png(user) is ugly. TwoFactorAuth.qrcode_png(user) or TFA.qrcode_png(user)? In any case seeing the <img src='cid:qrcode.png'> is where my oh yeah, I'm stupid kicked in.

BTW, Great job on a mail system!

princemaple · 2021-04-30T23:00:46Z

Thanks for that. I'll update the docs to include your example.

Documentation PRs are always welcome.

princemaple · 2021-05-01T03:14:05Z

Pushed more docs updates.

theodowling added 17 commits October 11, 2017 15:44

Add test to allow attachment type to be specified

744cb50

Add ability to specify attachment type. Including updating docs to in…

a20bba8

…dicate new addition

Add inline-attachment to Postmark adapter

1e5a4e6

Add inline-attachment to Sendgrid adapter

f574a34

Add inline-attachment to Mandrill adapter

9349065

Update Attachment type to use an Atom instead

4091c89

Add inline-attachment to Mailgun adapter

17a2f46

Add inline-attachment to Sparkpost adapter

a6fefc3

Fix tests for sparkpost adapter

3697dc7

Fix issues with Mandrill adapter

5ae9ecb

Fixes to Mandrill & Sendgrid inline-attachments

ea5562c

Updates to inline-attachment handling

2b7ffb0

Fix inline content-disposition for Mailgun Adapter

affeab8

Remove debugging logging

388513e

Remove IEx import not used.

838ec2d

Credo consistency cleanup

353b954

Fix inline-attachment test

8fdaef7

theodowling mentioned this pull request Oct 16, 2017

Support inline attachments #122

Closed

Merge branch 'master' into inline-attachments

10b20c9

princemaple requested a review from stevedomin October 16, 2017 20:59

princemaple reviewed Oct 16, 2017

View reviewed changes

Code styling updates

14b5fac

stevedomin approved these changes Oct 18, 2017

View reviewed changes

Revert mix.lock

f9bc125

theodowling added 2 commits October 18, 2017 14:11

Add inline-attachment support to SMTP

cbe39b2

Remove left over debugging from SMTP adapter

1915488

Update example in email.ex

4d7cf15

princemaple approved these changes Oct 21, 2017

View reviewed changes

Code styling updates

b55fa43

Merge branch 'master' into inline-attachments

f827bca

princemaple merged commit e5c2711 into swoosh:master Oct 22, 2017

		@@ -1,20 +1,20 @@
		%{"bunt": {:hex, :bunt, "0.2.0", "951c6e801e8b1d2cbe58ebbd3e616a869061ddadcc4863d0a2182541acae9a38", [:mix], [], "hexpm"},

Inline attachments #159

Inline attachments #159

Conversation

theodowling commented Oct 16, 2017 • edited Loading

princemaple commented Oct 16, 2017

princemaple 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

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

theodowling commented Oct 17, 2017

stevedomin 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

theodowling commented Oct 18, 2017

princemaple commented Oct 18, 2017

stevedomin commented Oct 18, 2017

princemaple 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theodowling commented Oct 21, 2017

stevedomin commented Oct 22, 2017

princemaple commented Oct 22, 2017

blackham commented Apr 30, 2021 • edited Loading

princemaple commented Apr 30, 2021

blackham commented Apr 30, 2021 • edited Loading

princemaple commented Apr 30, 2021

princemaple commented May 1, 2021

theodowling commented Oct 16, 2017 •

edited

Loading

blackham commented Apr 30, 2021 •

edited

Loading

blackham commented Apr 30, 2021 •

edited

Loading