Last Comment Bug 509035 - An empty e-mail gets sent to non-timetrackinggroup members if I change time tracking information
: An empty e-mail gets sent to non-timetrackinggroup members if I change time t...
Status: RESOLVED FIXED
: regression
Product: Bugzilla
Classification: Server Software
Component: Email Notifications (show other bugs)
: 3.4.1
: All All
: -- normal (vote)
: Bugzilla 3.4
Assigned To: Frédéric Buclin
: default-qa
Mentors:
Depends on: 457657
Blocks: 516041
  Show dependency treegraph
 
Reported: 2009-08-07 07:54 PDT by Marc Schumann [:Wurblzap]
Modified: 2010-02-28 10:58 PST (History)
4 users (show)
LpSolit: approval+
LpSolit: approval3.4+
LpSolit: blocking3.4.2+
LpSolit: testcase? (wicked)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch, v1 (786 bytes, patch)
2009-08-20 14:07 PDT, Frédéric Buclin
no flags Details | Diff | Splinter Review
patch, v1 (1.81 KB, patch)
2009-08-22 05:05 PDT, Frédéric Buclin
mkanat: review+
Details | Diff | Splinter Review

Description Marc Schumann [:Wurblzap] 2009-08-07 07:54:24 PDT
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 2009-08-10 21:46:03 PDT
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] 2009-08-10 22:29:55 PDT
Iirc this used to work with 3.0.x or even some 3.2.x...
Comment 3 Marc Schumann [:Wurblzap] 2009-08-13 07:11:38 PDT
I just checked on 3.2.3. This works correctly there, so this bug is indeed a regression.
Comment 4 Frédéric Buclin 2009-08-13 07:13:58 PDT
So let's block. Any idea which bug regressed this?
Comment 5 Marc Schumann [:Wurblzap] 2009-08-13 10:17:04 PDT
I don't know exactly, but I think a safe guess would be bug 457657.
Comment 6 Frédéric Buclin 2009-08-20 14:07:51 PDT
Created attachment 395675 [details] [diff] [review]
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 2009-08-20 15:35:10 PDT
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 2009-08-20 16:18:45 PDT
(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 2009-08-21 14:52:52 PDT
Comment on attachment 395675 [details] [diff] [review]
patch, v1

You are indeed correct, this works just fine.
Comment 10 Frédéric Buclin 2009-08-21 18:26:46 PDT
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 2009-08-22 05:05:59 PDT
Created attachment 396057 [details] [diff] [review]
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 2009-08-22 05:06:41 PDT
(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] 2009-08-27 01:45:08 PDT
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 2009-08-27 04:18:44 PDT
(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 2009-09-07 06:43:22 PDT
Marc: ping?
Comment 16 Max Kanat-Alexander 2009-09-08 08:48:27 PDT
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 2009-09-08 08:52:13 PDT
(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 2009-09-08 08:54:46 PDT
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 2009-09-08 09:12:50 PDT
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

Note You need to log in before you can comment on or make changes to this bug.