[PATCH] notification: use Sender and From header to clarify comment and pull request mails

[Thomas De Schampheleire]

Hi Mads,

On Fri, Jun 12, 2015 at 11:41 PM, Mads Kiilerich <mads at kiilerich.com> wrote:
> On 06/12/2015 09:09 PM, Thomas De Schampheleire wrote:
>>
>> # HG changeset patch
>> # User Cedric De Herdt <cedric.de_herdt at alcatel-lucent.com>
>> # Date 1434135934 -7200
>> #      Fri Jun 12 21:05:34 2015 +0200
>> # Node ID c9c5310da1771baed04e70fb45293b8d212e8bb9
>> # Parent  42feaacb78feecb2a07a3ca19db6cf7ace3fd4c1
>> notification: use Sender and From header to clarify comment and pull
>> request mails
>>
>> Current e-mails are sent from the Kallithea-configured e-mail address. The
>> subject line then needs to refer to the user to be useful.
>> Instead, use the author of comments and pull requests as 'From', and make
>> the Kallithea-configured address the 'Sender' in accordance with RFC5322.
>
>
> So this will not cause problems for domains using domain keys or other anti
> spam "authenticated sender" schemes?
>
>> Additionally, add the changeset/pullrequest description in the subject.
>
>
> Separate patch, please ... especially because these two aspects are
> unrelated and have different concerns and review comments.
>
>
>> [Thomas De Schampheleire:
>>    - extend commit message
>>    - update after threading requirements described in commit 8d45a14d3191
>> ]
>>
>> 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.
>>   @@ -274,7 +274,17 @@ def send_email(recipients, subject, body
>>           log.error("No recipients specified")
>>           return False
>>   -    mail_from = email_config.get('app_email_from', 'Kallithea')
>> +    mail_sender = email_config.get('app_email_from', 'Kallithea')
>> +    if author:
>> +        # indicates that there is a real author which should be in the
>> 'From' field
>> +        # the agent will be put in 'Sender' field: see
>> http://tools.ietf.org/html/rfc5322#section-3.6.2
>> +        mail_from = author
>> +        try:
>> +            headers['Sender'] = mail_sender
>> +        except TypeError:
>> +            headers = {'Sender': mail_sender}
>
>
> That is for the case where headers is None? Then please detect that
> explicitly with "headers is None" or "not headers".
>
> Or (assuming we have the default headers=None to avoid having mutable
> objects as default values), have an early "it headers is None: headers={}"
> in the function.
>
> Anyway, how about a first patch that refactors and consistently renames
> "mail_from" to "envelope_from" (which seems to be the right technical term
> for what it is), and then another patch for setting "header_from" (or reuse
> mail_from).

After further investigation, I don't think that 'envelope_from' is the
right term. The envelope is something _around_ the message, the
message being the contents _and_ the headers. These headers include
both From: and Sender:
There exist indeed the possibility of having an envelope sender, but
this is not what we're dealing with here.

Some references:
http://www.avolio.com/columns/E-mailheaders.html
http://tools.ietf.org/html/rfc5322

/Thomas