diff --git a/resources/sql/autopatches/20140228.dxcomment.1.sql b/resources/sql/autopatches/20140228.dxcomment.1.sql new file mode 100644 index 0000000000..a62de29fc9 --- /dev/null +++ b/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; diff --git a/src/applications/differential/controller/DifferentialDiffViewController.php b/src/applications/differential/controller/DifferentialDiffViewController.php index b20c6cd8d0..4bfd373c2b 100644 --- a/src/applications/differential/controller/DifferentialDiffViewController.php +++ b/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(''); } diff --git a/src/applications/differential/controller/DifferentialRevisionEditControllerPro.php b/src/applications/differential/controller/DifferentialRevisionEditControllerPro.php index d998d754c3..41a6a50d4a 100644 --- a/src/applications/differential/controller/DifferentialRevisionEditControllerPro.php +++ b/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) diff --git a/src/applications/differential/controller/DifferentialRevisionViewController.php b/src/applications/differential/controller/DifferentialRevisionViewController.php index 803154db57..dc51fba4b5 100644 --- a/src/applications/differential/controller/DifferentialRevisionViewController.php +++ b/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; } diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index 3c7b3db887..c0208603c0 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/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(); + } } diff --git a/src/applications/differential/phid/DifferentialPHIDTypeDiff.php b/src/applications/differential/phid/DifferentialPHIDTypeDiff.php index f7ddcc7fe2..dda8b61b94 100644 --- a/src/applications/differential/phid/DifferentialPHIDTypeDiff.php +++ b/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}/"); } } diff --git a/src/applications/differential/storage/DifferentialTransaction.php b/src/applications/differential/storage/DifferentialTransaction.php index f2c50fb2d2..61269ccff4 100644 --- a/src/applications/differential/storage/DifferentialTransaction.php +++ b/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( - '%s updated this revision to Diff #%d.', - $author_handle, - $new); + if (phid_get_type($new) == 'DIFF') { + return pht( + '%s updated this revision to %s.', + $author_handle, + $this->renderHandleLink($new)); + } else { + return pht( + '%s updated this revision.', + $author_handle); + } } else { return pht( '%s updated this revision.',