From d321cc810aab52be00d75c9dcfa8b9cabd34828e Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 22 Nov 2017 06:09:41 -0800
Subject: [PATCH] Freeze "maniphest.gettasktransactions" and make
 status/priority transactions more consistent

Summary:
Ref T13020. See PHI221.

Freeze legacy method `maniphest.gettasktransactions` in favor of modern method `transaction.search`.

Remove legacy "null on create" behavior from Maniphest status and priority transactions. This behavior is obsolete with EditEngine, and leads to inconsistent transaction sets in the transaction record.

The desired behavior is that transactions which don't do anything (e.g., default value was not changed) don't appear in the transaction log.

Test Plan:
  - Viewed API UI and saw `maniphest.gettasktransactions` marked as "Frozen".
  - Created a new task via web UI (without changing status/priority), queried transactions with `maniphest.gettasktransacitons`/`transaction.search`, no longer saw "null on create" no-op transactions in record.
    - Web UI is unchanged, since these transactions were hidden before and now do not exist.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13020

Differential Revision: https://secure.phabricator.com/D18777
---
 .../ManiphestGetTaskTransactionsConduitAPIMethod.php   | 10 ++++++++++
 .../xaction/ManiphestTaskPriorityTransaction.php       |  9 +++------
 .../xaction/ManiphestTaskStatusTransaction.php         |  3 ---
 3 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/src/applications/maniphest/conduit/ManiphestGetTaskTransactionsConduitAPIMethod.php b/src/applications/maniphest/conduit/ManiphestGetTaskTransactionsConduitAPIMethod.php
index 8b0d0496cf..357d118bc4 100644
--- a/src/applications/maniphest/conduit/ManiphestGetTaskTransactionsConduitAPIMethod.php
+++ b/src/applications/maniphest/conduit/ManiphestGetTaskTransactionsConduitAPIMethod.php
@@ -21,6 +21,16 @@ final class ManiphestGetTaskTransactionsConduitAPIMethod
     return 'nonempty list<dict<string, wild>>';
   }
 
+  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 "transaction.search" instead.');
+  }
+
   protected function execute(ConduitAPIRequest $request) {
     $results = array();
     $task_ids = $request->getValue('ids');
diff --git a/src/applications/maniphest/xaction/ManiphestTaskPriorityTransaction.php b/src/applications/maniphest/xaction/ManiphestTaskPriorityTransaction.php
index 2ed7c4e15b..83f38fc659 100644
--- a/src/applications/maniphest/xaction/ManiphestTaskPriorityTransaction.php
+++ b/src/applications/maniphest/xaction/ManiphestTaskPriorityTransaction.php
@@ -6,20 +6,17 @@ final class ManiphestTaskPriorityTransaction
   const TRANSACTIONTYPE = 'priority';
 
   public function generateOldValue($object) {
-    if ($this->isNewObject()) {
-      return null;
-    }
-    return $object->getPriority();
+    return (string)$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
+    // 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)$object->getPriority();
     }
 
     return (string)ManiphestTaskPriority::getTaskPriorityFromKeyword($value);
diff --git a/src/applications/maniphest/xaction/ManiphestTaskStatusTransaction.php b/src/applications/maniphest/xaction/ManiphestTaskStatusTransaction.php
index a3780e81b9..dd51a63799 100644
--- a/src/applications/maniphest/xaction/ManiphestTaskStatusTransaction.php
+++ b/src/applications/maniphest/xaction/ManiphestTaskStatusTransaction.php
@@ -6,9 +6,6 @@ final class ManiphestTaskStatusTransaction
   const TRANSACTIONTYPE = 'status';
 
   public function generateOldValue($object) {
-    if ($this->isNewObject()) {
-      return null;
-    }
     return $object->getStatus();
   }