Extend ext_smtp.py

[TomFreudenberg]

1. Respect empty CC and BCC and do not deliver <> to SMTP Server
2. Allow TLS also on non-SSL sessions
3. Turn on debugging before TLS to see that log as well
4. Check for parameter files and empty if missed
5. Loop through files and append to attachments
6. Allow Attachment Disposition Name differ from Filename by path/filename.ext=attname.ext
7. Allow body to send in text and html by list or dict

---

Covered Issues:

1. ext_smtp.py TLS without SSL #667
2. ext_smtp.py sends empty (wrong) email addresses in server dialog #668

[TomFreudenberg]

Hi @derks

please let me know if this does fit for you.

Cheers
Tom

[derks]

@TomFreudenberg I appreciate the PR, and don't see any immediate issues that would block this from merging in, however there is no test coverage. Ideally, running make test should show 100% coverage. Do you have the capacity to add unit tests that cover?

[derks]

@TomFreudenberg I just realized covering this will be difficult due to the fact that the server needs to support TLS to test, and unit tests need to be portable. I will probably need to spend time analyzing how to 1. test TLS local and 2. test TLS with Travis.

If you have ideas, please share... but I expect I'll need to spend time on it. Thank you either way.

[TomFreudenberg]

Hi @derks I am the maintainer of midi-smtp-server and there we have to check TLS as well. If you like, I can prepare some tests from or with that component.

https://github.com/4commerce-technologies-AG/midi-smtp-server/blob/master/test/construct/certificate_integration_test_class.rb#L20

[derks]

Thanks @TomFreudenberg I'll spend some time on this... I typically use Mailhog for local dev, and just need to test some things for Travis CI testing.

[derks]

@TomFreudenberg I've reviewed the changes and all appears to line up and make sense to me, except this chunk:

        # add body as text and or or as html
        partText = None
        partHtml = None
        if isinstance(body, str):
            partText = MIMEText(body)
        elif isinstance(body, list):
            if len(body) >= 1:
                partText = MIMEText(body[0])
            if len(body) >= 2:
                partHtml = MIMEText(body[1], 'html')
        elif isinstance(body, dict):
            if 'text' in body:
                partText = MIMEText(body['text'])
            if 'html' in body:
                partHtml = MIMEText(body['html'], 'html')
        if partText:
            msg.attach(partText)
        if partHtml:
            msg.attach(partHtml)

Is the support for string/list/dict body based on any sort of standard (regarding Python/SMTP, etc)... or was this just a nice-to-have based on your use cases? Supporting multiple argument types can get messy... so want to be clear.

I see this as a separate feature request and would like to separate that part out so that I fully understand the use-case/requirements, and add proper testing and documentation for it.

Correct me if i'm wrong, but without mixing in the "how".... the feature request is:

• Support for sending HTML type body
• Support for sending both TEXT and HTML type body in a single message

Is that right?

[TomFreudenberg]

Hey @derks

thanks for all your comments and effort on this.

Yes:

the feature request is:

Support for sending HTML type body
Support for sending TEXT and HTML alternative type body

To make it sure from the SMTP standard, you "never should" send out a HTML body message without a TEXT part even that this does work and is used a lot by marketing mail pages.

The reason is that, a TEXT only client will see just an empty message if only a HTML body was attached.

---

My idea in doing it this way (mixin body) was to be compatible for existing projects. For me it would also be fine if we add new arguments on the line if that fits better to your mind.

I checked the PR with my latest changes in the project and update it. There will be just "plain" set as type for TEXT body.

---

In case that you do not prefer mixin things, have you seen the variation for filenames and attachment dispositions as written in Point 6. of my starting comment? This aligns to

https://github.com/datafolklabs/cement/pull/669/files#diff-a6ee82810445181968d9673e2d8cf87e0eb58a00fb63157182f5f7ca32dc8a1cR175-R187

Give me a note if you want to have more changes.

[TomFreudenberg]

@derks if you check the lines

https://github.com/datafolklabs/cement/pull/669/files#diff-a6ee82810445181968d9673e2d8cf87e0eb58a00fb63157182f5f7ca32dc8a1cL64-L68

I wonder why you have created the special lines for "subject_prefix". Looking on that I would say it does fit also into the array of one of the above items:

# some keyword args override configuration defaults
        for item in ['to', 'from_addr', 'cc', 'bcc', 'subject']:

From my point of view it should be appended into that?

So there could be a config default but could be overwritten for some messages as well?

---

I came up here while thinking about a default for files. Wouldn't it make sense as well to add a default for files to the default_config and allow some configuration?

So I would end up here:

# some keyword args override configuration defaults
        for item in ['to', 'from_addr', 'cc', 'bcc', 'subject', 'subject_prefix', 'files']:

[TomFreudenberg]

The last change allows to config subject_prefix and files using config_defaults or config_file

[derks]

@TomFreudenberg I can work with this... everything makes sense and just need to do some work for test coverage, etc. I've realized testing for the SMTP extension is heavily mocked so going to work on getting it going through real testing. Might modify the above somewhat, but will ensure it meets your use case.

Will get this merged in as soon as I can.

[TomFreudenberg]

Hi @derks thanks for your feedback, I am not in hurry.

Currently I deliver an own smtp_ext.py on tokeo, so all is covered.

Offtopic:

Thanks for the great Cement, it is fun to work on that.

Maybe you find the time about my question about dividing configs for production, testing and development in case that only a few settings are changed in between.