From d6d3e015509c27c1bac0c7625805152b35aeee3a Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Sat, 10 Oct 2026 22:12:05 -0400 Subject: [PATCH 1/4] 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 | 102 +++++++++++++------------------ extensions/Push/t/flag_changes.t | 82 +++++++++++++++++++++++++ 2 files changed, 126 insertions(+), 58 deletions(-) create mode 100644 extensions/Push/t/flag_changes.t diff --git a/extensions/Push/Extension.pm b/extensions/Push/Extension.pm index 265330a971..2545c1d2e2 100644 --- a/extensions/Push/Extension.pm +++ b/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,34 @@ 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; } + + # pair removed and added values of the same flag type in order, so a single + # flag changing state (eg. review? to review+) is still one change + my @flag_changes; + foreach my $name (sort keys %values) { + my @removed = @{$values{$name}{removed} || []}; + my @added = @{$values{$name}{added} || []}; + while (@removed || @added) { + push @flag_changes, [$name, shift(@removed) // '', shift(@added) // '']; + } + } + return @flag_changes; } sub _split_flagtype { @@ -219,52 +229,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 +275,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; diff --git a/extensions/Push/t/flag_changes.t b/extensions/Push/t/flag_changes.t new file mode 100644 index 0000000000..6cfb7094cc --- /dev/null +++ b/extensions/Push/t/flag_changes.t @@ -0,0 +1,82 @@ +#!/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' +); + +done_testing; From a65ed35c69924e3334940703a1e3bcd2f9b8752a Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Mon, 12 Oct 2026 10:16:28 -0400 Subject: [PATCH 2/4] Potential fix for pull request finding 'Avoid positional pairing of multiple changed flags' Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- extensions/Push/Extension.pm | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/extensions/Push/Extension.pm b/extensions/Push/Extension.pm index 2545c1d2e2..0e9e89edca 100644 --- a/extensions/Push/Extension.pm +++ b/extensions/Push/Extension.pm @@ -205,8 +205,12 @@ sub _split_flagtypes { foreach my $name (sort keys %values) { my @removed = @{$values{$name}{removed} || []}; my @added = @{$values{$name}{added} || []}; - while (@removed || @added) { - push @flag_changes, [$name, shift(@removed) // '', shift(@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; From 447eded9de8c26b5d532358b40c84dc2bf79bf3a Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Mon, 12 Oct 2026 16:55:34 -0400 Subject: [PATCH 3/4] Update extensions/Push/Extension.pm Co-authored-by: Xavier L'Hour --- extensions/Push/Extension.pm | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/extensions/Push/Extension.pm b/extensions/Push/Extension.pm index 0e9e89edca..7c83c155f7 100644 --- a/extensions/Push/Extension.pm +++ b/extensions/Push/Extension.pm @@ -199,8 +199,9 @@ sub _split_flagtypes { push @{$values{$name}{added}}, $value; } - # pair removed and added values of the same flag type in order, so a single - # flag changing state (eg. review? to review+) is still one change + # 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} || []}; From b5bd570c141b564db9617425558120c40f9beba0 Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Mon, 12 Oct 2026 16:55:46 -0400 Subject: [PATCH 4/4] Update extensions/Push/t/flag_changes.t Co-authored-by: Xavier L'Hour --- extensions/Push/t/flag_changes.t | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/extensions/Push/t/flag_changes.t b/extensions/Push/t/flag_changes.t index 6cfb7094cc..c4e7b0d2d5 100644 --- a/extensions/Push/t/flag_changes.t +++ b/extensions/Push/t/flag_changes.t @@ -79,4 +79,18 @@ is( {}, '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;