Changeset View
Standalone View
src/applications/policy/editor/PhabricatorPolicyEditEngineExtension.php
| Show First 20 Lines • Show All 62 Lines • ▼ Show 20 Lines | $map = array( | ||||
| 'capability' => PhabricatorPolicyCapability::CAN_JOIN, | 'capability' => PhabricatorPolicyCapability::CAN_JOIN, | ||||
| 'label' => pht('Join Policy'), | 'label' => pht('Join Policy'), | ||||
| 'description' => pht('Controls who can join the object.'), | 'description' => pht('Controls who can join the object.'), | ||||
| 'description.conduit' => pht('Change the join policy of the object.'), | 'description.conduit' => pht('Change the join policy of the object.'), | ||||
| 'edit' => 'join', | 'edit' => 'join', | ||||
| ), | ), | ||||
| ); | ); | ||||
| if ($object instanceof PhabricatorPolicyCodexInterface) { | |||||
| $codex = PhabricatorPolicyCodex::newFromObject( | |||||
| $object, | |||||
| $viewer); | |||||
| } else { | |||||
| $codex = null; | |||||
| } | |||||
amckinley: I'm //slightly// worried about adding upstream support for installs to create exciting new… | |||||
Done Inline ActionsI'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.) epriestley: I'm at least //hoping// that at the point that you type `class ... Codex` you're clearly opting… | |||||
| $fields = array(); | $fields = array(); | ||||
| foreach ($map as $type => $spec) { | foreach ($map as $type => $spec) { | ||||
| if (empty($types[$type])) { | if (empty($types[$type])) { | ||||
| continue; | continue; | ||||
| } | } | ||||
| $capability = $spec['capability']; | $capability = $spec['capability']; | ||||
| $key = $spec['key']; | $key = $spec['key']; | ||||
| $aliases = $spec['aliases']; | $aliases = $spec['aliases']; | ||||
| $label = $spec['label']; | $label = $spec['label']; | ||||
| $description = $spec['description']; | $description = $spec['description']; | ||||
| $conduit_description = $spec['description.conduit']; | $conduit_description = $spec['description.conduit']; | ||||
| $edit = $spec['edit']; | $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); | |||||
| } | |||||
Not Done Inline ActionsDo 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. amckinley: Do we also want to add a note in the policy control explaining this? I don't think this will be… | |||||
Done Inline ActionsYeah, 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. epriestley: Yeah, this is a bit rough and I think we can probably be clearer here. Partly just waiting for… | |||||
| $policy_field = id(new PhabricatorPolicyEditField()) | $policy_field = id(new PhabricatorPolicyEditField()) | ||||
| ->setKey($key) | ->setKey($key) | ||||
| ->setLabel($label) | ->setLabel($label) | ||||
| ->setAliases($aliases) | ->setAliases($aliases) | ||||
| ->setIsCopyable(true) | ->setIsCopyable(true) | ||||
| ->setCapability($capability) | ->setCapability($capability) | ||||
| ->setPolicies($policies) | ->setPolicies($policies) | ||||
| ->setTransactionType($type) | ->setTransactionType($type) | ||||
| ->setEditTypeKey($edit) | ->setEditTypeKey($edit) | ||||
| ->setDescription($description) | ->setDescription($description) | ||||
| ->setConduitDescription($conduit_description) | ->setConduitDescription($conduit_description) | ||||
| ->setConduitTypeDescription(pht('New policy PHID or constant.')) | ->setConduitTypeDescription(pht('New policy PHID or constant.')) | ||||
| ->setValue($object->getPolicy($capability)); | ->setValue($policy_value); | ||||
| $fields[] = $policy_field; | $fields[] = $policy_field; | ||||
| if ($object instanceof PhabricatorSpacesInterface) { | if ($object instanceof PhabricatorSpacesInterface) { | ||||
| if ($capability == PhabricatorPolicyCapability::CAN_VIEW) { | if ($capability == PhabricatorPolicyCapability::CAN_VIEW) { | ||||
| $type_space = PhabricatorTransactions::TYPE_SPACE; | $type_space = PhabricatorTransactions::TYPE_SPACE; | ||||
| if (isset($types[$type_space])) { | if (isset($types[$type_space])) { | ||||
| $space_phid = PhabricatorSpacesNamespaceQuery::getObjectSpacePHID( | $space_phid = PhabricatorSpacesNamespaceQuery::getObjectSpacePHID( | ||||
| $object); | $object); | ||||
| Show All 28 Lines | |||||
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.