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', @@ -3549,6 +3550,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 @@ $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', + 'suffixes' => 'optional list', + )); + } + + $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 @@ + 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)); + } + +}