[PATCH 2 of 5] e-mail: send comment and pullrequest mails with the author's name in From

[Mads Kiilerich]

On 07/13/2015 10:45 AM, Thomas De Schampheleire wrote:
> # HG changeset patch
> # User Thomas De Schampheleire <thomas.de.schampheleire at gmail.com>
> # Date 1436471455 -7200
> #      Thu Jul 09 21:50:55 2015 +0200
> # Node ID 95c18c5fdb59f2d985e2cd8ac6db093cb3e5f884
> # Parent  c22a219636b830bac4b6125b306f6c8506f81d99
> e-mail: send comment and pullrequest mails with the author's name in From
>
> When e-mails are sent for comments and pullrequest invitations, set the From
> header to:
>      Author's Name <generic e-mail address>

The rationale for this has been discussed before. It could be nice to 
have a brief summary here in the commit message.

Also, I wonder if the author name should be augmented with 'something' 
to make it clear that it not is something that should be added to 
address books or used to reach the person with that name. Perhaps add 
"virtually" or some other suffix?

> The sender used for other e-mail types, e.g. password reset mails, is
> untouched and remains the value configured in app_email_from.
>
> The sender used for the SMTP envelope is untouched as well.
>
> Based on code by Cedric De Herdt.
>
> diff --git a/kallithea/lib/celerylib/tasks.py b/kallithea/lib/celerylib/tasks.py
> --- a/kallithea/lib/celerylib/tasks.py
> +++ b/kallithea/lib/celerylib/tasks.py
> @@ -247,7 +247,7 @@ def get_commits_stats(repo_name, ts_min_
>   
>   @task(ignore_result=True)
>   @dbsession
> -def send_email(recipients, subject, body='', html_body='', headers=None):
> +def send_email(recipients, subject, body='', html_body='', headers=None, author=None):
>       """
>       Sends an email with defined parameters from the .ini files.
>   

It would be nice with a docstring explaining what 'author' is and what 
it is used for. Looking at the code, I guess it must be a User object? 
Perhaps just call it 'user' instead? Or rework the code to make it a 
more generic 'sender_full_name' string?

> @@ -259,6 +259,8 @@ def send_email(recipients, subject, body
>       """
>       log = get_logger(send_email)
>       assert isinstance(recipients, list), recipients
> +    if headers is None:
> +        headers = {}

If headers is passed, it is a mutable object which we will mutate. I 
suggest creating a copy before modifying.

>   
>       email_config = config
>       email_prefix = email_config.get('email_prefix', '')
> @@ -274,7 +276,23 @@ def send_email(recipients, subject, body
>           log.error("No recipients specified")
>           return False
>   
> -    mail_from = email_config.get('app_email_from', 'Kallithea')
> +    # SMTP sender
> +    envelope_from = email_config.get('app_email_from', 'Kallithea')
> +    # From header
> +    if author is None:
> +        headers['From'] = envelope_from

Why set the From header if none has been specified?

> +    else:
> +        # set From header based on author but with a generic e-mail address
> +        # In case app_email_from is in "Some Name <e-mail>" format, we first
> +        # extract the e-mail address.

(It would be nice if the hints in the .ini file told which formats are 
valid.)

> +        import re

Please just import globally. The re object could perhaps also be 
compiled globally ... this is not used very often so probably no big deal.

> +        m = re.match('.*<(.*)>', envelope_from)
> +        if m is not None:
> +            envelope_addr = m.group(1)
> +        else:
> +            envelope_addr = envelope_from

- but I guess there is a more generic mail address parser in utils or 
helpers that we can use?

> +        headers['From'] = '%s <%s>' % (author.full_name_or_username, envelope_addr)
> +
>       user = email_config.get('smtp_username')
>       passwd = email_config.get('smtp_password')
>       mail_server = email_config.get('smtp_server')
> @@ -285,13 +303,18 @@ def send_email(recipients, subject, body
>       smtp_auth = email_config.get('smtp_auth')
>   
>       if not mail_server:
> -        log.error("SMTP mail server not configured - cannot send mail '%s' to %s", subject, ' '.join(recipients))
> -        log.warning("body:\n%s", body)
> -        log.warning("html:\n%s", html_body)
> +        log.error("SMTP mail server not configured - cannot send mail to %s", ' '.join(recipients))
> +        log.warning("Mail details:\n"
> +                    "envelope_from: %s\n"
> +                    "headers: %s\n"
> +                    "subject: %s\n"
> +                    "body:\n%s\n"
> +                    "html:\n%s\n"
> +                    % (envelope_from, headers, subject, body, html_body))

I agree that it can be convenient to have debug logging of the mail 
details ... but it should only be if no mail_server has been configured.

(The cases of no mail server or no/unknown/invalid recipients should 
really be handled earlier instead of ending up in the error log here.)

>           return False
>   
>       try:
> -        m = SmtpMailer(mail_from, user, passwd, mail_server, smtp_auth,
> +        m = SmtpMailer(envelope_from, user, passwd, mail_server, smtp_auth,
>                          mail_port, ssl, tls, debug=debug)
>           m.send(recipients, subject, body, html_body, headers=headers)
>       except:
> diff --git a/kallithea/model/notification.py b/kallithea/model/notification.py
> --- a/kallithea/model/notification.py
> +++ b/kallithea/model/notification.py
> @@ -145,7 +145,7 @@ class NotificationModel(BaseModel):
>                                   .get_email_tmpl(type_, 'html', **html_kwargs)
>   
>               run_task(tasks.send_email, [rec.email], email_subject, email_txt_body,
> -                     email_html_body, headers)
> +                     email_html_body, headers, author=created_by_obj)
>   
>           return notif
>   
> diff --git a/kallithea/tests/other/test_mail.py b/kallithea/tests/other/test_mail.py
> new file mode 100644
> --- /dev/null
> +++ b/kallithea/tests/other/test_mail.py
> @@ -0,0 +1,94 @@
> +import mock

A new dependency? Does it work with nosetests too?

> +
> +from kallithea.tests import *
> +from kallithea.model.db import User
> +import kallithea.lib.celerylib.tasks as t

tasks is not used that much - why give it such a short and obfuscated 
name? Looking at how it is used, it seems like the full 
kallithea.lib.celerylib.tasks would be fine?

> +
> +class smtplib_mock(object):
> +
> +    @classmethod
> +    def SMTP(cls, server, port, local_hostname):
> +        return smtplib_mock()
> +        print 'mocked'
> +
> +    def ehlo(self):
> +        pass
> +    def quit(self):
> +        pass
> +    def sendmail(self, sender, dest, msg):
> +        smtplib_mock.lastsender = sender
> +        smtplib_mock.lastdest = dest
> +        smtplib_mock.lastmsg = msg
> +        pass
> +
> + at mock.patch('kallithea.lib.rcmail.smtp_mailer.smtplib', smtplib_mock)
> +class TestMail(BaseTestCase):
> +
> +    def test_send_mail_trivial(self):
> +        mailserver = 'smtp.mailserver.org'
> +        recipients = ['rcpt1', 'rcpt2']
> +        envelope_from = 'noreply at mailserver.org'
> +        subject = 'subject'
> +        body = 'body'
> +        html_body = 'html_body'
> +
> +        config_mock = {
> +                'smtp_server': mailserver,
> +                'app_email_from': envelope_from,

Intentionally using double indentation?

> +        }
> +        with mock.patch('kallithea.lib.celerylib.tasks.config', config_mock):
> +            t.send_email(recipients, subject, body, html_body)
> +
> +        self.assertSetEqual(smtplib_mock.lastdest, set(recipients))
> +        self.assertEqual(smtplib_mock.lastsender, envelope_from)
> +        self.assertIn('From: %s' % envelope_from, smtplib_mock.lastmsg)
> +        self.assertIn('Subject: %s' % subject, smtplib_mock.lastmsg)
> +        self.assertIn(body, smtplib_mock.lastmsg)
> +        self.assertIn(html_body, smtplib_mock.lastmsg)
> +
> +    def test_send_mail_with_author(self):
> +        mailserver = 'smtp.mailserver.org'
> +        recipients = ['rcpt1', 'rcpt2']
> +        envelope_from = 'noreply at mailserver.org'
> +        subject = 'subject'
> +        body = 'body'
> +        html_body = 'html_body'
> +        author = User.get_by_username(TEST_USER_REGULAR_LOGIN)
> +
> +        config_mock = {
> +                'smtp_server': mailserver,
> +                'app_email_from': envelope_from,
> +        }
> +        with mock.patch('kallithea.lib.celerylib.tasks.config', config_mock):
> +            t.send_email(recipients, subject, body, html_body, author=author)
> +
> +        self.assertSetEqual(smtplib_mock.lastdest, set(recipients))
> +        self.assertEqual(smtplib_mock.lastsender, envelope_from)
> +        self.assertIn('From: Kallithea Admin <%s>' % envelope_from, smtplib_mock.lastmsg)
> +        self.assertIn('Subject: %s' % subject, smtplib_mock.lastmsg)
> +        self.assertIn(body, smtplib_mock.lastmsg)
> +        self.assertIn(html_body, smtplib_mock.lastmsg)
> +
> +    def test_send_mail_with_author_full_mail_from(self):
> +        mailserver = 'smtp.mailserver.org'
> +        recipients = ['rcpt1', 'rcpt2']
> +        envelope_addr = 'noreply at mailserver.org'
> +        envelope_from = 'Some Name <%s>' % envelope_addr
> +        subject = 'subject'
> +        body = 'body'
> +        html_body = 'html_body'
> +        author = User.get_by_username(TEST_USER_REGULAR_LOGIN)
> +
> +        config_mock = {
> +                'smtp_server': mailserver,
> +                'app_email_from': envelope_from,
> +        }
> +        with mock.patch('kallithea.lib.celerylib.tasks.config', config_mock):
> +            t.send_email(recipients, subject, body, html_body, author=author)
> +
> +        self.assertSetEqual(smtplib_mock.lastdest, set(recipients))
> +        self.assertEqual(smtplib_mock.lastsender, envelope_from)
> +        self.assertIn('From: Kallithea Admin <%s>' % envelope_addr, smtplib_mock.lastmsg)
> +        self.assertIn('Subject: %s' % subject, smtplib_mock.lastmsg)
> +        self.assertIn(body, smtplib_mock.lastmsg)
> +        self.assertIn(html_body, smtplib_mock.lastmsg)