457657 - Make e-mail comment header localizable

Status: RESOLVED FIXED

(Bugzilla :: Email Notifications, defect)

Description

[Marc Schumann [:Wurblzap]]

Attachment: Patch

The comment header of email notifications should be localizable.

This might be considered a part of bug 215210, but it doesn't go all the way, so here is a separate bug.

Comment 1

[Marc Schumann [:Wurblzap]]

The patch duplicates format_comment of Bugzilla::Bug, which can go away in a follow-up bug. The follow-up bug should make bug/comments.html.tmpl use bug/format_comment.txt.tmpl, and drop format_comment from Bugzilla::Bug.

Comment 2

[Frédéric Buclin]

Comment on attachment 340905 [details] [diff] [review]
Patch

>Index: Bugzilla/BugMail.pm

>+    my ($comments, $anyprivate, $count) = get_comments_by_bug($id, $start, $end);

$count is no longer used. Remove it from values returned by get_comments_by_bug().


>         my $lang = $user->settings->{'lang'}->{'value'};

$lang is no longer used. Remove this line.


>+    if ($count) {
>+        foreach my $comment (@$comments) {
>+            $comment->{count} = $count++;
>+        }
>+    }

You have to set (and increment) ->{count} in all cases, even if $count == 0. See the original code.



>Index: template/en/default/email/newchangedmail.txt.tmpl

>+[%+ PROCESS bug/format_comment.txt.tmpl
>+    comment = comment
>+ %]

Nit: you could write this on a single line.


Otherwise, your patch looks good. IMO, it would make sense to remove Bug::format_comment() in this bug too, but if you prefer to file a separate bug for it, I'm fine with that, assuming the 2nd bug is fixed quickly (I wouldn't want 3.4 to have duplicated code).

Comment 3

[Marc Schumann [:Wurblzap]]

Not an enhancement; we advertise Bugzilla to be fully localizable.

Comment 4

[Max Kanat-Alexander]

Realistically this doesn't look like it's going to make 3.4. Please feel free to rest the TM if you think otherwise.

Comment 5

[Marc Schumann [:Wurblzap]]

Attachment: Patch 1.1

Addressing comments.

Comment 6

[Frédéric Buclin]

Comment on attachment 364757 [details] [diff] [review]
Patch 1.1

>Index: Bugzilla/BugMail.pm

>+    my ($comments, $anyprivate) = get_comments_by_bug($id, $start, $end);

>     return ($comments, $anyprivate, $count);

As we no longer need $count, don't return it anymore.



>-sub prepare_comments {

>-        $result .= ($comment->{'already_wrapped'} ? $body : wrap_comment($body));

Here, we looked at 'already_wrapped' to decide if we have to wrap the comment or not, but you omit this check in your new code, see below.



>Index: template/en/default/bug/format_comment.txt.tmpl

>+[% comment.body FILTER wrap_comment %]

For instance here, and elsewhere too, see above.


>+If the move succeeded, [% comment.login %] will receive a mail containing the

comment.login doesn't exist.



>Index: template/en/default/email/newchangedmail.txt.tmpl

>+--- Comment #[% comment.count %] from [% comment.author.identity %] [%+ comment.time FILTER time('%Y-%m-%d %T %Z') %] ---

Nit: no need to specify this time format. That's the default format.


Your patch looks good, but I didn't test it yet. Maybe more comments will come with your updated patch.

Comment 7

[Marc Schumann [:Wurblzap]]

Attachment: Patch 1.2

Addressing comments.

Comment 8

[Frédéric Buclin]

Comment on attachment 373324 [details] [diff] [review]
Patch 1.2

>Index: template/en/default/bug/format_comment.txt.tmpl

>+[% comment.already_wrapped ? comment.body : comment.body FILTER wrap_comment %]

Are you sure this work as expected and FILTER is only triggered if the condition is false? I mean, this syntax really means:

IF foo ? bar : (bar FILTER baz)

and not:

IF foo ? (bar : bar) FILTER baz

I didn't check, so I hope you did, else this test must be done.



Also, wouldn't is be better to write something like:

[% comment = comment.already_wrapped ? comment.body : comment.body FILTER wrap_comment %]

and then simply write [% comment %] where needed? This would make the code cleaner.



>+[% ELSIF comment.type == constants.CMT_HAS_DUPE %]
>+*** [% terms.Bug %] [%+ comment.extra_data %] has been marked as a duplicate of this [% terms.bug %]. ***

For some reason, the header is closer to the diff table with this comment than with any other comment. Not sure why.


Now, the reason I deny review is because something strange happens with your patch applied: if bug X already blocks bug Y and you mark bug X as a dupe of bug Y, users of bug Y get two bugmails instead of one (one for the dupe, one for the blocking bug being resolved), but the second one is empty. No idea why. When I back out your patch, only one bugmail is generated (the one about dupe), with the correct content, as expected. Could you investigate?



One last thing. I applied your patch, then I copied template/en/ (with your patch applied) to template/fr/, and translated needed templates in french to better see how things work. And what I get is:


Fred <[e-mail]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|REOPENED                    |RESOLVED
         Resolution|                            |DUPLICATE


-- Commentaire #29 de Fred <[e-mail]> 2009-04-17 20:18:54 CEST --
*** Ce bogue a été marqué comme doublon du bogue 1 ***


Note that "Commentaire #29 de ..." and the following text are correctly translated, but text *before* the diff table is still in english, as well as table headers. Are you aware of this problem and plan to fix it in a separate bug or did you miss it as part of this bug? For the record, these lines are still generated by BugMail.pm at lines 263-264:

            $diffheader = "\n$fullwho changed:\n\n";
            $diffheader .= three_columns("What    ", "Removed", "Added");

Comment 9

[Marc Schumann [:Wurblzap]]

Attachment: Patch 1.3

(In reply to comment #8)
> Are you sure this work as expected and FILTER is only triggered if the
> condition is false? I mean, this syntax really means:

Turns out FILTER works like the pipe symbol, and is lower on the chain than the ?: operator.
Reworked.

> Now, the reason I deny review is because something strange happens with your

I can't reproduce.
I think this might happen if Bugzilla::Bug::format_comment() returns something else than an empty string, because it'll trigger Bugzilla::User::wants_bug_mail() with a non-empty string. In all my testing, it worked correctly, though.
Can you check whether you're having line ending conflicts? What does format_comment() return for each bug of your test case?

> Note that "Commentaire #29 de ..." and the following text are correctly
> translated, but text *before* the diff table is still in english, as well as
> table headers. Are you aware of this problem and plan to fix it in a separate
> bug or did you miss it as part of this bug? For the record, these lines are
> still generated by BugMail.pm at lines 263-264:

Right. See comment 0 -- all this is left for bug 215210.

Comment 10

[Frédéric Buclin]

Comment on attachment 373407 [details] [diff] [review]
Patch 1.3

>Index: Bugzilla/BugMail.pm

>                 if ($user->wants_bug_mail($id,
>                                           $relationship, 
>                                           $diffs, 
>-                                          $comments{$lang},
>+                                          $comments,
>                                           $deptext,
>                                           $changer,
>                                           !$start))

I found what's wrong with your patch: you replaced the string $comments{$lang}, which is generated by prepare_comments(), by the new arrayref $comments, which is generated by get_comments_by_bug(). But $user->wants_bug_mail() expects the comment argument to be a string, as it runs:

    elsif ($commentField ne '') {
        $events{+EVT_COMMENT} = 1;
    }

As an arrayref is not equal to '', it sets EVT_COMMENT and so a bugmail is sent (despite it shouldn't).

Comment 11

[Frédéric Buclin]

Comment on attachment 373407 [details] [diff] [review]
Patch 1.3

>Index: template/en/default/bug/format_comment.txt.tmpl

>+[% IF comment.already_wrapped %]
>+  [% comment.wrapped_body = comment.body %]
>+[% ELSE %]
>+  [% comment.wrapped_body = comment.body FILTER wrap_comment %]
>+[% END %]

Oh, one more thing: please don't add the wrapped comment to the hash. It will become an object soon and we got a similar problem with bug objects where adding new "keys" to the object was triggering crashes due to AUTOLOAD trying to detect a method with that name. Bugzilla::Bug::Comment won't have an AUTOLOAD, but I still prefer to store the wrapped comment in a separate variable, for safety, e.g. [% wrapped_comment = ... %]

Comment 12

[Marc Schumann [:Wurblzap]]

Attachment: Patch 1.4

Addressing comments. Thank you for your analysis, it was very helpful.

I didn't test your exact case, but I did test whether the comment list is being handled correctly.

Comment 13

[Frédéric Buclin]

Comment on attachment 378327 [details] [diff] [review]
Patch 1.4

>Index: Bugzilla/User.pm

>+    my $first_comment = ${$$ar_comments[0]}{body};

1) What means $ar_comments?? "ar_" is for... ?
2) $ar_comments->[0]->{body} is more readable, IMO, instead of three consecutive $$$ (I had to read it several times to understand what it was doing). Also, we use the arrow -> notation everywhere else.
3) Why do you restrict the check against the first comment only? This is a change compared to the current code, and I don't get it. And in that case, you could as well pass $comments->[0]->{body} to this method directly.

Comment 14

[Marc Schumann [:Wurblzap]]

Attachment: Patch 1.5

(In reply to comment #13)
> 1) What means $ar_comments?? "ar_" is for... ?

"Array Reference". Removed.

> 2) $ar_comments->[0]->{body} is more readable, IMO, instead of three
> consecutive $$$ (I had to read it several times to understand what it was
> doing). Also, we use the arrow -> notation everywhere else.

Gone away with (3).

> 3) Why do you restrict the check against the first comment only? This is a
> change compared to the current code, and I don't get it. And in that case, you
> could as well pass $comments->[0]->{body} to this method directly.

This was in error. I saw a leading ^ in the attachment regexp when there wasn't one.
Fixed.

Comment 15

[Frédéric Buclin]

Comment on attachment 379597 [details] [diff] [review]
Patch 1.5

>Index: Bugzilla/User.pm

>+    elsif (defined($$comments[0])) {
>         $events{+EVT_COMMENT} = 1;
>     }

Nit: why not checking for (defined $comments_concatenated) for consistency with the previous line? Otherwise looks good and works fine. My testcase described in comment 8 is now fixed. r=LpSolit \o/

Comment 16

[Marc Schumann [:Wurblzap]]

Great, thanks!

I believe join() doesn't return undef but an empty string if there is nothing to join. I can't test atm, so I'm checking in as is.

Tip:
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.278; previous revision: 1.277
done
Checking in Bugzilla/BugMail.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/BugMail.pm,v  <--  BugMail.pm
new revision: 1.125; previous revision: 1.124
done
Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.190; previous revision: 1.189
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/format_comm
ent.txt.tmpl,v
done
Checking in template/en/default/bug/format_comment.txt.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/format_comment.txt.tm
pl,v  <--  format_comment.txt.tmpl
initial revision: 1.1
done
Checking in template/en/default/email/newchangedmail.txt.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/email/newchangedmail.txt.
tmpl,v  <--  newchangedmail.txt.tmpl
new revision: 1.13; previous revision: 1.12
done
Checking in template/en/default/global/messages.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/messages.html.tmpl
,v  <--  messages.html.tmpl
new revision: 1.86; previous revision: 1.85
done

Branch:
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.276.2.2; previous revision: 1.276.2.1
done
Checking in Bugzilla/BugMail.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/BugMail.pm,v  <--  BugMail.pm
new revision: 1.124.2.1; previous revision: 1.124
done
Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.178.2.3; previous revision: 1.178.2.2
done
Checking in template/en/default/bug/format_comment.txt.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/format_comment.txt.tm
pl,v  <--  format_comment.txt.tmpl
new revision: 1.1.2.2; previous revision: 1.1.2.1
done
Checking in template/en/default/email/newchangedmail.txt.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/email/newchangedmail.txt.
tmpl,v  <--  newchangedmail.txt.tmpl
new revision: 1.12.2.1; previous revision: 1.12
done
Checking in template/en/default/global/messages.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/messages.html.tmpl
,v  <--  messages.html.tmpl
new revision: 1.83.2.1; previous revision: 1.83
done