Share Revision/Task attaching code

Summary:
We have this code in two places; split it into an editor class so we can share
it.

This also fixes some probems with this field not //detaching// tasks properly.

Test Plan:
  - Created a revision with no attached tasks.
  - Attached it to a task.
  - Updated it.
  - Detached it.
  - Used web UI to attach/detach tasks/revisions.

Reviewers: btrahan, jungejason

Reviewed By: btrahan

CC: aran, btrahan, epriestley

Differential Revision: 1225

src/__phutil_library_map__.php

@@ -522,6 +522,7 @@ phutil_register_library_map(array(
     'PhabricatorOAuthProviderGoogle' => 'applications/auth/oauth/provider/google',
     'PhabricatorOAuthRegistrationController' => 'applications/auth/controller/oauthregistration/base',
     'PhabricatorOAuthUnlinkController' => 'applications/auth/controller/unlink',
+    'PhabricatorObjectAttachmentEditor' => 'applications/search/editor/attach',
     'PhabricatorObjectGraph' => 'applications/phid/graph',
     'PhabricatorObjectHandle' => 'applications/phid/handle',
     'PhabricatorObjectHandleConstants' => 'applications/phid/handle/const/base',

src/applications/differential/field/specification/maniphesttasks/DifferentialManiphestTasksFieldSpecification.php

@@ -59,43 +59,14 @@ final class DifferentialManiphestTasksFieldSpecification
    * @return void
    */
   public function didWriteRevision(DifferentialRevisionEditor $editor) {
-    // 1 -- revision => tasks
-    $revision = $editor->getRevision();
-    $revision->setAttachedPHIDs(PhabricatorPHIDConstants::PHID_TYPE_TASK,
-      $this->maniphestTasks);
-
-    // 2 -- tasks => revision
-    $maniphest_editor = new ManiphestTransactionEditor();
-    $user = $this->getUser();
-    $type = ManiphestTransactionType::TYPE_ATTACH;
-    $attach_type = PhabricatorPHIDConstants::PHID_TYPE_DREV;
-
-    $tasks = array();
-    if ($this->maniphestTasks) {
-      $tasks = id(new ManiphestTask())->loadAllWhere(
-        'phid IN (%Ls)',
-        $this->maniphestTasks);
-    }
-
-    foreach ($tasks as $task) {
-      $transaction = new ManiphestTransaction();
-      $transaction->setAuthorPHID($user->getPHID());
-      $transaction->setTransactionType($type);
-
-      $new = $task->getAttached();
-      if (empty($new[$attach_type])) {
-        $new[$attach_type] = array();
-      }
-      if (array_key_exists($revision->getPHID(), $new[$attach_type])) {
-        // Already attached, just skip the update.
-        continue;
-      }
-
-      $new[$attach_type][$revision->getPHID()] = array();
-
-      $transaction->setNewValue($new);
-      $maniphest_editor->applyTransactions($task, array($transaction));
-    }
+    $aeditor = new PhabricatorObjectAttachmentEditor(
+      PhabricatorPHIDConstants::PHID_TYPE_DREV,
+      $editor->getRevision());
+    $aeditor->setUser($this->getUser());
+    $aeditor->attachObjects(
+      PhabricatorPHIDConstants::PHID_TYPE_TASK,
+      $this->maniphestTasks,
+      $two_way = true);
   }
 
   protected function didSetRevision() {

src/applications/differential/field/specification/maniphesttasks/__init__.php

@@ -8,11 +8,9 @@
 
 phutil_require_module('phabricator', 'applications/differential/field/exception/parse');
 phutil_require_module('phabricator', 'applications/differential/field/specification/base');
-phutil_require_module('phabricator', 'applications/maniphest/constants/transactiontype');
-phutil_require_module('phabricator', 'applications/maniphest/editor/transaction');
 phutil_require_module('phabricator', 'applications/maniphest/storage/task');
-phutil_require_module('phabricator', 'applications/maniphest/storage/transaction');
 phutil_require_module('phabricator', 'applications/phid/constants');
+phutil_require_module('phabricator', 'applications/search/editor/attach');
 phutil_require_module('phabricator', 'infrastructure/env');
 
 phutil_require_module('phutil', 'utils');

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

@@ -73,12 +73,16 @@ class PhabricatorSearchAttachController extends PhabricatorSearchController {
               $attach_type,
               $phids);
           }
-          $this->performAttach(
+
+          $editor = new PhabricatorObjectAttachmentEditor(
             $object_type,
-            $object,
+            $object);
+          $editor->setUser($this->getRequest()->getUser());
+          $editor->attachObjects(
             $attach_type,
             $phids,
             $two_way);
+
           return id(new AphrontReloadResponse())->setURI($handle->getURI());
         default:
           throw new Exception("Unsupported attach action.");
@@ -123,84 +127,6 @@ class PhabricatorSearchAttachController extends PhabricatorSearchController {
     return id(new AphrontDialogResponse())->setDialog($dialog);
   }
 
-  private function applyTaskTransaction(
-    ManiphestTask $task,
-    $attach_type,
-    array $new_phids) {
-
-    $user = $this->getRequest()->getUser();
-
-    $editor = new ManiphestTransactionEditor();
-    $type = ManiphestTransactionType::TYPE_ATTACH;
-
-    $transaction = new ManiphestTransaction();
-    $transaction->setAuthorPHID($user->getPHID());
-    $transaction->setTransactionType($type);
-
-    $new = $task->getAttached();
-    $new[$attach_type] = array_fill_keys($new_phids, array());
-
-    $transaction->setNewValue($new);
-    $editor->applyTransactions($task, array($transaction));
-  }
-
-  private function performAttach(
-    $object_type,
-    $object,
-    $attach_type,
-    array $phids,
-    $two_way) {
-
-    $object_phid = $object->getPHID();
-
-    // sort() so that removing [X, Y] and then adding [Y, X] is correctly
-    // detected as a no-op.
-    sort($phids);
-    $old_phids = $object->getAttachedPHIDs($attach_type);
-    sort($old_phids);
-    $phids = array_values($phids);
-    $old_phids = array_values($old_phids);
-
-    if ($phids === $old_phids) {
-      return;
-    }
-
-    $all_phids = array_merge($phids, $old_phids);
-    $attach_objs = id(new PhabricatorObjectHandleData($all_phids))
-      ->loadObjects();
-
-    // Remove PHIDs which don't actually exist, to prevent silliness.
-    $phids = array_keys(array_select_keys($attach_objs, $phids));
-    if ($phids) {
-      $phids = array_combine($phids, $phids);
-    }
-
-    // Update the primary object.
-    $this->writeOutboundEdges($object_type, $object, $attach_type, $phids);
-
-    if (!$two_way) {
-      return;
-    }
-
-    // Loop through all of the attached/detached objects and update them.
-    foreach ($attach_objs as $phid => $attach_obj) {
-      $attached_phids = $attach_obj->getAttachedPHIDs($object_type);
-      // Figure out if we're attaching or detaching this object.
-      if (isset($phids[$phid])) {
-        $attached_phids[] = $object_phid;
-      } else {
-        $attached_phids = array_fill_keys($attached_phids, true);
-        unset($attached_phids[$object_phid]);
-        $attached_phids = array_keys($attached_phids);
-      }
-      $this->writeOutboundEdges(
-        $attach_type,
-        $attach_obj,
-        $object_type,
-        $attached_phids);
-    }
-  }
-
   private function performMerge(
     ManiphestTask $task,
     PhabricatorObjectHandle $handle,
@@ -264,26 +190,6 @@ class PhabricatorSearchAttachController extends PhabricatorSearchController {
     return $response;
   }
 
-  private function writeOutboundEdges(
-    $object_type,
-    $object,
-    $attach_type,
-    array $attach_phids) {
-
-    switch ($object_type) {
-      case PhabricatorPHIDConstants::PHID_TYPE_DREV:
-        $object->setAttachedPHIDs($attach_type, $attach_phids);
-        $object->save();
-        break;
-      case PhabricatorPHIDConstants::PHID_TYPE_TASK:
-        $this->applyTaskTransaction(
-          $object,
-          $attach_type,
-          $attach_phids);
-        break;
-    }
-  }
-
   private function getStrings() {
     switch ($this->type) {
       case PhabricatorPHIDConstants::PHID_TYPE_DREV:

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

@@ -18,6 +18,7 @@ phutil_require_module('phabricator', 'applications/phid/constants');
 phutil_require_module('phabricator', 'applications/phid/graph');
 phutil_require_module('phabricator', 'applications/phid/handle/data');
 phutil_require_module('phabricator', 'applications/search/controller/search');
+phutil_require_module('phabricator', 'applications/search/editor/attach');
 phutil_require_module('phabricator', 'view/control/objectselector');
 
 phutil_require_module('phutil', 'utils');

src/applications/search/editor/attach/PhabricatorObjectAttachmentEditor.php

@@ -0,0 +1,139 @@
+<?php
+
+/*
+ * Copyright 2011 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+/**
+ * Manage attaching, detaching and updating edges between objects (for instance,
+ * relationships between Tasks and Revisions).
+ */
+final class PhabricatorObjectAttachmentEditor {
+
+  private $objectType;
+  private $object;
+  private $user;
+
+  public function __construct($object_type, $object) {
+    $this->objectType = $object_type;
+    $this->object = $object;
+  }
+
+  public function setUser(PhabricatorUser $user) {
+    $this->user = $user;
+    return $this;
+  }
+
+  public function attachObjects($attach_type, array $phids, $two_way) {
+    $object_type = $this->objectType;
+    $object = $this->object;
+    $object_phid = $object->getPHID();
+
+    // sort() so that removing [X, Y] and then adding [Y, X] is correctly
+    // detected as a no-op.
+    sort($phids);
+    $old_phids = $object->getAttachedPHIDs($attach_type);
+    sort($old_phids);
+    $phids = array_values($phids);
+    $old_phids = array_values($old_phids);
+
+    if ($phids === $old_phids) {
+      return;
+    }
+
+    $all_phids = array_merge($phids, $old_phids);
+    $attach_objs = id(new PhabricatorObjectHandleData($all_phids))
+      ->loadObjects();
+
+    // Remove PHIDs which don't actually exist, to prevent silliness.
+    $phids = array_keys(array_select_keys($attach_objs, $phids));
+    if ($phids) {
+      $phids = array_combine($phids, $phids);
+    }
+
+    // Update the primary object.
+    $this->writeOutboundEdges($object_type, $object, $attach_type, $phids);
+
+    if (!$two_way) {
+      return;
+    }
+
+    // Loop through all of the attached/detached objects and update them.
+    foreach ($attach_objs as $phid => $attach_obj) {
+      $attached_phids = $attach_obj->getAttachedPHIDs($object_type);
+      // Figure out if we're attaching or detaching this object.
+      if (isset($phids[$phid])) {
+        if (in_array($object_phid, $attached_phids)) {
+          // Already attached.
+          continue;
+        }
+        $attached_phids[] = $object_phid;
+      } else {
+        $attached_phids = array_fill_keys($attached_phids, true);
+        unset($attached_phids[$object_phid]);
+        $attached_phids = array_keys($attached_phids);
+      }
+      $this->writeOutboundEdges(
+        $attach_type,
+        $attach_obj,
+        $object_type,
+        $attached_phids);
+    }
+  }
+
+  private function writeOutboundEdges(
+    $object_type,
+    $object,
+    $attach_type,
+    array $attach_phids) {
+    switch ($object_type) {
+      case PhabricatorPHIDConstants::PHID_TYPE_DREV:
+        $object->setAttachedPHIDs($attach_type, $attach_phids);
+        $object->save();
+        break;
+      case PhabricatorPHIDConstants::PHID_TYPE_TASK:
+        $this->applyTaskTransaction(
+          $object,
+          $attach_type,
+          $attach_phids);
+        break;
+    }
+  }
+
+  private function applyTaskTransaction(
+    ManiphestTask $task,
+    $attach_type,
+    array $new_phids) {
+
+    if (!$this->user) {
+      throw new Exception("Call setUser() before editing attachments!");
+    }
+    $user = $this->user;
+
+    $editor = new ManiphestTransactionEditor();
+    $type = ManiphestTransactionType::TYPE_ATTACH;
+
+    $transaction = new ManiphestTransaction();
+    $transaction->setAuthorPHID($user->getPHID());
+    $transaction->setTransactionType($type);
+
+    $new = $task->getAttached();
+    $new[$attach_type] = array_fill_keys($new_phids, array());
+
+    $transaction->setNewValue($new);
+    $editor->applyTransactions($task, array($transaction));
+  }
+
+}

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

@@ -0,0 +1,18 @@
+<?php
+/**
+ * This file is automatically generated. Lint this module to rebuild it.
+ * @generated
+ */
+
+
+
+phutil_require_module('phabricator', 'applications/maniphest/constants/transactiontype');
+phutil_require_module('phabricator', 'applications/maniphest/editor/transaction');
+phutil_require_module('phabricator', 'applications/maniphest/storage/transaction');
+phutil_require_module('phabricator', 'applications/phid/constants');
+phutil_require_module('phabricator', 'applications/phid/handle/data');
+
+phutil_require_module('phutil', 'utils');
+
+
+phutil_require_source('PhabricatorObjectAttachmentEditor.php');