mirror of
https://we.phorge.it/source/phorge.git
synced 2024-12-22 21:40:55 +01:00
Add config validation for task status config
Summary: Ref T1812. This still doesn't expose configuration to the user, but adds validation for it. Test Plan: Added a pile of unit tests. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T1812 Differential Revision: https://secure.phabricator.com/D8584
This commit is contained in:
epriestley
parent
07fdcde87e
commit
7713fb5d99
3 changed files with 252 additions and 1 deletions
|
|
@ -898,6 +898,7 @@ phutil_register_library_map(array(
|
|||
'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 @@ phutil_register_library_map(array(
|
|||
'ManiphestTaskQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
|
||||
'ManiphestTaskSearchEngine' => 'PhabricatorApplicationSearchEngine',
|
||||
'ManiphestTaskStatus' => 'ManiphestConstants',
|
||||
'ManiphestTaskStatusTestCase' => 'PhabricatorTestCase',
|
||||
'ManiphestTaskSubscriber' => 'ManiphestDAO',
|
||||
'ManiphestTransaction' => 'PhabricatorApplicationTransaction',
|
||||
'ManiphestTransactionComment' => 'PhabricatorApplicationTransactionComment',
|
||||
|
|
|
|||
|
|
@ -1,5 +1,8 @@
|
|||
<?php
|
||||
|
||||
/**
|
||||
* @task validate Configuration Validation
|
||||
*/
|
||||
final class ManiphestTaskStatus extends ManiphestConstants {
|
||||
|
||||
const STATUS_OPEN = 'open';
|
||||
|
|
@ -151,7 +154,7 @@ final class ManiphestTaskStatus extends ManiphestConstants {
|
|||
}
|
||||
|
||||
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 @@ final class ManiphestTaskStatus extends ManiphestConstants {
|
|||
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));
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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));
|
||||
}
|
||||
|
||||
}
|
||||
Loading…
Reference in a new issue