From d5c1d67198505cc72845d512628852ace9ce799e Mon Sep 17 00:00:00 2001 From: Byron Jones Date: Tue, 26 Aug 2014 12:51:09 +0800 Subject: 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) --- Bugzilla/Bug.pm | 11 +- extensions/Review/Extension.pm | 324 +++++++++++++-------- .../messages-component_updated_fields.html.tmpl | 11 + post_bug.cgi | 14 +- process_bug.cgi | 23 +- 5 files changed, 244 insertions(+), 139 deletions(-) create mode 100644 extensions/Review/template/en/default/hook/global/messages-component_updated_fields.html.tmpl 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 %] +
  • Suggested Reviewers changed to '[% comp.reviewers.join(", ") FILTER html %]'
    • +[% 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) { -- cgit v1.2.3-24-g4f1b