The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Bugzilla 3.4

Status

()

Bugzilla
Email Notifications
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: Wurblzap, Assigned: Frédéric Buclin)

Tracking

({regression})

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

Details

Attachments

(1 attachment, 1 obsolete attachment)

1.81 KB, patch
Max Kanat-Alexander
: review+
Details | Diff | Splinter Review
(Reporter)

Description

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

Comment 1

8 years ago
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

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

Comment 3

8 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

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

Comment 5

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

Comment 6

8 years ago
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.
Assignee: email-notifications → LpSolit
Status: NEW → ASSIGNED
Attachment #395675 - Flags: review?(mkanat)

Comment 7

8 years ago
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

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

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

You are indeed correct, this works just fine.
Attachment #395675 - Flags: review?(mkanat) → review+

Updated

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

Comment 10

8 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

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

Comment 11

8 years ago
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.
Attachment #396057 - Flags: review?(wurblzap)
(Assignee)

Comment 12

8 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

8 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

8 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

8 years ago
Marc: ping?
(Assignee)

Updated

8 years ago
Attachment #396057 - Flags: review?(mkanat)

Comment 16

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

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

Comment 17

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

8 years ago
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

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

Updated

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

Comment 19

8 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: 8 years ago
Flags: testcase?(wicked)
Resolution: --- → FIXED
(Assignee)

Updated

8 years ago
Blocks: 516041
You need to log in before you can comment on or make changes to this bug.