Bug 2079991 - When one update requests needinfo from several users , the webhook's 'event.changes' has a single 'flag.needinfo' entry for the last requestee only; the others are dropped.

extensions/Push/Extension.pm

@@ -24,6 +24,7 @@ use Bugzilla::Extension::Push::Push;
 use Bugzilla::Extension::Push::Serialize;
 use Bugzilla::Extension::Push::Util;
 use Bugzilla::Install::Filesystem;
+use Bugzilla::Util qw(diff_arrays);
 
 use Encode;
 use Scalar::Util 'blessed';
@@ -144,24 +145,24 @@ sub _object_modified {
   }
 
   # make flagtypes changes easier to process
+  my @field_changes;
   if (exists $changes->{'flagtypes.name'}) {
-    _split_flagtypes($changes);
+    @field_changes = _split_flagtypes($changes);
   }
 
   # TODO split group changes?
 
   # restructure the changes hash
+  push @field_changes,
+    map { [$_, $changes->{$_}[0], $changes->{$_}[1]] } keys %$changes;
   my $changes_data = {timestamp => $args->{'timestamp'}, changes => [],};
-  foreach my $field_name (sort keys %$changes) {
+  foreach my $change (sort { $a->[0] cmp $b->[0] } @field_changes) {
+    my ($field_name, $removed, $added) = @$change;
     my $new_field_name = $field_name;
     $new_field_name =~ s/isprivate/is_private/;
 
     push @{$changes_data->{'changes'}},
-      {
-      field   => $new_field_name,
-      removed => $changes->{$field_name}[0],
-      added   => $changes->{$field_name}[1],
-      };
+      {field => $new_field_name, removed => $removed, added => $added,};
   }
 
   $self->_push_object('modify', $object, $change_set, $changes_data);
@@ -181,25 +182,39 @@ sub _should_push {
 }
 
 # changes to bug flags are presented in a single field 'flagtypes.name' split
-# into individual fields
+# into individual changes.  a flag type can be changed more than once in a
+# single update (eg. needinfo requested from several users), so each flag gets
+# its own change rather than being keyed by flag name.
 sub _split_flagtypes {
   my ($changes) = @_;
 
-  my @removed = _split_flagtype($changes->{'flagtypes.name'}->[0]);
-  my @added   = _split_flagtype($changes->{'flagtypes.name'}->[1]);
-  delete $changes->{'flagtypes.name'};
-
-  foreach my $ra (@removed, @added) {
-    $changes->{$ra->[0]} = ['', ''];
-  }
-  foreach my $ra (@removed) {
+  my ($removed, $added) = @{delete $changes->{'flagtypes.name'}};
+  my %values;
+  foreach my $ra (_split_flagtype($removed)) {
     my ($name, $value) = @$ra;
-    $changes->{$name}->[0] = $value;
+    push @{$values{$name}{removed}}, $value;
   }
-  foreach my $ra (@added) {
+  foreach my $ra (_split_flagtype($added)) {
     my ($name, $value) = @$ra;
-    $changes->{$name}->[1] = $value;
+    push @{$values{$name}{added}}, $value;
+  }
+
+  # only pair when a flag type has exactly one removed and one added value, so a
+  # single flag changing state (eg. review? to review+) is still one change.
+  # otherwise we can't tell which removal goes with which addition.
+  my @flag_changes;
+  foreach my $name (sort keys %values) {
+    my @removed = @{$values{$name}{removed} || []};
+    my @added   = @{$values{$name}{added}   || []};
+    if (@removed == 1 && @added == 1) {
+      push @flag_changes, [$name, $removed[0], $added[0]];
+    }
+    else {
+      push @flag_changes, map { [$name, $_, ''] } @removed;
+      push @flag_changes, map { [$name, '', $_] } @added;
+    }
   }
+  return @flag_changes;
 }
 
 sub _split_flagtype {
@@ -219,52 +234,25 @@ sub _split_flagtype {
 
 # changes to attachment flags come in via flag_end_of_update which has a
 # completely different structure for reporting changes than
-# object_end_of_update.  this morphs flag to object updates.
+# object_end_of_update.  this morphs flag to object updates, using the same
+# 'flagtypes.name' format as bug flag changes.
 sub _morph_flag_updates {
   my ($args) = @_;
 
-  my @removed = _morph_flag_update($args->{'old_flags'});
-  my @added   = _morph_flag_update($args->{'new_flags'});
+  # strip the setter so a flag re-set by a different user isn't a change
+  my @old_flags = @{$args->{'old_flags'}};
+  my @new_flags = @{$args->{'new_flags'}};
+  s/^[^:]+:// foreach (@old_flags, @new_flags);
 
-  my $changes = {};
-  foreach my $ra (@removed, @added) {
-    $changes->{$ra->[0]} = ['', ''];
-  }
-  foreach my $ra (@removed) {
-    my ($name, $value) = @$ra;
-    $changes->{$name}->[0] = $value;
-  }
-  foreach my $ra (@added) {
-    my ($name, $value) = @$ra;
-    $changes->{$name}->[1] = $value;
-  }
+  my ($removed, $added) = diff_arrays(\@old_flags, \@new_flags);
 
-  foreach my $flag (keys %$changes) {
-    if ($changes->{$flag}->[0] eq $changes->{$flag}->[1]) {
-      delete $changes->{$flag};
-    }
+  my $changes = {};
+  if (@$removed || @$added) {
+    $changes->{'flagtypes.name'} = [join(', ', @$removed), join(', ', @$added)];
   }
-
   $args->{'changes'} = $changes;
 }
 
-sub _morph_flag_update {
-  my ($values) = @_;
-  my @result;
-  foreach my $orig_change (@$values) {
-    my $change = $orig_change;    # work on a copy
-    $change =~ s/^[^:]+://;
-    my $requestee = '';
-    if ($change =~ s/\(([^\)]+)\)$//) {
-      $requestee = $1;
-    }
-    my ($name, $value) = $change =~ /^(.+)(.)$/;
-    $value .= " ($requestee)" if $requestee;
-    push @result, ["flag.$name", $value];
-  }
-  return @result;
-}
-
 #
 # serialize and insert into the table
 #
@@ -292,8 +280,11 @@ sub _push_object {
   $rh_event->{'change_set'}  = $change_set;
   $rh_event->{'routing_key'} = "$name.$message_type";
   if (exists $rh_event->{'changes'}) {
-    $rh_event->{'routing_key'}
-      .= ':' . join(',', map { $_->{'field'} } @{$rh_event->{'changes'}});
+    # a field can have more than one change (eg. multiple flags of one type)
+    my %seen;
+    $rh_event->{'routing_key'} .= ':'
+      . join(',',
+      grep { !$seen{$_}++ } map { $_->{'field'} } @{$rh_event->{'changes'}});
   }
   $rh->{'event'} = $rh_event;
 

extensions/Push/t/flag_changes.t

@@ -0,0 +1,96 @@
+#!/usr/bin/env perl
+# This Source Code Form is subject to the terms of the Mozilla Public
+# License, v. 2.0. If a copy of the MPL was not distributed with this
+# file, You can obtain one at http://mozilla.org/MPL/2.0/.
+#
+# This Source Code Form is "Incompatible With Secondary Licenses", as
+# defined by the Mozilla Public License, v. 2.0.
+
+use 5.10.1;
+use strict;
+use warnings;
+use lib qw( . lib local/lib/perl5 );
+
+use Bugzilla;
+BEGIN { Bugzilla->extensions }
+
+use Test2::V0;
+
+sub split_flagtypes {
+  my ($removed, $added) = @_;
+  my $changes = {'flagtypes.name' => [$removed, $added]};
+  my @result = Bugzilla::Extension::Push::_split_flagtypes($changes);
+  ok(!exists $changes->{'flagtypes.name'}, 'flagtypes.name is removed');
+  return \@result;
+}
+
+sub morph_flag_updates {
+  my ($old_flags, $new_flags) = @_;
+  my $args = {old_flags => $old_flags, new_flags => $new_flags};
+  Bugzilla::Extension::Push::_morph_flag_updates($args);
+  return $args->{changes};
+}
+
+is(
+  split_flagtypes('', 'needinfo?(a@example.com)'),
+  [['flag.needinfo', '', '? (a@example.com)']],
+  'single needinfo request'
+);
+
+is(
+  split_flagtypes('', 'needinfo?(a@example.com), needinfo?(b@example.com)'),
+  [
+    ['flag.needinfo', '', '? (a@example.com)'],
+    ['flag.needinfo', '', '? (b@example.com)'],
+  ],
+  'needinfo requested from two users gives one change per requestee'
+);
+
+is(
+  split_flagtypes('needinfo?(a@example.com), needinfo?(b@example.com)', ''),
+  [
+    ['flag.needinfo', '? (a@example.com)', ''],
+    ['flag.needinfo', '? (b@example.com)', ''],
+  ],
+  'needinfo cleared for two users gives one change per requestee'
+);
+
+is(
+  split_flagtypes('review?(a@example.com)', 'review+, needinfo?(b@example.com)'),
+  [
+    ['flag.needinfo', '',                 '? (b@example.com)'],
+    ['flag.review',   '? (a@example.com)', '+'],
+  ],
+  'a single flag changing state is still one change'
+);
+
+is(
+  morph_flag_updates(
+    ['x:review?(a@example.com)'],
+    ['x:review?(a@example.com)', 'y:review?(b@example.com)',
+      'y:review?(c@example.com)']
+  ),
+  {'flagtypes.name' => ['', 'review?(b@example.com), review?(c@example.com)']},
+  'attachment review requested from two more users keeps every added flag'
+);
+
+is(
+  morph_flag_updates(['x:review?(a@example.com)'], ['y:review?(a@example.com)']),
+  {}, 'a flag re-set by a different user is not a change'
+);
+
+is(
+  split_flagtypes(
+    'needinfo?(a@example.com), needinfo?(b@example.com)',
+    'needinfo+, needinfo?(d@example.com)'
+  ),
+  [
+    ['flag.needinfo', '? (a@example.com)', ''],
+    ['flag.needinfo', '? (b@example.com)', ''],
+    ['flag.needinfo', '',                  '+'],
+    ['flag.needinfo', '',                  '? (d@example.com)'],
+  ],
+  'several changes to one flag type are not paired'
+);
+
+done_testing;