From 8113b7691089062013229b7325a48f313065e027 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 20 Jan 2017 11:03:09 -0800 Subject: [PATCH] Validate menu item fields (links, projects, dashboards, applications, forms, etc) Summary: Ref T12128. This adds validation to menu items. This feels a touch flimsy-ish (kind of copy/paste heavy?) but maybe it can be cleaned up a bit once some similar lightweight modular item types (build steps in Harbormaster, blueprints in Drydock) convert. Test Plan: - Tried to create each item with errors (no dashboard, no project, etc). Got appropriate form errors. - Created valid items of each type. Reviewers: chad Reviewed By: chad Maniphest Tasks: T12128 Differential Revision: https://secure.phabricator.com/D17235 --- .../PhabricatorProfileMenuEditEngine.php | 43 ++++++++++++++ .../editor/PhabricatorProfileMenuEditor.php | 35 ++++++++++++ .../PhabricatorApplicationProfileMenuItem.php | 51 ++++++++++++++++- .../PhabricatorDashboardProfileMenuItem.php | 49 +++++++++++++++- .../PhabricatorEditEngineProfileMenuItem.php | 56 ++++++++++++++++++- .../PhabricatorLinkProfileMenuItem.php | 56 ++++++++++++++++++- .../menuitem/PhabricatorProfileMenuItem.php | 37 ++++++++++++ .../PhabricatorProjectProfileMenuItem.php | 48 +++++++++++++++- ...habricatorProfileMenuItemConfiguration.php | 24 ++++++++ .../editengine/PhabricatorEditEngine.php | 4 ++ 10 files changed, 396 insertions(+), 7 deletions(-) diff --git a/src/applications/search/editor/PhabricatorProfileMenuEditEngine.php b/src/applications/search/editor/PhabricatorProfileMenuEditEngine.php index 176938afb6..b21d03cbd1 100644 --- a/src/applications/search/editor/PhabricatorProfileMenuEditEngine.php +++ b/src/applications/search/editor/PhabricatorProfileMenuEditEngine.php @@ -149,4 +149,47 @@ final class PhabricatorProfileMenuEditEngine return $fields; } + protected function getValidationExceptionShortMessage( + PhabricatorApplicationTransactionValidationException $ex, + PhabricatorEditField $field) { + + // Menu item properties all have the same transaction type, so we need + // to make sure errors about a specific property (like the URI for a + // link) are only applied to the field for that particular property. If + // we don't do this, the red error text like "Required" will display + // next to every field. + + $property_type = + PhabricatorProfileMenuItemConfigurationTransaction::TYPE_PROPERTY; + + $xaction_type = $field->getTransactionType(); + if ($xaction_type == $property_type) { + $field_key = $field->getKey(); + foreach ($ex->getErrors() as $error) { + if ($error->getType() !== $xaction_type) { + continue; + } + + $xaction = $error->getTransaction(); + if (!$xaction) { + continue; + } + + $xaction_setting = $xaction->getMetadataValue('property.key'); + if ($xaction_setting != $field_key) { + continue; + } + + $short_message = $error->getShortMessage(); + if ($short_message !== null) { + return $short_message; + } + } + + return null; + } + + return parent::getValidationExceptionShortMessage($ex, $field); + } + } diff --git a/src/applications/search/editor/PhabricatorProfileMenuEditor.php b/src/applications/search/editor/PhabricatorProfileMenuEditor.php index d91bba272b..308cde0db5 100644 --- a/src/applications/search/editor/PhabricatorProfileMenuEditor.php +++ b/src/applications/search/editor/PhabricatorProfileMenuEditor.php @@ -87,4 +87,39 @@ final class PhabricatorProfileMenuEditor return parent::applyCustomExternalTransaction($object, $xaction); } + protected function validateTransaction( + PhabricatorLiskDAO $object, + $type, + array $xactions) { + + $errors = parent::validateTransaction($object, $type, $xactions); + + $actor = $this->getActor(); + $menu_item = $object->getMenuItem(); + $menu_item->setViewer($actor); + + switch ($type) { + case PhabricatorProfileMenuItemConfigurationTransaction::TYPE_PROPERTY: + $key_map = array(); + foreach ($xactions as $xaction) { + $xaction_key = $xaction->getMetadataValue('property.key'); + $old = $this->getCustomTransactionOldValue($object, $xaction); + $new = $xaction->getNewValue(); + $key_map[$xaction_key][] = array( + 'xaction' => $xaction, + 'old' => $old, + 'new' => $new, + ); + } + + foreach ($object->validateTransactions($key_map) as $error) { + $errors[] = $error; + } + break; + } + + return $errors; + } + + } diff --git a/src/applications/search/menuitem/PhabricatorApplicationProfileMenuItem.php b/src/applications/search/menuitem/PhabricatorApplicationProfileMenuItem.php index a207f57f3a..2cd6440cf5 100644 --- a/src/applications/search/menuitem/PhabricatorApplicationProfileMenuItem.php +++ b/src/applications/search/menuitem/PhabricatorApplicationProfileMenuItem.php @@ -5,6 +5,8 @@ final class PhabricatorApplicationProfileMenuItem const MENUITEMKEY = 'application'; + const FIELD_APPLICATION = 'application'; + public function getMenuItemTypeIcon() { return 'fa-globe'; } @@ -32,10 +34,11 @@ final class PhabricatorApplicationProfileMenuItem PhabricatorProfileMenuItemConfiguration $config) { return array( id(new PhabricatorDatasourceEditField()) - ->setKey('application') + ->setKey(self::FIELD_APPLICATION) ->setLabel(pht('Application')) ->setIsRequired(true) ->setDatasource(new PhabricatorApplicationDatasource()) + ->setIsRequired(true) ->setSingleValue($config->getMenuItemProperty('application')), ); } @@ -44,6 +47,7 @@ final class PhabricatorApplicationProfileMenuItem PhabricatorProfileMenuItemConfiguration $config) { $viewer = $this->getViewer(); $phid = $config->getMenuItemProperty('application'); + $app = id(new PhabricatorApplicationQuery()) ->setViewer($viewer) ->withPHIDs(array($phid)) @@ -77,4 +81,49 @@ final class PhabricatorApplicationProfileMenuItem ); } + public function validateTransactions( + PhabricatorProfileMenuItemConfiguration $config, + $field_key, + $value, + array $xactions) { + + $viewer = $this->getViewer(); + $errors = array(); + + if ($field_key == self::FIELD_APPLICATION) { + if ($this->isEmptyTransaction($value, $xactions)) { + $errors[] = $this->newRequiredError( + pht('You must choose an application.'), + $field_key); + } + + foreach ($xactions as $xaction) { + $new = $xaction['new']; + + if (!$new) { + continue; + } + + if ($new === $value) { + continue; + } + + $applications = id(new PhabricatorApplicationQuery()) + ->setViewer($viewer) + ->withPHIDs(array($new)) + ->execute(); + if (!$applications) { + $errors[] = $this->newInvalidError( + pht( + 'Application "%s" is not a valid application which you have '. + 'permission to see.', + $new), + $xaction['xaction']); + } + } + } + + return $errors; + } + } diff --git a/src/applications/search/menuitem/PhabricatorDashboardProfileMenuItem.php b/src/applications/search/menuitem/PhabricatorDashboardProfileMenuItem.php index 9b9d1dff7c..2aab6eee34 100644 --- a/src/applications/search/menuitem/PhabricatorDashboardProfileMenuItem.php +++ b/src/applications/search/menuitem/PhabricatorDashboardProfileMenuItem.php @@ -5,6 +5,8 @@ final class PhabricatorDashboardProfileMenuItem const MENUITEMKEY = 'dashboard'; + const FIELD_DASHBOARD = 'dashboardPHID'; + private $dashboard; public function getMenuItemTypeIcon() { @@ -75,7 +77,7 @@ final class PhabricatorDashboardProfileMenuItem ->setLabel(pht('Name')) ->setValue($this->getName($config)), id(new PhabricatorDatasourceEditField()) - ->setKey('dashboardPHID') + ->setKey(self::FIELD_DASHBOARD) ->setLabel(pht('Dashboard')) ->setIsRequired(true) ->setDatasource(new PhabricatorDashboardDatasource()) @@ -110,4 +112,49 @@ final class PhabricatorDashboardProfileMenuItem ); } + public function validateTransactions( + PhabricatorProfileMenuItemConfiguration $config, + $field_key, + $value, + array $xactions) { + + $viewer = $this->getViewer(); + $errors = array(); + + if ($field_key == self::FIELD_DASHBOARD) { + if ($this->isEmptyTransaction($value, $xactions)) { + $errors[] = $this->newRequiredError( + pht('You must choose a dashboard.'), + $field_key); + } + + foreach ($xactions as $xaction) { + $new = $xaction['new']; + + if (!$new) { + continue; + } + + if ($new === $value) { + continue; + } + + $dashboards = id(new PhabricatorDashboardQuery()) + ->setViewer($viewer) + ->withPHIDs(array($new)) + ->execute(); + if (!$dashboards) { + $errors[] = $this->newInvalidError( + pht( + 'Dashboard "%s" is not a valid dashboard which you have '. + 'permission to see.', + $new), + $xaction['xaction']); + } + } + } + + return $errors; + } + } diff --git a/src/applications/search/menuitem/PhabricatorEditEngineProfileMenuItem.php b/src/applications/search/menuitem/PhabricatorEditEngineProfileMenuItem.php index 071efa80dd..db4804996a 100644 --- a/src/applications/search/menuitem/PhabricatorEditEngineProfileMenuItem.php +++ b/src/applications/search/menuitem/PhabricatorEditEngineProfileMenuItem.php @@ -5,6 +5,8 @@ final class PhabricatorEditEngineProfileMenuItem const MENUITEMKEY = 'editengine'; + const FIELD_FORM = 'formKey'; + private $form; public function getMenuItemTypeIcon() { @@ -51,7 +53,8 @@ final class PhabricatorEditEngineProfileMenuItem foreach ($items as $item) { $key = $item->getMenuItemProperty('formKey'); - list($engine_key, $form_key) = explode('/', $key); + list($engine_key, $form_key) = PhabricatorEditEngine::splitFullKey($key); + if (is_numeric($form_key)) { $form = idx($form_ids, $form_key, null); $item->getMenuItem()->attachForm($form); @@ -83,7 +86,7 @@ final class PhabricatorEditEngineProfileMenuItem ->setLabel(pht('Name')) ->setValue($this->getName($config)), id(new PhabricatorDatasourceEditField()) - ->setKey('formKey') + ->setKey(self::FIELD_FORM) ->setLabel(pht('Form')) ->setIsRequired(true) ->setDatasource(new PhabricatorEditEngineDatasource()) @@ -120,4 +123,53 @@ final class PhabricatorEditEngineProfileMenuItem ); } + public function validateTransactions( + PhabricatorProfileMenuItemConfiguration $config, + $field_key, + $value, + array $xactions) { + + $viewer = $this->getViewer(); + $errors = array(); + + if ($field_key == self::FIELD_FORM) { + if ($this->isEmptyTransaction($value, $xactions)) { + $errors[] = $this->newRequiredError( + pht('You must choose a form.'), + $field_key); + } + + foreach ($xactions as $xaction) { + $new = $xaction['new']; + + if (!$new) { + continue; + } + + if ($new === $value) { + continue; + } + + list($engine_key, $form_key) = PhabricatorEditEngine::splitFullKey( + $new); + + $forms = id(new PhabricatorEditEngineConfigurationQuery()) + ->setViewer($viewer) + ->withEngineKeys(array($engine_key)) + ->withIdentifiers(array($form_key)) + ->execute(); + if (!$forms) { + $errors[] = $this->newInvalidError( + pht( + 'Form "%s" is not a valid form which you have permission to '. + 'see.', + $new), + $xaction['xaction']); + } + } + } + + return $errors; + } + } diff --git a/src/applications/search/menuitem/PhabricatorLinkProfileMenuItem.php b/src/applications/search/menuitem/PhabricatorLinkProfileMenuItem.php index 5571788cc1..3b136321c2 100644 --- a/src/applications/search/menuitem/PhabricatorLinkProfileMenuItem.php +++ b/src/applications/search/menuitem/PhabricatorLinkProfileMenuItem.php @@ -5,6 +5,9 @@ final class PhabricatorLinkProfileMenuItem const MENUITEMKEY = 'link'; + const FIELD_URI = 'uri'; + const FIELD_NAME = 'name'; + public function getMenuItemTypeIcon() { return 'fa-link'; } @@ -26,12 +29,12 @@ final class PhabricatorLinkProfileMenuItem PhabricatorProfileMenuItemConfiguration $config) { return array( id(new PhabricatorTextEditField()) - ->setKey('name') + ->setKey(self::FIELD_NAME) ->setLabel(pht('Name')) ->setIsRequired(true) ->setValue($this->getLinkName($config)), id(new PhabricatorTextEditField()) - ->setKey('uri') + ->setKey(self::FIELD_URI) ->setLabel(pht('URI')) ->setIsRequired(true) ->setValue($this->getLinkURI($config)), @@ -91,4 +94,53 @@ final class PhabricatorLinkProfileMenuItem ); } + public function validateTransactions( + PhabricatorProfileMenuItemConfiguration $config, + $field_key, + $value, + array $xactions) { + + $viewer = $this->getViewer(); + $errors = array(); + + if ($field_key == self::FIELD_NAME) { + if ($this->isEmptyTransaction($value, $xactions)) { + $errors[] = $this->newRequiredError( + pht('You must choose a link name.'), + $field_key); + } + } + + if ($field_key == self::FIELD_URI) { + if ($this->isEmptyTransaction($value, $xactions)) { + $errors[] = $this->newRequiredError( + pht('You must choose a URI to link to.'), + $field_key); + } + + foreach ($xactions as $xaction) { + $new = $xaction['new']; + + if (!$new) { + continue; + } + + if ($new === $value) { + continue; + } + + if (!$this->isValidLinkURI($new)) { + $errors[] = $this->newInvalidError( + pht( + 'URI "%s" is not a valid link URI. It should be a full, valid '. + 'URI beginning with a protocol like "%s".', + $new, + 'https://'), + $xaction['xaction']); + } + } + } + + return $errors; + } } diff --git a/src/applications/search/menuitem/PhabricatorProfileMenuItem.php b/src/applications/search/menuitem/PhabricatorProfileMenuItem.php index c47d9afe8e..05fa6543d3 100644 --- a/src/applications/search/menuitem/PhabricatorProfileMenuItem.php +++ b/src/applications/search/menuitem/PhabricatorProfileMenuItem.php @@ -70,4 +70,41 @@ abstract class PhabricatorProfileMenuItem extends Phobject { return new PHUIListItemView(); } + public function valdateTransactions( + PhabricatorProfileMenuItemConfiguration $config, + $field_key, + $value, + array $xactions) { + return array(); + } + + final protected function isEmptyTransaction($value, array $xactions) { + $result = $value; + foreach ($xactions as $xaction) { + $result = $xaction['new']; + } + + return !strlen($result); + } + + final protected function newError($title, $message, $xaction = null) { + return new PhabricatorApplicationTransactionValidationError( + PhabricatorProfileMenuItemConfigurationTransaction::TYPE_PROPERTY, + $title, + $message, + $xaction); + } + + final protected function newRequiredError($message, $type) { + $xaction = id(new PhabricatorProfileMenuItemConfigurationTransaction()) + ->setMetadataValue('property.key', $type); + + return $this->newError(pht('Required'), $message, $xaction) + ->setIsMissingFieldError(true); + } + + final protected function newInvalidError($message, $xaction = null) { + return $this->newError(pht('Invalid'), $message, $xaction); + } + } diff --git a/src/applications/search/menuitem/PhabricatorProjectProfileMenuItem.php b/src/applications/search/menuitem/PhabricatorProjectProfileMenuItem.php index e47d97677f..364a68e4b8 100644 --- a/src/applications/search/menuitem/PhabricatorProjectProfileMenuItem.php +++ b/src/applications/search/menuitem/PhabricatorProjectProfileMenuItem.php @@ -4,6 +4,7 @@ final class PhabricatorProjectProfileMenuItem extends PhabricatorProfileMenuItem { const MENUITEMKEY = 'project'; + const FIELD_PROJECT = 'project'; private $project; @@ -76,7 +77,7 @@ final class PhabricatorProjectProfileMenuItem ->setLabel(pht('Name')) ->setValue($this->getName($config)), id(new PhabricatorDatasourceEditField()) - ->setKey('project') + ->setKey(self::FIELD_PROJECT) ->setLabel(pht('Project')) ->setIsRequired(true) ->setDatasource(new PhabricatorProjectDatasource()) @@ -111,4 +112,49 @@ final class PhabricatorProjectProfileMenuItem ); } + public function validateTransactions( + PhabricatorProfileMenuItemConfiguration $config, + $field_key, + $value, + array $xactions) { + + $viewer = $this->getViewer(); + $errors = array(); + + if ($field_key == self::FIELD_PROJECT) { + if ($this->isEmptyTransaction($value, $xactions)) { + $errors[] = $this->newRequiredError( + pht('You must choose a project.'), + $field_key); + } + + foreach ($xactions as $xaction) { + $new = $xaction['new']; + + if (!$new) { + continue; + } + + if ($new === $value) { + continue; + } + + $projects = id(new PhabricatorProjectQuery()) + ->setViewer($viewer) + ->withPHIDs(array($new)) + ->execute(); + if (!$projects) { + $errors[] = $this->newInvalidError( + pht( + 'Project "%s" is not a valid project which you have '. + 'permission to see.', + $new), + $xaction['xaction']); + } + } + } + + return $errors; + } + } diff --git a/src/applications/search/storage/PhabricatorProfileMenuItemConfiguration.php b/src/applications/search/storage/PhabricatorProfileMenuItemConfiguration.php index e1571ee6f6..107fa63c72 100644 --- a/src/applications/search/storage/PhabricatorProfileMenuItemConfiguration.php +++ b/src/applications/search/storage/PhabricatorProfileMenuItemConfiguration.php @@ -126,6 +126,30 @@ final class PhabricatorProfileMenuItemConfiguration return $this->getMenuItem()->willBuildNavigationItems($items); } + public function validateTransactions(array $map) { + $item = $this->getMenuItem(); + + $fields = $item->buildEditEngineFields($this); + $errors = array(); + foreach ($fields as $field) { + $field_key = $field->getKey(); + + $xactions = idx($map, $field_key, array()); + $value = $this->getMenuItemProperty($field_key); + + $field_errors = $item->validateTransactions( + $this, + $field_key, + $value, + $xactions); + foreach ($field_errors as $error) { + $errors[] = $error; + } + } + + return $errors; + } + public function getSortVector() { // Sort custom items above global items. if ($this->getCustomPHID()) { diff --git a/src/applications/transactions/editengine/PhabricatorEditEngine.php b/src/applications/transactions/editengine/PhabricatorEditEngine.php index 65901e1294..e594ae4b67 100644 --- a/src/applications/transactions/editengine/PhabricatorEditEngine.php +++ b/src/applications/transactions/editengine/PhabricatorEditEngine.php @@ -95,6 +95,10 @@ abstract class PhabricatorEditEngine return $keys; } + public static function splitFullKey($full_key) { + return explode('/', $full_key, 2); + } + public function getQuickCreateOrderVector() { return id(new PhutilSortVector()) ->addString($this->getObjectCreateShortText());