Bug 1396957 - Synchronize CC list on security bugs over to revisions as subscribers (#230)

* Bug 1396957 - Synchronize CC list on security bugs over to revisions as subscribers

* - Updated to only make subscriber changes if no groups have changed.
- This cuts down on the amount of custom policy changes in the revision
activity.

* Bug 1396957 - Synchronize CC list on security bugs over to revisions as subscribers

* - Updated to only make subscriber changes if no groups have changed.
- This cuts down on the amount of custom policy changes in the revision
activity.

* Bug 1396042 - Implement logging for the BMO push connector

* Bug 1397747 - Move _get_all_group_names to Bugzilla::Config::Common::_get_all_group_names, and remove copy-pasted code

2 files changed, 40 insertions, 9 deletions

diff --git a/extensions/PhabBugz/lib/Util.pm b/extensions/PhabBugz/lib/Util.pm
index becd5bb29..9e2cac149 100644
--- a/extensions/PhabBugz/lib/Util.pm
+++ b/extensions/PhabBugz/lib/Util.pm
@@ -38,6 +38,7 @@ our @EXPORT = qw(
     make_revision_public
     request
     set_project_members
+    set_revision_subscribers
 );
 
 sub get_revisions_by_ids {
@@ -202,7 +203,7 @@ sub edit_revision_policy {
 
     if (@$subscribers) {
         push(@{ $data->{transactions} }, {
-            type  => 'subscribers.add',
+            type  => 'subscribers.set',
             value => $subscribers
         });
     }
@@ -210,6 +211,22 @@ sub edit_revision_policy {
     return request('differential.revision.edit', $data);
 }
 
+sub set_revision_subscribers {
+    my ($revision_phid, $subscribers) = @_;
+
+    my $data = {
+        transactions => [
+            {
+                type  => 'subscribers.set',
+                value => $subscribers
+            }
+        ],
+        objectIdentifier => $revision_phid
+    };
+
+    return request('differential.revision.edit', $data);
+}
+
 sub add_comment_to_revision {
     my ($revision_phid, $comment) = @_;
 
diff --git a/extensions/Push/lib/Connector/Phabricator.pm b/extensions/Push/lib/Connector/Phabricator.pm
index 8a307ea72..b9917d7c1 100644
--- a/extensions/Push/lib/Connector/Phabricator.pm
+++ b/extensions/Push/lib/Connector/Phabricator.pm
@@ -23,7 +23,7 @@ use Bugzilla::Extension::PhabBugz::Util qw(
   add_comment_to_revision create_private_revision_policy
   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);
+  make_revision_public make_revision_private set_revision_subscribers);
 use Bugzilla::Extension::Push::Constants;
 use Bugzilla::Extension::Push::Util qw(is_public);
 
@@ -44,9 +44,10 @@ sub should_send {
 
     return 0 unless Bugzilla->params->{phabricator_enabled};
 
+    # We are only interested currently in bug group, assignee, qa-contact, or cc changes.
     return 0
       unless $message->routing_key =~
-      /^(?:attachment|bug)\.modify:.*\bbug_group\b/;
+      /^(?:attachment|bug)\.modify:.*\b(bug_group|assigned_to|qa_contact|cc)\b/;
 
     my $bug = $self->_get_bug_by_data( $message->payload_decoded ) || return 0;
 
@@ -59,6 +60,7 @@ sub send {
     my $logger = Bugzilla->push_ext->logger;
 
     my $data = $message->payload_decoded;
+
     my $bug = $self->_get_bug_by_data($data) || return PUSH_RESULT_OK;
 
     my $is_public = is_public($bug);
@@ -73,11 +75,12 @@ sub send {
 
     my @set_groups = intersect( $bug_group_names, $sync_group_names );
 
+    my @revisions = get_attachment_revisions($bug);
+
     if ( !$is_public && !@set_groups ) {
         my $phab_error_message =
           'Revision is being made private due to unknown Bugzilla groups.';
 
-        my @revisions = get_attachment_revisions($bug);
         foreach my $revision (@revisions) {
             Bugzilla->audit(sprintf(
               'Making revision %s for bug %s private due to unkown Bugzilla groups: %s',
@@ -109,18 +112,20 @@ sub send {
         return PUSH_RESULT_OK;
     }
 
-    my $policy_phid;
+    my $group_change =
+      ($message->routing_key =~ /^(?:attachment|bug)\.modify:.*\bbug_group\b/)
+      ? 1
+      : 0;
+
     my $subscribers;
     if ( !$is_public ) {
-        $policy_phid = create_private_revision_policy( $bug, \@set_groups );
         $subscribers = get_bug_role_phids($bug);
     }
 
-    my @revisions = get_attachment_revisions($bug);
     foreach my $revision (@revisions) {
         my $revision_phid = $revision->{phid};
 
-        if ($is_public) {
+        if ( $is_public && $group_change ) {
             Bugzilla->audit(sprintf(
               'Making revision %s public for bug %s',
               $revision->{id},
@@ -128,14 +133,23 @@ sub send {
             ));
             make_revision_public($revision_phid);
         }
-        else {
+        elsif ( !$is_public && $group_change ) {
             Bugzilla->audit(sprintf(
               'Giving revision %s a custom policy for bug %s',
               $revision->{id},
               $bug->id
             ));
+            my $policy_phid = create_private_revision_policy( $bug, \@set_groups );
             edit_revision_policy( $revision_phid, $policy_phid, $subscribers );
         }
+        elsif ( !$is_public && !$group_change ) {
+            Bugzilla->audit(sprintf(
+              'Updating subscribers for %s for bug %s',
+              $revision->{id},
+              $bug->id
+            ));
+            set_revision_subscribers( $revision_phid, $subscribers );
+        }
     }
 
     return PUSH_RESULT_OK;