Page MenuHomePhabricator

Add a "members of project" policy rule
AbandonedPublic

Authored by epriestley on Nov 12 2015, 8:06 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Mar 20, 4:00 PM
Unknown Object (File)
Wed, Mar 20, 3:20 PM
Unknown Object (File)
Tue, Mar 19, 9:19 PM
Unknown Object (File)
Tue, Mar 19, 9:08 PM
Unknown Object (File)
Tue, Mar 19, 8:04 PM
Unknown Object (File)
Tue, Mar 19, 7:57 PM
Unknown Object (File)
Tue, Mar 19, 7:49 PM
Unknown Object (File)
Mon, Mar 18, 4:02 PM
Subscribers

Details

Summary

Fixes T9019. Adds a project.members object policy rule, similar to the existing subscriptions.subscribers object policy rule.

Test Plan

Created a project with Edit Policy: Members. Tried to edit the project without membership and hit "You shall not passed". Joined the project and was able to edit project details.

Diff Detail

Repository
rP Phabricator
Branch
master
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 8851
Build 10342: Run Core Tests
Build 10341: arc lint + arc unit

Event Timeline

joshuaspence retitled this revision from to Add a "members of project" policy rule.
joshuaspence updated this object.
joshuaspence edited the test plan for this revision. (Show Details)
joshuaspence added a reviewer: epriestley.

One issue here is that you can't create a new project with the edit policy of "Members", specifically:

You can not select this edit policy, because you would no longer be able to edit the object.

I'm not entirely sure how to fix this.

src/applications/project/controller/PhabricatorProjectProfileController.php
95–97

Unrelated minor fix.

src/applications/project/policyrule/PhabricatorProjectProjectMembersPolicyRule.php
3–4

I renamed this class to make it a little more clear.

epriestley edited edge metadata.

The phabricator_policy.policy table encodes rules by using class names. As written, this patch changes a class name but does not migrate rules, so I expect it to break all existing rules which use the "Project Members" rule.

I'm not entirely sure how to fix [creating a project with this rule].

It's pretty messy right now (and I don't really have any ideas on how to get out of it cleanly). The getTransactionHint() mechanism is used to tell rules about what state we expect after transactions apply, so we can validate the expected state. PhabricatorSubscriptionsSubscribersPolicyRule and PhabricatorApplicationTransactionEditor->adjustObjectForPolicyChecks() have examples of how the "Subscribers" rule currently accomplishes this. You'll need to do something similar (or come up with a better approach to this problem -- the current one is admittedly pretty gross).

src/applications/policy/rule/PhabricatorPolicyRule.php
39

Maybe "...can only operate on tasks".

src/applications/subscriptions/policyrule/PhabricatorSubscriptionsSubscribersPolicyRule.php
105–119

Minor, but I think these classes read slightly better if these short, descriptive methods are at the top of the file. If you're seeking to understand what a class does, these methods are more informative at a glance than methods like applyRule().

This revision now requires changes to proceed.Nov 15 2015, 4:04 PM
joshuaspence edited edge metadata.
joshuaspence marked an inline comment as done.
  • Rebase
  • Add migration
epriestley edited reviewers, added: joshuaspence; removed: epriestley.

I can take a crack at navigating the "create a project with this policy" issue, I think I want this policy soon-ish to do T6502.

Split into D14868 + D14869. I didn't pick up the rule rename. Given that it needs a migration, I'm not sure it's really worth it (if we're going to migrate, maybe we'd be better moving all rules to constants? I think constants have pretty clearly won the day over class names in terms of "stuff we stick in the database" in modern code).