Bug 1457900 - When restricting a revision to a bugzilla group we should tag the revision with the project

4 files changed, 107 insertions, 73 deletions

diff --git a/extensions/PhabBugz/lib/Feed.pm b/extensions/PhabBugz/lib/Feed.pm
index 31dd8bca0..9fb1dac11 100644
--- a/extensions/PhabBugz/lib/Feed.pm
+++ b/extensions/PhabBugz/lib/Feed.pm
@@ -337,20 +337,6 @@ sub process_revision_change {
         blessed $revision_phid
         ? $revision_phid
         : Bugzilla::Extension::PhabBugz::Revision->new_from_query({ phids => [ $revision_phid ] });
-  
-    # Project tags/groups that will be used later for policies, etc.
-    my $secure_revision =
-      Bugzilla::Extension::PhabBugz::Project->new_from_query(
-        {
-          name => 'secure-revision'
-        }
-      );
-    my $edit_bugs = 
-        Bugzilla::Extension::PhabBugz::Project->new_from_query(
-        {
-          name => 'bmo-editbugs-team'
-        }
-      );
 
     # NO BUG ID
 
@@ -358,9 +344,7 @@ sub process_revision_change {
         if ($story_text =~ /\s+created\s+D\d+/) {
             # If new revision and bug id was omitted, make revision public
             INFO("No bug associated with new revision. Marking public.");
-            $revision->set_policy('view', 'public');
-            $revision->set_policy('edit', ($edit_bugs ? $edit_bugs->phid : 'users'));
-            $revision->remove_project($secure_revision->phid);
+            $revision->make_public();
             $revision->update();
             INFO("SUCCESS");
             return;
@@ -388,9 +372,7 @@ sub process_revision_change {
     # If bug is public then remove privacy policy
     if (!@{ $bug->groups_in }) {
         INFO('Bug is public so setting view/edit public');
-        $revision->set_policy('view', 'public');
-        $revision->set_policy('edit', ($edit_bugs ? $edit_bugs->phid : 'users'));
-        $revision->remove_project($secure_revision->phid);
+        $revision->make_public();
     }
     # else bug is private.
     else {
@@ -405,7 +387,7 @@ sub process_revision_change {
         # Otherwise, we create a new custom policy containing the project
         # groups that are mapped to bugzilla groups.
         else {
-            my @set_projects = map { "bmo-" . $_ } @set_groups;
+            my $set_project_names = [ map { "bmo-" . $_ } @set_groups ];
 
             # If current policy projects matches what we want to set, then
             # we leave the current policy alone.
@@ -414,13 +396,12 @@ sub process_revision_change {
                 INFO("Loading current policy: " . $revision->view_policy);
                 $current_policy
                     = Bugzilla::Extension::PhabBugz::Policy->new_from_query({ phids => [ $revision->view_policy ]});
-                my $current_projects = $current_policy->rule_projects;
-                INFO("Current policy projects: " . join(", ", @$current_projects));
-                my ($added, $removed) = diff_arrays($current_projects, \@set_projects);
+                my $current_project_names = [ map { $_->name } @{ $current_policy->rule_projects } ];
+                INFO("Current policy projects: " . join(", ", @$current_project_names));
+                my ($added, $removed) = diff_arrays($current_project_names, $set_project_names);
                 if (@$added || @$removed) {
                     INFO('Project groups do not match. Need new custom policy');
                     $current_policy = undef;
-
                 }
                 else {
                     INFO('Project groups match. Leaving current policy as-is');
@@ -428,13 +409,9 @@ sub process_revision_change {
             }
 
             if (!$current_policy) {
-                INFO("Creating new custom policy: " . join(", ", @set_projects));
-                my $new_policy = Bugzilla::Extension::PhabBugz::Policy->create(\@set_projects);
-                $revision->set_policy('view', $new_policy->phid);
-                $revision->set_policy('edit', $new_policy->phid);
+                INFO("Creating new custom policy: " . join(", ", @$set_project_names));
+                $revision->make_private($set_project_names);
             }
-
-            $revision->add_project($secure_revision->phid);
         }
 
         # Subscriber list of the private revision should always match
diff --git a/extensions/PhabBugz/lib/Policy.pm b/extensions/PhabBugz/lib/Policy.pm
index 1e925f55a..a86c83036 100644
--- a/extensions/PhabBugz/lib/Policy.pm
+++ b/extensions/PhabBugz/lib/Policy.pm
@@ -41,7 +41,7 @@ has 'rules' => (
 
 has 'rule_projects' => (
     is => 'lazy',
-    isa => ArrayRef[Str],
+    isa => ArrayRef[Object],
 );
 
 # {
@@ -88,7 +88,7 @@ sub new_from_query {
 }
 
 sub create {
-    my ($class, $project_names) = @_;
+    my ($class, $projects) = @_;
 
     my $data = {
         objectType => 'DREV',
@@ -105,19 +105,11 @@ sub create {
         ]
     };
 
-    if (@$project_names) {
-        my $project_phids = [];
-        foreach my $project_name (@$project_names) {
-            my $project = Bugzilla::Extension::PhabBugz::Project->new_from_query({ name => $project_name });
-            push @$project_phids, $project->phid if $project;
-        }
-
-        ThrowUserError('invalid_phabricator_projects') unless @$project_phids;
-
+    if (@$projects) {
         push @{ $data->{policy} }, {
             action => 'allow',
             rule   => 'PhabricatorProjectsAllPolicyRule',
-            value  => $project_phids,
+            value  => [ map { $_->phid } @$projects ],
         };
     }
     else {
@@ -138,8 +130,6 @@ sub _build_rule_projects {
     my $rule = first { $_->{rule} =~ /PhabricatorProjects(?:All)?PolicyRule/ } @{ $self->rules };
     return [] unless $rule;
     return [
-        map  { $_->name }
-        grep { $_ }
         map  { Bugzilla::Extension::PhabBugz::Project->new_from_query( { phids => [$_] } ) }
         @{ $rule->{value} }
     ];
diff --git a/extensions/PhabBugz/lib/Revision.pm b/extensions/PhabBugz/lib/Revision.pm
index 854cc48d4..a39e7169c 100644
--- a/extensions/PhabBugz/lib/Revision.pm
+++ b/extensions/PhabBugz/lib/Revision.pm
@@ -17,6 +17,7 @@ use Type::Utils;
 use Bugzilla::Bug;
 use Bugzilla::Error;
 use Bugzilla::Util qw(trim);
+use Bugzilla::Extension::PhabBugz::Project;
 use Bugzilla::Extension::PhabBugz::User;
 use Bugzilla::Extension::PhabBugz::Util qw(request);
 
@@ -35,12 +36,12 @@ has author_phid      => ( is => 'ro',   isa => Str );
 has bug_id           => ( is => 'ro',   isa => Str );
 has view_policy      => ( is => 'ro',   isa => Str );
 has edit_policy      => ( is => 'ro',   isa => Str );
-has projects_raw     => ( is => 'ro',   isa => ArrayRef [Str] );
 has subscriber_count => ( is => 'ro',   isa => Int );
 has bug              => ( is => 'lazy', isa => Object );
 has author           => ( is => 'lazy', isa => Object );
 has reviewers        => ( is => 'lazy', isa => ArrayRef [Object] );
 has subscribers      => ( is => 'lazy', isa => ArrayRef [Object] );
+has projects         => ( is => 'lazy', isa => ArrayRef [Object] );
 has reviewers_raw => (
     is  => 'ro',
     isa => ArrayRef [
@@ -60,6 +61,12 @@ has subscribers_raw => (
         viewerIsSubscribed => Bool,
     ]
 );
+has projects_raw => (
+    is => 'ro',
+    isa => Dict [
+        projectPHIDs => ArrayRef [Str]
+    ]
+);
 
 sub new_from_query {
     my ( $class, $params ) = @_;
@@ -91,18 +98,18 @@ sub new_from_query {
 sub BUILDARGS {
     my ( $class, $params ) = @_;
 
-    $params->{title}           = $params->{fields}->{title};
-    $params->{summary}         = $params->{fields}->{summary};
-    $params->{status}          = $params->{fields}->{status}->{value};
-    $params->{creation_ts}     = $params->{fields}->{dateCreated};
-    $params->{modification_ts} = $params->{fields}->{dateModified};
-    $params->{author_phid}     = $params->{fields}->{authorPHID};
-    $params->{bug_id}          = $params->{fields}->{'bugzilla.bug-id'};
-    $params->{view_policy}     = $params->{fields}->{policy}->{view};
-    $params->{edit_policy}     = $params->{fields}->{policy}->{edit};
-    $params->{reviewers_raw} = $params->{attachments}->{reviewers}->{reviewers};
-    $params->{subscribers_raw} = $params->{attachments}->{subscribers};
-    $params->{projects}        = $params->{attachments}->{projects};
+    $params->{title}            = $params->{fields}->{title};
+    $params->{summary}          = $params->{fields}->{summary};
+    $params->{status}           = $params->{fields}->{status}->{value};
+    $params->{creation_ts}      = $params->{fields}->{dateCreated};
+    $params->{modification_ts}  = $params->{fields}->{dateModified};
+    $params->{author_phid}      = $params->{fields}->{authorPHID};
+    $params->{bug_id}           = $params->{fields}->{'bugzilla.bug-id'};
+    $params->{view_policy}      = $params->{fields}->{policy}->{view};
+    $params->{edit_policy}      = $params->{fields}->{policy}->{edit};
+    $params->{reviewers_raw}    = $params->{attachments}->{reviewers}->{reviewers};
+    $params->{subscribers_raw}  = $params->{attachments}->{subscribers};
+    $params->{projects_raw}     = $params->{attachments}->{projects};
     $params->{subscriber_count} =
       $params->{attachments}->{subscribers}->{subscriberCount};
 
@@ -343,6 +350,24 @@ sub _build_subscribers {
     return $self->{subscribers} = $users;
 }
 
+sub _build_projects {
+    my ($self) = @_;
+
+    return $self->{projects} if $self->{projects};
+    return [] unless $self->projects_raw->{projectPHIDs};
+
+    my @projects;
+    foreach my $phid ( @{ $self->projects_raw->{projectPHIDs} } ) {
+        push @projects, Bugzilla::Extension::PhabBugz::Project->new_from_query(
+          {
+            phids => [ $phid ]
+          }
+        );
+    }
+
+    return $self->{projects} = \@projects;
+}
+
 #########################
 #       Mutators        #
 #########################
@@ -416,4 +441,57 @@ sub remove_project {
     push @{ $self->{remove_projects} }, $project_phid;
 }
 
+sub make_private {
+    my ( $self, $project_names ) = @_;
+
+    my $secure_revision_project =
+      Bugzilla::Extension::PhabBugz::Project->new_from_query(
+        {
+          name => 'secure-revision'
+        }
+      );
+
+    my @set_projects;
+    foreach my $name (@$project_names) {
+        my $set_project =
+          Bugzilla::Extension::PhabBugz::Project->new_from_query(
+            {
+                name => $name
+            }
+            );
+        push @set_projects, $set_project;
+    }
+
+    my $new_policy = Bugzilla::Extension::PhabBugz::Policy->create(\@set_projects);
+    $self->set_policy('view', $new_policy->phid);
+    $self->set_policy('edit', $new_policy->phid);
+
+    foreach my $project ($secure_revision_project, @set_projects) {
+        $self->add_project($project->phid);
+    }
+
+    return $self;
+}
+
+sub make_public {
+    my ( $self ) = @_;
+
+    my $edit_bugs = 
+        Bugzilla::Extension::PhabBugz::Project->new_from_query(
+        {
+          name => 'bmo-editbugs-team'
+        }
+      );
+
+    $self->set_policy('view', 'public');
+    $self->set_policy('edit', ($edit_bugs ? $edit_bugs->phid : 'users'));
+
+    my @current_group_projects = grep { $_->name =~ /^(bmo-.*|secure-revision)$/ } @{ $self->projects };
+    foreach my $project (@current_group_projects) {
+        $self->remove_project($project->phid);
+    }
+
+    return $self;
+}
+
 1;
diff --git a/extensions/Push/lib/Connector/Phabricator.pm b/extensions/Push/lib/Connector/Phabricator.pm
index aeef32ab4..5d5e4e639 100644
--- a/extensions/Push/lib/Connector/Phabricator.pm
+++ b/extensions/Push/lib/Connector/Phabricator.pm
@@ -78,19 +78,13 @@ sub send {
       : 0;
 
     foreach my $revision (@$revisions) {
-        my $secure_revision = Bugzilla::Extension::PhabBugz::Project->new_from_query({
-            name => 'secure-revision'
-        });
-
         if ( $is_public && $group_change ) {
             Bugzilla->audit(sprintf(
               'Making revision %s public for bug %s',
               $revision->id,
               $bug->id
             ));
-            $revision->set_policy('view', 'public');
-            $revision->set_policy('edit', 'users');
-            $revision->remove_project($secure_revision->phid);
+            $revision->make_public();
         }
         elsif ( !$is_public && !@set_groups ) {
             Bugzilla->audit(sprintf(
@@ -99,9 +93,7 @@ sub send {
               $bug->id,
               join(', ', @set_groups)
             ));
-            $revision->set_policy('view', $secure_revision->phid);
-            $revision->set_policy('edit', $secure_revision->phid);
-            $revision->add_project($secure_revision->phid);
+            $revision->make_private(['secure-revision']);
             add_security_sync_comments([$revision], $bug);
         }
         elsif ( !$is_public && $group_change ) {
@@ -110,11 +102,8 @@ sub send {
               $revision->id,
               $bug->id
             ));
-            my @set_projects = map { "bmo-" . $_ } @set_groups;
-            my $new_policy = Bugzilla::Extension::PhabBugz::Policy->create(\@set_projects);
-            $revision->set_policy('view', $new_policy->phid);
-            $revision->set_policy('edit', $new_policy->phid);
-            $revision->add_project($secure_revision->phid);
+            my @set_project_names = map { "bmo-" . $_ } @set_groups;
+            $revision->make_private(\@set_project_names);
         }
 
         # Subscriber list of the private revision should always match