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

Sat Jun 13 02:41:26 EDT 2015

On June 12, 2015 11:41:30 PM CEST, 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?

I don't know for sure, but I do know that many other sites use the same principle of different from+sender. Moreover, this is standard RFC, so spam detectors should respect it. Finally, if I were a spammer, I would use the standard From to forge a mail as this is how most mail is sent.

>
>> 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.

Will do.

>
>> [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).

Ok

>
>> +    else:
>> +        mail_from = mail_sender
>>       user = email_config.get('smtp_username')
>>       passwd = email_config.get('smtp_password')
>>       mail_server = email_config.get('smtp_server')
>> diff --git a/kallithea/model/comment.py b/kallithea/model/comment.py
>> --- a/kallithea/model/comment.py
>> +++ b/kallithea/model/comment.py
>> @@ -105,6 +105,7 @@ class ChangesetCommentsModel(BaseModel):
>>                   'cs_comment_url': comment_url,
>>                   'raw_id': revision,
>>                   'message': cs.message,
>> +                'message_firstline': cs.message.splitlines()[0],
>>                   'repo_name': repo.repo_name,
>>                   'short_id': h.short_id(revision),
>>                   'branch': cs.branch,
>> diff --git a/kallithea/model/notification.py
>b/kallithea/model/notification.py
>> --- a/kallithea/model/notification.py
>> +++ b/kallithea/model/notification.py
>> @@ -117,6 +117,7 @@ class NotificationModel(BaseModel):
>>           headers = None
>>           if 'threading' in email_kwargs:
>>               headers = {'References': ' '.join('<%s>' % x for x in
>email_kwargs['threading'])}
>> +        author = created_by_obj.full_contact
>>   
>>           # send email with notification to all other participants
>>           for rec in rec_objs:
>> @@ -145,7 +146,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)
>>   
>>           return notif
>>   
>> @@ -299,13 +300,13 @@ class EmailNotificationModel(BaseModel):
>>               self.TYPE_PULL_REQUEST_COMMENT: 'pull_request_comment',
>>           }
>>           self._subj_map = {
>> -            self.TYPE_CHANGESET_COMMENT: _('[Comment from
>%(comment_username)s] %(repo_name)s changeset %(short_id)s on
>%(branch)s'),
>> +            self.TYPE_CHANGESET_COMMENT: _('[Comment] %(repo_name)s
>changeset %(short_id)s: "%(message_firstline)s" on %(branch)s'),
>
>Isn't it very useful to have in the subject _who_ the comment was from?

Well, that us already in the From header if the mail so doesn't need repeating imo.

>
>Anyway, that seems unrelated to adding the first line of the commit 
>message. Also, I guess it would make sense to restrict the commit 
>message to something like the first 50 characters.

Ok

>
>And: wouldn't it be more useful to have the first line of the actual 
>comment in the mail subject instead of the first line of the PR
>description?

Maybe, but given your findings on threading with gmail, it would mean that comment mails for pull requests are no longer threaded together with the pull request creation mail, which made sense to me.

>
>(Somewhat related: I have had a request to put the branch name
>"earlier" 
>in the subject line. This will go in the opposite direction. One thing 
>that could help could be to hide the branch name if it is default (hg) 
>or master (git).)

Our original patch had [repo#branch] in front of the subject. I'm open to changing that, what would your prefer as subjects?
Main items from our side are the removal of the user from the subject and addition of the title.

Thanks,
Thomas