Make policy violation dialog more flexible
ClosedPublic
Actions

Authored by epriestley on Jun 4 2015, 7:49 PM.

Details

Reviewers

btrahan

Maniphest Tasks

T8424: Integrate Spaces into the core policy infrastructure

Commits

Restricted Diffusion Commit
rPe595478f1dff: Make policy violation dialog more flexible

Summary

Ref T8424. When users are rejected because they can't see the space an object is in, this isn't really a capability rejection. Don't require a capability when rejecting objects.

This mostly simplifies upcoming changes.

Test Plan

Viewed a capability exception dialog, it looked the same as always.
(After additional changes, viewed a space exception dialog.)

Diff Detail

Repository

rP Phabricator

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

epriestley updated this revision to Diff 31793.Jun 4 2015, 7:49 PM

epriestley retitled this revision from to Make policy violation dialog more flexible.

epriestley updated this object.

epriestley edited the test plan for this revision. (Show Details)

epriestley added a reviewer: btrahan.

epriestley added a task: T8424: Integrate Spaces into the core policy infrastructure.

Herald added a subscriber: epriestley. · View Herald TranscriptJun 4 2015, 7:49 PM

btrahan accepted this revision.Jun 4 2015, 9:50 PM

btrahan edited edge metadata.

btrahan added inline comments.

src/applications/policy/filter/PhabricatorPolicyFilter.php
656	I don't think this really matters / I assume what's actually getting passed in here makes sense, but when I was wondering about correctness I looked up PhabricatorPolicyInterface and it doesn't have getPHID. Ergo / I guess at this anyway, isn't a method_exists sort of check maybe? Part of this is I think getPHID() usually returns something unless the object hasn't been saved yet.

This revision is now accepted and ready to land.Jun 4 2015, 9:50 PM

epriestley added inline comments.Jun 4 2015, 10:31 PM

src/applications/policy/filter/PhabricatorPolicyFilter.php
656	`PhabricatorPolicyInterface` extends `PhabricatorPHIDInterface` which does guarantee that `getPHID()` exists, so this shouldn't ever fatal, at least. However, this is probably at least somewhat wrong/sketchy/out-of-date; I don't know which types of objects the comment was talking about offhand. Two cases I can think of: New objects getting policy checked by TransactionEditor before they're saved. PhabricatorPolicyTestObjects in unit tests.