Clean up Maniphest transaction rendering a bit more

Summary:
Ref T2217. This partially retreads the ground from D7115.

  - We're rendering silly transactions about descriptions when creating tasks. Hide those.
  - Move the "created" transaction back to status. This fixes two things that are otherwise more of a mess than I'd anticipated:
    - It fixes Reports without making a mess (see <https://github.com/facebook/phabricator/issues/395>).
    - It renders old transactions properly (i.e., "created" instead of "reopened" for tasks older than the migration).
  - Be explicit about action strength, so emails always say the most important thing in the subject.

Test Plan: Created and edited tasks, looked at resulting transactions, saw a cleaner transaction record.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2217

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

src/applications/maniphest/editor/ManiphestTransactionEditorPro.php

@@ -30,10 +30,19 @@ final class ManiphestTransactionEditorPro
       case ManiphestTransaction::TYPE_PRIORITY:
         return (int)$object->getPriority();
       case ManiphestTransaction::TYPE_STATUS:
+        if ($this->getIsNewObject()) {
+          return null;
+        }
         return (int)$object->getStatus();
       case ManiphestTransaction::TYPE_TITLE:
+        if ($this->getIsNewObject()) {
+          return null;
+        }
         return $object->getTitle();
       case ManiphestTransaction::TYPE_DESCRIPTION:
+        if ($this->getIsNewObject()) {
+          return null;
+        }
         return $object->getDescription();
       case ManiphestTransaction::TYPE_OWNER:
         return nonempty($object->getOwnerPHID(), null);

src/applications/maniphest/storage/ManiphestTransaction.php

@@ -74,17 +74,40 @@ final class ManiphestTransaction
     return $phids;
   }
 
+  public function shouldHide() {
+    switch ($this->getTransactionType()) {
+      case self::TYPE_TITLE:
+      case self::TYPE_DESCRIPTION:
+        if ($this->getOldValue() === null) {
+          return true;
+        } else {
+          return false;
+        }
+        break;
+    }
+
+    return false;
+  }
+
+  public function getActionStrength() {
+    switch ($this->getTransactionType()) {
+      case self::TYPE_STATUS:
+        return 1.3;
+      case self::TYPE_OWNER:
+        return 1.2;
+      case self::TYPE_PRIORITY:
+        return 1.1;
+    }
+
+    return parent::getActionStrength();
+  }
+
 
   public function getColor() {
     $old = $this->getOldValue();
     $new = $this->getNewValue();
 
     switch ($this->getTransactionType()) {
-      case self::TYPE_TITLE:
-        if (!strlen($old)) {
-          return 'green';
-        }
-
       case self::TYPE_STATUS:
         if ($new == ManiphestTaskStatus::STATUS_OPEN) {
           return 'green';
@@ -112,16 +135,16 @@ final class ManiphestTransaction
 
     switch ($this->getTransactionType()) {
       case self::TYPE_TITLE:
-        if (!strlen($old)) {
+        return pht('Retitled');
-          return pht('Created');
-        } else {
-          return pht('Retitled');
-        }
 
       case self::TYPE_STATUS:
         switch ($new) {
           case ManiphestTaskStatus::STATUS_OPEN:
-            return pht('Reopened');
+            if ($old === null) {
+              return pht('Created');
+            } else {
+              return pht('Reopened');
+            }
           case ManiphestTaskStatus::STATUS_CLOSED_SPITE:
             return pht('Spited');
           case ManiphestTaskStatus::STATUS_CLOSED_DUPLICATE:
@@ -173,11 +196,7 @@ final class ManiphestTransaction
 
     switch ($this->getTransactionType()) {
       case self::TYPE_TITLE:
-        if (!strlen($old)) {
+        return 'edit';
-          return 'create';
-        } else {
-          return 'edit';
-        }
 
       case self::TYPE_STATUS:
         switch ($new) {
@@ -226,19 +245,11 @@ final class ManiphestTransaction
 
     switch ($this->getTransactionType()) {
       case self::TYPE_TITLE:
-        if (!strlen($old)) {
+        return pht(
-          return pht(
+          '%s changed the title from "%s" to "%s".',
-            '%s created this task.',
+          $this->renderHandleLink($author_phid),
-            $this->renderHandleLink($author_phid),
+          $old,
-            $old,
+          $new);
-            $new);
-        } else {
-          return pht(
-            '%s changed the title from "%s" to "%s".',
-            $this->renderHandleLink($author_phid),
-            $old,
-            $new);
-        }
 
       case self::TYPE_DESCRIPTION:
         return pht(
@@ -248,9 +259,15 @@ final class ManiphestTransaction
       case self::TYPE_STATUS:
         switch ($new) {
           case ManiphestTaskStatus::STATUS_OPEN:
-            return pht(
+            if ($old === null) {
-              '%s reopened this task.',
+              return pht(
-              $this->renderHandleLink($author_phid));
+                '%s created this task.',
+                $this->renderHandleLink($author_phid));
+            } else {
+              return pht(
+                '%s reopened this task.',
+                $this->renderHandleLink($author_phid));
+            }
 
           case ManiphestTaskStatus::STATUS_CLOSED_SPITE:
             return pht(
@@ -405,19 +422,12 @@ final class ManiphestTransaction
 
     switch ($this->getTransactionType()) {
       case self::TYPE_TITLE:
-        if (!strlen($old)) {
+        return pht(
-          return pht(
+          '%s renamed %s from "%s" to "%s".',
-            '%s created %s.',
+          $this->renderHandleLink($author_phid),
-            $this->renderHandleLink($author_phid),
+          $this->renderHandleLink($object_phid),
-            $this->renderHandleLink($object_phid));
+          $old,
-        } else {
+          $new);
-          return pht(
-            '%s renamed %s from "%s" to "%s".',
-            $this->renderHandleLink($author_phid),
-            $this->renderHandleLink($object_phid),
-            $old,
-            $new);
-        }
 
       case self::TYPE_DESCRIPTION:
         return pht(
@@ -426,34 +436,40 @@ final class ManiphestTransaction
           $this->renderHandleLink($object_phid));
 
       case self::TYPE_STATUS:
-        if ($new == ManiphestTaskStatus::STATUS_OPEN) {
+        switch ($new) {
-          return pht(
+          case ManiphestTaskStatus::STATUS_OPEN:
-            '%s reopened %s.',
+            if ($old === null) {
-            $this->renderHandleLink($author_phid),
-            $this->renderHandleLink($object_phid));
-        } else {
-          switch ($new) {
-            case ManiphestTaskStatus::STATUS_CLOSED_SPITE:
               return pht(
-                '%s closed %s out of spite.',
+                '%s created %s.',
                 $this->renderHandleLink($author_phid),
                 $this->renderHandleLink($object_phid));
-            case ManiphestTaskStatus::STATUS_CLOSED_DUPLICATE:
+            } else {
               return pht(
-                '%s closed %s as a duplicate.',
+                '%s reopened %s.',
                 $this->renderHandleLink($author_phid),
                 $this->renderHandleLink($object_phid));
-            default:
+            }
-              $status_name = idx(
+
-                ManiphestTaskStatus::getTaskStatusMap(),
+          case ManiphestTaskStatus::STATUS_CLOSED_SPITE:
-                $new,
+            return pht(
-                '???');
+              '%s closed %s out of spite.',
-              return pht(
+              $this->renderHandleLink($author_phid),
-                '%s closed %s as "%s".',
+              $this->renderHandleLink($object_phid));
-                $this->renderHandleLink($author_phid),
+          case ManiphestTaskStatus::STATUS_CLOSED_DUPLICATE:
-                $this->renderHandleLink($object_phid),
+            return pht(
-                $status_name);
+              '%s closed %s as a duplicate.',
-          }
+              $this->renderHandleLink($author_phid),
+              $this->renderHandleLink($object_phid));
+          default:
+            $status_name = idx(
+              ManiphestTaskStatus::getTaskStatusMap(),
+              $new,
+              '???');
+            return pht(
+              '%s closed %s as "%s".',
+              $this->renderHandleLink($author_phid),
+              $this->renderHandleLink($object_phid),
+              $status_name);
         }
 
       case self::TYPE_OWNER: