Page MenuHomePhabricator

Add a "members of project" policy rule

Authored by epriestley on Nov 12 2015, 8:06 AM.
Referenced Files
Unknown Object (File)
Mon, Mar 27, 3:17 AM
Unknown Object (File)
Sun, Mar 5, 4:14 PM
Unknown Object (File)
Feb 25 2023, 1:27 AM
Unknown Object (File)
Feb 24 2023, 3:23 AM
Unknown Object (File)
Feb 19 2023, 6:31 PM
Unknown Object (File)
Feb 10 2023, 9:50 AM
Unknown Object (File)
Jan 31 2023, 3:50 PM
Unknown Object (File)
Jan 30 2023, 2:06 PM



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

rP Phabricator
Lint Passed
Tests Skipped
Build Status
Buildable 8830
Build 10305: Run Core Tests
Build 10304: 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.


Unrelated minor fix.


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).


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


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).