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

[Jan Heylen]

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/kallithea/model/notification.py   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

>
>
>>           """
>>             Creates notification of given type
>> @@ -103,10 +103,16 @@
>>           #else: silently skip notification mails?
>>             # TODO: inform user who are notified
>> -        notif = Notification.create(
>> -            created_by=created_by_obj, subject=subject,
>> -            body=body, recipients=recipients_objs, type_=type_
>> -        )
>> +        if not bodies:
>> +            bodies = [body]
>> +
>> +        bodies_html = []
>> +        for body in bodies:
>> +            notif = Notification.create(
>> +                created_by=created_by_obj, subject=subject,
>> +                body=body, recipients=recipients_objs, type_=type_
>> +            )
>> +            bodies_html.append(h.rst_w_mentions(body))
>>             if not with_email:
>>               return notif
>> @@ -123,14 +129,14 @@
>>               ## this is passed into template
>>               html_kwargs = {
>>                         'subject': subject,
>> -                      'body': h.rst_w_mentions(body),
>> +                      'bodies': bodies_html,
>>                         'when': h.fmt_date(notif.created_on),
>>                         'user': notif.created_by_user.username,
>>                         }
>>                 txt_kwargs = {
>>                         'subject': subject,
>> -                      'body': body,
>> +                      'bodies': bodies,
>>                         'when': h.fmt_date(notif.created_on),
>>                         'user': notif.created_by_user.username,
>>                         }
>> diff -r cb41b8211291 -r 55ba8e68269d
>> kallithea/templates/email_templates/changeset_comment.html
>> --- 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.

>
>>   %if status_change:
>>       <p>${_('The changeset status was changed to')}:
>> <b>${status_change}</b></p>
>>   %endif
>> diff -r cb41b8211291 -r 55ba8e68269d
>> kallithea/templates/email_templates/changeset_comment.txt
>> --- a/kallithea/templates/email_templates/changeset_comment.txt Fri Mar 27
>> 13:58:04 2015 +0100
>> +++ b/kallithea/templates/email_templates/changeset_comment.txt Fri Mar 27
>> 16:13:46 2015 +0100
>> @@ -6,7 +6,9 @@
>>   %else:
>>   ${_('Comment from %s on %s changeset %s') % (cs_comment_user,
>> cs_target_repo, h.short_id(raw_id))|n,unicode}:
>>   %endif
>> +%for body in bodies:
>>   ${body|n,unicode}
>> +%endfor
>>     %if status_change:
>>   ${_('The changeset status was changed to')|n,unicode}:
>> ${status_change|n,unicode}
>> diff -r cb41b8211291 -r 55ba8e68269d
>> kallithea/templates/email_templates/default.html
>> --- a/kallithea/templates/email_templates/default.html  Fri Mar 27
>> 13:58:04 2015 +0100
>> +++ b/kallithea/templates/email_templates/default.html  Fri Mar 27
>> 16:13:46 2015 +0100
>> @@ -1,4 +1,6 @@
>>   ## -*- coding: utf-8 -*-
>>   <%inherit file="main.html"/>
>>   +%for body in bodies:
>>   ${body}
>> +%endfor
>
>
> It doesn't make much sense to have multiple bodies if we only concatenate
> them. But ok, if it is simpler this way.
>
> But I guess this really depends on how to solve the problem mentioned above:
> We do not just have a list of bodies; we have a list of entries that each
> have a file and line and url and body.
>
> /Mads