Make "EditPro" controller work with diff updates

Summary:
Ref T2222. Make the "EditPro" controller accommodate diff updates, and support the transaction type. This one is pretty straightforward.

Also make `revisionPHID` in the comments table nullable to fix the "Edit" action.

Test Plan:
  - Created new revision.
  - Updated revision.
  - Tried to do some invalid stuff.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

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

resources/sql/autopatches/20140228.dxcomment.1.sql

@@ -0,0 +1,4 @@
+/* Make this column nullable. */
+
+ALTER TABLE {$NAMESPACE}_differential.differential_transaction_comment
+  CHANGE revisionPHID revisionPHID VARCHAR(64) COLLATE utf8_bin;

src/applications/differential/controller/DifferentialDiffViewController.php

@@ -58,7 +58,8 @@ final class DifferentialDiffViewController extends DifferentialController {
             array(
               'value' => $revision->getID(),
             ),
-            'D'.$revision->getID().' '.$revision->getTitle());
+            phutil_utf8_shorten(
+              'D'.$revision->getID().' '.$revision->getTitle(), 128));
         }
         $select[] = hsprintf('</optgroup>');
       }

src/applications/differential/controller/DifferentialRevisionEditControllerPro.php

@@ -23,6 +23,7 @@ final class DifferentialRevisionEditControllerPro
         ->withIDs(array($this->id))
         ->needRelationships(true)
         ->needReviewerStatus(true)
+        ->needActiveDiffs(true)
         ->requireCapabilities(
           array(
             PhabricatorPolicyCapability::CAN_VIEW,
@@ -34,6 +35,7 @@ final class DifferentialRevisionEditControllerPro
       }
     } else {
       $revision = DifferentialRevision::initializeNewRevision($viewer);
+      $revision->attachReviewerStatus(array());
     }
 
     $diff_id = $request->getInt('diffID');
@@ -53,6 +55,15 @@ final class DifferentialRevisionEditControllerPro
       $diff = null;
     }
 
+    if (!$diff) {
+      if (!$revision->getID()) {
+        throw new Exception(
+          pht('You can not create a new revision without a diff!'));
+      }
+    } else {
+      // TODO: It would be nice to show the diff being attached in the UI.
+    }
+
     $field_list = PhabricatorCustomField::getObjectFields(
       $revision,
       PhabricatorCustomField::ROLE_EDIT);
@@ -66,6 +77,21 @@ final class DifferentialRevisionEditControllerPro
         new DifferentialTransaction(),
         $request);
 
+      if ($diff) {
+        $xactions[] = id(new DifferentialTransaction())
+          ->setTransactionType(DifferentialTransaction::TYPE_UPDATE)
+          ->setNewValue($diff->getPHID());
+      }
+
+      $comments = $request->getStr('comments');
+      if (strlen($comments)) {
+        $xactions[] = id(new DifferentialTransaction())
+          ->setTransactionType(PhabricatorTransactions::TYPE_COMMENT)
+          ->attachComment(
+            id(new DifferentialTransactionComment())
+              ->setContent($comments));
+      }
+
       $editor = id(new DifferentialTransactionEditor())
         ->setActor($viewer)
         ->setContentSourceFromRequest($request)

src/applications/differential/controller/DifferentialRevisionViewController.php

@@ -881,11 +881,6 @@ final class DifferentialRevisionViewController extends DifferentialController {
       ->setRightDiff($right_diff)
       ->setTransactions($xactions);
 
-    // TODO: Make this work and restore edit links. We need to copy
-    // `revisionPHID` to the new version of the comment. This should be simple,
-    // but can happen post-merge. See T2222.
-    $timeline->setShowEditActions(false);
-
     return $timeline;
   }
 

src/applications/differential/editor/DifferentialTransactionEditor.php

@@ -14,11 +14,7 @@ final class DifferentialTransactionEditor
     $types[] = DifferentialTransaction::TYPE_ACTION;
     $types[] = DifferentialTransaction::TYPE_INLINE;
     $types[] = DifferentialTransaction::TYPE_STATUS;
-
-/*
-
     $types[] = DifferentialTransaction::TYPE_UPDATE;
-*/
 
     return $types;
   }
@@ -36,6 +32,12 @@ final class DifferentialTransactionEditor
         return null;
       case DifferentialTransaction::TYPE_INLINE:
         return null;
+      case DifferentialTransaction::TYPE_UPDATE:
+        if ($this->getIsNewObject()) {
+          return null;
+        } else {
+          return $object->getActiveDiff()->getPHID();
+        }
     }
 
     return parent::getCustomTransactionOldValue($object, $xaction);
@@ -49,6 +51,7 @@ final class DifferentialTransactionEditor
       case PhabricatorTransactions::TYPE_VIEW_POLICY:
       case PhabricatorTransactions::TYPE_EDIT_POLICY:
       case DifferentialTransaction::TYPE_ACTION:
+      case DifferentialTransaction::TYPE_UPDATE:
         return $xaction->getNewValue();
       case DifferentialTransaction::TYPE_INLINE:
         return null;
@@ -147,6 +150,9 @@ final class DifferentialTransactionEditor
         return;
       case PhabricatorTransactions::TYPE_EDGE:
         return;
+      case DifferentialTransaction::TYPE_UPDATE:
+        // TODO: Update the `diffPHID` once we add that.
+        return;
       case DifferentialTransaction::TYPE_ACTION:
         $status_review = ArcanistDifferentialRevisionStatus::NEEDS_REVIEW;
         $status_revision = ArcanistDifferentialRevisionStatus::NEEDS_REVISION;
@@ -336,6 +342,29 @@ final class DifferentialTransactionEditor
       case DifferentialTransaction::TYPE_ACTION:
       case DifferentialTransaction::TYPE_INLINE:
         return;
+      case DifferentialTransaction::TYPE_UPDATE:
+        // Now that we're inside the transaction, do a final check.
+        $diff = $this->loadDiff($xaction->getNewValue());
+
+        // TODO: It would be slightly cleaner to just revalidate this
+        // transaction somehow using the same validation code, but that's
+        // not easy to do at the moment.
+
+        if (!$diff) {
+          throw new Exception(pht('Diff does not exist!'));
+        } else {
+          $revision_id = $diff->getRevisionID();
+          if ($revision_id && ($revision_id != $object->getID())) {
+            throw new Exception(
+              pht(
+                'Diff is already attached to another revision. You lost '.
+                'a race?'));
+          }
+        }
+
+        $diff->setRevisionID($object->getID());
+        $diff->save();
+        return;
     }
 
     return parent::applyCustomExternalTransaction($object, $xaction);
@@ -471,6 +500,25 @@ final class DifferentialTransactionEditor
 
     foreach ($xactions as $xaction) {
       switch ($type) {
+        case DifferentialTransaction::TYPE_UPDATE:
+          $diff = $this->loadDiff($xaction->getNewValue());
+          if (!$diff) {
+            $errors[] = new PhabricatorApplicationTransactionValidationError(
+              $type,
+              pht('Invalid'),
+              pht('The specified diff does not exist.'),
+              $xaction);
+          } else if (($diff->getRevisionID()) &&
+            ($diff->getRevisionID() != $object->getID())) {
+            $errors[] = new PhabricatorApplicationTransactionValidationError(
+              $type,
+              pht('Invalid'),
+              pht(
+                'You can not update this revision to the specified diff, '.
+                'because the diff is already attached to another revision.'),
+              $xaction);
+          }
+          break;
         case DifferentialTransaction::TYPE_ACTION:
           $error = $this->validateDifferentialAction(
             $object,
@@ -966,6 +1014,12 @@ final class DifferentialTransactionEditor
     return implode("\n", $result);
   }
 
+  private function loadDiff($phid) {
+    return id(new DifferentialDiffQuery())
+      ->withPHIDs(array($phid))
+      ->setViewer($this->getActor())
+      ->executeOne();
+  }
 
 
 }

src/applications/differential/phid/DifferentialPHIDTypeDiff.php

@@ -35,6 +35,7 @@ final class DifferentialPHIDTypeDiff extends PhabricatorPHIDType {
       $id = $diff->getID();
 
       $handle->setName(pht('Diff %d', $id));
+      $handle->setURI("/differential/diff/{$id}/");
     }
   }
 

src/applications/differential/storage/DifferentialTransaction.php

@@ -70,6 +70,22 @@ final class DifferentialTransaction extends PhabricatorApplicationTransaction {
     return parent::getBodyForMail();
   }
 
+  public function getRequiredHandlePHIDs() {
+    $phids = parent::getRequiredHandlePHIDs();
+
+    $old = $this->getOldValue();
+    $new = $this->getNewValue();
+
+    switch ($this->getTransactionType()) {
+      case self::TYPE_UPDATE:
+        if ($new) {
+          $phids[] = $new;
+        }
+        break;
+    }
+
+    return $phids;
+  }
 
   public function getTitle() {
     $author_phid = $this->getAuthorPHID();
@@ -87,10 +103,16 @@ final class DifferentialTransaction extends PhabricatorApplicationTransaction {
         if ($new) {
           // TODO: Migrate to PHIDs and use handles here?
           // TODO: Link this?
-          return pht(
+          if (phid_get_type($new) == 'DIFF') {
-            '%s updated this revision to Diff #%d.',
+            return pht(
-            $author_handle,
+              '%s updated this revision to %s.',
-            $new);
+              $author_handle,
+              $this->renderHandleLink($new));
+          } else {
+            return pht(
+              '%s updated this revision.',
+              $author_handle);
+          }
         } else {
           return pht(
             '%s updated this revision.',