Fixes T9019. Adds a project.members object policy rule, similar to the existing subscriptions.subscribers object policy rule.
Details
Details
- Reviewers
joshuaspence - Group Reviewers
Blessed Reviewers - Maniphest Tasks
- T9019: Create a "Project Members" object policy for Projects
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
Diff Detail
- Repository
- rP Phabricator
- Branch
- master
- Lint
Lint Passed - Unit
Tests Passed - Build Status
Buildable 8828 Build 10301: Run Core Tests Build 10300: arc lint + arc unit
Event Timeline
Comment Actions
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.
Comment Actions
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 ↗ | (On Diff #35041) | Maybe "...can only operate on tasks". |
| src/applications/subscriptions/policyrule/PhabricatorSubscriptionsSubscribersPolicyRule.php | ||
| 105–119 ↗ | (On Diff #35041) | 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(). |
Comment Actions
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.
Comment Actions
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).