From 0b2d25778d8f71a860c4622d2e92e76f1bd47351 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 13 Feb 2019 09:13:46 -0800 Subject: [PATCH] Add basic, rough support for changing field behavior based on object subtype Summary: Ref T13248. This will probably need quite a bit of refinement, but we can reasonably allow subtype definitions to adjust custom field behavior. Some places where we use fields are global, and always need to show all the fields. For example, on `/maniphest/`, where you can search across all tasks, you need to be able to search across all fields that are present on any task. Likewise, if you "export" a bunch of tasks into a spreadsheet, we need to have columns for every field. However, when you're clearly in the scope of a particular task (like viewing or editing `T123`), there's no reason we can't hide fields based on the task subtype. To start with, allow subtypes to override "disabled" and "name" for custom fields. Test Plan: - Defined several custom fields and several subtypes. - Disabled/renamed some fields for some subtypes. - Viewed/edited tasks of different subtypes, got desired field behavior. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13248 Differential Revision: https://secure.phabricator.com/D20161 --- .../PhabricatorManiphestConfigOptions.php | 23 +++++ .../editengine/PhabricatorEditEngine.php | 19 +++- .../PhabricatorEditEngineSubtype.php | 34 ++++++++ .../field/PhabricatorCustomField.php | 87 +++++++++++++++++++ .../PhabricatorStandardCustomField.php | 14 +++ 5 files changed, 175 insertions(+), 2 deletions(-) diff --git a/src/applications/maniphest/config/PhabricatorManiphestConfigOptions.php b/src/applications/maniphest/config/PhabricatorManiphestConfigOptions.php index 8f2830908b..05f98a2f62 100644 --- a/src/applications/maniphest/config/PhabricatorManiphestConfigOptions.php +++ b/src/applications/maniphest/config/PhabricatorManiphestConfigOptions.php @@ -342,6 +342,7 @@ dictionary with these keys: - `icon` //Optional string.// Icon for the subtype. - `children` //Optional map.// Configure options shown to the user when they "Create Subtask". See below. + - `fields` //Optional map.// Configure field behaviors. See below. Each subtype must have a unique key, and you must define a subtype with the key "%s", which is used as a default subtype. @@ -397,6 +398,28 @@ be used when presenting options to the user. If only one option would be presented, the user will be taken directly to the appropriate form instead of being prompted to choose a form. + +The `fields` key can configure the behavior of custom fields on specific +task subtypes. For example: + +``` +{ + ... + "fields": { + "custom.some-field": { + "disabled": true + } + } + ... +} +``` + +Each field supports these options: + + - `disabled` //Optional bool.// Allows you to disable fields on certain + subtypes. + - `name` //Optional string.// Custom name of this field for the subtype. + EOTEXT , $subtype_default_key)); diff --git a/src/applications/transactions/editengine/PhabricatorEditEngine.php b/src/applications/transactions/editengine/PhabricatorEditEngine.php index 353d7ee385..82af45dca8 100644 --- a/src/applications/transactions/editengine/PhabricatorEditEngine.php +++ b/src/applications/transactions/editengine/PhabricatorEditEngine.php @@ -165,14 +165,29 @@ abstract class PhabricatorEditEngine $extensions = array(); } + // See T13248. Create a template object to provide to extensions. We + // adjust the template to have the intended subtype, so that extensions + // may change behavior based on the form subtype. + + $template_object = clone $object; + if ($this->getIsCreate()) { + if ($this->supportsSubtypes()) { + $config = $this->getEditEngineConfiguration(); + $subtype = $config->getSubtype(); + $template_object->setSubtype($subtype); + } + } + foreach ($extensions as $extension) { $extension->setViewer($viewer); - if (!$extension->supportsObject($this, $object)) { + if (!$extension->supportsObject($this, $template_object)) { continue; } - $extension_fields = $extension->buildCustomEditFields($this, $object); + $extension_fields = $extension->buildCustomEditFields( + $this, + $template_object); // TODO: Validate this in more detail with a more tailored error. assert_instances_of($extension_fields, 'PhabricatorEditField'); diff --git a/src/applications/transactions/editengine/PhabricatorEditEngineSubtype.php b/src/applications/transactions/editengine/PhabricatorEditEngineSubtype.php index 9e754a3ca8..6e1d1de115 100644 --- a/src/applications/transactions/editengine/PhabricatorEditEngineSubtype.php +++ b/src/applications/transactions/editengine/PhabricatorEditEngineSubtype.php @@ -13,6 +13,7 @@ final class PhabricatorEditEngineSubtype private $color; private $childSubtypes = array(); private $childIdentifiers = array(); + private $fieldConfiguration = array(); public function setKey($key) { $this->key = $key; @@ -94,6 +95,17 @@ final class PhabricatorEditEngineSubtype return $view; } + public function setSubtypeFieldConfiguration( + $subtype_key, + array $configuration) { + $this->fieldConfiguration[$subtype_key] = $configuration; + return $this; + } + + public function getSubtypeFieldConfiguration($subtype_key) { + return idx($this->fieldConfiguration, $subtype_key); + } + public static function validateSubtypeKey($subtype) { if (strlen($subtype) > 64) { throw new Exception( @@ -139,6 +151,7 @@ final class PhabricatorEditEngineSubtype 'color' => 'optional string', 'icon' => 'optional string', 'children' => 'optional map', + 'fields' => 'optional map', )); $key = $value['key']; @@ -183,6 +196,18 @@ final class PhabricatorEditEngineSubtype 'or the other, but not both.')); } } + + $fields = idx($value, 'fields'); + if ($fields) { + foreach ($fields as $field_key => $configuration) { + PhutilTypeSpec::checkMap( + $configuration, + array( + 'disabled' => 'optional bool', + 'name' => 'optional string', + )); + } + } } if (!isset($map[self::SUBTYPE_DEFAULT])) { @@ -233,6 +258,15 @@ final class PhabricatorEditEngineSubtype $subtype->setChildFormIdentifiers($child_forms); } + $field_configurations = idx($entry, 'fields'); + if ($field_configurations) { + foreach ($field_configurations as $field_key => $field_configuration) { + $subtype->setSubtypeFieldConfiguration( + $field_key, + $field_configuration); + } + } + $map[$key] = $subtype; } diff --git a/src/infrastructure/customfield/field/PhabricatorCustomField.php b/src/infrastructure/customfield/field/PhabricatorCustomField.php index d7df3c5b78..c6c70a9614 100644 --- a/src/infrastructure/customfield/field/PhabricatorCustomField.php +++ b/src/infrastructure/customfield/field/PhabricatorCustomField.php @@ -74,9 +74,22 @@ abstract class PhabricatorCustomField extends Phobject { $spec, $object); + $fields = self::adjustCustomFieldsForObjectSubtype( + $object, + $role, + $fields); + foreach ($fields as $key => $field) { + // NOTE: We perform this filtering in "buildFieldList()", but may need + // to filter again after subtype adjustment. + if (!$field->isFieldEnabled()) { + unset($fields[$key]); + continue; + } + if (!$field->shouldEnableForRole($role)) { unset($fields[$key]); + continue; } } @@ -1622,4 +1635,78 @@ abstract class PhabricatorCustomField extends Phobject { return null; } + private static function adjustCustomFieldsForObjectSubtype( + PhabricatorCustomFieldInterface $object, + $role, + array $fields) { + assert_instances_of($fields, __CLASS__); + + // We only apply subtype adjustment for some roles. For example, when + // writing Herald rules or building a Search interface, we always want to + // show all the fields in their default state, so we do not apply any + // adjustments. + $subtype_roles = array( + self::ROLE_EDITENGINE, + self::ROLE_VIEW, + ); + + $subtype_roles = array_fuse($subtype_roles); + if (!isset($subtype_roles[$role])) { + return $fields; + } + + // If the object doesn't support subtypes, we can't possibly make + // any adjustments based on subtype. + if (!($object instanceof PhabricatorEditEngineSubtypeInterface)) { + return $fields; + } + + $subtype_map = $object->newEditEngineSubtypeMap(); + $subtype_key = $object->getEditEngineSubtype(); + $subtype_object = $subtype_map->getSubtype($subtype_key); + + $map = array(); + foreach ($fields as $field) { + $modern_key = $field->getModernFieldKey(); + if (!strlen($modern_key)) { + continue; + } + + $map[$modern_key] = $field; + } + + foreach ($map as $field_key => $field) { + // For now, only support overriding standard custom fields. In the + // future there's no technical or product reason we couldn't let you + // override (some properites of) other fields like "Title", but they + // don't usually support appropriate "setX()" methods today. + if (!($field instanceof PhabricatorStandardCustomField)) { + // For fields that are proxies on top of StandardCustomField, which + // is how most application custom fields work today, we can reconfigure + // the proxied field instead. + $field = $field->getProxy(); + if (!$field || !($field instanceof PhabricatorStandardCustomField)) { + continue; + } + } + + $subtype_config = $subtype_object->getSubtypeFieldConfiguration( + $field_key); + + if (!$subtype_config) { + continue; + } + + if (isset($subtype_config['disabled'])) { + $field->setIsEnabled(!$subtype_config['disabled']); + } + + if (isset($subtype_config['name'])) { + $field->setFieldName($subtype_config['name']); + } + } + + return $fields; + } + } diff --git a/src/infrastructure/customfield/standard/PhabricatorStandardCustomField.php b/src/infrastructure/customfield/standard/PhabricatorStandardCustomField.php index 5bd6256b73..4c0bce861b 100644 --- a/src/infrastructure/customfield/standard/PhabricatorStandardCustomField.php +++ b/src/infrastructure/customfield/standard/PhabricatorStandardCustomField.php @@ -18,6 +18,7 @@ abstract class PhabricatorStandardCustomField private $isCopyable; private $hasStorageValue; private $isBuiltin; + private $isEnabled = true; abstract public function getFieldType(); @@ -175,6 +176,19 @@ abstract class PhabricatorStandardCustomField return $this->rawKey; } + public function setIsEnabled($is_enabled) { + $this->isEnabled = $is_enabled; + return $this; + } + + public function getIsEnabled() { + return $this->isEnabled; + } + + public function isFieldEnabled() { + return $this->getIsEnabled(); + } + /* -( PhabricatorCustomField )--------------------------------------------- */