On 07/13/2015 09:46 PM, Thomas De Schampheleire wrote:
On Mon, Jul 13, 2015 at 9:02 PM, Mads Kiilerich <[email protected]> wrote:
On 07/13/2015 10:45 AM, Thomas De Schampheleire wrote:
# HG changeset patch
# User Thomas De Schampheleire <[email protected]>
# 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@...>
So giving the application's name as suffix. What about that?
My example is probably not common practice. Your example might be
better. Or how about just hardcode adding '(Noreply)' ?
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?
I think either of them would be fine - I would just prefer a docstring
to explain which of them it is.
@@ -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?
I would prefer to not have unrelated and (by themself) pointless changes
bundled together with stuff that matters. No big deal, though.
+ 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
Or kallithea.lib.vcs.utils.author_email or the weird wrappers in
kallithea.lib.helpers .
+ 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.
Heh - I missed a "not".
I think it should be possible to debug envelope_from and headers even if
mail_server is configured. They are however not very relevant if the
user has the much bigger problem of not having any mail server.
/Mads
_______________________________________________
kallithea-general mailing list
[email protected]
http://lists.sfconservancy.org/mailman/listinfo/kallithea-general
