Modularize Owners package transactions

Summary: Converts Owners package transactions to modular transactions.

Test Plan:
 - created a new package
 - edited all simple properties from the web ui
 - checked that project and user owners were added as reviewers appropriately to new diffs
 - inspected the change details for various types of path add / remove / update / reorder changes

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D16651

src/__phutil_library_map__.php

@@ -3017,20 +3017,30 @@ phutil_register_library_map(array(
     'PhabricatorOwnersListController' => 'applications/owners/controller/PhabricatorOwnersListController.php',
     'PhabricatorOwnersOwner' => 'applications/owners/storage/PhabricatorOwnersOwner.php',
     'PhabricatorOwnersPackage' => 'applications/owners/storage/PhabricatorOwnersPackage.php',
+    'PhabricatorOwnersPackageAuditingTransaction' => 'applications/owners/xaction/PhabricatorOwnersPackageAuditingTransaction.php',
+    'PhabricatorOwnersPackageAutoreviewTransaction' => 'applications/owners/xaction/PhabricatorOwnersPackageAutoreviewTransaction.php',
     'PhabricatorOwnersPackageDatasource' => 'applications/owners/typeahead/PhabricatorOwnersPackageDatasource.php',
+    'PhabricatorOwnersPackageDescriptionTransaction' => 'applications/owners/xaction/PhabricatorOwnersPackageDescriptionTransaction.php',
+    'PhabricatorOwnersPackageDominionTransaction' => 'applications/owners/xaction/PhabricatorOwnersPackageDominionTransaction.php',
     'PhabricatorOwnersPackageEditEngine' => 'applications/owners/editor/PhabricatorOwnersPackageEditEngine.php',
     'PhabricatorOwnersPackageFulltextEngine' => 'applications/owners/query/PhabricatorOwnersPackageFulltextEngine.php',
     'PhabricatorOwnersPackageFunctionDatasource' => 'applications/owners/typeahead/PhabricatorOwnersPackageFunctionDatasource.php',
     'PhabricatorOwnersPackageNameNgrams' => 'applications/owners/storage/PhabricatorOwnersPackageNameNgrams.php',
+    'PhabricatorOwnersPackageNameTransaction' => 'applications/owners/xaction/PhabricatorOwnersPackageNameTransaction.php',
     'PhabricatorOwnersPackageOwnerDatasource' => 'applications/owners/typeahead/PhabricatorOwnersPackageOwnerDatasource.php',
+    'PhabricatorOwnersPackageOwnersTransaction' => 'applications/owners/xaction/PhabricatorOwnersPackageOwnersTransaction.php',
     'PhabricatorOwnersPackagePHIDType' => 'applications/owners/phid/PhabricatorOwnersPackagePHIDType.php',
+    'PhabricatorOwnersPackagePathsTransaction' => 'applications/owners/xaction/PhabricatorOwnersPackagePathsTransaction.php',
+    'PhabricatorOwnersPackagePrimaryTransaction' => 'applications/owners/xaction/PhabricatorOwnersPackagePrimaryTransaction.php',
     'PhabricatorOwnersPackageQuery' => 'applications/owners/query/PhabricatorOwnersPackageQuery.php',
     'PhabricatorOwnersPackageRemarkupRule' => 'applications/owners/remarkup/PhabricatorOwnersPackageRemarkupRule.php',
     'PhabricatorOwnersPackageSearchEngine' => 'applications/owners/query/PhabricatorOwnersPackageSearchEngine.php',
+    'PhabricatorOwnersPackageStatusTransaction' => 'applications/owners/xaction/PhabricatorOwnersPackageStatusTransaction.php',
     'PhabricatorOwnersPackageTestCase' => 'applications/owners/storage/__tests__/PhabricatorOwnersPackageTestCase.php',
     'PhabricatorOwnersPackageTransaction' => 'applications/owners/storage/PhabricatorOwnersPackageTransaction.php',
     'PhabricatorOwnersPackageTransactionEditor' => 'applications/owners/editor/PhabricatorOwnersPackageTransactionEditor.php',
     'PhabricatorOwnersPackageTransactionQuery' => 'applications/owners/query/PhabricatorOwnersPackageTransactionQuery.php',
+    'PhabricatorOwnersPackageTransactionType' => 'applications/owners/xaction/PhabricatorOwnersPackageTransactionType.php',
     'PhabricatorOwnersPath' => 'applications/owners/storage/PhabricatorOwnersPath.php',
     'PhabricatorOwnersPathsController' => 'applications/owners/controller/PhabricatorOwnersPathsController.php',
     'PhabricatorOwnersPathsSearchEngineAttachment' => 'applications/owners/engineextension/PhabricatorOwnersPathsSearchEngineAttachment.php',
@@ -7949,20 +7959,30 @@ phutil_register_library_map(array(
       'PhabricatorFulltextInterface',
       'PhabricatorNgramsInterface',
     ),
+    'PhabricatorOwnersPackageAuditingTransaction' => 'PhabricatorOwnersPackageTransactionType',
+    'PhabricatorOwnersPackageAutoreviewTransaction' => 'PhabricatorOwnersPackageTransactionType',
     'PhabricatorOwnersPackageDatasource' => 'PhabricatorTypeaheadDatasource',
+    'PhabricatorOwnersPackageDescriptionTransaction' => 'PhabricatorOwnersPackageTransactionType',
+    'PhabricatorOwnersPackageDominionTransaction' => 'PhabricatorOwnersPackageTransactionType',
     'PhabricatorOwnersPackageEditEngine' => 'PhabricatorEditEngine',
     'PhabricatorOwnersPackageFulltextEngine' => 'PhabricatorFulltextEngine',
     'PhabricatorOwnersPackageFunctionDatasource' => 'PhabricatorTypeaheadCompositeDatasource',
     'PhabricatorOwnersPackageNameNgrams' => 'PhabricatorSearchNgrams',
+    'PhabricatorOwnersPackageNameTransaction' => 'PhabricatorOwnersPackageTransactionType',
     'PhabricatorOwnersPackageOwnerDatasource' => 'PhabricatorTypeaheadCompositeDatasource',
+    'PhabricatorOwnersPackageOwnersTransaction' => 'PhabricatorOwnersPackageTransactionType',
     'PhabricatorOwnersPackagePHIDType' => 'PhabricatorPHIDType',
+    'PhabricatorOwnersPackagePathsTransaction' => 'PhabricatorOwnersPackageTransactionType',
+    'PhabricatorOwnersPackagePrimaryTransaction' => 'PhabricatorOwnersPackageTransactionType',
     'PhabricatorOwnersPackageQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
     'PhabricatorOwnersPackageRemarkupRule' => 'PhabricatorObjectRemarkupRule',
     'PhabricatorOwnersPackageSearchEngine' => 'PhabricatorApplicationSearchEngine',
+    'PhabricatorOwnersPackageStatusTransaction' => 'PhabricatorOwnersPackageTransactionType',
     'PhabricatorOwnersPackageTestCase' => 'PhabricatorTestCase',
-    'PhabricatorOwnersPackageTransaction' => 'PhabricatorApplicationTransaction',
+    'PhabricatorOwnersPackageTransaction' => 'PhabricatorModularTransaction',
     'PhabricatorOwnersPackageTransactionEditor' => 'PhabricatorApplicationTransactionEditor',
     'PhabricatorOwnersPackageTransactionQuery' => 'PhabricatorApplicationTransactionQuery',
+    'PhabricatorOwnersPackageTransactionType' => 'PhabricatorModularTransactionType',
     'PhabricatorOwnersPath' => 'PhabricatorOwnersDAO',
     'PhabricatorOwnersPathsController' => 'PhabricatorOwnersController',
     'PhabricatorOwnersPathsSearchEngineAttachment' => 'PhabricatorSearchEngineAttachment',

src/applications/owners/controller/PhabricatorOwnersArchiveController.php

@@ -31,8 +31,9 @@ final class PhabricatorOwnersArchiveController
 
       $xactions = array();
 
+      $type = PhabricatorOwnersPackageStatusTransaction::TRANSACTIONTYPE;
       $xactions[] = id(new PhabricatorOwnersPackageTransaction())
-        ->setTransactionType(PhabricatorOwnersPackageTransaction::TYPE_STATUS)
+        ->setTransactionType($type)
         ->setNewValue($new_status);
 
       id(new PhabricatorOwnersPackageTransactionEditor())

src/applications/owners/controller/PhabricatorOwnersPathsController.php

@@ -48,7 +48,7 @@ final class PhabricatorOwnersPathsController
         );
       }
 
-      $type_paths = PhabricatorOwnersPackageTransaction::TYPE_PATHS;
+      $type_paths = PhabricatorOwnersPackagePathsTransaction::TRANSACTIONTYPE;
 
       $xactions = array();
       $xactions[] = id(new PhabricatorOwnersPackageTransaction())

src/applications/owners/editor/PhabricatorOwnersPackageEditEngine.php

@@ -95,14 +95,16 @@ EOTEXT
         ->setKey('name')
         ->setLabel(pht('Name'))
         ->setDescription(pht('Name of the package.'))
-        ->setTransactionType(PhabricatorOwnersPackageTransaction::TYPE_NAME)
+        ->setTransactionType(
+          PhabricatorOwnersPackageNameTransaction::TRANSACTIONTYPE)
         ->setIsRequired(true)
         ->setValue($object->getName()),
       id(new PhabricatorDatasourceEditField())
         ->setKey('owners')
         ->setLabel(pht('Owners'))
         ->setDescription(pht('Users and projects which own the package.'))
-        ->setTransactionType(PhabricatorOwnersPackageTransaction::TYPE_OWNERS)
+        ->setTransactionType(
+          PhabricatorOwnersPackageOwnersTransaction::TRANSACTIONTYPE)
         ->setDatasource(new PhabricatorProjectOrUserDatasource())
         ->setIsCopyable(true)
         ->setValue($object->getOwnerPHIDs()),
@@ -112,7 +114,7 @@ EOTEXT
         ->setDescription(
           pht('Change package dominion rules.'))
         ->setTransactionType(
-          PhabricatorOwnersPackageTransaction::TYPE_DOMINION)
+          PhabricatorOwnersPackageDominionTransaction::TRANSACTIONTYPE)
         ->setIsCopyable(true)
         ->setValue($object->getDominion())
         ->setOptions($dominion_map),
@@ -124,7 +126,7 @@ EOTEXT
             'Automatically trigger reviews for commits affecting files in '.
             'this package.'))
         ->setTransactionType(
-          PhabricatorOwnersPackageTransaction::TYPE_AUTOREVIEW)
+          PhabricatorOwnersPackageAutoreviewTransaction::TRANSACTIONTYPE)
         ->setIsCopyable(true)
         ->setValue($object->getAutoReview())
         ->setOptions($autoreview_map),
@@ -135,7 +137,8 @@ EOTEXT
           pht(
             'Automatically trigger audits for commits affecting files in '.
             'this package.'))
-        ->setTransactionType(PhabricatorOwnersPackageTransaction::TYPE_AUDITING)
+        ->setTransactionType(
+          PhabricatorOwnersPackageAuditingTransaction::TRANSACTIONTYPE)
         ->setIsCopyable(true)
         ->setValue($object->getAuditingEnabled())
         ->setOptions(
@@ -148,13 +151,14 @@ EOTEXT
         ->setLabel(pht('Description'))
         ->setDescription(pht('Human-readable description of the package.'))
         ->setTransactionType(
-          PhabricatorOwnersPackageTransaction::TYPE_DESCRIPTION)
+          PhabricatorOwnersPackageDescriptionTransaction::TRANSACTIONTYPE)
         ->setValue($object->getDescription()),
       id(new PhabricatorSelectEditField())
         ->setKey('status')
         ->setLabel(pht('Status'))
         ->setDescription(pht('Archive or enable the package.'))
-        ->setTransactionType(PhabricatorOwnersPackageTransaction::TYPE_STATUS)
+        ->setTransactionType(
+          PhabricatorOwnersPackageStatusTransaction::TRANSACTIONTYPE)
         ->setIsConduitOnly(true)
         ->setValue($object->getStatus())
         ->setOptions($object->getStatusNameMap()),
@@ -162,7 +166,8 @@ EOTEXT
         ->setKey('paths.set')
         ->setLabel(pht('Paths'))
         ->setIsConduitOnly(true)
-        ->setTransactionType(PhabricatorOwnersPackageTransaction::TYPE_PATHS)
+        ->setTransactionType(
+          PhabricatorOwnersPackagePathsTransaction::TRANSACTIONTYPE)
         ->setConduitDescription(
           pht('Overwrite existing package paths with new paths.'))
         ->setConduitTypeDescription(

src/applications/owners/editor/PhabricatorOwnersPackageTransactionEditor.php

@@ -14,354 +14,12 @@ final class PhabricatorOwnersPackageTransactionEditor
   public function getTransactionTypes() {
     $types = parent::getTransactionTypes();
 
-    $types[] = PhabricatorOwnersPackageTransaction::TYPE_NAME;
-    $types[] = PhabricatorOwnersPackageTransaction::TYPE_OWNERS;
-    $types[] = PhabricatorOwnersPackageTransaction::TYPE_AUDITING;
-    $types[] = PhabricatorOwnersPackageTransaction::TYPE_DESCRIPTION;
-    $types[] = PhabricatorOwnersPackageTransaction::TYPE_PATHS;
-    $types[] = PhabricatorOwnersPackageTransaction::TYPE_STATUS;
-    $types[] = PhabricatorOwnersPackageTransaction::TYPE_AUTOREVIEW;
-    $types[] = PhabricatorOwnersPackageTransaction::TYPE_DOMINION;
-
     $types[] = PhabricatorTransactions::TYPE_VIEW_POLICY;
     $types[] = PhabricatorTransactions::TYPE_EDIT_POLICY;
 
     return $types;
   }
 
-  protected function getCustomTransactionOldValue(
-    PhabricatorLiskDAO $object,
-    PhabricatorApplicationTransaction $xaction) {
-
-    switch ($xaction->getTransactionType()) {
-      case PhabricatorOwnersPackageTransaction::TYPE_NAME:
-        return $object->getName();
-      case PhabricatorOwnersPackageTransaction::TYPE_OWNERS:
-        $phids = mpull($object->getOwners(), 'getUserPHID');
-        $phids = array_values($phids);
-        return $phids;
-      case PhabricatorOwnersPackageTransaction::TYPE_AUDITING:
-        return (int)$object->getAuditingEnabled();
-      case PhabricatorOwnersPackageTransaction::TYPE_DESCRIPTION:
-        return $object->getDescription();
-      case PhabricatorOwnersPackageTransaction::TYPE_PATHS:
-        $paths = $object->getPaths();
-        return mpull($paths, 'getRef');
-      case PhabricatorOwnersPackageTransaction::TYPE_STATUS:
-        return $object->getStatus();
-      case PhabricatorOwnersPackageTransaction::TYPE_AUTOREVIEW:
-        return $object->getAutoReview();
-      case PhabricatorOwnersPackageTransaction::TYPE_DOMINION:
-        return $object->getDominion();
-    }
-  }
-
-  protected function getCustomTransactionNewValue(
-    PhabricatorLiskDAO $object,
-    PhabricatorApplicationTransaction $xaction) {
-
-    switch ($xaction->getTransactionType()) {
-      case PhabricatorOwnersPackageTransaction::TYPE_NAME:
-      case PhabricatorOwnersPackageTransaction::TYPE_DESCRIPTION:
-      case PhabricatorOwnersPackageTransaction::TYPE_STATUS:
-      case PhabricatorOwnersPackageTransaction::TYPE_AUTOREVIEW:
-      case PhabricatorOwnersPackageTransaction::TYPE_DOMINION:
-        return $xaction->getNewValue();
-      case PhabricatorOwnersPackageTransaction::TYPE_PATHS:
-        $new = $xaction->getNewValue();
-        foreach ($new as $key => $info) {
-          $new[$key]['excluded'] = (int)idx($info, 'excluded');
-        }
-        return $new;
-      case PhabricatorOwnersPackageTransaction::TYPE_AUDITING:
-        return (int)$xaction->getNewValue();
-      case PhabricatorOwnersPackageTransaction::TYPE_OWNERS:
-        $phids = $xaction->getNewValue();
-        $phids = array_unique($phids);
-        $phids = array_values($phids);
-        return $phids;
-    }
-  }
-
-  protected function transactionHasEffect(
-    PhabricatorLiskDAO $object,
-    PhabricatorApplicationTransaction $xaction) {
-
-    switch ($xaction->getTransactionType()) {
-      case PhabricatorOwnersPackageTransaction::TYPE_PATHS:
-        $old = $xaction->getOldValue();
-        $new = $xaction->getNewValue();
-
-        $diffs = PhabricatorOwnersPath::getTransactionValueChanges($old, $new);
-        list($rem, $add) = $diffs;
-
-        return ($rem || $add);
-    }
-
-    return parent::transactionHasEffect($object, $xaction);
-  }
-
-  protected function applyCustomInternalTransaction(
-    PhabricatorLiskDAO $object,
-    PhabricatorApplicationTransaction $xaction) {
-
-    switch ($xaction->getTransactionType()) {
-      case PhabricatorOwnersPackageTransaction::TYPE_NAME:
-        $object->setName($xaction->getNewValue());
-        return;
-      case PhabricatorOwnersPackageTransaction::TYPE_DESCRIPTION:
-        $object->setDescription($xaction->getNewValue());
-        return;
-      case PhabricatorOwnersPackageTransaction::TYPE_AUDITING:
-        $object->setAuditingEnabled($xaction->getNewValue());
-        return;
-      case PhabricatorOwnersPackageTransaction::TYPE_OWNERS:
-      case PhabricatorOwnersPackageTransaction::TYPE_PATHS:
-        return;
-      case PhabricatorOwnersPackageTransaction::TYPE_STATUS:
-        $object->setStatus($xaction->getNewValue());
-        return;
-      case PhabricatorOwnersPackageTransaction::TYPE_AUTOREVIEW:
-        $object->setAutoReview($xaction->getNewValue());
-        return;
-      case PhabricatorOwnersPackageTransaction::TYPE_DOMINION:
-        $object->setDominion($xaction->getNewValue());
-        return;
-    }
-
-    return parent::applyCustomInternalTransaction($object, $xaction);
-  }
-
-  protected function applyCustomExternalTransaction(
-    PhabricatorLiskDAO $object,
-    PhabricatorApplicationTransaction $xaction) {
-
-    switch ($xaction->getTransactionType()) {
-      case PhabricatorOwnersPackageTransaction::TYPE_NAME:
-      case PhabricatorOwnersPackageTransaction::TYPE_DESCRIPTION:
-      case PhabricatorOwnersPackageTransaction::TYPE_AUDITING:
-      case PhabricatorOwnersPackageTransaction::TYPE_STATUS:
-      case PhabricatorOwnersPackageTransaction::TYPE_AUTOREVIEW:
-      case PhabricatorOwnersPackageTransaction::TYPE_DOMINION:
-        return;
-      case PhabricatorOwnersPackageTransaction::TYPE_OWNERS:
-        $old = $xaction->getOldValue();
-        $new = $xaction->getNewValue();
-
-        $owners = $object->getOwners();
-        $owners = mpull($owners, null, 'getUserPHID');
-
-        $rem = array_diff($old, $new);
-        foreach ($rem as $phid) {
-          if (isset($owners[$phid])) {
-            $owners[$phid]->delete();
-            unset($owners[$phid]);
-          }
-        }
-
-        $add = array_diff($new, $old);
-        foreach ($add as $phid) {
-          $owners[$phid] = id(new PhabricatorOwnersOwner())
-            ->setPackageID($object->getID())
-            ->setUserPHID($phid)
-            ->save();
-        }
-
-        // TODO: Attach owners here
-        return;
-      case PhabricatorOwnersPackageTransaction::TYPE_PATHS:
-        $old = $xaction->getOldValue();
-        $new = $xaction->getNewValue();
-
-        $paths = $object->getPaths();
-
-        $diffs = PhabricatorOwnersPath::getTransactionValueChanges($old, $new);
-        list($rem, $add) = $diffs;
-
-        $set = PhabricatorOwnersPath::getSetFromTransactionValue($rem);
-        foreach ($paths as $path) {
-          $ref = $path->getRef();
-          if (PhabricatorOwnersPath::isRefInSet($ref, $set)) {
-            $path->delete();
-          }
-        }
-
-        foreach ($add as $ref) {
-          $path = PhabricatorOwnersPath::newFromRef($ref)
-            ->setPackageID($object->getID())
-            ->save();
-        }
-
-        return;
-    }
-
-    return parent::applyCustomExternalTransaction($object, $xaction);
-  }
-
-  protected function validateTransaction(
-    PhabricatorLiskDAO $object,
-    $type,
-    array $xactions) {
-
-    $errors = parent::validateTransaction($object, $type, $xactions);
-
-    switch ($type) {
-      case PhabricatorOwnersPackageTransaction::TYPE_NAME:
-        $missing = $this->validateIsEmptyTextField(
-          $object->getName(),
-          $xactions);
-
-        if ($missing) {
-          $error = new PhabricatorApplicationTransactionValidationError(
-            $type,
-            pht('Required'),
-            pht('Package name is required.'),
-            nonempty(last($xactions), null));
-
-          $error->setIsMissingFieldError(true);
-          $errors[] = $error;
-        }
-
-        foreach ($xactions as $xaction) {
-          $new = $xaction->getNewValue();
-          if (preg_match('([,!])', $new)) {
-            $errors[] = new PhabricatorApplicationTransactionValidationError(
-              $type,
-              pht('Invalid'),
-              pht(
-                'Package names may not contain commas (",") or exclamation '.
-                'marks ("!"). These characters are ambiguous when package '.
-                'names are parsed from the command line.'),
-              $xaction);
-          }
-        }
-
-        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_DOMINION:
-        $map = PhabricatorOwnersPackage::getDominionOptionsMap();
-        foreach ($xactions as $xaction) {
-          $new = $xaction->getNewValue();
-
-          if (empty($map[$new])) {
-            $valid = array_keys($map);
-
-            $errors[] = new PhabricatorApplicationTransactionValidationError(
-              $type,
-              pht('Invalid'),
-              pht(
-                'Dominion setting "%s" is not valid. '.
-                'Valid settings are: %s.',
-                $new,
-                implode(', ', $valid)),
-              $xaction);
-          }
-        }
-        break;
-      case PhabricatorOwnersPackageTransaction::TYPE_PATHS:
-        if (!$xactions) {
-          continue;
-        }
-
-        $old = mpull($object->getPaths(), 'getRef');
-        foreach ($xactions as $xaction) {
-          $new = $xaction->getNewValue();
-
-          // Check that we have a list of paths.
-          if (!is_array($new)) {
-            $errors[] = new PhabricatorApplicationTransactionValidationError(
-              $type,
-              pht('Invalid'),
-              pht('Path specification must be a list of paths.'),
-              $xaction);
-            continue;
-          }
-
-          // Check that each item in the list is formatted properly.
-          $type_exception = null;
-          foreach ($new as $key => $value) {
-            try {
-              PhutilTypeSpec::checkMap(
-                $value,
-                array(
-                  'repositoryPHID' => 'string',
-                  'path' => 'string',
-                  'excluded' => 'optional wild',
-                ));
-            } catch (PhutilTypeCheckException $ex) {
-              $errors[] = new PhabricatorApplicationTransactionValidationError(
-                $type,
-                pht('Invalid'),
-                pht(
-                  'Path specification list contains invalid value '.
-                  'in key "%s": %s.',
-                  $key,
-                  $ex->getMessage()),
-                $xaction);
-              $type_exception = $ex;
-            }
-          }
-
-          if ($type_exception) {
-            continue;
-          }
-
-          // Check that any new paths reference legitimate repositories which
-          // the viewer has permission to see.
-          list($rem, $add) = PhabricatorOwnersPath::getTransactionValueChanges(
-            $old,
-            $new);
-
-          if ($add) {
-            $repository_phids = ipull($add, 'repositoryPHID');
-
-            $repositories = id(new PhabricatorRepositoryQuery())
-              ->setViewer($this->getActor())
-              ->withPHIDs($repository_phids)
-              ->execute();
-            $repositories = mpull($repositories, null, 'getPHID');
-
-            foreach ($add as $ref) {
-              $repository_phid = $ref['repositoryPHID'];
-              if (isset($repositories[$repository_phid])) {
-                continue;
-              }
-
-              $errors[] = new PhabricatorApplicationTransactionValidationError(
-                $type,
-                pht('Invalid'),
-                pht(
-                  'Path specification list references repository PHID "%s", '.
-                  'but that is not a valid, visible repository.',
-                  $repository_phid));
-            }
-          }
-        }
-        break;
-    }
-
-    return $errors;
-  }
-
   protected function shouldSendMail(
     PhabricatorLiskDAO $object,
     array $xactions) {

src/applications/owners/storage/PhabricatorOwnersPackageTransaction.php

@@ -1,17 +1,7 @@
 <?php
 
 final class PhabricatorOwnersPackageTransaction
-  extends PhabricatorApplicationTransaction {
+  extends PhabricatorModularTransaction {
-
-  const TYPE_NAME = 'owners.name';
-  const TYPE_PRIMARY = 'owners.primary';
-  const TYPE_OWNERS = 'owners.owners';
-  const TYPE_AUDITING = 'owners.auditing';
-  const TYPE_DESCRIPTION = 'owners.description';
-  const TYPE_PATHS = 'owners.paths';
-  const TYPE_STATUS = 'owners.status';
-  const TYPE_AUTOREVIEW = 'owners.autoreview';
-  const TYPE_DOMINION = 'owners.dominion';
 
   public function getApplicationName() {
     return 'owners';
@@ -21,246 +11,8 @@ final class PhabricatorOwnersPackageTransaction
     return PhabricatorOwnersPackagePHIDType::TYPECONST;
   }
 
-  public function getRequiredHandlePHIDs() {
+  public function getBaseTransactionClass() {
-    $phids = parent::getRequiredHandlePHIDs();
+    return 'PhabricatorOwnersPackageTransactionType';
-
-    $old = $this->getOldValue();
-    $new = $this->getNewValue();
-
-    switch ($this->getTransactionType()) {
-      case self::TYPE_OWNERS:
-        if (!is_array($old)) {
-          $old = array();
-        }
-
-        if (!is_array($new)) {
-          $new = array();
-        }
-
-        $add = array_diff($new, $old);
-        foreach ($add as $phid) {
-          $phids[] = $phid;
-        }
-        $rem = array_diff($old, $new);
-        foreach ($rem as $phid) {
-          $phids[] = $phid;
-        }
-        break;
-    }
-
-    return $phids;
-  }
-
-  public function shouldHide() {
-    $old = $this->getOldValue();
-    $new = $this->getNewValue();
-
-    switch ($this->getTransactionType()) {
-      case self::TYPE_DESCRIPTION:
-        if ($old === null) {
-          return true;
-        }
-        break;
-      case self::TYPE_PRIMARY:
-        // TODO: Eventually, remove these transactions entirely.
-        return true;
-    }
-
-    return parent::shouldHide();
-  }
-
-  public function getTitle() {
-    $old = $this->getOldValue();
-    $new = $this->getNewValue();
-    $author_phid = $this->getAuthorPHID();
-
-    switch ($this->getTransactionType()) {
-      case PhabricatorTransactions::TYPE_CREATE:
-        return pht(
-          '%s created this package.',
-          $this->renderHandleLink($author_phid));
-      case self::TYPE_NAME:
-        if ($old === null) {
-          return pht(
-            '%s created this package.',
-            $this->renderHandleLink($author_phid));
-        } else {
-          return pht(
-            '%s renamed this package from "%s" to "%s".',
-            $this->renderHandleLink($author_phid),
-            $old,
-            $new);
-        }
-      case self::TYPE_OWNERS:
-        $add = array_diff($new, $old);
-        $rem = array_diff($old, $new);
-        if ($add && !$rem) {
-          return pht(
-            '%s added %s owner(s): %s.',
-            $this->renderHandleLink($author_phid),
-            count($add),
-            $this->renderHandleList($add));
-        } else if ($rem && !$add) {
-          return pht(
-            '%s removed %s owner(s): %s.',
-            $this->renderHandleLink($author_phid),
-            count($rem),
-            $this->renderHandleList($rem));
-        } else {
-          return pht(
-            '%s changed %s package owner(s), added %s: %s; removed %s: %s.',
-            $this->renderHandleLink($author_phid),
-            count($add) + count($rem),
-            count($add),
-            $this->renderHandleList($add),
-            count($rem),
-            $this->renderHandleList($rem));
-        }
-      case self::TYPE_AUDITING:
-        if ($new) {
-          return pht(
-            '%s enabled auditing for this package.',
-            $this->renderHandleLink($author_phid));
-        } else {
-          return pht(
-            '%s disabled auditing for this package.',
-            $this->renderHandleLink($author_phid));
-        }
-      case self::TYPE_DESCRIPTION:
-        return pht(
-          '%s updated the description for this package.',
-          $this->renderHandleLink($author_phid));
-      case self::TYPE_PATHS:
-        // TODO: Flesh this out.
-        return pht(
-          '%s updated paths for this package.',
-          $this->renderHandleLink($author_phid));
-      case self::TYPE_STATUS:
-        if ($new == PhabricatorOwnersPackage::STATUS_ACTIVE) {
-          return pht(
-            '%s activated this package.',
-            $this->renderHandleLink($author_phid));
-        } else if ($new == PhabricatorOwnersPackage::STATUS_ARCHIVED) {
-          return pht(
-            '%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);
-      case self::TYPE_DOMINION:
-        $map = PhabricatorOwnersPackage::getDominionOptionsMap();
-        $map = ipull($map, 'short');
-
-        $old = idx($map, $old, $old);
-        $new = idx($map, $new, $new);
-
-        return pht(
-          '%s adjusted package dominion rules from "%s" to "%s".',
-          $this->renderHandleLink($author_phid),
-          $old,
-          $new);
-    }
-
-    return parent::getTitle();
-  }
-
-  public function hasChangeDetails() {
-    switch ($this->getTransactionType()) {
-      case self::TYPE_DESCRIPTION:
-        return ($this->getOldValue() !== null);
-      case self::TYPE_PATHS:
-        return true;
-    }
-
-    return parent::hasChangeDetails();
-  }
-
-  public function renderChangeDetails(PhabricatorUser $viewer) {
-    switch ($this->getTransactionType()) {
-      case self::TYPE_DESCRIPTION:
-        $old = $this->getOldValue();
-        $new = $this->getNewValue();
-
-        return $this->renderTextCorpusChangeDetails(
-          $viewer,
-          $old,
-          $new);
-      case self::TYPE_PATHS:
-        $old = $this->getOldValue();
-        $new = $this->getNewValue();
-
-        $diffs = PhabricatorOwnersPath::getTransactionValueChanges($old, $new);
-        list($rem, $add) = $diffs;
-
-        $rows = array();
-        foreach ($rem as $ref) {
-          $rows[] = array(
-            'class' => 'diff-removed',
-            'change' => '-',
-          ) + $ref;
-        }
-
-        foreach ($add as $ref) {
-          $rows[] = array(
-            'class' => 'diff-added',
-            'change' => '+',
-          ) + $ref;
-        }
-
-        $rowc = array();
-        foreach ($rows as $key => $row) {
-          $rowc[] = $row['class'];
-          $rows[$key] = array(
-            $row['change'],
-            $row['excluded'] ? pht('Exclude') : pht('Include'),
-            $viewer->renderHandle($row['repositoryPHID']),
-            $row['path'],
-          );
-        }
-
-        $table = id(new AphrontTableView($rows))
-          ->setRowClasses($rowc)
-          ->setHeaders(
-            array(
-              null,
-              pht('Type'),
-              pht('Repository'),
-              pht('Path'),
-            ))
-          ->setColumnClasses(
-            array(
-              null,
-              null,
-              null,
-              'wide',
-            ));
-
-        return $table;
-    }
-
-    return parent::renderChangeDetails($viewer);
-  }
-
-  public function getRemarkupBlocks() {
-    $blocks = parent::getRemarkupBlocks();
-
-    switch ($this->getTransactionType()) {
-      case self::TYPE_DESCRIPTION:
-        $blocks[] = $this->getNewValue();
-        break;
-    }
-
-    return $blocks;
   }
 
 }

src/applications/owners/xaction/PhabricatorOwnersPackageAuditingTransaction.php

@@ -0,0 +1,32 @@
+<?php
+
+final class PhabricatorOwnersPackageAuditingTransaction
+  extends PhabricatorOwnersPackageTransactionType {
+
+  const TRANSACTIONTYPE = 'owners.auditing';
+
+  public function generateOldValue($object) {
+    return (int)$object->getAuditingEnabled();
+  }
+
+  public function generateNewValue($object, $value) {
+    return (int)$value;
+  }
+
+  public function applyInternalEffects($object, $value) {
+    $object->setAuditingEnabled($value);
+  }
+
+  public function getTitle() {
+    if ($this->getNewValue()) {
+      return pht(
+        '%s enabled auditing for this package.',
+        $this->renderAuthor());
+    } else {
+      return pht(
+        '%s disabled auditing for this package.',
+        $this->renderAuthor());
+    }
+  }
+
+}

src/applications/owners/xaction/PhabricatorOwnersPackageAutoreviewTransaction.php

@@ -0,0 +1,56 @@
+<?php
+
+final class PhabricatorOwnersPackageAutoreviewTransaction
+  extends PhabricatorOwnersPackageTransactionType {
+
+  const TRANSACTIONTYPE = 'owners.autoreview';
+
+  public function generateOldValue($object) {
+    return $object->getAutoReview();
+  }
+
+  public function validateTransactions($object, array $xactions) {
+    $errors = array();
+
+    $map = PhabricatorOwnersPackage::getAutoreviewOptionsMap();
+    foreach ($xactions as $xaction) {
+      $new = $xaction->getNewValue();
+
+      if (empty($map[$new])) {
+        $valid = array_keys($map);
+
+        $errors[] = $this->newInvalidError(
+          pht(
+            'Autoreview setting "%s" is not valid. '.
+            'Valid settings are: %s.',
+            $new,
+            implode(', ', $valid)),
+          $xaction);
+      }
+    }
+
+    return $errors;
+  }
+
+  public function applyInternalEffects($object, $value) {
+    $object->setAutoReview($value);
+  }
+
+  public function getTitle() {
+    $map = PhabricatorOwnersPackage::getAutoreviewOptionsMap();
+    $map = ipull($map, 'name');
+
+    $old = $this->getOldValue();
+    $new = $this->getNewValue();
+
+    $old = idx($map, $old, $old);
+    $new = idx($map, $new, $new);
+
+    return pht(
+      '%s adjusted autoreview from %s to %s.',
+      $this->renderAuthor(),
+      $this->renderValue($old),
+      $this->renderValue($new));
+  }
+
+}

src/applications/owners/xaction/PhabricatorOwnersPackageDescriptionTransaction.php

@@ -0,0 +1,29 @@
+<?php
+
+final class PhabricatorOwnersPackageDescriptionTransaction
+  extends PhabricatorOwnersPackageTransactionType {
+
+  const TRANSACTIONTYPE = 'owners.description';
+
+  public function generateOldValue($object) {
+    return $object->getDescription();
+  }
+
+  public function applyInternalEffects($object, $value) {
+    $object->setDescription($value);
+  }
+
+  public function getTitle() {
+    return pht(
+      '%s updated the description for this package.',
+      $this->renderAuthor());
+  }
+
+  public function newChangeDetailView() {
+    return id(new PhabricatorApplicationTransactionTextDiffDetailView())
+      ->setViewer($this->getViewer())
+      ->setOldText($this->getOldValue())
+      ->setNewText($this->getNewValue());
+  }
+
+}

src/applications/owners/xaction/PhabricatorOwnersPackageDominionTransaction.php

@@ -0,0 +1,56 @@
+<?php
+
+final class PhabricatorOwnersPackageDominionTransaction
+  extends PhabricatorOwnersPackageTransactionType {
+
+  const TRANSACTIONTYPE = 'owners.dominion';
+
+  public function generateOldValue($object) {
+    return $object->getDominion();
+  }
+
+  public function validateTransactions($object, array $xactions) {
+    $errors = array();
+
+    $map = PhabricatorOwnersPackage::getDominionOptionsMap();
+    foreach ($xactions as $xaction) {
+      $new = $xaction->getNewValue();
+
+      if (empty($map[$new])) {
+        $valid = array_keys($map);
+
+        $errors[] = $this->newInvalidError(
+          pht(
+            'Dominion setting "%s" is not valid. '.
+            'Valid settings are: %s.',
+            $new,
+            implode(', ', $valid)),
+          $xaction);
+      }
+    }
+
+    return $errors;
+  }
+
+  public function applyInternalEffects($object, $value) {
+    $object->setDominion($value);
+  }
+
+  public function getTitle() {
+    $map = PhabricatorOwnersPackage::getDominionOptionsMap();
+    $map = ipull($map, 'short');
+
+    $old = $this->getOldValue();
+    $new = $this->getNewValue();
+
+    $old = idx($map, $old, $old);
+    $new = idx($map, $new, $new);
+
+    return pht(
+      '%s adjusted package dominion rules from %s to %s.',
+      $this->renderAuthor(),
+      $this->renderValue($old),
+      $this->renderValue($new));
+  }
+
+}

src/applications/owners/xaction/PhabricatorOwnersPackageNameTransaction.php

@@ -0,0 +1,52 @@
+<?php
+
+final class PhabricatorOwnersPackageNameTransaction
+  extends PhabricatorOwnersPackageTransactionType {
+
+  const TRANSACTIONTYPE = 'owners.name';
+
+  public function generateOldValue($object) {
+    return $object->getName();
+  }
+
+  public function validateTransactions($object, array $xactions) {
+    $errors = array();
+
+    $missing = $this->isEmptyTextTransaction(
+      $object->getName(),
+      $xactions);
+
+    if ($missing) {
+      $errors[] = $this->newRequiredError(
+        pht('Package name is required.'),
+        nonempty(last($xactions), null));
+    }
+
+    foreach ($xactions as $xaction) {
+      $new = $xaction->getNewValue();
+      if (preg_match('([,!])', $new)) {
+        $errors[] = $this->newInvalidError(
+          pht(
+            'Package names may not contain commas (",") or exclamation '.
+            'marks ("!"). These characters are ambiguous when package '.
+            'names are parsed from the command line.'),
+          $xaction);
+      }
+    }
+
+    return $errors;
+  }
+
+  public function applyInternalEffects($object, $value) {
+    $object->setName($value);
+  }
+
+  public function getTitle() {
+    return pht(
+      '%s renamed this package from %s to %s.',
+      $this->renderAuthor(),
+      $this->renderOldValue(),
+      $this->renderNewValue());
+  }
+
+}

src/applications/owners/xaction/PhabricatorOwnersPackageOwnersTransaction.php

@@ -0,0 +1,76 @@
+<?php
+
+final class PhabricatorOwnersPackageOwnersTransaction
+  extends PhabricatorOwnersPackageTransactionType {
+
+  const TRANSACTIONTYPE = 'owners.owners';
+
+  public function generateOldValue($object) {
+    $phids = mpull($object->getOwners(), 'getUserPHID');
+    $phids = array_values($phids);
+    return $phids;
+  }
+
+  public function generateNewValue($object, $value) {
+    $phids = array_unique($value);
+    $phids = array_values($phids);
+    return $phids;
+  }
+
+  public function applyExternalEffects($object, $value) {
+    $old = $this->generateOldValue($object);
+    $new = $value;
+
+    $owners = $object->getOwners();
+    $owners = mpull($owners, null, 'getUserPHID');
+
+    $rem = array_diff($old, $new);
+    foreach ($rem as $phid) {
+      if (isset($owners[$phid])) {
+        $owners[$phid]->delete();
+        unset($owners[$phid]);
+      }
+    }
+
+    $add = array_diff($new, $old);
+    foreach ($add as $phid) {
+      $owners[$phid] = id(new PhabricatorOwnersOwner())
+        ->setPackageID($object->getID())
+        ->setUserPHID($phid)
+        ->save();
+    }
+
+    // TODO: Attach owners here
+  }
+
+  public function getTitle() {
+    $old = $this->getOldValue();
+    $new = $this->getNewValue();
+
+    $add = array_diff($new, $old);
+    $rem = array_diff($old, $new);
+    if ($add && !$rem) {
+      return pht(
+        '%s added %s owner(s): %s.',
+        $this->renderAuthor(),
+        count($add),
+        $this->renderHandleList($add));
+    } else if ($rem && !$add) {
+      return pht(
+        '%s removed %s owner(s): %s.',
+        $this->renderAuthor(),
+        count($rem),
+        $this->renderHandleList($rem));
+    } else {
+      return pht(
+        '%s changed %s package owner(s), added %s: %s; removed %s: %s.',
+        $this->renderAuthor(),
+        count($add) + count($rem),
+        count($add),
+        $this->renderHandleList($add),
+        count($rem),
+        $this->renderHandleList($rem));
+    }
+  }
+
+}

src/applications/owners/xaction/PhabricatorOwnersPackagePathsTransaction.php

@@ -0,0 +1,189 @@
+<?php
+
+final class PhabricatorOwnersPackagePathsTransaction
+  extends PhabricatorOwnersPackageTransactionType {
+
+  const TRANSACTIONTYPE = 'owners.paths';
+
+  public function generateOldValue($object) {
+    $paths = $object->getPaths();
+    return mpull($paths, 'getRef');
+  }
+
+  public function generateNewValue($object, $value) {
+    $new = $value;
+    foreach ($new as $key => $info) {
+      $new[$key]['excluded'] = (int)idx($info, 'excluded');
+    }
+    return $new;
+  }
+
+  public function validateTransactions($object, array $xactions) {
+    $errors = array();
+
+    if (!$xactions) {
+      return $errors;
+    }
+
+    $old = mpull($object->getPaths(), 'getRef');
+    foreach ($xactions as $xaction) {
+      $new = $xaction->getNewValue();
+
+      // Check that we have a list of paths.
+      if (!is_array($new)) {
+        $errors[] = $this->newInvalidError(
+          pht('Path specification must be a list of paths.'),
+          $xaction);
+        continue;
+      }
+
+      // Check that each item in the list is formatted properly.
+      $type_exception = null;
+      foreach ($new as $key => $value) {
+        try {
+          PhutilTypeSpec::checkMap(
+            $value,
+            array(
+              'repositoryPHID' => 'string',
+              'path' => 'string',
+              'excluded' => 'optional wild',
+            ));
+        } catch (PhutilTypeCheckException $ex) {
+          $errors[] = $this->newInvalidError(
+            pht(
+              'Path specification list contains invalid value '.
+              'in key "%s": %s.',
+              $key,
+              $ex->getMessage()),
+            $xaction);
+          $type_exception = $ex;
+        }
+      }
+
+      if ($type_exception) {
+        continue;
+      }
+
+      // Check that any new paths reference legitimate repositories which
+      // the viewer has permission to see.
+      list($rem, $add) = PhabricatorOwnersPath::getTransactionValueChanges(
+        $old,
+        $new);
+
+      if ($add) {
+        $repository_phids = ipull($add, 'repositoryPHID');
+
+        $repositories = id(new PhabricatorRepositoryQuery())
+          ->setViewer($this->getActor())
+          ->withPHIDs($repository_phids)
+          ->execute();
+        $repositories = mpull($repositories, null, 'getPHID');
+
+        foreach ($add as $ref) {
+          $repository_phid = $ref['repositoryPHID'];
+          if (isset($repositories[$repository_phid])) {
+            continue;
+          }
+
+          $errors[] = $this->newInvalidError(
+            pht(
+              'Path specification list references repository PHID "%s", '.
+              'but that is not a valid, visible repository.',
+              $repository_phid));
+        }
+      }
+    }
+
+    return $errors;
+  }
+
+  public function applyExternalEffects($object, $value) {
+    $old = $this->generateOldValue($object);
+    $new = $value;
+
+    $paths = $object->getPaths();
+
+    $diffs = PhabricatorOwnersPath::getTransactionValueChanges($old, $new);
+    list($rem, $add) = $diffs;
+
+    $set = PhabricatorOwnersPath::getSetFromTransactionValue($rem);
+    foreach ($paths as $path) {
+      $ref = $path->getRef();
+      if (PhabricatorOwnersPath::isRefInSet($ref, $set)) {
+        $path->delete();
+      }
+    }
+
+    foreach ($add as $ref) {
+      $path = PhabricatorOwnersPath::newFromRef($ref)
+        ->setPackageID($object->getID())
+        ->save();
+    }
+  }
+
+  public function getTitle() {
+    // TODO: Flesh this out.
+    return pht(
+      '%s updated paths for this package.',
+      $this->renderAuthor());
+  }
+
+  public function hasChangeDetailView() {
+    return true;
+  }
+
+  public function newChangeDetailView() {
+    $old = $this->getOldValue();
+    $new = $this->getNewValue();
+
+    $diffs = PhabricatorOwnersPath::getTransactionValueChanges($old, $new);
+    list($rem, $add) = $diffs;
+
+    $rows = array();
+    foreach ($rem as $ref) {
+      $rows[] = array(
+        'class' => 'diff-removed',
+        'change' => '-',
+      ) + $ref;
+    }
+
+    foreach ($add as $ref) {
+      $rows[] = array(
+        'class' => 'diff-added',
+        'change' => '+',
+      ) + $ref;
+    }
+
+    $rowc = array();
+    foreach ($rows as $key => $row) {
+      $rowc[] = $row['class'];
+      $rows[$key] = array(
+        $row['change'],
+        $row['excluded'] ? pht('Exclude') : pht('Include'),
+        $this->renderHandle($row['repositoryPHID']),
+        $row['path'],
+      );
+    }
+
+    $table = id(new AphrontTableView($rows))
+      ->setViewer($this->getViewer())
+      ->setRowClasses($rowc)
+      ->setHeaders(
+        array(
+          null,
+          pht('Type'),
+          pht('Repository'),
+          pht('Path'),
+        ))
+      ->setColumnClasses(
+        array(
+          null,
+          null,
+          null,
+          'wide',
+        ));
+
+    return $table;
+  }
+
+}

src/applications/owners/xaction/PhabricatorOwnersPackagePrimaryTransaction.php

@@ -0,0 +1,15 @@
+<?php
+
+/**
+ * @deprecated
+ */
+final class PhabricatorOwnersPackagePrimaryTransaction
+  extends PhabricatorOwnersPackageTransactionType {
+
+  const TRANSACTIONTYPE = 'owners.primary';
+
+  public function shouldHide() {
+    return true;
+  }
+
+}

src/applications/owners/xaction/PhabricatorOwnersPackageStatusTransaction.php

@@ -0,0 +1,29 @@
+<?php
+
+final class PhabricatorOwnersPackageStatusTransaction
+  extends PhabricatorOwnersPackageTransactionType {
+
+  const TRANSACTIONTYPE = 'owners.status';
+
+  public function generateOldValue($object) {
+    return $object->getStatus();
+  }
+
+  public function applyInternalEffects($object, $value) {
+    $object->setStatus($value);
+  }
+
+  public function getTitle() {
+    $new = $this->getNewValue();
+    if ($new == PhabricatorOwnersPackage::STATUS_ACTIVE) {
+      return pht(
+        '%s activated this package.',
+        $this->renderAuthor());
+    } else if ($new == PhabricatorOwnersPackage::STATUS_ARCHIVED) {
+      return pht(
+        '%s archived this package.',
+        $this->renderAuthor());
+    }
+  }
+
+}

src/applications/owners/xaction/PhabricatorOwnersPackageTransactionType.php

@@ -0,0 +1,4 @@
+<?php
+
+abstract class PhabricatorOwnersPackageTransactionType
+  extends PhabricatorModularTransactionType {}