Index: src/applications/conpherence/editor/ConpherenceEditor.php =================================================================== --- src/applications/conpherence/editor/ConpherenceEditor.php +++ src/applications/conpherence/editor/ConpherenceEditor.php @@ -304,6 +304,8 @@ } $participant->save(); } + + return $xactions; } protected function mergeTransactions( Index: src/applications/differential/editor/DifferentialTransactionEditor.php =================================================================== --- src/applications/differential/editor/DifferentialTransactionEditor.php +++ src/applications/differential/editor/DifferentialTransactionEditor.php @@ -13,6 +13,7 @@ $types[] = DifferentialTransaction::TYPE_ACTION; $types[] = DifferentialTransaction::TYPE_INLINE; + $types[] = DifferentialTransaction::TYPE_STATUS; /* @@ -145,7 +146,6 @@ case DifferentialTransaction::TYPE_INLINE: return; case PhabricatorTransactions::TYPE_EDGE: - // TODO: Update review status. return; case DifferentialTransaction::TYPE_ACTION: $status_review = ArcanistDifferentialRevisionStatus::NEEDS_REVIEW; @@ -166,15 +166,12 @@ return; case DifferentialAction::ACTION_RECLAIM: $object->setStatus($status_review); - // TODO: Update review status? return; case DifferentialAction::ACTION_REOPEN: $object->setStatus($status_review); - // TODO: Update review status? return; case DifferentialAction::ACTION_REQUEST: $object->setStatus($status_review); - // TODO: Update review status? return; case DifferentialAction::ACTION_CLOSE: $object->setStatus(ArcanistDifferentialRevisionStatus::CLOSED); @@ -315,6 +312,98 @@ return parent::applyCustomExternalTransaction($object, $xaction); } + protected function applyFinalEffects( + PhabricatorLiskDAO $object, + array $xactions) { + + $status_accepted = ArcanistDifferentialRevisionStatus::ACCEPTED; + $status_revision = ArcanistDifferentialRevisionStatus::NEEDS_REVISION; + $status_review = ArcanistDifferentialRevisionStatus::NEEDS_REVIEW; + + $old_status = $object->getStatus(); + switch ($old_status) { + case $status_accepted: + case $status_revision: + case $status_review: + // Load the most up-to-date version of the revision and its reviewers, + // so we don't need to try to deduce the state of reviewers by examining + // all the changes made by the transactions. + $new_revision = id(new DifferentialRevisionQuery()) + ->setViewer($this->getActor()) + ->needReviewerStatus(true) + ->withIDs(array($object->getID())) + ->executeOne(); + if (!$new_revision) { + throw new Exception( + pht('Failed to load revision from transaction finalization.')); + } + + // Try to move a revision to "accepted". We look for: + // + // - at least one accepting reviewer who is a user; and + // - no rejects; and + // - no blocking reviewers. + + $has_accepting_user = false; + $has_rejecting_reviewer = false; + $has_blocking_reviewer = false; + foreach ($new_revision->getReviewerStatus() as $reviewer) { + $reviewer_status = $reviewer->getStatus(); + switch ($reviewer_status) { + case DifferentialReviewerStatus::STATUS_REJECTED: + $has_rejecting_reviewer = true; + break; + case DifferentialReviewerStatus::STATUS_BLOCKING: + $has_blocking_reviewer = true; + break; + case DifferentialReviewerStatus::STATUS_ACCEPTED: + if ($reviewer->isUser()) { + $has_accepting_user = true; + } + break; + } + } + + $new_status = null; + if ($has_accepting_user && + !$has_rejecting_reviewer && + !$has_blocking_reviewer) { + $new_status = $status_accepted; + } else if ($has_rejecting_reviewer) { + // This isn't accepted, and there's at least one rejecting reviewer, + // so the revision needs changes. This usually happens after a + // "reject". + $new_status = $status_revision; + } else if ($old_status == $status_accepted) { + // This revision was accepted, but it no longer satisfies the + // conditions for acceptance. This usually happens after an accepting + // reviewer resigns or is removed. + $new_status = $status_review; + } + + if ($new_status !== null && $new_status != $old_status) { + $xaction = id(new DifferentialTransaction()) + ->setTransactionType(DifferentialTransaction::TYPE_STATUS) + ->setOldValue($old_status) + ->setNewValue($new_status); + + $xaction = $this->populateTransaction($object, $xaction)->save(); + + $xactions[] = $xaction; + + $object->setStatus($new_status)->save(); + } + break; + default: + // Revisions can't transition out of other statuses (like closed or + // abandoned) as a side effect of reviewer status changes. + break; + } + + return $xactions; + } + + protected function validateTransaction( PhabricatorLiskDAO $object, $type, Index: src/applications/differential/storage/DifferentialTransaction.php =================================================================== --- src/applications/differential/storage/DifferentialTransaction.php +++ src/applications/differential/storage/DifferentialTransaction.php @@ -5,6 +5,7 @@ const TYPE_INLINE = 'differential:inline'; const TYPE_UPDATE = 'differential:update'; const TYPE_ACTION = 'differential:action'; + const TYPE_STATUS = 'differential:status'; public function getApplicationName() { return 'differential'; @@ -18,6 +19,28 @@ return new DifferentialTransactionComment(); } + public function shouldHide() { + switch ($this->getTransactionType()) { + case PhabricatorTransactions::TYPE_EDGE: + $old = $this->getOldValue(); + $new = $this->getNewValue(); + $add = array_diff_key($new, $old); + $rem = array_diff_key($old, $new); + + // Hide metadata-only edge transactions. These correspond to users + // accepting or rejecting revisions, but the change is always explicit + // because of the TYPE_ACTION transaction. Rendering these transactions + // just creates clutter. + + if (!$add && !$rem) { + return true; + } + break; + } + + return false; + } + public function getTitle() { $author_phid = $this->getAuthorPHID(); $author_handle = $this->renderHandleLink($author_phid); @@ -45,6 +68,18 @@ } case self::TYPE_ACTION: return DifferentialAction::getBasicStoryText($new, $author_handle); + case self::TYPE_STATUS: + switch ($this->getNewValue()) { + case ArcanistDifferentialRevisionStatus::ACCEPTED: + return pht( + 'This revision is now accepted and ready to land.'); + case ArcanistDifferentialRevisionStatus::NEEDS_REVISION: + return pht( + 'This revision now requires changes to proceed.'); + case ArcanistDifferentialRevisionStatus::NEEDS_REVIEW: + return pht( + 'This revision now requires review to proceed.'); + } } return parent::getTitle(); @@ -56,6 +91,16 @@ return 'comment'; case self::TYPE_UPDATE: return 'refresh'; + case self::TYPE_STATUS: + switch ($this->getNewValue()) { + case ArcanistDifferentialRevisionStatus::ACCEPTED: + return 'enable'; + case ArcanistDifferentialRevisionStatus::NEEDS_REVISION: + return 'delete'; + case ArcanistDifferentialRevisionStatus::NEEDS_REVIEW: + return 'refresh'; + } + break; case self::TYPE_ACTION: switch ($this->getNewValue()) { case DifferentialAction::ACTION_CLOSE: @@ -82,10 +127,41 @@ return parent::getIcon(); } + public function shouldDisplayGroupWith(array $group) { + + // Never group status changes with other types of actions, they're indirect + // and don't make sense when combined with direct actions. + + $type_status = self::TYPE_STATUS; + + if ($this->getTransactionType() == $type_status) { + return false; + } + + foreach ($group as $xaction) { + if ($xaction->getTransactionType() == $type_status) { + return false; + } + } + + return parent::shouldDisplayGroupWith($group); + } + + public function getColor() { switch ($this->getTransactionType()) { case self::TYPE_UPDATE: return PhabricatorTransactions::COLOR_SKY; + case self::TYPE_STATUS: + switch ($this->getNewValue()) { + case ArcanistDifferentialRevisionStatus::ACCEPTED: + return PhabricatorTransactions::COLOR_GREEN; + case ArcanistDifferentialRevisionStatus::NEEDS_REVISION: + return PhabricatorTransactions::COLOR_RED; + case ArcanistDifferentialRevisionStatus::NEEDS_REVIEW: + return PhabricatorTransactions::COLOR_ORANGE; + } + break; case self::TYPE_ACTION: switch ($this->getNewValue()) { case DifferentialAction::ACTION_CLOSE: Index: src/applications/legalpad/editor/LegalpadDocumentEditor.php =================================================================== --- src/applications/legalpad/editor/LegalpadDocumentEditor.php +++ src/applications/legalpad/editor/LegalpadDocumentEditor.php @@ -104,6 +104,8 @@ $object->save(); } + + return $xactions; } protected function mergeTransactions( Index: src/applications/paste/editor/PhabricatorPasteEditor.php =================================================================== --- src/applications/paste/editor/PhabricatorPasteEditor.php +++ src/applications/paste/editor/PhabricatorPasteEditor.php @@ -113,6 +113,8 @@ $this->getActor(), $object->getPHID()); } + + return $xactions; } Index: src/applications/pholio/editor/PholioMockEditor.php =================================================================== --- src/applications/pholio/editor/PholioMockEditor.php +++ src/applications/pholio/editor/PholioMockEditor.php @@ -275,6 +275,8 @@ $image->setMockID($object->getID()); $image->save(); } + + return $xactions; } protected function mergeTransactions( Index: src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php =================================================================== --- src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -377,9 +377,36 @@ "implementation!"); } + /** + * Fill in a transaction's common values, like author and content source. + */ + protected function populateTransaction( + PhabricatorLiskDAO $object, + PhabricatorApplicationTransaction $xaction) { + + $actor = $this->getActor(); + + // TODO: This needs to be more sophisticated once we have meta-policies. + $xaction->setViewPolicy(PhabricatorPolicies::POLICY_PUBLIC); + $xaction->setEditPolicy($actor->getPHID()); + + $xaction->setAuthorPHID($actor->getPHID()); + $xaction->setContentSource($this->getContentSource()); + $xaction->attachViewer($actor); + $xaction->attachObject($object); + + if ($object->getPHID()) { + $xaction->setObjectPHID($object->getPHID()); + } + + return $xaction; + } + + protected function applyFinalEffects( PhabricatorLiskDAO $object, array $xactions) { + return $xactions; } public function setContentSource(PhabricatorContentSource $content_source) { @@ -421,14 +448,7 @@ $xactions = $this->combineTransactions($xactions); foreach ($xactions as $xaction) { - // TODO: This needs to be more sophisticated once we have meta-policies. - $xaction->setViewPolicy(PhabricatorPolicies::POLICY_PUBLIC); - $xaction->setEditPolicy($actor->getPHID()); - - $xaction->setAuthorPHID($actor->getPHID()); - $xaction->setContentSource($this->getContentSource()); - $xaction->attachViewer($this->getActor()); - $xaction->attachObject($object); + $xaction = $this->populateTransaction($object, $xaction); } $is_preview = $this->getIsPreview(); @@ -557,7 +577,7 @@ $this->applyHeraldRules($object, $xactions); } - $this->applyFinalEffects($object, $xactions); + $xactions = $this->applyFinalEffects($object, $xactions); if ($read_locking) { $object->endReadLocking();