From 1c7443f8f242a1f6b9fb10b5b90178fabf2f1e42 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 27 Oct 2015 18:51:59 +0000 Subject: [PATCH] Make "Land Revision" button state consistent, prevent non-accepted lands Summary: Ref T182. Make the disabled state of the button more accurately reflect whether clicking it will work. Don't allow "land" to proceed unless the revision is accepted. Test Plan: Saw button in disabled state, clicked it, got "only accepted revisions" message. Reviewers: chad Reviewed By: chad Maniphest Tasks: T182 Differential Revision: https://secure.phabricator.com/D14350 --- ...ifferentialRevisionOperationController.php | 100 ++---------------- ...erentialLandingActionMenuEventListener.php | 20 +++- .../DrydockLandRepositoryOperation.php | 97 +++++++++++++++++ 3 files changed, 120 insertions(+), 97 deletions(-) diff --git a/src/applications/differential/controller/DifferentialRevisionOperationController.php b/src/applications/differential/controller/DifferentialRevisionOperationController.php index 85f2a0b1b2..a5d5b5f98f 100644 --- a/src/applications/differential/controller/DifferentialRevisionOperationController.php +++ b/src/applications/differential/controller/DifferentialRevisionOperationController.php @@ -18,86 +18,13 @@ final class DifferentialRevisionOperationController $detail_uri = "/D{$id}"; - $repository = $revision->getRepository(); - if (!$repository) { - return $this->rejectOperation( - $revision, - pht('No Repository'), - pht( - 'This revision is not associated with a known repository. Only '. - 'revisions associated with a tracked repository can be landed '. - 'automatically.')); - } - - if (!$repository->canPerformAutomation()) { - return $this->rejectOperation( - $revision, - pht('No Repository Automation'), - pht( - 'The repository this revision is associated with ("%s") is not '. - 'configured to support automation. Configure automation for the '. - 'repository to enable revisions to be landed automatically.', - $repository->getMonogram())); - } - - // TODO: At some point we should allow installs to give "land reviewed - // code" permission to more users than "push any commit", because it is - // a much less powerful operation. For now, just require push so this - // doesn't do anything users can't do on their own. - $can_push = PhabricatorPolicyFilter::hasCapability( - $viewer, - $repository, - DiffusionPushCapability::CAPABILITY); - if (!$can_push) { - return $this->rejectOperation( - $revision, - pht('Unable to Push'), - pht( - 'You do not have permission to push to the repository this '. - 'revision is associated with ("%s"), so you can not land it.', - $repository->getMonogram())); - } - $op = new DrydockLandRepositoryOperation(); - - // Check for other operations. Eventually this should probably be more - // general (e.g., it's OK to land to multiple different branches - // simultaneously) but just put this in as a sanity check for now. - $other_operations = id(new DrydockRepositoryOperationQuery()) - ->setViewer($viewer) - ->withObjectPHIDs(array($revision->getPHID())) - ->withOperationTypes( - array( - $op->getOperationConstant(), - )) - ->withOperationStates( - array( - DrydockRepositoryOperation::STATE_WAIT, - DrydockRepositoryOperation::STATE_WORK, - DrydockRepositoryOperation::STATE_DONE, - )) - ->execute(); - - if ($other_operations) { - $any_done = false; - foreach ($other_operations as $operation) { - if ($operation->isDone()) { - $any_done = true; - break; - } - } - - if ($any_done) { - return $this->rejectOperation( - $revision, - pht('Already Complete'), - pht('This revision has already landed.')); - } else { - return $this->rejectOperation( - $revision, - pht('Already In Flight'), - pht('This revision is already landing.')); - } + $barrier = $op->getBarrierToLanding($viewer, $revision); + if ($barrier) { + return $this->newDialog() + ->setTitle($barrier['title']) + ->appendParagraph($barrier['body']) + ->addCancelButton($detail_uri); } if ($request->isFormPost()) { @@ -106,6 +33,7 @@ final class DifferentialRevisionOperationController // occurs. $diff = $revision->getActiveDiff(); + $repository = $revision->getRepository(); $operation = DrydockRepositoryOperation::initializeNewOperation($op) ->setAuthorPHID($viewer->getPHID()) @@ -136,18 +64,4 @@ final class DifferentialRevisionOperationController ->addSubmitButton(pht('Mutate Repository Unpredictably')); } - private function rejectOperation( - DifferentialRevision $revision, - $title, - $body) { - - $id = $revision->getID(); - $detail_uri = "/D{$id}"; - - return $this->newDialog() - ->setTitle($title) - ->appendParagraph($body) - ->addCancelButton($detail_uri); - } - } diff --git a/src/applications/differential/landing/DifferentialLandingActionMenuEventListener.php b/src/applications/differential/landing/DifferentialLandingActionMenuEventListener.php index 7dcf3f462d..2cf3eaa4e5 100644 --- a/src/applications/differential/landing/DifferentialLandingActionMenuEventListener.php +++ b/src/applications/differential/landing/DifferentialLandingActionMenuEventListener.php @@ -26,7 +26,9 @@ final class DifferentialLandingActionMenuEventListener } private function renderRevisionAction(PhutilEvent $event) { - if (!$this->canUseApplication($event->getUser())) { + $viewer = $event->getUser(); + + if (!$this->canUseApplication($viewer)) { return null; } @@ -40,11 +42,22 @@ final class DifferentialLandingActionMenuEventListener if ($repository->canPerformAutomation()) { $revision_id = $revision->getID(); + $op = new DrydockLandRepositoryOperation(); + $barrier = $op->getBarrierToLanding($viewer, $revision); + + if ($barrier) { + $can_land = false; + } else { + $can_land = true; + } + $action = id(new PhabricatorActionView()) - ->setWorkflow(true) ->setName(pht('Land Revision')) ->setIcon('fa-fighter-jet') - ->setHref("/differential/revision/operation/{$revision_id}/"); + ->setHref("/differential/revision/operation/{$revision_id}/") + ->setWorkflow(true) + ->setDisabled(!$can_land); + $this->addActionMenuItems($event, $action); } @@ -54,7 +67,6 @@ final class DifferentialLandingActionMenuEventListener ->execute(); foreach ($strategies as $strategy) { - $viewer = $event->getUser(); $action = $strategy->createMenuItem($viewer, $revision, $repository); if ($action == null) { continue; diff --git a/src/applications/drydock/operation/DrydockLandRepositoryOperation.php b/src/applications/drydock/operation/DrydockLandRepositoryOperation.php index 00b5c71181..6da4c2ae5d 100644 --- a/src/applications/drydock/operation/DrydockLandRepositoryOperation.php +++ b/src/applications/drydock/operation/DrydockLandRepositoryOperation.php @@ -189,4 +189,101 @@ final class DrydockLandRepositoryOperation return $diff; } + public function getBarrierToLanding( + PhabricatorUser $viewer, + DifferentialRevision $revision) { + + $repository = $revision->getRepository(); + if (!$repository) { + return array( + 'title' => pht('No Repository'), + 'body' => pht( + 'This revision is not associated with a known repository. Only '. + 'revisions associated with a tracked repository can be landed '. + 'automatically.'), + ); + } + + if (!$repository->canPerformAutomation()) { + return array( + 'title' => pht('No Repository Automation'), + 'body' => pht( + 'The repository this revision is associated with ("%s") is not '. + 'configured to support automation. Configure automation for the '. + 'repository to enable revisions to be landed automatically.', + $repository->getMonogram()), + ); + } + + // TODO: At some point we should allow installs to give "land reviewed + // code" permission to more users than "push any commit", because it is + // a much less powerful operation. For now, just require push so this + // doesn't do anything users can't do on their own. + $can_push = PhabricatorPolicyFilter::hasCapability( + $viewer, + $repository, + DiffusionPushCapability::CAPABILITY); + if (!$can_push) { + return array( + 'title' => pht('Unable to Push'), + 'body' => pht( + 'You do not have permission to push to the repository this '. + 'revision is associated with ("%s"), so you can not land it.', + $repository->getMonogram()), + ); + } + + $status_accepted = ArcanistDifferentialRevisionStatus::ACCEPTED; + if ($revision->getStatus() !== $status_accepted) { + return array( + 'title' => pht('Revision Not Accepted'), + 'body' => pht( + 'This revision is still under review. Only revisions which have '. + 'been accepted may land.'), + ); + } + + // Check for other operations. Eventually this should probably be more + // general (e.g., it's OK to land to multiple different branches + // simultaneously) but just put this in as a sanity check for now. + $other_operations = id(new DrydockRepositoryOperationQuery()) + ->setViewer($viewer) + ->withObjectPHIDs(array($revision->getPHID())) + ->withOperationTypes( + array( + $this->getOperationConstant(), + )) + ->withOperationStates( + array( + DrydockRepositoryOperation::STATE_WAIT, + DrydockRepositoryOperation::STATE_WORK, + DrydockRepositoryOperation::STATE_DONE, + )) + ->execute(); + + if ($other_operations) { + $any_done = false; + foreach ($other_operations as $operation) { + if ($operation->isDone()) { + $any_done = true; + break; + } + } + + if ($any_done) { + return array( + 'title' => pht('Already Complete'), + 'body' => pht('This revision has already landed.'), + ); + } else { + return array( + 'title' => pht('Already In Flight'), + 'body' => pht('This revision is already landing.'), + ); + } + } + + return null; + } + }