From cb957f8d62424ace76a06331d9859d92dfb9f5db Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 4 Jan 2018 09:34:40 -0800 Subject: [PATCH] Pile more atrocities onto the Maniphest burnup report Summary: See PHI273. Ref T13020. After D18777, tasks created directly into the default status (which is common) via the web UI no longer write a "status" transaction. This is consistent with other applications, and consistent with the API/email behavior for tasks since early 2016. It also improves the consistency of //reading// tasks via the API. However, it impacted the "Burnup Report" which relies on directly reading these rows to detect task creation. Until this is fixed properly (T1562), synthetically generate the "missing" transactions which this page expects by looking at task creation dates instead. Specifically, we: - Generate a fake `status: null -> "open"` transaction for every task by looking at the Task table. - Go through the transaction list and remove all the legacy `status: null -> "any open status"` transactions. These will only exist for older tasks. - Merge all our new fake transactions into the list of transactions. - Continue on as though nothing happened, letting the rendering code continue to operate on legacy-looking data. I think this will slightly miscount tasks which were created directly into a closed status, but this is very rare, and does not significantly impact the accuracy of this report relative to other known issues (notably, merging closed tasks). This will also get the wrong result if the default status has changed from an "open" status to a "closed" status at any point, but this is exceptionally bizarre/rare. Ultimately, T1562 will let us delete all this stuff and disavow its existence. Test Plan: - Created some tasks, loaded burnup before/after this patch. - My local chart looks more accurate afterwards, but the data is super weird (I used `bin/lipsum` to create a huge number of tasks a couple months ago). I'll vet this on `secure`, which has more reasonable data. Here's my local chart: {F5356499} That's what it //should// look like, it's just hard to be confident that nothing else is hiding there. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13020 Differential Revision: https://secure.phabricator.com/D18853 --- .../controller/ManiphestReportController.php | 57 ++++++++++++++++++- 1 file changed, 55 insertions(+), 2 deletions(-) diff --git a/src/applications/maniphest/controller/ManiphestReportController.php b/src/applications/maniphest/controller/ManiphestReportController.php index 66aa154e8f..821c96a2aa 100644 --- a/src/applications/maniphest/controller/ManiphestReportController.php +++ b/src/applications/maniphest/controller/ManiphestReportController.php @@ -99,13 +99,66 @@ final class ManiphestReportController extends ManiphestController { ManiphestTaskMergedIntoTransaction::TRANSACTIONTYPE, )); + // See PHI273. After the move to EditEngine, we no longer create a + // "status" transaction if a task is created directly into the default + // status. This likely impacted API/email tasks after 2016 and all other + // tasks after late 2017. Until Facts can fix this properly, use the + // task creation dates to generate synthetic transactions which look like + // the older transactions that this page expects. + + $default_status = ManiphestTaskStatus::getDefaultStatus(); + $duplicate_status = ManiphestTaskStatus::getDuplicateStatus(); + + // Build synthetic transactions which take status from `null` to the + // default value. + $create_rows = queryfx_all( + $conn, + 'SELECT dateCreated FROM %T', + id(new ManiphestTask())->getTableName()); + foreach ($create_rows as $key => $create_row) { + $create_rows[$key] = array( + 'transactionType' => 'status', + 'oldValue' => null, + 'newValue' => $default_status, + 'dateCreated' => $create_row['dateCreated'], + ); + } + + // Remove any actual legacy status transactions which take status from + // `null` to any open status. + foreach ($data as $key => $row) { + if ($row['transactionType'] != 'status') { + continue; + } + + $oldv = trim($row['oldValue'], '"'); + $newv = trim($row['oldValue'], '"'); + + // If this is a status change, preserve it. + if ($oldv != 'null') { + continue; + } + + // If this task was created directly into a closed status, preserve + // the transaction. + if (!ManiphestTaskStatus::isOpenStatus($newv)) { + continue; + } + + // If this is a legacy "create" transaction, discard it in favor of the + // synthetic one. + unset($data[$key]); + } + + // Merge the synthetic rows into the real transactions. + $data = array_merge($create_rows, $data); + $data = array_values($data); + $stats = array(); $day_buckets = array(); $open_tasks = array(); - $default_status = ManiphestTaskStatus::getDefaultStatus(); - $duplicate_status = ManiphestTaskStatus::getDuplicateStatus(); foreach ($data as $key => $row) { switch ($row['transactionType']) { case ManiphestTaskStatusTransaction::TRANSACTIONTYPE: