From 6e06bb255acdf53d2bbed3911378bd6859a0e3f0 Mon Sep 17 00:00:00 2001 From: dklawren Date: Wed, 30 Aug 2017 11:15:14 -0400 Subject: Bug 1393023: Approving a revision creates an r+ flag on the revision attachment in the associated bug (#219) Approving a revision creates an r+ flag on the revision attachment in the associated bug * - Working version - Splits accepted_users on ':' instead of accepting a list (phab issue) --- extensions/PhabBugz/lib/Util.pm | 28 ++++- extensions/PhabBugz/lib/WebService.pm | 159 +++++++++++++++++++++++++-- extensions/Push/lib/Connector/Phabricator.pm | 35 +----- 3 files changed, 183 insertions(+), 39 deletions(-) (limited to 'extensions') diff --git a/extensions/PhabBugz/lib/Util.pm b/extensions/PhabBugz/lib/Util.pm index 57dc1d7d0..5a8385d0c 100644 --- a/extensions/PhabBugz/lib/Util.pm +++ b/extensions/PhabBugz/lib/Util.pm @@ -27,6 +27,7 @@ our @EXPORT = qw( create_private_revision_policy create_project edit_revision_policy + get_attachment_revisions get_bug_role_phids get_members_by_bmo_id get_project_phid @@ -297,7 +298,32 @@ sub is_attachment_phab_revision { my ($attachment, $include_obsolete) = @_; return ($attachment->contenttype eq PHAB_CONTENT_TYPE && ($include_obsolete || !$attachment->isobsolete) - && $attachment->attacher->login eq 'phab-bot@bmo.tld') ? 1 : 0; + && $attachment->attacher->login eq PHAB_AUTOMATION_USER) ? 1 : 0; +} + +sub get_attachment_revisions { + my ($self, $bug) = @_; + + my @revisions; + + my @attachments = + grep { is_attachment_phab_revision($_) } @{ $bug->attachments() }; + + if (@attachments) { + my @revision_ids; + foreach my $attachment (@attachments) { + my ($revision_id) = + ( $attachment->filename =~ PHAB_ATTACHMENT_PATTERN ); + next if !$revision_id; + push( @revision_ids, int($revision_id) ); + } + + if (@revision_ids) { + @revisions = get_revisions_by_ids( \@revision_ids ); + } + } + + return @revisions; } sub request { diff --git a/extensions/PhabBugz/lib/WebService.pm b/extensions/PhabBugz/lib/WebService.pm index 84561c3db..5668eac81 100644 --- a/extensions/PhabBugz/lib/WebService.pm +++ b/extensions/PhabBugz/lib/WebService.pm @@ -32,17 +32,22 @@ use Bugzilla::Extension::PhabBugz::Util qw( get_project_phid get_revisions_by_ids intersect + is_attachment_phab_revision make_revision_public request ); +use List::Util qw(first); +use List::MoreUtils qw(any); use MIME::Base64 qw(decode_base64); use constant PUBLIC_METHODS => qw( + check_user_permission_for_bug + obsolete_attachments revision + update_reviewer_statuses ); - sub revision { my ($self, $params) = @_; @@ -59,6 +64,9 @@ sub revision { my $user = Bugzilla->login(LOGIN_REQUIRED); + # Prechecks + _phabricator_precheck($user); + unless (defined $params->{revision} && detaint_natural($params->{revision})) { ThrowCodeError('param_required', { param => 'revision' }) } @@ -117,13 +125,8 @@ sub check_user_permission_for_bug { my $user = Bugzilla->login(LOGIN_REQUIRED); - # Ensure PhabBugz is on - ThrowUserError('phabricator_not_enabled') - unless Bugzilla->params->{phabricator_enabled}; - - # Validate that the requesting user's email matches phab-bot - ThrowUserError('phabricator_unauthorized_user') - unless $user->login eq PHAB_AUTOMATION_USER; + # Prechecks + _phabricator_precheck($user); # Validate that a bug id and user id are provided ThrowUserError('phabricator_invalid_request_params') @@ -138,6 +141,134 @@ sub check_user_permission_for_bug { }; } +sub update_reviewer_statuses { + my ($self, $params) = @_; + + my $user = Bugzilla->login(LOGIN_REQUIRED); + + # Prechecks + _phabricator_precheck($user); + + my $revision_id = $params->{revision_id}; + unless (defined $revision_id && detaint_natural($revision_id)) { + ThrowCodeError('param_required', { param => 'revision_id' }) + } + + my $bug_id = $params->{bug_id}; + unless (defined $bug_id && detaint_natural($bug_id)) { + ThrowCodeError('param_required', { param => 'bug_id' }) + } + + my $accepted_user_ids = $params->{accepted_users}; + defined $accepted_user_ids + || ThrowCodeError('param_required', { param => 'accepted_users' }); + $accepted_user_ids = [ split(':', $accepted_user_ids) ]; + + my $bug = Bugzilla::Bug->check($bug_id); + + my @attachments = + grep { is_attachment_phab_revision($_) } @{ $bug->attachments() }; + + return { result => [] } if !@attachments; + + my $dbh = Bugzilla->dbh; + my ($timestamp) = $dbh->selectrow_array("SELECT NOW()"); + + my @updated_attach_ids; + foreach my $attachment (@attachments) { + my ($curr_revision_id) = ($attachment->filename =~ PHAB_ATTACHMENT_PATTERN); + next if $revision_id != $curr_revision_id; + + # Clear old flags if no longer accepted or a previous + # acceptor is not in the new list. + my (@old_flags, @new_flags, %accepted_done, $flag_type); + foreach my $flag (@{ $attachment->flags }) { + next if $flag->type->name ne 'review'; + $flag_type = $flag->type; + unless (any { $flag->setter->id == $_ } @$accepted_user_ids) { + push(@old_flags, { id => $flag->id, status => 'X' }); + } + else { + $accepted_done{$flag->setter->id}++; # so we do not set it again as new + } + } + + $flag_type ||= first { $_->name eq 'review' } @{ $attachment->flag_types }; + + # Create new flags + foreach my $user_id (@$accepted_user_ids) { + next if $accepted_done{$user_id}; + my $user = Bugzilla::User->check({ id => $user_id, cache => 1 }); + push(@new_flags, { type_id => $flag_type->id, setter => $user, status => '+' }); + } + + $attachment->set_flags(\@old_flags, \@new_flags); + $attachment->update($timestamp); + + push(@updated_attach_ids, $attachment->id); + } + + Bugzilla::BugMail::Send($bug_id, { changer => $user }) if @updated_attach_ids; + + return { result => \@updated_attach_ids }; +} + +sub obsolete_attachments { + my ($self, $params) = @_; + + my $user = Bugzilla->login(LOGIN_REQUIRED); + + # Prechecks + _phabricator_precheck($user); + + my $revision_id = $params->{revision_id}; + unless (defined $revision_id && detaint_natural($revision_id)) { + ThrowCodeError('param_required', { param => 'revision' }) + } + + my $bug_id= $params->{bug_id}; + unless (defined $bug_id && detaint_natural($bug_id)) { + ThrowCodeError('param_required', { param => 'bug_id' }) + } + + my $bug = Bugzilla::Bug->check($bug_id); + + my @attachments = + grep { is_attachment_phab_revision($_) } @{ $bug->attachments() }; + + return { result => [] } if !@attachments; + + my $dbh = Bugzilla->dbh; + my ($timestamp) = $dbh->selectrow_array("SELECT NOW()"); + + my @updated_attach_ids; + foreach my $attachment (@attachments) { + my ($curr_revision_id) = ($attachment->filename =~ PHAB_ATTACHMENT_PATTERN); + next if $revision_id != $curr_revision_id; + + $attachment->set_is_obsolete(1); + $attachment->update($timestamp); + + push(@updated_attach_ids, $attachment->id); + } + + Bugzilla::BugMail::Send($bug_id, { changer => $user }) if @updated_attach_ids; + + return { result => \@updated_attach_ids }; +} + +sub _phabricator_precheck { + my ($user) = @_; + + # Ensure PhabBugz is on + ThrowUserError('phabricator_not_enabled') + unless Bugzilla->params->{phabricator_enabled}; + + # Validate that the requesting user's email matches phab-bot + ThrowUserError('phabricator_unauthorized_user') + unless $user->login eq PHAB_AUTOMATION_USER; +} + sub rest_resources { return [ # Revision creation @@ -157,6 +288,18 @@ sub rest_resources { return { bug_id => $_[0], user_id => $_[1] }; } } + }, + # Update reviewer statuses + qr{^/phabbugz/update_reviewer_statuses$}, { + PUT => { + method => 'update_reviewer_statuses', + } + }, + # Obsolete attachments + qr{^/phabbugz/obsolete$}, { + PUT => { + method => 'obsolete_attachments', + } } ]; } diff --git a/extensions/Push/lib/Connector/Phabricator.pm b/extensions/Push/lib/Connector/Phabricator.pm index 2571c6d37..c92df3173 100644 --- a/extensions/Push/lib/Connector/Phabricator.pm +++ b/extensions/Push/lib/Connector/Phabricator.pm @@ -21,9 +21,9 @@ use Bugzilla::User; use Bugzilla::Extension::PhabBugz::Constants; use Bugzilla::Extension::PhabBugz::Util qw( add_comment_to_revision create_private_revision_policy - edit_revision_policy get_bug_role_phids get_revisions_by_ids - intersect is_attachment_phab_revision make_revision_public - make_revision_private); + edit_revision_policy get_attachment_revisions get_bug_role_phids + get_revisions_by_ids intersect is_attachment_phab_revision + make_revision_public make_revision_private); use Bugzilla::Extension::Push::Constants; use Bugzilla::Extension::Push::Util qw(is_public); @@ -77,7 +77,7 @@ sub send { my $phab_error_message = 'Revision is being made private due to unknown Bugzilla groups.'; - my @revisions = $self->_get_attachment_revisions($bug); + my @revisions = get_attachment_revisions($bug); foreach my $revision (@revisions) { add_comment_to_revision( $revision->{phid}, $phab_error_message ); make_revision_private( $revision->{phid} ); @@ -110,7 +110,7 @@ sub send { $subscribers = get_bug_role_phids($bug); } - my @revisions = $self->_get_attachment_revisions($bug); + my @revisions = get_attachment_revisions($bug); foreach my $revision (@revisions) { my $revision_phid = $revision->{phid}; @@ -125,31 +125,6 @@ sub send { return PUSH_RESULT_OK; } -sub _get_attachment_revisions() { - my ( $self, $bug ) = @_; - - my @revisions; - - my @attachments = - grep { is_attachment_phab_revision($_) } @{ $bug->attachments() }; - - if (@attachments) { - my @revision_ids; - foreach my $attachment (@attachments) { - my ($revision_id) = - ( $attachment->filename =~ PHAB_ATTACHMENT_PATTERN ); - next if !$revision_id; - push( @revision_ids, int($revision_id) ); - } - - if (@revision_ids) { - @revisions = get_revisions_by_ids( \@revision_ids ); - } - } - - return @revisions; -} - sub _get_bug_by_data { my ( $self, $data ) = @_; my $bug_data = $self->_get_bug_data($data) || return 0; -- cgit v1.2.3-24-g4f1b