From 5251e2f14d8e54157cc33094e7c29d8a6c2f9259 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 19 Sep 2013 11:56:30 -0700 Subject: [PATCH] Remove vestigal AuxiliaryField code from Maniphest Summary: Gets rid of as much of this as possible. We'll batch handles and remarkup again some day, but after ApplicationTransactions. Test Plan: Edited, viewed, and checked email for custom field edits. Reviewers: btrahan Reviewed By: btrahan CC: aran Differential Revision: https://secure.phabricator.com/D7037 --- src/__phutil_library_map__.php | 7 - ...hestAuxiliaryFieldDefaultSpecification.php | 463 ------------------ .../ManiphestAuxiliaryFieldSpecification.php | 183 +------ .../ManiphestTaskDetailController.php | 14 - .../ManiphestTaskEditController.php | 7 +- .../maniphest/field/ManiphestCustomField.php | 55 +-- .../view/ManiphestTransactionDetailView.php | 12 +- 7 files changed, 9 insertions(+), 732 deletions(-) delete mode 100644 src/applications/maniphest/auxiliaryfield/ManiphestAuxiliaryFieldDefaultSpecification.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 8a7cff2f68..8bbc910150 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -685,7 +685,6 @@ phutil_register_library_map(array( 'LiskMigrationIterator' => 'infrastructure/storage/lisk/LiskMigrationIterator.php', 'LiskRawMigrationIterator' => 'infrastructure/storage/lisk/LiskRawMigrationIterator.php', 'ManiphestAction' => 'applications/maniphest/constants/ManiphestAction.php', - 'ManiphestAuxiliaryFieldDefaultSpecification' => 'applications/maniphest/auxiliaryfield/ManiphestAuxiliaryFieldDefaultSpecification.php', 'ManiphestAuxiliaryFieldSpecification' => 'applications/maniphest/auxiliaryfield/ManiphestAuxiliaryFieldSpecification.php', 'ManiphestBatchEditController' => 'applications/maniphest/controller/ManiphestBatchEditController.php', 'ManiphestConfiguredCustomField' => 'applications/maniphest/field/ManiphestConfiguredCustomField.php', @@ -2759,12 +2758,6 @@ phutil_register_library_map(array( 'LiskMigrationIterator' => 'PhutilBufferedIterator', 'LiskRawMigrationIterator' => 'PhutilBufferedIterator', 'ManiphestAction' => 'ManiphestConstants', - 'ManiphestAuxiliaryFieldDefaultSpecification' => 'ManiphestAuxiliaryFieldSpecification', - 'ManiphestAuxiliaryFieldSpecification' => - array( - 0 => 'ManiphestCustomField', - 1 => 'PhabricatorMarkupInterface', - ), 'ManiphestBatchEditController' => 'ManiphestController', 'ManiphestConfiguredCustomField' => array( diff --git a/src/applications/maniphest/auxiliaryfield/ManiphestAuxiliaryFieldDefaultSpecification.php b/src/applications/maniphest/auxiliaryfield/ManiphestAuxiliaryFieldDefaultSpecification.php deleted file mode 100644 index f5be110ae2..0000000000 --- a/src/applications/maniphest/auxiliaryfield/ManiphestAuxiliaryFieldDefaultSpecification.php +++ /dev/null @@ -1,463 +0,0 @@ -fieldType; - } - - public function setFieldType($val) { - $this->fieldType = $val; - return $this; - } - - public function getError() { - return $this->error; - } - - public function setError($val) { - $this->error = $val; - return $this; - } - - public function getSelectOptions() { - return $this->selectOptions; - } - - public function setSelectOptions($array) { - $this->selectOptions = $array; - return $this; - } - - public function setRequired($bool) { - $this->required = $bool; - return $this; - } - - public function isRequired() { - return $this->required; - } - - public function setCheckboxLabel($checkbox_label) { - $this->checkboxLabel = $checkbox_label; - return $this; - } - - public function getCheckboxLabel() { - return $this->checkboxLabel; - } - - public function setCheckboxValue($checkbox_value) { - $this->checkboxValue = $checkbox_value; - return $this; - } - - public function getCheckboxValue() { - return $this->checkboxValue; - } - - public function renderEditControl() { - $control = null; - - $type = $this->getFieldType(); - switch ($type) { - case self::TYPE_INT: - $control = new AphrontFormTextControl(); - break; - case self::TYPE_STRING: - $control = new AphrontFormTextControl(); - break; - case self::TYPE_SELECT: - $control = new AphrontFormSelectControl(); - $control->setOptions($this->getSelectOptions()); - break; - case self::TYPE_BOOL: - $control = new AphrontFormCheckboxControl(); - break; - case self::TYPE_DATE: - $control = new AphrontFormDateControl(); - $control->setUser($this->getUser()); - if (!$this->isRequired()) { - $control->setAllowNull(true); - } - break; - case self::TYPE_REMARKUP: - $control = new PhabricatorRemarkupControl(); - $control->setUser($this->getUser()); - break; - case self::TYPE_USER: - case self::TYPE_USERS: - $control = new AphrontFormTokenizerControl(); - $control->setDatasource('/typeahead/common/users/'); - if ($type == self::TYPE_USER) { - $control->setLimit(1); - } - break; - case self::TYPE_HEADER: - $control = new AphrontFormMarkupControl(); - break; - default: - $label = $this->getLabel(); - throw new Exception( - "Field type '{$type}' is not a valid type (for field '{$label}')."); - break; - } - - switch ($type) { - case self::TYPE_BOOL: - $control->addCheckbox( - 'auxiliary['.$this->getAuxiliaryKey().']', - 1, - $this->getCheckboxLabel(), - (bool)$this->getValue()); - break; - case self::TYPE_DATE: - if ($this->getValue() > 0) { - $control->setValue($this->getValue()); - } - $control->setName('auxiliary_date_'.$this->getAuxiliaryKey()); - break; - case self::TYPE_USER: - case self::TYPE_USERS: - $control->setName('auxiliary_tokenizer_'.$this->getAuxiliaryKey()); - $value = array(); - foreach ($this->getValue() as $phid) { - $value[$phid] = $this->getHandle($phid)->getFullName(); - } - $control->setValue($value); - break; - case self::TYPE_HEADER: - $control->setValue( - phutil_tag( - 'h2', - array( - 'class' => 'maniphest-auxiliary-header', - ), - $this->getLabel())); - break; - default: - $control->setValue($this->getValue()); - $control->setName('auxiliary['.$this->getAuxiliaryKey().']'); - break; - } - - switch ($type) { - case self::TYPE_HEADER: - break; - default: - $control->setLabel($this->getLabel()); - $control->setCaption($this->getCaption()); - $control->setError($this->getError()); - break; - } - - $stripped_auxiliary_key = preg_replace( - '/[\w\d\.\-\:]+/', '', $this->getAuxiliaryKey()); - - if (strlen($stripped_auxiliary_key)) { - $unique_key_chars = array_unique(str_split($stripped_auxiliary_key)); - $unique_key_chars = implode(" ,", $unique_key_chars); - $control->setDisabled(true); - $control->setCaption( - "This control is not configured correctly, the key must only contain - ( a-z, A-Z, 0-9, ., -, : ) but has ( {$unique_key_chars} ), so it can - not be rendered, go fix your config."); - } - - return $control; - } - - public function setValueFromRequest(AphrontRequest $request) { - $type = $this->getFieldType(); - switch ($type) { - case self::TYPE_DATE: - $control = $this->renderEditControl(); - $value = $control->readValueFromRequest($request); - break; - case self::TYPE_USER: - case self::TYPE_USERS: - $name = 'auxiliary_tokenizer_'.$this->getAuxiliaryKey(); - $value = $request->getArr($name); - if ($type == self::TYPE_USER) { - $value = array_slice($value, 0, 1); - } - break; - default: - $aux_post_values = $request->getArr('auxiliary'); - $value = idx($aux_post_values, $this->getAuxiliaryKey(), ''); - break; - } - return $this->setValue($value); - } - - public function getValueForStorage() { - switch ($this->getFieldType()) { - case self::TYPE_USER: - case self::TYPE_USERS: - return json_encode($this->getValue()); - case self::TYPE_DATE: - return (int)$this->getValue(); - default: - return $this->getValue(); - } - } - - public function setValueFromStorage($value) { - switch ($this->getFieldType()) { - case self::TYPE_USER: - case self::TYPE_USERS: - $value = json_decode($value, true); - if (!is_array($value)) { - $value = array(); - } - break; - case self::TYPE_DATE: - $value = (int)$value; - $this->setDefaultValue($value); - break; - default: - break; - } - return $this->setValue($value); - } - - public function validateApplicationTransactions( - PhabricatorApplicationTransactionEditor $editor, - $type, - array $xactions) { - - $errors = parent::validateApplicationTransactions( - $editor, - $type, - $xactions); - - $has_value = false; - $value = null; - foreach ($xactions as $xaction) { - $has_value = true; - $value = $xaction->getNewValue(); - switch ($this->getFieldType()) { - case self::TYPE_INT: - if ($value && !is_numeric($value)) { - $errors[] = new PhabricatorApplicationTransactionValidationError( - $type, - pht('Invalid'), - pht('%s must be an integer value.', $this->getLabel()), - $xaction); - $this->setError(pht('Invalid')); - } - break; - case self::TYPE_DATE: - if ((int)$value <= 0 && $this->isRequired()) { - $errors[] = new PhabricatorApplicationTransactionValidationError( - $type, - pht('Invalid'), - pht('%s must be a valid date.', $this->getLabel()), - $xaction); - $this->setError(pht('Invalid')); - } - break; - } - } - - if ($this->isRequired()) { - if (!$has_value) { - $value = $this->getOldValueForApplicationTransactions(); - } - if (!$value) { - $errors[] = new PhabricatorApplicationTransactionValidationError( - $type, - pht('Required'), - pht('%s is required.', $this->getLabel()), - null); - $this->setError(pht('Required')); - } - } - - return $errors; - } - - public function setDefaultValue($value) { - switch ($this->getFieldType()) { - case self::TYPE_DATE: - $value = strtotime($value); - if ($value <= 0) { - $value = time(); - } - $this->setValue($value); - break; - case self::TYPE_USER: - case self::TYPE_USERS: - if (!is_array($value)) { - $value = array(); - } else { - $value = array_values($value); - } - $this->setValue($value); - break; - default: - $this->setValue((string)$value); - break; - } - } - - public function getMarkupFields() { - switch ($this->getFieldType()) { - case self::TYPE_REMARKUP: - return array('default'); - } - return parent::getMarkupFields(); - } - - public function renderForDetailView() { - switch ($this->getFieldType()) { - case self::TYPE_BOOL: - if ($this->getValue()) { - return $this->getCheckboxValue(); - } else { - return null; - } - case self::TYPE_SELECT: - return idx($this->getSelectOptions(), $this->getValue()); - case self::TYPE_DATE: - return phabricator_datetime($this->getValue(), $this->getUser()); - case self::TYPE_REMARKUP: - return $this->getMarkupEngine()->getOutput( - $this, - 'default'); - case self::TYPE_USER: - case self::TYPE_USERS: - return $this->renderHandleList($this->getValue()); - case self::TYPE_HEADER: - return phutil_tag('hr'); - } - return parent::renderForDetailView(); - } - - public function getRequiredHandlePHIDs() { - switch ($this->getFieldType()) { - case self::TYPE_USER; - case self::TYPE_USERS: - return $this->getValue(); - } - return parent::getRequiredHandlePHIDs(); - } - - protected function renderHandleList(array $phids) { - $links = array(); - foreach ($phids as $phid) { - $links[] = $this->getHandle($phid)->renderLink(); - } - return phutil_implode_html(', ', $links); - } - - public function renderTransactionDescription( - ManiphestTransaction $transaction, - $target) { - - $label = $this->getLabel(); - $old = $transaction->getOldValue(); - $new = $transaction->getNewValue(); - - switch ($this->getFieldType()) { - case self::TYPE_BOOL: - if ($new) { - $desc = "set field '{$label}' true"; - } else { - $desc = "set field '{$label}' false"; - } - break; - case self::TYPE_SELECT: - $old_display = idx($this->getSelectOptions(), $old); - $new_display = idx($this->getSelectOptions(), $new); - if ($old === null) { - $desc = "set field '{$label}' to '{$new_display}'"; - } else { - $desc = "changed field '{$label}' ". - "from '{$old_display}' to '{$new_display}'"; - } - break; - case self::TYPE_DATE: - // NOTE: Although it should be impossible to get bad data in these - // fields normally, users can change the type of an existing field and - // leave us with uninterpretable data in old transactions. - if ((int)$new <= 0) { - $new_display = "none"; - } else { - $new_display = phabricator_datetime($new, $this->getUser()); - } - if ($old === null) { - $desc = "set field '{$label}' to '{$new_display}'"; - } else { - if ((int)$old <= 0) { - $old_display = "none"; - } else { - $old_display = phabricator_datetime($old, $this->getUser()); - } - $desc = "changed field '{$label}' ". - "from '{$old_display}' to '{$new_display}'"; - } - break; - case self::TYPE_REMARKUP: - // TODO: After we get ApplicationTransactions, straighten this out. - $desc = "updated field '{$label}'"; - break; - case self::TYPE_USER: - case self::TYPE_USERS: - // TODO: As above, this is a mess that should get straightened out, - // but it will be easier after T2217. - $desc = "updated field '{$label}'"; - break; - default: - if (!strlen($old)) { - if (!strlen($new)) { - return null; - } - $desc = "set field '{$label}' to '{$new}'"; - } else { - $desc = "updated '{$label}' ". - "from '{$old}' to '{$new}'"; - } - break; - } - - return $desc; - } - - public function setShouldCopyWhenCreatingSimilarTask($copy) { - $this->shouldCopyWhenCreatingSimilarTask = $copy; - return $this; - } - - public function shouldCopyWhenCreatingSimilarTask() { - return $this->shouldCopyWhenCreatingSimilarTask; - } - -} diff --git a/src/applications/maniphest/auxiliaryfield/ManiphestAuxiliaryFieldSpecification.php b/src/applications/maniphest/auxiliaryfield/ManiphestAuxiliaryFieldSpecification.php index 0e2593123f..60df075771 100644 --- a/src/applications/maniphest/auxiliaryfield/ManiphestAuxiliaryFieldSpecification.php +++ b/src/applications/maniphest/auxiliaryfield/ManiphestAuxiliaryFieldSpecification.php @@ -1,188 +1,9 @@ getObject(); - } - - // TODO: Remove; obsolete. - public function getUser() { - return $this->getViewer(); - } - - public function setLabel($val) { - $this->label = $val; - return $this; - } - - public function getLabel() { - return $this->label; - } - - public function setAuxiliaryKey($val) { - $this->auxiliaryKey = $val; - return $this; - } - - public function getAuxiliaryKey() { - return 'std:maniphest:'.$this->auxiliaryKey; - } - - public function setCaption($val) { - $this->caption = $val; - return $this; - } - - public function getCaption() { - return $this->caption; - } - - public function setValue($val) { - $this->value = $val; - return $this; - } - - public function getValue() { - return $this->value; - } - - public function isRequired() { - return false; - } - - public function setType($val) { - $this->type = $val; - return $this; - } - - public function getType() { - return $this->type; - } - - public function renderForDetailView() { - return $this->getValue(); - } - - public function getRequiredHandlePHIDs() { - return array(); - } - - public function setHandles(array $handles) { - assert_instances_of($handles, 'PhabricatorObjectHandle'); - $this->handles = array_select_keys( - $handles, - $this->getRequiredHandlePHIDs()); - return $this; - } - - public function getHandle($phid) { - if (empty($this->handles[$phid])) { - throw new Exception( - "Field is requesting a handle ('{$phid}') it did not require."); - } - return $this->handles[$phid]; - } - - public function getMarkupFields() { - return array(); - } - - public function setMarkupEngine(PhabricatorMarkupEngine $engine) { - $this->markupEngine = $engine; - return $this; - } - - public function getMarkupEngine() { - return $this->markupEngine; - } - - -/* -( PhabricatorMarkupInterface )----------------------------------------- */ - - - public function getMarkupFieldKey($field) { - $hash = PhabricatorHash::digestForIndex($this->getMarkupText($field)); - return 'maux:'.$this->getAuxiliaryKey().':'.$hash; - } - - - public function newMarkupEngine($field) { - return PhabricatorMarkupEngine::newManiphestMarkupEngine(); - } - - - public function getMarkupText($field) { - return $this->getValue(); - } - - public function didMarkupText( - $field, - $output, - PhutilMarkupEngine $engine) { - return phutil_tag( - 'div', - array( - 'class' => 'phabricator-remarkup', - ), - $output); - } - - public function shouldUseMarkupCache($field) { - return true; - } - - -/* -( API Compatibility With New Custom Fields )--------------------------- */ - - - public function getFieldKey() { - return $this->getAuxiliaryKey(); - } - - public function shouldAppearInEditView() { - return true; - } - - public function shouldAppearInPropertyView() { - return true; - } - - public function shouldUseStorage() { - return true; - } - - public function renderPropertyViewValue() { - $value = $this->renderForDetailView(); - if (!strlen($value)) { - return null; - } - return $value; - } - - public function renderPropertyViewLabel() { - return $this->getLabel(); - } - - public function readValueFromRequest(AphrontRequest $request) { - return $this->setValueFromRequest($request); - } +final class ManiphestAuxiliaryFieldSpecification { public static function writeLegacyAuxiliaryUpdates( ManiphestTask $task, diff --git a/src/applications/maniphest/controller/ManiphestTaskDetailController.php b/src/applications/maniphest/controller/ManiphestTaskDetailController.php index 3f8f2ab43b..094454952f 100644 --- a/src/applications/maniphest/controller/ManiphestTaskDetailController.php +++ b/src/applications/maniphest/controller/ManiphestTaskDetailController.php @@ -98,16 +98,9 @@ final class ManiphestTaskDetailController extends ManiphestController { $phids = array_keys($phids); - $phids = array_merge( - $phids, - array_mergev(mpull($aux_fields, 'getRequiredHandlePHIDs'))); - $this->loadHandles($phids); $handles = $this->getLoadedHandles(); - foreach ($aux_fields as $aux_field) { - $aux_field->setHandles($handles); - } $context_bar = null; @@ -152,13 +145,6 @@ final class ManiphestTaskDetailController extends ManiphestController { } } - foreach ($aux_fields as $aux_field) { - foreach ($aux_field->getMarkupFields() as $markup_field) { - $engine->addObject($aux_field, $markup_field); - $aux_field->setMarkupEngine($engine); - } - } - $engine->process(); $transaction_types = ManiphestTransactionType::getTransactionTypeMap(); diff --git a/src/applications/maniphest/controller/ManiphestTaskEditController.php b/src/applications/maniphest/controller/ManiphestTaskEditController.php index 359bf95e05..a9839cdc55 100644 --- a/src/applications/maniphest/controller/ManiphestTaskEditController.php +++ b/src/applications/maniphest/controller/ManiphestTaskEditController.php @@ -354,8 +354,7 @@ final class ManiphestTaskEditController extends ManiphestController { $phids = array_merge( array($task->getOwnerPHID()), $task->getCCPHIDs(), - $task->getProjectPHIDs(), - array_mergev(mpull($aux_fields, 'getRequiredHandlePHIDs'))); + $task->getProjectPHIDs()); if ($parent_task) { $phids[] = $parent_task->getPHID(); @@ -366,10 +365,6 @@ final class ManiphestTaskEditController extends ManiphestController { $handles = $this->loadViewerHandles($phids); - foreach ($aux_fields as $aux_field) { - $aux_field->setHandles($handles); - } - $tvalues = mpull($handles, 'getFullName', 'getPHID'); $error_view = null; diff --git a/src/applications/maniphest/field/ManiphestCustomField.php b/src/applications/maniphest/field/ManiphestCustomField.php index d2368e9a8d..cd51f67b93 100644 --- a/src/applications/maniphest/field/ManiphestCustomField.php +++ b/src/applications/maniphest/field/ManiphestCustomField.php @@ -28,70 +28,23 @@ abstract class ManiphestCustomField return false; } + // TODO: All of this is legacy junk. - public function getRequiredHandlePHIDs() { - return array(); - } - public function setHandles(array $handles) { - } - - /** - * Render a verb to appear in email titles when a transaction involving this - * field occurs. Specifically, Maniphest emails are formatted like this: - * - * [Maniphest] [Verb Here] TNNN: Task title here - * ^^^^^^^^^ - * - * You should optionally return a title-case verb or short phrase like - * "Created", "Retitled", "Closed", "Resolved", "Commented On", - * "Lowered Priority", etc., which describes the transaction. - * - * @param ManiphestTransaction The transaction which needs description. - * @return string|null A short description of the transaction. - */ public function renderTransactionEmailVerb( ManiphestTransaction $transaction) { return null; } - - /** - * Render a short description of the transaction, to appear above comments - * in the Maniphest transaction log. The string will be rendered after the - * acting user's name. Examples are: - * - * added a comment - * added alincoln to CC - * claimed this task - * created this task - * closed this task out of spite - * - * You should return a similar string, describing the transaction. - * - * Note the ##$target## parameter -- Maniphest needs to render transaction - * descriptions for different targets, like web and email. This method will - * be called with a ##ManiphestAuxiliaryFieldSpecification::RENDER_TARGET_*## - * constant describing the intended target. - * - * @param ManiphestTransaction The transaction which needs description. - * @param const Constant describing the rendering target (e.g., html or text). - * @return string|null Description of the transaction. - */ public function renderTransactionDescription( - ManiphestTransaction $transaction, - $target) { + ManiphestTransaction $transaction) { $old = $transaction->getOldValue(); $new = $transaction->getNewValue(); return pht( 'updated field %s from %s to %s', $this->getFieldName(), - $old, - $new); - } - - public function getMarkupFields() { - return array(); + hsprintf('%s', $old), + hsprintf('%s', $new)); } } diff --git a/src/applications/maniphest/view/ManiphestTransactionDetailView.php b/src/applications/maniphest/view/ManiphestTransactionDetailView.php index b4feeb349c..986601d101 100644 --- a/src/applications/maniphest/view/ManiphestTransactionDetailView.php +++ b/src/applications/maniphest/view/ManiphestTransactionDetailView.php @@ -541,19 +541,11 @@ final class ManiphestTransactionDetailView extends ManiphestView { $desc = null; if ($aux_field) { - $use_field = $aux_field; + $desc = $aux_field->renderTransactionDescription($transaction); } else { - $use_field = id(new ManiphestAuxiliaryFieldDefaultSpecification()) - ->setFieldType( - ManiphestAuxiliaryFieldDefaultSpecification::TYPE_STRING); + $desc = 'updated a field'; } - $desc = $use_field->renderTransactionDescription( - $transaction, - $this->forEmail - ? ManiphestAuxiliaryFieldSpecification::RENDER_TARGET_TEXT - : ManiphestAuxiliaryFieldSpecification::RENDER_TARGET_HTML); - break; default: return array($type, ' brazenly '.$type."'d", $classes);