Page MenuHomePhabricator

Allow task statuses to specify that either "comments" or "edits" are "locked"
ClosedPublic

Authored by epriestley on Feb 14 2019, 3:10 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Mar 25, 8:39 AM
Unknown Object (File)
Mon, Mar 25, 8:39 AM
Unknown Object (File)
Tue, Mar 5, 2:31 AM
Unknown Object (File)
Feb 4 2024, 12:35 PM
Unknown Object (File)
Feb 3 2024, 8:18 PM
Unknown Object (File)
Jan 15 2024, 3:48 AM
Unknown Object (File)
Jan 12 2024, 1:53 PM
Unknown Object (File)
Jan 9 2024, 9:02 AM
Subscribers
None

Details

Summary

Ref T13249. See PHI1059. This allows "locked" in maniphest.statuses to specify that either "comments" are locked (current behavior, advisory, overridable by users with edit permission, e.g. for calming discussion on a contentious issue or putting a guard rail on things); or "edits" are locked (hard lock, only task owner can edit things).

Roughly, "comments" is a soft/advisory lock. "edits" is a hard/strict lock. (I think both types of locks have reasonable use cases, which is why I'm not just making locks stronger across the board.)

When "edits" are locked:

  • The edit policy looks like "no one" to normal callers.
  • In one special case, we sneak the real value through a back channel using PolicyCodex in the specific narrow case that you're editing the object. Otherwise, the policy selector control incorrectly switches to "No One".
  • We also have to do a little more validation around applying a mixture of status + owner transactions that could leave the task uneditable.

For now, I'm allowing you to reassign a hard-locked task to someone else. If you get this wrong, we can end up in a state where no one can edit the task. If this is an issue, we could respond in various ways: prevent these edits; prevent assigning to disabled users; provide a bin/task reassign; uh maybe have a quorum convene?

Test Plan
  • Defined "Soft Locked" and "Hard Locked" statues.
  • "Hard Locked" a task, hit errors (trying to unassign myself, trying to hard lock an unassigned task).
  • Saw nice new policy guidance icon in header.

Screen Shot 2019-02-13 at 6.50.43 PM.png (179×387 px, 16 KB)

Diff Detail

Repository
rP Phabricator
Branch
lock2
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 21993
Build 30040: Run Core Tests
Build 30039: arc lint + arc unit

Event Timeline

  • Undo introduction of random whitespace.
amckinley added inline comments.
src/applications/maniphest/constants/ManiphestTaskStatus.php
306–308

thisisfine.jpg

src/applications/policy/editor/PhabricatorPolicyEditEngineExtension.php
71–77

I'm slightly worried about adding upstream support for installs to create exciting new Codii that subvert the rest of the policy infrastructure. On the other hand, users would have no one to blame but themselves, and anyone adding classes can already shoot themselves in the foot without this revision, so I'm probably worrying about nothing.

93–103

Do we also want to add a note in the policy control explaining this? I don't think this will be confusing as far as locked tasks are concerned, but I'm a little worried about how these "policy exceptions" might expand over time and interact in surprising ways.

This revision is now accepted and ready to land.Feb 15 2019, 4:37 PM
src/applications/policy/editor/PhabricatorPolicyEditEngineExtension.php
71–77

I'm at least hoping that at the point that you type class ... Codex you're clearly opting into some know-what-you're-doing wizardry.

(You can't add an extension which changes the Maniphest codex. You can create some new MyThing object and give it whatever bizarre policy behavior you want, but you could already flip a coin and return a random policy from getPolicy() or whatever, so I don't think this is really that much more powerful if you're dedicated.)

93–103

Yeah, this is a bit rough and I think we can probably be clearer here. Partly just waiting for feedback before I go too far down this route, partly trying to keep this change smallish.

In the web UI, I think we could possibly even hide the field completely (but: "why is the field hidden?") or lock it with an explanation (but: editing the stored policy is technically a legitimate operation, and you might reasonably want to unlock-and-change-the-policy in one edit, so maybe this isn't a great idea?).

(Even if we lock or hide the field in the UI you'd still be able to use Conduit to edit it.)

I think an explanation of some sort is probably reasonable, and probably a banner on top of the task like "This is locked because it's in policy X, so only you can edit it (no matter what the controls say)" would probably be helpful enough often enough to justify the clutter. Now that we have a codex, I might go back and give "soft locked" tasks some special behavior, too.

A minor issue here is that captions look awful and we don't have another UI element for adding explanations. I'd sort of like to just put a " Task Locked" hint next to the field with a more detailed explanation on a tooltip or click, not a huge paragraph of text in the middle of the form.

This revision was automatically updated to reflect the committed changes.