Modularize HarbormasterBuildTransaction

Summary: Ref T13072. Update "HarbormasterBuild" to use modern modular transactions.

Test Plan:
  - Aborted, restarted, paused, and resumed a build.
  - Used `bin/harbormaster restart`.
  - Grepped for use of old "::TYPE_COMMAND" constant, didn't find any hits.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13072

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

src/__phutil_library_map__.php

@@ -1409,6 +1409,7 @@ phutil_register_library_map(array(
     'HarbormasterBuildLogViewController' => 'applications/harbormaster/controller/HarbormasterBuildLogViewController.php',
     'HarbormasterBuildMessage' => 'applications/harbormaster/storage/HarbormasterBuildMessage.php',
     'HarbormasterBuildMessageQuery' => 'applications/harbormaster/query/HarbormasterBuildMessageQuery.php',
+    'HarbormasterBuildMessageTransaction' => 'applications/harbormaster/xaction/build/HarbormasterBuildMessageTransaction.php',
     'HarbormasterBuildPHIDType' => 'applications/harbormaster/phid/HarbormasterBuildPHIDType.php',
     'HarbormasterBuildPlan' => 'applications/harbormaster/storage/configuration/HarbormasterBuildPlan.php',
     'HarbormasterBuildPlanBehavior' => 'applications/harbormaster/plan/HarbormasterBuildPlanBehavior.php',
@@ -1459,6 +1460,7 @@ phutil_register_library_map(array(
     'HarbormasterBuildTransaction' => 'applications/harbormaster/storage/HarbormasterBuildTransaction.php',
     'HarbormasterBuildTransactionEditor' => 'applications/harbormaster/editor/HarbormasterBuildTransactionEditor.php',
     'HarbormasterBuildTransactionQuery' => 'applications/harbormaster/query/HarbormasterBuildTransactionQuery.php',
+    'HarbormasterBuildTransactionType' => 'applications/harbormaster/xaction/build/HarbormasterBuildTransactionType.php',
     'HarbormasterBuildUnitMessage' => 'applications/harbormaster/storage/build/HarbormasterBuildUnitMessage.php',
     'HarbormasterBuildUnitMessageQuery' => 'applications/harbormaster/query/HarbormasterBuildUnitMessageQuery.php',
     'HarbormasterBuildView' => 'applications/harbormaster/view/HarbormasterBuildView.php',
@@ -7628,6 +7630,7 @@ phutil_register_library_map(array(
       'PhabricatorDestructibleInterface',
     ),
     'HarbormasterBuildMessageQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
+    'HarbormasterBuildMessageTransaction' => 'HarbormasterBuildTransactionType',
     'HarbormasterBuildPHIDType' => 'PhabricatorPHIDType',
     'HarbormasterBuildPlan' => array(
       'HarbormasterDAO',
@@ -7698,9 +7701,10 @@ phutil_register_library_map(array(
     'HarbormasterBuildTargetPHIDType' => 'PhabricatorPHIDType',
     'HarbormasterBuildTargetQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
     'HarbormasterBuildTargetSearchEngine' => 'PhabricatorApplicationSearchEngine',
-    'HarbormasterBuildTransaction' => 'PhabricatorApplicationTransaction',
+    'HarbormasterBuildTransaction' => 'PhabricatorModularTransaction',
     'HarbormasterBuildTransactionEditor' => 'PhabricatorApplicationTransactionEditor',
     'HarbormasterBuildTransactionQuery' => 'PhabricatorApplicationTransactionQuery',
+    'HarbormasterBuildTransactionType' => 'PhabricatorModularTransactionType',
     'HarbormasterBuildUnitMessage' => array(
       'HarbormasterDAO',
       'PhabricatorPolicyInterface',

src/applications/harbormaster/editor/HarbormasterBuildTransactionEditor.php

@@ -11,92 +11,4 @@ final class HarbormasterBuildTransactionEditor
     return pht('Harbormaster Builds');
   }
 
-  public function getTransactionTypes() {
-    $types = parent::getTransactionTypes();
-
-    $types[] = HarbormasterBuildTransaction::TYPE_COMMAND;
-
-    return $types;
-  }
-
-  protected function getCustomTransactionOldValue(
-    PhabricatorLiskDAO $object,
-    PhabricatorApplicationTransaction $xaction) {
-
-    switch ($xaction->getTransactionType()) {
-      case HarbormasterBuildTransaction::TYPE_COMMAND:
-        return null;
-    }
-
-    return parent::getCustomTransactionOldValue($object, $xaction);
-  }
-
-  protected function getCustomTransactionNewValue(
-    PhabricatorLiskDAO $object,
-    PhabricatorApplicationTransaction $xaction) {
-
-    switch ($xaction->getTransactionType()) {
-      case HarbormasterBuildTransaction::TYPE_COMMAND:
-        return $xaction->getNewValue();
-    }
-
-    return parent::getCustomTransactionNewValue($object, $xaction);
-  }
-
-  protected function applyCustomInternalTransaction(
-    PhabricatorLiskDAO $object,
-    PhabricatorApplicationTransaction $xaction) {
-
-    switch ($xaction->getTransactionType()) {
-      case HarbormasterBuildTransaction::TYPE_COMMAND:
-        return $this->executeBuildCommand($object, $xaction);
-    }
-
-    return parent::applyCustomInternalTransaction($object, $xaction);
-  }
-
-  private function executeBuildCommand(
-    HarbormasterBuild $build,
-    HarbormasterBuildTransaction $xaction) {
-
-    $actor = $this->getActor();
-    $message_type = $xaction->getNewValue();
-
-    // TODO: Restore logic that tests if the command can issue without causing
-    // anything to lapse into an invalid state. This should not be the same
-    // as the logic which powers the web UI: for example, if an "abort" is
-    // queued we want to disable "Abort" in the web UI, but should obviously
-    // process it here.
-
-    switch ($message_type) {
-      case HarbormasterBuildCommand::COMMAND_ABORT:
-        // TODO: This should move to external effects, perhaps.
-        $build->releaseAllArtifacts($actor);
-        $build->setBuildStatus(HarbormasterBuildStatus::STATUS_ABORTED);
-        break;
-      case HarbormasterBuildCommand::COMMAND_RESTART:
-        $build->restartBuild($actor);
-        $build->setBuildStatus(HarbormasterBuildStatus::STATUS_BUILDING);
-        break;
-      case HarbormasterBuildCommand::COMMAND_RESUME:
-        $build->setBuildStatus(HarbormasterBuildStatus::STATUS_BUILDING);
-        break;
-      case HarbormasterBuildCommand::COMMAND_PAUSE:
-        $build->setBuildStatus(HarbormasterBuildStatus::STATUS_PAUSED);
-        break;
-    }
-  }
-
-  protected function applyCustomExternalTransaction(
-    PhabricatorLiskDAO $object,
-    PhabricatorApplicationTransaction $xaction) {
-
-    switch ($xaction->getTransactionType()) {
-      case HarbormasterBuildTransaction::TYPE_COMMAND:
-        return;
-    }
-
-    return parent::applyCustomExternalTransaction($object, $xaction);
-  }
-
 }

src/applications/harbormaster/engine/HarbormasterBuildEngine.php

@@ -124,13 +124,15 @@ final class HarbormasterBuildEngine extends Phobject {
 
     $xactions = array();
 
+    $message_xaction = HarbormasterBuildMessageTransaction::TRANSACTIONTYPE;
+
     $messages = $build->getUnprocessedMessagesForApply();
     foreach ($messages as $message) {
       $message_type = $message->getType();
 
       $xactions[] = $build->getApplicationTransactionTemplate()
         ->setAuthorPHID($message->getAuthorPHID())
-        ->setTransactionType(HarbormasterBuildTransaction::TYPE_COMMAND)
+        ->setTransactionType($message_xaction)
         ->setNewValue($message_type);
     }
 

src/applications/harbormaster/management/HarbormasterManagementRestartWorkflow.php

@@ -32,10 +32,10 @@ final class HarbormasterManagementRestartWorkflow
 
     if (!$ids && !$active) {
       throw new PhutilArgumentUsageException(
-        pht('Use --id or --active to select builds.'));
+        pht('Use "--id" or "--active" to select builds.'));
     } if ($ids && $active) {
       throw new PhutilArgumentUsageException(
-        pht('Use one of --id or --active to select builds, but not both.'));
+        pht('Use one of "--id" or "--active" to select builds, but not both.'));
     }
 
     $query = id(new HarbormasterBuildQuery())
@@ -48,50 +48,38 @@ final class HarbormasterManagementRestartWorkflow
     }
     $builds = $query->execute();
 
-    $console = PhutilConsole::getConsole();
     $count = count($builds);
     if (!$count) {
-      $console->writeOut("%s\n", pht('No builds to restart.'));
+      $this->logSkip(
+        pht('SKIP'),
+        pht('No builds to restart.'));
       return 0;
     }
+
     $prompt = pht('Restart %s build(s)?', new PhutilNumber($count));
     if (!phutil_console_confirm($prompt)) {
-      $console->writeOut("%s\n", pht('Cancelled.'));
+      throw new ArcanistUserAbortException();
-      return 1;
     }
 
-    $app_phid = id(new PhabricatorHarbormasterApplication())->getPHID();
-    $editor = id(new HarbormasterBuildTransactionEditor())
-      ->setActor($viewer)
-      ->setActingAsPHID($app_phid)
-      ->setContentSource($this->newContentSource());
     foreach ($builds as $build) {
-      $console->writeOut(
+      $this->logInfo(
-        "<bg:blue> %s </bg> %s\n",
         pht('RESTARTING'),
         pht('Build %d: %s', $build->getID(), $build->getName()));
+
       if (!$build->canRestartBuild()) {
-        $console->writeOut(
+        $this->logWarn(
-          "<bg:yellow> %s </bg> %s\n",
           pht('INVALID'),
-          pht('Cannot be restarted.'));
+          pht('Build can not be restarted.'));
         continue;
       }
-      $xactions = array();
+
-      $xactions[] = id(new HarbormasterBuildTransaction())
+      $build->sendMessage(
-        ->setTransactionType(HarbormasterBuildTransaction::TYPE_COMMAND)
+        $viewer,
-        ->setNewValue(HarbormasterBuildCommand::COMMAND_RESTART);
+        HarbormasterBuildCommand::COMMAND_RESTART);
-      try {
+
-        $editor->applyTransactions($build, $xactions);
+      $this->logOkay(
-      } catch (Exception $e) {
+        pht('QUEUED'),
-        $message = phutil_console_wrap($e->getMessage(), 2);
+        pht('Sent a restart message to build.'));
-        $console->writeOut(
-          "<bg:red> %s </bg>\n%s\n",
-          pht('FAILED'),
-          $message);
-        continue;
-      }
-      $console->writeOut("<bg:green> %s </bg>\n", pht('SUCCESS'));
     }
 
     return 0;

src/applications/harbormaster/storage/HarbormasterBuildTransaction.php

@@ -1,9 +1,7 @@
 <?php
 
 final class HarbormasterBuildTransaction
-  extends PhabricatorApplicationTransaction {
+  extends PhabricatorModularTransaction {
-
-  const TYPE_COMMAND = 'harbormaster:build:command';
 
   public function getApplicationName() {
     return 'harbormaster';
@@ -13,73 +11,8 @@ final class HarbormasterBuildTransaction
     return HarbormasterBuildPHIDType::TYPECONST;
   }
 
-  public function getTitle() {
+  public function getBaseTransactionClass() {
-    $author_phid = $this->getAuthorPHID();
+    return 'HarbormasterBuildTransactionType';
-
-    $old = $this->getOldValue();
-    $new = $this->getNewValue();
-
-    switch ($this->getTransactionType()) {
-      case self::TYPE_COMMAND:
-        switch ($new) {
-          case HarbormasterBuildCommand::COMMAND_RESTART:
-            return pht(
-              '%s restarted this build.',
-              $this->renderHandleLink($author_phid));
-          case HarbormasterBuildCommand::COMMAND_ABORT:
-            return pht(
-              '%s aborted this build.',
-              $this->renderHandleLink($author_phid));
-          case HarbormasterBuildCommand::COMMAND_RESUME:
-            return pht(
-              '%s resumed this build.',
-              $this->renderHandleLink($author_phid));
-          case HarbormasterBuildCommand::COMMAND_PAUSE:
-            return pht(
-              '%s paused this build.',
-              $this->renderHandleLink($author_phid));
-        }
-    }
-    return parent::getTitle();
   }
 
-  public function getIcon() {
-    $author_phid = $this->getAuthorPHID();
-
-    $old = $this->getOldValue();
-    $new = $this->getNewValue();
-
-    switch ($this->getTransactionType()) {
-      case self::TYPE_COMMAND:
-        switch ($new) {
-          case HarbormasterBuildCommand::COMMAND_RESTART:
-            return 'fa-backward';
-          case HarbormasterBuildCommand::COMMAND_RESUME:
-            return 'fa-play';
-          case HarbormasterBuildCommand::COMMAND_PAUSE:
-            return 'fa-pause';
-          case HarbormasterBuildCommand::COMMAND_ABORT:
-            return 'fa-exclamation-triangle';
-        }
-    }
-
-    return parent::getIcon();
-  }
-
-  public function getColor() {
-    $author_phid = $this->getAuthorPHID();
-
-    $old = $this->getOldValue();
-    $new = $this->getNewValue();
-
-    switch ($this->getTransactionType()) {
-      case self::TYPE_COMMAND:
-        switch ($new) {
-          case HarbormasterBuildCommand::COMMAND_PAUSE:
-          case HarbormasterBuildCommand::COMMAND_ABORT:
-            return 'red';
-        }
-    }
-    return parent::getColor();
-  }
 }

src/applications/harbormaster/xaction/build/HarbormasterBuildMessageTransaction.php

@@ -0,0 +1,128 @@
+<?php
+
+final class HarbormasterBuildMessageTransaction
+  extends HarbormasterBuildTransactionType {
+
+  const TRANSACTIONTYPE = 'harbormaster:build:command';
+
+  public function generateOldValue($object) {
+    return null;
+  }
+
+  public function getTitle() {
+    $new = $this->getNewValue();
+
+    switch ($new) {
+      case HarbormasterBuildCommand::COMMAND_RESTART:
+        return pht(
+          '%s restarted this build.',
+          $this->renderAuthor());
+      case HarbormasterBuildCommand::COMMAND_ABORT:
+        return pht(
+          '%s aborted this build.',
+          $this->renderAuthor());
+      case HarbormasterBuildCommand::COMMAND_RESUME:
+        return pht(
+          '%s resumed this build.',
+          $this->renderAuthor());
+      case HarbormasterBuildCommand::COMMAND_PAUSE:
+        return pht(
+          '%s paused this build.',
+          $this->renderAuthor());
+    }
+
+    return pht(
+      '%s issued an unknown command ("%s") to this build.',
+      $this->renderAuthor(),
+      $this->renderValue($new));
+  }
+
+  public function getIcon() {
+    $new = $this->getNewValue();
+
+    switch ($new) {
+      case HarbormasterBuildCommand::COMMAND_RESTART:
+        return 'fa-backward';
+      case HarbormasterBuildCommand::COMMAND_RESUME:
+        return 'fa-play';
+      case HarbormasterBuildCommand::COMMAND_PAUSE:
+        return 'fa-pause';
+      case HarbormasterBuildCommand::COMMAND_ABORT:
+        return 'fa-exclamation-triangle';
+      default:
+        return 'fa-question';
+    }
+  }
+
+  public function getColor() {
+    $new = $this->getNewValue();
+
+    switch ($new) {
+      case HarbormasterBuildCommand::COMMAND_PAUSE:
+      case HarbormasterBuildCommand::COMMAND_ABORT:
+        return 'red';
+    }
+
+    return parent::getColor();
+  }
+
+  public function getTransactionTypeForConduit($xaction) {
+    return 'message';
+  }
+
+  public function getFieldValuesForConduit($xaction, $data) {
+    return array(
+      'type' => $xaction->getNewValue(),
+    );
+  }
+
+  public function validateTransactions($object, array $xactions) {
+    $errors = array();
+
+    // TODO: Restore logic that tests if the command can issue without causing
+    // anything to lapse into an invalid state. This should not be the same
+    // as the logic which powers the web UI: for example, if an "abort" is
+    // queued we want to disable "Abort" in the web UI, but should obviously
+    // process it here.
+
+    return $errors;
+  }
+
+  public function applyInternalEffects($object, $value) {
+    $actor = $this->getActor();
+    $build = $object;
+
+    $new = $this->getNewValue();
+
+    switch ($new) {
+      case HarbormasterBuildCommand::COMMAND_ABORT:
+        $build->setBuildStatus(HarbormasterBuildStatus::STATUS_ABORTED);
+        break;
+      case HarbormasterBuildCommand::COMMAND_RESTART:
+        $build->restartBuild($actor);
+        $build->setBuildStatus(HarbormasterBuildStatus::STATUS_BUILDING);
+        break;
+      case HarbormasterBuildCommand::COMMAND_RESUME:
+        $build->setBuildStatus(HarbormasterBuildStatus::STATUS_BUILDING);
+        break;
+      case HarbormasterBuildCommand::COMMAND_PAUSE:
+        $build->setBuildStatus(HarbormasterBuildStatus::STATUS_PAUSED);
+        break;
+    }
+  }
+
+  public function applyExternalEffects($object, $value) {
+    $actor = $this->getActor();
+    $build = $object;
+
+    $new = $this->getNewValue();
+
+    switch ($new) {
+      case HarbormasterBuildCommand::COMMAND_ABORT:
+        $build->releaseAllArtifacts($actor);
+        break;
+    }
+  }
+
+
+}

src/applications/harbormaster/xaction/build/HarbormasterBuildTransactionType.php

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