From c0d42a89430a655f48e30adc1dbc2085c4a904e6 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 2 May 2016 05:35:05 -0700 Subject: [PATCH] Split Repository EditEngine form into smaller pages Summary: Ref T10748. This allows an EditEngine form to be broken up into pages. This is less powerful than `PHUIPagedFormView`, because the pages are not sequential / stateful. Each form saves immediately once it's submitted, and can not take you to a new form or back/forward in a series of forms. For example, you can't create a workflow where the user fills out 5 pages of information before we create an object, like the current repository workflow does. However, the only place we've ever wanted to do this is repositories and it's fairly bad there, so I feel reasonably confident we aren't going to miss this in the future. (We do "choose a type of service/repository/rule -> fill out one page of info" fairly often, but can do this without the full-power paging stuff.) Test Plan: - Created a repository usin the new Manage UI, filling out only a handful of fields. - Edited a repository using the new Manage UI. - All forms are now EditEngine forms offering paged views of the big huge underlying form: {F1254371} Reviewers: chad Reviewed By: chad Maniphest Tasks: T10748 Differential Revision: https://secure.phabricator.com/D15832 --- src/__phutil_library_map__.php | 2 + .../base/PhabricatorApplication.php | 6 +- ...usionRepositoryEditDangerousController.php | 26 +++- .../editor/DiffusionRepositoryEditEngine.php | 42 ++++++ ...fusionRepositoryActionsManagementPanel.php | 9 +- ...ionRepositoryAutomationManagementPanel.php | 8 +- ...ffusionRepositoryBasicsManagementPanel.php | 13 +- ...usionRepositoryBranchesManagementPanel.php | 10 +- .../DiffusionRepositoryManagementPanel.php | 38 ++++++ ...usionRepositoryPoliciesManagementPanel.php | 10 +- ...fusionRepositoryStagingManagementPanel.php | 8 +- ...fusionRepositorySymbolsManagementPanel.php | 9 +- .../editengine/PhabricatorEditEngine.php | 123 +++++++++++++++++- .../editengine/PhabricatorEditPage.php | 58 +++++++++ 14 files changed, 342 insertions(+), 20 deletions(-) create mode 100644 src/applications/transactions/editengine/PhabricatorEditPage.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 537bc69221..ea12d54c1d 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2358,6 +2358,7 @@ phutil_register_library_map(array( 'PhabricatorEditEngineSelectCommentAction' => 'applications/transactions/commentaction/PhabricatorEditEngineSelectCommentAction.php', 'PhabricatorEditEngineTokenizerCommentAction' => 'applications/transactions/commentaction/PhabricatorEditEngineTokenizerCommentAction.php', 'PhabricatorEditField' => 'applications/transactions/editfield/PhabricatorEditField.php', + 'PhabricatorEditPage' => 'applications/transactions/editengine/PhabricatorEditPage.php', 'PhabricatorEditType' => 'applications/transactions/edittype/PhabricatorEditType.php', 'PhabricatorEditor' => 'infrastructure/PhabricatorEditor.php', 'PhabricatorElasticFulltextStorageEngine' => 'applications/search/fulltextstorage/PhabricatorElasticFulltextStorageEngine.php', @@ -6872,6 +6873,7 @@ phutil_register_library_map(array( 'PhabricatorEditEngineSelectCommentAction' => 'PhabricatorEditEngineCommentAction', 'PhabricatorEditEngineTokenizerCommentAction' => 'PhabricatorEditEngineCommentAction', 'PhabricatorEditField' => 'Phobject', + 'PhabricatorEditPage' => 'Phobject', 'PhabricatorEditType' => 'Phobject', 'PhabricatorEditor' => 'Phobject', 'PhabricatorElasticFulltextStorageEngine' => 'PhabricatorFulltextStorageEngine', diff --git a/src/applications/base/PhabricatorApplication.php b/src/applications/base/PhabricatorApplication.php index 0bbb38ff89..a667717dd9 100644 --- a/src/applications/base/PhabricatorApplication.php +++ b/src/applications/base/PhabricatorApplication.php @@ -624,11 +624,11 @@ abstract class PhabricatorApplication '(?P[0-9]\d*)/)?'. '(?:'. '(?:'. - '(?Pparameters|nodefault|nocreate|nomanage|comment)'. + '(?Pparameters|nodefault|nocreate|nomanage|comment)/'. '|'. - '(?:form/(?P[^/]+))'. + '(?:form/(?P[^/]+)/)?(?:page/(?P[^/]+)/)?'. ')'. - '/)?'; + ')?'; } protected function getQueryRoutePattern($base = null) { diff --git a/src/applications/diffusion/controller/DiffusionRepositoryEditDangerousController.php b/src/applications/diffusion/controller/DiffusionRepositoryEditDangerousController.php index 26d8b57f33..f2a49f820a 100644 --- a/src/applications/diffusion/controller/DiffusionRepositoryEditDangerousController.php +++ b/src/applications/diffusion/controller/DiffusionRepositoryEditDangerousController.php @@ -13,12 +13,30 @@ final class DiffusionRepositoryEditDangerousController $drequest = $this->getDiffusionRequest(); $repository = $drequest->getRepository(); - if (!$repository->canAllowDangerousChanges()) { - return new Aphront400Response(); - } - $edit_uri = $this->getRepositoryControllerURI($repository, 'edit/'); + if (!$repository->canAllowDangerousChanges()) { + if ($repository->isSVN()) { + return $this->newDialog() + ->setTitle(pht('Not in Danger')) + ->appendParagraph( + pht( + 'It is not possible for users to push any dangerous changes '. + 'to a Subversion repository. Pushes to a Subversion repository '. + 'can always be reverted and never destroy data.')) + ->addCancelButton($edit_uri); + } else { + return $this->newDialog() + ->setTitle(pht('Unprotectable Repository')) + ->appendParagraph( + pht( + 'This repository can not be protected from dangerous changes '. + 'because Phabricator does not control what users are allowed '. + 'to push to it.')) + ->addCancelButton($edit_uri); + } + } + if ($request->isFormPost()) { $xaction = id(new PhabricatorRepositoryTransaction()) ->setTransactionType(PhabricatorRepositoryTransaction::TYPE_DANGEROUS) diff --git a/src/applications/diffusion/editor/DiffusionRepositoryEditEngine.php b/src/applications/diffusion/editor/DiffusionRepositoryEditEngine.php index 1ebbabe2ed..f6ba777edc 100644 --- a/src/applications/diffusion/editor/DiffusionRepositoryEditEngine.php +++ b/src/applications/diffusion/editor/DiffusionRepositoryEditEngine.php @@ -85,6 +85,48 @@ final class DiffusionRepositoryEditEngine DiffusionCreateRepositoriesCapability::CAPABILITY); } + protected function newPages($object) { + $panels = DiffusionRepositoryManagementPanel::getAllPanels(); + + $pages = array(); + $uris = array(); + foreach ($panels as $panel_key => $panel) { + $panel->setRepository($object); + + $uris[$panel_key] = $panel->getPanelURI(); + + $page = $panel->newEditEnginePage(); + if (!$page) { + continue; + } + $pages[] = $page; + } + + $basics_key = DiffusionRepositoryBasicsManagementPanel::PANELKEY; + $basics_uri = $uris[$basics_key]; + + $more_pages = array( + id(new PhabricatorEditPage()) + ->setKey('encoding') + ->setLabel(pht('Text Encoding')) + ->setViewURI($basics_uri) + ->setFieldKeys( + array( + 'encoding', + )), + id(new PhabricatorEditPage()) + ->setKey('extensions') + ->setLabel(pht('Extensions')) + ->setIsDefault(true), + ); + + foreach ($more_pages as $page) { + $pages[] = $page; + } + + return $pages; + } + protected function buildCustomEditFields($object) { $viewer = $this->getViewer(); diff --git a/src/applications/diffusion/management/DiffusionRepositoryActionsManagementPanel.php b/src/applications/diffusion/management/DiffusionRepositoryActionsManagementPanel.php index 4b9fa5b528..14bcc250d9 100644 --- a/src/applications/diffusion/management/DiffusionRepositoryActionsManagementPanel.php +++ b/src/applications/diffusion/management/DiffusionRepositoryActionsManagementPanel.php @@ -13,6 +13,13 @@ final class DiffusionRepositoryActionsManagementPanel return 1100; } + protected function getEditEngineFieldKeys() { + return array( + 'publish', + 'autoclose', + ); + } + protected function buildManagementPanelActions() { $repository = $this->getRepository(); $viewer = $this->getViewer(); @@ -22,7 +29,7 @@ final class DiffusionRepositoryActionsManagementPanel $repository, PhabricatorPolicyCapability::CAN_EDIT); - $actions_uri = $repository->getPathURI('edit/actions/'); + $actions_uri = $this->getEditPageURI(); return array( id(new PhabricatorActionView()) diff --git a/src/applications/diffusion/management/DiffusionRepositoryAutomationManagementPanel.php b/src/applications/diffusion/management/DiffusionRepositoryAutomationManagementPanel.php index beed039c4f..911a594d05 100644 --- a/src/applications/diffusion/management/DiffusionRepositoryAutomationManagementPanel.php +++ b/src/applications/diffusion/management/DiffusionRepositoryAutomationManagementPanel.php @@ -13,6 +13,12 @@ final class DiffusionRepositoryAutomationManagementPanel return 800; } + protected function getEditEngineFieldKeys() { + return array( + 'automationBlueprintPHIDs', + ); + } + protected function buildManagementPanelActions() { $repository = $this->getRepository(); $viewer = $this->getViewer(); @@ -24,7 +30,7 @@ final class DiffusionRepositoryAutomationManagementPanel $can_test = $can_edit && $repository->canPerformAutomation(); - $automation_uri = $repository->getPathURI('edit/automation/'); + $automation_uri = $this->getEditPageURI(); $test_uri = $repository->getPathURI('edit/testautomation/'); return array( diff --git a/src/applications/diffusion/management/DiffusionRepositoryBasicsManagementPanel.php b/src/applications/diffusion/management/DiffusionRepositoryBasicsManagementPanel.php index 7ddb38d34c..40ba95045a 100644 --- a/src/applications/diffusion/management/DiffusionRepositoryBasicsManagementPanel.php +++ b/src/applications/diffusion/management/DiffusionRepositoryBasicsManagementPanel.php @@ -13,6 +13,15 @@ final class DiffusionRepositoryBasicsManagementPanel return 100; } + protected function getEditEngineFieldKeys() { + return array( + 'name', + 'callsign', + 'shortName', + 'description', + ); + } + protected function buildManagementPanelActions() { $repository = $this->getRepository(); $viewer = $this->getViewer(); @@ -22,10 +31,10 @@ final class DiffusionRepositoryBasicsManagementPanel $repository, PhabricatorPolicyCapability::CAN_EDIT); - $edit_uri = $repository->getPathURI('manage/'); + $edit_uri = $this->getEditPageURI(); $activate_uri = $repository->getPathURI('edit/activate/'); $delete_uri = $repository->getPathURI('edit/delete/'); - $encoding_uri = $repository->getPathURI('edit/encoding/'); + $encoding_uri = $this->getEditPageURI('encoding'); $dangerous_uri = $repository->getPathURI('edit/dangerous/'); if ($repository->isTracked()) { diff --git a/src/applications/diffusion/management/DiffusionRepositoryBranchesManagementPanel.php b/src/applications/diffusion/management/DiffusionRepositoryBranchesManagementPanel.php index 54184cd8ba..4ba7eed73c 100644 --- a/src/applications/diffusion/management/DiffusionRepositoryBranchesManagementPanel.php +++ b/src/applications/diffusion/management/DiffusionRepositoryBranchesManagementPanel.php @@ -13,6 +13,14 @@ final class DiffusionRepositoryBranchesManagementPanel return 1000; } + protected function getEditEngineFieldKeys() { + return array( + 'defaultBranch', + 'trackOnly', + 'autocloseOnly', + ); + } + protected function buildManagementPanelActions() { $repository = $this->getRepository(); $viewer = $this->getViewer(); @@ -22,7 +30,7 @@ final class DiffusionRepositoryBranchesManagementPanel $repository, PhabricatorPolicyCapability::CAN_EDIT); - $branches_uri = $repository->getPathURI('edit/branches/'); + $branches_uri = $this->getEditPageURI(); return array( id(new PhabricatorActionView()) diff --git a/src/applications/diffusion/management/DiffusionRepositoryManagementPanel.php b/src/applications/diffusion/management/DiffusionRepositoryManagementPanel.php index f278c3323a..a8f68d71b9 100644 --- a/src/applications/diffusion/management/DiffusionRepositoryManagementPanel.php +++ b/src/applications/diffusion/management/DiffusionRepositoryManagementPanel.php @@ -98,4 +98,42 @@ abstract class DiffusionRepositoryManagementPanel return $this->controller->newTimeline($this->getRepository()); } + final public function getPanelURI() { + $repository = $this->getRepository(); + $key = $this->getManagementPanelKey(); + return $repository->getPathURI("manage/{$key}/"); + } + + final public function newEditEnginePage() { + $field_keys = $this->getEditEngineFieldKeys(); + if (!$field_keys) { + return null; + } + + $key = $this->getManagementPanelKey(); + $label = $this->getManagementPanelLabel(); + $panel_uri = $this->getPanelURI(); + + return id(new PhabricatorEditPage()) + ->setKey($key) + ->setLabel($label) + ->setViewURI($panel_uri) + ->setFieldKeys($field_keys); + } + + protected function getEditEngineFieldKeys() { + return array(); + } + + protected function getEditPageURI($page = null) { + if ($page === null) { + $page = $this->getManagementPanelKey(); + } + + $repository = $this->getRepository(); + $id = $repository->getID(); + return "/diffusion/editpro/{$id}/page/{$page}/"; + } + + } diff --git a/src/applications/diffusion/management/DiffusionRepositoryPoliciesManagementPanel.php b/src/applications/diffusion/management/DiffusionRepositoryPoliciesManagementPanel.php index 9bfe572070..7cca25d95c 100644 --- a/src/applications/diffusion/management/DiffusionRepositoryPoliciesManagementPanel.php +++ b/src/applications/diffusion/management/DiffusionRepositoryPoliciesManagementPanel.php @@ -13,6 +13,14 @@ final class DiffusionRepositoryPoliciesManagementPanel return 300; } + protected function getEditEngineFieldKeys() { + return array( + 'policy.view', + 'policy.edit', + 'policy.push', + ); + } + protected function buildManagementPanelActions() { $repository = $this->getRepository(); $viewer = $this->getViewer(); @@ -22,7 +30,7 @@ final class DiffusionRepositoryPoliciesManagementPanel $repository, PhabricatorPolicyCapability::CAN_EDIT); - $edit_uri = $repository->getPathURI('manage/'); + $edit_uri = $this->getEditPageURI(); return array( id(new PhabricatorActionView()) diff --git a/src/applications/diffusion/management/DiffusionRepositoryStagingManagementPanel.php b/src/applications/diffusion/management/DiffusionRepositoryStagingManagementPanel.php index bff0420193..09cc3c58c4 100644 --- a/src/applications/diffusion/management/DiffusionRepositoryStagingManagementPanel.php +++ b/src/applications/diffusion/management/DiffusionRepositoryStagingManagementPanel.php @@ -13,6 +13,12 @@ final class DiffusionRepositoryStagingManagementPanel return 700; } + protected function getEditEngineFieldKeys() { + return array( + 'stagingAreaURI', + ); + } + protected function buildManagementPanelActions() { $repository = $this->getRepository(); $viewer = $this->getViewer(); @@ -22,7 +28,7 @@ final class DiffusionRepositoryStagingManagementPanel $repository, PhabricatorPolicyCapability::CAN_EDIT); - $staging_uri = $repository->getPathURI('edit/staging/'); + $staging_uri = $this->getEditPageURI(); return array( id(new PhabricatorActionView()) diff --git a/src/applications/diffusion/management/DiffusionRepositorySymbolsManagementPanel.php b/src/applications/diffusion/management/DiffusionRepositorySymbolsManagementPanel.php index 4e934c67af..3a9d94a54d 100644 --- a/src/applications/diffusion/management/DiffusionRepositorySymbolsManagementPanel.php +++ b/src/applications/diffusion/management/DiffusionRepositorySymbolsManagementPanel.php @@ -13,6 +13,13 @@ final class DiffusionRepositorySymbolsManagementPanel return 900; } + protected function getEditEngineFieldKeys() { + return array( + 'symbolLanguages', + 'symbolRepositoryPHIDs', + ); + } + protected function buildManagementPanelActions() { $repository = $this->getRepository(); $viewer = $this->getViewer(); @@ -22,7 +29,7 @@ final class DiffusionRepositorySymbolsManagementPanel $repository, PhabricatorPolicyCapability::CAN_EDIT); - $symbols_uri = $repository->getPathURI('edit/symbols/'); + $symbols_uri = $this->getEditPageURI(); return array( id(new PhabricatorActionView()) diff --git a/src/applications/transactions/editengine/PhabricatorEditEngine.php b/src/applications/transactions/editengine/PhabricatorEditEngine.php index 7c9c0fbed1..f67860f938 100644 --- a/src/applications/transactions/editengine/PhabricatorEditEngine.php +++ b/src/applications/transactions/editengine/PhabricatorEditEngine.php @@ -24,6 +24,8 @@ abstract class PhabricatorEditEngine private $editEngineConfiguration; private $contextParameters = array(); private $targetObject; + private $page; + private $pages; final public function setViewer(PhabricatorUser $viewer) { $this->viewer = $viewer; @@ -146,6 +148,8 @@ abstract class PhabricatorEditEngine $fields = $this->willConfigureFields($object, $fields); $fields = $config->applyConfigurationToFields($this, $object, $fields); + $fields = $this->applyPageToFields($object, $fields); + return $fields; } @@ -492,6 +496,30 @@ abstract class PhabricatorEditEngine return implode('', $parts); } + public function getEffectiveObjectViewURI($object) { + $page = $this->getSelectedPage(); + if ($page) { + $view_uri = $page->getViewURI(); + if ($view_uri !== null) { + return $view_uri; + } + } + + return $this->getObjectViewURI($object); + } + + public function getEffectiveObjectEditCancelURI($object) { + $page = $this->getSelectedPage(); + if ($page) { + $view_uri = $page->getViewURI(); + if ($view_uri !== null) { + return $view_uri; + } + } + + return $this->getObjectEditCancelURI($object); + } + /* -( Creating and Loading Objects )--------------------------------------- */ @@ -810,6 +838,14 @@ abstract class PhabricatorEditEngine return $this->buildDisabledFormResponse($object, $config); } + $page_key = $request->getURIData('pageKey'); + if (strlen($page_key)) { + $page = $this->selectPage($object, $page_key); + if (!$page) { + return new Aphront404Response(); + } + } + switch ($action) { case 'parameters': return $this->buildParametersResponse($object); @@ -841,7 +877,7 @@ abstract class PhabricatorEditEngine } else { $crumbs->addTextCrumb( $this->getObjectEditShortText($object), - $this->getObjectViewURI($object)); + $this->getEffectiveObjectViewURI($object)); $edit_text = pht('Edit'); if ($final) { @@ -1029,7 +1065,7 @@ abstract class PhabricatorEditEngine $cancel_uri = $this->getObjectCreateCancelURI($object); $submit_button = $this->getObjectCreateButtonText($object); } else { - $cancel_uri = $this->getObjectEditCancelURI($object); + $cancel_uri = $this->getEffectiveObjectEditCancelURI($object); $submit_button = $this->getObjectEditButtonText($object); } @@ -1079,7 +1115,7 @@ abstract class PhabricatorEditEngine $object, array $xactions) { return id(new AphrontRedirectResponse()) - ->setURI($this->getObjectViewURI($object)); + ->setURI($this->getEffectiveObjectViewURI($object)); } private function buildEditForm($object, array $fields) { @@ -1103,7 +1139,7 @@ abstract class PhabricatorEditEngine $cancel_uri = $this->getObjectCreateCancelURI($object); $submit_button = $this->getObjectCreateButtonText($object); } else { - $cancel_uri = $this->getObjectEditCancelURI($object); + $cancel_uri = $this->getEffectiveObjectEditCancelURI($object); $submit_button = $this->getObjectEditButtonText($object); } @@ -1547,7 +1583,7 @@ abstract class PhabricatorEditEngine $fields = $this->buildEditFields($object); $is_preview = $request->isPreviewRequest(); - $view_uri = $this->getObjectViewURI($object); + $view_uri = $this->getEffectiveObjectViewURI($object); $template = $object->getApplicationTransactionTemplate(); $comment_template = $template->getApplicationTransactionCommentObject(); @@ -1955,6 +1991,83 @@ abstract class PhabricatorEditEngine PhabricatorPolicyCapability::CAN_EDIT); } +/* -( Form Pages )--------------------------------------------------------- */ + + + public function getSelectedPage() { + return $this->page; + } + + + private function selectPage($object, $page_key) { + $pages = $this->getPages($object); + + if (empty($pages[$page_key])) { + return null; + } + + $this->page = $pages[$page_key]; + return $this->page; + } + + + protected function newPages($object) { + return array(); + } + + + protected function getPages($object) { + if ($this->pages === null) { + $pages = $this->newPages($object); + + assert_instances_of($pages, 'PhabricatorEditPage'); + $pages = mpull($pages, null, 'getKey'); + + $this->pages = $pages; + } + + return $this->pages; + } + + private function applyPageToFields($object, array $fields) { + $pages = $this->getPages($object); + if (!$pages) { + return $fields; + } + + $page_picks = array(); + $default_key = head($pages)->getKey(); + foreach ($pages as $page_key => $page) { + foreach ($page->getFieldKeys() as $field_key) { + $page_picks[$field_key] = $page_key; + } + if ($page->getIsDefault()) { + $default_key = $page_key; + } + } + + $page_map = array_fill_keys(array_keys($pages), array()); + foreach ($fields as $field_key => $field) { + if (isset($page_picks[$field_key])) { + $page_map[$page_picks[$field_key]][$field_key] = $field; + continue; + } + + // TODO: Maybe let the field pick a page to associate itself with so + // extensions can force themselves onto a particular page? + + $page_map[$default_key][$field_key] = $field; + } + + $page = $this->getSelectedPage(); + if (!$page) { + $page = head($pages); + } + + $selected_key = $page->getKey(); + return $page_map[$selected_key]; + } + /* -( PhabricatorPolicyInterface )----------------------------------------- */ diff --git a/src/applications/transactions/editengine/PhabricatorEditPage.php b/src/applications/transactions/editengine/PhabricatorEditPage.php new file mode 100644 index 0000000000..3669ef4494 --- /dev/null +++ b/src/applications/transactions/editengine/PhabricatorEditPage.php @@ -0,0 +1,58 @@ +key = $key; + return $this; + } + + public function getKey() { + return $this->key; + } + + public function setLabel($label) { + $this->label = $label; + return $this; + } + + public function getLabel() { + return $this->label; + } + + public function setFieldKeys(array $field_keys) { + $this->fieldKeys = $field_keys; + return $this; + } + + public function getFieldKeys() { + return $this->fieldKeys; + } + + public function setIsDefault($is_default) { + $this->isDefault = $is_default; + return $this; + } + + public function getIsDefault() { + return $this->isDefault; + } + + public function setViewURI($view_uri) { + $this->viewURI = $view_uri; + return $this; + } + + public function getViewURI() { + return $this->viewURI; + } + +}