From 939008e64c2661aa82d42514e043a4a3e8c4fe99 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 14 Sep 2017 04:23:53 -0700
Subject: [PATCH] Correct an issue where Maniphest's awful legacy "reports" UI
 was extra broken on merges

Summary:
See PHI66. See that issue for context. This UI is bad broken legacy junk, but was especially broken when reporting merges.

These do not currently generate a "status" transaction, so they were never counted as task closures. Pretend they're normal closures.

This is still wrong, but should be much closer to the real numbers. Specifically, if you merge a closed task into another task, it will incorrectly be counted as an extra close. This could result in negative tasks, but the numbers should be much closer to reality than they are today even so.

The "Facts" application (T1562) is the real pathway forward here in the longer term.

Test Plan:
  - Moved my `maniphest_transactions` table aside with `RENAME TABLE ...`.
  - Created a new empty table with `CREATE TABLE ... LIKE ...`.
  - Reloaded reports UI, saw empty chart.
  - Created, closed, and reopened tasks while reloading the chart, saw accurate reporting.
  - Merged an open task into another task, saw bad reporting.
  - Applied patch, saw the right chart again.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18601
---
 .../controller/ManiphestReportController.php  | 31 ++++++++++++++-----
 1 file changed, 24 insertions(+), 7 deletions(-)

diff --git a/src/applications/maniphest/controller/ManiphestReportController.php b/src/applications/maniphest/controller/ManiphestReportController.php
index 3cc420a4c6..6a5b3ad50c 100644
--- a/src/applications/maniphest/controller/ManiphestReportController.php
+++ b/src/applications/maniphest/controller/ManiphestReportController.php
@@ -88,23 +88,40 @@ final class ManiphestReportController extends ManiphestController {
 
     $data = queryfx_all(
       $conn,
-      'SELECT x.oldValue, x.newValue, x.dateCreated FROM %T x %Q
-        WHERE transactionType = %s
+      'SELECT x.transactionType, x.oldValue, x.newValue, x.dateCreated
+        FROM %T x %Q
+        WHERE transactionType IN (%Ls)
         ORDER BY x.dateCreated ASC',
       $table->getTableName(),
       $joins,
-      ManiphestTaskStatusTransaction::TRANSACTIONTYPE);
+      array(
+        ManiphestTaskStatusTransaction::TRANSACTIONTYPE,
+        ManiphestTaskMergedIntoTransaction::TRANSACTIONTYPE,
+      ));
 
     $stats = array();
     $day_buckets = array();
 
     $open_tasks = array();
 
+    $default_status = ManiphestTaskStatus::getDefaultStatus();
+    $duplicate_status = ManiphestTaskStatus::getDuplicateStatus();
     foreach ($data as $key => $row) {
-
-      // NOTE: Hack to avoid json_decode().
-      $oldv = trim($row['oldValue'], '"');
-      $newv = trim($row['newValue'], '"');
+      switch ($row['transactionType']) {
+        case ManiphestTaskStatusTransaction::TRANSACTIONTYPE:
+          // NOTE: Hack to avoid json_decode().
+          $oldv = trim($row['oldValue'], '"');
+          $newv = trim($row['newValue'], '"');
+          break;
+        case ManiphestTaskMergedIntoTransaction::TRANSACTIONTYPE:
+          // NOTE: Merging a task does not generate a "status" transaction.
+          // We pretend it did. Note that this is not always accurate: it is
+          // possble to merge a task which was previously closed, but this
+          // fake transaction always counts a merge as a closure.
+          $oldv = $default_status;
+          $newv = $duplicate_status;
+          break;
+      }
 
       if ($oldv == 'null') {
         $old_is_open = false;