The "simultate" stuff should be unnecessary with the move to proper transactions: they already have code which prevents you from setting invalid policies on things (specifically, policies which would prevent you from viewing or editing the object). I'm not totally sure if it will work as-is, but in theory the Editor should already prevent you from setting the "Can View Application" policy to something which excludes you. If it allows you to make this change, let me know and I can see what's going on.

Right now it's a little hard to distinguish between user-only, project-scoped, custom-policy scoped, etc. Can I embed little icons into the timeline text to indicate "project"/"person"/"policy", etc?

In theory, yes, but I don't think we do that with any other transaction type so it might cause a bunch of weird layout issues or something like that. You can try just rendering something like this and see what it looks like:

phutil_tag(
  'span', 
  array(
    'class' => 'timeline-policy',
  ),
  array(
    id(new PHUIIconView())->setIcon('fa-user'),
    $actual_policy_name,
  ));

(There might be a better CSS class name to use, similar to other related CSS.)

If that looks OK, I think Policy objects and Policy handles have an "icon" property.

We could also do a hovercard or tooltip or something.

¿

Current:

epriestley changed the "Can Use Application" policy from "All Users" to "Phacility".

Proposed:

epriestley changed the "Can Use Application" policy from " All Users" to " Phacility".

This doesn't look too awful, but I'll leave it for sharper minds to improve the spacing between the icon and the text:

In D17757#214604, @epriestley wrote:

The "simultate" stuff should be unnecessary with the move to proper transactions: they already have code which prevents you from setting invalid policies on things (specifically, policies which would prevent you from viewing or editing the object). I'm not totally sure if it will work as-is, but in theory the Editor should already prevent you from setting the "Can View Application" policy to something which excludes you. If it allows you to make this change, let me know and I can see what's going on.

Doesn't seem to work out of the box. Without putting any code in validateTransactions(), I can set the "View" policy on the Badges application to "No One" without being stopped. I end up at the "You Shall Not Pass: Restricted Application" dialogue when trying to view the config.

Let's just move forward without that check and then I can counter-diff with whatever dark magic we need to get it working. It's probably something simple but I'm not sure offhand.

This is all working except for the issue where the new value for a policy can be null.

New UI:

To tackle the null thing first, I think there are two issues:

First, we try to edit every capability the object has, even capabilities which should not be editable. Here, notably, this includes CAN_EDIT, which is always locked to "Administrators". We check for this correctly when building the form (to figure out whether the UI will show an editable control or not) by calling:

$can_edit = $application->isCapabilityEditable($capability);

...but we don't perform a similar check before trying to actually apply the edit. To fix this, I think:

• Since we're applying the edit from within the transaction, let's move the validation logic (the stuff between $request->getStr('policy:'.$capability); and where we build the transaction) to validateTransactions() on the TransactionType.
• validateTransactions() should read the 'capability.name' metadata from each transaction and make sure that the capability is valid (exists in $application->getCapabilities()) and editable (isCapablityEditable()), and raise an "invalid" error if it isn't. If you try to do a null edit after this change, we should get a proper error.
• Then, the $request->getStr(...) loop should test that capabilities are editable before building transactions for them, so only valid capability edits get submitted to the editor.

Second, the logic around PhabricatorPolicyQuery is currently constructed incorrectly, because PhabricatorPolicyQuery always loads some policy object -- it's just possible that the object it loads is an invalid/incomplete/restricted object. Specifically, this condition will never be met because $policy will always be a valid object:

if (!$policy) {
  // Not a custom policy either. Can't set the policy to something
  // invalid, so skip this.
  continue;
}

99% of the time, I think this (always loading an object) is the right behavior for policies and just makes things work properly, but in this case we need to do a little more work.

To step back slightly, for Handles, we have two general classes of "invalid" object:

• Filtered/Restricted: The PHID is valid, but the viewer doesn't have permission to see it (for example, we tried to load the handle for a project you can't see). This usually renders like "Restricted Project". (Viewers are allowed to know that something they can't see exists, they just aren't allowed to know any of the content.)
• Incomplete: We failed to load the object completely. This happens if you, e.g., load "cat" as a PHID or something like that. This usually renders as "Unknown Object".

In Policies, we don't currently make this distinction. Some day, we possibly should, but for now we can just lump these into one "invalid" bucket since neither is valid to select when editing something.

In the normal case (for example, when you set "Visible To" on a task), we validate policies implicitly by making sure that the user still satisfies the policy check. If the user provides an invalid policy (like null or "cat" or the PHID of something they can't see) they'll fail the check anyway, so we don't need to explicitly do a validity test.

However, it's legitimate to set a policy like "Default task view policy" to something which excludes you (for example, if I set it to "Default View Policy: Only user alincoln" that's silly, but perfectly fine) so we can't rely on this same check. Instead, I think we can do this:

• Per above, move this validation logic to the TransactionType.
• Add a isValidPolicyForEdit() or similar method to PhabricatorPolicy, and just have it return $this->getType() !== PhabricatorPolicyType::TYPE_MASKED;.
• Test each policy by calling that method, raising an invalid error if the policy isn't valid.

This will make it easier to refine "restricted" vs "incomplete" policies later if we want by putting the logic in one place and making it relatively grep-able.

One way to to test this is to right-click/control-click the form and edit the hidden input for a control to read "cat" in the inspector:

When you submit the form, you should get an "invalid policy" error.

Sort that out first and then we can look at the issue with setting "Can Use Application" to something which excludes you.

Just because I dug into this: I think the reason we aren't throwing "can't lock yourself out" errors is because PhabricatorApplicationTransactionEditor is only invoking validatePolicyTransaction() for transaction objects that were constructed with ->setTransactionType(PhabricatorTransactions::TYPE_EDIT_POLICY) (or similar).

If I change PhabricatorApplicationEditController to construct xactions with either TYPE_EDIT_POLICY or TYPE_VIEW_POLICY, I trigger the expected validation errors.

User austin viewing the timeline after bob switched to a project members policy that austin isn't a member of (and a transaction at the end that attempts to change the transaction to 'cat'):

User bob viewing the same timeline:

This looks good and the explanation about CAN_VIEW makes perfect sense, nice job hunting that down. I'm probably going to call it a night once the import/backup stuff settles but I'll give this a more detailed look tomorrow, try to break it, and see if I can come up with a clean approach to get the CAN_VIEW logic running on this pathway. Those screeshots look good, and I think I like the "User/Members" compromise -- feels more clear without looking weird to me.

On master, if you try and change the view policy to something that prevent you from seeing it, you just get the unhandled exception dialogue:

I copy/pasted some code I found, and now it shows a little dialogue box:

Oh, I actually missed that the old old/new code was still around.

The transaction should get dropped automatically a little later on anyway, we just validate it first so we can give you a better error if you're doing something wrong. Depending on how the edit is being applied, we also sometimes tell you that the action will have no effect, explicitly.

An example of this is if you try to add a tag which an object already has by using the comment form (this isn't the greatest example, but conveys the general idea):

The general goal of this feature is to prevent surprises where two users assign a task to the same victim or add the same tag at the same time, and then end up with silly-looking similar comments or no-op transactions or an empty effect (all of which could happen long ago, and confused users).

Putting the $old == $new code in the validate block supports these things and lets the same code work for API, form, and comment cases.

Since we only have a form case right now and don't care about this fine-grained edit feedback there's no actual effect yet and your version is equally correct, but moving it into validation will improve behavior if we eventually expose this over the API, let you change policies in the comment form (seems very unlikely) or maaaaaybe add a more full-featured CLI tool (maybe a little more likely?). In this case these are all a bit far-fetched, of course, but for many edits they're more realistic.

Some minor excitement making this work until I found the typo. On the last line, it should be $capability, not $capablity:

In D17757#214905, @epriestley wrote:

I think something like this is reasonable to make the CAN_VIEW validation work, and it seems to work properly:

diff --git a/src/applications/meta/xactions/PhabricatorApplicationPolicyChangeTransaction.php b/src/applications/meta/xactions/PhabricatorApplicationPolicyChangeTransaction.php
index 50e4f15afa..7cb496fffe 100644
--- a/src/applications/meta/xactions/PhabricatorApplicationPolicyChangeTransaction.php
+++ b/src/applications/meta/xactions/PhabricatorApplicationPolicyChangeTransaction.php
@@ -119,6 +119,40 @@ final class PhabricatorApplicationPolicyChangeTransaction
       }
     }
 
+    // If we're changing these policies, the viewer needs to still be able to
+    // view or edit the application under the new policy.
+    $validate_map = array(
+      PhabricatorPolicyCapability::CAN_VIEW,
+      PhabricatorPolicyCapability::CAN_EDIT,
+    );
+    $validate_map = array_fill_keys($validate_map, array());
+
+    foreach ($xactions as $xaction) {
+      $capability = $xaction->getMetadataValue(self::METADATA_ATTRIBUTE);
+      if (!isset($validate_map[$capability])) {
+        continue;
+      }
+
+      $validate_map[$capability][] = $xaction;
+    }
+
+    foreach ($validate_map as $capablity => $xactions) {

I'm going to put the old === new check back the way it was and land this, because in practice I couldn't break it and the current version is definitely broken (because old is always null).