Lift inline state transactions into core (in Diffusion)

Summary:
Ref T1460. Ref T6403. Replace `Diffusion::INLINEDONE` with `Transactions::INLINESTATE` and generalize things enough that we can lift it into core.

The next change will lift Differential's similar implementation into the core.

Also start implementing a fix for T6403, providing an alternate hook for optional builtin transactions.

Test Plan: Changed inline state in Diffusion.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T6403, T1460

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

src/applications/audit/editor/PhabricatorAuditEditor.php

@@ -9,7 +9,8 @@ final class PhabricatorAuditEditor
   private $heraldEmailPHIDs = array();
   private $affectedFiles;
   private $rawPatch;
-  private $expandedDone;
+
+  private $didExpandInlineState;
 
   public function addAuditReason($phid, $reason) {
     if (!isset($this->auditReasonMap[$phid])) {
@@ -53,9 +54,9 @@ final class PhabricatorAuditEditor
 
     $types[] = PhabricatorTransactions::TYPE_COMMENT;
     $types[] = PhabricatorTransactions::TYPE_EDGE;
+    $types[] = PhabricatorTransactions::TYPE_INLINESTATE;
 
     $types[] = PhabricatorAuditTransaction::TYPE_COMMIT;
-    $types[] = PhabricatorAuditTransaction::TYPE_INLINEDONE;
 
     // TODO: These will get modernized eventually, but that can happen one
     // at a time later on.
@@ -104,7 +105,6 @@ final class PhabricatorAuditEditor
       case PhabricatorAuditActionConstants::INLINE:
       case PhabricatorAuditActionConstants::ADD_AUDITORS:
       case PhabricatorAuditTransaction::TYPE_COMMIT:
-      case PhabricatorAuditTransaction::TYPE_INLINEDONE:
         return $xaction->getNewValue();
     }
 
@@ -123,7 +123,6 @@ final class PhabricatorAuditEditor
       case PhabricatorAuditActionConstants::INLINE:
       case PhabricatorAuditActionConstants::ADD_AUDITORS:
       case PhabricatorAuditTransaction::TYPE_COMMIT:
-      case PhabricatorAuditTransaction::TYPE_INLINEDONE:
         return;
     }
 
@@ -147,18 +146,6 @@ final class PhabricatorAuditEditor
           $reply->setHasReplies(1)->save();
         }
         return;
-      case PhabricatorAuditTransaction::TYPE_INLINEDONE:
-        $table = new PhabricatorAuditTransactionComment();
-        $conn_w = $table->establishConnection('w');
-        foreach ($xaction->getNewValue() as $phid => $state) {
-          queryfx(
-            $conn_w,
-            'UPDATE %T SET fixedState = %s WHERE phid = %s',
-            $table->getTableName(),
-            $state,
-            $phid);
-        }
-        return;
       case PhabricatorAuditActionConstants::ADD_AUDITORS:
         $new = $xaction->getNewValue();
         if (!is_array($new)) {
@@ -204,6 +191,28 @@ final class PhabricatorAuditEditor
     return parent::applyCustomExternalTransaction($object, $xaction);
   }
 
+  protected function applyBuiltinExternalTransaction(
+    PhabricatorLiskDAO $object,
+    PhabricatorApplicationTransaction $xaction) {
+
+    switch ($xaction->getTransactionType()) {
+      case PhabricatorTransactions::TYPE_INLINESTATE:
+        $table = new PhabricatorAuditTransactionComment();
+        $conn_w = $table->establishConnection('w');
+        foreach ($xaction->getNewValue() as $phid => $state) {
+          queryfx(
+            $conn_w,
+            'UPDATE %T SET fixedState = %s WHERE phid = %s',
+            $table->getTableName(),
+            $state,
+            $phid);
+        }
+        return;
+    }
+
+    return parent::applyBuiltinExternalTransaction($object, $xaction);
+  }
+
   protected function applyFinalEffects(
     PhabricatorLiskDAO $object,
     array $xactions) {
@@ -361,12 +370,11 @@ final class PhabricatorAuditEditor
         break;
     }
 
-    if (!$this->expandedDone) {
+    if (!$this->didExpandInlineState) {
-
       switch ($xaction->getTransactionType()) {
         case PhabricatorTransactions::TYPE_COMMENT:
         case PhabricatorAuditActionConstants::ACTION:
-          $this->expandedDone = true;
+          $this->didExpandInlineState = true;
 
           $actor_phid = $this->getActingAsPHID();
           $actor_is_author = ($object->getAuthorPHID() == $actor_phid);
@@ -374,12 +382,7 @@ final class PhabricatorAuditEditor
             break;
           }
 
-          $state_map = array(
+          $state_map = PhabricatorTransactions::getInlineStateMap();
-            PhabricatorInlineCommentInterface::STATE_DRAFT =>
-              PhabricatorInlineCommentInterface::STATE_DONE,
-            PhabricatorInlineCommentInterface::STATE_UNDRAFT =>
-              PhabricatorInlineCommentInterface::STATE_UNDONE,
-          );
 
           $inlines = id(new DiffusionDiffInlineCommentQuery())
             ->setViewer($this->getActor())
@@ -398,7 +401,7 @@ final class PhabricatorAuditEditor
           }
 
           $xactions[] = id(new PhabricatorAuditTransaction())
-            ->setTransactionType(PhabricatorAuditTransaction::TYPE_INLINEDONE)
+            ->setTransactionType(PhabricatorTransactions::TYPE_INLINESTATE)
             ->setIgnoreOnNoEffect(true)
             ->setOldValue($old_value)
             ->setNewValue($new_value);

src/applications/audit/storage/PhabricatorAuditTransaction.php

@@ -4,7 +4,6 @@ final class PhabricatorAuditTransaction
   extends PhabricatorApplicationTransaction {
 
   const TYPE_COMMIT = 'audit:commit';
-  const TYPE_INLINEDONE = 'audit:inlinedone';
 
   const MAILTAG_ACTION_CONCERN = 'audit-action-concern';
   const MAILTAG_ACTION_ACCEPT  = 'audit-action-accept';
@@ -183,36 +182,6 @@ final class PhabricatorAuditTransaction
         }
         return $title;
 
-      case self::TYPE_INLINEDONE:
-        $done = 0;
-        $undone = 0;
-        foreach ($new as $phid => $state) {
-          if ($state == PhabricatorInlineCommentInterface::STATE_DONE) {
-            $done++;
-          } else {
-            $undone++;
-          }
-        }
-        if ($done && $undone) {
-          return pht(
-            '%s marked %s inline comment(s) as done and %s inline comment(s) '.
-            'as not done.',
-            $author_handle,
-            new PhutilNumber($done),
-            new PhutilNumber($undone));
-        } else if ($done) {
-          return pht(
-            '%s marked %s inline comment(s) as done.',
-            $author_handle,
-            new PhutilNumber($done));
-        } else {
-          return pht(
-            '%s marked %s inline comment(s) as not done.',
-            $author_handle,
-            new PhutilNumber($undone));
-        }
-        break;
-
       case PhabricatorAuditActionConstants::INLINE:
         return pht(
           '%s added inline comments.',
@@ -418,39 +387,17 @@ final class PhabricatorAuditTransaction
     return parent::getBodyForFeed($story);
   }
 
-
+  public function isInlineCommentTransaction() {
-  public function shouldGenerateOldValue() {
     switch ($this->getTransactionType()) {
-      case self::TYPE_INLINEDONE:
+      case PhabricatorAuditActionConstants::INLINE:
-        return false;
+        return true;
     }
 
-    return parent::shouldGenerateOldValue();
+    return parent::isInlineCommentTransaction();
-  }
-
-
-  // TODO: These two mail methods can likely be abstracted by introducing a
-  // formal concept of "inline comment" transactions.
-
-  public function shouldHideForMail(array $xactions) {
-    $type_inline = PhabricatorAuditActionConstants::INLINE;
-    switch ($this->getTransactionType()) {
-      case $type_inline:
-        foreach ($xactions as $xaction) {
-          if ($xaction->getTransactionType() != $type_inline) {
-            return true;
-          }
-        }
-        return ($this !== head($xactions));
-    }
-
-    return parent::shouldHideForMail($xactions);
   }
 
   public function getBodyForMail() {
     switch ($this->getTransactionType()) {
-      case PhabricatorAuditActionConstants::INLINE:
-        return null;
       case self::TYPE_COMMIT:
         $data = $this->getNewValue();
         return $data['description'];

src/applications/transactions/constants/PhabricatorTransactions.php

@@ -11,6 +11,7 @@ final class PhabricatorTransactions {
   const TYPE_CUSTOMFIELD  = 'core:customfield';
   const TYPE_BUILDABLE    = 'harbormaster:buildable';
   const TYPE_TOKEN        = 'token:give';
+  const TYPE_INLINESTATE  = 'core:inlinestate';
 
   const COLOR_RED         = 'red';
   const COLOR_ORANGE      = 'orange';
@@ -23,4 +24,14 @@ final class PhabricatorTransactions {
   const COLOR_GREY        = 'grey';
   const COLOR_BLACK       = 'black';
 
+
+  public static function getInlineStateMap() {
+    return array(
+      PhabricatorInlineCommentInterface::STATE_DRAFT =>
+        PhabricatorInlineCommentInterface::STATE_DONE,
+      PhabricatorInlineCommentInterface::STATE_UNDRAFT =>
+        PhabricatorInlineCommentInterface::STATE_UNDONE,
+    );
+  }
+
 }

src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php

@@ -294,6 +294,7 @@ abstract class PhabricatorApplicationTransactionEditor
       case PhabricatorTransactions::TYPE_JOIN_POLICY:
       case PhabricatorTransactions::TYPE_BUILDABLE:
       case PhabricatorTransactions::TYPE_TOKEN:
+      case PhabricatorTransactions::TYPE_INLINESTATE:
         return $xaction->getNewValue();
       case PhabricatorTransactions::TYPE_EDGE:
         return $this->getEdgeTransactionNewValue($xaction);
@@ -395,6 +396,8 @@ abstract class PhabricatorApplicationTransactionEditor
       case PhabricatorTransactions::TYPE_CUSTOMFIELD:
         $field = $this->getCustomFieldForTransaction($object, $xaction);
         return $field->applyApplicationTransactionInternalEffects($xaction);
+      case PhabricatorTransactions::TYPE_INLINESTATE:
+        return $this->applyBuiltinInternalTransaction($object, $xaction);
     }
 
     return $this->applyCustomInternalTransaction($object, $xaction);
@@ -489,6 +492,8 @@ abstract class PhabricatorApplicationTransactionEditor
       case PhabricatorTransactions::TYPE_CUSTOMFIELD:
         $field = $this->getCustomFieldForTransaction($object, $xaction);
         return $field->applyApplicationTransactionExternalEffects($xaction);
+      case PhabricatorTransactions::TYPE_INLINESTATE:
+        return $this->applyBuiltinExternalTransaction($object, $xaction);
     }
 
     return $this->applyCustomExternalTransaction($object, $xaction);
@@ -512,6 +517,23 @@ abstract class PhabricatorApplicationTransactionEditor
       "implementation!");
   }
 
+  // TODO: Write proper documentation for these hooks. These are like the
+  // "applyCustom" hooks, except that implementation is optional, so you do
+  // not need to handle all of the builtin transaction types. See T6403. These
+  // are not completely implemented.
+
+  protected function applyBuiltinInternalTransaction(
+    PhabricatorLiskDAO $object,
+    PhabricatorApplicationTransaction $xaction) {
+    return;
+  }
+
+  protected function applyBuiltinExternalTransaction(
+    PhabricatorLiskDAO $object,
+    PhabricatorApplicationTransaction $xaction) {
+    return;
+  }
+
   /**
    * Fill in a transaction's common values, like author and content source.
    */

src/applications/transactions/storage/PhabricatorApplicationTransaction.php

@@ -57,6 +57,7 @@ abstract class PhabricatorApplicationTransaction
       case PhabricatorTransactions::TYPE_BUILDABLE:
       case PhabricatorTransactions::TYPE_TOKEN:
       case PhabricatorTransactions::TYPE_CUSTOMFIELD:
+      case PhabricatorTransactions::TYPE_INLINESTATE:
         return false;
     }
     return true;
@@ -501,6 +502,26 @@ abstract class PhabricatorApplicationTransaction
         break;
     }
 
+    // If a transaction publishes an inline comment:
+    //
+    //   - Don't show it if there are other kinds of transactions. The
+    //     rationale here is that application mail will make the presence
+    //     of inline comments obvious enough by including them prominently
+    //     in the body. We could change this in the future if the obviousness
+    //     needs to be increased.
+    //   - If there are only inline transactions, only show the first
+    //     transaction. The rationale is that seeing multiple "added an inline
+    //     comment" transactions is not useful.
+
+    if ($this->isInlineCommentTransaction()) {
+      foreach ($xactions as $xaction) {
+        if (!$xaction->isInlineCommentTransaction()) {
+          return true;
+        }
+      }
+      return ($this !== head($xactions));
+    }
+
     return $this->shouldHide();
   }
 
@@ -539,10 +560,18 @@ abstract class PhabricatorApplicationTransaction
   }
 
   public function getBodyForMail() {
+    if ($this->isInlineCommentTransaction()) {
+      // We don't return inline comment content as mail body content, because
+      // applications need to contextualize it (by adding line numbers, for
+      // example) in order for it to make sense.
+      return null;
+    }
+
     $comment = $this->getComment();
     if ($comment && strlen($comment->getContent())) {
       return $comment->getContent();
     }
+
     return null;
   }
 
@@ -720,6 +749,36 @@ abstract class PhabricatorApplicationTransaction
             return null;
         }
 
+      case PhabricatorTransactions::TYPE_INLINESTATE:
+        $done = 0;
+        $undone = 0;
+        foreach ($new as $phid => $state) {
+          if ($state == PhabricatorInlineCommentInterface::STATE_DONE) {
+            $done++;
+          } else {
+            $undone++;
+          }
+        }
+        if ($done && $undone) {
+          return pht(
+            '%s marked %s inline comment(s) as done and %s inline comment(s) '.
+            'as not done.',
+            $this->renderHandleLink($author_phid),
+            new PhutilNumber($done),
+            new PhutilNumber($undone));
+        } else if ($done) {
+          return pht(
+            '%s marked %s inline comment(s) as done.',
+            $this->renderHandleLink($author_phid),
+            new PhutilNumber($done));
+        } else {
+          return pht(
+            '%s marked %s inline comment(s) as not done.',
+            $this->renderHandleLink($author_phid),
+            new PhutilNumber($undone));
+        }
+        break;
+
       default:
         return pht(
           '%s edited this %s.',
@@ -932,6 +991,10 @@ abstract class PhabricatorApplicationTransaction
     return false;
   }
 
+  public function isInlineCommentTransaction() {
+    return false;
+  }
+
   public function getActionName() {
     switch ($this->getTransactionType()) {
       case PhabricatorTransactions::TYPE_COMMENT: