search

mailman-elixir / mailman

Mailman provides a clean way of defining mailers in your Elixir applications
https://github.com/mailman-elixir/mailman
Other
203 stars 73 forks source link

Update to gen_smpt 1.0.1, some small bug fixes #90

Closed skosch closed 3 years ago

skosch commented 4 years ago

This fixes compatibility issues with gen_smtp 0.15, which will pave the way for allowing multiple emails to be sent via a persistent connection with the relay. This is useful when sending many emails at once.

It also fixes an encoding issue with Reply-To headers.

@kamilc if this looks ok to you, please go ahead and merge and publish to hex.pm. Thanks :)

kamilc commented 4 years ago

Hey @skosch - awesome sauce! I'll have a look most prolly first thing tomorrow. Thanks!

kamilc commented 4 years ago

@skosch not sure why but I'm seeing 14 tests failing here.

Seems that it's somewhat related to rendering and what :mimemail.encode is expecting. Here's an excerpt showing one of those tests:

1) test should load text part of email from external file (MailmanTest)
     test/mailman_test.exs:233
     ** (FunctionClauseError) no function clause matching in :proplists.get_value/3

     The following arguments were given to :proplists.get_value/3:

         # 1
         "content-type-params"

         # 2
         %{}

         # 3
         []

     code: {:ok, message} = MyApp.ExternalTextMailer.deliver(email_with_external_text)
     stacktrace:
       (stdlib 3.13) proplists.erl:215: :proplists.get_value/3
       (gen_smtp 0.15.0) /home/kamil/projects/mailman/deps/gen_smtp/src/mimemail.erl:653: :mimemail.ensure_content_headers/7
       (gen_smtp 0.15.0) /home/kamil/projects/mailman/deps/gen_smtp/src/mimemail.erl:155: :mimemail.encode/2
       (mailman 0.4.3) lib/mailman.ex:43: Mailman.deliver/3
       test/mailman_test.exs:235: (test)
skosch commented 4 years ago

Huh, thanks so much for catching that. Somehow I didn't get these last time, but I can reproduce them now. :thinking: let me look into that

skosch commented 4 years ago

Ah, I see what happened now. My bad! I made this work based on https://github.com/gen-smtp/gen_smtp/issues/188, and when they recently released their new 0.15 I had assumed that this change was included in that ... but turns out they excluded it, as it's a breaking change.

Let's hold off on this until their 1.0 is out. Again, sorry about the noise

skosch commented 4 years ago

Good news ­– the folks at gen_smtp were very quick and released their v1.0 yesterday! Now this PR should work. Could you take another look?

skosch commented 3 years ago

@kamilc ping :)

Could you take a look at this one and #91, and merge and publish if it looks ok to you? Thanks!

kamilc commented 3 years ago

@skosch Oh my - apologies for keeping you waiting for so long! In my defense - I've been massively swamped lately. Thanks for merging that in!

kamilc commented 3 years ago

I'll push it to hex soon

skosch commented 3 years ago

Hey, no worries. I figured I've been using it in production long enough that merging would be ok. I just fixed the CI script as well; you can push to hex now. Thanks!

kamilc commented 3 years ago

It's just been published @skosch - https://hex.pm/packages/mailman Happy Friday!