From 9e096cd274b0e22e3c8048ef847ef61be4a6f147 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 29 Jun 2019 15:23:58 -0700 Subject: [PATCH] Give the workboard "default" workflows more modern state handling Summary: Depends on D20628. Ref T4900. Currently, the "Save Current Order/Filter As Default" flows on workboards duplicate some state construction, and require parameters to be passed to them explicitly. Now that state management is separate, they can reuse a bit more code and be made to look more like other similar controllers. Test Plan: - Changed the default order of a workboard. - Changed the default filter of a workboard. - Changed the order of a board to something non-default, then changed the filter, then saved the new filter as the default. Saw the modified order preserved and the modified filter removed, so I ended up in the right ("most correct") place: on the board, with my custom order in a URI parameter, and no filter URI parameter so I could see my new default filter behavior. This is an edge case that's not terribly important to get right, but we do get it right. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T4900 Differential Revision: https://secure.phabricator.com/D20629 --- src/__phutil_library_map__.php | 4 +- .../PhabricatorProjectApplication.php | 4 +- .../PhabricatorProjectBoardController.php | 13 ++++- ...bricatorProjectBoardDefaultController.php} | 55 ++++++++----------- .../PhabricatorProjectBoardViewController.php | 10 ++-- .../PhabricatorProjectController.php | 16 ++++++ .../state/PhabricatorWorkboardViewState.php | 4 ++ 7 files changed, 63 insertions(+), 43 deletions(-) rename src/applications/project/controller/{PhabricatorProjectDefaultController.php => PhabricatorProjectBoardDefaultController.php} (60%) diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 7f0ef2afde..ff0f462807 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -4158,6 +4158,7 @@ phutil_register_library_map(array( 'PhabricatorProjectArchiveController' => 'applications/project/controller/PhabricatorProjectArchiveController.php', 'PhabricatorProjectBoardBackgroundController' => 'applications/project/controller/PhabricatorProjectBoardBackgroundController.php', 'PhabricatorProjectBoardController' => 'applications/project/controller/PhabricatorProjectBoardController.php', + 'PhabricatorProjectBoardDefaultController' => 'applications/project/controller/PhabricatorProjectBoardDefaultController.php', 'PhabricatorProjectBoardDisableController' => 'applications/project/controller/PhabricatorProjectBoardDisableController.php', 'PhabricatorProjectBoardImportController' => 'applications/project/controller/PhabricatorProjectBoardImportController.php', 'PhabricatorProjectBoardManageController' => 'applications/project/controller/PhabricatorProjectBoardManageController.php', @@ -4207,7 +4208,6 @@ phutil_register_library_map(array( 'PhabricatorProjectCustomFieldStringIndex' => 'applications/project/storage/PhabricatorProjectCustomFieldStringIndex.php', 'PhabricatorProjectDAO' => 'applications/project/storage/PhabricatorProjectDAO.php', 'PhabricatorProjectDatasource' => 'applications/project/typeahead/PhabricatorProjectDatasource.php', - 'PhabricatorProjectDefaultController' => 'applications/project/controller/PhabricatorProjectDefaultController.php', 'PhabricatorProjectDescriptionField' => 'applications/project/customfield/PhabricatorProjectDescriptionField.php', 'PhabricatorProjectDetailsProfileMenuItem' => 'applications/project/menuitem/PhabricatorProjectDetailsProfileMenuItem.php', 'PhabricatorProjectDropEffect' => 'applications/project/icon/PhabricatorProjectDropEffect.php', @@ -10422,6 +10422,7 @@ phutil_register_library_map(array( 'PhabricatorProjectArchiveController' => 'PhabricatorProjectController', 'PhabricatorProjectBoardBackgroundController' => 'PhabricatorProjectBoardController', 'PhabricatorProjectBoardController' => 'PhabricatorProjectController', + 'PhabricatorProjectBoardDefaultController' => 'PhabricatorProjectBoardController', 'PhabricatorProjectBoardDisableController' => 'PhabricatorProjectBoardController', 'PhabricatorProjectBoardImportController' => 'PhabricatorProjectBoardController', 'PhabricatorProjectBoardManageController' => 'PhabricatorProjectBoardController', @@ -10484,7 +10485,6 @@ phutil_register_library_map(array( 'PhabricatorProjectCustomFieldStringIndex' => 'PhabricatorCustomFieldStringIndexStorage', 'PhabricatorProjectDAO' => 'PhabricatorLiskDAO', 'PhabricatorProjectDatasource' => 'PhabricatorTypeaheadDatasource', - 'PhabricatorProjectDefaultController' => 'PhabricatorProjectBoardController', 'PhabricatorProjectDescriptionField' => 'PhabricatorProjectStandardCustomField', 'PhabricatorProjectDetailsProfileMenuItem' => 'PhabricatorProfileMenuItem', 'PhabricatorProjectDropEffect' => 'Phobject', diff --git a/src/applications/project/application/PhabricatorProjectApplication.php b/src/applications/project/application/PhabricatorProjectApplication.php index d8e78c5f7d..75df0d9a10 100644 --- a/src/applications/project/application/PhabricatorProjectApplication.php +++ b/src/applications/project/application/PhabricatorProjectApplication.php @@ -90,6 +90,8 @@ final class PhabricatorProjectApplication extends PhabricatorApplication { => 'PhabricatorProjectBoardManageController', 'background/' => 'PhabricatorProjectBoardBackgroundController', + 'default/(?P[^/]+)/' + => 'PhabricatorProjectBoardDefaultController', ), 'column/' => array( 'remove/(?P\d+)/' => @@ -112,8 +114,6 @@ final class PhabricatorProjectApplication extends PhabricatorApplication { => 'PhabricatorProjectSilenceController', 'warning/(?P[1-9]\d*)/' => 'PhabricatorProjectSubprojectWarningController', - 'default/(?P[1-9]\d*)/(?P[^/]+)/' - => 'PhabricatorProjectDefaultController', ), '/tag/' => array( '(?P[^/]+)/' => 'PhabricatorProjectViewController', diff --git a/src/applications/project/controller/PhabricatorProjectBoardController.php b/src/applications/project/controller/PhabricatorProjectBoardController.php index 394f8a2fb8..73c0d9e307 100644 --- a/src/applications/project/controller/PhabricatorProjectBoardController.php +++ b/src/applications/project/controller/PhabricatorProjectBoardController.php @@ -13,7 +13,7 @@ abstract class PhabricatorProjectBoardController return $this->viewState; } - final private function newViewState() { + private function newViewState() { $project = $this->getProject(); $request = $this->getRequest(); @@ -22,4 +22,15 @@ abstract class PhabricatorProjectBoardController ->readFromRequest($request); } + final protected function newBoardDialog() { + $dialog = $this->newDialog(); + + $state = $this->getViewState(); + foreach ($state->getQueryParameters() as $key => $value) { + $dialog->addHiddenInput($key, $value); + } + + return $dialog; + } + } diff --git a/src/applications/project/controller/PhabricatorProjectDefaultController.php b/src/applications/project/controller/PhabricatorProjectBoardDefaultController.php similarity index 60% rename from src/applications/project/controller/PhabricatorProjectDefaultController.php rename to src/applications/project/controller/PhabricatorProjectBoardDefaultController.php index 2c7a47b2df..5248f7f8b3 100644 --- a/src/applications/project/controller/PhabricatorProjectDefaultController.php +++ b/src/applications/project/controller/PhabricatorProjectBoardDefaultController.php @@ -1,25 +1,20 @@ getViewer(); - $project_id = $request->getURIData('projectID'); - $project = id(new PhabricatorProjectQuery()) - ->setViewer($viewer) - ->requireCapabilities( - array( - PhabricatorPolicyCapability::CAN_VIEW, - PhabricatorPolicyCapability::CAN_EDIT, - )) - ->withIDs(array($project_id)) - ->executeOne(); - if (!$project) { - return new Aphront404Response(); + $response = $this->loadProjectForEdit(); + if ($response) { + return $response; } - $this->setProject($project); + + $project = $this->getProject(); + $state = $this->getViewState(); + $board_uri = $state->newWorkboardURI(); + $remove_param = null; $target = $request->getURIData('target'); switch ($target) { @@ -31,8 +26,10 @@ final class PhabricatorProjectDefaultController 'the board.'); $button = pht('Save Default Filter'); - $xaction_value = $request->getStr('filter'); + $xaction_value = $state->getQueryKey(); $xaction_type = PhabricatorProjectFilterTransaction::TRANSACTIONTYPE; + + $remove_param = 'filter'; break; case 'sort': $title = pht('Set Board Default Order'); @@ -42,8 +39,10 @@ final class PhabricatorProjectDefaultController 'the board.'); $button = pht('Save Default Order'); - $xaction_value = $request->getStr('order'); + $xaction_value = $state->getOrder(); $xaction_type = PhabricatorProjectSortTransaction::TRANSACTIONTYPE; + + $remove_param = 'order'; break; default: return new Aphront404Response(); @@ -51,12 +50,6 @@ final class PhabricatorProjectDefaultController $id = $project->getID(); - $view_uri = $this->getApplicationURI("board/{$id}/"); - $view_uri = new PhutilURI($view_uri); - foreach ($request->getPassthroughRequestData() as $key => $value) { - $view_uri->replaceQueryParam($key, $value); - } - if ($request->isFormPost()) { $xactions = array(); @@ -71,20 +64,18 @@ final class PhabricatorProjectDefaultController ->setContinueOnMissingFields(true) ->applyTransactions($project, $xactions); - return id(new AphrontRedirectResponse())->setURI($view_uri); + // If the parameter we just modified is present in the query string, + // throw it away so the user is redirected back to the default view of + // the board, allowing them to see the new default behavior. + $board_uri->removeQueryParam($remove_param); + + return id(new AphrontRedirectResponse())->setURI($board_uri); } - $dialog = $this->newDialog() + return $this->newBoardDialog() ->setTitle($title) ->appendChild($body) - ->setDisableWorkflowOnCancel(true) - ->addCancelButton($view_uri) + ->addCancelButton($board_uri) ->addSubmitButton($title); - - foreach ($request->getPassthroughRequestData() as $key => $value) { - $dialog->addHiddenInput($key, $value); - } - - return $dialog; } } diff --git a/src/applications/project/controller/PhabricatorProjectBoardViewController.php b/src/applications/project/controller/PhabricatorProjectBoardViewController.php index f06fd9c52d..d1d41e926a 100644 --- a/src/applications/project/controller/PhabricatorProjectBoardViewController.php +++ b/src/applications/project/controller/PhabricatorProjectBoardViewController.php @@ -797,9 +797,7 @@ final class PhabricatorProjectBoardViewController $id = $project->getID(); - $save_uri = "default/{$id}/sort/"; - $save_uri = $this->getApplicationURI($save_uri); - $save_uri = $this->getURIWithState($save_uri, $force = true); + $save_uri = $state->newWorkboardURI('default/sort/'); $can_edit = PhabricatorPolicyFilter::hasCapability( $viewer, @@ -842,6 +840,8 @@ final class PhabricatorProjectBoardViewController PhabricatorApplicationSearchEngine $engine, $query_key) { + $state = $this->getViewState(); + $named = array( 'open' => pht('Open Tasks'), 'all' => pht('All Tasks'), @@ -898,9 +898,7 @@ final class PhabricatorProjectBoardViewController ->setWorkflow(true) ->setName(pht('Advanced Filter...')); - $save_uri = "default/{$id}/filter/"; - $save_uri = $this->getApplicationURI($save_uri); - $save_uri = $this->getURIWithState($save_uri, $force = true); + $save_uri = $state->newWorkboardURI('default/filter/'); $can_edit = PhabricatorPolicyFilter::hasCapability( $viewer, diff --git a/src/applications/project/controller/PhabricatorProjectController.php b/src/applications/project/controller/PhabricatorProjectController.php index 8551a09cbf..4391a61ff3 100644 --- a/src/applications/project/controller/PhabricatorProjectController.php +++ b/src/applications/project/controller/PhabricatorProjectController.php @@ -16,6 +16,21 @@ abstract class PhabricatorProjectController extends PhabricatorController { } protected function loadProject() { + return $this->loadProjectWithCapabilities( + array( + PhabricatorPolicyCapability::CAN_VIEW, + )); + } + + protected function loadProjectForEdit() { + return $this->loadProjectWithCapabilities( + array( + PhabricatorPolicyCapability::CAN_VIEW, + PhabricatorPolicyCapability::CAN_EDIT, + )); + } + + private function loadProjectWithCapabilities(array $capabilities) { $viewer = $this->getViewer(); $request = $this->getRequest(); @@ -35,6 +50,7 @@ abstract class PhabricatorProjectController extends PhabricatorController { $query = id(new PhabricatorProjectQuery()) ->setViewer($viewer) + ->requireCapabilities($capabilities) ->needMembers(true) ->needWatchers(true) ->needImages(true) diff --git a/src/applications/project/state/PhabricatorWorkboardViewState.php b/src/applications/project/state/PhabricatorWorkboardViewState.php index 6a037d1400..d3f1e87676 100644 --- a/src/applications/project/state/PhabricatorWorkboardViewState.php +++ b/src/applications/project/state/PhabricatorWorkboardViewState.php @@ -147,4 +147,8 @@ final class PhabricatorWorkboardViewState return 'open'; } + public function getQueryParameters() { + return $this->requestState; + } + }