Basic plumbing for Badges application.
- You can make Badges.
- You can look at a list of them.
- They can be edited.
- They can be assigned to people.
- You can revoke them from people.
- You can subscribe to them.
Basic plumbing for Badges application.
Make Badges with various options. Give them to people. Take them away from people.
Some inlines, but they're like 95% style/most-modern-way-to-accomplish-things nitpicks. Generally looks solid to me.
These should have a 'template' property now I think, pointing at the PHID type.
This is really "default edit policy", right?
(You probably can't ever be rejected for this.)
handleRequest / getViewer() (see below)
This is inconsistent with other applications, and should always redirect to the badge detail page (with ID determined by $badge->getID()).
These should just be set on the controls instead of on the object.
handlerequest / getViewer (see below)
handleRequest / getViewer (see below)
I think this controller does not enforce policies correctly.
Although it changes the form based on the user's edit permissions, it does not enforce that a user must actually have permission in order to perform edits. An attacker can just edit the HTML of the form on the client, then submit whatever parameters they want, removing other users' badges.
The actual edits need to have the checks applied to them.
(Or maybe I'm misunderstanding, I haven't actually patched this locally.)
Prefer $handles = $viewer->loadHandles($phids) in modern code.
Prefer $this->getApplicationURI('x') over $this->getApplicationURI().'x'. The former is more flexible, since we can, e.g., correct or verify URIs inside getApplicationURI(...).
handleRequest (see bleow)
handleRequest() / getViewer() (see below)
In modern code, this can be simplified as $this->newDialog(), which automatically calls setUser() for you.
In modern code, just return $dialog;.
Prefer to implement handleRequest(AphrontRequest $request) in modern code, instead.
Then get id with $request->getURIData('id').
Prefer $this->getViewer() to get the viewer.
Using the PHID or "Badge 123" is better here, so the thread doesn't break if the badge is renamed. This header isn't visible to users, it's only used by (some) mail clients to do mail threading.
I don't think we need this, and can remove it.
You can omit this if it's the same as setName().
Since this PHIDType does not implement canLoadNamedObject() (and should not -- I think badges aren't core/important enough to get monograms), this method will never be called. You can just remove it.
This should be simplified to:
Then implement newResultObject() (as return new PhabricatorBadge();).
This load+attach stuff should happen in didFilterPage(), so we don't need to load data for objects we're going to throw away a second later when we do policy filtering.
This should also only happen if needRecipients() is set. Otherwise, we'll load all these edges every time, which might be a huge amount of data if there are some very common badges with thousands of recipients.
This is fine as written, but could possibly be simplified a bit with getDestinationPHIDs() on the EdgeQuery.
This can be simplified slightly by implementing buildWhereClauseParts() instead, in modern code.
This should more properly be called PhabricatorBadgesBadge.
This key is probably unnecessary.
Does this conflict with Fund now?
Prefer // comments inside methods.
Worth noting then that this is copy/pastey from Project Membership. Not sure what you want to to change here (iNoob).
Can't see to get $this->newDialog() to work, crashes and I get:
EXCEPTION: (PhabricatorFileStorageConfigurationException) Malformed local disk storage root. You must provide an absolute path, and can not use '/' as the root. at [<phabricator>/src/applications/files/engine/PhabricatorLocalDiskFileStorageEngine.php:104]
All other dialogs see fine. I'll look more into next upate.
I couldn't get that to work (returns different results).
I fixed this error, but the Dialog still won't submit for me with $this->newDialog(). Nothing now in the logs.
I made these up?
Partial, lemme fix the EdgeType thing so we can get the right value...
Keep allowPublic, nuke + getURIData
Nuke / use $request->getURIData().
Per elsewhere, you can remove the $id stuff now.
Oh, haha. Let me shoot you a diff for T6859, which will show the right values.
Since you're using modern approaches (notably, you've implemented buildCustomSearchFields()), you should be able to just remove this method without anything breaking/changing.
|29–35 ↗||(On Diff #33008)|
We might want to make these numeric so you can ORDER BY them, although that's more involved since the qualities would no longer be meaningful on their own.
Handful of minor/cleanup stuff.
I can counterdiff you for the controller thing after this lands, the Project controller might need fixing too.
Oh, sorry -- this one shouldn't have a 'template'.
The idea is the policies which act as object defaults (e.g., "default view", "default edit") get a template parameter, which tells the rest of the system that object-specific policies for that object type are appropriate values. In Applications → Badges → Help/Options → Edit Policies, object policies will show up in the dropdowns and be selectable.
For example, in the future, we might define a custom object policy like "Badge Author", and make it the default edit policy. We do something similar in Passphrase today. The "template" parameter tells the system that object-specific policies (like "Room Members" in Conpherence, "Task Author" in Maniphest, "Credential Author" in Passphrase, etc) are appropriate.
It would make sense to select "Badge Author" as the "Default Edit Policy", and also make sense (from a technical point of view, at least -- maybe not actually be useful) to select "Badge Author" as the "Default View Policy".
However, it doesn't make sense to select "Badge Author" as the "can create" policy, because the "can create" policy is evaluated before a badge is created, so there's no badge yet and also no badge author (the object the policy is evaluated against will be the Application, not a Badge).
Per elsewhere, prefer getURIData().
Prefer getApplicationURI('x') over getApplicationURI().'x'.
Remove now that you're using getURIData().
To follow up on the policy issue, Projects have unusual policies that are enforced in the Editor because you're always allowed to leave a project if you're a member, and you can join projects if you have CAN_JOIN, even if you don't have CAN_EDIT, and the page also serves as a member list.
So everything appears fine policy-wise, Projects is just a little weird because of the policy complexity.