Implement "Auto Review" in packages with a "Subscribe" option

Summary: Ref T10939. Ref T8887. This moves toward letting packages automatically become reviewers or blocking reviewers of owned code. This change adds an "Auto Review" option to packages. Because adding reviewers/blocking reviewers is a little tricky, it doesn't actually have these options yet -- just a "subscribe" option. I'll do the reviewer work in the next update. Test Plan: Created a revision in a package with "Auto Review: Subscribe to Changes". The package got subscribed. {F1311677} {F1311678} {F1311679} Reviewers: chad Reviewed By: chad Maniphest Tasks: T8887, T10939 Differential Revision: https://secure.phabricator.com/D15915
2024-11-18 12:52:42 +01:00 · 2016-05-13 06:26:54 -07:00 · 2016-05-13 06:26:54 -07:00 · 52ac242eb3
commit 52ac242eb3
parent 70ddb1c45f
10 changed files with 182 additions and 9 deletions
--- a/resources/sql/autopatches/20160513.owners.01.autoreview.sql
+++ b/resources/sql/autopatches/20160513.owners.01.autoreview.sql
@ -0,0 +1,2 @@
+ALTER TABLE {$NAMESPACE}_owners.owners_package
+  ADD autoReview VARCHAR(32) NOT NULL COLLATE {$COLLATE_TEXT};
--- a/resources/sql/autopatches/20160513.owners.02.autoreviewnone.sql
+++ b/resources/sql/autopatches/20160513.owners.02.autoreviewnone.sql
@ -0,0 +1,2 @@
+UPDATE {$NAMESPACE}_owners.owners_package
+  SET autoReview = 'none' WHERE autoReview = '';
--- a/src/applications/differential/customfield/DifferentialProjectReviewersField.php
+++ b/src/applications/differential/customfield/DifferentialProjectReviewersField.php
@ -8,7 +8,7 @@ final class DifferentialProjectReviewersField
  }

  public function getFieldName() {
-    return pht('Coalition Reviewers');
+    return pht('Group Reviewers');
  }

  public function getFieldDescription() {
--- a/src/applications/differential/editor/DifferentialTransactionEditor.php
+++ b/src/applications/differential/editor/DifferentialTransactionEditor.php
@ -7,6 +7,7 @@ final class DifferentialTransactionEditor
  private $isCloseByCommit;
  private $repositoryPHIDOverride = false;
  private $didExpandInlineState = false;
+  private $affectedPaths;

  public function getEditorApplicationClass() {
    return 'PhabricatorDifferentialApplication';
@ -1481,6 +1482,75 @@ final class DifferentialTransactionEditor
    return parent::shouldApplyHeraldRules($object, $xactions);
  }

+  protected function didApplyHeraldRules(
+    PhabricatorLiskDAO $object,
+    HeraldAdapter $adapter,
+    HeraldTranscript $transcript) {
+
+    $repository = $object->getRepository();
+    if (!$repository) {
+      return array();
+    }
+
+    if (!$this->affectedPaths) {
+      return array();
+    }
+
+    $packages = PhabricatorOwnersPackage::loadAffectedPackages(
+      $repository,
+      $this->affectedPaths);
+
+    foreach ($packages as $key => $package) {
+      if ($package->isArchived()) {
+        unset($packages[$key]);
+      }
+    }
+
+    if (!$packages) {
+      return array();
+    }
+
+    $auto_subscribe = array();
+    $auto_review = array();
+    $auto_block = array();
+
+    foreach ($packages as $package) {
+      switch ($package->getAutoReview()) {
+        case PhabricatorOwnersPackage::AUTOREVIEW_SUBSCRIBE:
+          $auto_subscribe[] = $package;
+          break;
+        case PhabricatorOwnersPackage::AUTOREVIEW_REVIEW:
+          $auto_review[] = $package;
+          break;
+        case PhabricatorOwnersPackage::AUTOREVIEW_BLOCK:
+          $auto_block[] = $package;
+          break;
+        case PhabricatorOwnersPackage::AUTOREVIEW_NONE:
+        default:
+          break;
+      }
+    }
+
+    $owners_phid = id(new PhabricatorOwnersApplication())
+      ->getPHID();
+
+    $xactions = array();
+    if ($auto_subscribe) {
+
+      $xactions[] = $object->getApplicationTransactionTemplate()
+        ->setAuthorPHID($owners_phid)
+        ->setTransactionType(PhabricatorTransactions::TYPE_SUBSCRIBERS)
+        ->setNewValue(
+          array(
+            '+' => mpull($auto_subscribe, 'getPHID'),
+          ));
+    }
+
+    // TODO: Implement autoreview and autoblock, but these are more invovled.
+
+    return $xactions;
+  }
+
  protected function buildHeraldAdapter(
    PhabricatorLiskDAO $object,
    array $xactions) {
@ -1557,6 +1627,9 @@ final class DifferentialTransactionEditor
    }
    $all_paths = array_keys($all_paths);

+    // Save the affected paths; we'll use them later to query Owners.
+    $this->affectedPaths = $all_paths;
+
    $path_ids =
      PhabricatorRepositoryCommitChangeParserWorker::lookupOrCreatePaths(
        $all_paths);
--- a/src/applications/owners/controller/PhabricatorOwnersDetailController.php
+++ b/src/applications/owners/controller/PhabricatorOwnersDetailController.php
@ -184,6 +184,12 @@ final class PhabricatorOwnersDetailController
    }
    $view->addProperty(pht('Owners'), $owner_list);

+    $auto = $package->getAutoReview();
+    $autoreview_map = PhabricatorOwnersPackage::getAutoreviewOptionsMap();
+    $spec = idx($autoreview_map, $auto, array());
+    $name = idx($spec, 'name', $auto);
+    $view->addProperty(pht('Auto Review'), $name);
+
    if ($package->getAuditingEnabled()) {
      $auditing = pht('Enabled');
    } else {
--- a/src/applications/owners/editor/PhabricatorOwnersPackageEditEngine.php
+++ b/src/applications/owners/editor/PhabricatorOwnersPackageEditEngine.php
@ -84,6 +84,9 @@ applying a transaction of this type.
 EOTEXT
      );

+    $autoreview_map = PhabricatorOwnersPackage::getAutoreviewOptionsMap();
+    $autoreview_map = ipull($autoreview_map, 'name');
+
    return array(
      id(new PhabricatorTextEditField())
        ->setKey('name')
@ -100,6 +103,18 @@ EOTEXT
        ->setDatasource(new PhabricatorProjectOrUserDatasource())
        ->setIsCopyable(true)
        ->setValue($object->getOwnerPHIDs()),
+      id(new PhabricatorSelectEditField())
+        ->setKey('autoReview')
+        ->setLabel(pht('Auto Review'))
+        ->setDescription(
+          pht(
+            'Automatically trigger reviews for commits affecting files in '.
+            'this package.'))
+        ->setTransactionType(
+          PhabricatorOwnersPackageTransaction::TYPE_AUTOREVIEW)
+        ->setIsCopyable(true)
+        ->setValue($object->getAutoReview())
+        ->setOptions($autoreview_map),
      id(new PhabricatorSelectEditField())
        ->setKey('auditing')
        ->setLabel(pht('Auditing'))
--- a/src/applications/owners/editor/PhabricatorOwnersPackageTransactionEditor.php
+++ b/src/applications/owners/editor/PhabricatorOwnersPackageTransactionEditor.php
@ -20,6 +20,7 @@ final class PhabricatorOwnersPackageTransactionEditor
    $types[] = PhabricatorOwnersPackageTransaction::TYPE_DESCRIPTION;
    $types[] = PhabricatorOwnersPackageTransaction::TYPE_PATHS;
    $types[] = PhabricatorOwnersPackageTransaction::TYPE_STATUS;
+    $types[] = PhabricatorOwnersPackageTransaction::TYPE_AUTOREVIEW;

    $types[] = PhabricatorTransactions::TYPE_VIEW_POLICY;
    $types[] = PhabricatorTransactions::TYPE_EDIT_POLICY;
@ -47,6 +48,8 @@ final class PhabricatorOwnersPackageTransactionEditor
        return mpull($paths, 'getRef');
      case PhabricatorOwnersPackageTransaction::TYPE_STATUS:
        return $object->getStatus();
+      case PhabricatorOwnersPackageTransaction::TYPE_AUTOREVIEW:
+        return $object->getAutoReview();
    }
  }

@ -58,6 +61,7 @@ final class PhabricatorOwnersPackageTransactionEditor
      case PhabricatorOwnersPackageTransaction::TYPE_NAME:
      case PhabricatorOwnersPackageTransaction::TYPE_DESCRIPTION:
      case PhabricatorOwnersPackageTransaction::TYPE_STATUS:
+      case PhabricatorOwnersPackageTransaction::TYPE_AUTOREVIEW:
        return $xaction->getNewValue();
      case PhabricatorOwnersPackageTransaction::TYPE_PATHS:
        $new = $xaction->getNewValue();
@ -113,6 +117,9 @@ final class PhabricatorOwnersPackageTransactionEditor
      case PhabricatorOwnersPackageTransaction::TYPE_STATUS:
        $object->setStatus($xaction->getNewValue());
        return;
+      case PhabricatorOwnersPackageTransaction::TYPE_AUTOREVIEW:
+        $object->setAutoReview($xaction->getNewValue());
+        return;
    }

    return parent::applyCustomInternalTransaction($object, $xaction);
@ -127,6 +134,7 @@ final class PhabricatorOwnersPackageTransactionEditor
      case PhabricatorOwnersPackageTransaction::TYPE_DESCRIPTION:
      case PhabricatorOwnersPackageTransaction::TYPE_AUDITING:
      case PhabricatorOwnersPackageTransaction::TYPE_STATUS:
+      case PhabricatorOwnersPackageTransaction::TYPE_AUTOREVIEW:
        return;
      case PhabricatorOwnersPackageTransaction::TYPE_OWNERS:
        $old = $xaction->getOldValue();
@ -220,6 +228,26 @@ final class PhabricatorOwnersPackageTransactionEditor
          }
        }

+        break;
+      case PhabricatorOwnersPackageTransaction::TYPE_AUTOREVIEW:
+        $map = PhabricatorOwnersPackage::getAutoreviewOptionsMap();
+        foreach ($xactions as $xaction) {
+          $new = $xaction->getNewValue();
+
+          if (empty($map[$new])) {
+            $valid = array_keys($map);
+
+            $errors[] = new PhabricatorApplicationTransactionValidationError(
+              $type,
+              pht('Invalid'),
+              pht(
+                'Autoreview setting "%s" is not valid. '.
+                'Valid settings are: %s.',
+                $new,
+                implode(', ', $valid)),
+              $xaction);
+          }
+        }
        break;
      case PhabricatorOwnersPackageTransaction::TYPE_PATHS:
        if (!$xactions) {
--- a/src/applications/owners/storage/PhabricatorOwnersPackage.php
+++ b/src/applications/owners/storage/PhabricatorOwnersPackage.php
@ -14,6 +14,7 @@ final class PhabricatorOwnersPackage
  protected $name;
  protected $originalName;
  protected $auditingEnabled;
+  protected $autoReview;
  protected $description;
  protected $primaryOwnerPHID;
  protected $mailKey;
@ -28,6 +29,11 @@ final class PhabricatorOwnersPackage
  const STATUS_ACTIVE = 'active';
  const STATUS_ARCHIVED = 'archived';

+  const AUTOREVIEW_NONE = 'none';
+  const AUTOREVIEW_SUBSCRIBE = 'subscribe';
+  const AUTOREVIEW_REVIEW = 'review';
+  const AUTOREVIEW_BLOCK = 'block';
+
  public static function initializeNewPackage(PhabricatorUser $actor) {
    $app = id(new PhabricatorApplicationQuery())
      ->setViewer($actor)
@ -41,6 +47,7 @@ final class PhabricatorOwnersPackage

    return id(new PhabricatorOwnersPackage())
      ->setAuditingEnabled(0)
+      ->setAutoReview(self::AUTOREVIEW_NONE)
      ->setViewPolicy($view_policy)
      ->setEditPolicy($edit_policy)
      ->attachPaths(array())
@ -56,6 +63,24 @@ final class PhabricatorOwnersPackage
    );
  }

+  public static function getAutoreviewOptionsMap() {
+    return array(
+      self::AUTOREVIEW_NONE => array(
+        'name' => pht('No Autoreview'),
+      ),
+      self::AUTOREVIEW_SUBSCRIBE => array(
+        'name' => pht('Subscribe to Changes'),
+      ),
+      // TODO: Implement these.
+      // self::AUTOREVIEW_REVIEW => array(
+      //   'name' => pht('Review Changes'),
+      // ),
+      // self::AUTOREVIEW_BLOCK => array(
+      //   'name' => pht('Review Changes (Blocking)'),
+      // ),
+    );
+  }
+
  protected function getConfiguration() {
    return array(
      // This information is better available from the history table.
@ -69,6 +94,7 @@ final class PhabricatorOwnersPackage
        'auditingEnabled' => 'bool',
        'mailKey' => 'bytes20',
        'status' => 'text32',
+        'autoReview' => 'text32',
      ),
    ) + parent::getConfiguration();
  }
--- a/src/applications/owners/storage/PhabricatorOwnersPackageTransaction.php
+++ b/src/applications/owners/storage/PhabricatorOwnersPackageTransaction.php
@ -10,6 +10,7 @@ final class PhabricatorOwnersPackageTransaction
  const TYPE_DESCRIPTION = 'owners.description';
  const TYPE_PATHS = 'owners.paths';
  const TYPE_STATUS = 'owners.status';
+  const TYPE_AUTOREVIEW = 'owners.autoreview';

  public function getApplicationName() {
    return 'owners';
@ -143,6 +144,18 @@ final class PhabricatorOwnersPackageTransaction
            '%s archived this package.',
            $this->renderHandleLink($author_phid));
        }
+      case self::TYPE_AUTOREVIEW:
+        $map = PhabricatorOwnersPackage::getAutoreviewOptionsMap();
+        $map = ipull($map, 'name');
+
+        $old = idx($map, $old, $old);
+        $new = idx($map, $new, $new);
+
+        return pht(
+          '%s adjusted autoreview from "%s" to "%s".',
+          $this->renderHandleLink($author_phid),
+          $old,
+          $new);
    }

    return parent::getTitle();
--- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
+++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
@ -703,7 +703,13 @@ abstract class PhabricatorApplicationTransactionEditor
      $xaction->setEditPolicy($this->getActingAsPHID());
    }

-    $xaction->setAuthorPHID($this->getActingAsPHID());
+    // If the transaction already has an explicit author PHID, allow it to
+    // stand. This is used by applications like Owners that hook into the
+    // post-apply change pipeline.
+    if (!$xaction->getAuthorPHID()) {
+      $xaction->setAuthorPHID($this->getActingAsPHID());
+    }
+
    $xaction->setContentSource($this->getContentSource());
    $xaction->attachViewer($actor);
    $xaction->attachObject($object);
@ -957,6 +963,12 @@ abstract class PhabricatorApplicationTransactionEditor
      if ($herald_xactions) {
        $xscript_id = $this->getHeraldTranscript()->getID();
        foreach ($herald_xactions as $herald_xaction) {
+          // Don't set a transcript ID if this is a transaction from another
+          // application or source, like Owners.
+          if ($herald_xaction->getAuthorPHID()) {
+            continue;
+          }
+
          $herald_xaction->setMetadataValue('herald:transcriptID', $xscript_id);
        }

@ -1217,6 +1229,7 @@ abstract class PhabricatorApplicationTransactionEditor
          $xaction,
          pht('You can not apply transactions which already have IDs/PHIDs!'));
      }
+
      if ($xaction->getObjectPHID()) {
        throw new PhabricatorApplicationTransactionStructureException(
          $xaction,
@ -1224,13 +1237,7 @@ abstract class PhabricatorApplicationTransactionEditor
            'You can not apply transactions which already have %s!',
            'objectPHIDs'));
      }
-      if ($xaction->getAuthorPHID()) {
-        throw new PhabricatorApplicationTransactionStructureException(
-          $xaction,
-          pht(
-            'You can not apply transactions which already have %s!',
-            'authorPHIDs'));
-      }
+
      if ($xaction->getCommentPHID()) {
        throw new PhabricatorApplicationTransactionStructureException(
          $xaction,
@ -1238,6 +1245,7 @@ abstract class PhabricatorApplicationTransactionEditor
            'You can not apply transactions which already have %s!',
            'commentPHIDs'));
      }
+
      if ($xaction->getCommentVersion() !== 0) {
        throw new PhabricatorApplicationTransactionStructureException(
          $xaction,