Implment ApplicationTransaction grouping rules

Summary: Ref T4266. This implements rules similar to the old rules. With D7842, maybe this is reasonable? I think it's not like grotesquely bad, at least.

Test Plan: See screenshot.

Reviewers: chad, wrotte

Reviewed By: chad

CC: aran

Maniphest Tasks: T4266

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

src/applications/transactions/storage/PhabricatorApplicationTransaction.php

@@ -233,7 +233,7 @@ abstract class PhabricatorApplicationTransaction
         return 'link';
     }
 
-    return null;
+    return 'edit';
   }
 
   public function getColor() {
@@ -537,6 +537,57 @@ abstract class PhabricatorApplicationTransaction
     return $this->transactionGroup;
   }
 
+  /**
+   * Should this transaction be visually grouped with an existing transaction
+   * group?
+   *
+   * @param list<PhabricatorApplicationTransaction> List of transactions.
+   * @return bool True to display in a group with the other transactions.
+   */
+  public function shouldDisplayGroupWith(array $group) {
+    $type_comment = PhabricatorTransactions::TYPE_COMMENT;
+
+    $this_source = null;
+    if ($this->getContentSource()) {
+      $this_source = $this->getContentSource()->getSource();
+    }
+
+    foreach ($group as $xaction) {
+      // Don't group transactions by different authors.
+      if ($xaction->getAuthorPHID() != $this->getAuthorPHID()) {
+        return false;
+      }
+
+      // Don't group transactions for different objects.
+      if ($xaction->getObjectPHID() != $this->getObjectPHID()) {
+        return false;
+      }
+
+      // Don't group anything into a group which already has a comment.
+      if ($xaction->getTransactionType() == $type_comment) {
+        return false;
+      }
+
+      // Don't group transactions from different content sources.
+      $other_source = null;
+      if ($xaction->getContentSource()) {
+        $other_source = $xaction->getContentSource()->getSource();
+      }
+
+      if ($other_source != $this_source) {
+        return false;
+      }
+
+      // Don't group transactions which happened more than 2 minutes apart.
+      $apart = abs($xaction->getDateCreated() - $this->getDateCreated());
+      if ($apart > (60 * 2)) {
+        return false;
+      }
+    }
+
+    return true;
+  }
+
 
 /* -(  PhabricatorPolicyInterface Implementation  )-------------------------- */
 

src/applications/transactions/view/PhabricatorApplicationTransactionView.php

@@ -61,91 +61,26 @@ class PhabricatorApplicationTransactionView extends AphrontView {
     $user = $this->getUser();
 
     $anchor = $this->anchorOffset;
-    $events = array();
 
     $xactions = $this->transactions;
-    foreach ($xactions as $key => $xaction) {
-      if ($xaction->shouldHide()) {
-        unset($xactions[$key]);
-      }
-    }
 
-    $last = null;
-    $last_key = null;
-    $groups = array();
-    foreach ($xactions as $key => $xaction) {
-      if ($last && $this->shouldGroupTransactions($last, $xaction)) {
-        $groups[$last_key][] = $xaction;
-        unset($xactions[$key]);
-      } else {
-        $last = $xaction;
-        $last_key = $key;
-      }
-    }
-
-    foreach ($xactions as $key => $xaction) {
-      $xaction->attachTransactionGroup(idx($groups, $key, array()));
-
-      $event = id(new PhabricatorTimelineEventView())
-        ->setUser($user)
-        ->setTransactionPHID($xaction->getPHID())
-        ->setUserHandle($xaction->getHandle($xaction->getAuthorPHID()))
-        ->setIcon($xaction->getIcon())
-        ->setColor($xaction->getColor());
-
-      $title = $xaction->getTitle();
-      if ($xaction->hasChangeDetails()) {
-        if ($this->isPreview || $this->isDetailView) {
-          $details = $this->buildChangeDetails($xaction);
-        } else {
-          $details = $this->buildChangeDetailsLink($xaction);
-        }
-        $title = array(
-          $title,
-          ' ',
-          $details,
-        );
-      }
-      $event->setTitle($title);
-
-      if ($this->isPreview) {
-        $event->setIsPreview(true);
-      } else {
-        $event
-          ->setDateCreated($xaction->getDateCreated())
-          ->setContentSource($xaction->getContentSource())
-          ->setAnchor($anchor);
+    $xactions = $this->filterHiddenTransactions($xactions);
+    $xactions = $this->groupRelatedTransactions($xactions);
+    $groups = $this->groupDisplayTransactions($xactions);
 
+    $events = array();
+    foreach ($groups as $group) {
+      $group_event = null;
+      foreach ($group as $xaction) {
+        $event = $this->renderEvent($xaction, $group, $anchor);
         $anchor++;
-      }
-
-      $has_deleted_comment = $xaction->getComment() &&
-        $xaction->getComment()->getIsDeleted();
-
-      if ($this->getShowEditActions() && !$this->isPreview) {
-        if ($xaction->getCommentVersion() > 1) {
-          $event->setIsEdited(true);
-        }
-
-        $can_edit = PhabricatorPolicyCapability::CAN_EDIT;
-
-        if ($xaction->hasComment() || $has_deleted_comment) {
-          $has_edit_capability = PhabricatorPolicyFilter::hasCapability(
-            $user,
-            $xaction,
-            $can_edit);
-          if ($has_edit_capability) {
-            $event->setIsEditable(true);
+        if (!$group_event) {
+          $group_event = $event;
+        } else {
+          $group_event->addEventToGroup($event);
         }
       }
-      }
-
-      $content = $this->renderTransactionContent($xaction);
-      if ($content) {
-        $event->appendChild($content);
-      }
-
-      $events[] = $event;
+      $events[] = $group_event;
     }
 
     return $events;
@@ -295,5 +230,148 @@ class PhabricatorApplicationTransactionView extends AphrontView {
     return null;
   }
 
+  private function filterHiddenTransactions(array $xactions) {
+    foreach ($xactions as $key => $xaction) {
+      if ($xaction->shouldHide()) {
+        unset($xactions[$key]);
+      }
+    }
+    return $xactions;
+  }
+
+  private function groupRelatedTransactions(array $xactions) {
+    $last = null;
+    $last_key = null;
+    $groups = array();
+    foreach ($xactions as $key => $xaction) {
+      if ($last && $this->shouldGroupTransactions($last, $xaction)) {
+        $groups[$last_key][] = $xaction;
+        unset($xactions[$key]);
+      } else {
+        $last = $xaction;
+        $last_key = $key;
+      }
+    }
+
+    foreach ($xactions as $key => $xaction) {
+      $xaction->attachTransactionGroup(idx($groups, $key, array()));
+    }
+
+    return $xactions;
+  }
+
+  private function groupDisplayTransactions(array $xactions) {
+    $groups = array();
+    $group = array();
+    foreach ($xactions as $xaction) {
+      if ($xaction->shouldDisplayGroupWith($group)) {
+        $group[] = $xaction;
+      } else {
+        if ($group) {
+          $groups[] = $group;
+        }
+        $group = array($xaction);
+      }
+    }
+
+    if ($group) {
+      $groups[] = $group;
+    }
+
+    foreach ($groups as $key => $group) {
+      $group = msort($group, 'getActionStrength');
+      $group = array_reverse($group);
+      $groups[$key] = $group;
+    }
+
+    return $groups;
+  }
+
+  private function renderEvent(
+    PhabricatorApplicationTransaction $xaction,
+    array $group,
+    $anchor) {
+    $viewer = $this->getUser();
+
+    $event = id(new PhabricatorTimelineEventView())
+      ->setUser($viewer)
+      ->setTransactionPHID($xaction->getPHID())
+      ->setUserHandle($xaction->getHandle($xaction->getAuthorPHID()))
+      ->setIcon($xaction->getIcon())
+      ->setColor($xaction->getColor());
+
+    if (!$this->shouldSuppressTitle($xaction, $group)) {
+      $title = $xaction->getTitle();
+      if ($xaction->hasChangeDetails()) {
+        if ($this->isPreview || $this->isDetailView) {
+          $details = $this->buildChangeDetails($xaction);
+        } else {
+          $details = $this->buildChangeDetailsLink($xaction);
+        }
+        $title = array(
+          $title,
+          ' ',
+          $details,
+        );
+      }
+      $event->setTitle($title);
+    }
+
+    if ($this->isPreview) {
+      $event->setIsPreview(true);
+    } else {
+      $event
+        ->setDateCreated($xaction->getDateCreated())
+        ->setContentSource($xaction->getContentSource())
+        ->setAnchor($anchor);
+    }
+
+    $has_deleted_comment = $xaction->getComment() &&
+      $xaction->getComment()->getIsDeleted();
+
+    if ($this->getShowEditActions() && !$this->isPreview) {
+      if ($xaction->getCommentVersion() > 1) {
+        $event->setIsEdited(true);
+      }
+
+      $can_edit = PhabricatorPolicyCapability::CAN_EDIT;
+
+      if ($xaction->hasComment() || $has_deleted_comment) {
+        $has_edit_capability = PhabricatorPolicyFilter::hasCapability(
+          $viewer,
+          $xaction,
+          $can_edit);
+        if ($has_edit_capability) {
+          $event->setIsEditable(true);
+        }
+      }
+    }
+
+    $content = $this->renderTransactionContent($xaction);
+    if ($content) {
+      $event->appendChild($content);
+    }
+
+    return $event;
+  }
+
+  private function shouldSuppressTitle(
+    PhabricatorApplicationTransaction $xaction,
+    array $group) {
+
+    // This is a little hard-coded, but we don't have any other reasonable
+    // cases for now. Suppress "commented on" if there are other actions in
+    // the display group.
+
+    if (count($group) > 1) {
+      $type_comment = PhabricatorTransactions::TYPE_COMMENT;
+      if ($xaction->getTransactionType() == $type_comment) {
+        return true;
+      }
+    }
+
+    return false;
+  }
+
 }
 

src/view/layout/PhabricatorTimelineEventView.php

@@ -119,15 +119,17 @@ final class PhabricatorTimelineEventView extends AphrontView {
       $extra = array();
       $is_first_extra = true;
       foreach ($this->getEventGroup() as $event) {
-        $extra[] = $this->renderExtra($is_first_extra);
+        $extra[] = $event->renderExtra($is_first_extra);
         $is_first_extra = false;
       }
+      $extra = array_reverse($extra);
+      $extra = array_mergev($extra);
       $extra = phutil_tag(
         'span',
         array(
           'class' => 'phabricator-timeline-extra',
         ),
-        phutil_implode_html(" \xC2\xB7 ", array_mergev($extra)));
+        phutil_implode_html(" \xC2\xB7 ", $extra));
     } else {
       $extra = null;
     }