Bug 1051655: mentor field updated/reset when a bug is updated as a result of a change on a different bug (eg. see also, duplicate)

5 files changed, 244 insertions, 139 deletions

diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm
index e8f0fd44a..3310afe3d 100644
--- a/Bugzilla/Bug.pm
+++ b/Bugzilla/Bug.pm
@@ -684,8 +684,13 @@ sub create {
     my ($class, $params) = @_;
     my $dbh = Bugzilla->dbh;
 
-    # BMO - allow parameter alteration before creation
-    Bugzilla::Hook::process('bug_before_create', { params => $params });
+    # BMO - allow parameter alteration before creation.  also add support for
+    # fields which are not bug columns (eg bug_mentors). extensions should move
+    # fields from $params to $stash, then use the bug_end_of_create hook to
+    # update the database
+    my $stash = {};
+    Bugzilla::Hook::process('bug_before_create', { params => $params,
+                                                   stash  => $stash });
 
     $dbh->bz_start_transaction();
 
@@ -803,8 +808,10 @@ sub create {
     # but sometimes it's blank.
     Bugzilla::Comment->insert_create_data($creation_comment);
 
+    # BMO - add the stash param from bug_start_of_create
     Bugzilla::Hook::process('bug_end_of_create', { bug => $bug,
                                                    timestamp => $timestamp,
+                                                   stash => $stash,
                                                  });
 
     $dbh->bz_commit_transaction();
diff --git a/extensions/Review/Extension.pm b/extensions/Review/Extension.pm
index 61822d076..cf2bc532e 100644
--- a/extensions/Review/Extension.pm
+++ b/extensions/Review/Extension.pm
@@ -35,7 +35,9 @@ BEGIN {
     *Bugzilla::Component::reviewers       = \&_component_reviewers;
     *Bugzilla::Component::reviewers_objs  = \&_component_reviewers_objs;
     *Bugzilla::Bug::mentors               = \&_bug_mentors;
+    *Bugzilla::Bug::bug_mentors           = \&_bug_mentors;
     *Bugzilla::Bug::is_mentor             = \&_bug_is_mentor;
+    *Bugzilla::Bug::set_bug_mentors       = \&_bug_set_bug_mentors;
     *Bugzilla::User::review_count         = \&_user_review_count;
 }
 
@@ -76,6 +78,27 @@ sub _reviewers_objs {
     return $object->{reviewers};
 }
 
+sub _user_review_count {
+    my ($self) = @_;
+    if (!exists $self->{review_count}) {
+        my $dbh = Bugzilla->dbh;
+        ($self->{review_count}) = $dbh->selectrow_array(
+            "SELECT COUNT(*)
+               FROM flags
+                    INNER JOIN flagtypes ON flagtypes.id = flags.type_id
+              WHERE flags.requestee_id = ?
+                    AND " . $dbh->sql_in('flagtypes.name', [ "'review'", "'feedback'" ]),
+            undef,
+            $self->id,
+        );
+    }
+    return $self->{review_count};
+}
+
+#
+# mentor
+#
+
 sub _bug_mentors {
     my ($self) = @_;
     my $dbh = Bugzilla->dbh;
@@ -102,25 +125,114 @@ sub _bug_is_mentor {
     return (grep { $_->id == $user_id} @{ $self->mentors }) ? 1 : 0;
 }
 
-sub _user_review_count {
-    my ($self) = @_;
-    if (!exists $self->{review_count}) {
-        my $dbh = Bugzilla->dbh;
-        ($self->{review_count}) = $dbh->selectrow_array(
-            "SELECT COUNT(*)
-               FROM flags
-                    INNER JOIN flagtypes ON flagtypes.id = flags.type_id
-              WHERE flags.requestee_id = ?
-                    AND " . $dbh->sql_in('flagtypes.name', [ "'review'", "'feedback'" ]),
-            undef,
-            $self->id,
-        );
+sub _bug_set_bug_mentors {
+    my ($self, $value) = @_;
+    $self->set('bug_mentors', $value);
+}
+
+sub object_validators {
+    my ($self, $args) = @_;
+    return unless $args->{class} eq 'Bugzilla::Bug';
+    $args->{validators}->{bug_mentors} = \&_bug_check_bug_mentors;
+}
+
+sub _bug_check_bug_mentors {
+    my ($self, $value) = @_;
+    return [
+        map { Bugzilla::User->check({ name => $_, cache => 1 }) }
+        ref($value) ? @$value : ($value)
+    ];
+}
+
+sub bug_user_match_fields {
+    my ($self, $args) = @_;
+    $args->{fields}->{bug_mentors} = { type => 'multi' };
+}
+
+sub bug_before_create {
+    my ($self, $args) = @_;
+    my $params = $args->{params};
+    my $stash = $args->{stash};
+    $stash->{bug_mentors} = delete $params->{bug_mentors};
+}
+
+sub bug_end_of_create {
+    my ($self, $args) = @_;
+    my $bug = $args->{bug};
+    my $stash = $args->{stash};
+    if (my $mentors = $stash->{bug_mentors}) {
+        $self->_update_user_table({
+            object      => $bug,
+            old_users   => [],
+            new_users   => $self->_bug_check_bug_mentors($mentors),
+            table       => 'bug_mentors',
+            id_field    => 'bug_id',
+        });
+    }
+}
+
+sub _update_user_table {
+    my ($self, $args) = @_;
+    my ($object, $old_users, $new_users, $table, $id_field, $has_sortkey, $return) =
+        @$args{qw(object old_users new_users table id_field has_sortkey return)};
+    my $dbh = Bugzilla->dbh;
+    my (@removed, @added);
+
+    # remove deleted users
+    foreach my $old_user (@$old_users) {
+        if (!grep { $_->id == $old_user->id } @$new_users) {
+            $dbh->do(
+                "DELETE FROM $table WHERE $id_field = ? AND user_id = ?",
+                undef,
+                $object->id, $old_user->id,
+            );
+            push @removed, $old_user;
+        }
+    }
+    # add new users
+    foreach my $new_user (@$new_users) {
+        if (!grep { $_->id == $new_user->id } @$old_users) {
+            $dbh->do(
+                "INSERT INTO $table ($id_field, user_id) VALUES (?, ?)",
+                undef,
+                $object->id, $new_user->id,
+            );
+            push @added, $new_user;
+        }
+    }
+
+    return unless @removed || @added;
+
+    if ($has_sortkey) {
+        # update the sortkey for all users
+        for (my $i = 0; $i < scalar(@$new_users); $i++) {
+            $dbh->do(
+                "UPDATE $table SET sortkey=? WHERE $id_field = ? AND user_id = ?",
+                undef,
+                ($i + 1) * 10, $object->id, $new_users->[$i]->id,
+            );
+        }
+    }
+
+    if (!$return) {
+        return undef;
+    }
+    elsif ($return eq 'diff') {
+        return [
+            @removed ? join(', ', map { $_->login } @removed) : undef,
+            @added   ? join(', ', map { $_->login } @added)   : undef,
+        ];
+    }
+    elsif ($return eq 'old-new') {
+        return [
+            @$old_users ? join(', ', map { $_->login } @$old_users) : '',
+            @$new_users ? join(', ', map { $_->login } @$new_users) : '',
+        ];
     }
-    return $self->{review_count};
 }
 
 #
-# reviewer-required
+# reviewer-required, review counters, etc
 #
 
 sub _product_reviewer_required { $_[0]->{reviewer_required} }
@@ -147,6 +259,19 @@ sub object_update_columns {
     }
 }
 
+sub _new_users_from_input {
+    my ($field) = @_;
+    my $input_params = Bugzilla->input_params;
+    return unless exists $input_params->{$field};
+    return [] unless $input_params->{$field};
+    Bugzilla::User::match_field({ $field => {'type' => 'multi'} });;
+    my $value = $input_params->{$field};
+    return [
+        map { Bugzilla::User->check({ name => $_, cache => 1 }) }
+        ref($value) ? @$value : ($value)
+    ];
+}
+
 #
 # create/update
 #
@@ -171,11 +296,25 @@ sub object_end_of_create {
     my ($self, $args) = @_;
     my ($object, $params) = @$args{qw(object params)};
 
-    if ($object->isa('Bugzilla::Product') || $object->isa('Bugzilla::Component')) {
-        my ($new, $new_users) = _new_users_from_input('reviewers');
-        if (defined $new) {
-            _update_users($object, [], $new_users);
-        }
+    if ($object->isa('Bugzilla::Product')) {
+        $self->_update_user_table({
+            object      => $object,
+            old_users   => [],
+            new_users   => _new_users_from_input('reviewers'),
+            table       => 'product_reviewers',
+            id_field    => 'product_id',
+            has_sortkey => 1,
+        });
+    }
+    elsif ($object->isa('Bugzilla::Component')) {
+        $self->_update_user_table({
+            object      => $object,
+            old_users   => [],
+            new_users   => _new_users_from_input('reviewers'),
+            table       => 'component_reviewers',
+            id_field    => 'component_id',
+            has_sortkey => 1,
+        });
     }
     elsif (_is_countable_flag($object) && $object->requestee_id && $object->status eq '?') {
         _adjust_request_count($object, +1);
@@ -183,27 +322,46 @@ sub object_end_of_create {
     if (_is_countable_flag($object)) {
         $self->_log_flag_state_activity($object, $object->status);
     }
-    elsif ($object->isa('Bugzilla::Bug')) {
-        my ($new, $new_users) = _new_users_from_input('bug_mentors');
-        if (defined $new) {
-            _update_users($object, [], $new_users);
-        }
-    }
 }
 
 sub object_end_of_update {
     my ($self, $args) = @_;
     my ($object, $old_object, $changes) = @$args{qw(object old_object changes)};
 
-    if ($object->isa('Bugzilla::Product') || $object->isa('Bugzilla::Component')) {
-        my ($new, $new_users) = _new_users_from_input('reviewers');
-        if (defined $new) {
-            my $old = $old_object->reviewers(1);
-            if ($old ne $new) {
-                _update_users($object, $old_object->reviewers_objs(1), $new_users);
-                $changes->{reviewers} = [ $old ? $old : undef, $new ? $new : undef ];
-            }
-        }
+    if ($object->isa('Bugzilla::Product')) {
+        my $diff = $self->_update_user_table({
+            object      => $object,
+            old_users   => $old_object->reviewers_objs(1),
+            new_users   => _new_users_from_input('reviewers'),
+            table       => 'product_reviewers',
+            id_field    => 'product_id',
+            has_sortkey => 1,
+            return      => 'old-new',
+        });
+        $changes->{reviewers} = $diff if $diff;
+    }
+    elsif ($object->isa('Bugzilla::Component')) {
+        my $diff = $self->_update_user_table({
+            object      => $object,
+            old_users   => $old_object->reviewers_objs(1),
+            new_users   => _new_users_from_input('reviewers'),
+            table       => 'component_reviewers',
+            id_field    => 'component_id',
+            has_sortkey => 1,
+            return      => 'old-new',
+        });
+        $changes->{reviewers} = $diff if $diff;
+    }
+    elsif ($object->isa('Bugzilla::Bug')) {
+        my $diff = $self->_update_user_table({
+            object      => $object,
+            old_users   => $old_object->mentors,
+            new_users   => $object->mentors,
+            table       => 'bug_mentors',
+            id_field    => 'bug_id',
+            return      => 'diff',
+        });
+        $changes->{bug_mentor} = $diff if $diff;
     }
     elsif (_is_countable_flag($object)) {
         my ($old_status, $new_status) = ($old_object->status, $object->status);
@@ -231,32 +389,15 @@ sub object_end_of_update {
             _adjust_request_count($object, +1);
         }
     }
-    elsif ($object->isa('Bugzilla::Bug')) {
-        my ($new, $new_mentors) = _new_users_from_input('bug_mentors');
-        if (defined $new) {
-            my $old = join(", ", map { $_->login } @{ $old_object->mentors });
-            if ($old ne $new) {
-                _update_users($object, $old_object->mentors, $new_mentors);
-
-                my @old_names = map { $_->login } @{ $old_object->mentors };
-                my @new_names = map { $_->login } @{ $new_mentors };
-                my ($removed, $added) = diff_arrays(\@old_names, \@new_names);
-                $changes->{bug_mentor} = [
-                    @$removed ? join(", ", @$removed) : undef,
-                    @$added   ? join(", ", @$added)   : undef,
-                ];
-            }
-        }
-    }
 }
 
 sub flag_updated {
-    my ( $self, $args ) = @_;
+    my ($self, $args) = @_;
     my $flag = $args->{flag};
     my $changes = $args->{changes};
 
     return unless scalar(keys %$changes);
-    if ( _is_countable_flag($flag) ) {
+    if (_is_countable_flag($flag)) {
         $self->_log_flag_state_activity( $flag, $flag->status );
     }
 }
@@ -318,81 +459,6 @@ sub _adjust_request_count {
     Bugzilla->memcached->clear({ table => 'profiles', id => $requestee_id });
 }
 
-sub _new_users_from_input {
-    my ($field) = @_;
-    return unless exists Bugzilla->input_params->{$field};
-    return ('', []) unless Bugzilla->input_params->{$field};
-    Bugzilla::User::match_field({ $field => {'type' => 'multi'} });
-    my $new = Bugzilla->input_params->{$field};
-    $new = [ $new ] unless ref($new);
-    my $new_users = [];
-    foreach my $login (@$new) {
-        push @$new_users, Bugzilla::User->check($login);
-    }
-    $new = join(', ', @$new);
-    return ($new, $new_users);
-}
-
-sub _update_users {
-    my ($object, $old_users, $new_users) = @_;
-    my $dbh = Bugzilla->dbh;
-
-    if ($object->isa('Bugzilla::Bug')) {
-        # remove deleted users
-        foreach my $old_user (@$old_users) {
-            if (!grep { $_->id == $old_user->id } @$new_users) {
-                $dbh->do(
-                    "DELETE FROM bug_mentors WHERE bug_id = ? AND user_id = ?",
-                    undef,
-                    $object->id, $old_user->id,
-                );
-            }
-        }
-        # add new users
-        foreach my $new_user (@$new_users) {
-            if (!grep { $_->id == $new_user->id } @$old_users) {
-                $dbh->do(
-                    "INSERT INTO bug_mentors (bug_id,user_id) VALUES (?, ?)",
-                    undef,
-                    $object->id, $new_user->id,
-                );
-            }
-        }
-    }
-    else {
-        my $type = $object->isa('Bugzilla::Product') ? 'product' : 'component';
-
-        # remove deleted users
-        foreach my $old_user (@$old_users) {
-            if (!grep { $_->id == $old_user->id } @$new_users) {
-                $dbh->do(
-                    "DELETE FROM ${type}_reviewers WHERE ${type}_id=? AND user_id=?",
-                    undef,
-                    $object->id, $old_user->id,
-                );
-            }
-        }
-        # add new users
-        foreach my $new_user (@$new_users) {
-            if (!grep { $_->id == $new_user->id } @$old_users) {
-                $dbh->do(
-                    "INSERT INTO ${type}_reviewers(${type}_id,user_id) VALUES (?,?)",
-                    undef,
-                    $object->id, $new_user->id,
-                );
-            }
-        }
-        # and update the sortkey for all users
-        for (my $i = 0; $i < scalar(@$new_users); $i++) {
-            $dbh->do(
-                "UPDATE ${type}_reviewers SET sortkey=? WHERE ${type}_id=? AND user_id=?",
-                undef,
-                ($i + 1) * 10, $object->id, $new_users->[$i]->id,
-            );
-        }
-    }
-}
-
 # bugzilla's handling of requestee matching when creating bugs is "if it's
 # wrong, or matches too many, default to empty", which breaks mandatory
 # reviewer requirements.  instead we just throw an error.
diff --git a/extensions/Review/template/en/default/hook/global/messages-component_updated_fields.html.tmpl b/extensions/Review/template/en/default/hook/global/messages-component_updated_fields.html.tmpl
new file mode 100644
index 000000000..05b7bde82
--- /dev/null
+++ b/extensions/Review/template/en/default/hook/global/messages-component_updated_fields.html.tmpl
@@ -0,0 +1,11 @@
+[%# 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.
+  #%]
+
+[% IF changes.reviewers.defined %]
+  <li>Suggested Reviewers changed to '[% comp.reviewers.join(", ") FILTER html %]'</li>
+[% END %]
diff --git a/post_bug.cgi b/post_bug.cgi
index a1ed77bac..ccdda3e2d 100755
--- a/post_bug.cgi
+++ b/post_bug.cgi
@@ -73,11 +73,14 @@ my $token = trim($cgi->param('token'));
 check_token_data($token, 'create_bug', 'index.cgi');
 
 # do a match on the fields if applicable
-Bugzilla::User::match_field ({
+# BMO: allow extensions to define custom user fields
+my $user_match_fields = {
     'cc'            => { 'type' => 'multi'  },
     'assigned_to'   => { 'type' => 'single' },
     'qa_contact'    => { 'type' => 'single' },
-});
+};
+Bugzilla::Hook::process('bug_user_match_fields', { fields => $user_match_fields });
+Bugzilla::User::match_field($user_match_fields);
 
 if (defined $cgi->param('maketemplate')) {
     $vars->{'url'} = $cgi->canonicalise_query('token');
@@ -152,6 +155,13 @@ foreach my $field (@multi_selects) {
     $bug_params{$field->name} = [$cgi->param($field->name)];
 }
 
+# BMO - add user_match_fields.  it's important to source from input_params
+# instead of $cgi->param to ensure we get the correct value.
+foreach my $field (keys %$user_match_fields) {
+    next if exists $bug_params{$field};
+    $bug_params{$field} = Bugzilla->input_params->{$field} // [];
+}
+
 my $bug = Bugzilla::Bug->create(\%bug_params);
 
 # Get the bug ID back and delete the token used to create this bug.
diff --git a/process_bug.cgi b/process_bug.cgi
index 20875fb29..39823f88e 100755
--- a/process_bug.cgi
+++ b/process_bug.cgi
@@ -58,6 +58,7 @@ use Bugzilla::Keyword;
 use Bugzilla::Flag;
 use Bugzilla::Status;
 use Bugzilla::Token;
+use Bugzilla::Hook;
 
 use List::MoreUtils qw(firstidx);
 use Storable qw(dclone);
@@ -133,12 +134,15 @@ if (defined $cgi->param('dontchange')) {
 }
 
 # do a match on the fields if applicable
-Bugzilla::User::match_field({
-    'qa_contact'                => { 'type' => 'single' },
-    'newcc'                     => { 'type' => 'multi'  },
-    'masscc'                    => { 'type' => 'multi'  },
-    'assigned_to'               => { 'type' => 'single' },
-});
+# BMO: allow extensions to define custom user fields
+my $user_match_fields = {
+    'qa_contact'    => { 'type' => 'single' },
+    'newcc'         => { 'type' => 'multi'  },
+    'masscc'        => { 'type' => 'multi'  },
+    'assigned_to'   => { 'type' => 'single' },
+};
+Bugzilla::Hook::process('bug_user_match_fields', { fields => $user_match_fields });
+Bugzilla::User::match_field($user_match_fields);
 
 print $cgi->header() unless Bugzilla->usage_mode == USAGE_MODE_EMAIL;
 
@@ -371,6 +375,13 @@ foreach my $field (@custom_fields) {
     }
 }
 
+# BMO - add user_match_fields.  it's important to source from input_params
+# instead of $cgi->param to ensure we get the correct value.
+foreach my $field (keys %$user_match_fields) {
+    next if exists $set_all_fields{$field};
+    $set_all_fields{$field} = Bugzilla->input_params->{$field} // [];
+}
+
 # We are going to alter the list of removed groups, so we keep a copy here.
 my @unchecked_groups = @$removed_groups;
 foreach my $b (@bug_objects) {