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

[Jan Heylen]

On Mon, Apr 27, 2015 at 2:49 AM, Mads Kiilerich <mads at kiilerich.com> wrote:
> 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'.

So, would that be:
-    def create(self, created_by, subject, body, recipients=None,
-                type_=Notification.TYPE_MESSAGE, with_email=True,
-               email_kwargs={}):
+    def create(self, created_by, subject, recipients=None,
+                type_=Notification.TYPE_MESSAGE, with_email=True,
+               email_kwargs={}, **kwargs):
or
+    def create(self, created_by, subject, recipients=None,
+                type_=Notification.TYPE_MESSAGE, with_email=True,
+               **kwargs):

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

Yes, I know, I'll try to address that.

>
> /Mads

Jan