Feature/inline attachments

[jarrodmoldrich]

Changes proposed in this pull request

For parts with content-dispostion: inline its important to reorganize the multipart arrangement in the adapter. In particular it needs to be arrange liked this:

start multipart/mixed
  start multipart/related
    start multipart/alternative
      <text and html parts>
    end multipart/alternative
    <inline parts>
  end multipart/related
  <attachment parts>
end multipart/mixed

My changes reorganize the parts as shown above, while maintaining backwards compatibility. It also adds matching functions for querying for inline attachments as for regular attachments. This change was required for inline images and .ics files to work properly, particularly in Outlook webmail.

I've stopped short of adding tests to the test suite, and I'll wait for you to validate my approach first. I duplicated this code for kalys/bamboo_ses, where it currently functions well for my use cases: kalys/bamboo_ses#43

Note: This PR also deprecates #133 as the original order of text parts should be maintained

[michelson]

Hi there,

First, thanks for this library. I'm trying this branch, and it works ok; I'm testing it with a set of .eml files, and it is parsed correctly.

the only thing that is missing in this PR is the get_attachments function to be able to get both inline and attachments:

it is a little change though:

  def get_attachments(%Mail.Message{} = message) do
    walk_parts([message], {:cont, []}, fn message, acc ->
      case Mail.Message.is_any_attachment?(message) do
        true ->
          ##### THIS
          case Mail.Message.get_header(message, :content_disposition) do
            ["inline", {"filename", filename}] ->
              {:cont, List.insert_at(acc, -1, {filename, message.body})}
            ["attachment", {"filename", filename}] ->
              {:cont, List.insert_at(acc, -1, {filename, message.body})}
          end
          ###### 
        false ->
          {:cont, acc}
      end
    end)
    |> elem(1)
  end

[bcardarella]

@jarrodmoldrich thank you very much apoligies this went stale. I know it's a bit rediculous to ask after years but if you could just add regression tests for the additional behavior added I can merge this in.

[Hermanverschooten]

+1

[jarrodmoldrich]

Sorry, I've been off the radar, but I'm back! I'll have a look at this in the coming weeks.

[gernotkogler]

+1

[jarrodmoldrich]

@bcardarella Sorry for the delay. Added some tests. Let me know if you think this needs anything else.

[princemaple]

Hi @bcardarella do you think this one can go in now?

[bcardarella]

that'lll be @andrewtimberlake's call

[andrewtimberlake]

Thanks for this.
I’ve added a few comments/questions and a request for some tests to address my concerns.

[andrewtimberlake]

Would it not be better to work with a tuple of lists {[], [], []} to be more explicit about the number of lists being generated?

[andrewtimberlake]

Might it not be more explicit to use !Enum.empty?(list) because the behaviour of Enum.any?/1 and Enum.all?/1 are not necessarily intuitive when providing empty lists.

[andrewtimberlake]

Do we need multipart/mixed if there are only inline attachments? Shouldn’t it then just be wrapped in related?

[andrewtimberlake]

What if it’s only text/html with inline attachments and no text/plain? Then there’s no alternative, right?

[andrewtimberlake]

I think we need tests for

• only text/html with only inline attachments
• only text/html with only normal attachments
• text/html and text/plain with only inline attachments
• text/html and text/plain with only normal attachments

[andrewtimberlake]

@jarrodmoldrich Thanks for working on this. I ended up going with a different implementation but added you as a co-author on the commit

[jarrodmoldrich]

@andrewtimberlake Thanks for taking the lead on this.