@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.) hq.recaptime.dev/wiki/Phorge
phorge phabricator

Track closed date and closing user for tasks explicitly

Summary:
Ref T4434. Although some of the use cases for this data are better fits for Facts, this data is reasonable to track separately.

I have an approximate view of it already ("closed, ordered by date modified") that's useful to review things that were fixed recently. This lets us make that view more effective.

This just adds (and populates) the storage. Followups will add Conduit, Export, Search, and UI support.

This is slightly tricky because merges work oddly (see T13020).

Test Plan:
- Ran migration, checked database for sensible results.
- Created a task in open/closed status, got the right database values.
- Modified a task to close/open it, got the right values.
- Merged an open task, got updates.

Maniphest Tasks: T4434

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

+106 -2
+5
resources/sql/autopatches/20180208.maniphest.01.close.sql
··· 1 + ALTER TABLE {$NAMESPACE}_maniphest.maniphest_task 2 + ADD closedEpoch INT UNSIGNED; 3 + 4 + ALTER TABLE {$NAMESPACE}_maniphest.maniphest_task 5 + ADD closerPHID VARBINARY(64);
+65
resources/sql/autopatches/20180208.maniphest.02.populate.php
··· 1 + <?php 2 + 3 + $table = new ManiphestTask(); 4 + $conn = $table->establishConnection('w'); 5 + $viewer = PhabricatorUser::getOmnipotentUser(); 6 + 7 + foreach (new LiskMigrationIterator($table) as $task) { 8 + if ($task->getClosedEpoch()) { 9 + // Task already has a closed date. 10 + continue; 11 + } 12 + 13 + $status = $task->getStatus(); 14 + if (!ManiphestTaskStatus::isClosedStatus($status)) { 15 + // Task isn't closed. 16 + continue; 17 + } 18 + 19 + // Look through the transactions from newest to oldest until we find one 20 + // where the task was closed. A merge also counts as a close, even though 21 + // it doesn't currently produce a separate transaction. 22 + 23 + $type_merge = ManiphestTaskStatusTransaction::TRANSACTIONTYPE; 24 + $type_status = ManiphestTaskMergedIntoTransaction::TRANSACTIONTYPE; 25 + 26 + $xactions = id(new ManiphestTransactionQuery()) 27 + ->setViewer($viewer) 28 + ->withObjectPHIDs(array($task->getPHID())) 29 + ->withTransactionTypes( 30 + array( 31 + $type_merge, 32 + $type_status, 33 + )) 34 + ->execute(); 35 + foreach ($xactions as $xaction) { 36 + $old = $xaction->getOldValue(); 37 + $new = $xaction->getNewValue(); 38 + 39 + $type = $xaction->getTransactionType(); 40 + 41 + // If this is a status change, but is not a close, don't use it. 42 + // (We always use merges, even though it's possible to merge a task which 43 + // was previously closed: we can't tell when this happens very easily.) 44 + if ($type === $type_status) { 45 + if (!ManiphestTaskStatus::isClosedStatus($new)) { 46 + continue; 47 + } 48 + 49 + if ($old && ManiphestTaskStatus::isClosedStatus($old)) { 50 + continue; 51 + } 52 + } 53 + 54 + queryfx( 55 + $conn, 56 + 'UPDATE %T SET closedEpoch = %d, closerPHID = %ns 57 + WHERE id = %d', 58 + $table->getTableName(), 59 + $xaction->getDateCreated(), 60 + $xaction->getAuthorPHID(), 61 + $task->getID()); 62 + 63 + break; 64 + } 65 + }
+11
src/applications/maniphest/storage/ManiphestTask.php
··· 44 44 protected $points; 45 45 protected $subtype; 46 46 47 + protected $closedEpoch; 48 + protected $closerPHID; 49 + 47 50 private $subscriberPHIDs = self::ATTACHABLE; 48 51 private $groupByProjectPHID = self::ATTACHABLE; 49 52 private $customFields = self::ATTACHABLE; ··· 90 93 'points' => 'double?', 91 94 'bridgedObjectPHID' => 'phid?', 92 95 'subtype' => 'text64', 96 + 'closedEpoch' => 'epoch?', 97 + 'closerPHID' => 'phid?', 93 98 ), 94 99 self::CONFIG_KEY_SCHEMA => array( 95 100 'key_phid' => null, ··· 130 135 ), 131 136 'key_subtype' => array( 132 137 'columns' => array('subtype'), 138 + ), 139 + 'key_closed' => array( 140 + 'columns' => array('closedEpoch'), 141 + ), 142 + 'key_closer' => array( 143 + 'columns' => array('closerPHID', 'closedEpoch'), 133 144 ), 134 145 ), 135 146 ) + parent::getConfiguration();
+1 -1
src/applications/maniphest/xaction/ManiphestTaskMergedIntoTransaction.php
··· 10 10 } 11 11 12 12 public function applyInternalEffects($object, $value) { 13 - $object->setStatus(ManiphestTaskStatus::getDuplicateStatus()); 13 + $this->updateStatus($object, ManiphestTaskStatus::getDuplicateStatus()); 14 14 } 15 15 16 16 public function getActionName() {
+1 -1
src/applications/maniphest/xaction/ManiphestTaskStatusTransaction.php
··· 10 10 } 11 11 12 12 public function applyInternalEffects($object, $value) { 13 - $object->setStatus($value); 13 + $this->updateStatus($object, $value); 14 14 } 15 15 16 16 public function shouldHide() {
+23
src/applications/maniphest/xaction/ManiphestTaskTransactionType.php
··· 3 3 abstract class ManiphestTaskTransactionType 4 4 extends PhabricatorModularTransactionType { 5 5 6 + protected function updateStatus($object, $new_value) { 7 + $old_value = $object->getStatus(); 8 + $object->setStatus($new_value); 9 + 10 + // If this status change closes or opens the task, update the closed 11 + // date and actor PHID. 12 + $old_closed = ManiphestTaskStatus::isClosedStatus($old_value); 13 + $new_closed = ManiphestTaskStatus::isClosedStatus($new_value); 14 + 15 + $is_close = ($new_closed && !$old_closed); 16 + $is_open = (!$new_closed && $old_closed); 17 + 18 + if ($is_close) { 19 + $object 20 + ->setClosedEpoch(PhabricatorTime::getNow()) 21 + ->setCloserPHID($this->getActingAsPHID()); 22 + } else if ($is_open) { 23 + $object 24 + ->setClosedEpoch(null) 25 + ->setCloserPHID(null); 26 + } 27 + } 28 + 6 29 }