Page MenuHomePhabricator

Phriction - add viewPolicy and editPolicy back-end data

Authored by btrahan on Nov 7 2014, 8:16 PM.



Ref T4029. this diff makes the pertinent database changes AND adds the migration script. This is important to get the data backend straightened away before we fully ship T4029. Next diff will expose the edit controls for these policies and whatever else work is needed to get that part done right.

Test Plan

made sure the lone project page on my wiki had a project with restrictive view policy. Post migration verified correct policy applied to this lone project page AND most open policy applied to the others

Diff Detail

rP Phabricator
Lint Not Applicable
Tests Not Applicable

Event Timeline

btrahan retitled this revision from to Phriction - add viewPolicy and editPolicy back-end data.
btrahan updated this object.
btrahan edited the test plan for this revision. (Show Details)
btrahan added a reviewer: epriestley.

do I need to add these to the PhrictionDocument schema definition?


ah, i need to default the policy to something in here... will update in a min or three.

add code to default view policy to *something*... note this does not maintain any project automagicalness.


bin/storage adjust will yell at you now if you screw up, so if you aren't sure you can just run that.

(If it gets it wrong, it's my fault.)

You don't need them for these -- we guess default types for anything named editPolicy, viewPolicy, maybe joinPolicy, anythingID, id, anythingPHID, any serialized column, and I think in a couple of other cases where the column type unambiguously follows from convention.

Okay, thanks! I guess maybe I should keep going on this diff or at least not check it in right away... I'm now thinking about things like how "stub" documents need to inherit the parent doc permissions at time of create. That could live in a future diff but it should be included at the same time this functionality here lands so people don't break stuff.

Yeah, we're getting into more-dangerous territory here. I'll look at this thoroughly in a sec and yell if I can come up with any issues.

epriestley edited edge metadata.

This seems good to me, but I agree that we should probably at least queue up things like stub document handling before landing this.

Things that would be good to get in before / shortly-after this lands, offhand:

  • Show policies when viewing documents, in the header (this could happen first).
  • Add shouldAllowPublic() if we don't already (can also happen first)?
  • Deal with stubs.
  • Do moves need special handling?
  • Remove the magical stuff from Projects? Probably? Like the automatic wiki link. Or generally do something with it, since without changes it will have confusing / different behavior than it does today.
  • Should a document start with the policies of its parent by default?

Basically, make sure we have a fairly cohesive approach in the works. I haven't thought through exactly how all the details should work but a lot of them are probably fairly obvious (e.g., only have one reasonable answer).

For example, if we default documents to the policies of their (nearest, existing) parent, which I think is reasonable, that also provides a sensible default for all the stubs.


I don't think this is currently true. You have to be able to view all of the parents, but do not need to be able to edit them, I think.

I believe there are two messy cases here:

  • It should be possible to let a user edit /personal/epriestley/ without letting them edit /.
  • You should be able to delete (and maybe move?) /x/y/ if you can edit it, even if you can't edit /x/y/z/.

Moves add some more magic to this.

This revision is now accepted and ready to land.Nov 7 2014, 8:46 PM
btrahan edited edge metadata.

update text to say you need view permission on parent docs to edit

(other bits will come in a new diff)

make default view policy most open possible and default edit policy to USERs as default edit policy of public would be badsauce


This one too.

fix the other spot we set policy
start showing the policy on the header (I just didnt feel like merging my "progress" forward so adding it here.)

This revision was automatically updated to reflect the committed changes.