On 04/24/2015 11:47 PM, Jan Heylen wrote:
On Fri, Apr 24, 2015 at 7:20 PM, Mads Kiilerich <[email protected]> wrote:
On 04/14/2015 06:03 PM, Jan Heylen wrote:
# HG changeset patch
# User Jan Heylen <[email protected]>
# 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
_______________________________________________
kallithea-general mailing list
[email protected]
http://lists.sfconservancy.org/mailman/listinfo/kallithea-general
