Make "Assign / Claim" stacked action work properly in Maniphest

Summary:
Ref T9132. This is kind of a mess because the tokenizer rewrite left rendering tokenizers in Javascript a little rough. This causes bugs like icons not showing up on tokens in the "Policy" dialog, which there's a task for somewhere I think.

I think I've fixed it enough that the beahavior is now correct (i.e., icons show up properly), but some of the code is a bit iffy. I'll eventually clean this up properly, but it's fairly well contained for now.

Test Plan:
  - Reassigned a task.
  - Put a task up for grabs.
  - No reassign on closed tasks.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9132

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

resources/celerity/map.php

@@ -8,7 +8,7 @@
 return array(
   'names' => array(
     'core.pkg.css' => '4d47b0a9',
-    'core.pkg.js' => '47dc9ebb',
+    'core.pkg.js' => '21eccc42',
     'darkconsole.pkg.js' => 'e7393ebb',
     'differential.pkg.css' => '2de124c9',
     'differential.pkg.js' => '6223dd9d',
@@ -456,7 +456,7 @@ return array(
     'rsrc/js/core/KeyboardShortcutManager.js' => 'c1700f6f',
     'rsrc/js/core/MultirowRowManager.js' => 'b5d57730',
     'rsrc/js/core/Notification.js' => 'ccf1cbf8',
-    'rsrc/js/core/Prefab.js' => '6920d200',
+    'rsrc/js/core/Prefab.js' => '2381d07a',
     'rsrc/js/core/ShapedRequest.js' => '7cbe244b',
     'rsrc/js/core/TextAreaUtils.js' => '5c93c52c',
     'rsrc/js/core/Title.js' => 'df5e11d2',
@@ -506,7 +506,7 @@ return array(
     'rsrc/js/phuix/PHUIXActionListView.js' => 'b5c256b8',
     'rsrc/js/phuix/PHUIXActionView.js' => '8cf6d262',
     'rsrc/js/phuix/PHUIXDropdownMenu.js' => 'bd4c8dca',
-    'rsrc/js/phuix/PHUIXFormControl.js' => '7e1dc09e',
+    'rsrc/js/phuix/PHUIXFormControl.js' => '1adf0d30',
     'rsrc/js/phuix/PHUIXIconView.js' => 'bff6884b',
   ),
   'symbols' => array(
@@ -757,7 +757,7 @@ return array(
     'phabricator-notification-menu-css' => 'f31c0bde',
     'phabricator-object-selector-css' => '85ee8ce6',
     'phabricator-phtize' => 'd254d646',
-    'phabricator-prefab' => '6920d200',
+    'phabricator-prefab' => '2381d07a',
     'phabricator-remarkup-css' => 'b1c10368',
     'phabricator-search-results-css' => '7dea472c',
     'phabricator-shaped-request' => '7cbe244b',
@@ -831,7 +831,7 @@ return array(
     'phuix-action-list-view' => 'b5c256b8',
     'phuix-action-view' => '8cf6d262',
     'phuix-dropdown-menu' => 'bd4c8dca',
-    'phuix-form-control-view' => '7e1dc09e',
+    'phuix-form-control-view' => '1adf0d30',
     'phuix-icon-view' => 'bff6884b',
     'policy-css' => '957ea14c',
     'policy-edit-css' => '815c66f7',
@@ -946,6 +946,10 @@ return array(
       'javelin-util',
       'javelin-reactor-node-calmer',
     ),
+    '1adf0d30' => array(
+      'javelin-install',
+      'javelin-dom',
+    ),
     '1ae869f2' => array(
       'javelin-install',
       'javelin-util',
@@ -1003,6 +1007,18 @@ return array(
       'javelin-workflow',
       'javelin-util',
     ),
+    '2381d07a' => array(
+      'javelin-install',
+      'javelin-util',
+      'javelin-dom',
+      'javelin-typeahead',
+      'javelin-tokenizer',
+      'javelin-typeahead-preloaded-source',
+      'javelin-typeahead-ondemand-source',
+      'javelin-dom',
+      'javelin-stratcom',
+      'javelin-util',
+    ),
     '246dc085' => array(
       'javelin-behavior',
       'javelin-stratcom',
@@ -1304,18 +1320,6 @@ return array(
     '6882e80a' => array(
       'javelin-dom',
     ),
-    '6920d200' => array(
-      'javelin-install',
-      'javelin-util',
-      'javelin-dom',
-      'javelin-typeahead',
-      'javelin-tokenizer',
-      'javelin-typeahead-preloaded-source',
-      'javelin-typeahead-ondemand-source',
-      'javelin-dom',
-      'javelin-stratcom',
-      'javelin-util',
-    ),
     '69adf288' => array(
       'javelin-install',
     ),
@@ -1430,10 +1434,6 @@ return array(
       'phuix-action-view',
       'javelin-workflow',
     ),
-    '7e1dc09e' => array(
-      'javelin-install',
-      'javelin-dom',
-    ),
     '7e41274a' => array(
       'javelin-install',
     ),

src/__phutil_library_map__.php

@@ -2087,6 +2087,7 @@ phutil_register_library_map(array(
     'PhabricatorDataNotAttachedException' => 'infrastructure/storage/lisk/PhabricatorDataNotAttachedException.php',
     'PhabricatorDatabaseSetupCheck' => 'applications/config/check/PhabricatorDatabaseSetupCheck.php',
     'PhabricatorDatasourceEditField' => 'applications/transactions/editfield/PhabricatorDatasourceEditField.php',
+    'PhabricatorDatasourceEditType' => 'applications/transactions/edittype/PhabricatorDatasourceEditType.php',
     'PhabricatorDateTimeSettingsPanel' => 'applications/settings/panel/PhabricatorDateTimeSettingsPanel.php',
     'PhabricatorDebugController' => 'applications/system/controller/PhabricatorDebugController.php',
     'PhabricatorDefaultRequestExceptionHandler' => 'aphront/handler/PhabricatorDefaultRequestExceptionHandler.php',
@@ -2590,6 +2591,7 @@ phutil_register_library_map(array(
     'PhabricatorPHIDConstants' => 'applications/phid/PhabricatorPHIDConstants.php',
     'PhabricatorPHIDInterface' => 'applications/phid/interface/PhabricatorPHIDInterface.php',
     'PhabricatorPHIDListEditField' => 'applications/transactions/editfield/PhabricatorPHIDListEditField.php',
+    'PhabricatorPHIDListEditType' => 'applications/transactions/edittype/PhabricatorPHIDListEditType.php',
     'PhabricatorPHIDResolver' => 'applications/phid/resolver/PhabricatorPHIDResolver.php',
     'PhabricatorPHIDType' => 'applications/phid/type/PhabricatorPHIDType.php',
     'PhabricatorPHIDTypeTestCase' => 'applications/phid/type/__tests__/PhabricatorPHIDTypeTestCase.php',
@@ -6202,6 +6204,7 @@ phutil_register_library_map(array(
     'PhabricatorDataNotAttachedException' => 'Exception',
     'PhabricatorDatabaseSetupCheck' => 'PhabricatorSetupCheck',
     'PhabricatorDatasourceEditField' => 'PhabricatorTokenizerEditField',
+    'PhabricatorDatasourceEditType' => 'PhabricatorPHIDListEditType',
     'PhabricatorDateTimeSettingsPanel' => 'PhabricatorSettingsPanel',
     'PhabricatorDebugController' => 'PhabricatorController',
     'PhabricatorDefaultRequestExceptionHandler' => 'PhabricatorRequestExceptionHandler',
@@ -6228,7 +6231,7 @@ phutil_register_library_map(array(
     'PhabricatorEdgeConfig' => 'PhabricatorEdgeConstants',
     'PhabricatorEdgeConstants' => 'Phobject',
     'PhabricatorEdgeCycleException' => 'Exception',
-    'PhabricatorEdgeEditType' => 'PhabricatorEditType',
+    'PhabricatorEdgeEditType' => 'PhabricatorPHIDListEditType',
     'PhabricatorEdgeEditor' => 'Phobject',
     'PhabricatorEdgeGraph' => 'AbstractDirectedGraph',
     'PhabricatorEdgeQuery' => 'PhabricatorQuery',
@@ -6777,6 +6780,7 @@ phutil_register_library_map(array(
     'PhabricatorPHID' => 'Phobject',
     'PhabricatorPHIDConstants' => 'Phobject',
     'PhabricatorPHIDListEditField' => 'PhabricatorEditField',
+    'PhabricatorPHIDListEditType' => 'PhabricatorEditType',
     'PhabricatorPHIDResolver' => 'Phobject',
     'PhabricatorPHIDType' => 'Phobject',
     'PhabricatorPHIDTypeTestCase' => 'PhutilTestCase',

src/applications/maniphest/editor/ManiphestEditEngine.php

@@ -61,19 +61,25 @@ final class ManiphestEditEngine
     $priority_map = ManiphestTaskPriority::getTaskPriorityMap();
 
     // TODO: Restore these or toss them:
-    // - Default owner to viewer.
-    // - Don't show "change owner" for closed tasks.
     // - When closing an unassigned task, assign the closing user.
     // - Make sure implicit CCs on actions are working reasonably.
 
     if ($object->isClosed()) {
       $priority_label = null;
+      $owner_label = null;
       $default_status = ManiphestTaskStatus::getDefaultStatus();
     } else {
       $priority_label = pht('Change Priority');
+      $owner_label = pht('Assign / Claim');
       $default_status = ManiphestTaskStatus::getDefaultClosedStatus();
     }
 
+    if ($object->getOwnerPHID()) {
+      $owner_value = array($object->getOwnerPHID());
+    } else {
+      $owner_value = array($this->getViewer()->getPHID());
+    }
+
     return array(
       id(new PhabricatorTextEditField())
         ->setKey('title')
@@ -97,7 +103,9 @@ final class ManiphestEditEngine
         ->setLabel(pht('Assigned To'))
         ->setDescription(pht('User who is responsible for the task.'))
         ->setTransactionType(ManiphestTransaction::TYPE_OWNER)
-        ->setSingleValue($object->getOwnerPHID()),
+        ->setSingleValue($object->getOwnerPHID())
+        ->setCommentActionLabel($owner_label)
+        ->setCommentActionDefaultValue($owner_value),
       id(new PhabricatorSelectEditField())
         ->setKey('priority')
         ->setLabel(pht('Priority'))

src/applications/transactions/editengine/PhabricatorEditEngine.php

@@ -1049,7 +1049,10 @@ abstract class PhabricatorEditEngine
       foreach ($fields as $field) {
         $types = $field->getCommentEditTypes();
         foreach ($types as $type) {
-          $type_map[$type->getEditType()] = $type;
+          $type_map[$type->getEditType()] = array(
+            'type' => $type,
+            'field' => $field,
+          );
         }
       }
 
@@ -1060,15 +1063,20 @@ abstract class PhabricatorEditEngine
           continue;
         }
 
-        $edit_type = idx($type_map, $type);
-        if (!$edit_type) {
+        $spec = idx($type_map, $type);
+        if (!$spec) {
           continue;
         }
 
+        $edit_type = $spec['type'];
+        $field = $spec['field'];
+
+        $field->readValueFromComment($action);
+
         $type_xactions = $edit_type->generateTransactions(
           $template,
           array(
-            'value' => idx($action, 'value'),
+            'value' => $field->getValueForTransaction(),
           ));
         foreach ($type_xactions as $type_xaction) {
           $xactions[] = $type_xaction;

src/applications/transactions/editfield/PhabricatorEditField.php

@@ -182,7 +182,6 @@ abstract class PhabricatorEditField extends Phobject {
     return $this->commentActionLabel;
   }
 
-
   protected function newControl() {
     throw new PhutilMethodNotImplementedException();
   }
@@ -322,6 +321,15 @@ abstract class PhabricatorEditField extends Phobject {
     return $this;
   }
 
+  public function readValueFromComment($action) {
+    $this->value = $this->getValueFromComment(idx($action, 'value'));
+    return $this;
+  }
+
+  protected function getValueFromComment($value) {
+    return $value;
+  }
+
   public function getAllReadValueFromRequestKeys() {
     $keys = array();
 

src/applications/transactions/editfield/PhabricatorPHIDListEditField.php

@@ -92,12 +92,8 @@ abstract class PhabricatorPHIDListEditField
       return new PhabricatorEdgeEditType();
     }
 
-    $type = parent::newEditType();
-
-    if ($this->getIsSingleValue()) {
-      $type->setValueType('phid');
-    }
-
+    $type = new PhabricatorDatasourceEditType();
+    $type->setIsSingleValue($this->getIsSingleValue());
     return $type;
   }
 

src/applications/transactions/editfield/PhabricatorTokenizerEditField.php

@@ -3,8 +3,19 @@
 abstract class PhabricatorTokenizerEditField
   extends PhabricatorPHIDListEditField {
 
+  private $commentActionDefaultValue;
+
   abstract protected function newDatasource();
 
+  public function setCommentActionDefaultValue(array $default) {
+    $this->commentActionDefaultValue = $default;
+    return $this;
+  }
+
+  public function getCommentActionDefaultValue() {
+    return $this->commentActionDefaultValue;
+  }
+
   protected function newControl() {
     $control = id(new AphrontFormTokenizerControl())
       ->setDatasource($this->newDatasource());
@@ -28,18 +39,17 @@ abstract class PhabricatorTokenizerEditField
   protected function newEditType() {
     $type = parent::newEditType();
 
-    if ($this->getUseEdgeTransactions()) {
     $datasource = $this->newDatasource()
       ->setViewer($this->getViewer());
     $type->setDatasource($datasource);
-    }
 
     return $type;
   }
 
   public function getCommentEditTypes() {
-    if (!$this->getUseEdgeTransactions()) {
-      return parent::getCommentEditTypes();
+    $label = $this->getCommentActionLabel();
+    if ($label === null) {
+      return array();
     }
 
     $transaction_type = $this->getTransactionType();
@@ -47,11 +57,7 @@ abstract class PhabricatorTokenizerEditField
       return array();
     }
 
-    $label = $this->getCommentActionLabel();
-    if ($label === null) {
-      return array();
-    }
-
+    if ($this->getUseEdgeTransactions()) {
       $type_key = $this->getEditTypeKey();
       $base = $this->getEditType();
 
@@ -63,4 +69,15 @@ abstract class PhabricatorTokenizerEditField
       return array($add);
     }
 
+    $edit = $this->getEditType()
+      ->setLabel($label);
+
+    $default = $this->getCommentActionDefaultValue();
+    if ($default) {
+      $edit->setDefaultValue($default);
+    }
+
+    return array($edit);
+  }
+
 }

src/applications/transactions/edittype/PhabricatorDatasourceEditType.php

@@ -0,0 +1,22 @@
+<?php
+
+final class PhabricatorDatasourceEditType
+  extends PhabricatorPHIDListEditType {
+
+  public function generateTransactions(
+    PhabricatorApplicationTransaction $template,
+    array $spec) {
+
+    $value = idx($spec, 'value');
+
+    $xaction = $this->newTransaction($template)
+      ->setNewValue($value);
+
+    return array($xaction);
+  }
+
+  public function getValueDescription() {
+    return '?';
+  }
+
+}

src/applications/transactions/edittype/PhabricatorEdgeEditType.php

@@ -1,10 +1,10 @@
 <?php
 
-final class PhabricatorEdgeEditType extends PhabricatorEditType {
+final class PhabricatorEdgeEditType
+  extends PhabricatorPHIDListEditType {
 
   private $edgeOperation;
   private $valueDescription;
-  private $datasource;
 
   public function setEdgeOperation($edge_operation) {
     $this->edgeOperation = $edge_operation;
@@ -15,19 +15,6 @@ final class PhabricatorEdgeEditType extends PhabricatorEditType {
     return $this->edgeOperation;
   }
 
-  public function setDatasource($datasource) {
-    $this->datasource = $datasource;
-    return $this;
-  }
-
-  public function getDatasource() {
-    return $this->datasource;
-  }
-
-  public function getValueType() {
-    return 'list<phid>';
-  }
-
   public function generateTransactions(
     PhabricatorApplicationTransaction $template,
     array $spec) {
@@ -56,33 +43,4 @@ final class PhabricatorEdgeEditType extends PhabricatorEditType {
     return $this->valueDescription;
   }
 
-  public function getPHUIXControlType() {
-    $datasource = $this->getDatasource();
-
-    if (!$datasource) {
-      return null;
-    }
-
-    return 'tokenizer';
-  }
-
-  public function getPHUIXControlSpecification() {
-    $datasource = $this->getDatasource();
-
-    if (!$datasource) {
-      return null;
-    }
-
-    $template = new AphrontTokenizerTemplateView();
-
-    return array(
-      'markup' => $template->render(),
-      'config' => array(
-        'src' => $datasource->getDatasourceURI(),
-        'browseURI' => $datasource->getBrowseURI(),
-        'placeholder' => $datasource->getPlaceholderText(),
-      ),
-    );
-  }
-
 }

src/applications/transactions/edittype/PhabricatorPHIDListEditType.php

@@ -0,0 +1,90 @@
+<?php
+
+abstract class PhabricatorPHIDListEditType
+  extends PhabricatorEditType {
+
+  private $datasource;
+  private $isSingleValue;
+  private $defaultValue;
+
+  public function setDatasource(PhabricatorTypeaheadDatasource $datasource) {
+    $this->datasource = $datasource;
+    return $this;
+  }
+
+  public function getDatasource() {
+    return $this->datasource;
+  }
+
+  public function setIsSingleValue($is_single_value) {
+    $this->isSingleValue = $is_single_value;
+    return $this;
+  }
+
+  public function getIsSingleValue() {
+    return $this->isSingleValue;
+  }
+
+  public function setDefaultValue(array $default_value) {
+    $this->defaultValue = $default_value;
+    return $this;
+  }
+
+  public function getDefaultValue() {
+    return $this->defaultValue;
+  }
+
+  public function getValueType() {
+    if ($this->getIsSingleValue()) {
+      return 'phid';
+    } else {
+      return 'list<phid>';
+    }
+  }
+
+  public function getPHUIXControlType() {
+    $datasource = $this->getDatasource();
+
+    if (!$datasource) {
+      return null;
+    }
+
+    return 'tokenizer';
+  }
+
+  public function getPHUIXControlSpecification() {
+    $datasource = $this->getDatasource();
+
+    if (!$datasource) {
+      return null;
+    }
+
+    $template = new AphrontTokenizerTemplateView();
+
+    if ($this->getIsSingleValue()) {
+      $limit = 1;
+    } else {
+      $limit = null;
+    }
+
+    $default = $this->getDefaultValue();
+    if ($default) {
+      $value = $datasource->getWireTokens($default);
+    } else {
+      $value = array();
+    }
+
+    return array(
+      'markup' => $template->render(),
+      'config' => array(
+        'src' => $datasource->getDatasourceURI(),
+        'browseURI' => $datasource->getBrowseURI(),
+        'placeholder' => $datasource->getPlaceholderText(),
+        'limit' => $limit,
+      ),
+      'value' => $value,
+    );
+  }
+
+
+}

src/applications/typeahead/datasource/PhabricatorTypeaheadDatasource.php

@@ -458,4 +458,25 @@ abstract class PhabricatorTypeaheadDatasource extends Phobject {
     return $tokens;
   }
 
+  public function getWireTokens(array $values) {
+    // TODO: This is a bit hacky for now: we're sort of generating wire
+    // results, rendering them, then reverting them back to wire results. This
+    // is pretty silly. It would probably be much cleaner to make
+    // renderTokens() call this method instead, then render from the result
+    // structure.
+    $rendered = $this->renderTokens($values);
+
+    $tokens = array();
+    foreach ($rendered as $key => $render) {
+      $tokens[$key] = id(new PhabricatorTypeaheadResult())
+        ->setPHID($key)
+        ->setIcon($render->getIcon())
+        ->setColor($render->getColor())
+        ->setDisplayName($render->getValue())
+        ->setTokenType($render->getTokenType());
+    }
+
+    return mpull($tokens, 'getWireFormat');
+  }
+
 }

webroot/rsrc/js/core/Prefab.js

@@ -173,7 +173,16 @@ JX.install('Prefab', {
       var tokenizer = new JX.Tokenizer(root);
       tokenizer.setTypeahead(typeahead);
       tokenizer.setRenderTokenCallback(function(value, key, container) {
-        var result = datasource.getResult(key);
+        var result;
+        if (value && (typeof value == 'object') && ('id' in value)) {
+          // TODO: In this case, we've been passed the decoded wire format
+          // dictionary directly. Token rendering is kind of a huge mess that
+          // should be cleaned up and made more consistent. Just force our
+          // way through for now.
+          result = value;
+        } else {
+          result = datasource.getResult(key);
+        }
 
         var icon;
         var type;

webroot/rsrc/js/phuix/PHUIXFormControl.js

@@ -118,16 +118,21 @@ JX.install('PHUIXFormControl', {
         spec.config);
       build.tokenizer.start();
 
+      function set_value(map) {
+        for (var k in map) {
+          var v = JX.Prefab.transformDatasourceResults(map[k]);
+          build.tokenizer.addToken(k, v);
+        }
+      }
+
+      set_value(spec.value || {});
+
       return {
         node: build.node,
         get: function() {
           return JX.keys(build.tokenizer.getTokens());
         },
-        set: function(map) {
-          for (var k in map) {
-            build.tokenizer.addToken(k, map[k]);
-          }
-        }
+        set: set_value
       };
     },