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

[Mads Kiilerich]

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

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

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?

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

>               self.TYPE_MESSAGE: 'Test Message',
>               # self.TYPE_PASSWORD_RESET
>               self.TYPE_REGISTRATION: _('New user %(new_username)s registered'),
>               # self.TYPE_DEFAULT
> -            self.TYPE_PULL_REQUEST: _('[Added by %(pr_username)s] %(repo_name)s pull request %(pr_nice_id)s from %(ref)s'),
> -            self.TYPE_PULL_REQUEST_COMMENT: _('[Comment from %(comment_username)s] %(repo_name)s pull request %(pr_nice_id)s from %(ref)s'),
> +            self.TYPE_PULL_REQUEST: _('[Added] %(repo_name)s pull request %(pr_nice_id)s: "%(pr_title)s" from %(ref)s'),
> +            self.TYPE_PULL_REQUEST_COMMENT: _('[Comment] %(repo_name)s pull request %(pr_nice_id)s: "%(pr_title)s" from %(ref)s'),
>           }
>   
>       def get_email_description(self, type_, **kwargs):

/Mads