Skip to content

bugzilla
bugzilla
Commit b29f60f6 authored by Frédéric Buclin's avatar Frédéric Buclin
Browse files
Options

Bug 396558: Dependency change e-mails should only include status changes that happened right now

r/a=mkanat
parent 457f46da
Hide whitespace changes
Inline Side-by-side
Showing with 125 additions and 164 deletions
Bugzilla/Bug.pm
View file @ b29f60f6
...@@ -1154,14 +1154,22 @@ sub send_changes { ...@@ -1154,14 +1154,22 @@ sub send_changes {
# If there were changes in dependencies, we need to notify those # If there were changes in dependencies, we need to notify those
# dependencies. # dependencies.
my %notify_deps;
if ($changes->{'bug_status'}) { if ($changes->{'bug_status'}) {
my ($old_status, $new_status) = @{ $changes->{'bug_status'} }; my ($old_status, $new_status) = @{ $changes->{'bug_status'} };
# If this bug has changed from opened to closed or vice-versa, # If this bug has changed from opened to closed or vice-versa,
# then all of the bugs we block need to be notified. # then all of the bugs we block need to be notified.
if (is_open_state($old_status) ne is_open_state($new_status)) { if (is_open_state($old_status) ne is_open_state($new_status)) {
$notify_deps{$_} = 1 foreach (@{ $self->blocked }); my $params = { forced => { changer => $user },
type => 'dep',
dep_only => 1,
blocker => $self,
changes => $changes };
foreach my $id (@{ $self->blocked }) {
$params->{id} = $id;
_send_bugmail($params, $vars);
}
} }
} }
...@@ -1177,8 +1185,7 @@ sub send_changes { ...@@ -1177,8 +1185,7 @@ sub send_changes {
# an error later. # an error later.
delete $changed_deps{''}; delete $changed_deps{''};
my %all_dep_changes = (%notify_deps, %changed_deps); foreach my $id (sort { $a <=> $b } (keys %changed_deps)) {
foreach my $id (sort { $a <=> $b } (keys %all_dep_changes)) {
_send_bugmail({ forced => { changer => $user }, type => "dep", _send_bugmail({ forced => { changer => $user }, type => "dep",
id => $id }, $vars); id => $id }, $vars);
} }
...@@ -1188,7 +1195,7 @@ sub _send_bugmail { ...@@ -1188,7 +1195,7 @@ sub _send_bugmail {
my ($params, $vars) = @_; my ($params, $vars) = @_;
my $results = my $results =
Bugzilla::BugMail::Send($params->{'id'}, $params->{'forced'}); Bugzilla::BugMail::Send($params->{'id'}, $params->{'forced'}, $params);
if (Bugzilla->usage_mode == USAGE_MODE_BROWSER) { if (Bugzilla->usage_mode == USAGE_MODE_BROWSER) {
my $template = Bugzilla->template; my $template = Bugzilla->template;
......
Bugzilla/BugMail.pm
View file @ b29f60f6
...@@ -109,14 +109,12 @@ sub relationships { ...@@ -109,14 +109,12 @@ sub relationships {
# All the names are email addresses, not userids # All the names are email addresses, not userids
# values are scalars, except for cc, which is a list # values are scalars, except for cc, which is a list
sub Send { sub Send {
my ($id, $forced) = (@_); my ($id, $forced, $params) = @_;
$params ||= {};
my $dbh = Bugzilla->dbh; my $dbh = Bugzilla->dbh;
my $bug = new Bugzilla::Bug($id); my $bug = new Bugzilla::Bug($id);
# Only used for headers in bugmail for new bugs
my @fields = Bugzilla->get_fields({obsolete => 0, mailhead => 1});
my $start = $bug->lastdiffed; my $start = $bug->lastdiffed;
my $end = $dbh->selectrow_array('SELECT LOCALTIMESTAMP(0)'); my $end = $dbh->selectrow_array('SELECT LOCALTIMESTAMP(0)');
...@@ -146,142 +144,21 @@ sub Send { ...@@ -146,142 +144,21 @@ sub Send {
} }
} }
my @args = ($bug->id); my ($diffparts, $changedfields, $diffs) =
$params->{dep_only} ? ([], [], []) : _get_diffs($bug, $end);
# If lastdiffed is NULL, then we don't limit the search on time.
my $when_restriction = ''; if ($params->{dep_only} && $start) {
if ($start) { my $blocker_id = $params->{blocker}->id;
$when_restriction = ' AND bug_when > ? AND bug_when <= ?'; my $deptext =
push @args, ($start, $end); "\nBug " . $bug->id . " depends on bug $blocker_id, which changed state.\n\n" .
} "Bug $blocker_id Summary: " . $params->{blocker}->short_desc . "\n" .
correct_urlbase() . "show_bug.cgi?id=$blocker_id\n\n";
my $diffs = $dbh->selectall_arrayref( $deptext .= three_columns("What ", "Old Value", "New Value");
"SELECT profiles.login_name, profiles.realname, fielddefs.description, $deptext .= ('-' x 76) . "\n";
bugs_activity.bug_when, bugs_activity.removed, $deptext .= three_columns('Status', @{$params->{changes}->{bug_status}});
bugs_activity.added, bugs_activity.attach_id, fielddefs.name, $deptext .= three_columns('Resolution', @{$params->{changes}->{resolution}});
bugs_activity.comment_id
FROM bugs_activity push(@$diffparts, {text => $deptext});
INNER JOIN fielddefs
ON fielddefs.id = bugs_activity.fieldid
INNER JOIN profiles
ON profiles.userid = bugs_activity.who
WHERE bugs_activity.bug_id = ?
$when_restriction
ORDER BY bugs_activity.bug_when", undef, @args);
my @new_depbugs;
my $difftext = "";
my $diffheader = "";
my @diffparts;
my $lastwho = "";
my $fullwho;
my @changedfields;
foreach my $ref (@$diffs) {
my ($who, $whoname, $what, $when, $old, $new, $attachid, $fieldname, $comment_id) = (@$ref);
my $diffpart = {};
if ($who ne $lastwho) {
$lastwho = $who;
$fullwho = $whoname ? "$whoname <$who>" : $who;
$diffheader = "\n$fullwho changed:\n\n";
$diffheader .= three_columns("What ", "Removed", "Added");
$diffheader .= ('-' x 76) . "\n";
}
$what =~ s/^(Attachment )?/Attachment #$attachid / if $attachid;
if( $fieldname eq 'estimated_time' ||
$fieldname eq 'remaining_time' ) {
$old = format_time_decimal($old);
$new = format_time_decimal($new);
}
if ($fieldname eq 'dependson') {
push(@new_depbugs, grep {$_ =~ /^\d+$/} split(/[\s,]+/, $new));
}
if ($attachid) {
($diffpart->{'isprivate'}) = $dbh->selectrow_array(
'SELECT isprivate FROM attachments WHERE attach_id = ?',
undef, ($attachid));
}
if ($fieldname eq 'longdescs.isprivate') {
my $comment = Bugzilla::Comment->new($comment_id);
my $comment_num = $comment->count;
$what =~ s/^(Comment )?/Comment #$comment_num /;
$diffpart->{'isprivate'} = $new;
}
$difftext = three_columns($what, $old, $new);
$diffpart->{'header'} = $diffheader;
$diffpart->{'fieldname'} = $fieldname;
$diffpart->{'text'} = $difftext;
push(@diffparts, $diffpart);
push(@changedfields, $what);
}
my @depbugs;
my $deptext = "";
# Do not include data about dependent bugs when they have just been added.
# Completely skip checking for dependent bugs on bug creation as all
# dependencies bugs will just have been added.
if ($start) {
my $dep_restriction = "";
if (scalar @new_depbugs) {
$dep_restriction = "AND bugs_activity.bug_id NOT IN (" .
join(", ", @new_depbugs) . ")";
}
my $dependency_diffs = $dbh->selectall_arrayref(
"SELECT bugs_activity.bug_id, bugs.short_desc, fielddefs.name,
fielddefs.description, bugs_activity.removed,
bugs_activity.added
FROM bugs_activity
INNER JOIN bugs
ON bugs.bug_id = bugs_activity.bug_id
INNER JOIN dependencies
ON bugs_activity.bug_id = dependencies.dependson
INNER JOIN fielddefs
ON fielddefs.id = bugs_activity.fieldid
WHERE dependencies.blocked = ?
AND (fielddefs.name = 'bug_status'
OR fielddefs.name = 'resolution')
$when_restriction
$dep_restriction
ORDER BY bugs_activity.bug_when, bugs.bug_id", undef, @args);
my $thisdiff = "";
my $lastbug = "";
my $interestingchange = 0;
foreach my $dependency_diff (@$dependency_diffs) {
my ($depbug, $summary, $fieldname, $what, $old, $new) = @$dependency_diff;
if ($depbug ne $lastbug) {
if ($interestingchange) {
$deptext .= $thisdiff;
}
$lastbug = $depbug;
$thisdiff =
"\nBug $id depends on bug $depbug, which changed state.\n\n" .
"Bug $depbug Summary: $summary\n" .
correct_urlbase() . "show_bug.cgi?id=$depbug\n\n";
$thisdiff .= three_columns("What ", "Old Value", "New Value");
$thisdiff .= ('-' x 76) . "\n";
$interestingchange = 0;
}
$thisdiff .= three_columns($what, $old, $new);
if ($fieldname eq 'bug_status'
&& is_open_state($old) ne is_open_state($new))
{
$interestingchange = 1;
}
push(@depbugs, $depbug);
}
if ($interestingchange) {
$deptext .= $thisdiff;
}
$deptext = trim($deptext);
if ($deptext) {
my $diffpart = {};
$diffpart->{'text'} = "\n" . trim($deptext);
push(@diffparts, $diffpart);
}
} }
my $comments = $bug->comments({ after => $start, to => $end }); my $comments = $bug->comments({ after => $start, to => $end });
...@@ -374,6 +251,9 @@ sub Send { ...@@ -374,6 +251,9 @@ sub Send {
my @sent; my @sent;
my @excluded; my @excluded;
# Only used for headers in bugmail for new bugs
my @fields = $start ? () : Bugzilla->get_fields({obsolete => 0, mailhead => 1});
foreach my $user_id (keys %recipients) { foreach my $user_id (keys %recipients) {
my %rels_which_want; my %rels_which_want;
my $sent_mail = 0; my $sent_mail = 0;
...@@ -389,7 +269,7 @@ sub Send { ...@@ -389,7 +269,7 @@ sub Send {
$relationship, $relationship,
$diffs, $diffs,
$comments, $comments,
$deptext, $params->{dep_only},
$changer, $changer,
!$start)) !$start))
{ {
...@@ -403,16 +283,12 @@ sub Send { ...@@ -403,16 +283,12 @@ sub Send {
# So the user exists, can see the bug, and wants mail in at least # So the user exists, can see the bug, and wants mail in at least
# one role. But do we want to send it to them? # one role. But do we want to send it to them?
# We shouldn't send mail if this is a dependency mail (i.e. there # We shouldn't send mail if this is a dependency mail and the
# is something in @depbugs), and any of the depending bugs are not # depending bug is not visible to the user.
# visible to the user. This is to avoid leaking the summaries of # This is to avoid leaking the summary of a confidential bug.
# confidential bugs.
my $dep_ok = 1; my $dep_ok = 1;
foreach my $dep_id (@depbugs) { if ($params->{dep_only}) {
if (!$user->can_see_bug($dep_id)) { $dep_ok = $user->can_see_bug($params->{blocker}->id) ? 1 : 0;
$dep_ok = 0;
last;
}
} }
# Make sure the user isn't in the nomail list, and the insider and # Make sure the user isn't in the nomail list, and the insider and
...@@ -425,12 +301,13 @@ sub Send { ...@@ -425,12 +301,13 @@ sub Send {
bug => $bug, bug => $bug,
comments => $comments, comments => $comments,
is_new => !$start, is_new => !$start,
delta_ts => $params->{dep_only} ? $end : undef,
changer => $changer, changer => $changer,
watchers => exists $watching{$user_id} ? watchers => exists $watching{$user_id} ?
$watching{$user_id} : undef, $watching{$user_id} : undef,
diff_parts => \@diffparts, diff_parts => $diffparts,
rels_which_want => \%rels_which_want, rels_which_want => \%rels_which_want,
changed_fields => \@changedfields, changed_fields => $changedfields,
}); });
} }
} }
...@@ -442,10 +319,15 @@ sub Send { ...@@ -442,10 +319,15 @@ sub Send {
push(@excluded, $user->login); push(@excluded, $user->login);
} }
} }
$dbh->do('UPDATE bugs SET lastdiffed = ? WHERE bug_id = ?', # When sending bugmail about a blocker being reopened or resolved,
undef, ($end, $id)); # we say nothing about changes in the bug being blocked, so we must
$bug->{lastdiffed} = $end; # not update lastdiffed in this case.
if (!$params->{dep_only}) {
$dbh->do('UPDATE bugs SET lastdiffed = ? WHERE bug_id = ?',
undef, ($end, $id));
$bug->{lastdiffed} = $end;
}
return {'sent' => \@sent, 'excluded' => \@excluded}; return {'sent' => \@sent, 'excluded' => \@excluded};
} }
...@@ -458,6 +340,7 @@ sub sendMail { ...@@ -458,6 +340,7 @@ sub sendMail {
my $bug = $params->{bug}; my $bug = $params->{bug};
my @send_comments = @{ $params->{comments} }; my @send_comments = @{ $params->{comments} };
my $isnew = $params->{is_new}; my $isnew = $params->{is_new};
my $delta_ts = $params->{delta_ts};
my $changer = $params->{changer}; my $changer = $params->{changer};
my $watchingRef = $params->{watchers}; my $watchingRef = $params->{watchers};
my @diffparts = @{ $params->{diff_parts} }; my @diffparts = @{ $params->{diff_parts} };
...@@ -558,6 +441,7 @@ sub sendMail { ...@@ -558,6 +441,7 @@ sub sendMail {
my $vars = { my $vars = {
isnew => $isnew, isnew => $isnew,
delta_ts => $delta_ts,
to_user => $user, to_user => $user,
bug => $bug, bug => $bug,
changedfields => \@changed_fields, changedfields => \@changed_fields,
...@@ -581,4 +465,74 @@ sub sendMail { ...@@ -581,4 +465,74 @@ sub sendMail {
return 1; return 1;
} }
sub _get_diffs {
my ($bug, $end) = @_;
my $dbh = Bugzilla->dbh;
my @args = ($bug->id);
# If lastdiffed is NULL, then we don't limit the search on time.
my $when_restriction = '';
if ($bug->lastdiffed) {
$when_restriction = ' AND bug_when > ? AND bug_when <= ?';
push @args, ($bug->lastdiffed, $end);
}
my $diffs = $dbh->selectall_arrayref(
"SELECT profiles.login_name, profiles.realname, fielddefs.description,
bugs_activity.bug_when, bugs_activity.removed,
bugs_activity.added, bugs_activity.attach_id, fielddefs.name,
bugs_activity.comment_id
FROM bugs_activity
INNER JOIN fielddefs
ON fielddefs.id = bugs_activity.fieldid
INNER JOIN profiles
ON profiles.userid = bugs_activity.who
WHERE bugs_activity.bug_id = ?
$when_restriction
ORDER BY bugs_activity.bug_when", undef, @args);
my $difftext = "";
my $diffheader = "";
my @diffparts;
my $lastwho = "";
my $fullwho;
my @changedfields;
foreach my $ref (@$diffs) {
my ($who, $whoname, $what, $when, $old, $new, $attachid, $fieldname, $comment_id) = @$ref;
my $diffpart = {};
if ($who ne $lastwho) {
$lastwho = $who;
$fullwho = $whoname ? "$whoname <$who>" : $who;
$diffheader = "\n$fullwho changed:\n\n";
$diffheader .= three_columns("What ", "Removed", "Added");
$diffheader .= ('-' x 76) . "\n";
}
$what =~ s/^(Attachment )?/Attachment #$attachid / if $attachid;
if( $fieldname eq 'estimated_time' ||
$fieldname eq 'remaining_time' ) {
$old = format_time_decimal($old);
$new = format_time_decimal($new);
}
if ($attachid) {
($diffpart->{'isprivate'}) = $dbh->selectrow_array(
'SELECT isprivate FROM attachments WHERE attach_id = ?',
undef, ($attachid));
}
if ($fieldname eq 'longdescs.isprivate') {
my $comment = Bugzilla::Comment->new($comment_id);
my $comment_num = $comment->count;
$what =~ s/^(Comment )?/Comment #$comment_num /;
$diffpart->{'isprivate'} = $new;
}
$difftext = three_columns($what, $old, $new);
$diffpart->{'header'} = $diffheader;
$diffpart->{'fieldname'} = $fieldname;
$diffpart->{'text'} = $difftext;
push(@diffparts, $diffpart);
push(@changedfields, $what);
}
return (\@diffparts, \@changedfields, $diffs);
}
1; 1;
Bugzilla/User.pm
View file @ b29f60f6
...@@ -1513,7 +1513,7 @@ our %names_to_events = ( ...@@ -1513,7 +1513,7 @@ our %names_to_events = (
# Note: the "+" signs before the constants suppress bareword quoting. # Note: the "+" signs before the constants suppress bareword quoting.
sub wants_bug_mail { sub wants_bug_mail {
my $self = shift; my $self = shift;
my ($bug_id, $relationship, $fieldDiffs, $comments, $dependencyText, my ($bug_id, $relationship, $fieldDiffs, $comments, $dep_mail,
$changer, $bug_is_new) = @_; $changer, $bug_is_new) = @_;
# Make a list of the events which have happened during this bug change, # Make a list of the events which have happened during this bug change,
...@@ -1572,7 +1572,7 @@ sub wants_bug_mail { ...@@ -1572,7 +1572,7 @@ sub wants_bug_mail {
# Dependent changed bugmails must have an event to ensure the bugmail is # Dependent changed bugmails must have an event to ensure the bugmail is
# emailed. # emailed.
if ($dependencyText ne '') { if ($dep_mail) {
$events{+EVT_DEPEND_BLOCK} = 1; $events{+EVT_DEPEND_BLOCK} = 1;
} }
......
template/en/default/email/newchangedmail.txt.tmpl
View file @ b29f60f6
...@@ -24,7 +24,7 @@ ...@@ -24,7 +24,7 @@
From: [% Param('mailfrom') %] From: [% Param('mailfrom') %]
To: [% to_user.email %] To: [% to_user.email %]
Subject: [[% terms.Bug %] [%+ bug.id %]] [% 'New: ' IF isnew %][%+ bug.short_desc %] Subject: [[% terms.Bug %] [%+ bug.id %]] [% 'New: ' IF isnew %][%+ bug.short_desc %]
Date: [% bug.delta_ts FILTER time("%a, %d %b %Y %T %z", to_user.timezone) %] Date: [% delta_ts || bug.delta_ts FILTER time("%a, %d %b %Y %T %z", to_user.timezone) %]
X-Bugzilla-Reason: [% reasonsheader %] X-Bugzilla-Reason: [% reasonsheader %]
X-Bugzilla-Type: [% isnew ? 'new' : 'changed' %] X-Bugzilla-Type: [% isnew ? 'new' : 'changed' %]
X-Bugzilla-Watch-Reason: [% reasonswatchheader %] X-Bugzilla-Watch-Reason: [% reasonswatchheader %]
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment