Modularize Maniphest Herald "Assign Task" action

Summary: Ref T8726. No surprises here.

Test Plan:
  - Created rules using this action.
  - Applied migration.
  - Verified rules still work.

{F659324}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: joshuaspence, epriestley

Maniphest Tasks: T8726

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

resources/sql/autopatches/20150724.herald.4.sql

@@ -0,0 +1,13 @@
+UPDATE {$NAMESPACE}_herald.herald_actionrecord a
+  JOIN {$NAMESPACE}_herald.herald_rule r
+  ON a.ruleID = r.id
+  SET a.action = 'maniphest.assign.other'
+  WHERE r.ruleType != 'personal'
+  AND a.action = 'assigntask';
+
+UPDATE {$NAMESPACE}_herald.herald_actionrecord a
+  JOIN {$NAMESPACE}_herald.herald_rule r
+  ON a.ruleID = r.id
+  SET a.action = 'maniphest.assign.self'
+  WHERE r.ruleType = 'personal'
+  AND a.action = 'assigntask';

src/__phutil_library_map__.php

@@ -1013,6 +1013,7 @@ phutil_register_library_map(array(
     'HeraldAdapter' => 'applications/herald/adapter/HeraldAdapter.php',
     'HeraldAlwaysField' => 'applications/herald/field/HeraldAlwaysField.php',
     'HeraldAnotherRuleField' => 'applications/herald/field/HeraldAnotherRuleField.php',
+    'HeraldApplicationActionGroup' => 'applications/herald/action/HeraldApplicationActionGroup.php',
     'HeraldApplyTranscript' => 'applications/herald/storage/transcript/HeraldApplyTranscript.php',
     'HeraldBasicFieldGroup' => 'applications/herald/field/HeraldBasicFieldGroup.php',
     'HeraldCommitAdapter' => 'applications/diffusion/herald/HeraldCommitAdapter.php',
@@ -1186,6 +1187,9 @@ phutil_register_library_map(array(
     'ManiphestStatusEmailCommand' => 'applications/maniphest/command/ManiphestStatusEmailCommand.php',
     'ManiphestSubpriorityController' => 'applications/maniphest/controller/ManiphestSubpriorityController.php',
     'ManiphestTask' => 'applications/maniphest/storage/ManiphestTask.php',
+    'ManiphestTaskAssignHeraldAction' => 'applications/maniphest/herald/ManiphestTaskAssignHeraldAction.php',
+    'ManiphestTaskAssignOtherHeraldAction' => 'applications/maniphest/herald/ManiphestTaskAssignOtherHeraldAction.php',
+    'ManiphestTaskAssignSelfHeraldAction' => 'applications/maniphest/herald/ManiphestTaskAssignSelfHeraldAction.php',
     'ManiphestTaskAssigneeHeraldField' => 'applications/maniphest/herald/ManiphestTaskAssigneeHeraldField.php',
     'ManiphestTaskAuthorHeraldField' => 'applications/maniphest/herald/ManiphestTaskAuthorHeraldField.php',
     'ManiphestTaskAuthorPolicyRule' => 'applications/maniphest/policyrule/ManiphestTaskAuthorPolicyRule.php',
@@ -4710,6 +4714,7 @@ phutil_register_library_map(array(
     'HeraldAdapter' => 'Phobject',
     'HeraldAlwaysField' => 'HeraldField',
     'HeraldAnotherRuleField' => 'HeraldField',
+    'HeraldApplicationActionGroup' => 'HeraldActionGroup',
     'HeraldApplyTranscript' => 'Phobject',
     'HeraldBasicFieldGroup' => 'HeraldFieldGroup',
     'HeraldCommitAdapter' => 'HeraldAdapter',
@@ -4922,6 +4927,9 @@ phutil_register_library_map(array(
       'PhabricatorProjectInterface',
       'PhabricatorSpacesInterface',
     ),
+    'ManiphestTaskAssignHeraldAction' => 'HeraldAction',
+    'ManiphestTaskAssignOtherHeraldAction' => 'ManiphestTaskAssignHeraldAction',
+    'ManiphestTaskAssignSelfHeraldAction' => 'ManiphestTaskAssignHeraldAction',
     'ManiphestTaskAssigneeHeraldField' => 'ManiphestTaskHeraldField',
     'ManiphestTaskAuthorHeraldField' => 'ManiphestTaskHeraldField',
     'ManiphestTaskAuthorPolicyRule' => 'PhabricatorPolicyRule',

src/applications/herald/action/HeraldApplicationActionGroup.php

@@ -0,0 +1,15 @@
+<?php
+
+final class HeraldApplicationActionGroup extends HeraldActionGroup {
+
+  const ACTIONGROUPKEY = 'application';
+
+  public function getGroupLabel() {
+    return pht('Application');
+  }
+
+  protected function getGroupOrder() {
+    return 1500;
+  }
+
+}

src/applications/herald/adapter/HeraldAdapter.php

@@ -27,7 +27,6 @@ abstract class HeraldAdapter extends Phobject {
   const CONDITION_IS_FALSE        = 'false';
 
   const ACTION_AUDIT        = 'audit';
-  const ACTION_ASSIGN_TASK  = 'assigntask';
   const ACTION_ADD_REVIEWERS = 'addreviewers';
   const ACTION_ADD_BLOCKING_REVIEWERS = 'addblockingreviewers';
   const ACTION_APPLY_BUILD_PLANS = 'applybuildplans';
@@ -718,7 +717,6 @@ abstract class HeraldAdapter extends Phobject {
       case HeraldRuleTypeConfig::RULE_TYPE_OBJECT:
         $standard = array(
           self::ACTION_AUDIT        => pht('Trigger an Audit by'),
-          self::ACTION_ASSIGN_TASK  => pht('Assign task to'),
           self::ACTION_ADD_REVIEWERS => pht('Add reviewers'),
           self::ACTION_ADD_BLOCKING_REVIEWERS => pht('Add blocking reviewers'),
           self::ACTION_APPLY_BUILD_PLANS => pht('Run build plans'),
@@ -729,7 +727,6 @@ abstract class HeraldAdapter extends Phobject {
       case HeraldRuleTypeConfig::RULE_TYPE_PERSONAL:
         $standard = array(
           self::ACTION_AUDIT        => pht('Trigger an Audit by me'),
-          self::ACTION_ASSIGN_TASK  => pht('Assign task to me'),
           self::ACTION_ADD_REVIEWERS => pht('Add me as a reviewer'),
           self::ACTION_ADD_BLOCKING_REVIEWERS =>
             pht('Add me as a blocking reviewer'),
@@ -772,7 +769,6 @@ abstract class HeraldAdapter extends Phobject {
     if ($rule_type == HeraldRuleTypeConfig::RULE_TYPE_PERSONAL) {
       switch ($action->getAction()) {
         case self::ACTION_AUDIT:
-        case self::ACTION_ASSIGN_TASK:
         case self::ACTION_ADD_REVIEWERS:
         case self::ACTION_ADD_BLOCKING_REVIEWERS:
           // For personal rules, force these actions to target the rule owner.
@@ -812,16 +808,12 @@ abstract class HeraldAdapter extends Phobject {
     if ($is_personal) {
       switch ($action) {
         case self::ACTION_AUDIT:
-        case self::ACTION_ASSIGN_TASK:
         case self::ACTION_ADD_REVIEWERS:
         case self::ACTION_ADD_BLOCKING_REVIEWERS:
           return new HeraldEmptyFieldValue();
       }
     } else {
       switch ($action) {
-        case self::ACTION_ASSIGN_TASK:
-          return $this->buildTokenizerFieldValue(
-            new PhabricatorPeopleDatasource());
         case self::ACTION_AUDIT:
         case self::ACTION_ADD_REVIEWERS:
         case self::ACTION_ADD_BLOCKING_REVIEWERS:

src/applications/herald/controller/HeraldTranscriptController.php

@@ -233,7 +233,9 @@ final class HeraldTranscriptController extends HeraldController {
     $rule_list = id(new PHUIObjectItemListView())
       ->setNoDataString(pht('No Herald rules applied to this object.'));
 
-    foreach ($xscript->getRuleTranscripts() as $rule_xscript) {
+    $rule_xscripts = $xscript->getRuleTranscripts();
+    $rule_xscripts = msort($rule_xscripts, 'getRuleID');
+    foreach ($rule_xscripts as $rule_xscript) {
       $rule_id = $rule_xscript->getRuleID();
 
       $rule_item = id(new PHUIObjectItemView())

src/applications/maniphest/editor/ManiphestTransactionEditor.php

@@ -506,23 +506,6 @@ final class ManiphestTransactionEditor
       ->setTask($object);
   }
 
-  protected function didApplyHeraldRules(
-    PhabricatorLiskDAO $object,
-    HeraldAdapter $adapter,
-    HeraldTranscript $transcript) {
-
-    $xactions = array();
-
-    $assign_phid = $adapter->getAssignPHID();
-    if ($assign_phid) {
-      $xactions[] = id(new ManiphestTransaction())
-        ->setTransactionType(ManiphestTransaction::TYPE_OWNER)
-        ->setNewValue($assign_phid);
-    }
-
-    return $xactions;
-  }
-
   protected function requireCapabilities(
     PhabricatorLiskDAO $object,
     PhabricatorApplicationTransaction $xaction) {

src/applications/maniphest/herald/HeraldManiphestTaskAdapter.php

@@ -3,7 +3,6 @@
 final class HeraldManiphestTaskAdapter extends HeraldAdapter {
 
   private $task;
-  private $assignPHID;
 
   protected function newObject() {
     return new ManiphestTask();
@@ -55,61 +54,12 @@ final class HeraldManiphestTaskAdapter extends HeraldAdapter {
     return $this->task;
   }
 
-  public function setAssignPHID($assign_phid) {
-    $this->assignPHID = $assign_phid;
-    return $this;
-  }
-  public function getAssignPHID() {
-    return $this->assignPHID;
-  }
-
   public function getAdapterContentName() {
     return pht('Maniphest Tasks');
   }
 
-  public function getActions($rule_type) {
-    switch ($rule_type) {
-      case HeraldRuleTypeConfig::RULE_TYPE_GLOBAL:
-        return array_merge(
-          array(
-            self::ACTION_ASSIGN_TASK,
-          ),
-          parent::getActions($rule_type));
-      case HeraldRuleTypeConfig::RULE_TYPE_PERSONAL:
-        return array_merge(
-          array(
-            self::ACTION_ASSIGN_TASK,
-          ),
-          parent::getActions($rule_type));
-    }
-  }
-
   public function getHeraldName() {
     return 'T'.$this->getTask()->getID();
   }
 
-  public function applyHeraldEffects(array $effects) {
-    assert_instances_of($effects, 'HeraldEffect');
-
-    $result = array();
-    foreach ($effects as $effect) {
-      $action = $effect->getAction();
-      switch ($action) {
-        case self::ACTION_ASSIGN_TASK:
-          $target_array = $effect->getTarget();
-          $assign_phid = reset($target_array);
-          $this->setAssignPHID($assign_phid);
-          $result[] = new HeraldApplyTranscript(
-            $effect,
-            true,
-            pht('Assigned task.'));
-          break;
-        default:
-          $result[] = $this->applyStandardEffect($effect);
-          break;
-      }
-    }
-    return $result;
-  }
-
 }

src/applications/maniphest/herald/ManiphestTaskAssignHeraldAction.php

@@ -0,0 +1,116 @@
+<?php
+
+abstract class ManiphestTaskAssignHeraldAction
+  extends HeraldAction {
+
+  const DO_EMPTY = 'do.send';
+  const DO_ALREADY = 'do.already';
+  const DO_INVALID = 'do.invalid';
+  const DO_PERMISSION = 'do.permission';
+  const DO_ASSIGN = 'do.assign';
+
+  public function supportsObject($object) {
+    return ($object instanceof ManiphestTask);
+  }
+
+  public function getActionGroupKey() {
+    return HeraldApplicationActionGroup::ACTIONGROUPKEY;
+  }
+
+  protected function applyAssign(array $phids) {
+    $phid = head($phids);
+
+    if (!$phid) {
+      $this->logEffect(self::DO_EMPTY);
+      return;
+    }
+
+    $adapter = $this->getAdapter();
+    $object = $adapter->getObject();
+
+    if ($object->getOwnerPHID() == $phid) {
+      $this->logEffect(self::DO_ALREADY, array($phid));
+      return;
+    }
+
+    $user = id(new PhabricatorPeopleQuery())
+      ->setViewer(PhabricatorUser::getOmnipotentUser())
+      ->withPHIDs(array($phid))
+      ->executeOne();
+    if (!$user) {
+      $this->logEffect(self::DO_INVALID, array($phid));
+      return;
+    }
+
+    $can_view = PhabricatorPolicyFilter::hasCapability(
+      $user,
+      $object,
+      PhabricatorPolicyCapability::CAN_VIEW);
+    if (!$can_view) {
+      $this->logEffect(self::DO_PERMISSION, array($phid));
+      return;
+    }
+
+    $xaction = $adapter->newTransaction()
+      ->setTransactionType(ManiphestTransaction::TYPE_OWNER)
+      ->setNewValue($phid);
+
+    $adapter->queueTransaction($xaction);
+
+    $this->logEffect(self::DO_ASSIGN, array($phid));
+  }
+
+  protected function getActionEffectMap() {
+    return array(
+      self::DO_EMPTY => array(
+        'icon' => 'fa-ban',
+        'color' => 'grey',
+        'name' => pht('Empty Action'),
+      ),
+      self::DO_ALREADY => array(
+        'icon' => 'fa-user',
+        'color' => 'grey',
+        'name' => pht('Already Assigned'),
+      ),
+      self::DO_INVALID => array(
+        'icon' => 'fa-ban',
+        'color' => 'red',
+        'name' => pht('Invalid Owner'),
+      ),
+      self::DO_PERMISSION => array(
+        'icon' => 'fa-ban',
+        'color' => 'red',
+        'name' => pht('No Permission'),
+      ),
+      self::DO_ASSIGN => array(
+        'icon' => 'fa-user',
+        'color' => 'green',
+        'name' => pht('Assigned Task'),
+      ),
+    );
+  }
+
+  public function renderActionEffectDescription($type, $data) {
+    switch ($type) {
+      case self::DO_EMPTY:
+        return pht('Action lists no user to assign.');
+      case self::DO_ALREADY:
+        return pht(
+          'User is already task owner: %s.',
+          $this->renderHandleList($data));
+      case self::DO_INVALID:
+        return pht(
+          'User is invalid: %s.',
+          $this->renderHandleList($data));
+      case self::DO_PERMISSION:
+        return pht(
+          'User does not have permission to see task: %s.',
+          $this->renderHandleList($data));
+      case self::DO_ASSIGN:
+        return pht(
+          'Assigned task to: %s.',
+          $this->renderHandleList($data));
+    }
+  }
+
+}

src/applications/maniphest/herald/ManiphestTaskAssignOtherHeraldAction.php

@@ -0,0 +1,34 @@
+<?php
+
+final class ManiphestTaskAssignOtherHeraldAction
+  extends ManiphestTaskAssignHeraldAction {
+
+  const ACTIONCONST = 'maniphest.assign.other';
+
+  public function getHeraldActionName() {
+    return pht('Assign task to');
+  }
+
+  public function supportsRuleType($rule_type) {
+    return ($rule_type != HeraldRuleTypeConfig::RULE_TYPE_PERSONAL);
+  }
+
+  public function applyEffect($object, HeraldEffect $effect) {
+    return $this->applyAssign($effect->getTarget());
+  }
+
+  public function getHeraldActionStandardType() {
+    return self::STANDARD_PHID_LIST;
+  }
+
+  protected function getDatasource() {
+    // TODO: Eventually, it would be nice to get "limit = 1" exported from here
+    // up to the UI.
+    return new PhabricatorPeopleDatasource();
+  }
+
+  public function renderActionDescription($value) {
+    return pht('Assign task to: %s.', $this->renderHandleList($value));
+  }
+
+}

src/applications/maniphest/herald/ManiphestTaskAssignSelfHeraldAction.php

@@ -0,0 +1,29 @@
+<?php
+
+final class ManiphestTaskAssignSelfHeraldAction
+  extends ManiphestTaskAssignHeraldAction {
+
+  const ACTIONCONST = 'maniphest.assign.self';
+
+  public function getHeraldActionName() {
+    return pht('Assign task to me');
+  }
+
+  public function supportsRuleType($rule_type) {
+    return ($rule_type == HeraldRuleTypeConfig::RULE_TYPE_PERSONAL);
+  }
+
+  public function applyEffect($object, HeraldEffect $effect) {
+    $phid = $effect->getRule()->getAuthorPHID();
+    return $this->applyAssign(array($phid));
+  }
+
+  public function getHeraldActionStandardType() {
+    return self::STANDARD_NONE;
+  }
+
+  public function renderActionDescription($value) {
+    return pht('Assign task to rule author.');
+  }
+
+}