Group inline transactions in Pholio

Summary:
Fixes T2639 by grouping related transactions at display time, so all the inlines merge into a nice block.

(Note that this does not do anything about T2709 yet, so there's still no way to figure out where the inlines actually are.)

Test Plan: {F35262}

Reviewers: chad

Reviewed By: chad

CC: aran

Maniphest Tasks: T2639

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

src/__phutil_library_map__.php

@@ -1484,6 +1484,7 @@ phutil_register_library_map(array(
     'PholioTransactionComment' => 'applications/pholio/storage/PholioTransactionComment.php',
     'PholioTransactionQuery' => 'applications/pholio/query/PholioTransactionQuery.php',
     'PholioTransactionType' => 'applications/pholio/constants/PholioTransactionType.php',
+    'PholioTransactionView' => 'applications/pholio/view/PholioTransactionView.php',
     'PhortuneMonthYearExpiryControl' => 'applications/phortune/control/PhortuneMonthYearExpiryControl.php',
     'PhortuneStripeBaseController' => 'applications/phortune/stripe/controller/PhortuneStripeBaseController.php',
     'PhortuneStripePaymentFormView' => 'applications/phortune/stripe/view/PhortuneStripePaymentFormView.php',
@@ -2998,6 +2999,7 @@ phutil_register_library_map(array(
     'PholioTransactionComment' => 'PhabricatorApplicationTransactionComment',
     'PholioTransactionQuery' => 'PhabricatorApplicationTransactionQuery',
     'PholioTransactionType' => 'PholioConstants',
+    'PholioTransactionView' => 'PhabricatorApplicationTransactionView',
     'PhortuneMonthYearExpiryControl' => 'AphrontFormControl',
     'PhortuneStripeBaseController' => 'PhabricatorController',
     'PhortuneStripePaymentFormView' => 'AphrontView',

src/applications/pholio/controller/PholioMockViewController.php

@@ -77,7 +77,7 @@ final class PholioMockViewController extends PholioController {
       ->setMock($mock)
       ->setImageID($this->imageID);
 
-    $xaction_view = id(new PhabricatorApplicationTransactionView())
+    $xaction_view = id(new PholioTransactionView())
       ->setUser($this->getRequest()->getUser())
       ->setTransactions($xactions)
       ->setMarkupEngine($engine);

src/applications/pholio/storage/PholioTransaction.php

@@ -17,6 +17,10 @@ final class PholioTransaction extends PhabricatorApplicationTransaction {
     return new PholioTransactionComment();
   }
 
+  public function getApplicationTransactionViewObject() {
+    return new PholioTransactionView();
+  }
+
   public function getApplicationObjectTypeName() {
     return pht('mock');
   }
@@ -33,13 +37,22 @@ final class PholioTransaction extends PhabricatorApplicationTransaction {
     return parent::shouldHide();
   }
 
+  public function getIcon() {
+    switch ($this->getTransactionType()) {
+      case PholioTransactionType::TYPE_INLINE:
+        return 'comment';
+    }
+    return parent::getIcon();
+  }
+
   public function getTitle() {
     $author_phid = $this->getAuthorPHID();
 
     $old = $this->getOldValue();
     $new = $this->getNewValue();
 
-    switch ($this->getTransactionType()) {
+    $type = $this->getTransactionType();
+    switch ($type) {
       case PholioTransactionType::TYPE_NAME:
         return pht(
           '%s renamed this mock from "%s" to "%s".',
@@ -53,9 +66,17 @@ final class PholioTransaction extends PhabricatorApplicationTransaction {
           $this->renderHandleLink($author_phid));
         break;
       case PholioTransactionType::TYPE_INLINE:
+        $count = 1;
+        foreach ($this->getTransactionGroup() as $xaction) {
+          if ($xaction->getTransactionType() == $type) {
+            $count++;
+          }
+        }
+
         return pht(
-          '%s added an inline comment.',
-          $this->renderHandleLink($author_phid));
+          '%s added %d inline comment(s).',
+          $this->renderHandleLink($author_phid),
+          $count);
     }
 
     return parent::getTitle();

src/applications/pholio/view/PholioTransactionView.php

@@ -0,0 +1,83 @@
+<?php
+
+final class PholioTransactionView
+  extends PhabricatorApplicationTransactionView {
+
+  protected function shouldGroupTransactions(
+    PhabricatorApplicationTransaction $u,
+    PhabricatorApplicationTransaction $v) {
+
+    if ($u->getAuthorPHID() != $v->getAuthorPHID()) {
+      // Don't group transactions by different authors.
+      return false;
+    }
+
+    if (($v->getDateCreated() - $u->getDateCreated()) > 60) {
+      // Don't group if transactions happend more than 60s apart.
+      return false;
+    }
+
+    switch ($u->getTransactionType()) {
+      case PhabricatorTransactions::TYPE_COMMENT:
+      case PholioTransactionType::TYPE_INLINE:
+        break;
+      default:
+        return false;
+    }
+
+    switch ($v->getTransactionType()) {
+      case PholioTransactionType::TYPE_INLINE:
+        return true;
+    }
+
+    return parent::shouldGroupTransactions($u, $v);
+  }
+
+  protected function renderTransactionContent(
+    PhabricatorApplicationTransaction $xaction) {
+
+    $out = array();
+
+    $group = $xaction->getTransactionGroup();
+    if ($xaction->getTransactionType() == PholioTransactionType::TYPE_INLINE) {
+      array_unshift($group, $xaction);
+    } else {
+      $out[] = parent::renderTransactionContent($xaction);
+    }
+
+    if (!$group) {
+      return $out;
+    }
+
+    $inlines = array();
+    foreach ($group as $xaction) {
+      switch ($xaction->getTransactionType()) {
+        case PholioTransactionType::TYPE_INLINE:
+          $inlines[] = $xaction;
+          break;
+        default:
+          throw new Exception("Unknown grouped transaction type!");
+      }
+    }
+
+    if ($inlines) {
+      $header = phutil_tag(
+        'div',
+        array(
+          'class' => 'phabricator-transaction-subheader',
+        ),
+        pht('Inline Comments'));
+
+      $out[] = $header;
+      foreach ($inlines as $inline) {
+        if (!$inline->getComment()) {
+          continue;
+        }
+        $out[] = parent::renderTransactionContent($inline);
+      }
+    }
+
+    return $out;
+  }
+
+}

src/applications/transactions/response/PhabricatorApplicationTransactionResponse.php

@@ -47,7 +47,14 @@ final class PhabricatorApplicationTransactionResponse
   }
 
   public function reduceProxyResponse() {
-    $view = id(new PhabricatorApplicationTransactionView())
+    if ($this->getTransactions()) {
+      $view = head($this->getTransactions())
+        ->getApplicationTransactionViewObject();
+    } else {
+      $view = new PhabricatorApplicationTransactionView();
+    }
+
+    $view
       ->setUser($this->getViewer())
       ->setTransactions($this->getTransactions())
       ->setIsPreview($this->isPreview);
@@ -70,4 +77,5 @@ final class PhabricatorApplicationTransactionResponse
     return $this->getProxy()->setContent($content);
   }
 
+
 }

src/applications/transactions/storage/PhabricatorApplicationTransaction.php

@@ -26,11 +26,16 @@ abstract class PhabricatorApplicationTransaction
 
   private $handles;
   private $renderingTarget = self::TARGET_HTML;
+  private $transactionGroup = array();
 
   abstract public function getApplicationTransactionType();
   abstract public function getApplicationTransactionCommentObject();
   abstract public function getApplicationObjectTypeName();
 
+  public function getApplicationTransactionViewObject() {
+    return new PhabricatorApplicationTransactionView();
+  }
+
   public function generatePHID() {
     $type = PhabricatorPHIDConstants::PHID_TYPE_XACT;
     $subtype = $this->getApplicationTransactionType();
@@ -329,6 +334,17 @@ abstract class PhabricatorApplicationTransaction
     return null;
   }
 
+  public function attachTransactionGroup(array $group) {
+    assert_instances_of($group, 'PhabricatorApplicationTransaction');
+    $this->transactionGroup = $group;
+    return $this;
+  }
+
+  public function getTransactionGroup() {
+    return $this->transactionGroup;
+  }
+
+
 /* -(  PhabricatorPolicyInterface Implementation  )-------------------------- */
 
 

src/applications/transactions/view/PhabricatorApplicationTransactionView.php

@@ -42,17 +42,33 @@ class PhabricatorApplicationTransactionView extends AphrontView {
   }
 
   public function buildEvents() {
-    $field = PhabricatorApplicationTransactionComment::MARKUP_FIELD_COMMENT;
-    $engine = $this->getOrBuildEngine();
-
     $user = $this->getUser();
 
     $anchor = $this->anchorOffset;
     $events = array();
-    foreach ($this->transactions as $xaction) {
+
+    $xactions = $this->transactions;
+    foreach ($xactions as $key => $xaction) {
       if ($xaction->shouldHide()) {
-        continue;
+        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)
@@ -103,12 +119,9 @@ class PhabricatorApplicationTransactionView extends AphrontView {
         }
       }
 
-      if ($xaction->hasComment()) {
-        $event->appendChild(
-          $engine->getOutput($xaction->getComment(), $field));
-      } else if ($has_deleted_comment) {
-        $event->appendChild(phutil_tag('em', array(), pht(
-          'This comment has been deleted.')));
+      $content = $this->renderTransactionContent($xaction);
+      if ($content) {
+        $event->appendChild($content);
       }
 
       $events[] = $event;
@@ -141,7 +154,7 @@ class PhabricatorApplicationTransactionView extends AphrontView {
   }
 
 
-  private function getOrBuildEngine() {
+  protected function getOrBuildEngine() {
     if ($this->engine) {
       return $this->engine;
     }
@@ -215,6 +228,32 @@ class PhabricatorApplicationTransactionView extends AphrontView {
     );
   }
 
+  protected function shouldGroupTransactions(
+    PhabricatorApplicationTransaction $u,
+    PhabricatorApplicationTransaction $v) {
+    return false;
+  }
+
+  protected function renderTransactionContent(
+    PhabricatorApplicationTransaction $xaction) {
+
+    $field = PhabricatorApplicationTransactionComment::MARKUP_FIELD_COMMENT;
+    $engine = $this->getOrBuildEngine();
+    $comment = $xaction->getComment();
+
+    if ($comment) {
+      if ($comment->getIsDeleted()) {
+        return phutil_tag(
+          'em',
+          array(),
+          pht('This comment has been deleted.'));
+      } else {
+        return $engine->getOutput($comment, $field);
+      }
+    }
+
+    return null;
+  }
 
 }
 

src/infrastructure/internationalization/PhabricatorBaseEnglishTranslation.php

@@ -279,6 +279,13 @@ abstract class PhabricatorBaseEnglishTranslation
         'You have %d unresolved setup issues...',
       ),
 
+      '%s added %d inline comment(s).' => array(
+        array(
+          '%s added an inline comment.',
+          '%s added inline comments.',
+        ),
+      ),
+
     );
   }
 

src/view/layout/PhabricatorPropertyListView.php

@@ -141,7 +141,7 @@ final class PhabricatorPropertyListView extends AphrontView {
       array(
         'class' => 'phabricator-property-list-properties',
       ),
-      $this->renderSingleView($items));
+      $items);
 
     $shortcuts = null;
     if ($this->hasKeyboardShortcuts) {

webroot/rsrc/css/aphront/transaction.css

@@ -51,3 +51,14 @@
   padding:        .3em 1em;
   overflow:       auto;
 }
+
+.phabricator-transaction-subheader {
+  color: #888888;
+  border-bottom: 1px solid #e0e0e0;
+  padding-bottom: 4px;
+  margin-bottom: 4px;
+}
+
+div.phabricator-remarkup + .phabricator-transaction-subheader {
+  margin-top: 12px;
+}