From 4ef87eeac832416fb850401d54ea5b410213c8d4 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 5 Mar 2014 10:47:47 -0800 Subject: [PATCH] Add an explcit "Changes Planned" state for Differential Summary: Ref T2222. Ref T4481. This fixes the issue where "Plan Changes" could immediately trigger a state change (e.g., back to accepted) because of state-based transitions out of the NEEDS_REVISION state. Test Plan: Planned changes an "accepted" revision, it didn't immediately return to being accepted. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T2222, T4481 Differential Revision: https://secure.phabricator.com/D8403 --- .../constants/DifferentialRevisionStatus.php | 10 ++++++++++ .../DifferentialRevisionViewController.php | 2 ++ .../editor/DifferentialRevisionEditor.php | 1 + .../editor/DifferentialTransactionEditor.php | 14 ++++++++++---- .../view/DifferentialRevisionListView.php | 1 + 5 files changed, 24 insertions(+), 4 deletions(-) diff --git a/src/applications/differential/constants/DifferentialRevisionStatus.php b/src/applications/differential/constants/DifferentialRevisionStatus.php index a35e00e9cc..64a330dc7e 100644 --- a/src/applications/differential/constants/DifferentialRevisionStatus.php +++ b/src/applications/differential/constants/DifferentialRevisionStatus.php @@ -21,12 +21,16 @@ final class DifferentialRevisionStatus { self::COLOR_STATUS_DEFAULT, ArcanistDifferentialRevisionStatus::NEEDS_REVISION => self::COLOR_STATUS_RED, + ArcanistDifferentialRevisionStatus::CHANGES_PLANNED => + self::COLOR_STATUS_RED, ArcanistDifferentialRevisionStatus::ACCEPTED => self::COLOR_STATUS_GREEN, ArcanistDifferentialRevisionStatus::CLOSED => self::COLOR_STATUS_DARK, ArcanistDifferentialRevisionStatus::ABANDONED => self::COLOR_STATUS_DARK, + ArcanistDifferentialRevisionStatus::IN_PREPARATION => + self::COLOR_STATUS_DARK, ); return idx($map, $status, $default); } @@ -39,12 +43,16 @@ final class DifferentialRevisionStatus { 'oh-open', ArcanistDifferentialRevisionStatus::NEEDS_REVISION => 'oh-open-red', + ArcanistDifferentialRevisionStatus::CHANGES_PLANNED => + 'oh-open-red', ArcanistDifferentialRevisionStatus::ACCEPTED => 'oh-open-green', ArcanistDifferentialRevisionStatus::CLOSED => 'oh-closed-dark', ArcanistDifferentialRevisionStatus::ABANDONED => 'oh-closed-dark', + ArcanistDifferentialRevisionStatus::IN_PREPARATION => + 'question-blue', ); return idx($map, $status, $default); } @@ -92,9 +100,11 @@ final class DifferentialRevisionStatus { return array( ArcanistDifferentialRevisionStatus::NEEDS_REVIEW, ArcanistDifferentialRevisionStatus::NEEDS_REVISION, + ArcanistDifferentialRevisionStatus::CHANGES_PLANNED, ArcanistDifferentialRevisionStatus::ACCEPTED, ArcanistDifferentialRevisionStatus::CLOSED, ArcanistDifferentialRevisionStatus::ABANDONED, + ArcanistDifferentialRevisionStatus::IN_PREPARATION, ); } diff --git a/src/applications/differential/controller/DifferentialRevisionViewController.php b/src/applications/differential/controller/DifferentialRevisionViewController.php index dc51fba4b5..9b919d1244 100644 --- a/src/applications/differential/controller/DifferentialRevisionViewController.php +++ b/src/applications/differential/controller/DifferentialRevisionViewController.php @@ -523,6 +523,7 @@ final class DifferentialRevisionViewController extends DifferentialController { $actions[DifferentialAction::ACTION_RETHINK] = true; break; case ArcanistDifferentialRevisionStatus::NEEDS_REVISION: + case ArcanistDifferentialRevisionStatus::CHANGES_PLANNED: $actions[DifferentialAction::ACTION_ACCEPT] = $allow_self_accept; $actions[DifferentialAction::ACTION_ABANDON] = true; $actions[DifferentialAction::ACTION_REQUEST] = true; @@ -547,6 +548,7 @@ final class DifferentialRevisionViewController extends DifferentialController { $actions[DifferentialAction::ACTION_RESIGN] = $viewer_is_reviewer; break; case ArcanistDifferentialRevisionStatus::NEEDS_REVISION: + case ArcanistDifferentialRevisionStatus::CHANGES_PLANNED: $actions[DifferentialAction::ACTION_ACCEPT] = true; $actions[DifferentialAction::ACTION_REJECT] = !$viewer_has_rejected; $actions[DifferentialAction::ACTION_RESIGN] = $viewer_is_reviewer; diff --git a/src/applications/differential/editor/DifferentialRevisionEditor.php b/src/applications/differential/editor/DifferentialRevisionEditor.php index cd38700b9a..421208ced7 100644 --- a/src/applications/differential/editor/DifferentialRevisionEditor.php +++ b/src/applications/differential/editor/DifferentialRevisionEditor.php @@ -437,6 +437,7 @@ final class DifferentialRevisionEditor extends PhabricatorEditor { // be able to put the revision into "accepted". switch ($revision->getStatus()) { case ArcanistDifferentialRevisionStatus::NEEDS_REVISION: + case ArcanistDifferentialRevisionStatus::CHANGES_PLANNED: case ArcanistDifferentialRevisionStatus::NEEDS_REVIEW: $revision = self::updateAcceptedStatus( $this->getActor(), diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index b4fdddbffc..c8ab780348 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -72,6 +72,7 @@ final class DifferentialTransactionEditor $status_abandoned = ArcanistDifferentialRevisionStatus::ABANDONED; $status_review = ArcanistDifferentialRevisionStatus::NEEDS_REVIEW; $status_revision = ArcanistDifferentialRevisionStatus::NEEDS_REVISION; + $status_plan = ArcanistDifferentialRevisionStatus::CHANGES_PLANNED; $action_type = $xaction->getNewValue(); switch ($action_type) { @@ -86,7 +87,7 @@ final class DifferentialTransactionEditor $actor = $this->getActor(); $actor_phid = $actor->getPHID(); - // These transactions can cause effects in to ways: by altering the + // These transactions can cause effects in two ways: by altering the // status of an existing reviewer; or by adding the actor as a new // reviewer. @@ -112,7 +113,7 @@ final class DifferentialTransactionEditor case DifferentialAction::ACTION_REOPEN: return ($object->getStatus() == $status_closed); case DifferentialAction::ACTION_RETHINK: - return ($object->getStatus() != $status_revision); + return ($object->getStatus() != $status_plan); case DifferentialAction::ACTION_REQUEST: return ($object->getStatus() != $status_review); case DifferentialAction::ACTION_RESIGN: @@ -139,6 +140,7 @@ final class DifferentialTransactionEditor $status_review = ArcanistDifferentialRevisionStatus::NEEDS_REVIEW; $status_revision = ArcanistDifferentialRevisionStatus::NEEDS_REVISION; + $status_plan = ArcanistDifferentialRevisionStatus::CHANGES_PLANNED; switch ($xaction->getTransactionType()) { case PhabricatorTransactions::TYPE_VIEW_POLICY: @@ -169,7 +171,7 @@ final class DifferentialTransactionEditor $object->setStatus(ArcanistDifferentialRevisionStatus::ABANDONED); return; case DifferentialAction::ACTION_RETHINK: - $object->setStatus($status_revision); + $object->setStatus($status_plan); return; case DifferentialAction::ACTION_RECLAIM: $object->setStatus($status_review); @@ -714,7 +716,7 @@ final class DifferentialTransactionEditor if (!$actor_is_author) { return pht( "You can not plan changes to this revision because you do not ". - "own it. To plan chagnes to a revision, you must be its owner."); + "own it. To plan changes to a revision, you must be its owner."); } switch ($revision_status) { @@ -723,6 +725,9 @@ final class DifferentialTransactionEditor case ArcanistDifferentialRevisionStatus::NEEDS_REVIEW: // These are OK. break; + case ArcanistDifferentialRevisionStatus::CHANGES_PLANNED: + // Let this through, it's a no-op. + break; case ArcanistDifferentialRevisionStatus::ABANDONED: return pht( "You can not plan changes to this revision because it has ". @@ -752,6 +757,7 @@ final class DifferentialTransactionEditor switch ($revision_status) { case ArcanistDifferentialRevisionStatus::ACCEPTED: case ArcanistDifferentialRevisionStatus::NEEDS_REVISION: + case ArcanistDifferentialRevisionStatus::CHANGES_PLANNED: // These are OK. break; case ArcanistDifferentialRevisionStatus::NEEDS_REVIEW: diff --git a/src/applications/differential/view/DifferentialRevisionListView.php b/src/applications/differential/view/DifferentialRevisionListView.php index 52565bb5ff..ad7f32f303 100644 --- a/src/applications/differential/view/DifferentialRevisionListView.php +++ b/src/applications/differential/view/DifferentialRevisionListView.php @@ -182,6 +182,7 @@ final class DifferentialRevisionListView extends AphrontView { case ArcanistDifferentialRevisionStatus::NEEDS_REVIEW: break; case ArcanistDifferentialRevisionStatus::NEEDS_REVISION: + case ArcanistDifferentialRevisionStatus::CHANGES_PLANNED: $item->setBarColor('red'); break; case ArcanistDifferentialRevisionStatus::ACCEPTED: