[PATCH 2 of 5] e-mail: send comment and pullrequest mails with the author's name in From
Thomas De Schampheleire
patrickdepinguin at gmail.com
Mon Jul 13 15:46:14 EDT 2015
On Mon, Jul 13, 2015 at 9:02 PM, Mads Kiilerich <mads at kiilerich.com> wrote:
> 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.
Ok.
>
> 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?
Is that common practice? At least, I haven't seen the 'virtually'
suffix. I think I have seen things like:
Full Name (Kallithea) <noreply at ...>
So giving the application's name as suffix. What about that?
>
>> 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?
The original code by Cedric was giving only a string (full_contact),
but I found it to restricting that the caller specified the way
e-mails were formed. So I changed it to take a User object, so that
the e-mail code has more flexibility. I can rename it to user of
course, is that fine enough or do you prefer other changes?
>
>> @@ -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.
Ok.
>
>> 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?
The e-mail will always have a From header. If it is not set here, the
SmtpMailer will default to the sender passed, currently also
envelope_from. The next patch will change that to app_email_noreply.
The code really is like this due to code evolution and so strictly
speaking this patch could be trimmed with a few lines, but I'd argue
that it makes little sense here as the behavior is the same and the
next patch will need this structure anyway?
>
>> + 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.)
Ok.
>
>> + 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.
Ok.
>
>> + 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?
parseaddr could be it:
https://docs.python.org/2/library/email.util.html
>
>> + 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.
That is the case in the above code, or I'm misunderstanding you. All I
did was group different warning calls in one, add some fields, and
move the subject field to warning with the rest of the mail details.
>
> (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?
No, it is used already in other places in the test suite, for example
kallithea/tests/other/test_libs.py
So yes, it works with nosetests.
In fact, with pytest, one would use the monkeypatch fixture for such actions.
>
>> +
>> +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?
Ok.
>
>
>> +
>> +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?
No, sorry. Will fix.
/Thomas
More information about the kallithea-general
mailing list