Use string constants, not integer constants, to represent task status internally

Summary:
Ref T1812. I think integer constants are going to be confusing and error prone for users to interact with. For example, because we use 0-5, adding a second "open" status like "needs verification" without disrupting the existing statuses would require users to define a status with, e.g., constant `6`, but order it between constants `0` and `1`. And if they later remove statuses, they need to avoid reusing existing constants.

Instead, use more manageable string constants like "open", "resolved", etc.

We must migrate three tables:

  - The task table itself, to update task status.
  - The transaction table, to update historic status changes.
  - The saved query table, to update saved queries which specify status sets.

Test Plan:
  - Saved a query with complicated status filters.
  - Ran migrations.
  - Looked at the query, at existing tasks, and at task transactions.
  - Forced migrations to run again to verify idempotentcy/safety.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T1812

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

resources/sql/autopatches/20140321.mstatus.1.col.sql

@@ -0,0 +1,2 @@
+ALTER TABLE {$NAMESPACE}_maniphest.maniphest_task
+  CHANGE status status VARCHAR(12) NOT NULL COLLATE latin1_bin;

resources/sql/autopatches/20140321.mstatus.2.mig.php

@@ -0,0 +1,94 @@
+<?php
+
+$status_map = array(
+  0 => 'open',
+  1 => 'resolved',
+  2 => 'wontfix',
+  3 => 'invalid',
+  4 => 'duplicate',
+  5 => 'spite',
+);
+
+$conn_w = id(new ManiphestTask())->establishConnection('w');
+
+echo "Migrating tasks to new status constants...\n";
+foreach (new LiskMigrationIterator(new ManiphestTask()) as $task) {
+  $id = $task->getID();
+  echo "Migrating T{$id}...\n";
+
+  $status = $task->getStatus();
+  if (isset($status_map[$status])) {
+    queryfx(
+      $conn_w,
+      'UPDATE %T SET status = %s WHERE id = %d',
+      $task->getTableName(),
+      $status_map[$status],
+      $id);
+  }
+}
+
+echo "Done.\n";
+
+
+echo "Migrating task transactions to new status constants...\n";
+foreach (new LiskMigrationIterator(new ManiphestTransaction()) as $xaction) {
+  $id = $xaction->getID();
+  echo "Migrating {$id}...\n";
+
+  if ($xaction->getTransactionType() == ManiphestTransaction::TYPE_STATUS) {
+    $old = $xaction->getOldValue();
+    if ($old !== null && isset($status_map[$old])) {
+      $old = $status_map[$old];
+    }
+
+    $new = $xaction->getNewValue();
+    if (isset($status_map[$new])) {
+      $new = $status_map[$new];
+    }
+
+    queryfx(
+      $conn_w,
+      'UPDATE %T SET oldValue = %s, newValue = %s WHERE id = %d',
+      $xaction->getTableName(),
+      json_encode($old),
+      json_encode($new),
+      $id);
+  }
+}
+echo "Done.\n";
+
+$conn_w = id(new PhabricatorSavedQuery())->establishConnection('w');
+
+echo "Migrating searches to new status constants...\n";
+foreach (new LiskMigrationIterator(new PhabricatorSavedQuery()) as $query) {
+  $id = $query->getID();
+  echo "Migrating {$id}...\n";
+
+  if ($query->getEngineClassName() !== 'ManiphestTaskSearchEngine') {
+    continue;
+  }
+
+  $params = $query->getParameters();
+  $statuses = idx($params, 'statuses', array());
+  if ($statuses) {
+    $changed = false;
+    foreach ($statuses as $key => $status) {
+      if (isset($status_map[$status])) {
+        $statuses[$key] = $status_map[$status];
+        $changed = true;
+      }
+    }
+
+    if ($changed) {
+      $params['statuses'] = $statuses;
+
+      queryfx(
+        $conn_w,
+        'UPDATE %T SET parameters = %s WHERE id = %d',
+        $query->getTableName(),
+        json_encode($params),
+        $id);
+    }
+  }
+}
+echo "Done.\n";

src/applications/maniphest/constants/ManiphestTaskStatus.php

@@ -2,12 +2,12 @@
 
 final class ManiphestTaskStatus extends ManiphestConstants {
 
-  const STATUS_OPEN               = 0;
-  const STATUS_CLOSED_RESOLVED    = 1;
-  const STATUS_CLOSED_WONTFIX     = 2;
-  const STATUS_CLOSED_INVALID     = 3;
-  const STATUS_CLOSED_DUPLICATE   = 4;
-  const STATUS_CLOSED_SPITE       = 5;
+  const STATUS_OPEN               = 'open';
+  const STATUS_CLOSED_RESOLVED    = 'resolved';
+  const STATUS_CLOSED_WONTFIX     = 'wontfix';
+  const STATUS_CLOSED_INVALID     = 'invalid';
+  const STATUS_CLOSED_DUPLICATE   = 'duplicate';
+  const STATUS_CLOSED_SPITE       = 'spite';
 
   const SPECIAL_DEFAULT     = 'default';
   const SPECIAL_CLOSED      = 'closed';

src/applications/maniphest/controller/ManiphestReportController.php

@@ -660,7 +660,6 @@ final class ManiphestReportController extends ManiphestController {
 
     $open_status_list = array();
     foreach (ManiphestTaskStatus::getOpenStatusConstants() as $constant) {
-      $open_status_list[] = json_encode((int)$constant);
       $open_status_list[] = json_encode((string)$constant);
     }
 

src/applications/maniphest/editor/ManiphestTransactionEditor.php

@@ -40,7 +40,7 @@ final class ManiphestTransactionEditor
         if ($this->getIsNewObject()) {
           return null;
         }
-        return (int)$object->getStatus();
+        return $object->getStatus();
       case ManiphestTransaction::TYPE_TITLE:
         if ($this->getIsNewObject()) {
           return null;
@@ -75,13 +75,13 @@ final class ManiphestTransactionEditor
 
     switch ($xaction->getTransactionType()) {
       case ManiphestTransaction::TYPE_PRIORITY:
-      case ManiphestTransaction::TYPE_STATUS:
         return (int)$xaction->getNewValue();
       case ManiphestTransaction::TYPE_CCS:
       case ManiphestTransaction::TYPE_PROJECTS:
         return array_values(array_unique($xaction->getNewValue()));
       case ManiphestTransaction::TYPE_OWNER:
         return nonempty($xaction->getNewValue(), null);
+      case ManiphestTransaction::TYPE_STATUS:
       case ManiphestTransaction::TYPE_TITLE:
       case ManiphestTransaction::TYPE_DESCRIPTION:
       case ManiphestTransaction::TYPE_ATTACH:

src/applications/maniphest/query/ManiphestTaskQuery.php

@@ -351,12 +351,12 @@ final class ManiphestTaskQuery
       case self::STATUS_OPEN:
         return qsprintf(
           $conn,
-          'status IN (%Ld)',
+          'status IN (%Ls)',
           ManiphestTaskStatus::getOpenStatusConstants());
       case self::STATUS_CLOSED:
         return qsprintf(
           $conn,
-          'status IN (%Ld)',
+          'status IN (%Ls)',
           ManiphestTaskStatus::getClosedStatusConstants());
       default:
         $constant = idx($map, $this->status);
@@ -365,7 +365,7 @@ final class ManiphestTaskQuery
         }
         return qsprintf(
           $conn,
-          'status = %d',
+          'status = %s',
           $constant);
     }
   }
@@ -374,7 +374,7 @@ final class ManiphestTaskQuery
     if ($this->statuses) {
       return qsprintf(
         $conn,
-        'status IN (%Ld)',
+        'status IN (%Ls)',
         $this->statuses);
     }
     return null;
@@ -826,8 +826,8 @@ final class ManiphestTaskQuery
       case self::GROUP_STATUS:
         $columns[] = array(
           'name' => 'task.status',
-          'value' => (int)$group_id,
-          'type' => 'int',
+          'value' => $group_id,
+          'type' => 'string',
         );
         break;
       case self::GROUP_PROJECT: