Page MenuHomePhabricator
Files F15416284

D8584.id20362.diff
No OneTemporary
Actions

D8584.id20362.diff
View Options

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -898,6 +898,7 @@
'ManiphestTaskQuery' => 'applications/maniphest/query/ManiphestTaskQuery.php',
'ManiphestTaskSearchEngine' => 'applications/maniphest/query/ManiphestTaskSearchEngine.php',
'ManiphestTaskStatus' => 'applications/maniphest/constants/ManiphestTaskStatus.php',
+ 'ManiphestTaskStatusTestCase' => 'applications/maniphest/constants/__tests__/ManiphestTaskStatusTestCase.php',
'ManiphestTaskSubscriber' => 'applications/maniphest/storage/ManiphestTaskSubscriber.php',
'ManiphestTransaction' => 'applications/maniphest/storage/ManiphestTransaction.php',
'ManiphestTransactionComment' => 'applications/maniphest/storage/ManiphestTransactionComment.php',
@@ -3548,6 +3549,7 @@
'ManiphestTaskQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
'ManiphestTaskSearchEngine' => 'PhabricatorApplicationSearchEngine',
'ManiphestTaskStatus' => 'ManiphestConstants',
+ 'ManiphestTaskStatusTestCase' => 'PhabricatorTestCase',
'ManiphestTaskSubscriber' => 'ManiphestDAO',
'ManiphestTransaction' => 'PhabricatorApplicationTransaction',
'ManiphestTransactionComment' => 'PhabricatorApplicationTransactionComment',
diff --git a/src/applications/maniphest/constants/ManiphestTaskStatus.php b/src/applications/maniphest/constants/ManiphestTaskStatus.php
--- a/src/applications/maniphest/constants/ManiphestTaskStatus.php
+++ b/src/applications/maniphest/constants/ManiphestTaskStatus.php
@@ -1,5 +1,8 @@
<?php
+/**
+ * @task validate Configuration Validation
+ */
final class ManiphestTaskStatus extends ManiphestConstants {
const STATUS_OPEN = 'open';
@@ -151,7 +154,7 @@
}
private static function getSpecialStatus($special) {
- foreach (self::getEnabledStatusMap() as $const => $status) {
+ foreach (self::getStatusConfig() as $const => $status) {
if (idx($status, 'special') == $special) {
return $const;
}
@@ -251,4 +254,136 @@
return $default;
}
+
+/* -( Configuration Validation )------------------------------------------- */
+
+
+ /**
+ * @task validate
+ */
+ public static function isValidStatusConstant($constant) {
+ if (strlen($constant) > 12) {
+ return false;
+ }
+ if (!preg_match('/^[a-z0-9]+\z/', $constant)) {
+ return false;
+ }
+ return true;
+ }
+
+
+ /**
+ * @task validate
+ */
+ public static function validateConfiguration(array $config) {
+ foreach ($config as $key => $value) {
+ if (!self::isValidStatusConstant($key)) {
+ throw new Exception(
+ pht(
+ 'Key "%s" is not a valid status constant. Status constants must '.
+ 'be 1-12 characters long and contain only lowercase letters (a-z) '.
+ 'and digits (0-9). For example, "%s" or "%s" are reasonable '.
+ 'choices.',
+ $key,
+ 'open',
+ 'closed'));
+ }
+ if (!is_array($value)) {
+ throw new Exception(
+ pht(
+ 'Value for key "%s" should be a dictionary.',
+ $key));
+ }
+
+ PhutilTypeSpec::checkMap(
+ $value,
+ array(
+ 'name' => 'string',
+ 'name.full' => 'optional string',
+ 'name.action' => 'optional string',
+ 'closed' => 'optional bool',
+ 'special' => 'optional string',
+ 'transaction.icon' => 'optional string',
+ 'transaction.color' => 'optional string',
+ 'silly' => 'optional bool',
+ 'prefixes' => 'optional list<string>',
+ 'suffixes' => 'optional list<string>',
+ ));
+ }
+
+ $special_map = array();
+ foreach ($config as $key => $value) {
+ $special = idx($value, 'special');
+ if (!$special) {
+ continue;
+ }
+
+ if (isset($special_map[$special])) {
+ throw new Exception(
+ pht(
+ 'Configuration has two statuses both marked with the special '.
+ 'attribute "%s" ("%s" and "%s"). There should be only one.',
+ $special,
+ $special_map[$special],
+ $key));
+ }
+
+ switch ($special) {
+ case self::SPECIAL_DEFAULT:
+ if (!empty($value['closed'])) {
+ throw new Exception(
+ pht(
+ 'Status "%s" is marked as default, but it is a closed '.
+ 'status. The default status should be an open status.',
+ $key));
+ }
+ break;
+ case self::SPECIAL_CLOSED:
+ if (empty($value['closed'])) {
+ throw new Exception(
+ pht(
+ 'Status "%s" is marked as the default status for closing '.
+ 'tasks, but is not a closed status. It should be a closed '.
+ 'status.',
+ $key));
+ }
+ break;
+ case self::SPECIAL_DUPLICATE:
+ if (empty($value['closed'])) {
+ throw new Exception(
+ pht(
+ 'Status "%s" is marked as the status for closing tasks as '.
+ 'duplicates, but it is not a closed status. It should '.
+ 'be a closed status.',
+ $key));
+ }
+ break;
+ }
+
+ $special_map[$special] = $key;
+ }
+
+ // NOTE: We're not explicitly validating that we have at least one open
+ // and one closed status, because the DEFAULT and CLOSED specials imply
+ // that to be true. If those change in the future, that might become a
+ // reasonable thing to validate.
+
+ $required = array(
+ self::SPECIAL_DEFAULT,
+ self::SPECIAL_CLOSED,
+ self::SPECIAL_DUPLICATE,
+ );
+
+ foreach ($required as $required_special) {
+ if (!isset($special_map[$required_special])) {
+ throw new Exception(
+ pht(
+ 'Configuration defines no task status with special attribute '.
+ '"%s", but you must specify a status which fills this special '.
+ 'role.',
+ $required_special));
+ }
+ }
+ }
+
}
diff --git a/src/applications/maniphest/constants/__tests__/ManiphestTaskStatusTestCase.php b/src/applications/maniphest/constants/__tests__/ManiphestTaskStatusTestCase.php
new file mode 100644
--- /dev/null
+++ b/src/applications/maniphest/constants/__tests__/ManiphestTaskStatusTestCase.php
@@ -0,0 +1,114 @@
+<?php
+
+final class ManiphestTaskStatusTestCase extends PhabricatorTestCase {
+
+ public function testManiphestStatusConstants() {
+ $map = array(
+ 'y' => true,
+ 'closed' => true,
+ 'longlonglong' => true,
+ 'duplicate2' => true,
+
+ '' => false,
+ 'longlonglonglong' => false,
+ '.' => false,
+ 'ABCD' => false,
+ 'a b c ' => false,
+ );
+
+ foreach ($map as $input => $expect) {
+ $this->assertEqual(
+ $expect,
+ ManiphestTaskStatus::isValidStatusConstant($input),
+ pht('Validate "%s"', $input));
+ }
+ }
+
+ public function testManiphestStatusConfigValidation() {
+ $this->assertConfigValid(
+ false,
+ pht('Empty'),
+ array());
+
+ // This is a minimal, valid configuration.
+
+ $valid = array(
+ 'open' => array(
+ 'name' => 'Open',
+ 'special' => 'default',
+ ),
+ 'closed' => array(
+ 'name' => 'Closed',
+ 'special' => 'closed',
+ 'closed' => true,
+ ),
+ 'duplicate' => array(
+ 'name' => 'Duplicate',
+ 'special' => 'duplicate',
+ 'closed' => true,
+ ),
+ );
+ $this->assertConfigValid(true, pht('Minimal Valid Config'), $valid);
+
+ // We should raise on a bad key.
+ $bad_key = $valid;
+ $bad_key['!'] = array('name' => 'Exclaim');
+ $this->assertConfigValid(false, pht('Bad Key'), $bad_key);
+
+ // We should raise on a value type.
+ $bad_type = $valid;
+ $bad_type['other'] = 'other';
+ $this->assertConfigValid(false, pht('Bad Value Type'), $bad_type);
+
+ // We should raise on an unknown configuration key.
+ $invalid_key = $valid;
+ $invalid_key['open']['imaginary'] = 'unicorn';
+ $this->assertConfigValid(false, pht('Invalid Key'), $invalid_key);
+
+ // We should raise on two statuses with the same special.
+ $double_close = $valid;
+ $double_close['finished'] = array(
+ 'name' => 'Finished',
+ 'special' => 'closed',
+ 'closed' => true,
+ );
+ $this->assertConfigValid(false, pht('Duplicate Special'), $double_close);
+
+ // We should raise if any of the special statuses are missing.
+ foreach ($valid as $key => $config) {
+ $missing = $valid;
+ unset($missing[$key]);
+ $this->assertConfigValid(false, pht('Missing Special'), $missing);
+ }
+
+ // The "default" special should be an open status.
+ $closed_default = $valid;
+ $closed_default['open']['closed'] = true;
+ $this->assertConfigValid(false, pht('Closed Default'), $closed_default);
+
+ // The "closed" special should be a closed status.
+ $open_closed = $valid;
+ $open_closed['closed']['closed'] = false;
+ $this->assertConfigValid(false, pht('Open Closed'), $open_closed);
+
+ // The "duplicate" special should be a closed status.
+ $open_duplicate = $valid;
+ $open_duplicate['duplicate']['closed'] = false;
+ $this->assertConfigValid(false, pht('Open Duplicate'), $open_duplicate);
+ }
+
+ private function assertConfigValid($expect, $name, array $config) {
+ $caught = null;
+ try {
+ ManiphestTaskStatus::validateConfiguration($config);
+ } catch (Exception $ex) {
+ $caught = $ex;
+ }
+
+ $this->assertEqual(
+ $expect,
+ !($caught instanceof Exception),
+ pht('Validation of "%s"', $name));
+ }
+
+}

File Metadata

Mime Type
text/plain
Expires
Fri, Mar 21, 10:18 AM (1 w, 6 d ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
7715343
Default Alt Text
D8584.id20362.diff (9 KB)

Event Timeline

Phabricator · Built by Phacility