Allow "Assign task" Herald Action in Maniphest to accept "None" to unassign

Summary:
Fixes T9206. This was also blocked on tokenizers being weird.

Also clean up some rendering stuff from the earlier changes.

Test Plan:
  - Added an "unassign" rule by typing "None", per instructions in the placeholder text.
  - Ran the rule.
  - Task got unassigned.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9206

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

src/applications/herald/action/HeraldAction.php

@@ -101,7 +101,9 @@ abstract class HeraldAction extends Phobject {
           return array();
         }
 
-        return $datasource->getWireTokens($target);
+        return $datasource
+          ->setViewer($viewer)
+          ->getWireTokens($target);
     }
 
     return $target;
@@ -348,10 +350,14 @@ abstract class HeraldAction extends Phobject {
         return pht(
           'This action specifies no targets.');
       case self::DO_STANDARD_NO_EFFECT:
-        return pht(
-          'This action has no effect on %s target(s): %s.',
-          phutil_count($data),
-          $this->renderHandleList($data));
+        if ($data && is_array($data)) {
+          return pht(
+            'This action has no effect on %s target(s): %s.',
+            phutil_count($data),
+            $this->renderHandleList($data));
+        } else {
+          return pht('This action has no effect.');
+        }
       case self::DO_STANDARD_INVALID:
         return pht(
           '%s target(s) are invalid or of the wrong type: %s.',

src/applications/maniphest/herald/ManiphestTaskAssignHeraldAction.php

@@ -23,12 +23,21 @@ abstract class ManiphestTaskAssignHeraldAction
       PhabricatorPeopleUserPHIDType::TYPECONST,
     );
 
-    $targets = $this->loadStandardTargets($phids, $allowed_types, $current);
-    if (!$targets) {
-      return;
-    }
+    if (head($phids) == PhabricatorPeopleNoOwnerDatasource::FUNCTION_TOKEN) {
+      $phid = null;
 
-    $phid = head_key($targets);
+      if ($object->getOwnerPHID() == null) {
+        $this->logEffect(self::DO_STANDARD_NO_EFFECT);
+        return;
+      }
+    } else {
+      $targets = $this->loadStandardTargets($phids, $allowed_types, $current);
+      if (!$targets) {
+        return;
+      }
+
+      $phid = head_key($targets);
+    }
 
     $xaction = $adapter->newTransaction()
       ->setTransactionType(ManiphestTransaction::TYPE_OWNER)
@@ -52,9 +61,13 @@ abstract class ManiphestTaskAssignHeraldAction
   protected function renderActionEffectDescription($type, $data) {
     switch ($type) {
       case self::DO_ASSIGN:
-        return pht(
-          'Assigned task to: %s.',
-          $this->renderHandleList($data));
+        if (head($data) === null) {
+          return pht('Unassigned task.');
+        } else {
+          return pht(
+            'Assigned task to: %s.',
+            $this->renderHandleList($data));
+        }
     }
   }
 

src/applications/maniphest/herald/ManiphestTaskAssignOtherHeraldAction.php

@@ -24,11 +24,15 @@ final class ManiphestTaskAssignOtherHeraldAction
   protected function getDatasource() {
     // TODO: Eventually, it would be nice to get "limit = 1" exported from here
     // up to the UI.
-    return new PhabricatorPeopleDatasource();
+    return new ManiphestAssigneeDatasource();
   }
 
   public function renderActionDescription($value) {
-    return pht('Assign task to: %s.', $this->renderHandleList($value));
+    if (head($value) === PhabricatorPeopleNoOwnerDatasource::FUNCTION_TOKEN) {
+      return pht('Unassign task.');
+    } else {
+      return pht('Assign task to: %s.', $this->renderHandleList($value));
+    }
   }
 
 }