Page MenuHomePhabricator

Make "Can Interact" and logged-out users interact more gracefully
ClosedPublic

Authored by epriestley on Mar 9 2017, 4:46 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Dec 24, 9:49 AM
Unknown Object (File)
Fri, Dec 13, 6:48 PM
Unknown Object (File)
Fri, Dec 6, 7:18 PM
Unknown Object (File)
Fri, Dec 6, 8:02 AM
Unknown Object (File)
Fri, Nov 29, 10:27 PM
Unknown Object (File)
Nov 24 2024, 8:53 PM
Unknown Object (File)
Nov 23 2024, 7:08 AM
Unknown Object (File)
Nov 21 2024, 1:37 AM
Subscribers
None

Details

Summary

Fixes T12378. Two minor issues here:

  • CAN_INTERACT on tasks uses "USER", but should just use the view policy, which may be more permissive ("PUBLIC").
  • CAN_INTERACT is currently prevented from being "PUBLIC" by additional safeguards. Define an explicit capability object for the permission which returns true from shouldAllowPublicPolicySetting().
Test Plan
  • Viewed an unlocked task as a logged-out user, saw "login to comment" instead of "locked".
  • Viewed a locked task as a logged-out user, saw "locked".

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

src/applications/policy/capability/PhabricatorPolicyCanInteractCapability.php
14

We can't reasonably specialize this string, but it won't currently ever appear in the UI.

This revision is now accepted and ready to land.Mar 9 2017, 4:48 PM

I had mistakenly believed this block was handling the "log in to comment" case, but it doesn't:

https://secure.phabricator.com/source/phabricator/browse/master/src/applications/transactions/editengine/PhabricatorEditEngine.php;4c7d464f8bf2627ef7455596fde2ae5667e67a57$1565-1570

The block already has a comment describing the case it does handle, so I think this was just carelessness on my part and don't see any technical change we can make to make this more clear.

We could add CAN_INTERACT tests to the test suite to shore this up a little bit, but I think the feature is potentially still in flux so I want to wait to pursue that.

This revision was automatically updated to reflect the committed changes.