diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2928,7 +2928,6 @@ 'PhabricatorFactManagementDestroyWorkflow' => 'applications/fact/management/PhabricatorFactManagementDestroyWorkflow.php', 'PhabricatorFactManagementListWorkflow' => 'applications/fact/management/PhabricatorFactManagementListWorkflow.php', 'PhabricatorFactManagementWorkflow' => 'applications/fact/management/PhabricatorFactManagementWorkflow.php', - 'PhabricatorFactManiphestTaskEngine' => 'applications/fact/engine/PhabricatorFactManiphestTaskEngine.php', 'PhabricatorFactObjectController' => 'applications/fact/controller/PhabricatorFactObjectController.php', 'PhabricatorFactObjectDimension' => 'applications/fact/storage/PhabricatorFactObjectDimension.php', 'PhabricatorFactRaw' => 'applications/fact/storage/PhabricatorFactRaw.php', @@ -3264,6 +3263,7 @@ 'PhabricatorManagementWorkflow' => 'infrastructure/management/PhabricatorManagementWorkflow.php', 'PhabricatorManiphestApplication' => 'applications/maniphest/application/PhabricatorManiphestApplication.php', 'PhabricatorManiphestConfigOptions' => 'applications/maniphest/config/PhabricatorManiphestConfigOptions.php', + 'PhabricatorManiphestTaskFactEngine' => 'applications/fact/engine/PhabricatorManiphestTaskFactEngine.php', 'PhabricatorManiphestTaskTestDataGenerator' => 'applications/maniphest/lipsum/PhabricatorManiphestTaskTestDataGenerator.php', 'PhabricatorManualActivitySetupCheck' => 'applications/config/check/PhabricatorManualActivitySetupCheck.php', 'PhabricatorMarkupCache' => 'applications/cache/storage/PhabricatorMarkupCache.php', @@ -4362,6 +4362,7 @@ 'PhabricatorTokensSettingsPanel' => 'applications/settings/panel/PhabricatorTokensSettingsPanel.php', 'PhabricatorTokensToken' => 'applications/tokens/storage/PhabricatorTokensToken.php', 'PhabricatorTransactionChange' => 'applications/transactions/data/PhabricatorTransactionChange.php', + 'PhabricatorTransactionFactEngine' => 'applications/fact/engine/PhabricatorTransactionFactEngine.php', 'PhabricatorTransactionRemarkupChange' => 'applications/transactions/data/PhabricatorTransactionRemarkupChange.php', 'PhabricatorTransactions' => 'applications/transactions/constants/PhabricatorTransactions.php', 'PhabricatorTransactionsApplication' => 'applications/transactions/application/PhabricatorTransactionsApplication.php', @@ -8459,7 +8460,6 @@ 'PhabricatorFactManagementDestroyWorkflow' => 'PhabricatorFactManagementWorkflow', 'PhabricatorFactManagementListWorkflow' => 'PhabricatorFactManagementWorkflow', 'PhabricatorFactManagementWorkflow' => 'PhabricatorManagementWorkflow', - 'PhabricatorFactManiphestTaskEngine' => 'PhabricatorFactEngine', 'PhabricatorFactObjectController' => 'PhabricatorFactController', 'PhabricatorFactObjectDimension' => 'PhabricatorFactDimension', 'PhabricatorFactRaw' => 'PhabricatorFactDAO', @@ -8833,6 +8833,7 @@ 'PhabricatorManagementWorkflow' => 'PhutilArgumentWorkflow', 'PhabricatorManiphestApplication' => 'PhabricatorApplication', 'PhabricatorManiphestConfigOptions' => 'PhabricatorApplicationConfigOptions', + 'PhabricatorManiphestTaskFactEngine' => 'PhabricatorTransactionFactEngine', 'PhabricatorManiphestTaskTestDataGenerator' => 'PhabricatorTestDataGenerator', 'PhabricatorManualActivitySetupCheck' => 'PhabricatorSetupCheck', 'PhabricatorMarkupCache' => 'PhabricatorCacheDAO', @@ -10156,6 +10157,7 @@ 'PhabricatorConduitResultInterface', ), 'PhabricatorTransactionChange' => 'Phobject', + 'PhabricatorTransactionFactEngine' => 'PhabricatorFactEngine', 'PhabricatorTransactionRemarkupChange' => 'PhabricatorTransactionChange', 'PhabricatorTransactions' => 'Phobject', 'PhabricatorTransactionsApplication' => 'PhabricatorApplication', diff --git a/src/applications/fact/controller/PhabricatorFactObjectController.php b/src/applications/fact/controller/PhabricatorFactObjectController.php --- a/src/applications/fact/controller/PhabricatorFactObjectController.php +++ b/src/applications/fact/controller/PhabricatorFactObjectController.php @@ -17,6 +17,10 @@ $engines = PhabricatorFactEngine::loadAllEngines(); foreach ($engines as $key => $engine) { + $engine = id(clone $engine) + ->setViewer($viewer); + $engines[$key] = $engine; + if (!$engine->supportsDatapointsForObject($object)) { unset($engines[$key]); } @@ -250,6 +254,58 @@ ->setTable($table); $content[] = $box; + + if ($engine instanceof PhabricatorTransactionFactEngine) { + $groups = $engine->newTransactionGroupsForObject($object); + $groups = array_values($groups); + + $xaction_phids = array(); + foreach ($groups as $group_key => $xactions) { + foreach ($xactions as $xaction) { + $xaction_phids[] = $xaction->getAuthorPHID(); + } + } + $xaction_handles = $viewer->loadHandles($xaction_phids); + + $rows = array(); + foreach ($groups as $group_key => $xactions) { + foreach ($xactions as $xaction) { + $rows[] = array( + $group_key, + $xaction->getTransactionType(), + $xaction_handles[$xaction->getAuthorPHID()]->renderLink(), + phabricator_datetime($xaction->getDateCreated(), $viewer), + ); + } + } + + $table = id(new AphrontTableView($rows)) + ->setHeaders( + array( + pht('Group'), + pht('Type'), + pht('Author'), + pht('Date'), + )) + ->setColumnClasses( + array( + null, + 'pri', + 'wide', + 'right', + )); + + $header = pht( + '%s (Transactions)', + get_class($engine)); + + $xaction_box = id(new PHUIObjectBoxView()) + ->setHeaderText($header) + ->setTable($table); + + $content[] = $xaction_box; + } + } $crumbs = $this->buildApplicationCrumbs() diff --git a/src/applications/fact/daemon/PhabricatorFactDaemon.php b/src/applications/fact/daemon/PhabricatorFactDaemon.php --- a/src/applications/fact/daemon/PhabricatorFactDaemon.php +++ b/src/applications/fact/daemon/PhabricatorFactDaemon.php @@ -62,6 +62,11 @@ public function setEngines(array $engines) { assert_instances_of($engines, 'PhabricatorFactEngine'); + $viewer = PhabricatorUser::getOmnipotentUser(); + foreach ($engines as $engine) { + $engine->setViewer($viewer); + } + $this->engines = $engines; return $this; } diff --git a/src/applications/fact/engine/PhabricatorFactEngine.php b/src/applications/fact/engine/PhabricatorFactEngine.php --- a/src/applications/fact/engine/PhabricatorFactEngine.php +++ b/src/applications/fact/engine/PhabricatorFactEngine.php @@ -3,6 +3,7 @@ abstract class PhabricatorFactEngine extends Phobject { private $factMap; + private $viewer; final public static function loadAllEngines() { return id(new PhutilClassMapQuery()) @@ -35,8 +36,17 @@ return $this->factMap[$key]; } - final protected function getViewer() { - return PhabricatorUser::getOmnipotentUser(); + public function setViewer(PhabricatorUser $viewer) { + $this->viewer = $viewer; + return $this; + } + + public function getViewer() { + if (!$this->viewer) { + throw new PhutilInvalidStateException('setViewer'); + } + + return $this->viewer; } } diff --git a/src/applications/fact/engine/PhabricatorFactManiphestTaskEngine.php b/src/applications/fact/engine/PhabricatorManiphestTaskFactEngine.php rename from src/applications/fact/engine/PhabricatorFactManiphestTaskEngine.php rename to src/applications/fact/engine/PhabricatorManiphestTaskFactEngine.php --- a/src/applications/fact/engine/PhabricatorFactManiphestTaskEngine.php +++ b/src/applications/fact/engine/PhabricatorManiphestTaskFactEngine.php @@ -1,7 +1,7 @@ setKey('tasks.open-points.status.owner'), id(new PhabricatorPointsFact()) - ->setKey('tasks.open-points.score.project'), + ->setKey('tasks.open-points.score.owner'), id(new PhabricatorPointsFact()) ->setKey('tasks.open-points.assign.owner'), ); @@ -84,16 +84,7 @@ } public function newDatapointsForObject(PhabricatorLiskDAO $object) { - $viewer = $this->getViewer(); - - $xaction_query = PhabricatorApplicationTransactionQuery::newQueryForObject( - $object); - $xactions = $xaction_query - ->setViewer($viewer) - ->withObjectPHIDs(array($object->getPHID())) - ->execute(); - - $xactions = msortv($xactions, 'newChronologicalSortVector'); + $xaction_groups = $this->newTransactionGroupsForObject($object); $old_open = false; $old_points = 0; @@ -104,7 +95,7 @@ $specs = array(); $datapoints = array(); - foreach ($xactions as $xaction_group) { + foreach ($xaction_groups as $xaction_group) { $add_projects = array(); $rem_projects = array(); @@ -112,8 +103,11 @@ $new_points = $old_points; $new_owner = $old_owner; - // TODO: Actually group concurrent transactions. - $xaction_group = array($xaction_group); + if ($is_create) { + // Assume tasks start open. + // TODO: This might be a questionable assumption? + $new_open = true; + } $group_epoch = last($xaction_group)->getDateCreated(); foreach ($xaction_group as $xaction) { @@ -167,15 +161,17 @@ if ($is_create) { $action = 'create'; $action_points = $new_points; + $include_open = $new_open; } else { $action = 'assign'; $action_points = $old_points; + $include_open = $old_open; } foreach ($project_sets as $project_set) { $scale = $project_set['scale']; foreach ($project_set['phids'] as $project_phid) { - if ($old_open) { + if ($include_open) { $specs[] = array( "tasks.open-count.{$action}.project", 1 * $scale, @@ -227,7 +223,9 @@ continue; } - if ($old_open) { + $scale = $owner_set['scale']; + + if ($old_open != $new_open) { $specs[] = array( "tasks.open-count.{$action}.owner", 1 * $scale, @@ -247,14 +245,14 @@ $owner_phid, ); - $specs[] = array( - "tasks.points.{$action}.owner", - $action_points * $scale, - $owner_phid, - ); + if ($action_points) { + $specs[] = array( + "tasks.points.{$action}.owner", + $action_points * $scale, + $owner_phid, + ); + } } - - $old_owner = $new_owner; } if ($is_create) { @@ -262,6 +260,7 @@ 'tasks.count.create', 1, ); + $specs[] = array( 'tasks.points.create', $new_points, @@ -316,11 +315,9 @@ $specs[] = array( 'tasks.open-points.status.project', $action_points * $scale, - $new_owner, + $project_phid, ); } - - $old_open = $new_open; } // The "score" facts only apply to rescoring tasks which already @@ -371,14 +368,23 @@ $delta, ); } - - $old_points = $new_points; } + $old_points = $new_points; + $old_open = $new_open; + $old_owner = $new_owner; + foreach ($specs as $spec) { $spec_key = $spec[0]; $spec_value = $spec[1]; + // Don't write any facts with a value of 0. The "count" facts never + // have a value of 0, and the "points" facts aren't meaningful if + // they have a value of 0. + if ($spec_value == 0) { + continue; + } + $datapoint = $this->getFact($spec_key) ->newDatapoint(); @@ -401,4 +407,5 @@ return $datapoints; } + } diff --git a/src/applications/fact/engine/PhabricatorTransactionFactEngine.php b/src/applications/fact/engine/PhabricatorTransactionFactEngine.php new file mode 100644 --- /dev/null +++ b/src/applications/fact/engine/PhabricatorTransactionFactEngine.php @@ -0,0 +1,84 @@ +getViewer(); + + $xaction_query = PhabricatorApplicationTransactionQuery::newQueryForObject( + $object); + $xactions = $xaction_query + ->setViewer($viewer) + ->withObjectPHIDs(array($object->getPHID())) + ->execute(); + + $xactions = msortv($xactions, 'newChronologicalSortVector'); + + return $this->groupTransactions($xactions); + } + + protected function groupTransactions(array $xactions) { + // These grouping rules are generally much looser than the display grouping + // rules. As long as the same user is editing the task and they don't leave + // it alone for a particularly long time, we'll group things together. + + $breaks = array(); + + $touch_window = phutil_units('15 minutes in seconds'); + $user_type = PhabricatorPeopleUserPHIDType::TYPECONST; + + $last_actor = null; + $last_epoch = null; + + foreach ($xactions as $key => $xaction) { + $this_actor = $xaction->getAuthorPHID(); + if (phid_get_type($this_actor) != $user_type) { + $this_actor = null; + } + + if ($this_actor && $last_actor && ($this_actor != $last_actor)) { + $breaks[$key] = true; + } + + // If too much time passed between changes, group them separately. + $this_epoch = $xaction->getDateCreated(); + if ($last_epoch) { + if (($this_epoch - $last_epoch) > $touch_window) { + $breaks[$key] = true; + } + } + + // The clock gets reset every time the same real user touches the + // task, but does not reset if an automated actor touches things. + if (!$last_actor || ($this_actor == $last_actor)) { + $last_epoch = $this_epoch; + } + + if ($this_actor && ($last_actor != $this_actor)) { + $last_actor = $this_actor; + $last_epoch = $this_epoch; + } + } + + $groups = array(); + $group = array(); + foreach ($xactions as $key => $xaction) { + if (isset($breaks[$key])) { + if ($group) { + $groups[] = $group; + $group = array(); + } + } + + $group[] = $xaction; + } + + if ($group) { + $groups[] = $group; + } + + return $groups; + } + +}