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.

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

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

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

Addressing comments.

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.

Addressing comments.

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 <LpSolit@netscape.net> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|REOPENED |RESOLVED Resolution| |DUPLICATE -- Commentaire #29 de Fred <LpSolit@netscape.net> 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");

(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 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 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 = ... %]

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

(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 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/

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