Bug 2085398 - Read flag changes from flag_activity and stop writing them to bugs_activity

Bugzilla/Attachment.pm

@@ -855,8 +855,9 @@ sub update {
   # Record changes in the activity table.
   require Bugzilla::Bug;
   foreach my $field (keys %$changes) {
+    next if $field eq 'flagtypes.name';    # now tracked in flag_activity
     my $change = $changes->{$field};
-    $field = "attachments.$field" unless $field eq "flagtypes.name";
+    $field = "attachments.$field";
     Bugzilla::Bug::LogActivityEntry($self->bug_id, $field, $change->[0],
       $change->[1], $user->id, $timestamp, undef, $self->id);
   }

Bugzilla/Bug.pm

@@ -1221,6 +1221,7 @@ sub update {
   # XXX Eventually, when bugs_activity is able to track the dupe_id,
   # this code should go below the duplicates-table-updating code below.
   foreach my $field (keys %$changes) {
+    next if $field eq 'flagtypes.name';    # now tracked in flag_activity
     my $change = $changes->{$field};
     my $from   = defined $change->[0] ? $change->[0] : '';
     my $to     = defined $change->[1] ? $change->[1] : '';
@@ -4854,6 +4855,18 @@ sub GetBugActivity {
     $suppwhere = "AND COALESCE(attachments.isprivate, 0) = 0";
   }
 
+  my $flag_suppjoins = "";
+  my $flag_suppwhere = "";
+  if (!$user->is_insider) {
+    $flag_suppjoins = "LEFT JOIN attachments fa_att ON fa_att.attach_id = fsa.attachment_id";
+    $flag_suppwhere = "AND COALESCE(fa_att.isprivate, 0) = 0";
+  }
+
+  my $flag_attachpart = "";
+  if ($attach_id) {
+    $flag_attachpart = "AND fsa.attachment_id = ?";
+  }
+
   my $bug_when
     = $dbh->sql_date_format('bugs_activity.bug_when', '%Y-%m-%d %H:%i:%s');
 
@@ -4880,7 +4893,8 @@ sub GetBugActivity {
             ON profiles.userid = bugs_activity.who
          WHERE bugs_activity.bug_id = ?
                $attachpart
-               $suppwhere ";
+               $suppwhere
+               AND (fielddefs.name IS NULL OR fielddefs.name != 'flagtypes.name') ";
 
   if ( Bugzilla->params->{'comment_taggers_group'}
     && $include_comment_activity
@@ -4916,6 +4930,46 @@ sub GetBugActivity {
     push @args, $bug_id;
   }
 
+  my $flag_bug_when = $dbh->sql_date_format('fsa.flag_when', '%Y-%m-%d %H:%i:%s');
+  $query .= "
+        UNION ALL
+        SELECT 'flagtypes.name' AS field_name,
+               NULL AS activity_id,
+               fsa.attachment_id AS attach_id,
+               $flag_bug_when AS bug_when,
+               CASE WHEN prev.id IS NULL THEN ''
+                    WHEN prev.status = '?' AND preq.login_name IS NOT NULL
+                         THEN CONCAT(ft.name, '?(', preq.login_name, ')')
+                    ELSE CONCAT(ft.name, prev.status)
+               END AS removed,
+               CASE WHEN fsa.status = 'X' THEN ''
+                    WHEN fsa.status = '?' AND req.login_name IS NOT NULL
+                         THEN CONCAT(ft.name, '?(', req.login_name, ')')
+                    ELSE CONCAT(ft.name, fsa.status)
+               END AS added,
+               setter.login_name AS who,
+               NULL AS comment_id
+          FROM flag_activity fsa
+          JOIN flagtypes ft ON ft.id = fsa.type_id
+          JOIN profiles setter ON setter.userid = fsa.setter_id
+          LEFT JOIN profiles req ON req.userid = fsa.requestee_id
+          LEFT JOIN flag_activity prev
+                 ON fsa.flag_id != 0
+                AND prev.flag_id = fsa.flag_id
+                AND prev.flag_when = (
+                      SELECT MAX(p2.flag_when)
+                        FROM flag_activity p2
+                       WHERE p2.flag_id = fsa.flag_id
+                         AND p2.flag_when < fsa.flag_when)
+          LEFT JOIN profiles preq ON preq.userid = prev.requestee_id
+          $flag_suppjoins
+         WHERE fsa.bug_id = ?
+               $flag_attachpart
+               $flag_suppwhere
+      ";
+  push @args, $bug_id;
+  push @args, $attach_id if $attach_id;
+
   $query .= ") AS OUTER_TABLE";
 
   # Only consider changes since $starttime, if given.

Bugzilla/BugMail.pm

@@ -708,9 +708,11 @@ sub _get_diffs {
   my @args = ($bug->id);
 
   # If lastdiffed is NULL, then we don't limit the search on time.
-  my $when_restriction = '';
+  my $when_restriction      = '';
+  my $flag_when_restriction = '';
   if ($bug->lastdiffed) {
-    $when_restriction = ' AND bug_when > ? AND bug_when <= ?';
+    $when_restriction      = ' AND bug_when > ? AND bug_when <= ?';
+    $flag_when_restriction = ' AND fsa.flag_when > ? AND fsa.flag_when <= ?';
     push @args, ($bug->lastdiffed, $end);
   }
 
@@ -725,8 +727,40 @@ sub _get_diffs {
                 ON fielddefs.id = bugs_activity.fieldid
              WHERE bugs_activity.bug_id = ?
                    $when_restriction
-          ORDER BY bugs_activity.bug_when, fielddefs.description", {Slice => {}},
-    @args
+                   AND fielddefs.name != 'flagtypes.name'
+        UNION ALL
+        SELECT 'flagtypes.name' AS field_name,
+               (SELECT description FROM fielddefs WHERE name = 'flagtypes.name') AS field_desc,
+               fsa.flag_when AS bug_when,
+               CASE WHEN prev.id IS NULL THEN ''
+                    WHEN prev.status = '?' AND preq.login_name IS NOT NULL
+                         THEN CONCAT(ft.name, '?(', preq.login_name, ')')
+                    ELSE CONCAT(ft.name, prev.status)
+               END AS old,
+               CASE WHEN fsa.status = 'X' THEN ''
+                    WHEN fsa.status = '?' AND req.login_name IS NOT NULL
+                         THEN CONCAT(ft.name, '?(', req.login_name, ')')
+                    ELSE CONCAT(ft.name, fsa.status)
+               END AS new,
+               fsa.attachment_id AS attach_id,
+               NULL AS comment_id,
+               fsa.setter_id AS who
+          FROM flag_activity fsa
+          JOIN flagtypes ft ON ft.id = fsa.type_id
+          LEFT JOIN profiles req ON req.userid = fsa.requestee_id
+          LEFT JOIN flag_activity prev
+                 ON fsa.flag_id != 0
+                AND prev.flag_id = fsa.flag_id
+                AND prev.flag_when = (
+                      SELECT MAX(p2.flag_when)
+                        FROM flag_activity p2
+                       WHERE p2.flag_id = fsa.flag_id
+                         AND p2.flag_when < fsa.flag_when)
+          LEFT JOIN profiles preq ON preq.userid = prev.requestee_id
+         WHERE fsa.bug_id = ?
+               $flag_when_restriction
+          ORDER BY bug_when, field_desc", {Slice => {}},
+    @args, $bug->id, ($bug->lastdiffed ? ($bug->lastdiffed, $end) : ())
   );
   my $referenced_bugs = [];
 

editusers.cgi

@@ -20,6 +20,7 @@ use Bugzilla::User;
 use Bugzilla::Bug;
 use Bugzilla::BugMail;
 use Bugzilla::Flag;
+use Bugzilla::FlagActivity;
 use Bugzilla::Field;
 use Bugzilla::Group;
 use Bugzilla::Token;
@@ -521,13 +522,19 @@ elsif ($action eq 'delete') {
   push(@{$bugs{$_->bug_id}->{$_->attach_id || 0}}, $_) foreach @$flags;
   foreach my $bug_id (keys %bugs) {
     foreach my $attach_id (keys %{$bugs{$bug_id}}) {
-      my @old_summaries = Bugzilla::Flag->snapshot($bugs{$bug_id}->{$attach_id});
       $_->_set_requestee() foreach @{$bugs{$bug_id}->{$attach_id}};
-      my @new_summaries = Bugzilla::Flag->snapshot($bugs{$bug_id}->{$attach_id});
-      my ($removed, $added)
-        = Bugzilla::Flag->update_activity(\@old_summaries, \@new_summaries);
-      LogActivityEntry($bug_id, 'flagtypes.name', $removed, $added, $userid,
-        $timestamp, undef, $attach_id);
+      foreach my $flag (@{$bugs{$bug_id}->{$attach_id}}) {
+        Bugzilla::FlagActivity->create({
+          flag_when     => $timestamp,
+          setter_id     => $userid,
+          status        => $flag->status,
+          type_id       => $flag->type_id,
+          flag_id       => $flag->id,
+          requestee_id  => $flag->requestee_id,
+          bug_id        => $bug_id,
+          attachment_id => $attach_id || undef,
+        });
+      }
     }
     $sth_set_bug_timestamp->execute($timestamp, $bug_id);
     $updatedbugs{$bug_id} = 1;

extensions/BugModal/lib/ActivityStream.pm

@@ -96,35 +96,36 @@ sub find_activity_id_for_attachment {
 
 sub find_activity_id_for_flag {
   my ($self, $flag) = @_;
-  my $flagtype_name = $flag->type->name;
-  my $date          = $flag->modification_date;
-  my $setter_id     = $flag->setter->id;
-  my $stream        = $self->activity_stream;
+  my $flag_id   = $flag->id;
+  my $setter_id = $flag->setter->id;
+  my $stream    = $self->activity_stream;
 
-  # unfortunately bugs_activity treats all flag changes as the same field, so
-  # we don't have an object_id to match on
+  if (!exists $self->{activity_cache}->{flag}->{$flag_id}) {
+    # flag_activity records each change against the flag's object id, so we
+    # can look up the exact timestamp without relying on flag-type name matching.
+    my ($flag_when) = Bugzilla->dbh->selectrow_array(
+      "SELECT flag_when FROM flag_activity WHERE flag_id = ?
+       ORDER BY flag_when DESC LIMIT 1",
+      undef, $flag_id
+    );
 
-  if (!exists $self->{activity_cache}->{flag}->{$flag->id}) {
-    foreach my $change_set (reverse @$stream) {
+    SEARCH: foreach my $change_set (reverse @$stream) {
       foreach my $activity (@{$change_set->{activity}}) {
-
-        # match by user, timestamp, and flag-type name
-        next unless $activity->{who}->id == $setter_id && $activity->{when} eq $date;
+        next unless $flag_when
+          && $activity->{who}->id == $setter_id
+          && $activity->{when} eq $flag_when;
         foreach my $change (@{$activity->{changes}}) {
-          next
-            unless $change->{fieldname} eq 'flagtypes.name'
-            && $change->{flagtype_name} eq $flagtype_name;
-          $self->{activity_cache}->{flag}->{$flag->id} = $change_set->{id};
-          return $change_set->{id};
+          next unless $change->{fieldname} eq 'flagtypes.name';
+          $self->{activity_cache}->{flag}->{$flag_id} = $change_set->{id};
+          last SEARCH;
         }
       }
     }
 
-    # if we couldn't find the flag in bugs_activity it means it was set
-    # during bug creation
-    $self->{activity_cache}->{flag}->{$flag->id} = 'c0';
+    # flag set during bug creation, or no flag_activity entry yet
+    $self->{activity_cache}->{flag}->{$flag_id} //= 'c0';
   }
-  return $self->{activity_cache}->{flag}->{$flag->id};
+  return $self->{activity_cache}->{flag}->{$flag_id};
 }
 
 # comments are processed first, so there's no need to merge into existing entries