From c97fcd12bceff2b1260f1ee7b466949dff27b705 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 16 Dec 2011 14:45:10 -0800 Subject: [PATCH] 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 | 1 + ...entialManiphestTasksFieldSpecification.php | 45 +----- .../specification/maniphesttasks/__init__.php | 4 +- .../PhabricatorSearchAttachController.php | 106 +------------ .../search/controller/attach/__init__.php | 1 + .../PhabricatorObjectAttachmentEditor.php | 139 ++++++++++++++++++ .../search/editor/attach/__init__.php | 18 +++ 7 files changed, 174 insertions(+), 140 deletions(-) create mode 100644 src/applications/search/editor/attach/PhabricatorObjectAttachmentEditor.php create mode 100644 src/applications/search/editor/attach/__init__.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 5c8f1d4dc0..417abce462 100644 --- a/src/__phutil_library_map__.php +++ b/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', diff --git a/src/applications/differential/field/specification/maniphesttasks/DifferentialManiphestTasksFieldSpecification.php b/src/applications/differential/field/specification/maniphesttasks/DifferentialManiphestTasksFieldSpecification.php index d262957660..3c69861607 100644 --- a/src/applications/differential/field/specification/maniphesttasks/DifferentialManiphestTasksFieldSpecification.php +++ b/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() { diff --git a/src/applications/differential/field/specification/maniphesttasks/__init__.php b/src/applications/differential/field/specification/maniphesttasks/__init__.php index 4f9b037319..287d872f19 100644 --- a/src/applications/differential/field/specification/maniphesttasks/__init__.php +++ b/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'); diff --git a/src/applications/search/controller/attach/PhabricatorSearchAttachController.php b/src/applications/search/controller/attach/PhabricatorSearchAttachController.php index f5e3e6a2c3..c722b88168 100644 --- a/src/applications/search/controller/attach/PhabricatorSearchAttachController.php +++ b/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: diff --git a/src/applications/search/controller/attach/__init__.php b/src/applications/search/controller/attach/__init__.php index c8bb6ac82e..e9184e5468 100644 --- a/src/applications/search/controller/attach/__init__.php +++ b/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'); diff --git a/src/applications/search/editor/attach/PhabricatorObjectAttachmentEditor.php b/src/applications/search/editor/attach/PhabricatorObjectAttachmentEditor.php new file mode 100644 index 0000000000..3d4bf4d149 --- /dev/null +++ b/src/applications/search/editor/attach/PhabricatorObjectAttachmentEditor.php @@ -0,0 +1,139 @@ +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)); + } + +} diff --git a/src/applications/search/editor/attach/__init__.php b/src/applications/search/editor/attach/__init__.php new file mode 100644 index 0000000000..1a879354e4 --- /dev/null +++ b/src/applications/search/editor/attach/__init__.php @@ -0,0 +1,18 @@ +