[PATCH 2 of 7] notification: allow multiple body parts in templates

[Mads Kiilerich]

On 04/24/2015 11:47 PM, Jan Heylen wrote:
> On Fri, Apr 24, 2015 at 7:20 PM, Mads Kiilerich <mads at kiilerich.com> wrote:
>> On 04/14/2015 06:03 PM, Jan Heylen wrote:
>>> # HG changeset patch
>>> # User Jan Heylen <heyleke at gmail.com>
>>> # Date 1427469226 -3600
>>> #      Fri Mar 27 16:13:46 2015 +0100
>>> # Node ID 55ba8e68269da8fb0c33c65711a57b4fd9e1b675
>>> # Parent  cb41b8211291b55bcd7c77919f714791927b8045
>>> notification: allow multiple body parts in templates
>>>
>>> diff -r cb41b8211291 -r 55ba8e68269d kallithea/model/notification.py
>>> --- a/kallithea/model/notification.py   Fri Mar 27 13:58:04 2015 +0100
>>> +++ b/	   Fri Mar 27 16:13:46 2015 +0100
>>> @@ -59,7 +59,7 @@
>>>          def create(self, created_by, subject, body, recipients=None,
>>>                   type_=Notification.TYPE_MESSAGE, with_email=True,
>>> -               email_kwargs={}):
>>> +               email_kwargs={}, bodies=None):
>>
>> I think it would be less ugly to allow body to be either a single instance
>> or a list, perhaps depending on the type, perhaps with the single instance
>> as a list with one element.
> I didn't think about that, I'll have a look how that would work in the code

Hmm. This notification creation API is not optimal. I think the body 
parameter should be completely replaced by 'kwargs' that could be used 
both for emails and notifications - perhaps passed as **kwargs. For a 
"multi comment commit", this parameter could have a 'comments' list of 
dicts (or tuples) with comment text and url and filename and line. For 
some types, one of the kwargs might still be a 'body'.

The content of the notifications is currently not as useful as the 
emails. The body stored in the notification should perhaps just be the 
essential html used for an email ... but we would probably still want 
one notification for each comment, even if we send mails with multiple 
comments. It could perhaps also be some serialization of the parameters 
so notifications can be shown nicely.

>>> --- a/kallithea/templates/email_templates/changeset_comment.html
>>> Fri Mar 27 13:58:04 2015 +0100
>>> +++ b/kallithea/templates/email_templates/changeset_comment.html
>>> Fri Mar 27 16:13:46 2015 +0100
>>> @@ -6,8 +6,9 @@
>>>    %else:
>>>    <p>${_('Comment from %s on %s changeset %s') % (cs_comment_user,
>>> cs_target_repo, h.short_id(raw_id))}:</p>
>>>    %endif
>>> +%for body in bodies:
>>>    <p>${body}</p>
>>> -
>>> +%endfor
>>
>> I would like to let the mails include filename and linenumber (and
>> ultimately also some lines of context). It seems like this change will make
>> it harder to add that. Could it be done first?
> Yes, I thought about that too, but I wanted to do the multiple
> comments in one email part first, and afterwards we could look into
> the 'make the email more attractive'. Bodies, or body as a list,
> should be an object with those attributes in that case? Or we add a
> new thing "comment" with those attributes, so we can still use "body"
> in the other templates?
>
>> And more important: There should be a cs_comment_url link for each comment.
> Same comment, I thouthg about that too, but didn't want to focus on
> the email look&feel with this patch series.

But I will point out that before, each email contains a link to see each 
comment in the right context. AFAICS, the emails with multiple comments 
no longer contains links to see each comment in the right content. For 
large PRs, it is essential to have links to each comment. It is 
essential to preserve that when changing things here.

/Mads