1 files changed, 93 insertions, 54 deletions

diff --git a/extensions/PhabBugz/lib/Feed.pm b/extensions/PhabBugz/lib/Feed.pm
index 8b6801e91..670bc5d24 100644
--- a/extensions/PhabBugz/lib/Feed.pm
+++ b/extensions/PhabBugz/lib/Feed.pm
@@ -14,7 +14,10 @@ use List::MoreUtils qw(any);
 use Moo;
 
 use Bugzilla::Constants;
+use Bugzilla::Util qw(diff_arrays);
+
 use Bugzilla::Extension::PhabBugz::Constants;
+use Bugzilla::Extension::PhabBugz::Policy;
 use Bugzilla::Extension::PhabBugz::Revision;
 use Bugzilla::Extension::PhabBugz::Util qw(
     add_security_sync_comments
@@ -104,25 +107,7 @@ sub feed_query {
             }
         }
 
-        my $revision = Bugzilla::Extension::PhabBugz::Revision->new({ phids => [$object_phid] });
-
-        if (!$revision->bug_id) {
-            if ($story_text =~ /\s+created\s+D\d+/) {
-                # If new revision and bug id was omitted, make revision public
-                $self->logger->debug("No bug associated with new revision. Marking public.");
-                $revision->set_policy('view', 'public');
-                $revision->set_policy('edit', 'users');
-                $revision->update();
-                $self->logger->info("SUCCESS");
-            }
-            else {
-                $self->logger->debug("SKIPPING: No bug associated with revision change");
-            }
-            $self->save_feed_last_id($story_id);
-            next;
-        }
-
-        $self->process_revision_change($revision, $story_text);
+        $self->process_revision_change($object_phid, $story_text);
         $self->save_feed_last_id($story_id);
     }
 }
@@ -136,18 +121,28 @@ sub save_feed_last_id {
 }
 
 sub process_revision_change {
-    my ($self, $revision, $story_text) = @_;
+    my ($self, $revision_phid, $story_text) = @_;
 
-    # Pre setup before making changes
-    my $old_user = set_phab_user();
+    # Load the revision from Phabricator
+    my $revision = Bugzilla::Extension::PhabBugz::Revision->new({ phids => [ $revision_phid ] });
 
-    my $is_shadow_db = Bugzilla->is_shadow_db;
-    Bugzilla->switch_to_main_db if $is_shadow_db;
+    # NO BUG ID
 
-    my $dbh = Bugzilla->dbh;
-    $dbh->bz_start_transaction;
-
-    my ($timestamp) = Bugzilla->dbh->selectrow_array("SELECT NOW()");
+    if (!$revision->bug_id) {
+        if ($story_text =~ /\s+created\s+D\d+/) {
+            # If new revision and bug id was omitted, make revision public
+            $self->logger->debug("No bug associated with new revision. Marking public.");
+            $revision->set_policy('view', 'public');
+            $revision->set_policy('edit', 'users');
+            $revision->update();
+            $self->logger->info("SUCCESS");
+            return;
+        }
+        else {
+            $self->logger->debug("SKIPPING: No bug associated with revision change");
+            return;
+        }
+    }
 
     my $log_message = sprintf(
         "REVISION CHANGE FOUND: D%d: %s | bug: %d | %s",
@@ -157,39 +152,73 @@ sub process_revision_change {
         $story_text);
     $self->logger->info($log_message);
 
+    # Pre setup before making changes
+    my $old_user = set_phab_user();
+
+    my $is_shadow_db = Bugzilla->is_shadow_db;
+    Bugzilla->switch_to_main_db if $is_shadow_db;
+
+    my $dbh = Bugzilla->dbh;
+    $dbh->bz_start_transaction;
+
     my $bug = Bugzilla::Bug->new({ id => $revision->bug_id, cache => 1 });
 
     # REVISION SECURITY POLICY
 
-    # Do not set policy if a custom policy has already been set
-    # This keeps from setting new custom policy everytime a change
-    # is made.
-    unless ($revision->view_policy =~ /^PHID-PLCY/) {
-
-        # If bug is public then remove privacy policy
-        if (!@{ $bug->groups_in }) {
-            $revision->set_policy('view', 'public');
-            $revision->set_policy('edit', 'users');
+    # If bug is public then remove privacy policy
+    if (!@{ $bug->groups_in }) {
+        $self->logger->debug('Bug is public so setting view/edit public');
+        $revision->set_policy('view', 'public');
+        $revision->set_policy('edit', 'users');
+    }
+    # else bug is private.
+    else {
+        my @set_groups = get_security_sync_groups($bug);
+
+        # If bug privacy groups do not have any matching synchronized groups,
+        # then leave revision private and it will have be dealt with manually.
+        if (!@set_groups) {
+            $self->logger->debug('No matching groups. Adding comments to bug and revision');
+            add_security_sync_comments([$revision], $bug);
         }
-        # else bug is private
+        # Otherwise, we create a new custom policy containing the project
+        # groups that are mapped to bugzilla groups.
         else {
-            my @set_groups = get_security_sync_groups($bug);
+            my @set_projects = map { "bmo-" . $_ } @set_groups;
+
+            # If current policy projects matches what we want to set, then
+            # we leave the current policy alone.
+            my $current_policy;
+            if ($revision->view_policy =~ /^PHID-PLCY/) {
+                $self->logger->debug("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;
+                $self->logger->debug("Current policy projects: " . join(", ", @$current_projects));
+                my ($added, $removed) = diff_arrays($current_projects, \@set_projects);
+                if (@$added || @$removed) {
+                    $self->logger->debug('Project groups do not match. Need new custom policy');
+                    $current_policy= undef;
+                }
+                else {
+                    $self->logger->debug('Project groups match. Leaving current policy as-is');
+                }
+            }
 
-            # If bug privacy groups do not have any matching synchronized groups,
-            # then leave revision private and it will have be dealt with manually.
-            if (!@set_groups) {
-                add_security_sync_comments([$revision], $bug);
+            if (!$current_policy) {
+                $self->logger->debug("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);
             }
 
-            my $policy_phid = create_private_revision_policy($bug, \@set_groups);
             my $subscribers = get_bug_role_phids($bug);
-
-            $revision->set_policy('view', $policy_phid);
-            $revision->set_policy('edit', $policy_phid);
             $revision->set_subscribers($subscribers);
         }
     }
 
+    my ($timestamp) = Bugzilla->dbh->selectrow_array("SELECT NOW()");
+
     my $attachment = create_revision_attachment($bug, $revision->id, $revision->title, $timestamp);
 
     # ATTACHMENT OBSOLETES
@@ -203,24 +232,28 @@ sub process_revision_change {
         next if $attach_revision_id != $revision->id;
 
         my $make_obsolete = $revision->status eq 'abandoned' ? 1 : 0;
+        $self->logger->debug('Updating obsolete status on attachmment ' . $attachment->id);
         $attachment->set_is_obsolete($make_obsolete);
 
-        if ($revision->id == $attach_revision_id
-            && $revision->title ne $attachment->description) {
+        if ($revision->title ne $attachment->description) {
+            $self->logger->debug('Updating description on attachment ' . $attachment->id);
             $attachment->set_description($revision->title);
         }
 
         $attachment->update($timestamp);
-        last;
     }
 
     # fixup attachments with same revision id but on different bugs
+    my %other_bugs;
     my $other_attachments = Bugzilla::Attachment->match({
         mimetype => PHAB_CONTENT_TYPE,
         filename => 'phabricator-D' . $revision->id . '-url.txt',
         WHERE    => { 'bug_id != ? AND NOT isobsolete' => $bug->id }
     });
     foreach my $attachment (@$other_attachments) {
+        $other_bugs{$attachment->bug_id}++;
+        $self->logger->debug('Updating obsolete status on attachment ' .
+                             $attachment->id . " for bug " . $attachment->bug_id);
         $attachment->set_is_obsolete(1);
         $attachment->update($timestamp);
     }
@@ -228,9 +261,11 @@ sub process_revision_change {
     # REVIEWER STATUSES
 
     my (@accepted_phids, @denied_phids, @accepted_user_ids, @denied_user_ids);
-    foreach my $reviewer (@{ $revision->reviewers }) {
-        push(@accepted_phids, $reviewer->phab_phid) if $reviewer->phab_review_status eq 'accepted';
-        push(@denied_phids, $reviewer->phab_phid) if $reviewer->phab_review_status eq 'rejected';
+    unless ($revision->status eq 'changes-planned' || $revision->status eq 'needs-review') {
+        foreach my $reviewer (@{ $revision->reviewers }) {
+            push(@accepted_phids, $reviewer->phab_phid) if $reviewer->phab_review_status eq 'accepted';
+            push(@denied_phids, $reviewer->phab_phid) if $reviewer->phab_review_status eq 'rejected';
+        }
     }
 
     my $phab_users = get_phab_bmo_ids({ phids => \@accepted_phids });
@@ -301,7 +336,11 @@ sub process_revision_change {
     $bug->update($timestamp);
     $revision->update();
 
-    Bugzilla::BugMail::Send($revision->bug_id, { changer => Bugzilla->user });
+    # Email changes for this revisions bug and also for any other
+    # bugs that previously had these revision attachments
+    foreach my $bug_id ($revision->bug_id, keys %other_bugs) {
+        Bugzilla::BugMail::Send($bug_id, { changer => Bugzilla->user });
+    }
 
     $dbh->bz_commit_transaction;
     Bugzilla->switch_to_shadow_db if $is_shadow_db;