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

[Mads Kiilerich]

On 07/13/2015 09:46 PM, Thomas De Schampheleire wrote:
> 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?

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