1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-22 23:02:42 +01:00

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
This commit is contained in:
epriestley 2017-03-02 12:46:17 -08:00
parent 0a0ac1302f
commit 0e7a5623e3
15 changed files with 272 additions and 17 deletions

View file

@ -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',

View file

@ -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

View file

@ -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<string>',
'disabled' => 'optional bool',
'claim' => 'optional bool',
'locked' => 'optional bool',
));
}

View file

@ -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;

View file

@ -0,0 +1,28 @@
<?php
final class ManiphestTaskEditEngineLock
extends PhabricatorEditEngineLock {
public function willPromptUserForLockOverrideWithDialog(
AphrontDialogView $dialog) {
return $dialog
->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.');
}
}

View file

@ -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();
}
}

View file

@ -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

View file

@ -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;

View file

@ -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(

View file

@ -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()) {

View file

@ -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();

View file

@ -0,0 +1,4 @@
<?php
final class PhabricatorEditEngineDefaultLock
extends PhabricatorEditEngineLock {}

View file

@ -0,0 +1,66 @@
<?php
abstract class PhabricatorEditEngineLock
extends Phobject {
private $viewer;
private $object;
final public function setViewer(PhabricatorUser $viewer) {
$this->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);
}
}

View file

@ -0,0 +1,7 @@
<?php
interface PhabricatorEditEngineLockableInterface {
public function newEditEngineLock();
}

View file

@ -22,6 +22,7 @@ class PhabricatorApplicationTransactionCommentView extends AphrontView {
private $noPermission;
private $fullWidth;
private $infoView;
private $editEngineLock;
private $currentVersion;
private $versionedDraft;
@ -149,11 +150,20 @@ class PhabricatorApplicationTransactionCommentView extends AphrontView {
return $this->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/'))