From 37b1b31638524f1236243245c242aa8469c7f365 Mon Sep 17 00:00:00 2001 From: Bob Trahan Date: Thu, 27 Feb 2014 09:39:59 -0800 Subject: [PATCH] Maniphest - move subpriority edits to be transaction powered Summary: ...this was nice to do for boards, since this diff also starts calling this code in the board move case. The big trick is to use the new expandTransactions hook to expand the subpriority transaction into a priority transaction if pertinent. The other stuff is just about hiding these new subpriority extractions. ...also removes the "edit" UI from the default board since we can't actually edit anything and it thus is buggy. Ref T4422. Next step is to move board edits into the editor with their own little transaction. Test Plan: re-orded tasks on a maniphest query, reloaded, and noted re-order stuck. Reviewers: epriestley Reviewed By: epriestley CC: Korvin, epriestley, aran Maniphest Tasks: T4422 Differential Revision: https://secure.phabricator.com/D8358 --- .../ManiphestSubpriorityController.php | 32 ++++------- .../editor/ManiphestTransactionEditor.php | 53 ++++++++++++++++--- .../storage/ManiphestTransaction.php | 3 ++ .../PhabricatorProjectBoardController.php | 6 ++- .../PhabricatorProjectMoveController.php | 27 +++++++++- src/view/phui/PHUIWorkpanelView.php | 17 +++--- 6 files changed, 101 insertions(+), 37 deletions(-) diff --git a/src/applications/maniphest/controller/ManiphestSubpriorityController.php b/src/applications/maniphest/controller/ManiphestSubpriorityController.php index 1dc95ad674..8ba98c957b 100644 --- a/src/applications/maniphest/controller/ManiphestSubpriorityController.php +++ b/src/applications/maniphest/controller/ManiphestSubpriorityController.php @@ -41,28 +41,18 @@ final class ManiphestSubpriorityController extends ManiphestController { $after_sub = null; } - $new_sub = ManiphestTransactionEditor::getNextSubpriority( - $after_pri, - $after_sub); + $xactions = array(id(new ManiphestTransaction()) + ->setTransactionType(ManiphestTransaction::TYPE_SUBPRIORITY) + ->setNewValue(array( + 'newPriority' => $after_pri, + 'newSubpriorityBase' => $after_sub))); + $editor = id(new ManiphestTransactionEditor()) + ->setActor($user) + ->setContinueOnMissingFields(true) + ->setContinueOnNoEffect(true) + ->setContentSourceFromRequest($request); - $task->setSubpriority($new_sub); - - if ($after_pri != $task->getPriority()) { - $xactions = array(); - $xactions[] = id(new ManiphestTransaction()) - ->setTransactionType(ManiphestTransaction::TYPE_PRIORITY) - ->setNewValue($after_pri); - - $editor = id(new ManiphestTransactionEditor()) - ->setActor($user) - ->setContinueOnMissingFields(true) - ->setContinueOnNoEffect(true) - ->setContentSourceFromRequest($request); - - $editor->applyTransactions($task, $xactions); - } else { - $task->save(); - } + $editor->applyTransactions($task, $xactions); return id(new AphrontAjaxResponse())->setContent( array( diff --git a/src/applications/maniphest/editor/ManiphestTransactionEditor.php b/src/applications/maniphest/editor/ManiphestTransactionEditor.php index 7a99c1cb36..a899192006 100644 --- a/src/applications/maniphest/editor/ManiphestTransactionEditor.php +++ b/src/applications/maniphest/editor/ManiphestTransactionEditor.php @@ -16,6 +16,7 @@ final class ManiphestTransactionEditor $types[] = ManiphestTransaction::TYPE_PROJECTS; $types[] = ManiphestTransaction::TYPE_ATTACH; $types[] = ManiphestTransaction::TYPE_EDGE; + $types[] = ManiphestTransaction::TYPE_SUBPRIORITY; $types[] = PhabricatorTransactions::TYPE_VIEW_POLICY; $types[] = PhabricatorTransactions::TYPE_EDIT_POLICY; @@ -58,6 +59,8 @@ final class ManiphestTransactionEditor case ManiphestTransaction::TYPE_EDGE: // These are pre-populated. return $xaction->getOldValue(); + case ManiphestTransaction::TYPE_SUBPRIORITY: + return $object->getSubpriority(); } } @@ -79,6 +82,7 @@ final class ManiphestTransactionEditor case ManiphestTransaction::TYPE_DESCRIPTION: case ManiphestTransaction::TYPE_ATTACH: case ManiphestTransaction::TYPE_EDGE: + case ManiphestTransaction::TYPE_SUBPRIORITY: return $xaction->getNewValue(); } } @@ -102,7 +106,6 @@ final class ManiphestTransactionEditor return parent::transactionHasEffect($object, $xaction); } - protected function applyCustomInternalTransaction( PhabricatorLiskDAO $object, PhabricatorApplicationTransaction $xaction) { @@ -147,10 +150,39 @@ final class ManiphestTransactionEditor // These are a weird, funky mess and are already being applied by the // time we reach this. return; + case ManiphestTransaction::TYPE_SUBPRIORITY: + $data = $xaction->getNewValue(); + $new_sub = $this->getNextSubpriority( + $data['newPriority'], + $data['newSubpriorityBase']); + $object->setSubpriority($new_sub); + return; } } + protected function expandTransaction( + PhabricatorLiskDAO $object, + PhabricatorApplicationTransaction $xaction) { + + $xactions = parent::expandTransaction($object, $xaction); + switch ($xaction->getTransactionType()) { + case ManiphestTransaction::TYPE_SUBPRIORITY: + $data = $xaction->getNewValue(); + $new_pri = $data['newPriority']; + if ($new_pri != $object->getPriority()) { + $xactions[] = id(new ManiphestTransaction()) + ->setTransactionType(ManiphestTransaction::TYPE_PRIORITY) + ->setNewValue($new_pri); + } + break; + default: + break; + } + + return $xactions; + } + protected function applyCustomExternalTransaction( PhabricatorLiskDAO $object, PhabricatorApplicationTransaction $xaction) { @@ -159,7 +191,19 @@ final class ManiphestTransactionEditor protected function shouldSendMail( PhabricatorLiskDAO $object, array $xactions) { - return true; + $should_mail = true; + if (count($xactions) == 1) { + $xaction = head($xactions); + switch ($xaction->getTransactionType()) { + case ManiphestTransaction::TYPE_SUBPRIORITY: + $should_mail = false; + break; + default: + $should_mail = true; + break; + } + } + return $should_mail; } protected function getMailSubjectPrefix() { @@ -305,10 +349,7 @@ final class ManiphestTransactionEditor } - public static function getNextSubpriority($pri, $sub) { - - // TODO: T603 Figure out what the policies here should be once this gets - // cleaned up. + private function getNextSubpriority($pri, $sub) { if ($sub === null) { $next = id(new ManiphestTask())->loadOneWhere( diff --git a/src/applications/maniphest/storage/ManiphestTransaction.php b/src/applications/maniphest/storage/ManiphestTransaction.php index a66b8500fe..627066cddd 100644 --- a/src/applications/maniphest/storage/ManiphestTransaction.php +++ b/src/applications/maniphest/storage/ManiphestTransaction.php @@ -12,6 +12,7 @@ final class ManiphestTransaction const TYPE_PRIORITY = 'priority'; const TYPE_EDGE = 'edge'; const TYPE_ATTACH = 'attach'; + const TYPE_SUBPRIORITY = 'subpriority'; public function getApplicationName() { return 'maniphest'; @@ -85,6 +86,8 @@ final class ManiphestTransaction return false; } break; + case self::TYPE_SUBPRIORITY: + return true; } return false; diff --git a/src/applications/project/controller/PhabricatorProjectBoardController.php b/src/applications/project/controller/PhabricatorProjectBoardController.php index f7ad05708b..f0e1591f43 100644 --- a/src/applications/project/controller/PhabricatorProjectBoardController.php +++ b/src/applications/project/controller/PhabricatorProjectBoardController.php @@ -96,8 +96,10 @@ final class PhabricatorProjectBoardController foreach ($columns as $column) { $panel = id(new PHUIWorkpanelView()) ->setHeader($column->getDisplayName()) - ->setHeaderColor($column->getHeaderColor()) - ->setEditURI('edit/'.$column->getID().'/'); + ->setHeaderColor($column->getHeaderColor()); + if (!$column->isDefaultColumn()) { + $panel->setEditURI('edit/'.$column->getID().'/'); + } $cards = id(new PHUIObjectItemListView()) ->setUser($viewer) diff --git a/src/applications/project/controller/PhabricatorProjectMoveController.php b/src/applications/project/controller/PhabricatorProjectMoveController.php index 7478a7dc79..be17d3362d 100644 --- a/src/applications/project/controller/PhabricatorProjectMoveController.php +++ b/src/applications/project/controller/PhabricatorProjectMoveController.php @@ -72,8 +72,31 @@ final class PhabricatorProjectMoveController $column_phid, $edge_phids); - // TODO: We also need to deal with priorities, so far this only gets stuff - // in the correct column. + if ($after_phid) { + $after_task = id(new ManiphestTaskQuery()) + ->setViewer($viewer) + ->withPHIDs(array($after_phid)) + ->requireCapabilities(array(PhabricatorPolicyCapability::CAN_EDIT)) + ->executeOne(); + if (!$after_task) { + return new Aphront404Response(); + } + $after_pri = $after_task->getPriority(); + $after_sub = $after_task->getSubpriority(); + + $xactions = array(id(new ManiphestTransaction()) + ->setTransactionType(ManiphestTransaction::TYPE_SUBPRIORITY) + ->setNewValue(array( + 'newPriority' => $after_pri, + 'newSubpriorityBase' => $after_sub))); + $editor = id(new ManiphestTransactionEditor()) + ->setActor($viewer) + ->setContinueOnMissingFields(true) + ->setContinueOnNoEffect(true) + ->setContentSourceFromRequest($request); + + $editor->applyTransactions($object, $xactions); + } return id(new AphrontAjaxResponse())->setContent(array()); } diff --git a/src/view/phui/PHUIWorkpanelView.php b/src/view/phui/PHUIWorkpanelView.php index 2d4b946c1a..dd80edd587 100644 --- a/src/view/phui/PHUIWorkpanelView.php +++ b/src/view/phui/PHUIWorkpanelView.php @@ -53,15 +53,20 @@ final class PHUIWorkpanelView extends AphrontTagView { $footer_tag); } - $header_edit = id(new PHUIIconView()) - ->setSpriteSheet(PHUIIconView::SPRITE_ACTIONS) - ->setSpriteIcon('settings-grey') - ->setHref($this->editURI); + $header_edit = null; + if ($this->editURI) { + $header_edit = id(new PHUIIconView()) + ->setSpriteSheet(PHUIIconView::SPRITE_ACTIONS) + ->setSpriteIcon('settings-grey') + ->setHref($this->editURI); + } $header = id(new PhabricatorActionHeaderView()) ->setHeaderTitle($this->header) - ->setHeaderColor($this->headerColor) - ->addAction($header_edit); + ->setHeaderColor($this->headerColor); + if ($header_edit) { + $header->addAction($header_edit); + } $body = phutil_tag( 'div',