From 3f24232d2b8da2f10100f2558ad5eb1057c22794 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 18 Sep 2013 15:31:58 -0700 Subject: [PATCH] Allow custom fields to have validation logic Summary: Ref T418. This is fairly messy, but basically: - Add a validation phase to TransactionEditor. - Add a validation phase to CustomField. - Bring it to StandardField. - Add validation logic for the int field. - Provide support in related classes. Test Plan: See screenshot. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T418 Differential Revision: https://secure.phabricator.com/D7028 --- src/__phutil_library_map__.php | 4 ++ ...PhabricatorPeopleProfileEditController.php | 11 +++- ...habricatorApplicationTransactionEditor.php | 62 ++++++++++++++++++- ...rApplicationTransactionValidationError.php | 39 ++++++++++++ ...licationTransactionValidationException.php | 43 +++++++++++++ .../field/PhabricatorCustomField.php | 30 +++++++++ .../PhabricatorStandardCustomField.php | 25 ++++++++ .../PhabricatorStandardCustomFieldInt.php | 29 ++++++++- src/view/form/PHUIFormBoxView.php | 30 ++++++++- 9 files changed, 265 insertions(+), 8 deletions(-) create mode 100644 src/applications/transactions/error/PhabricatorApplicationTransactionValidationError.php create mode 100644 src/applications/transactions/exception/PhabricatorApplicationTransactionValidationException.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index b8b0e3cb13..9664594652 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -868,6 +868,8 @@ phutil_register_library_map(array( 'PhabricatorApplicationTransactionQuery' => 'applications/transactions/query/PhabricatorApplicationTransactionQuery.php', 'PhabricatorApplicationTransactionResponse' => 'applications/transactions/response/PhabricatorApplicationTransactionResponse.php', 'PhabricatorApplicationTransactionTextDiffDetailView' => 'applications/transactions/view/PhabricatorApplicationTransactionTextDiffDetailView.php', + 'PhabricatorApplicationTransactionValidationError' => 'applications/transactions/error/PhabricatorApplicationTransactionValidationError.php', + 'PhabricatorApplicationTransactionValidationException' => 'applications/transactions/exception/PhabricatorApplicationTransactionValidationException.php', 'PhabricatorApplicationTransactionView' => 'applications/transactions/view/PhabricatorApplicationTransactionView.php', 'PhabricatorApplicationTransactions' => 'applications/transactions/application/PhabricatorApplicationTransactions.php', 'PhabricatorApplicationUIExamples' => 'applications/uiexample/application/PhabricatorApplicationUIExamples.php', @@ -2960,6 +2962,8 @@ phutil_register_library_map(array( 'PhabricatorApplicationTransactionQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'PhabricatorApplicationTransactionResponse' => 'AphrontProxyResponse', 'PhabricatorApplicationTransactionTextDiffDetailView' => 'AphrontView', + 'PhabricatorApplicationTransactionValidationError' => 'Phobject', + 'PhabricatorApplicationTransactionValidationException' => 'Exception', 'PhabricatorApplicationTransactionView' => 'AphrontView', 'PhabricatorApplicationTransactions' => 'PhabricatorApplication', 'PhabricatorApplicationUIExamples' => 'PhabricatorApplication', diff --git a/src/applications/people/controller/PhabricatorPeopleProfileEditController.php b/src/applications/people/controller/PhabricatorPeopleProfileEditController.php index d7a059603a..2458440bb2 100644 --- a/src/applications/people/controller/PhabricatorPeopleProfileEditController.php +++ b/src/applications/people/controller/PhabricatorPeopleProfileEditController.php @@ -39,6 +39,7 @@ final class PhabricatorPeopleProfileEditController ->setViewer($user) ->readFieldsFromStorage($user); + $validation_exception = null; if ($request->isFormPost()) { $xactions = $field_list->buildFieldTransactionsFromRequest( new PhabricatorUserTransaction(), @@ -50,9 +51,12 @@ final class PhabricatorPeopleProfileEditController PhabricatorContentSource::newFromRequest($request)) ->setContinueOnNoEffect(true); - $editor->applyTransactions($user, $xactions); - - return id(new AphrontRedirectResponse())->setURI($profile_uri); + try { + $editor->applyTransactions($user, $xactions); + return id(new AphrontRedirectResponse())->setURI($profile_uri); + } catch (PhabricatorApplicationTransactionValidationException $ex) { + $validation_exception = $ex; + } } $title = pht('Edit Profile'); @@ -78,6 +82,7 @@ final class PhabricatorPeopleProfileEditController $form_box = id(new PHUIFormBoxView()) ->setHeaderText(pht('Edit Your Profile')) + ->setValidationException($validation_exception) ->setForm($form); return $this->buildApplicationPage( diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php index a44ae8e4c7..cb2b4dbeb6 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -185,7 +185,6 @@ abstract class PhabricatorApplicationTransactionEditor PhabricatorLiskDAO $object, array $xactions) { return false; - } protected function applyInitialEffects( @@ -356,6 +355,18 @@ abstract class PhabricatorApplicationTransactionEditor $transaction_open = false; if (!$is_preview) { + $errors = array(); + $type_map = mgroup($xactions, 'getTransactionType'); + foreach ($this->getTransactionTypes() as $type) { + $type_xactions = idx($type_map, $type, array()); + $errors[] = $this->validateTransaction($object, $type, $type_xactions); + } + + $errors = array_mergev($errors); + if ($errors) { + throw new PhabricatorApplicationTransactionValidationException($errors); + } + if ($object->getID()) { foreach ($xactions as $xaction) { @@ -1003,6 +1014,55 @@ abstract class PhabricatorApplicationTransactionEditor } + /** + * Hook for validating transactions. This callback will be invoked for each + * available transaction type, even if an edit does not apply any transactions + * of that type. This allows you to raise exceptions when required fields are + * missing, by detecting that the object has no field value and there is no + * transaction which sets one. + * + * @param PhabricatorLiskDAO Object being edited. + * @param string Transaction type to validate. + * @param list Transactions of given type, + * which may be empty if the edit does not apply any transactions of the + * given type. + * @return list List of + * validation errors. + */ + protected function validateTransaction( + PhabricatorLiskDAO $object, + $type, + array $xactions) { + + $errors = array(); + switch ($type) { + case PhabricatorTransactions::TYPE_CUSTOMFIELD: + $groups = array(); + foreach ($xactions as $xaction) { + $groups[$xaction->getMetadataValue('customfield:key')][] = $xaction; + } + + $field_list = PhabricatorCustomField::getObjectFields( + $object, + PhabricatorCustomField::ROLE_EDIT); + + $role_xactions = PhabricatorCustomField::ROLE_APPLICATIONTRANSACTIONS; + foreach ($field_list->getFields() as $field) { + if (!$field->shouldEnableForRole($role_xactions)) { + continue; + } + $errors[] = $field->validateApplicationTransactions( + $this, + $type, + idx($groups, $field->getFieldKey(), array())); + } + break; + } + + return array_mergev($errors); + } + + /* -( Implicit CCs )------------------------------------------------------- */ diff --git a/src/applications/transactions/error/PhabricatorApplicationTransactionValidationError.php b/src/applications/transactions/error/PhabricatorApplicationTransactionValidationError.php new file mode 100644 index 0000000000..65661abc06 --- /dev/null +++ b/src/applications/transactions/error/PhabricatorApplicationTransactionValidationError.php @@ -0,0 +1,39 @@ +type = $type; + $this->shortMessage = $short_message; + $this->message = $message; + $this->transaction = $xaction; + } + + public function getType() { + return $this->type; + } + + public function getTransaction() { + return $this->tranaction; + } + + public function getShortMessage() { + return $this->shortMessage; + } + + public function getMessage() { + return $this->message; + } + +} diff --git a/src/applications/transactions/exception/PhabricatorApplicationTransactionValidationException.php b/src/applications/transactions/exception/PhabricatorApplicationTransactionValidationException.php new file mode 100644 index 0000000000..edc8d97985 --- /dev/null +++ b/src/applications/transactions/exception/PhabricatorApplicationTransactionValidationException.php @@ -0,0 +1,43 @@ +errors = $errors; + + $message = array(); + $message[] = 'Validation errors:'; + foreach ($this->errors as $error) { + $message[] = ' - '.$error->getMessage(); + } + + parent::__construct(implode("\n", $message)); + } + + public function getErrors() { + return $this->errors; + } + + public function getErrorMessages() { + return mpull($this->errors, 'getMessage'); + } + + public function getShortMessage($type) { + foreach ($this->errors as $error) { + if ($error->getType() === $type) { + if ($error->getShortMessage() !== null) { + return $error->getShortMessage(); + } + } + } + return null; + } + +} diff --git a/src/infrastructure/customfield/field/PhabricatorCustomField.php b/src/infrastructure/customfield/field/PhabricatorCustomField.php index 5621e24f10..b7b2bbf9ad 100644 --- a/src/infrastructure/customfield/field/PhabricatorCustomField.php +++ b/src/infrastructure/customfield/field/PhabricatorCustomField.php @@ -844,6 +844,36 @@ abstract class PhabricatorCustomField { } + /** + * Validate transactions for an object. This allows you to raise an error + * when a transaction would set a field to an invalid value, or when a field + * is required but no transactions provide value. + * + * @param PhabricatorLiskDAO Editor applying the transactions. + * @param string Transaction type. This type is always + * `PhabricatorTransactions::TYPE_CUSTOMFIELD`, it is provided for + * convenience when constructing exceptions. + * @param list Transactions being applied, + * which may be empty if this field is not being edited. + * @return list Validation + * errors. + * + * @task appxaction + */ + public function validateApplicationTransactions( + PhabricatorApplicationTransactionEditor $editor, + $type, + array $xactions) { + if ($this->proxy) { + return $this->proxy->validateApplicationTransactions( + $editor, + $type, + $xactions); + } + return array(); + } + + /* -( Edit View )---------------------------------------------------------- */ diff --git a/src/infrastructure/customfield/standard/PhabricatorStandardCustomField.php b/src/infrastructure/customfield/standard/PhabricatorStandardCustomField.php index b136e11419..69076a92c0 100644 --- a/src/infrastructure/customfield/standard/PhabricatorStandardCustomField.php +++ b/src/infrastructure/customfield/standard/PhabricatorStandardCustomField.php @@ -11,6 +11,7 @@ abstract class PhabricatorStandardCustomField private $applicationField; private $strings; private $caption; + private $fieldError; abstract public function getFieldType(); @@ -114,6 +115,14 @@ abstract class PhabricatorStandardCustomField return idx($this->fieldConfig, $key, $default); } + public function setFieldError($field_error) { + $this->fieldError = $field_error; + return $this; + } + + public function getFieldError() { + return $this->fieldError; + } /* -( PhabricatorCustomField )--------------------------------------------- */ @@ -178,6 +187,7 @@ abstract class PhabricatorStandardCustomField ->setName($this->getFieldKey()) ->setCaption($this->getCaption()) ->setValue($this->getFieldValue()) + ->setError($this->getFieldError()) ->setLabel($this->getFieldName()); } @@ -230,4 +240,19 @@ abstract class PhabricatorStandardCustomField return; } + public function validateApplicationTransactions( + PhabricatorApplicationTransactionEditor $editor, + $type, + array $xactions) { + + $this->setFieldError(null); + + $errors = parent::validateApplicationTransactions( + $editor, + $type, + $xactions); + + return $errors; + } + } diff --git a/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldInt.php b/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldInt.php index a467b3c6a5..1c0c6677ef 100644 --- a/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldInt.php +++ b/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldInt.php @@ -21,7 +21,7 @@ final class PhabricatorStandardCustomFieldInt public function getValueForStorage() { $value = $this->getFieldValue(); if (strlen($value)) { - return (int)$value; + return $value; } else { return null; } @@ -68,4 +68,31 @@ final class PhabricatorStandardCustomFieldInt ->setValue($value)); } + public function validateApplicationTransactions( + PhabricatorApplicationTransactionEditor $editor, + $type, + array $xactions) { + + $errors = parent::validateApplicationTransactions( + $editor, + $type, + $xactions); + + foreach ($xactions as $xaction) { + $value = $xaction->getNewValue(); + if (strlen($value)) { + if (!preg_match('/^-?\d+/', $value)) { + $errors[] = new PhabricatorApplicationTransactionValidationError( + $type, + pht('Invalid'), + pht('%s must be an integer.', $this->getFieldName()), + $xaction); + $this->setFieldError(pht('Invalid')); + } + } + } + + return $errors; + } + } diff --git a/src/view/form/PHUIFormBoxView.php b/src/view/form/PHUIFormBoxView.php index 16ca8c7de8..bf1aa10a3f 100644 --- a/src/view/form/PHUIFormBoxView.php +++ b/src/view/form/PHUIFormBoxView.php @@ -5,6 +5,7 @@ final class PHUIFormBoxView extends AphrontView { private $headerText; private $formError = null; private $form; + private $validationException; public function setHeaderText($text) { $this->headerText = $text; @@ -21,16 +22,39 @@ final class PHUIFormBoxView extends AphrontView { return $this; } - public function render() { + public function setValidationException( + PhabricatorApplicationTransactionValidationException $ex = null) { + $this->validationException = $ex; + return $this; + } - $error = $this->formError ? $this->formError : null; + public function render() { $header = id(new PhabricatorActionHeaderView()) ->setHeaderTitle($this->headerText) ->setHeaderColor(PhabricatorActionHeaderView::HEADER_LIGHTBLUE); + $ex = $this->validationException; + $exception_errors = null; + if ($ex) { + $messages = array(); + foreach ($ex->getErrors() as $error) { + $messages[] = $error->getMessage(); + } + if ($messages) { + $exception_errors = id(new AphrontErrorView()) + ->setErrors($messages); + } + } + $content = id(new PHUIBoxView()) - ->appendChild(array($header, $error, $this->form)) + ->appendChild( + array( + $header, + $this->formError, + $exception_errors, + $this->form, + )) ->setBorder(true) ->addMargin(PHUI::MARGIN_LARGE_TOP) ->addMargin(PHUI::MARGIN_LARGE_LEFT)