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

r/a=mkanat

3 files changed, 124 insertions, 163 deletions

diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm
index fce2a1634..6638573c6 100644
--- a/Bugzilla/Bug.pm
+++ b/Bugzilla/Bug.pm
@@ -1154,14 +1154,22 @@ sub send_changes {
 
     # If there were changes in dependencies, we need to notify those
     # dependencies.
-    my %notify_deps;
     if ($changes->{'bug_status'}) {
         my ($old_status, $new_status) = @{ $changes->{'bug_status'} };
 
         # If this bug has changed from opened to closed or vice-versa,
         # then all of the bugs we block need to be notified.
         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 {
     # an error later.
     delete $changed_deps{''};
 
-    my %all_dep_changes = (%notify_deps, %changed_deps);
-    foreach my $id (sort { $a <=> $b } (keys %all_dep_changes)) {
+    foreach my $id (sort { $a <=> $b } (keys %changed_deps)) {
         _send_bugmail({ forced => { changer => $user }, type => "dep",
                          id => $id }, $vars);
     }
@@ -1188,7 +1195,7 @@ sub _send_bugmail {
     my ($params, $vars) = @_;
 
     my $results = 
-        Bugzilla::BugMail::Send($params->{'id'}, $params->{'forced'});
+        Bugzilla::BugMail::Send($params->{'id'}, $params->{'forced'}, $params);
 
     if (Bugzilla->usage_mode == USAGE_MODE_BROWSER) {
         my $template = Bugzilla->template;
diff --git a/Bugzilla/BugMail.pm b/Bugzilla/BugMail.pm
index b2482858a..3e97b4457 100644
--- a/Bugzilla/BugMail.pm
+++ b/Bugzilla/BugMail.pm
@@ -109,14 +109,12 @@ sub relationships {
 # All the names are email addresses, not userids
 # values are scalars, except for cc, which is a list
 sub Send {
-    my ($id, $forced) = (@_);
+    my ($id, $forced, $params) = @_;
+    $params ||= {};
 
     my $dbh = Bugzilla->dbh;
     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 $end   = $dbh->selectrow_array('SELECT LOCALTIMESTAMP(0)');
 
@@ -146,142 +144,21 @@ sub Send {
         }
     }
     
-    my @args = ($bug->id);
-
-    # If lastdiffed is NULL, then we don't limit the search on time.
-    my $when_restriction = '';
-    if ($start) {
-        $when_restriction = ' AND bug_when > ? AND bug_when <= ?';
-        push @args, ($start, $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 @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 ($diffparts, $changedfields, $diffs) =
+      $params->{dep_only} ? ([], [], []) : _get_diffs($bug, $end);
+
+    if ($params->{dep_only} && $start) {
+        my $blocker_id = $params->{blocker}->id;
+        my $deptext =
+          "\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";
+        $deptext .= three_columns("What    ", "Old Value", "New Value");
+        $deptext .= ('-' x 76) . "\n";
+        $deptext .= three_columns('Status', @{$params->{changes}->{bug_status}});
+        $deptext .= three_columns('Resolution', @{$params->{changes}->{resolution}});
+
+        push(@$diffparts, {text => $deptext});
     }
 
     my $comments = $bug->comments({ after => $start, to => $end });
@@ -374,6 +251,9 @@ sub Send {
     my @sent;
     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) {
         my %rels_which_want;
         my $sent_mail = 0;
@@ -389,7 +269,7 @@ sub Send {
                                           $relationship, 
                                           $diffs, 
                                           $comments,
-                                          $deptext,
+                                          $params->{dep_only},
                                           $changer,
                                           !$start))
                 {
@@ -403,16 +283,12 @@ sub Send {
             # 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?
 
-            # We shouldn't send mail if this is a dependency mail (i.e. there 
-            # is something in @depbugs), and any of the depending bugs are not 
-            # visible to the user. This is to avoid leaking the summaries of 
-            # confidential bugs.
+            # We shouldn't send mail if this is a dependency mail and the
+            # depending bug is not visible to the user.
+            # This is to avoid leaking the summary of a confidential bug.
             my $dep_ok = 1;
-            foreach my $dep_id (@depbugs) {
-                if (!$user->can_see_bug($dep_id)) {
-                   $dep_ok = 0;
-                   last;
-                }
+            if ($params->{dep_only}) {
+                $dep_ok = $user->can_see_bug($params->{blocker}->id) ? 1 : 0;
             }
 
             # Make sure the user isn't in the nomail list, and the insider and 
@@ -425,12 +301,13 @@ sub Send {
                       bug      => $bug,
                       comments => $comments,
                       is_new   => !$start,
+                      delta_ts => $params->{dep_only} ? $end : undef,
                       changer  => $changer,
                       watchers => exists $watching{$user_id} ?
                                   $watching{$user_id} : undef,
-                      diff_parts      => \@diffparts,
+                      diff_parts      => $diffparts,
                       rels_which_want => \%rels_which_want,
-                      changed_fields  => \@changedfields,
+                      changed_fields  => $changedfields,
                     });
             }
         }
@@ -442,10 +319,15 @@ sub Send {
             push(@excluded, $user->login); 
         } 
     }
-    
-    $dbh->do('UPDATE bugs SET lastdiffed = ? WHERE bug_id = ?',
-             undef, ($end, $id));
-    $bug->{lastdiffed} = $end;
+
+    # When sending bugmail about a blocker being reopened or resolved,
+    # we say nothing about changes in the bug being blocked, so we must
+    # 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};
 }
@@ -458,6 +340,7 @@ sub sendMail {
     my $bug    = $params->{bug};
     my @send_comments = @{ $params->{comments} };
     my $isnew   = $params->{is_new};
+    my $delta_ts = $params->{delta_ts};
     my $changer = $params->{changer};
     my $watchingRef = $params->{watchers};
     my @diffparts   = @{ $params->{diff_parts} };
@@ -558,6 +441,7 @@ sub sendMail {
 
     my $vars = {
         isnew => $isnew,
+        delta_ts => $delta_ts,
         to_user => $user,
         bug => $bug,
         changedfields => \@changed_fields,
@@ -581,4 +465,74 @@ sub sendMail {
     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;
diff --git a/Bugzilla/User.pm b/Bugzilla/User.pm
index 09af233ec..2bbe63101 100644
--- a/Bugzilla/User.pm
+++ b/Bugzilla/User.pm
@@ -1513,7 +1513,7 @@ our %names_to_events = (
 # Note: the "+" signs before the constants suppress bareword quoting.
 sub wants_bug_mail {
     my $self = shift;
-    my ($bug_id, $relationship, $fieldDiffs, $comments, $dependencyText,
+    my ($bug_id, $relationship, $fieldDiffs, $comments, $dep_mail,
         $changer, $bug_is_new) = @_;
 
     # Make a list of the events which have happened during this bug change,
@@ -1572,7 +1572,7 @@ sub wants_bug_mail {
     
     # Dependent changed bugmails must have an event to ensure the bugmail is
     # emailed.
-    if ($dependencyText ne '') {
+    if ($dep_mail) {
         $events{+EVT_DEPEND_BLOCK} = 1;
     }