509035 - An empty e-mail gets sent to non-timetrackinggroup members if I change time tracking information

Status: RESOLVED FIXED

(Bugzilla :: Email Notifications, defect)

Description

[Marc Schumann [:Wurblzap]]

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.

Comment 1

[Max Kanat-Alexander]

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

Comment 2

[Marc Schumann [:Wurblzap]]

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

Comment 3

[Marc Schumann [:Wurblzap]]

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

Comment 4

[Frédéric Buclin]

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

Comment 5

[Marc Schumann [:Wurblzap]]

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

Comment 6

[Frédéric Buclin]

Attachment: patch, v1

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

Comment 7

[Max Kanat-Alexander]

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?

Comment 8

[Frédéric Buclin]

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

[Max Kanat-Alexander]

Comment on attachment 395675 [details] [diff] [review]
patch, v1

You are indeed correct, this works just fine.

Comment 10

[Frédéric Buclin]

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

Comment 11

[Frédéric Buclin]

Attachment: patch, v1

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.

Comment 12

[Frédéric Buclin]

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

[Marc Schumann [:Wurblzap]]

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?

Comment 14

[Frédéric Buclin]

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

Comment 15

[Frédéric Buclin]

Marc: ping?

Comment 16

[Max Kanat-Alexander]

Comment on attachment 396057 [details] [diff] [review]
patch, v1

Does that attachment link fix not have to do with this bug?

Comment 17

[Frédéric Buclin]

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

[Max Kanat-Alexander]

Comment on attachment 396057 [details] [diff] [review]
patch, v1

Okay. I haven't tested this, but it looks right.

Comment 19

[Frédéric Buclin]

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