This affects trunk and branch. If I change time tracking information, an empty mail gets sent to non-timetrackinggroup members (i.e. only header, buglink and footer). (E-mail preferences set to receive all mail.) The changed field (I tried deadline) or its values are not included in the mail (which is good), but the header field X-Bugzilla-Changed-Fields mentions it (which is not good). If only time tracking information is changed in a bug, then no mail must be sent to non-timetrackinggroup members at all. If both time tracking information and other information is changed in a bug, then a mail must be sent to non-timetrackinggroup members but the X-Bugzilla-Changed-Fields mail header field must not disclose changed time tracking fields.

Definitely something we should look into. Nominating as a blocker (but not yet confirming) to keep it on our radar.

Iirc this used to work with 3.0.x or even some 3.2.x...

I just checked on 3.2.3. This works correctly there, so this bug is indeed a regression.

So let's block. Any idea which bug regressed this?

I don't know exactly, but I think a safe guess would be bug 457657.

I also made sure I didn't regress bug 73330, and I don't. The spacing remains exactly the same.

Comment on attachment 395675 [details] [diff] [review] patch, v1 >Index: template/en/default/email/newchangedmail.txt.tmpl >=================================================================== >RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/email/newchangedmail.txt.tmpl,v >retrieving revision 1.14 >diff -3 -p -u -r1.14 newchangedmail.txt.tmpl >--- template/en/default/email/newchangedmail.txt.tmpl 19 Aug 2009 04:45:05 -0000 1.14 >+++ template/en/default/email/newchangedmail.txt.tmpl 20 Aug 2009 21:05:10 -0000 >@@ -47,7 +47,9 @@ X-Bugzilla-Changed-Fields: [% changedfie > [% END -%] > [% FOREACH comment = new_comments %] > >+[%- IF comment.count %] That looks like a problem to me--I think we want to keep the extra space before the comment header, no?

(In reply to comment #7) > That looks like a problem to me--I think we want to keep the extra space > before the comment header, no? The extra space is still present in my testing.

Comment on attachment 395675 [details] [diff] [review] patch, v1 You are indeed correct, this works just fine.

Big oops! I just realized that I attached the right patch to the wrong bug. This patch is for bug 510798, not this one!

The regression is that $newcomments is now an arrayref, not a string, and so the check to decide if the email must be sent was wrong. I also had to move the linkification of "Created an attachment" in get_comments_by_bug() to do it only once, for the same reason as above.

(In reply to comment #5) > I don't know exactly, but I think a safe guess would be bug 457657. Yes, that's the one.

Comment on attachment 396057 [details] [diff] [review] patch, v1 I didn't test yet, here's my comments from looking at it. >Index: Bugzilla/BugMail.pm >+ if ($difftext eq "" && !scalar(@$newcomments) && !$isnew) { The second term doesn't really explain itself... Can this be written as (scalar(@$newcomments) == 0) or similar for readability? >@@ -687,9 +676,15 @@ sub get_comments_by_bug { > foreach my $comment (@$comments) { > $comment->{count} = $count++; >+ # If an attachment was created, then add an URL. (Note: the 'g'lobal >+ # replace should work with comments with multiple attachments.) >+ if ($comment->{body} =~ /Created an attachment \(/) { >+ $comment->{body} =~ s/(Created an attachment \(id=([0-9]+)\))/$1\n --> \($attach_base$2\)/g; >+ } Moving this to get_comments_by_bug means that other places get links, too. Is this what we want? I'm thinking maybe XML or webservice shouldn't get them?

(In reply to comment #13) > The second term doesn't really explain itself... Can this be written as > (scalar(@$newcomments) == 0) or similar for readability? No. I much prefer the !foo notation over foo == 0. !foo simply means "no comments". > Moving this to get_comments_by_bug means that other places get links, too. Is > this what we want? I'm thinking maybe XML or webservice shouldn't get them? Do XML and WebService call BugMail.pm at all?? get_comments_by_bug() is in BugMail.pm, not Bug.pm.

Marc: ping?

Comment on attachment 396057 [details] [diff] [review] patch, v1 Does that attachment link fix not have to do with this bug?

(In reply to comment #16) > Does that attachment link fix not have to do with this bug? I explained in comment 11 why this change is also needed.

Comment on attachment 396057 [details] [diff] [review] patch, v1 Okay. I haven't tested this, but it looks right.

wicked, does your Selenium script handle this special case, where only a time tracking field is edited with no other changes? tip: Checking in Bugzilla/BugMail.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/BugMail.pm,v <-- BugMail.pm new revision: 1.128; previous revision: 1.127 done 3.4.1: Checking in Bugzilla/BugMail.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/BugMail.pm,v <-- BugMail.pm new revision: 1.124.2.4; previous revision: 1.124.2.3 done