From c1558031c29063d0195f78b0838522dac221e8fb Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 11 Apr 2018 08:36:05 -0700 Subject: [PATCH] Make various small quality-of-life improvements for Almanac properties Summary: Depends on D19341. Ref T12414. Ref T13120. - Fix a bug where default-valued properties didn't get rendered in grey as they're supposed to (as a hint that the value isn't customized). - When resetting a builtin property won't do anything, visually disable the button as a hint. - Allow Services to specify properties on their Bindings. - Specify that repository bindings have a "protocol" property, so it becomes an explicit thing in the UI. Previously, you had to read the documentation to figure this out. - When editing bindings, use the EditField and its configuration if possible. This turns the "Protocol" property into a dropdown in the UI where you select between "http", "https" and "ssh". - Give the "protocol" binding a smart default based on the port number of the corresponding interface. Test Plan: - Viewed properties on Services, Devices and Bindings. - Saw them render sensibly, and grey out + grey button when a builtin value has a default setting. - Saw "Protocol" appear as a default property on repository cluster bindings and get a smart value. - Edited "protocol", got a nice dropdown. {F5518791} Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13120, T12414 Differential Revision: https://secure.phabricator.com/D19342 --- .../almanac/controller/AlmanacController.php | 5 +-- .../editor/AlmanacPropertyEditEngine.php | 9 +++++- .../almanac/query/AlmanacQuery.php | 4 ++- .../AlmanacClusterRepositoryServiceType.php | 32 +++++++++++++++++++ .../servicetype/AlmanacServiceType.php | 4 +++ .../almanac/storage/AlmanacBinding.php | 6 +++- .../almanac/storage/AlmanacService.php | 5 +++ 7 files changed, 60 insertions(+), 5 deletions(-) diff --git a/src/applications/almanac/controller/AlmanacController.php b/src/applications/almanac/controller/AlmanacController.php index d6d7188511..24a8986766 100644 --- a/src/applications/almanac/controller/AlmanacController.php +++ b/src/applications/almanac/controller/AlmanacController.php @@ -38,7 +38,7 @@ abstract class AlmanacController )); $builtins = $object->getAlmanacPropertyFieldSpecifications(); - $defaults = mpull($builtins, null, 'getValueForTransaction'); + $defaults = mpull($builtins, 'getValueForTransaction'); // Sort fields so builtin fields appear first, then fields are ordered // alphabetically. @@ -65,6 +65,7 @@ abstract class AlmanacController $value = $property->getFieldValue(); $is_builtin = isset($builtins[$key]); + $is_persistent = (bool)$property->getID(); $delete_uri = id(new PhutilURI($delete_base)) ->setQueryParams( @@ -83,7 +84,7 @@ abstract class AlmanacController $delete = javelin_tag( 'a', array( - 'class' => ($can_edit + 'class' => (($can_edit && $is_persistent) ? 'button button-grey small' : 'button button-grey small disabled'), 'sigil' => 'workflow', diff --git a/src/applications/almanac/editor/AlmanacPropertyEditEngine.php b/src/applications/almanac/editor/AlmanacPropertyEditEngine.php index e5bfd2822a..38139f79fa 100644 --- a/src/applications/almanac/editor/AlmanacPropertyEditEngine.php +++ b/src/applications/almanac/editor/AlmanacPropertyEditEngine.php @@ -66,8 +66,15 @@ abstract class AlmanacPropertyEditEngine $property_key = $this->getPropertyKey(); $xaction_type = $object->getAlmanacPropertySetTransactionType(); + $specs = $object->getAlmanacPropertyFieldSpecifications(); + if (isset($specs[$property_key])) { + $field_template = clone $specs[$property_key]; + } else { + $field_template = new PhabricatorTextEditField(); + } + return array( - id(new PhabricatorTextEditField()) + $field_template ->setKey('value') ->setMetadataValue('almanac.property', $property_key) ->setLabel($property_key) diff --git a/src/applications/almanac/query/AlmanacQuery.php b/src/applications/almanac/query/AlmanacQuery.php index b046171d32..b5b6146c7f 100644 --- a/src/applications/almanac/query/AlmanacQuery.php +++ b/src/applications/almanac/query/AlmanacQuery.php @@ -34,10 +34,12 @@ abstract class AlmanacQuery $specs = $object->getAlmanacPropertyFieldSpecifications(); foreach ($specs as $key => $spec) { if (empty($object_properties[$key])) { + $default_value = $spec->getValueForTransaction(); + $object_properties[$key] = id(new AlmanacProperty()) ->setObjectPHID($object->getPHID()) ->setFieldName($key) - ->setFieldValue($spec->getValueForTransaction()); + ->setFieldValue($default_value); } } diff --git a/src/applications/almanac/servicetype/AlmanacClusterRepositoryServiceType.php b/src/applications/almanac/servicetype/AlmanacClusterRepositoryServiceType.php index 259092b5b1..fffe94997a 100644 --- a/src/applications/almanac/servicetype/AlmanacClusterRepositoryServiceType.php +++ b/src/applications/almanac/servicetype/AlmanacClusterRepositoryServiceType.php @@ -24,4 +24,36 @@ final class AlmanacClusterRepositoryServiceType ); } + public function getBindingFieldSpecifications(AlmanacBinding $binding) { + $protocols = array( + array( + 'value' => 'http', + 'port' => 80, + ), + array( + 'value' => 'https', + 'port' => 443, + ), + array( + 'value' => 'ssh', + 'port' => 22, + ), + ); + + $default_value = 'http'; + if ($binding->hasInterface()) { + $interface = $binding->getInterface(); + $port = $interface->getPort(); + + $default_ports = ipull($protocols, 'value', 'port'); + $default_value = idx($default_ports, $port, $default_value); + } + + return array( + 'protocol' => id(new PhabricatorSelectEditField()) + ->setOptions(ipull($protocols, 'value', 'value')) + ->setValue($default_value), + ); + } + } diff --git a/src/applications/almanac/servicetype/AlmanacServiceType.php b/src/applications/almanac/servicetype/AlmanacServiceType.php index 2eb56dc8c4..2f7f503fe8 100644 --- a/src/applications/almanac/servicetype/AlmanacServiceType.php +++ b/src/applications/almanac/servicetype/AlmanacServiceType.php @@ -60,6 +60,10 @@ abstract class AlmanacServiceType extends Phobject { return array(); } + public function getBindingFieldSpecifications(AlmanacBinding $binding) { + return array(); + } + /** * List all available service type implementations. * diff --git a/src/applications/almanac/storage/AlmanacBinding.php b/src/applications/almanac/storage/AlmanacBinding.php index be70a5ecdc..c593e40fa7 100644 --- a/src/applications/almanac/storage/AlmanacBinding.php +++ b/src/applications/almanac/storage/AlmanacBinding.php @@ -88,6 +88,10 @@ final class AlmanacBinding return $this; } + public function hasInterface() { + return ($this->interface !== self::ATTACHABLE); + } + public function getInterface() { return $this->assertAttached($this->interface); } @@ -129,7 +133,7 @@ final class AlmanacBinding } public function getAlmanacPropertyFieldSpecifications() { - return array(); + return $this->getService()->getBindingFieldSpecifications($this); } public function newAlmanacPropertyEditEngine() { diff --git a/src/applications/almanac/storage/AlmanacService.php b/src/applications/almanac/storage/AlmanacService.php index 361eabca2a..ee40d83400 100644 --- a/src/applications/almanac/storage/AlmanacService.php +++ b/src/applications/almanac/storage/AlmanacService.php @@ -156,6 +156,11 @@ final class AlmanacService return $this->getServiceImplementation()->getFieldSpecifications(); } + public function getBindingFieldSpecifications(AlmanacBinding $binding) { + $impl = $this->getServiceImplementation(); + return $impl->getBindingFieldSpecifications($binding); + } + public function newAlmanacPropertyEditEngine() { return new AlmanacServicePropertyEditEngine(); }