Fix some of the most obvious bugs in fact generation from Maniphest tasks

Summary:
Depends on D19121. Ref T13083. Group transactions and show groups in the debugging view.

Fix some of the most obvious issues with fact generation:

  - No more 0-point facts.
  - Engine can now generate at least one of every type of fact.

Test Plan: Generated facts, viewed them in the debugging view, fact generation largely appeared to align with reality. No more "no facts in storage" facts.

Subscribers: yelirekim

Maniphest Tasks: T13083

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

src/__phutil_library_map__.php

@@ -2928,7 +2928,6 @@ phutil_register_library_map(array(
     '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 @@ phutil_register_library_map(array(
     '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 @@ phutil_register_library_map(array(
     '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 @@ phutil_register_library_map(array(
     'PhabricatorFactManagementDestroyWorkflow' => 'PhabricatorFactManagementWorkflow',
     'PhabricatorFactManagementListWorkflow' => 'PhabricatorFactManagementWorkflow',
     'PhabricatorFactManagementWorkflow' => 'PhabricatorManagementWorkflow',
-    'PhabricatorFactManiphestTaskEngine' => 'PhabricatorFactEngine',
     'PhabricatorFactObjectController' => 'PhabricatorFactController',
     'PhabricatorFactObjectDimension' => 'PhabricatorFactDimension',
     'PhabricatorFactRaw' => 'PhabricatorFactDAO',
@@ -8833,6 +8833,7 @@ phutil_register_library_map(array(
     'PhabricatorManagementWorkflow' => 'PhutilArgumentWorkflow',
     'PhabricatorManiphestApplication' => 'PhabricatorApplication',
     'PhabricatorManiphestConfigOptions' => 'PhabricatorApplicationConfigOptions',
+    'PhabricatorManiphestTaskFactEngine' => 'PhabricatorTransactionFactEngine',
     'PhabricatorManiphestTaskTestDataGenerator' => 'PhabricatorTestDataGenerator',
     'PhabricatorManualActivitySetupCheck' => 'PhabricatorSetupCheck',
     'PhabricatorMarkupCache' => 'PhabricatorCacheDAO',
@@ -10156,6 +10157,7 @@ phutil_register_library_map(array(
       'PhabricatorConduitResultInterface',
     ),
     'PhabricatorTransactionChange' => 'Phobject',
+    'PhabricatorTransactionFactEngine' => 'PhabricatorFactEngine',
     'PhabricatorTransactionRemarkupChange' => 'PhabricatorTransactionChange',
     'PhabricatorTransactions' => 'Phobject',
     'PhabricatorTransactionsApplication' => 'PhabricatorApplication',

src/applications/fact/controller/PhabricatorFactObjectController.php

@@ -17,6 +17,10 @@ final class PhabricatorFactObjectController
 
     $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 @@ final class PhabricatorFactObjectController
         ->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()

src/applications/fact/daemon/PhabricatorFactDaemon.php

@@ -62,6 +62,11 @@ final class PhabricatorFactDaemon extends PhabricatorDaemon {
   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;
   }

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 @@ abstract class PhabricatorFactEngine extends Phobject {
     return $this->factMap[$key];
   }
 
-  final protected function getViewer() {
+  public function setViewer(PhabricatorUser $viewer) {
-    return PhabricatorUser::getOmnipotentUser();
+    $this->viewer = $viewer;
+    return $this;
+  }
+
+  public function getViewer() {
+    if (!$this->viewer) {
+      throw new PhutilInvalidStateException('setViewer');
+    }
+
+    return $this->viewer;
   }
 
 }

src/applications/fact/engine/PhabricatorManiphestTaskFactEngine.php

@@ -1,7 +1,7 @@
 <?php
 
-final class PhabricatorFactManiphestTaskEngine
+final class PhabricatorManiphestTaskFactEngine
-  extends PhabricatorFactEngine {
+  extends PhabricatorTransactionFactEngine {
 
   public function newFacts() {
     return array(
@@ -73,7 +73,7 @@ final class PhabricatorFactManiphestTaskEngine
       id(new PhabricatorPointsFact())
         ->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 @@ final class PhabricatorFactManiphestTaskEngine
   }
 
   public function newDatapointsForObject(PhabricatorLiskDAO $object) {
-    $viewer = $this->getViewer();
+    $xaction_groups = $this->newTransactionGroupsForObject($object);
-
-    $xaction_query = PhabricatorApplicationTransactionQuery::newQueryForObject(
-      $object);
-    $xactions = $xaction_query
-      ->setViewer($viewer)
-      ->withObjectPHIDs(array($object->getPHID()))
-      ->execute();
-
-    $xactions = msortv($xactions, 'newChronologicalSortVector');
 
     $old_open = false;
     $old_points = 0;
@@ -104,7 +95,7 @@ final class PhabricatorFactManiphestTaskEngine
 
     $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 @@ final class PhabricatorFactManiphestTaskEngine
       $new_points = $old_points;
       $new_owner = $old_owner;
 
-      // TODO: Actually group concurrent transactions.
+      if ($is_create) {
-      $xaction_group = array($xaction_group);
+        // 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 @@ final class PhabricatorFactManiphestTaskEngine
       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 @@ final class PhabricatorFactManiphestTaskEngine
             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 @@ final class PhabricatorFactManiphestTaskEngine
             $owner_phid,
           );
 
-          $specs[] = array(
+          if ($action_points) {
-            "tasks.points.{$action}.owner",
+            $specs[] = array(
-            $action_points * $scale,
+              "tasks.points.{$action}.owner",
-            $owner_phid,
+              $action_points * $scale,
-          );
+              $owner_phid,
+            );
+          }
         }
-
-        $old_owner = $new_owner;
       }
 
       if ($is_create) {
@@ -262,6 +260,7 @@ final class PhabricatorFactManiphestTaskEngine
           'tasks.count.create',
           1,
         );
+
         $specs[] = array(
           'tasks.points.create',
           $new_points,
@@ -316,11 +315,9 @@ final class PhabricatorFactManiphestTaskEngine
           $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 @@ final class PhabricatorFactManiphestTaskEngine
             $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 @@ final class PhabricatorFactManiphestTaskEngine
     return $datapoints;
   }
 
+
 }

src/applications/fact/engine/PhabricatorTransactionFactEngine.php

@@ -0,0 +1,84 @@
+<?php
+
+abstract class PhabricatorTransactionFactEngine
+  extends PhabricatorFactEngine {
+
+  public function newTransactionGroupsForObject(PhabricatorLiskDAO $object) {
+    $viewer = $this->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;
+  }
+
+}