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)

3 files changed, 183 insertions, 39 deletions

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;