Use keywords instead of ints to update task priority in ManiphestEditEngine

Summary: Fixes T12124. Changes `ManiphestEditEngine` to populate the select using priority keywords instead of the integer value. Marks `maniphest.querystatuses` as frozen. Adds a new Conduit method for fetching potential task statuses.

Test Plan: Created tasks and changed their priorities, observed that transactions in the DB still have the same type (integers as strings). Invoked `maniphest.update` with `priority => '90'` and observed that it still works. Invoked `maniphest.edit` with `priority => 'unbreak'` and observed that it now works.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, epriestley

Maniphest Tasks: T12124

Differential Revision: https://secure.phabricator.com/D18111

resources/sql/autopatches/20170614.taskstatus.sql

@@ -0,0 +1,4 @@
+/* Extend from 12 characters to 64. */
+
+ALTER TABLE {$NAMESPACE}_maniphest.maniphest_task
+  CHANGE status status VARCHAR(64) COLLATE {$COLLATE_TEXT} NOT NULL;

src/__phutil_library_map__.php

@@ -1507,6 +1507,7 @@ phutil_register_library_map(array(
     'ManiphestSearchConduitAPIMethod' => 'applications/maniphest/conduit/ManiphestSearchConduitAPIMethod.php',
     'ManiphestStatusConfigOptionType' => 'applications/maniphest/config/ManiphestStatusConfigOptionType.php',
     'ManiphestStatusEmailCommand' => 'applications/maniphest/command/ManiphestStatusEmailCommand.php',
+    'ManiphestStatusSearchConduitAPIMethod' => 'applications/maniphest/conduit/ManiphestStatusSearchConduitAPIMethod.php',
     'ManiphestSubpriorityController' => 'applications/maniphest/controller/ManiphestSubpriorityController.php',
     'ManiphestSubtypesConfigOptionsType' => 'applications/maniphest/config/ManiphestSubtypesConfigOptionsType.php',
     'ManiphestTask' => 'applications/maniphest/storage/ManiphestTask.php',
@@ -6607,6 +6608,7 @@ phutil_register_library_map(array(
     'ManiphestSearchConduitAPIMethod' => 'PhabricatorSearchEngineAPIMethod',
     'ManiphestStatusConfigOptionType' => 'PhabricatorConfigJSONOptionType',
     'ManiphestStatusEmailCommand' => 'ManiphestEmailCommand',
+    'ManiphestStatusSearchConduitAPIMethod' => 'ManiphestConduitAPIMethod',
     'ManiphestSubpriorityController' => 'ManiphestController',
     'ManiphestSubtypesConfigOptionsType' => 'PhabricatorConfigJSONOptionType',
     'ManiphestTask' => array(

src/applications/maniphest/__tests__/ManiphestTaskTestCase.php

@@ -194,11 +194,14 @@ final class ManiphestTaskTestCase extends PhabricatorTestCase {
       $dst,
       $is_after);
 
+    $keyword_map = ManiphestTaskPriority::getTaskPriorityKeywordsMap();
+    $keyword = head($keyword_map[$pri]);
+
     $xactions = array();
 
     $xactions[] = id(new ManiphestTransaction())
       ->setTransactionType(ManiphestTaskPriorityTransaction::TRANSACTIONTYPE)
-      ->setNewValue($pri);
+      ->setNewValue($keyword);
 
     $xactions[] = id(new ManiphestTransaction())
       ->setTransactionType(ManiphestTaskSubpriorityTransaction::TRANSACTIONTYPE)
@@ -217,11 +220,14 @@ final class ManiphestTaskTestCase extends PhabricatorTestCase {
       $target_priority,
       $is_end);
 
+    $keyword_map = ManiphestTaskPriority::getTaskPriorityKeywordsMap();
+    $keyword = head($keyword_map[$pri]);
+
     $xactions = array();
 
     $xactions[] = id(new ManiphestTransaction())
       ->setTransactionType(ManiphestTaskPriorityTransaction::TRANSACTIONTYPE)
-      ->setNewValue($pri);
+      ->setNewValue($keyword);
 
     $xactions[] = id(new ManiphestTransaction())
       ->setTransactionType(ManiphestTaskSubpriorityTransaction::TRANSACTIONTYPE)

src/applications/maniphest/bulk/ManiphestTaskEditBulkJobType.php

@@ -285,6 +285,11 @@ final class ManiphestTaskEditBulkJobType
                 '=' => array_fuse($value),
               ));
           break;
+        case ManiphestTaskPriorityTransaction::TRANSACTIONTYPE:
+          $keyword_map = ManiphestTaskPriority::getTaskPriorityKeywordsMap();
+          $keyword = head(idx($keyword_map, $value));
+          $xaction->setNewValue($keyword);
+          break;
         default:
           $xaction->setNewValue($value);
           break;

src/applications/maniphest/command/ManiphestPriorityEmailCommand.php

@@ -49,18 +49,8 @@ final class ManiphestPriorityEmailCommand
     array $argv) {
     $xactions = array();
 
-    $target = phutil_utf8_strtolower(head($argv));
-    $priority = null;
-
-    $keywords = ManiphestTaskPriority::getTaskPriorityKeywordsMap();
-    foreach ($keywords as $key => $words) {
-      foreach ($words as $word) {
-        if ($word == $target) {
-          $priority = $key;
-          break;
-        }
-      }
-    }
+    $keyword = phutil_utf8_strtolower(head($argv));
+    $priority = ManiphestTaskPriority::getTaskPriorityFromKeyword($keyword);
 
     if ($priority === null) {
       return array();
@@ -72,7 +62,7 @@ final class ManiphestPriorityEmailCommand
 
     $xactions[] = $object->getApplicationTransactionTemplate()
       ->setTransactionType(ManiphestTaskPriorityTransaction::TRANSACTIONTYPE)
-      ->setNewValue($priority);
+      ->setNewValue($keyword);
 
     return $xactions;
   }

src/applications/maniphest/conduit/ManiphestConduitAPIMethod.php

@@ -99,7 +99,9 @@ abstract class ManiphestConduitAPIMethod extends ConduitAPIMethod {
         throw id(new ConduitException('ERR-INVALID-PARAMETER'))
           ->setErrorDescription(pht('Priority set to invalid value.'));
       }
-      $changes[ManiphestTaskPriorityTransaction::TRANSACTIONTYPE] = $priority;
+      $keyword_map = ManiphestTaskPriority::getTaskPriorityKeywordsMap();
+      $keyword = head(idx($keyword_map, $priority));
+      $changes[ManiphestTaskPriorityTransaction::TRANSACTIONTYPE] = $keyword;
     }
 
     $owner_phid = $request->getValue('ownerPHID');

src/applications/maniphest/conduit/ManiphestQueryStatusesConduitAPIMethod.php

@@ -33,4 +33,14 @@ final class ManiphestQueryStatusesConduitAPIMethod
     return $results;
   }
 
+  public function getMethodStatus() {
+     return self::METHOD_STATUS_FROZEN;
+   }
+
+   public function getMethodStatusDescription() {
+     return pht(
+       'This method is frozen and will eventually be deprecated. New code '.
+       'should use "maniphest.status.search" instead.');
+   }
+
 }

src/applications/maniphest/conduit/ManiphestStatusSearchConduitAPIMethod.php

@@ -0,0 +1,52 @@
+<?php
+
+final class ManiphestStatusSearchConduitAPIMethod
+  extends ManiphestConduitAPIMethod {
+
+  public function getAPIMethodName() {
+    return 'maniphest.status.search';
+  }
+
+  public function getMethodSummary() {
+    return pht('Read information about task statuses.');
+  }
+
+  public function getMethodDescription() {
+    return pht(
+      'Returns information about the possible statuses for Maniphest '.
+      'tasks.');
+  }
+
+  protected function defineParamTypes() {
+    return array();
+  }
+
+  protected function defineReturnType() {
+    return 'map<string, wild>';
+  }
+
+  public function getRequiredScope() {
+    return self::SCOPE_ALWAYS;
+  }
+
+  protected function execute(ConduitAPIRequest $request) {
+    $config = PhabricatorEnv::getEnvConfig('maniphest.statuses');
+    $results = array();
+    foreach ($config as $code => $status) {
+      $stripped_status = array(
+        'name' => $status['name'],
+        'value' => $code,
+        'closed' => !empty($status['closed']),
+      );
+
+      if (isset($status['special'])) {
+        $stripped_status['special'] = $status['special'];
+      }
+
+      $results[] = $stripped_status;
+    }
+
+    return array('data' => $results);
+  }
+
+}

src/applications/maniphest/constants/ManiphestTaskPriority.php

@@ -2,6 +2,8 @@
 
 final class ManiphestTaskPriority extends ManiphestConstants {
 
+  const UNKNOWN_PRIORITY_KEYWORD = '!!unknown!!';
+
   /**
    * Get the priorities and their full descriptions.
    *
@@ -105,6 +107,18 @@ final class ManiphestTaskPriority extends ManiphestConstants {
     return 'fa-arrow-right';
   }
 
+  public static function getTaskPriorityFromKeyword($keyword) {
+    $map = self::getTaskPriorityKeywordsMap();
+
+    foreach ($map as $priority => $keywords) {
+      if (in_array($keyword, $keywords)) {
+        return $priority;
+      }
+    }
+
+    return null;
+  }
+
   public static function isDisabledPriority($priority) {
     $config = idx(self::getConfig(), $priority, array());
     return idx($config, 'disabled', false);
@@ -116,6 +130,18 @@ final class ManiphestTaskPriority extends ManiphestConstants {
     return $config;
   }
 
+  private static function isValidPriorityKeyword($keyword) {
+    if (!strlen($keyword) || strlen($keyword) > 64) {
+      return false;
+    }
+
+    // Alphanumeric, but not exclusively numeric
+    if (!preg_match('/^(?![0-9]*$)[a-zA-Z0-9]+$/', $keyword)) {
+      return false;
+    }
+    return true;
+  }
+
   public static function validateConfiguration($config) {
     if (!is_array($config)) {
       throw new Exception(
@@ -147,9 +173,24 @@ final class ManiphestTaskPriority extends ManiphestConstants {
           'name' => 'string',
           'short' => 'optional string',
           'color' => 'optional string',
-          'keywords' => 'optional list<string>',
+          'keywords' => 'list<string>',
           'disabled' => 'optional bool',
         ));
+
+      $keywords = $value['keywords'];
+      foreach ($keywords as $keyword) {
+        if (!self::isValidPriorityKeyword($keyword)) {
+          throw new Exception(
+            pht(
+              'Key "%s" is not a valid priority keyword. Priority keywords '.
+              'must be 1-64 alphanumeric characters and cannot be '.
+              'exclusively digits. For example, "%s" or "%s" are '.
+              'reasonable choices.',
+              $keyword,
+              'low',
+              'critical'));
+        }
+      }
     }
   }
 

src/applications/maniphest/constants/ManiphestTaskStatus.php

@@ -232,16 +232,17 @@ final class ManiphestTaskStatus extends ManiphestConstants {
    * @task validate
    */
   public static function isValidStatusConstant($constant) {
-    if (strlen($constant) > 12) {
+    if (!strlen($constant) || strlen($constant) > 64) {
       return false;
     }
-    if (!preg_match('/^[a-z0-9]+\z/', $constant)) {
+
+    // Alphanumeric, but not exclusively numeric
+    if (!preg_match('/^(?![0-9]*$)[a-zA-Z0-9]+$/', $constant)) {
       return false;
     }
     return true;
   }
 
-
   /**
    * @task validate
    */
@@ -250,10 +251,9 @@ final class ManiphestTaskStatus extends ManiphestConstants {
       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 "%s" is not a valid status constant. Status constants '.
+            'must be 1-64 alphanumeric characters and cannot be exclusively '.
+            'digits. For example, "%s" or "%s" are reasonable choices.',
             $key,
             'open',
             'closed'));

src/applications/maniphest/constants/__tests__/ManiphestTaskStatusTestCase.php

@@ -10,10 +10,15 @@ final class ManiphestTaskStatusTestCase extends PhabricatorTestCase {
       'duplicate2' => true,
 
       '' => false,
-      'longlonglonglong' => false,
+      'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa' =>
+         false,
       '.' => false,
-      'ABCD' => false,
+      ' ' => false,
+      'ABCD' => true,
       'a b c ' => false,
+      '1' => false,
+      '111' => false,
+      '11a' => true,
     );
 
     foreach ($map as $input => $expect) {

src/applications/maniphest/controller/ManiphestSubpriorityController.php

@@ -40,11 +40,14 @@ final class ManiphestSubpriorityController extends ManiphestController {
         $is_end = false);
     }
 
+    $keyword_map = ManiphestTaskPriority::getTaskPriorityKeywordsMap();
+    $keyword = head(idx($keyword_map, $pri));
+
     $xactions = array();
 
     $xactions[] = id(new ManiphestTransaction())
       ->setTransactionType(ManiphestTaskPriorityTransaction::TRANSACTIONTYPE)
-      ->setNewValue($pri);
+      ->setNewValue($keyword);
 
     $xactions[] = id(new ManiphestTransaction())
       ->setTransactionType(ManiphestTaskSubpriorityTransaction::TRANSACTIONTYPE)

src/applications/maniphest/editor/ManiphestEditEngine.php

@@ -215,7 +215,7 @@ EODOCS
         ->setConduitTypeDescription(pht('New task priority constant.'))
         ->setTransactionType(ManiphestTaskPriorityTransaction::TRANSACTIONTYPE)
         ->setIsCopyable(true)
-        ->setValue($object->getPriority())
+        ->setValue($object->getPriorityKeyword())
         ->setOptions($priority_map)
         ->setCommentActionLabel(pht('Change Priority')),
     );
@@ -289,29 +289,29 @@ EODOCS
 
   private function getTaskPriorityMap(ManiphestTask $task) {
     $priority_map = ManiphestTaskPriority::getTaskPriorityMap();
+    $priority_keywords = ManiphestTaskPriority::getTaskPriorityKeywordsMap();
     $current_priority = $task->getPriority();
+    $results = array();
+
+    foreach ($priority_map as $priority => $priority_name) {
+      $disabled = ManiphestTaskPriority::isDisabledPriority($priority);
+      if ($disabled && !($priority == $current_priority)) {
+        continue;
+      }
+
+      $keyword = head(idx($priority_keywords, $priority));
+      $results[$keyword] = $priority_name;
+    }
 
     // If the current value isn't a legitimate one, put it in the dropdown
-    // anyway so saving the form doesn't cause a side effects.
+    // anyway so saving the form doesn't cause any side effects.
     if (idx($priority_map, $current_priority) === null) {
-      $priority_map[$current_priority] = pht(
+      $results[ManiphestTaskPriority::UNKNOWN_PRIORITY_KEYWORD] = pht(
         '<Unknown: %s>',
         $current_priority);
     }
 
-    foreach ($priority_map as $priority => $priority_name) {
-      // Always keep the current priority.
-      if ($priority == $current_priority) {
-        continue;
-      }
-
-      if (ManiphestTaskPriority::isDisabledPriority($priority)) {
-        unset($priority_map[$priority]);
-        continue;
-      }
-    }
-
-    return $priority_map;
+    return $results;
   }
 
   protected function newEditResponse(

src/applications/maniphest/herald/ManiphestTaskPriorityHeraldAction.php

@@ -39,12 +39,15 @@ final class ManiphestTaskPriorityHeraldAction
       return;
     }
 
+    $keyword_map = ManiphestTaskPriority::getTaskPriorityKeywordsMap();
+    $keyword = head(idx($keyword_map, $priority));
+
     $xaction = $adapter->newTransaction()
       ->setTransactionType(ManiphestTaskPriorityTransaction::TRANSACTIONTYPE)
-      ->setNewValue($priority);
+      ->setNewValue($keyword);
 
     $adapter->queueTransaction($xaction);
-    $this->logEffect(self::DO_PRIORITY, $priority);
+    $this->logEffect(self::DO_PRIORITY, $keyword);
   }
 
   public function getHeraldActionStandardType() {

src/applications/maniphest/lipsum/PhabricatorManiphestTaskTestDataGenerator.php

@@ -100,7 +100,10 @@ final class PhabricatorManiphestTaskTestDataGenerator
   }
 
   public function generateTaskPriority() {
-    return array_rand(ManiphestTaskPriority::getTaskPriorityMap());
+    $pri = array_rand(ManiphestTaskPriority::getTaskPriorityMap());
+    $keyword_map = ManiphestTaskPriority::getTaskPriorityKeywordsMap();
+    $keyword = head(idx($keyword_map, $pri));
+    return $keyword;
   }
 
   public function generateTaskSubPriority() {

src/applications/maniphest/storage/ManiphestTask.php

@@ -79,7 +79,7 @@ final class ManiphestTask extends ManiphestDAO
       ),
       self::CONFIG_COLUMN_SCHEMA => array(
         'ownerPHID' => 'phid?',
-        'status' => 'text12',
+        'status' => 'text64',
         'priority' => 'uint32',
         'title' => 'sort',
         'originalTitle' => 'text',
@@ -245,6 +245,14 @@ final class ManiphestTask extends ManiphestDAO
     );
   }
 
+  public function getPriorityKeyword() {
+    $priority = $this->getPriority();
+    $map = ManiphestTaskPriority::getTaskPriorityKeywordsMap();
+    $default = array(ManiphestTaskPriority::UNKNOWN_PRIORITY_KEYWORD);
+    $keywords = idx($map, $priority, $default);
+    return head($keywords);
+  }
+
   private function comparePriorityTo(ManiphestTask $other) {
     $upri = $this->getPriority();
     $vpri = $other->getPriority();

src/applications/maniphest/xaction/ManiphestTaskPriorityTransaction.php

@@ -12,6 +12,19 @@ final class ManiphestTaskPriorityTransaction
     return $object->getPriority();
   }
 
+  public function generateNewValue($object, $value) {
+    // `$value` is supposed to be a keyword, but if the priority
+    // assigned to a task has been removed from the config,
+    // no such keyword will be available. Other edits to the task
+    // should still be allowed, even if the priority is  no longer
+    // valid, so treat this as a no-op.
+    if ($value === ManiphestTaskPriority::UNKNOWN_PRIORITY_KEYWORD) {
+      return $object->getPriority();
+    }
+
+    return (string)ManiphestTaskPriority::getTaskPriorityFromKeyword($value);
+  }
+
   public function applyInternalEffects($object, $value) {
     $object->setPriority($value);
   }

src/applications/project/controller/PhabricatorProjectMoveController.php

@@ -153,11 +153,14 @@ final class PhabricatorProjectMoveController
       break;
     }
 
+    $keyword_map = ManiphestTaskPriority::getTaskPriorityKeywordsMap();
+    $keyword = head(idx($keyword_map, $pri));
+
     $xactions = array();
     if ($pri !== null) {
       $xactions[] = id(new ManiphestTransaction())
         ->setTransactionType(ManiphestTaskPriorityTransaction::TRANSACTIONTYPE)
-        ->setNewValue($pri);
+        ->setNewValue($keyword);
       $xactions[] = id(new ManiphestTransaction())
         ->setTransactionType(
           ManiphestTaskSubpriorityTransaction::TRANSACTIONTYPE)