Lift inline comment state transactions into core (in Differential)

Summary: Ref T1460. Follows D12129 and reduces code duplication.

Test Plan: Changed inline state in Differential.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T1460

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

src/applications/differential/editor/DifferentialTransactionEditor.php

@@ -7,7 +7,7 @@ final class DifferentialTransactionEditor
   private $changedPriorToCommitURI;
   private $isCloseByCommit;
   private $repositoryPHIDOverride = false;
-  private $expandedDone = false;
+  private $didExpandInlineState = false;
 
   public function getEditorApplicationClass() {
     return 'PhabricatorDifferentialApplication';
@@ -100,7 +100,6 @@ final class DifferentialTransactionEditor
       case PhabricatorTransactions::TYPE_EDIT_POLICY:
       case DifferentialTransaction::TYPE_ACTION:
       case DifferentialTransaction::TYPE_UPDATE:
-      case DifferentialTransaction::TYPE_INLINEDONE:
         return $xaction->getNewValue();
       case DifferentialTransaction::TYPE_INLINE:
         return null;
@@ -199,7 +198,6 @@ final class DifferentialTransactionEditor
       case PhabricatorTransactions::TYPE_SUBSCRIBERS:
       case PhabricatorTransactions::TYPE_COMMENT:
       case DifferentialTransaction::TYPE_INLINE:
-      case DifferentialTransaction::TYPE_INLINEDONE:
         return;
       case PhabricatorTransactions::TYPE_EDGE:
         return;
@@ -527,13 +525,13 @@ final class DifferentialTransactionEditor
       break;
     }
 
-    if (!$this->expandedDone) {
+    if (!$this->didExpandInlineState) {
       switch ($xaction->getTransactionType()) {
         case PhabricatorTransactions::TYPE_COMMENT:
         case DifferentialTransaction::TYPE_ACTION:
         case DifferentialTransaction::TYPE_UPDATE:
         case DifferentialTransaction::TYPE_INLINE:
-          $this->expandedDone = true;
+          $this->didExpandInlineState = true;
 
           $actor_phid = $this->getActingAsPHID();
           $actor_is_author = ($object->getAuthorPHID() == $actor_phid);
@@ -541,12 +539,7 @@ final class DifferentialTransactionEditor
             break;
           }
 
-          $state_map = array(
+          $state_map = PhabricatorTransactions::getInlineStateMap();
-            PhabricatorInlineCommentInterface::STATE_DRAFT =>
-              PhabricatorInlineCommentInterface::STATE_DONE,
-            PhabricatorInlineCommentInterface::STATE_UNDRAFT =>
-              PhabricatorInlineCommentInterface::STATE_UNDONE,
-          );
 
           $inlines = id(new DifferentialDiffInlineCommentQuery())
             ->setViewer($this->getActor())
@@ -565,7 +558,7 @@ final class DifferentialTransactionEditor
           }
 
           $results[] = id(new DifferentialTransaction())
-            ->setTransactionType(DifferentialTransaction::TYPE_INLINEDONE)
+            ->setTransactionType(PhabricatorTransactions::TYPE_INLINESTATE)
             ->setIgnoreOnNoEffect(true)
             ->setOldValue($old_value)
             ->setNewValue($new_value);
@@ -595,18 +588,6 @@ final class DifferentialTransactionEditor
           $reply->setHasReplies(1)->save();
         }
         return;
-      case DifferentialTransaction::TYPE_INLINEDONE:
-        $table = new DifferentialTransactionComment();
-        $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 DifferentialTransaction::TYPE_UPDATE:
         // Now that we're inside the transaction, do a final check.
         $diff = $this->requireDiff($xaction->getNewValue());
@@ -631,6 +612,28 @@ final class DifferentialTransactionEditor
     return parent::applyCustomExternalTransaction($object, $xaction);
   }
 
+  protected function applyBuiltinExternalTransaction(
+    PhabricatorLiskDAO $object,
+    PhabricatorApplicationTransaction $xaction) {
+
+    switch ($xaction->getTransactionType()) {
+      case PhabricatorTransactions::TYPE_INLINESTATE:
+        $table = new DifferentialTransactionComment();
+        $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 mergeEdgeData($type, array $u, array $v) {
     $result = parent::mergeEdgeData($type, $u, $v);
 

src/applications/differential/storage/DifferentialTransaction.php

@@ -18,7 +18,6 @@ final class DifferentialTransaction extends PhabricatorApplicationTransaction {
   const TYPE_UPDATE = 'differential:update';
   const TYPE_ACTION = 'differential:action';
   const TYPE_STATUS = 'differential:status';
-  const TYPE_INLINEDONE = 'differential:inlinedone';
 
   public function getApplicationName() {
     return 'differential';
@@ -36,15 +35,6 @@ final class DifferentialTransaction extends PhabricatorApplicationTransaction {
     return new DifferentialTransactionView();
   }
 
-  public function shouldGenerateOldValue() {
-    switch ($this->getTransactionType()) {
-      case DifferentialTransaction::TYPE_INLINEDONE:
-        return false;
-    }
-
-    return parent::shouldGenerateOldValue();
-  }
-
   public function shouldHide() {
     $old = $this->getOldValue();
     $new = $this->getNewValue();
@@ -80,33 +70,13 @@ final class DifferentialTransaction extends PhabricatorApplicationTransaction {
     return parent::shouldHide();
   }
 
-  public function shouldHideForMail(array $xactions) {
+  public function isInlineCommentTransaction() {
     switch ($this->getTransactionType()) {
       case self::TYPE_INLINE:
-        // Hide inlines when rendering mail transactions if any other
+        return true;
-        // transaction type exists.
-        foreach ($xactions as $xaction) {
-          if ($xaction->getTransactionType() != self::TYPE_INLINE) {
-            return true;
-          }
-        }
-
-        // If only inline transactions exist, we just render the first one.
-        return ($this !== head($xactions));
     }
 
-    return parent::shouldHideForMail($xactions);
+    return parent::isInlineCommentTransaction();
-  }
-
-  public function getBodyForMail() {
-    switch ($this->getTransactionType()) {
-      case self::TYPE_INLINE:
-        // Don't render inlines into the mail body; they render into a special
-        // section immediately after the body instead.
-        return null;
-    }
-
-    return parent::getBodyForMail();
   }
 
   public function getRequiredHandlePHIDs() {
@@ -145,8 +115,6 @@ final class DifferentialTransaction extends PhabricatorApplicationTransaction {
         return 3;
       case self::TYPE_UPDATE:
         return 2;
-      case self::TYPE_INLINE:
-        return 0.25;
     }
 
     return parent::getActionStrength();
@@ -241,35 +209,6 @@ final class DifferentialTransaction extends PhabricatorApplicationTransaction {
         return pht(
           '%s added inline comments.',
           $author_handle);
-      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 self::TYPE_UPDATE:
         if ($this->getMetadataValue('isCommitUpdate')) {
           return pht(

src/applications/transactions/storage/PhabricatorApplicationTransaction.php

@@ -550,6 +550,8 @@ abstract class PhabricatorApplicationTransaction
             break;
         }
         break;
+     case PhabricatorTransactions::TYPE_INLINESTATE:
+       return true;
     }
 
     return $this->shouldHide();
@@ -944,6 +946,10 @@ abstract class PhabricatorApplicationTransaction
   }
 
   public function getActionStrength() {
+    if ($this->isInlineCommentTransaction()) {
+      return 0.25;
+    }
+
     switch ($this->getTransactionType()) {
       case PhabricatorTransactions::TYPE_COMMENT:
         return 0.5;