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
2024-11-29 18:22:41 +01:00 · 2017-01-20 11:03:09 -08:00 · 2017-01-20 11:03:09 -08:00 · 8113b76910
commit 8113b76910
parent 98a29f3de9
10 changed files with 396 additions and 7 deletions
--- 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);
+  }
+
 }
--- 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;
+  }
+
+
 }
--- 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;
+  }
+
 }
--- 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;
+  }
+
 }
--- 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;
+  }
+
 }
--- 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;
+  }
 }
--- 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);
+  }
+
 }
--- 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;
+  }
+
 }
--- 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()) {
--- 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());