"Merge Duplicates" in Maniphest

Summary:
Allow duplicate tasks to be selected and merged in Maniphest.

I didn't create a separate transaction type for this because that implies a
bunch of really complicated rules which I don't want to sort out right now
(e.g., do we need to do cycle detection for merges? If so, what do we do when we
detect a cycle?) since I think it's unnecessary to get right for the initial
implementation (my Tasks merge implementation was similar to this and worked
quite well) and if/when we eventually need the metadata to be available in a
computer-readable form that need should inform the implementation.

Plenty of room for improvement here, of course.

Test Plan:
Merged duplicate tasks, tried to perform invalid merge operations (e.g., merge a
task into itself).
Tested existing attach workflows (task -> revision, revision -> task).

Reviewed By: aran
Reviewers: tuomaspelkonen, jungejason, aran
CC: anjali, aran
Differential Revision: 459

src/__celerity_resource_map__.php

@@ -63,7 +63,7 @@ celerity_register_resource_map(array(
   ),
   'aphront-headsup-action-list-view-css' =>
   array(
-    'uri' => '/res/71783633/rsrc/css/aphront/headsup-action-list-view.css',
+    'uri' => '/res/b7c6bbc2/rsrc/css/aphront/headsup-action-list-view.css',
     'type' => 'css',
     'requires' =>
     array(
@@ -626,7 +626,7 @@ celerity_register_resource_map(array(
   ),
   'javelin-behavior-phabricator-object-selector' =>
   array(
-    'uri' => '/res/12d4d90d/rsrc/js/application/core/behavior-object-selector.js',
+    'uri' => '/res/34f9a11e/rsrc/js/application/core/behavior-object-selector.js',
     'type' => 'js',
     'requires' =>
     array(
@@ -902,7 +902,7 @@ celerity_register_resource_map(array(
   ),
   'maniphest-transaction-detail-css' =>
   array(
-    'uri' => '/res/7ee02b5e/rsrc/css/application/maniphest/transaction-detail.css',
+    'uri' => '/res/149fccab/rsrc/css/application/maniphest/transaction-detail.css',
     'type' => 'css',
     'requires' =>
     array(
@@ -1038,7 +1038,7 @@ celerity_register_resource_map(array(
   ),
   'phabricator-object-selector-css' =>
   array(
-    'uri' => '/res/ced4098a/rsrc/css/application/objectselector/object-selector.css',
+    'uri' => '/res/62a8fd92/rsrc/css/application/objectselector/object-selector.css',
     'type' => 'css',
     'requires' =>
     array(

src/aphront/default/configuration/AphrontDefaultApplicationConfiguration.php

@@ -188,7 +188,7 @@ class AphrontDefaultApplicationConfiguration
       '/search/' => array(
         '$' => 'PhabricatorSearchController',
         '(?P<id>\d+)/$' => 'PhabricatorSearchController',
-        'attach/(?P<phid>[^/]+)/(?P<type>\w+)/$'
+        'attach/(?P<phid>[^/]+)/(?P<type>\w+)/(?:(?P<action>\w+)/)?$'
           => 'PhabricatorSearchAttachController',
         'select/(?P<type>\w+)/$'
           => 'PhabricatorSearchSelectController',

src/applications/maniphest/controller/taskdetail/ManiphestTaskDetailController.php

@@ -174,6 +174,13 @@ class ManiphestTaskDetailController extends ManiphestController {
     require_celerity_resource('phabricator-object-selector-css');
     require_celerity_resource('javelin-behavior-phabricator-object-selector');
 
+    $action = new AphrontHeadsupActionView();
+    $action->setName('Merge Duplicates');
+    $action->setURI('/search/attach/'.$task->getPHID().'/TASK/merge/');
+    $action->setWorkflow(true);
+    $action->setClass('action-merge');
+    $actions[] = $action;
+
     $action = new AphrontHeadsupActionView();
     $action->setName('Edit Differential Revisions');
     $action->setURI('/search/attach/'.$task->getPHID().'/DREV/');

src/applications/maniphest/view/transactiondetail/ManiphestTransactionDetailView.php

@@ -396,6 +396,10 @@ class ManiphestTransactionDetailView extends AphrontView {
           $verb = 'Spited';
           $desc = 'closed this task out of spite';
           $classes[] = 'spited';
+        } else if ($new == ManiphestTaskStatus::STATUS_CLOSED_DUPLICATE) {
+          $verb = 'Merged';
+          $desc = 'closed this task as a duplicate';
+          $classes[] = 'duplicate';
         } else {
           $verb = 'Closed';
           $full = idx(ManiphestTaskStatus::getTaskStatusMap(), $new, '???');

src/applications/search/controller/attach/PhabricatorSearchAttachController.php

@@ -20,10 +20,15 @@ class PhabricatorSearchAttachController extends PhabricatorSearchController {
 
   private $phid;
   private $type;
+  private $action;
+
+  const ACTION_ATTACH = 'attach';
+  const ACTION_MERGE  = 'merge';
 
   public function willProcessRequest(array $data) {
     $this->phid = $data['phid'];
     $this->type = $data['type'];
+    $this->action = idx($data, 'action', self::ACTION_ATTACH);
   }
 
   public function processRequest() {
@@ -39,7 +44,6 @@ class PhabricatorSearchAttachController extends PhabricatorSearchController {
     $object_type = $handle->getType();
     $attach_type = $this->type;
 
-
     // Load the object we're going to attach/detach stuff from. This is the
     // object that triggered the action, e.g. the revision you clicked
     // "Edit Maniphest Tasks" on.
@@ -65,6 +69,17 @@ class PhabricatorSearchAttachController extends PhabricatorSearchController {
       $phids = explode(';', $request->getStr('phids'));
       $phids = array_filter($phids);
       $phids = array_values($phids);
+
+      switch ($this->action) {
+        case self::ACTION_MERGE:
+          return $this->performMerge($object, $handle, $phids);
+        case self::ACTION_ATTACH:
+          // Fall through to the workflow below.
+          break;
+        default:
+          throw new Exception("Unsupported attach action.");
+      }
+
       // sort() so that removing [X, Y] and then adding [Y, X] is correctly
       // detected as a no-op.
       sort($phids);
@@ -139,10 +154,17 @@ class PhabricatorSearchAttachController extends PhabricatorSearchController {
 
       return id(new AphrontReloadResponse())->setURI($handle->getURI());
     } else {
-      $phids = $object->getAttachedPHIDs($attach_type);
+      switch ($this->action) {
+        case self::ACTION_ATTACH:
+          $phids = $object->getAttachedPHIDs($attach_type);
+          break;
+        default:
+          $phids = array();
+          break;
+      }
     }
 
-    switch ($attach_type) {
+    switch ($this->type) {
       case PhabricatorPHIDConstants::PHID_TYPE_DREV:
         $noun = 'Revisions';
         $selected = 'created';
@@ -153,6 +175,24 @@ class PhabricatorSearchAttachController extends PhabricatorSearchController {
         break;
     }
 
+    switch ($this->action) {
+      case self::ACTION_ATTACH:
+        $dialog_title = "Manage Attached {$noun}";
+        $header_text = "Currently Attached {$noun}";
+        $button_text = "Save {$noun}";
+        $instructions = null;
+        break;
+      case self::ACTION_MERGE:
+        $dialog_title = "Merge Duplicate Tasks";
+        $header_text = "Tasks To Merge";
+        $button_text = "Merge {$noun}";
+        $instructions =
+          "These tasks will be merged into the current task and then closed. ".
+          "The current task (\"".phutil_escape_html($handle->getName())."\") ".
+          "will grow stronger.";
+        break;
+    }
+
     $handles = id(new PhabricatorObjectHandleData($phids))
       ->loadHandles();
 
@@ -169,7 +209,10 @@ class PhabricatorSearchAttachController extends PhabricatorSearchController {
       ->setSelectedFilter($selected)
       ->setCancelURI($handle->getURI())
       ->setSearchURI('/search/select/'.$attach_type.'/')
-      ->setNoun($noun);
+      ->setTitle($dialog_title)
+      ->setHeader($header_text)
+      ->setButtonText($button_text)
+      ->setInstructions($instructions);
 
     $dialog = $obj_dialog->buildDialog();
 
@@ -196,4 +239,67 @@ class PhabricatorSearchAttachController extends PhabricatorSearchController {
     $transaction->setNewValue($new);
     $editor->applyTransactions($task, array($transaction));
   }
+
+  private function performMerge(
+    ManiphestTask $task,
+    PhabricatorObjectHandle $handle,
+    array $phids) {
+
+    $user = $this->getRequest()->getUser();
+    $response = id(new AphrontReloadResponse())->setURI($handle->getURI());
+
+    $phids = array_fill_keys($phids, true);
+    unset($phids[$task->getPHID()]); // Prevent merging a task into itself.
+
+    if (!$phids) {
+      return $response;
+    }
+
+    $targets = id(new ManiphestTask())->loadAllWhere(
+      'phid in (%Ls) ORDER BY id ASC',
+      array_keys($phids));
+
+    if (empty($targets)) {
+      return $response;
+    }
+
+    $editor = new ManiphestTransactionEditor();
+
+    $task_names = array();
+
+    $merge_into_name = 'T'.$task->getID();
+
+    $cc_vector = array();
+    $cc_vector[] = $task->getCCPHIDs();
+    foreach ($targets as $target) {
+      $cc_vector[] = $target->getCCPHIDs();
+      $cc_vector[] = array(
+        $target->getAuthorPHID(),
+        $target->getOwnerPHID());
+
+      $close_task = id(new ManiphestTransaction())
+        ->setAuthorPHID($user->getPHID())
+        ->setTransactionType(ManiphestTransactionType::TYPE_STATUS)
+        ->setNewValue(ManiphestTaskStatus::STATUS_CLOSED_DUPLICATE)
+        ->setComments("\xE2\x9C\x98 Merged into {$merge_into_name}.");
+
+      $editor->applyTransactions($target, array($close_task));
+
+      $task_names[] = 'T'.$target->getID();
+    }
+    $all_ccs = array_mergev($cc_vector);
+    $all_ccs = array_filter($all_ccs);
+    $all_ccs = array_unique($all_ccs);
+
+    $task_names = implode(', ', $task_names);
+
+    $add_ccs = id(new ManiphestTransaction())
+      ->setAuthorPHID($user->getPHID())
+      ->setTransactionType(ManiphestTransactionType::TYPE_CCS)
+      ->setNewValue($all_ccs)
+      ->setComments("\xE2\x97\x80 Merged tasks: {$task_names}.");
+    $editor->applyTransactions($task, array($add_ccs));
+
+    return $response;
+  }
 }

src/applications/search/controller/attach/__init__.php

@@ -10,6 +10,7 @@ phutil_require_module('phabricator', 'aphront/response/404');
 phutil_require_module('phabricator', 'aphront/response/dialog');
 phutil_require_module('phabricator', 'aphront/response/reload');
 phutil_require_module('phabricator', 'applications/differential/storage/revision');
+phutil_require_module('phabricator', 'applications/maniphest/constants/status');
 phutil_require_module('phabricator', 'applications/maniphest/constants/transactiontype');
 phutil_require_module('phabricator', 'applications/maniphest/editor/transaction');
 phutil_require_module('phabricator', 'applications/maniphest/storage/task');
@@ -19,6 +20,7 @@ phutil_require_module('phabricator', 'applications/phid/handle/data');
 phutil_require_module('phabricator', 'applications/search/controller/search');
 phutil_require_module('phabricator', 'view/control/objectselector');
 
+phutil_require_module('phutil', 'markup');
 phutil_require_module('phutil', 'utils');
 
 

src/view/control/objectselector/PhabricatorObjectSelectorDialog.php

@@ -23,10 +23,14 @@ class PhabricatorObjectSelectorDialog {
   private $handles = array();
   private $cancelURI;
   private $submitURI;
-  private $noun;
   private $searchURI;
   private $selectedFilter;
 
+  private $title;
+  private $header;
+  private $buttonText;
+  private $instructions;
+
   public function setUser($user) {
     $this->user = $user;
     return $this;
@@ -62,8 +66,23 @@ class PhabricatorObjectSelectorDialog {
     return $this;
   }
 
-  public function setNoun($noun) {
+  public function setTitle($title) {
-    $this->noun = $noun;
+    $this->title = $title;
+    return $this;
+  }
+
+  public function setHeader($header) {
+    $this->header = $header;
+    return $this;
+  }
+
+  public function setButtonText($button_text) {
+    $this->buttonText = $button_text;
+    return $this;
+  }
+
+  public function setInstructions($instructions) {
+    $this->instructions = $instructions;
     return $this;
   }
 
@@ -93,6 +112,14 @@ class PhabricatorObjectSelectorDialog {
     }
     $options = implode("\n", $options);
 
+    $instructions = null;
+    if ($this->instructions) {
+      $instructions =
+        '<p class="phabricator-object-selector-instructions">'.
+          $this->instructions.
+        '</p>';
+    }
+
     $search_box = phabricator_render_form(
       $user,
       array(
@@ -119,10 +146,11 @@ class PhabricatorObjectSelectorDialog {
       '<div class="phabricator-object-selector-current">'.
         '<div class="phabricator-object-selector-currently-attached">'.
           '<div class="phabricator-object-selector-header">'.
-            'Currently Attached '.$this->noun.
+            phutil_escape_html($this->header).
           '</div>'.
           '<div id="'.$current_id.'">'.
           '</div>'.
+          $instructions.
         '</div>'.
       '</div>';
 
@@ -130,14 +158,14 @@ class PhabricatorObjectSelectorDialog {
     $dialog = new AphrontDialogView();
     $dialog
       ->setUser($this->user)
-      ->setTitle('Manage Attached '.$this->noun)
+      ->setTitle($this->title)
       ->setClass('phabricator-object-selector-dialog')
       ->appendChild($search_box)
       ->appendChild($result_box)
       ->appendChild($attached_box)
       ->setRenderDialogAsDiv()
       ->setFormID($form_id)
-      ->addSubmitButton('Save '.$this->noun);
+      ->addSubmitButton($this->buttonText);
 
     if ($this->cancelURI) {
       $dialog->addCancelButton($this->cancelURI);

webroot/rsrc/css/aphront/headsup-action-list-view.css

@@ -58,3 +58,6 @@
   background-image: url(/rsrc/image/icon/tango/log.png);
 }
 
+.aphront-headsup-action-list .action-merge {
+  background-image: url(/rsrc/image/icon/fatcow/arrow_merge.png);
+}

webroot/rsrc/css/application/maniphest/transaction-detail.css

@@ -55,6 +55,10 @@
   border-color: #aa0000;
 }
 
+.maniphest-transaction-detail-container .duplicate {
+  border-color: #333333;
+}
+
 .maniphest-transaction-header {
   background: #f3f3f3;
   padding: 4px 1em;

webroot/rsrc/css/application/objectselector/object-selector.css

@@ -86,3 +86,9 @@ td.phabricator-object-selector-search-text {
   color: #888888;
   text-align: center;
 }
+
+.phabricator-object-selector-instructions {
+  font-size: 11px;
+  color: #666666;
+  margin-top: 1.25em;
+}

webroot/rsrc/image/icon/fatcow/README

@@ -8,4 +8,5 @@ They are available under the Creative Commons Attribution 3.0 License:
 
 Some icons have been adapted from the FatCow set for use in Phabricator:
 
-  key_question.png
+  key_question.png
+

webroot/rsrc/js/application/core/behavior-object-selector.js

@@ -80,7 +80,7 @@ JX.behavior('phabricator-object-selector', function(config) {
     var btn = JX.$N(
       'a',
       {className: 'button small grey'},
-      attach ? 'Attach' : 'Remove');
+      attach ? 'Select' : 'Remove');
 
     JX.Stratcom.addSigil(btn, 'object-attach-button');
     JX.Stratcom.addData(btn, {handle : h, table : table});