From a69cca9fbb15c749ad75b400943cf1e324c13f3a Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 25 Feb 2014 12:36:49 -0800 Subject: [PATCH] Update overall revision status after reviewers change Summary: Ref T2222. This doesn't feel super clean, but doesn't feel too bad either. Basically, Differential transactions can have secondary state-based effects (changing the overall revision status) when reviewers resign, are removed, accept, or reject revisions. To deal with this in ApplicationTransactions, I did this: - `applyFinalEffects()` can now alter the transaction set (notably, add new ones). This mostly matters for email, notifications and feed. - In Differential, check for an overall revision state transition in `applyFinalEffects()` (e.g., your reject moving the revision to a rejected state). - I'm only writing the transaction if the transition is implied and indirect. - For example, if you "Plan Changes", that action changes the state on its own so there's no implicit state change transaction added. The transactions themselves are kind of fluff, but it seems useful to keep a record of when state changes occurred in the transaction log. If people complain we can hide/remove them. Test Plan: {F118143} Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T2222 Differential Revision: https://secure.phabricator.com/D8339 --- .../conpherence/editor/ConpherenceEditor.php | 2 + .../editor/DifferentialTransactionEditor.php | 97 ++++++++++++++++++- .../storage/DifferentialTransaction.php | 76 +++++++++++++++ .../editor/LegalpadDocumentEditor.php | 2 + .../paste/editor/PhabricatorPasteEditor.php | 2 + .../pholio/editor/PholioMockEditor.php | 2 + ...habricatorApplicationTransactionEditor.php | 38 ++++++-- 7 files changed, 206 insertions(+), 13 deletions(-) diff --git a/src/applications/conpherence/editor/ConpherenceEditor.php b/src/applications/conpherence/editor/ConpherenceEditor.php index 8c8737edaf..06f1537d87 100644 --- a/src/applications/conpherence/editor/ConpherenceEditor.php +++ b/src/applications/conpherence/editor/ConpherenceEditor.php @@ -304,6 +304,8 @@ final class ConpherenceEditor extends PhabricatorApplicationTransactionEditor { } $participant->save(); } + + return $xactions; } protected function mergeTransactions( diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index 1dc1985b0c..b6c5bdc902 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -13,6 +13,7 @@ final class DifferentialTransactionEditor $types[] = DifferentialTransaction::TYPE_ACTION; $types[] = DifferentialTransaction::TYPE_INLINE; + $types[] = DifferentialTransaction::TYPE_STATUS; /* @@ -145,7 +146,6 @@ final class DifferentialTransactionEditor 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 @@ final class DifferentialTransactionEditor 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 @@ final class DifferentialTransactionEditor 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, diff --git a/src/applications/differential/storage/DifferentialTransaction.php b/src/applications/differential/storage/DifferentialTransaction.php index 14848d01ef..cd82ab1737 100644 --- a/src/applications/differential/storage/DifferentialTransaction.php +++ b/src/applications/differential/storage/DifferentialTransaction.php @@ -5,6 +5,7 @@ final class DifferentialTransaction extends PhabricatorApplicationTransaction { 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 @@ final class DifferentialTransaction extends PhabricatorApplicationTransaction { 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 @@ final class DifferentialTransaction extends PhabricatorApplicationTransaction { } 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 @@ final class DifferentialTransaction extends PhabricatorApplicationTransaction { 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 @@ final class DifferentialTransaction extends PhabricatorApplicationTransaction { 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: diff --git a/src/applications/legalpad/editor/LegalpadDocumentEditor.php b/src/applications/legalpad/editor/LegalpadDocumentEditor.php index fc86acf048..9a5c9e97bb 100644 --- a/src/applications/legalpad/editor/LegalpadDocumentEditor.php +++ b/src/applications/legalpad/editor/LegalpadDocumentEditor.php @@ -104,6 +104,8 @@ final class LegalpadDocumentEditor $object->save(); } + + return $xactions; } protected function mergeTransactions( diff --git a/src/applications/paste/editor/PhabricatorPasteEditor.php b/src/applications/paste/editor/PhabricatorPasteEditor.php index cb3d8a5e71..4ae98a7cf0 100644 --- a/src/applications/paste/editor/PhabricatorPasteEditor.php +++ b/src/applications/paste/editor/PhabricatorPasteEditor.php @@ -113,6 +113,8 @@ final class PhabricatorPasteEditor $this->getActor(), $object->getPHID()); } + + return $xactions; } diff --git a/src/applications/pholio/editor/PholioMockEditor.php b/src/applications/pholio/editor/PholioMockEditor.php index a55cd62595..3597c286cb 100644 --- a/src/applications/pholio/editor/PholioMockEditor.php +++ b/src/applications/pholio/editor/PholioMockEditor.php @@ -275,6 +275,8 @@ final class PholioMockEditor extends PhabricatorApplicationTransactionEditor { $image->setMockID($object->getID()); $image->save(); } + + return $xactions; } protected function mergeTransactions( diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php index 73dbc2e145..cfd3ffd6d4 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -377,9 +377,36 @@ abstract class PhabricatorApplicationTransactionEditor "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 @@ abstract class PhabricatorApplicationTransactionEditor $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 @@ abstract class PhabricatorApplicationTransactionEditor $this->applyHeraldRules($object, $xactions); } - $this->applyFinalEffects($object, $xactions); + $xactions = $this->applyFinalEffects($object, $xactions); if ($read_locking) { $object->endReadLocking();