From e8c490958c8a2584c5d2dee5431fb34b4776c626 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 17 Jul 2014 15:43:40 -0700 Subject: [PATCH] Stop writing new TYPE_PROJECTS transactions to Maniphest Summary: Ref T5245. We'll still display the old ones, but write real edge transactions now -- not TYPE_PROJECTS transactions. Some code remains to show the existing transactions. The next diff will modernize the old transactions so we can remove this code. Test Plan: - Previewed a project-editing comment. - Submitted a project-editing comment. - Edited a task's projects. - Batch edited a task's projects. Reviewers: joshuaspence, chad, btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T5245 Differential Revision: https://secure.phabricator.com/D9852 --- src/__phutil_library_map__.php | 2 - .../conduit/ConduitAPI_maniphest_Method.php | 13 ++++- .../ManiphestBatchEditController.php | 12 +++++ .../ManiphestTaskEditController.php | 11 +++++ .../ManiphestTransactionPreviewController.php | 15 ++++-- .../ManiphestTransactionSaveController.php | 11 ++++- .../editor/ManiphestTransactionEditor.php | 40 +++++++-------- ...bricatorManiphestTaskTestDataGenerator.php | 2 - .../storage/ManiphestTaskProject.php | 49 ------------------- .../storage/ManiphestTransaction.php | 11 ++++- 10 files changed, 82 insertions(+), 84 deletions(-) delete mode 100644 src/applications/maniphest/storage/ManiphestTaskProject.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 6718dc6a72..fec80ea1af 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -954,7 +954,6 @@ phutil_register_library_map(array( 'ManiphestTaskOwner' => 'applications/maniphest/constants/ManiphestTaskOwner.php', 'ManiphestTaskPriority' => 'applications/maniphest/constants/ManiphestTaskPriority.php', 'ManiphestTaskPriorityDatasource' => 'applications/maniphest/typeahead/ManiphestTaskPriorityDatasource.php', - 'ManiphestTaskProject' => 'applications/maniphest/storage/ManiphestTaskProject.php', 'ManiphestTaskQuery' => 'applications/maniphest/query/ManiphestTaskQuery.php', 'ManiphestTaskResultListView' => 'applications/maniphest/view/ManiphestTaskResultListView.php', 'ManiphestTaskSearchEngine' => 'applications/maniphest/query/ManiphestTaskSearchEngine.php', @@ -3716,7 +3715,6 @@ phutil_register_library_map(array( 'ManiphestTaskOwner' => 'ManiphestConstants', 'ManiphestTaskPriority' => 'ManiphestConstants', 'ManiphestTaskPriorityDatasource' => 'PhabricatorTypeaheadDatasource', - 'ManiphestTaskProject' => 'ManiphestDAO', 'ManiphestTaskQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'ManiphestTaskResultListView' => 'ManiphestView', 'ManiphestTaskSearchEngine' => 'PhabricatorApplicationSearchEngine', diff --git a/src/applications/maniphest/conduit/ConduitAPI_maniphest_Method.php b/src/applications/maniphest/conduit/ConduitAPI_maniphest_Method.php index b2dd625bfc..d45c42dc88 100644 --- a/src/applications/maniphest/conduit/ConduitAPI_maniphest_Method.php +++ b/src/applications/maniphest/conduit/ConduitAPI_maniphest_Method.php @@ -116,18 +116,27 @@ abstract class ConduitAPI_maniphest_Method extends ConduitAPIMethod { $changes[ManiphestTransaction::TYPE_CCS] = $ccs; } + $transactions = array(); + $project_phids = $request->getValue('projectPHIDs'); if ($project_phids !== null) { $this->validatePHIDList( $project_phids, PhabricatorProjectPHIDTypeProject::TYPECONST, 'projectPHIDS'); - $changes[ManiphestTransaction::TYPE_PROJECTS] = $project_phids; + + $project_type = PhabricatorProjectObjectHasProjectEdgeType::EDGECONST; + $transactions[] = id(new ManiphestTransaction()) + ->setTransactionType(PhabricatorTransactions::TYPE_EDGE) + ->setMetadataValue('edge:type', $project_type) + ->setNewValue( + array( + '=' => array_fuse($project_phids), + )); } $template = new ManiphestTransaction(); - $transactions = array(); foreach ($changes as $type => $value) { $transaction = clone $template; $transaction->setTransactionType($type); diff --git a/src/applications/maniphest/controller/ManiphestBatchEditController.php b/src/applications/maniphest/controller/ManiphestBatchEditController.php index fbcb84884b..5dfaba42f3 100644 --- a/src/applications/maniphest/controller/ManiphestBatchEditController.php +++ b/src/applications/maniphest/controller/ManiphestBatchEditController.php @@ -324,6 +324,18 @@ final class ManiphestBatchEditController extends ManiphestController { id(new ManiphestTransactionComment()) ->setContent($value)); break; + case ManiphestTransaction::TYPE_PROJECTS: + + // TODO: Clean this mess up. + $project_type = PhabricatorProjectObjectHasProjectEdgeType::EDGECONST; + $xaction + ->setTransactionType(PhabricatorTransactions::TYPE_EDGE) + ->setMetadataValue('edge:type', $project_type) + ->setNewValue( + array( + '=' => array_fuse($value), + )); + break; default: $xaction->setNewValue($value); break; diff --git a/src/applications/maniphest/controller/ManiphestTaskEditController.php b/src/applications/maniphest/controller/ManiphestTaskEditController.php index ad7e306c36..de6b272068 100644 --- a/src/applications/maniphest/controller/ManiphestTaskEditController.php +++ b/src/applications/maniphest/controller/ManiphestTaskEditController.php @@ -269,6 +269,17 @@ final class ManiphestTaskEditController extends ManiphestController { if ($type == ManiphestTransaction::TYPE_PROJECT_COLUMN) { $transaction->setNewValue($value['new']); $transaction->setOldValue($value['old']); + } else if ($type == ManiphestTransaction::TYPE_PROJECTS) { + // TODO: Gross. + $project_type = + PhabricatorProjectObjectHasProjectEdgeType::EDGECONST; + $transaction + ->setTransactionType(PhabricatorTransactions::TYPE_EDGE) + ->setMetadataValue('edge:type', $project_type) + ->setNewValue( + array( + '=' => array_fuse($value), + )); } else { $transaction->setNewValue($value); } diff --git a/src/applications/maniphest/controller/ManiphestTransactionPreviewController.php b/src/applications/maniphest/controller/ManiphestTransactionPreviewController.php index 75f75c49bc..ac2b8a861a 100644 --- a/src/applications/maniphest/controller/ManiphestTransactionPreviewController.php +++ b/src/applications/maniphest/controller/ManiphestTransactionPreviewController.php @@ -83,14 +83,19 @@ final class ManiphestTransactionPreviewController extends ManiphestController { $value = array(); } - $phids = $value; - foreach ($task->getProjectPHIDs() as $project_phid) { + $phids = array(); + $value = array_fuse($value); + foreach ($value as $project_phid) { $phids[] = $project_phid; - $value[] = $project_phid; + $value[$project_phid] = array('dst' => $project_phid); } - $transaction->setOldValue($task->getProjectPHIDs()); - $transaction->setNewValue($value); + $project_type = PhabricatorProjectObjectHasProjectEdgeType::EDGECONST; + $transaction + ->setTransactionType(PhabricatorTransactions::TYPE_EDGE) + ->setMetadataValue('edge:type', $project_type) + ->setOldValue(array()) + ->setNewValue($value); break; case ManiphestTransaction::TYPE_STATUS: $phids = array(); diff --git a/src/applications/maniphest/controller/ManiphestTransactionSaveController.php b/src/applications/maniphest/controller/ManiphestTransactionSaveController.php index 34e2f57f37..a11149982b 100644 --- a/src/applications/maniphest/controller/ManiphestTransactionSaveController.php +++ b/src/applications/maniphest/controller/ManiphestTransactionSaveController.php @@ -53,7 +53,16 @@ final class ManiphestTransactionSaveController extends ManiphestController { $projects = array_merge($projects, $task->getProjectPHIDs()); $projects = array_filter($projects); $projects = array_unique($projects); - $transaction->setNewValue($projects); + + // TODO: Bleh. + $project_type = PhabricatorProjectObjectHasProjectEdgeType::EDGECONST; + $transaction + ->setTransactionType(PhabricatorTransactions::TYPE_EDGE) + ->setMetadataValue('edge:type', $project_type) + ->setNewValue( + array( + '+' => array_fuse($projects), + )); break; case ManiphestTransaction::TYPE_CCS: // Accumulate the new explicit CCs into the array that we'll add in diff --git a/src/applications/maniphest/editor/ManiphestTransactionEditor.php b/src/applications/maniphest/editor/ManiphestTransactionEditor.php index d7abddbcd1..58f3d5cda8 100644 --- a/src/applications/maniphest/editor/ManiphestTransactionEditor.php +++ b/src/applications/maniphest/editor/ManiphestTransactionEditor.php @@ -16,7 +16,6 @@ final class ManiphestTransactionEditor $types[] = ManiphestTransaction::TYPE_DESCRIPTION; $types[] = ManiphestTransaction::TYPE_OWNER; $types[] = ManiphestTransaction::TYPE_CCS; - $types[] = ManiphestTransaction::TYPE_PROJECTS; $types[] = ManiphestTransaction::TYPE_SUBPRIORITY; $types[] = ManiphestTransaction::TYPE_PROJECT_COLUMN; $types[] = ManiphestTransaction::TYPE_UNBLOCK; @@ -55,8 +54,6 @@ final class ManiphestTransactionEditor return nonempty($object->getOwnerPHID(), null); case ManiphestTransaction::TYPE_CCS: return array_values(array_unique($object->getCCPHIDs())); - case ManiphestTransaction::TYPE_PROJECTS: - return array_values(array_unique($object->getProjectPHIDs())); case ManiphestTransaction::TYPE_PROJECT_COLUMN: // These are pre-populated. return $xaction->getOldValue(); @@ -74,7 +71,6 @@ final class ManiphestTransactionEditor case ManiphestTransaction::TYPE_PRIORITY: return (int)$xaction->getNewValue(); case ManiphestTransaction::TYPE_CCS: - case ManiphestTransaction::TYPE_PROJECTS: return array_values(array_unique($xaction->getNewValue())); case ManiphestTransaction::TYPE_OWNER: return nonempty($xaction->getNewValue(), null); @@ -97,7 +93,6 @@ final class ManiphestTransactionEditor $new = $xaction->getNewValue(); switch ($xaction->getTransactionType()) { - case ManiphestTransaction::TYPE_PROJECTS: case ManiphestTransaction::TYPE_CCS: sort($old); sort($new); @@ -149,11 +144,6 @@ final class ManiphestTransactionEditor return $object->setOwnerPHID($phid); case ManiphestTransaction::TYPE_CCS: return $object->setCCPHIDs($xaction->getNewValue()); - case ManiphestTransaction::TYPE_PROJECTS: - ManiphestTaskProject::updateTaskProjects( - $object, - $xaction->getNewValue()); - return $object; case ManiphestTransaction::TYPE_SUBPRIORITY: $data = $xaction->getNewValue(); $new_sub = $this->getNextSubpriority( @@ -425,15 +415,14 @@ final class ManiphestTransactionEditor $project_phids = $adapter->getProjectPHIDs(); if ($project_phids) { - $existing_projects = $object->getProjectPHIDs(); - $new_projects = array_unique( - array_merge( - $project_phids, - $existing_projects)); - + $project_type = PhabricatorProjectObjectHasProjectEdgeType::EDGECONST; $xactions[] = id(new ManiphestTransaction()) - ->setTransactionType(ManiphestTransaction::TYPE_PROJECTS) - ->setNewValue($new_projects); + ->setTransactionType(PhabricatorTransactions::TYPE_EDGE) + ->setMetadataValue('edge:type', $project_type) + ->setNewValue( + array( + '+' => array_fuse($project_phids), + )); } return $xactions; @@ -450,8 +439,6 @@ final class ManiphestTransactionEditor ManiphestCapabilityEditPriority::CAPABILITY, ManiphestTransaction::TYPE_STATUS => ManiphestCapabilityEditStatus::CAPABILITY, - ManiphestTransaction::TYPE_PROJECTS => - ManiphestCapabilityEditProjects::CAPABILITY, ManiphestTransaction::TYPE_OWNER => ManiphestCapabilityEditAssign::CAPABILITY, PhabricatorTransactions::TYPE_EDIT_POLICY => @@ -460,8 +447,19 @@ final class ManiphestTransactionEditor ManiphestCapabilityEditPolicies::CAPABILITY, ); + $transaction_type = $xaction->getTransactionType(); - $app_capability = idx($app_capability_map, $transaction_type); + + $app_capability = null; + if ($transaction_type == PhabricatorTransactions::TYPE_EDGE) { + switch ($xaction->getMetadataValue('edge:type')) { + case PhabricatorProjectObjectHasProjectEdgeType::EDGECONST: + $app_capability = ManiphestCapabilityEditProjects::CAPABILITY; + break; + } + } else { + $app_capability = idx($app_capability_map, $transaction_type); + } if ($app_capability) { $app = id(new PhabricatorApplicationQuery()) diff --git a/src/applications/maniphest/lipsum/PhabricatorManiphestTaskTestDataGenerator.php b/src/applications/maniphest/lipsum/PhabricatorManiphestTaskTestDataGenerator.php index e4211dc9c9..a3d27ca1c3 100644 --- a/src/applications/maniphest/lipsum/PhabricatorManiphestTaskTestDataGenerator.php +++ b/src/applications/maniphest/lipsum/PhabricatorManiphestTaskTestDataGenerator.php @@ -30,8 +30,6 @@ final class PhabricatorManiphestTaskTestDataGenerator $this->generateTaskPriority(); $changes[ManiphestTransaction::TYPE_CCS] = $this->getCCPHIDs(); - $changes[ManiphestTransaction::TYPE_PROJECTS] = - $this->getProjectPHIDs(); $transactions = array(); foreach ($changes as $type => $value) { $transaction = clone $template; diff --git a/src/applications/maniphest/storage/ManiphestTaskProject.php b/src/applications/maniphest/storage/ManiphestTaskProject.php deleted file mode 100644 index d5d002466b..0000000000 --- a/src/applications/maniphest/storage/ManiphestTaskProject.php +++ /dev/null @@ -1,49 +0,0 @@ - Project table, which denormalizes the - * relationship between tasks and projects into a link table so it can be - * efficiently queried. This table is not authoritative; the projectPHIDs field - * of ManiphestTask is. The rows in this table are regenerated when transactions - * are applied to tasks which affected their associated projects. - */ -final class ManiphestTaskProject extends ManiphestDAO { - - protected $taskPHID; - protected $projectPHID; - - public function getConfiguration() { - return array( - self::CONFIG_IDS => self::IDS_MANUAL, - self::CONFIG_TIMESTAMPS => false, - ); - } - - public static function updateTaskProjects( - ManiphestTask $task, - array $new_phids) { - - $edge_type = PhabricatorProjectObjectHasProjectEdgeType::EDGECONST; - - $old_phids = PhabricatorEdgeQuery::loadDestinationPHIDs( - $task->getPHID(), - $edge_type); - - $add_phids = array_diff($new_phids, $old_phids); - $rem_phids = array_diff($old_phids, $new_phids); - - if (!$add_phids && !$rem_phids) { - return; - } - - $editor = new PhabricatorEdgeEditor(); - foreach ($add_phids as $phid) { - $editor->addEdge($task->getPHID(), $edge_type, $phid); - } - foreach ($rem_phids as $phid) { - $editor->remEdge($task->getPHID(), $edge_type, $phid); - } - $editor->save(); - } - -} diff --git a/src/applications/maniphest/storage/ManiphestTransaction.php b/src/applications/maniphest/storage/ManiphestTransaction.php index 685d56f393..d3185d661c 100644 --- a/src/applications/maniphest/storage/ManiphestTransaction.php +++ b/src/applications/maniphest/storage/ManiphestTransaction.php @@ -788,8 +788,15 @@ final class ManiphestTransaction case self::TYPE_CCS: $tags[] = MetaMTANotificationType::TYPE_MANIPHEST_CC; break; - case self::TYPE_PROJECTS: - $tags[] = MetaMTANotificationType::TYPE_MANIPHEST_PROJECTS; + case PhabricatorTransactions::TYPE_EDGE: + switch ($this->getMetadataValue('edge:type')) { + case PhabricatorProjectObjectHasProjectEdgeType::EDGECONST: + $tags[] = MetaMTANotificationType::TYPE_MANIPHEST_PROJECTS; + break; + default: + $tags[] = MetaMTANotificationType::TYPE_MANIPHEST_OTHER; + break; + } break; case self::TYPE_PRIORITY: $tags[] = MetaMTANotificationType::TYPE_MANIPHEST_PRIORITY;