Page Menu
Home
Phabricator
Search
Configure Global Search
Log In
Files
F13178209
D20165.diff
No One
Temporary
Actions
View File
Edit File
Delete File
View Transforms
Subscribe
Mute Notifications
Award Token
Flag For Later
Size
16 KB
Referenced Files
None
Subscribers
None
D20165.diff
View Options
diff --git a/resources/celerity/map.php b/resources/celerity/map.php
--- a/resources/celerity/map.php
+++ b/resources/celerity/map.php
@@ -9,7 +9,7 @@
'names' => array(
'conpherence.pkg.css' => '3c8a0668',
'conpherence.pkg.js' => '020aebcf',
- 'core.pkg.css' => '85a1da99',
+ 'core.pkg.css' => 'f2319e1f',
'core.pkg.js' => '5c737607',
'differential.pkg.css' => 'b8df73d4',
'differential.pkg.js' => '67c9ea4c',
@@ -154,7 +154,7 @@
'rsrc/css/phui/phui-form-view.css' => '01b796c0',
'rsrc/css/phui/phui-form.css' => '159e2d9c',
'rsrc/css/phui/phui-head-thing.css' => 'd7f293df',
- 'rsrc/css/phui/phui-header-view.css' => '93cea4ec',
+ 'rsrc/css/phui/phui-header-view.css' => '285c9139',
'rsrc/css/phui/phui-hovercard.css' => '6ca90fa0',
'rsrc/css/phui/phui-icon-set-selector.css' => '7aa5f3ec',
'rsrc/css/phui/phui-icon.css' => '4cbc684a',
@@ -821,7 +821,7 @@
'phui-form-css' => '159e2d9c',
'phui-form-view-css' => '01b796c0',
'phui-head-thing-view-css' => 'd7f293df',
- 'phui-header-view-css' => '93cea4ec',
+ 'phui-header-view-css' => '285c9139',
'phui-hovercard' => '074f0783',
'phui-hovercard-view-css' => '6ca90fa0',
'phui-icon-set-selector-css' => '7aa5f3ec',
diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -1743,6 +1743,7 @@
'ManiphestTaskParentTransaction' => 'applications/maniphest/xaction/ManiphestTaskParentTransaction.php',
'ManiphestTaskPoints' => 'applications/maniphest/constants/ManiphestTaskPoints.php',
'ManiphestTaskPointsTransaction' => 'applications/maniphest/xaction/ManiphestTaskPointsTransaction.php',
+ 'ManiphestTaskPolicyCodex' => 'applications/maniphest/policy/ManiphestTaskPolicyCodex.php',
'ManiphestTaskPriority' => 'applications/maniphest/constants/ManiphestTaskPriority.php',
'ManiphestTaskPriorityDatasource' => 'applications/maniphest/typeahead/ManiphestTaskPriorityDatasource.php',
'ManiphestTaskPriorityHeraldAction' => 'applications/maniphest/herald/ManiphestTaskPriorityHeraldAction.php',
@@ -7384,6 +7385,7 @@
'PhabricatorEditEngineSubtypeInterface',
'PhabricatorEditEngineLockableInterface',
'PhabricatorEditEngineMFAInterface',
+ 'PhabricatorPolicyCodexInterface',
),
'ManiphestTaskAssignHeraldAction' => 'HeraldAction',
'ManiphestTaskAssignOtherHeraldAction' => 'ManiphestTaskAssignHeraldAction',
@@ -7435,6 +7437,7 @@
'ManiphestTaskParentTransaction' => 'ManiphestTaskTransactionType',
'ManiphestTaskPoints' => 'Phobject',
'ManiphestTaskPointsTransaction' => 'ManiphestTaskTransactionType',
+ 'ManiphestTaskPolicyCodex' => 'PhabricatorPolicyCodex',
'ManiphestTaskPriority' => 'ManiphestConstants',
'ManiphestTaskPriorityDatasource' => 'PhabricatorTypeaheadDatasource',
'ManiphestTaskPriorityHeraldAction' => 'HeraldAction',
diff --git a/src/applications/maniphest/config/PhabricatorManiphestConfigOptions.php b/src/applications/maniphest/config/PhabricatorManiphestConfigOptions.php
--- a/src/applications/maniphest/config/PhabricatorManiphestConfigOptions.php
+++ b/src/applications/maniphest/config/PhabricatorManiphestConfigOptions.php
@@ -210,8 +210,9 @@
- `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.
+ - `locked` //Optional string.// Lock tasks in this status. Specify "comments"
+ to lock comments (users who can edit the task may override this lock).
+ Specify "edits" to prevent anyone except the task owner from making edits.
- `mfa` //Optional bool.// Require all edits to this task to be signed with
multi-factor authentication.
diff --git a/src/applications/maniphest/constants/ManiphestTaskStatus.php b/src/applications/maniphest/constants/ManiphestTaskStatus.php
--- a/src/applications/maniphest/constants/ManiphestTaskStatus.php
+++ b/src/applications/maniphest/constants/ManiphestTaskStatus.php
@@ -16,6 +16,9 @@
const SPECIAL_CLOSED = 'closed';
const SPECIAL_DUPLICATE = 'duplicate';
+ const LOCKED_COMMENTS = 'comments';
+ const LOCKED_EDITS = 'edits';
+
private static function getStatusConfig() {
return PhabricatorEnv::getEnvConfig('maniphest.statuses');
}
@@ -156,8 +159,13 @@
return !self::isOpenStatus($status);
}
- public static function isLockedStatus($status) {
- return self::getStatusAttribute($status, 'locked', false);
+ public static function areCommentsLockedInStatus($status) {
+ return (bool)self::getStatusAttribute($status, 'locked', false);
+ }
+
+ public static function areEditsLockedInStatus($status) {
+ $locked = self::getStatusAttribute($status, 'locked');
+ return ($locked === self::LOCKED_EDITS);
}
public static function isMFAStatus($status) {
@@ -285,11 +293,35 @@
'keywords' => 'optional list<string>',
'disabled' => 'optional bool',
'claim' => 'optional bool',
- 'locked' => 'optional bool',
+ 'locked' => 'optional bool|string',
'mfa' => 'optional bool',
));
}
+ // Supported values are "comments" or "edits". For backward compatibility,
+ // "true" is an alias of "comments".
+
+ foreach ($config as $key => $value) {
+ $locked = idx($value, 'locked', false);
+ if ($locked === true || $locked === false) {
+ continue;
+ }
+
+ if ($locked === self::LOCKED_EDITS ||
+ $locked === self::LOCKED_COMMENTS) {
+ continue;
+ }
+
+ throw new Exception(
+ pht(
+ 'Task status ("%s") has unrecognized value for "locked" '.
+ 'configuration ("%s"). Supported values are: "%s", "%s".',
+ $key,
+ $locked,
+ self::LOCKED_COMMENTS,
+ self::LOCKED_EDITS));
+ }
+
$special_map = array();
foreach ($config as $key => $value) {
$special = idx($value, 'special');
diff --git a/src/applications/maniphest/editor/ManiphestTransactionEditor.php b/src/applications/maniphest/editor/ManiphestTransactionEditor.php
--- a/src/applications/maniphest/editor/ManiphestTransactionEditor.php
+++ b/src/applications/maniphest/editor/ManiphestTransactionEditor.php
@@ -552,6 +552,10 @@
$errors = array_merge($errors, $this->moreValidationErrors);
}
+ foreach ($this->getLockValidationErrors($object, $xactions) as $error) {
+ $errors[] = $error;
+ }
+
return $errors;
}
@@ -1011,5 +1015,86 @@
}
+ private function getLockValidationErrors($object, array $xactions) {
+ $errors = array();
+
+ $old_owner = $object->getOwnerPHID();
+ $old_status = $object->getStatus();
+
+ $new_owner = $old_owner;
+ $new_status = $old_status;
+
+ $owner_xaction = null;
+ $status_xaction = null;
+
+ foreach ($xactions as $xaction) {
+ switch ($xaction->getTransactionType()) {
+ case ManiphestTaskOwnerTransaction::TRANSACTIONTYPE:
+ $new_owner = $xaction->getNewValue();
+ $owner_xaction = $xaction;
+ break;
+ case ManiphestTaskStatusTransaction::TRANSACTIONTYPE:
+ $new_status = $xaction->getNewValue();
+ $status_xaction = $xaction;
+ break;
+ }
+ }
+
+ $actor_phid = $this->getActingAsPHID();
+
+ $was_locked = ManiphestTaskStatus::areEditsLockedInStatus(
+ $old_status);
+ $now_locked = ManiphestTaskStatus::areEditsLockedInStatus(
+ $new_status);
+
+ if (!$now_locked) {
+ // If we're not ending in an edit-locked status, everything is good.
+ } else if ($new_owner !== null) {
+ // If we ending the edit with some valid owner, this is allowed for
+ // now. We might need to revisit this.
+ } else {
+ // The edits end with the task locked and unowned. No one will be able
+ // to edit it, so we forbid this. We try to be specific about what the
+ // user did wrong.
+
+ $owner_changed = ($old_owner && !$new_owner);
+ $status_changed = ($was_locked !== $now_locked);
+ $message = null;
+
+ if ($status_changed && $owner_changed) {
+ $message = pht(
+ 'You can not lock this task and unassign it at the same time '.
+ 'because no one will be able to edit it anymore. Lock the task '.
+ 'or remove the owner, but not both.');
+ $problem_xaction = $status_xaction;
+ } else if ($status_changed) {
+ $message = pht(
+ 'You can not lock this task because it does not have an owner. '.
+ 'No one would be able to edit the task. Assign the task to an '.
+ 'owner before locking it.');
+ $problem_xaction = $status_xaction;
+ } else if ($owner_changed) {
+ $message = pht(
+ 'You can not remove the owner of this task because it is locked '.
+ 'and no one would be able to edit the task. Reassign the task or '.
+ 'unlock it before removing the owner.');
+ $problem_xaction = $owner_xaction;
+ } else {
+ // If the task was already broken, we don't have a transaction to
+ // complain about so just let it through. In theory, this is
+ // impossible since policy rules should kick in before we get here.
+ }
+
+ if ($message) {
+ $errors[] = new PhabricatorApplicationTransactionValidationError(
+ $problem_xaction->getTransactionType(),
+ pht('Lock Error'),
+ $message,
+ $problem_xaction);
+ }
+ }
+
+ return $errors;
+ }
}
diff --git a/src/applications/maniphest/policy/ManiphestTaskPolicyCodex.php b/src/applications/maniphest/policy/ManiphestTaskPolicyCodex.php
new file mode 100644
--- /dev/null
+++ b/src/applications/maniphest/policy/ManiphestTaskPolicyCodex.php
@@ -0,0 +1,70 @@
+<?php
+
+final class ManiphestTaskPolicyCodex
+ extends PhabricatorPolicyCodex {
+
+ public function getPolicyShortName() {
+ $object = $this->getObject();
+
+ if ($object->areEditsLocked()) {
+ return pht('Edits Locked');
+ }
+
+ return null;
+ }
+
+ public function getPolicyIcon() {
+ $object = $this->getObject();
+
+ if ($object->areEditsLocked()) {
+ return 'fa-lock';
+ }
+
+ return null;
+ }
+
+ public function getPolicyTagClasses() {
+ $object = $this->getObject();
+ $classes = array();
+
+ if ($object->areEditsLocked()) {
+ $classes[] = 'policy-adjusted-locked';
+ }
+
+ return $classes;
+ }
+
+ public function getPolicySpecialRuleDescriptions() {
+ $object = $this->getObject();
+
+ $rules = array();
+
+ $rules[] = $this->newRule()
+ ->setCapabilities(
+ array(
+ PhabricatorPolicyCapability::CAN_EDIT,
+ ))
+ ->setIsActive($object->areEditsLocked())
+ ->setDescription(
+ pht(
+ 'Tasks with edits locked may only be edited by their owner.'));
+
+ return $rules;
+ }
+
+ public function getPolicyForEdit($capability) {
+
+ // When a task has its edits locked, the effective edit policy is locked
+ // to "No One". However, the task owner may still bypass the lock and edit
+ // the task. When they do, we want the control in the UI to have the
+ // correct value. Return the real value stored on the object.
+
+ switch ($capability) {
+ case PhabricatorPolicyCapability::CAN_EDIT:
+ return $this->getObject()->getEditPolicy();
+ }
+
+ return parent::getPolicyForEdit($capability);
+ }
+
+}
diff --git a/src/applications/maniphest/storage/ManiphestTask.php b/src/applications/maniphest/storage/ManiphestTask.php
--- a/src/applications/maniphest/storage/ManiphestTask.php
+++ b/src/applications/maniphest/storage/ManiphestTask.php
@@ -20,7 +20,8 @@
DoorkeeperBridgedObjectInterface,
PhabricatorEditEngineSubtypeInterface,
PhabricatorEditEngineLockableInterface,
- PhabricatorEditEngineMFAInterface {
+ PhabricatorEditEngineMFAInterface,
+ PhabricatorPolicyCodexInterface {
const MARKUP_FIELD_DESCRIPTION = 'markup:desc';
@@ -217,8 +218,16 @@
return ManiphestTaskStatus::isClosedStatus($this->getStatus());
}
- public function isLocked() {
- return ManiphestTaskStatus::isLockedStatus($this->getStatus());
+ public function areCommentsLocked() {
+ if ($this->areEditsLocked()) {
+ return true;
+ }
+
+ return ManiphestTaskStatus::areCommentsLockedInStatus($this->getStatus());
+ }
+
+ public function areEditsLocked() {
+ return ManiphestTaskStatus::areEditsLockedInStatus($this->getStatus());
}
public function setProperty($key, $value) {
@@ -371,13 +380,17 @@
case PhabricatorPolicyCapability::CAN_VIEW:
return $this->getViewPolicy();
case PhabricatorPolicyCapability::CAN_INTERACT:
- if ($this->isLocked()) {
+ if ($this->areCommentsLocked()) {
return PhabricatorPolicies::POLICY_NOONE;
} else {
return $this->getViewPolicy();
}
case PhabricatorPolicyCapability::CAN_EDIT:
- return $this->getEditPolicy();
+ if ($this->areEditsLocked()) {
+ return PhabricatorPolicies::POLICY_NOONE;
+ } else {
+ return $this->getEditPolicy();
+ }
}
}
@@ -628,4 +641,12 @@
return new ManiphestTaskMFAEngine();
}
+
+/* -( PhabricatorPolicyCodexInterface )------------------------------------ */
+
+
+ public function newPolicyCodex() {
+ return new ManiphestTaskPolicyCodex();
+ }
+
}
diff --git a/src/applications/policy/codex/PhabricatorPolicyCodex.php b/src/applications/policy/codex/PhabricatorPolicyCodex.php
--- a/src/applications/policy/codex/PhabricatorPolicyCodex.php
+++ b/src/applications/policy/codex/PhabricatorPolicyCodex.php
@@ -29,6 +29,10 @@
return array();
}
+ public function getPolicyForEdit($capability) {
+ return $this->getObject()->getPolicy($capability);
+ }
+
public function getDefaultPolicy() {
return PhabricatorPolicyQuery::getDefaultPolicyForObject(
$this->viewer,
diff --git a/src/applications/policy/editor/PhabricatorPolicyEditEngineExtension.php b/src/applications/policy/editor/PhabricatorPolicyEditEngineExtension.php
--- a/src/applications/policy/editor/PhabricatorPolicyEditEngineExtension.php
+++ b/src/applications/policy/editor/PhabricatorPolicyEditEngineExtension.php
@@ -68,6 +68,14 @@
),
);
+ if ($object instanceof PhabricatorPolicyCodexInterface) {
+ $codex = PhabricatorPolicyCodex::newFromObject(
+ $object,
+ $viewer);
+ } else {
+ $codex = null;
+ }
+
$fields = array();
foreach ($map as $type => $spec) {
if (empty($types[$type])) {
@@ -82,6 +90,18 @@
$conduit_description = $spec['description.conduit'];
$edit = $spec['edit'];
+ // Objects may present a policy value to the edit workflow that is
+ // different from their nominal policy value: for example, when tasks
+ // are locked, they appear as "Editable By: No One" to other applications
+ // but we still want to edit the actual policy stored in the database
+ // when we show the user a form with a policy control in it.
+
+ if ($codex) {
+ $policy_value = $codex->getPolicyForEdit($capability);
+ } else {
+ $policy_value = $object->getPolicy($capability);
+ }
+
$policy_field = id(new PhabricatorPolicyEditField())
->setKey($key)
->setLabel($label)
@@ -94,7 +114,7 @@
->setDescription($description)
->setConduitDescription($conduit_description)
->setConduitTypeDescription(pht('New policy PHID or constant.'))
- ->setValue($object->getPolicy($capability));
+ ->setValue($policy_value);
$fields[] = $policy_field;
if ($object instanceof PhabricatorSpacesInterface) {
diff --git a/webroot/rsrc/css/phui/phui-header-view.css b/webroot/rsrc/css/phui/phui-header-view.css
--- a/webroot/rsrc/css/phui/phui-header-view.css
+++ b/webroot/rsrc/css/phui/phui-header-view.css
@@ -249,6 +249,16 @@
color: {$sh-indigotext};
}
+.policy-header-callout.policy-adjusted-locked {
+ background: {$sh-pinkbackground};
+}
+
+.policy-header-callout.policy-adjusted-locked .policy-link,
+.policy-header-callout.policy-adjusted-locked .phui-icon-view {
+ color: {$sh-pinktext};
+}
+
+
.policy-header-callout .policy-space-container {
font-weight: bold;
color: {$sh-redtext};
File Metadata
Details
Attached
Mime Type
text/plain
Expires
Thu, May 9, 8:15 PM (1 w, 6 d ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
6276544
Default Alt Text
D20165.diff (16 KB)
Attached To
Mode
D20165: Allow task statuses to specify that either "comments" or "edits" are "locked"
Attached
Detach File
Event Timeline
Log In to Comment