466968 - Templatise 'newchangedmail' email's "diffs" parameter (BugMail)

Status: RESOLVED FIXED

(Bugzilla :: Email Notifications, enhancement)

Description

[Stephen Lee]

Seeing Bug 275636 fixed, I thought I would experiment with trying to adjust the template to create a multipart/alternative HTML email...

Steps to reproduce:
1. load template/en/default/email/newchangedemail.txt.tmpl into an editor
2. find the part where the differences are shown

Actual results:
The entire content of the email is substituted into a single parameter
[%+ diffs %]
which itself is constructed of hard-coded text within processmail.

Expected results:
The formatting of lines like the following would be defined within this or another template.

[% username %] changed:

           What    |Removed                     |Added
--------------------------------------------------------------------------
--
...
(loop within template to enumerate changes passed to it, and format these)
...
------- Additional Comments From [% username %] [% commenttime %] -------
[% comments %]


At present only the top and bottom of the email appear to be templatised.

Separating out the different fields into separate parameters that the template can address would also seem to be a pre-requisite for creating a meaningful XML format email, so marking dep...
Something like "<diffs>[%+ diffs %]</diffs>" probably wouldn't cut it ;)

Comment 1

[Vitaly Fedrushkov]

See bug 215210 comment 32

Looks like duplicate to me

Comment 2

[Stephen Lee]

(In reply to comment #1)

Hmmm... bug 215210 comment 32 contains a workaround for allowing the language text to be localised in the absence of a templatised 'diffs' parameter.

Looks like this *blocks* bug 215210 to me. There would be reasons for editing the template other than to localise (e.g. my originally intended purpose of making an HTML mail). Will let someone more up-to-speed with current b.m.o policy decide whether to roll this into bug 215210 or make this block it.

Comment 3

[Frédéric Buclin]

Attachment: patch, v0.9

Here is a fully functional patch. Bugmails are now 100% templatized. The only minor bug I found is that for bugmail for new bugs, the first line has no indentation. I couldn't find a trivial fix for it. But if that's the only problem found in this patch, I propose we fix it in a separate bug, as it's very minor compared to the improvement of this patch.

Comment 4

[Frédéric Buclin]

Attachment: patch, v1

This patch fixes the indentation problem I mentioned in comment 3. AFAICT, there is no other regressions introduced by this patch. :) Of course, some indentations in the generate_diffs BLOCK of the email template could look weird to you, but remember that we are in a .txt file, so we sometimes have to sacrifice readability (i.e. the indentation) to make things looks correctly in the generated emails.

Comment 5

[Frédéric Buclin]

Attachment: patch, v1.1

unbitrotten patch (due to the checkin of bug 583287).

Comment 6

[Max Kanat-Alexander]

Comment on attachment 462258 [details] [diff] [review]
patch, v1.1

Looks pretty cool. :-)

I think it would make this a bit easier to review if the Bugzilla::User stuff was in a separate patch.

Comment 7

[Frédéric Buclin]

(In reply to comment #6)
> I think it would make this a bit easier to review if the Bugzilla::User stuff
> was in a separate patch.

No, because I really change the stuff passes to it (I no longer pulls fielddefs.description from the DB).

Comment 8

[Max Kanat-Alexander]

(In reply to comment #7)
> (In reply to comment #6)
> > I think it would make this a bit easier to review if the Bugzilla::User stuff
> > was in a separate patch.
> 
> No, because I really change the stuff passes to it (I no longer pulls
> fielddefs.description from the DB).

  Okay. Couldn't all of that be done in a separate bug?

Comment 9

[Max Kanat-Alexander]

Comment on attachment 462258 [details] [diff] [review]
patch, v1.1

>Index: Bugzilla/BugMail.pm
>+    if (!$start) {

  Might be cool to put the code of this block into its own subroutine.

>+        my @fields = Bugzilla->get_fields({obsolete => 0, mailhead => 1});
>+            elsif (ref $value && $value->isa('Bugzilla::User')) {
>+                $value = $value->login;
>+            }
>+            elsif (ref $value && $value->isa('Bugzilla::Object')) {
>+                $value = $value->name;
>+            }

  Probably better to use "blessed" than "ref".

>+            elsif ($name eq 'estimated_time') {
>+                # "0.00" (which is what we get from the DB) is true,
>+                # so we explicitly do a numerical comparison with 0.
>+                $value = 0 if ($value == 0);

  Nit: The parens aren't necessary.

>+    if ($params->{dep_only} && $start) {

  Hmm, I wonder--does it even matter if $start is specified, if dep_only is set?

>+        push(@diffs, { field_name => 'bug_status',
>+                       old => $params->{changes}->{bug_status}->[0],
>+                       new => $params->{changes}->{bug_status}->[1],
>+                       login_name => $changer->login,
>+                       blocker => $params->{blocker} },

  It's cool that we're always passing around hashrefs now--it'll make it easy to switch to ChangeSet objects.

>+    if (!scalar(@display_diffs) && !scalar(@send_comments)) {

  Hmm, does this work when we have a blank (only a single space) comment?

>+        changedfields => [map { $_->{field_name} } @display_diffs],

  Hmm, maybe you should call "uniq" there? (From List::MoreUtils.)

>Index: template/en/default/email/newchangedmail.txt.tmpl
>@@ -40,14 +43,11 @@ X-Bugzilla-Status: [% bug.bug_status %]
> X-Bugzilla-Priority: [% bug.priority %]
> X-Bugzilla-Assigned-To: [% bug.assigned_to.login %]
> X-Bugzilla-Target-Milestone: [% bug.target_milestone %]
>-X-Bugzilla-Changed-Fields: [% changedfields.join(" ") %]
>+X-Bugzilla-Changed-Fields: [% changedfields.join(" ") IF !isnew %]

  Well, a bug can have had several changes after being created, and never have had email sent--also, flags can be included in the initial diffs, and those would show up there.

>+[%+ PROCESS generate_diffs -%]

  Hmm, I'm kind of thinking that maybe the diffs part should be a separate file?

>@@ -68,3 +68,52 @@ Configure [% terms.bug %]mail: [% urlbas
>+    [% field_label = field_descs.${change.field_name} %]
>+    [% old_value = display_value(change.field_name, change.old) %]
>+    [% new_value = display_value(change.field_name, change.new) %]

  This won't work for fields with multiple values, but I suppose that's not as important, and it can wait until the future.

  All in all, this is a pretty awesome patch. :-) The code is awesome, what it does is awesome, and I'm super happy about it. :-) The only hard r- is for the two comments related to changedfields.

Comment 10

[Max Kanat-Alexander]

(In reply to comment #8)
>   Okay. Couldn't all of that be done in a separate bug?

  And nevermind, I see why it's an important part of this bug, now. :-) And it's a very straightforward change. :-)

Comment 11

[Frédéric Buclin]

(In reply to comment #9)
> >+    if ($params->{dep_only} && $start) {
> 
>   Hmm, I wonder--does it even matter if $start is specified, if dep_only is
> set?

I thought about that too, but didn't investigate. The code was so hard to understand when I started working on this patch that I left it alone. But now that the code is much easier to read, I can give it a look again. :)


> >+    if (!scalar(@display_diffs) && !scalar(@send_comments)) {
> 
>   Hmm, does this work when we have a blank (only a single space) comment?

No, it doesn't. This will be fixed in bug 583154, because branches are affected too. So I didn't want to mix that fix in this bug.


> >-X-Bugzilla-Changed-Fields: [% changedfields.join(" ") %]
> >+X-Bugzilla-Changed-Fields: [% changedfields.join(" ") IF !isnew %]
> 
>   Well, a bug can have had several changes after being created, and never have
> had email sent--also, flags can be included in the initial diffs, and those
> would show up there.

We currently don't fill this header on bug creation, which is why I added IF !isnew. Else you would get the 20 or so fields which are populated on bug creation. I wouldn't be too worried about emails not being sent, honestly.


> >+[%+ PROCESS generate_diffs -%]
> 
>   Hmm, I'm kind of thinking that maybe the diffs part should be a separate
> file?

I thought about that too, but for now, I see no reason to do it, especially because there will be no other template calling this BLOCK as is. I first want to see how we will implement HTML and XML bugmails before moving it in its own template. They will all share some common code (especially the one about display_value()), but I prefer to wait.


>   This won't work for fields with multiple values, but I suppose that's not as
> important, and it can wait until the future.

Yes, definitely. :-D


>   All in all, this is a pretty awesome patch. :-) The code is awesome, what it
> does is awesome, and I'm super happy about it. :-)

Thanks! :)

Comment 12

[Frédéric Buclin]

Attachment: patch, v2

OK, I fixed everything I didn't reply to.

Comment 13

[Max Kanat-Alexander]

Comment on attachment 462783 [details] [diff] [review]
patch, v2

Looks good! :-) If there are any regressions, I'm sure we'll catch them; it's really early in the development cycle right now.

Comment 14

[Frédéric Buclin]

I think it worths to relnote that people can now easily create their own templates for bugmails, if they want to. And for localizers, bugmails are now 100% localizable.

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified Bugzilla/BugMail.pm
modified Bugzilla/Template.pm
modified Bugzilla/User.pm
modified Bugzilla/Util.pm
modified template/en/default/email/newchangedmail.txt.tmpl
Committed revision 7433.

Comment 15

[Marc Schumann [:Wurblzap]]

Comment on attachment 462783 [details] [diff] [review]
patch, v2

>Index: Bugzilla/BugMail.pm
>+        elsif ($name eq 'deadline') {
>+            $value = time2str("%Y-%m-%d", str2time($value)) if $value;
>+        }

The date format should be determined by the template. This didn't work before, either, so it can be done in a separate bug.

Thanks for the patch! :)

Comment 17

[Frédéric Buclin]

Added to relnotes in bug 713346.