From 0e7a5623e32c4e6644b28ba690081dd27d87cd1d Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 2 Mar 2017 12:46:17 -0800 Subject: [PATCH] Allow task statuses to "lock" them, preventing additional comments and interactions Summary: Ref T12335. See that task for discussion. Here are the behavioral changes: - Statuses can be flagged with `locked`, which means that tasks in that status are locked to further discussion and interaction. - A new "CAN_INTERACT" permission facilitates this. For most objects, "CAN_INTERACT" is just the same as "CAN_VIEW". - For tasks, "CAN_INTERACT" is everyone if the status is a normal status, and no one if the status is a locked status. - If a user doesn't have "Interact" permission: - They can not submit the comment form. - The comment form is replaced with text indicating "This thing is locked.". - The "Edit" workflow prompts them. This is a mixture of advisory and hard policy checks but sholuld represent a reasonable starting point. Test Plan: Created a new "Locked" status, locked a task. Couldn't comment, saw lock warning, saw lock prompt on edit. Unlocked a task. Reviewers: chad Reviewed By: chad Maniphest Tasks: T12335 Differential Revision: https://secure.phabricator.com/D17453 --- src/__phutil_library_map__.php | 8 +++ .../PhabricatorManiphestConfigOptions.php | 2 + .../constants/ManiphestTaskStatus.php | 5 ++ .../ManiphestTaskDetailController.php | 8 ++- .../editor/ManiphestTaskEditEngineLock.php | 28 ++++++++ .../maniphest/storage/ManiphestTask.php | 22 +++++- .../PhabricatorPolicyCapability.php | 1 + .../policy/filter/PhabricatorPolicyFilter.php | 30 ++++++++ ...PhabricatorSubscriptionsEditController.php | 9 +++ ...habricatorSubscriptionsUIEventListener.php | 8 ++- .../editengine/PhabricatorEditEngine.php | 69 +++++++++++++++---- .../PhabricatorEditEngineDefaultLock.php | 4 ++ .../editengine/PhabricatorEditEngineLock.php | 66 ++++++++++++++++++ ...PhabricatorEditEngineLockableInterface.php | 7 ++ ...catorApplicationTransactionCommentView.php | 22 +++++- 15 files changed, 272 insertions(+), 17 deletions(-) create mode 100644 src/applications/maniphest/editor/ManiphestTaskEditEngineLock.php create mode 100644 src/applications/transactions/editengine/PhabricatorEditEngineDefaultLock.php create mode 100644 src/applications/transactions/editengine/PhabricatorEditEngineLock.php create mode 100644 src/applications/transactions/editengine/PhabricatorEditEngineLockableInterface.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 23c923dc25..4ade1636d7 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1487,6 +1487,7 @@ phutil_register_library_map(array( 'ManiphestTaskDetailController' => 'applications/maniphest/controller/ManiphestTaskDetailController.php', 'ManiphestTaskEditBulkJobType' => 'applications/maniphest/bulk/ManiphestTaskEditBulkJobType.php', 'ManiphestTaskEditController' => 'applications/maniphest/controller/ManiphestTaskEditController.php', + 'ManiphestTaskEditEngineLock' => 'applications/maniphest/editor/ManiphestTaskEditEngineLock.php', 'ManiphestTaskFulltextEngine' => 'applications/maniphest/search/ManiphestTaskFulltextEngine.php', 'ManiphestTaskGraph' => 'infrastructure/graph/ManiphestTaskGraph.php', 'ManiphestTaskHasCommitEdgeType' => 'applications/maniphest/edge/ManiphestTaskHasCommitEdgeType.php', @@ -2617,9 +2618,12 @@ phutil_register_library_map(array( 'PhabricatorEditEngineConfigurationViewController' => 'applications/transactions/controller/PhabricatorEditEngineConfigurationViewController.php', 'PhabricatorEditEngineController' => 'applications/transactions/controller/PhabricatorEditEngineController.php', 'PhabricatorEditEngineDatasource' => 'applications/transactions/typeahead/PhabricatorEditEngineDatasource.php', + 'PhabricatorEditEngineDefaultLock' => 'applications/transactions/editengine/PhabricatorEditEngineDefaultLock.php', 'PhabricatorEditEngineExtension' => 'applications/transactions/engineextension/PhabricatorEditEngineExtension.php', 'PhabricatorEditEngineExtensionModule' => 'applications/transactions/engineextension/PhabricatorEditEngineExtensionModule.php', 'PhabricatorEditEngineListController' => 'applications/transactions/controller/PhabricatorEditEngineListController.php', + 'PhabricatorEditEngineLock' => 'applications/transactions/editengine/PhabricatorEditEngineLock.php', + 'PhabricatorEditEngineLockableInterface' => 'applications/transactions/editengine/PhabricatorEditEngineLockableInterface.php', 'PhabricatorEditEnginePointsCommentAction' => 'applications/transactions/commentaction/PhabricatorEditEnginePointsCommentAction.php', 'PhabricatorEditEngineProfileMenuItem' => 'applications/search/menuitem/PhabricatorEditEngineProfileMenuItem.php', 'PhabricatorEditEngineQuery' => 'applications/transactions/query/PhabricatorEditEngineQuery.php', @@ -6372,6 +6376,7 @@ phutil_register_library_map(array( 'PhabricatorFulltextInterface', 'DoorkeeperBridgedObjectInterface', 'PhabricatorEditEngineSubtypeInterface', + 'PhabricatorEditEngineLockableInterface', ), 'ManiphestTaskAssignHeraldAction' => 'HeraldAction', 'ManiphestTaskAssignOtherHeraldAction' => 'ManiphestTaskAssignHeraldAction', @@ -6387,6 +6392,7 @@ phutil_register_library_map(array( 'ManiphestTaskDetailController' => 'ManiphestController', 'ManiphestTaskEditBulkJobType' => 'PhabricatorWorkerBulkJobType', 'ManiphestTaskEditController' => 'ManiphestController', + 'ManiphestTaskEditEngineLock' => 'PhabricatorEditEngineLock', 'ManiphestTaskFulltextEngine' => 'PhabricatorFulltextEngine', 'ManiphestTaskGraph' => 'PhabricatorObjectGraph', 'ManiphestTaskHasCommitEdgeType' => 'PhabricatorEdgeType', @@ -7679,9 +7685,11 @@ phutil_register_library_map(array( 'PhabricatorEditEngineConfigurationViewController' => 'PhabricatorEditEngineController', 'PhabricatorEditEngineController' => 'PhabricatorApplicationTransactionController', 'PhabricatorEditEngineDatasource' => 'PhabricatorTypeaheadDatasource', + 'PhabricatorEditEngineDefaultLock' => 'PhabricatorEditEngineLock', 'PhabricatorEditEngineExtension' => 'Phobject', 'PhabricatorEditEngineExtensionModule' => 'PhabricatorConfigModule', 'PhabricatorEditEngineListController' => 'PhabricatorEditEngineController', + 'PhabricatorEditEngineLock' => 'Phobject', 'PhabricatorEditEnginePointsCommentAction' => 'PhabricatorEditEngineCommentAction', 'PhabricatorEditEngineProfileMenuItem' => 'PhabricatorProfileMenuItem', 'PhabricatorEditEngineQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', diff --git a/src/applications/maniphest/config/PhabricatorManiphestConfigOptions.php b/src/applications/maniphest/config/PhabricatorManiphestConfigOptions.php index 9d692b539e..4a1927c5a8 100644 --- a/src/applications/maniphest/config/PhabricatorManiphestConfigOptions.php +++ b/src/applications/maniphest/config/PhabricatorManiphestConfigOptions.php @@ -210,6 +210,8 @@ The keys you can provide in a specification are: - `claim` //Optional bool.// By default, closing an unassigned task claims it. You can set this to `false` to disable this behavior for a particular status. + - `locked` //Optional bool.// Lock tasks in this status, preventing users + from commenting. Statuses will appear in the UI in the order specified. Note the status marked `special` as `duplicate` is not settable directly and will not appear in UI diff --git a/src/applications/maniphest/constants/ManiphestTaskStatus.php b/src/applications/maniphest/constants/ManiphestTaskStatus.php index eeeb4cf165..5734892f0a 100644 --- a/src/applications/maniphest/constants/ManiphestTaskStatus.php +++ b/src/applications/maniphest/constants/ManiphestTaskStatus.php @@ -156,6 +156,10 @@ final class ManiphestTaskStatus extends ManiphestConstants { return !self::isOpenStatus($status); } + public static function isLockedStatus($status) { + return self::getStatusAttribute($status, 'locked', false); + } + public static function getStatusActionName($status) { return self::getStatusAttribute($status, 'name.action'); } @@ -277,6 +281,7 @@ final class ManiphestTaskStatus extends ManiphestConstants { 'keywords' => 'optional list', 'disabled' => 'optional bool', 'claim' => 'optional bool', + 'locked' => 'optional bool', )); } diff --git a/src/applications/maniphest/controller/ManiphestTaskDetailController.php b/src/applications/maniphest/controller/ManiphestTaskDetailController.php index 60d3a81b1b..514090dda8 100644 --- a/src/applications/maniphest/controller/ManiphestTaskDetailController.php +++ b/src/applications/maniphest/controller/ManiphestTaskDetailController.php @@ -257,6 +257,12 @@ final class ManiphestTaskDetailController extends ManiphestController { $task, PhabricatorPolicyCapability::CAN_EDIT); + $can_interact = PhabricatorPolicyFilter::canInteract($viewer, $task); + + // We expect a policy dialog if you can't edit the task, and expect a + // lock override dialog if you can't interact with it. + $workflow_edit = (!$can_edit || !$can_interact); + $curtain = $this->newCurtainView($task); $curtain->addAction( @@ -265,7 +271,7 @@ final class ManiphestTaskDetailController extends ManiphestController { ->setIcon('fa-pencil') ->setHref($this->getApplicationURI("/task/edit/{$id}/")) ->setDisabled(!$can_edit) - ->setWorkflow(!$can_edit)); + ->setWorkflow($workflow_edit)); $edit_config = $edit_engine->loadDefaultEditConfiguration($task); $can_create = (bool)$edit_config; diff --git a/src/applications/maniphest/editor/ManiphestTaskEditEngineLock.php b/src/applications/maniphest/editor/ManiphestTaskEditEngineLock.php new file mode 100644 index 0000000000..5f51ef242f --- /dev/null +++ b/src/applications/maniphest/editor/ManiphestTaskEditEngineLock.php @@ -0,0 +1,28 @@ +setTitle(pht('Edit Locked Task')) + ->appendParagraph(pht('This task is locked. Edit it anyway?')) + ->addSubmitButton(pht('Override Task Lock')); + } + + public function willBlockUserInteractionWithDialog( + AphrontDialogView $dialog) { + + return $dialog + ->setTitle(pht('Task Locked')) + ->appendParagraph( + pht('You can not interact with this task because it is locked.')); + } + + public function getLockedObjectDisplayText() { + return pht('This task has been locked.'); + } + +} diff --git a/src/applications/maniphest/storage/ManiphestTask.php b/src/applications/maniphest/storage/ManiphestTask.php index be540b9056..36d0a21b07 100644 --- a/src/applications/maniphest/storage/ManiphestTask.php +++ b/src/applications/maniphest/storage/ManiphestTask.php @@ -17,7 +17,8 @@ final class ManiphestTask extends ManiphestDAO PhabricatorConduitResultInterface, PhabricatorFulltextInterface, DoorkeeperBridgedObjectInterface, - PhabricatorEditEngineSubtypeInterface { + PhabricatorEditEngineSubtypeInterface, + PhabricatorEditEngineLockableInterface { const MARKUP_FIELD_DESCRIPTION = 'markup:desc'; @@ -213,6 +214,10 @@ final class ManiphestTask extends ManiphestDAO return ManiphestTaskStatus::isClosedStatus($this->getStatus()); } + public function isLocked() { + return ManiphestTaskStatus::isLockedStatus($this->getStatus()); + } + public function setProperty($key, $value) { $this->properties[$key] = $value; return $this; @@ -343,6 +348,7 @@ final class ManiphestTask extends ManiphestDAO public function getCapabilities() { return array( PhabricatorPolicyCapability::CAN_VIEW, + PhabricatorPolicyCapability::CAN_INTERACT, PhabricatorPolicyCapability::CAN_EDIT, ); } @@ -351,6 +357,12 @@ final class ManiphestTask extends ManiphestDAO switch ($capability) { case PhabricatorPolicyCapability::CAN_VIEW: return $this->getViewPolicy(); + case PhabricatorPolicyCapability::CAN_INTERACT: + if ($this->isLocked()) { + return PhabricatorPolicies::POLICY_NOONE; + } else { + return PhabricatorPolicies::POLICY_USER; + } case PhabricatorPolicyCapability::CAN_EDIT: return $this->getEditPolicy(); } @@ -562,4 +574,12 @@ final class ManiphestTask extends ManiphestDAO return PhabricatorEditEngineSubtype::newSubtypeMap($config); } + +/* -( PhabricatorEditEngineLockableInterface )----------------------------- */ + + + public function newEditEngineLock() { + return new ManiphestTaskEditEngineLock(); + } + } diff --git a/src/applications/policy/capability/PhabricatorPolicyCapability.php b/src/applications/policy/capability/PhabricatorPolicyCapability.php index 36b8ea87c0..2bb1ce4238 100644 --- a/src/applications/policy/capability/PhabricatorPolicyCapability.php +++ b/src/applications/policy/capability/PhabricatorPolicyCapability.php @@ -5,6 +5,7 @@ abstract class PhabricatorPolicyCapability extends Phobject { const CAN_VIEW = 'view'; const CAN_EDIT = 'edit'; const CAN_JOIN = 'join'; + const CAN_INTERACT = 'interact'; /** * Get the unique key identifying this capability. This key must be globally diff --git a/src/applications/policy/filter/PhabricatorPolicyFilter.php b/src/applications/policy/filter/PhabricatorPolicyFilter.php index 44931085fc..f3e1bd2083 100644 --- a/src/applications/policy/filter/PhabricatorPolicyFilter.php +++ b/src/applications/policy/filter/PhabricatorPolicyFilter.php @@ -86,6 +86,36 @@ final class PhabricatorPolicyFilter extends Phobject { return (count($result) == 1); } + public static function canInteract( + PhabricatorUser $user, + PhabricatorPolicyInterface $object) { + + $capabilities = $object->getCapabilities(); + $capabilities = array_fuse($capabilities); + + $can_interact = PhabricatorPolicyCapability::CAN_INTERACT; + $can_view = PhabricatorPolicyCapability::CAN_VIEW; + + $require = array(); + + // If the object doesn't support a separate "Interact" capability, we + // only use the "View" capability: for most objects, you can interact + // with them if you can see them. + $require[] = $can_view; + + if (isset($capabilities[$can_interact])) { + $require[] = $can_interact; + } + + foreach ($require as $capability) { + if (!self::hasCapability($user, $object, $capability)) { + return false; + } + } + + return true; + } + public function setViewer(PhabricatorUser $user) { $this->viewer = $user; return $this; diff --git a/src/applications/subscriptions/controller/PhabricatorSubscriptionsEditController.php b/src/applications/subscriptions/controller/PhabricatorSubscriptionsEditController.php index add833c127..e005c120f0 100644 --- a/src/applications/subscriptions/controller/PhabricatorSubscriptionsEditController.php +++ b/src/applications/subscriptions/controller/PhabricatorSubscriptionsEditController.php @@ -47,6 +47,15 @@ final class PhabricatorSubscriptionsEditController $handle->getURI()); } + if (!PhabricatorPolicyFilter::canInteract($viewer, $object)) { + $lock = PhabricatorEditEngineLock::newForObject($viewer, $object); + + $dialog = $this->newDialog() + ->addCancelButton($handle->getURI()); + + return $lock->willBlockUserInteractionWithDialog($dialog); + } + if ($object instanceof PhabricatorApplicationTransactionInterface) { if ($is_add) { $xaction_value = array( diff --git a/src/applications/subscriptions/events/PhabricatorSubscriptionsUIEventListener.php b/src/applications/subscriptions/events/PhabricatorSubscriptionsUIEventListener.php index acbb978238..09b26819c0 100644 --- a/src/applications/subscriptions/events/PhabricatorSubscriptionsUIEventListener.php +++ b/src/applications/subscriptions/events/PhabricatorSubscriptionsUIEventListener.php @@ -56,20 +56,24 @@ final class PhabricatorSubscriptionsUIEventListener $subscribed = isset($edges[$src_phid][$edge_type][$user_phid]); } + $can_interact = PhabricatorPolicyFilter::canInteract($user, $object); + if ($subscribed) { $sub_action = id(new PhabricatorActionView()) ->setWorkflow(true) ->setRenderAsForm(true) ->setHref('/subscriptions/delete/'.$object->getPHID().'/') ->setName(pht('Unsubscribe')) - ->setIcon('fa-minus-circle'); + ->setIcon('fa-minus-circle') + ->setDisabled(!$can_interact); } else { $sub_action = id(new PhabricatorActionView()) ->setWorkflow(true) ->setRenderAsForm(true) ->setHref('/subscriptions/add/'.$object->getPHID().'/') ->setName(pht('Subscribe')) - ->setIcon('fa-plus-circle'); + ->setIcon('fa-plus-circle') + ->setDisabled(!$can_interact); } if (!$user->isLoggedIn()) { diff --git a/src/applications/transactions/editengine/PhabricatorEditEngine.php b/src/applications/transactions/editengine/PhabricatorEditEngine.php index 9d769c0e66..4ad6e39be8 100644 --- a/src/applications/transactions/editengine/PhabricatorEditEngine.php +++ b/src/applications/transactions/editengine/PhabricatorEditEngine.php @@ -985,9 +985,33 @@ abstract class PhabricatorEditEngine $fields = $this->buildEditFields($object); $template = $object->getApplicationTransactionTemplate(); + if ($this->getIsCreate()) { + $cancel_uri = $this->getObjectCreateCancelURI($object); + $submit_button = $this->getObjectCreateButtonText($object); + } else { + $cancel_uri = $this->getEffectiveObjectEditCancelURI($object); + $submit_button = $this->getObjectEditButtonText($object); + } + $config = $this->getEditEngineConfiguration() ->attachEngine($this); + $can_interact = PhabricatorPolicyFilter::canInteract($viewer, $object); + if (!$can_interact && + !$request->getBool('editEngine') && + !$request->getBool('overrideLock')) { + + $lock = PhabricatorEditEngineLock::newForObject($viewer, $object); + + $dialog = $this->getController() + ->newDialog() + ->addHiddenInput('overrideLock', true) + ->setDisableWorkflowOnSubmit(true) + ->addCancelButton($cancel_uri); + + return $lock->willPromptUserForLockOverrideWithDialog($dialog); + } + $validation_exception = null; if ($request->isFormPost() && $request->getBool('editEngine')) { $submit_fields = $fields; @@ -1154,14 +1178,6 @@ abstract class PhabricatorEditEngine $form = $this->buildEditForm($object, $fields); if ($request->isAjax()) { - if ($this->getIsCreate()) { - $cancel_uri = $this->getObjectCreateCancelURI($object); - $submit_button = $this->getObjectCreateButtonText($object); - } else { - $cancel_uri = $this->getEffectiveObjectEditCancelURI($object); - $submit_button = $this->getObjectEditButtonText($object); - } - return $this->getController() ->newDialog() ->setWidth(AphrontDialogView::WIDTH_FULL) @@ -1554,8 +1570,16 @@ abstract class PhabricatorEditEngine } $viewer = $this->getViewer(); - $object_phid = $object->getPHID(); + $can_interact = PhabricatorPolicyFilter::canInteract($viewer, $object); + if (!$can_interact) { + $lock = PhabricatorEditEngineLock::newForObject($viewer, $object); + + return id(new PhabricatorApplicationTransactionCommentView()) + ->setEditEngineLock($lock); + } + + $object_phid = $object->getPHID(); $is_serious = PhabricatorEnv::getEnvConfig('phabricator.serious-business'); if ($is_serious) { @@ -1700,11 +1724,19 @@ abstract class PhabricatorEditEngine private function buildError($object, $title, $body) { $cancel_uri = $this->getObjectCreateCancelURI($object); - return $this->getController() + $dialog = $this->getController() ->newDialog() - ->setTitle($title) - ->appendParagraph($body) ->addCancelButton($cancel_uri); + + if ($title !== null) { + $dialog->setTitle($title); + } + + if ($body !== null) { + $dialog->appendParagraph($body); + } + + return $dialog; } @@ -1761,6 +1793,14 @@ abstract class PhabricatorEditEngine $config->getName())); } + private function buildLockedObjectResponse($object) { + $dialog = $this->buildError($object, null, null); + $viewer = $this->getViewer(); + + $lock = PhabricatorEditEngineLock::newForObject($viewer, $object); + return $lock->willBlockUserInteractionWithDialog($dialog); + } + private function buildCommentResponse($object) { $viewer = $this->getViewer(); @@ -1775,6 +1815,11 @@ abstract class PhabricatorEditEngine return new Aphront400Response(); } + $can_interact = PhabricatorPolicyFilter::canInteract($viewer, $object); + if (!$can_interact) { + return $this->buildLockedObjectResponse($object); + } + $config = $this->loadDefaultEditConfiguration($object); if (!$config) { return new Aphront404Response(); diff --git a/src/applications/transactions/editengine/PhabricatorEditEngineDefaultLock.php b/src/applications/transactions/editengine/PhabricatorEditEngineDefaultLock.php new file mode 100644 index 0000000000..dc1118344a --- /dev/null +++ b/src/applications/transactions/editengine/PhabricatorEditEngineDefaultLock.php @@ -0,0 +1,4 @@ +viewer = $viewer; + return $this; + } + + final public function getViewer() { + return $this->viewer; + } + + final public function setObject($object) { + $this->object = $object; + return $this; + } + + final public function getObject() { + return $this->object; + } + + public function willPromptUserForLockOverrideWithDialog( + AphrontDialogView $dialog) { + + return $dialog + ->setTitle(pht('Edit Locked Object')) + ->appendParagraph(pht('This object is locked. Edit it anyway?')) + ->addSubmitButton(pht('Override Lock')); + } + + public function willBlockUserInteractionWithDialog( + AphrontDialogView $dialog) { + + return $dialog + ->setTitle(pht('Object Locked')) + ->appendParagraph( + pht('You can not interact with this object because it is locked.')); + } + + public function getLockedObjectDisplayText() { + return pht('This object has been locked.'); + } + + public static function newForObject( + PhabricatorUser $viewer, + $object) { + + if ($object instanceof PhabricatorEditEngineLockableInterface) { + $lock = $object->newEditEngineLock(); + } else { + $lock = new PhabricatorEditEngineDefaultLock(); + } + + return $lock + ->setViewer($viewer) + ->setObject($object); + } + + + +} diff --git a/src/applications/transactions/editengine/PhabricatorEditEngineLockableInterface.php b/src/applications/transactions/editengine/PhabricatorEditEngineLockableInterface.php new file mode 100644 index 0000000000..8d7587728b --- /dev/null +++ b/src/applications/transactions/editengine/PhabricatorEditEngineLockableInterface.php @@ -0,0 +1,7 @@ +noPermission; } + public function setEditEngineLock(PhabricatorEditEngineLock $lock) { + $this->editEngineLock = $lock; + return $this; + } + + public function getEditEngineLock() { + return $this->editEngineLock; + } + public function setTransactionTimeline( PhabricatorApplicationTransactionView $timeline) { $timeline->setQuoteTargetID($this->getCommentID()); - if ($this->getNoPermission()) { + if ($this->getNoPermission() || $this->getEditEngineLock()) { $timeline->setShouldTerminate(true); } @@ -166,6 +176,16 @@ class PhabricatorApplicationTransactionCommentView extends AphrontView { return null; } + $lock = $this->getEditEngineLock(); + if ($lock) { + return id(new PHUIInfoView()) + ->setSeverity(PHUIInfoView::SEVERITY_WARNING) + ->setErrors( + array( + $lock->getLockedObjectDisplayText(), + )); + } + $user = $this->getUser(); if (!$user->isLoggedIn()) { $uri = id(new PhutilURI('/login/'))