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

RESOLVED FIXED in Bugzilla 3.4

Status

()

defect
RESOLVED FIXED
10 years ago
3 months ago

People

(Reporter: Wurblzap, Assigned: LpSolit)

Tracking

({regression})

Dependency tree / graph
Bug Flags:
approval +
approval3.4 +
blocking3.4.2 +
testcase ?

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

10 years ago
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.
Flags: blocking3.4.2?
Target Milestone: --- → Bugzilla 3.4
(Reporter)

Comment 2

10 years ago
Iirc this used to work with 3.0.x or even some 3.2.x...
(Reporter)

Comment 3

10 years ago
I just checked on 3.2.3. This works correctly there, so this bug is indeed a regression.
Keywords: regression
(Assignee)

Comment 4

10 years ago
So let's block. Any idea which bug regressed this?
Flags: blocking3.4.2? → blocking3.4.2+
(Reporter)

Comment 5

10 years ago
I don't know exactly, but I think a safe guess would be bug 457657.
(Assignee)

Comment 6

10 years ago
Posted patch patch, v1 (obsolete) — Splinter Review
I also made sure I didn't regress bug 73330, and I don't. The spacing remains exactly the same.
Assignee: email-notifications → LpSolit
Status: NEW → ASSIGNED
Attachment #395675 - Flags: review?(mkanat)
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?
(Assignee)

Comment 8

10 years ago
(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.
Attachment #395675 - Flags: review?(mkanat) → review+

Updated

10 years ago
Flags: approval3.4+
Flags: approval+
(Assignee)

Comment 10

10 years ago
Big oops! I just realized that I attached the right patch to the wrong bug. This patch is for bug 510798, not this one!
Flags: approval3.4+
Flags: approval+
(Assignee)

Updated

10 years ago
Attachment #395675 - Attachment is obsolete: true
Attachment #395675 - Flags: review+
(Assignee)

Comment 11

10 years ago
Posted patch patch, v1Splinter Review
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.
Attachment #396057 - Flags: review?(wurblzap)
(Assignee)

Comment 12

10 years ago
(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.
Depends on: 457657
(Reporter)

Comment 13

10 years ago
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?
(Assignee)

Comment 14

10 years ago
(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.
(Assignee)

Comment 15

10 years ago
Marc: ping?
(Assignee)

Updated

10 years ago
Attachment #396057 - Flags: review?(mkanat)
Comment on attachment 396057 [details] [diff] [review]
patch, v1

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

Comment 17

10 years ago
(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.
Attachment #396057 - Flags: review?(mkanat) → review+
(Assignee)

Updated

10 years ago
Attachment #396057 - Flags: review?(wurblzap)
(Assignee)

Updated

10 years ago
Flags: approval3.4+
Flags: approval+
(Assignee)

Comment 19

10 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Flags: testcase?(wicked)
Resolution: --- → FIXED
(Assignee)

Updated

10 years ago
Blocks: 516041
Flags: testcase?(wicked) → testcase?
You need to log in before you can comment on or make changes to this bug.