Page MenuHomePhabricator

Badges v0.1

Authored by chad on Jul 13 2015, 5:33 AM.



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.
Test Plan

Make Badges with various options. Give them to people. Take them away from people.

Diff Detail

rP Phabricator
Lint OK
Unit Tests OK
Build Status
Buildable 7254
Build 7550: [Placeholder Plan] Wait for 30 Seconds
Build 7549: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
chad added a comment.Jul 16 2015, 3:51 AM
  • Write translations for transactions

This is one big pile of diff. I like it.

epriestley edited edge metadata.Jul 16 2015, 8:37 PM

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

12–14 ↗(On Diff #32962)

handleRequest / getViewer() (see below)

50 ↗(On Diff #32962)

Never used.

121–127 ↗(On Diff #32962)

This is inconsistent with other applications, and should always redirect to the badge detail page (with ID determined by $badge->getID()).

132 ↗(On Diff #32962)

Never used.

134–135 ↗(On Diff #32962)

These should just be set on the controls instead of on the object.

12–14 ↗(On Diff #32962)

handlerequest / getViewer (see below)

12–14 ↗(On Diff #32962)

handleRequest / getViewer (see below)

22–24 ↗(On Diff #32962)

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

66 ↗(On Diff #32962)

Prefer $handles = $viewer->loadHandles($phids) in modern code.

109 ↗(On Diff #32962)

Prefer $this->getApplicationURI('x') over $this->getApplicationURI().'x'. The former is more flexible, since we can, e.g., correct or verify URIs inside getApplicationURI(...).

16 ↗(On Diff #32962)

handleRequest (see bleow)

6–14 ↗(On Diff #32962)

handleRequest() / getViewer() (see below)

64–67 ↗(On Diff #32962)


69 ↗(On Diff #32962)

In modern code, this can be simplified as $this->newDialog(), which automatically calls setUser() for you.

80 ↗(On Diff #32962)

In modern code, just return $dialog;.

16 ↗(On Diff #32962)

Prefer to implement handleRequest(AphrontRequest $request) in modern code, instead.

Then get id with $request->getURIData('id').

133 ↗(On Diff #32962)

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:

return $this->loadStandardPage($this->newResultObject());

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?

597 ↗(On Diff #32962)

Prefer // comments inside methods.

epriestley requested changes to this revision.Jul 16 2015, 8:38 PM
epriestley edited edge metadata.


This revision now requires changes to proceed.Jul 16 2015, 8:38 PM
chad updated this revision to Diff 32976.EditedJul 16 2015, 10:17 PM
chad edited edge metadata.
chad marked 22 inline comments as done.
  • Updates per comments
chad added inline comments.Jul 16 2015, 10:20 PM
22–24 ↗(On Diff #32962)
69 ↗(On Diff #32962)

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.

chad marked 3 inline comments as done.Jul 16 2015, 10:57 PM
chad added inline comments.
67 ↗(On Diff #32976)

I couldn't get that to work (returns different results).

70 ↗(On Diff #32976)

I fixed this error, but the Dialog still won't submit for me with $this->newDialog(). Nothing now in the logs.

6 ↗(On Diff #32976)

I made these up?

chad updated this revision to Diff 32977.Jul 16 2015, 11:08 PM
chad marked an inline comment as done.
chad edited edge metadata.
  • another round of updates
chad updated this revision to Diff 32978.Jul 16 2015, 11:29 PM
chad marked 3 inline comments as done.
  • Final Cleanup
chad updated this revision to Diff 32984.Jul 17 2015, 3:08 AM
  • Add Search by Quality
chad updated this revision to Diff 32990.Jul 17 2015, 4:01 PM
  • Rebase
chad updated this revision to Diff 33008.Jul 18 2015, 8:22 PM
  • Make Badges Flaggable
scp awarded a token.Jul 19 2015, 9:36 PM

Partial, lemme fix the EdgeType thing so we can get the right value...

6–14 ↗(On Diff #33008)

Keep allowPublic, nuke + getURIData

6–11 ↗(On Diff #33008)

Nuke / use $request->getURIData().

7–14 ↗(On Diff #33008)

Per elsewhere, you can remove the $id stuff now.

7 ↗(On Diff #33008)

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.

After D13662, ConfigEdge Types shows 58 + 59 as the next two unused IDs.

epriestley requested changes to this revision.Jul 21 2015, 12:02 PM
epriestley edited edge metadata.

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 ApplicationsBadgesHelp/OptionsEdit 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).

6–12 ↗(On Diff #33008)

Per elsewhere, prefer getURIData().

43 ↗(On Diff #33008)

Prefer getApplicationURI('x') over getApplicationURI().'x'.

15 ↗(On Diff #33008)


6–10 ↗(On Diff #33008)

Remove now that you're using getURIData().

This revision now requires changes to proceed.Jul 21 2015, 12:02 PM
chad updated this revision to Diff 33033.Jul 21 2015, 4:19 PM
chad edited edge metadata.
chad marked 17 inline comments as done.
  • Updates per feedback
epriestley accepted this revision.Jul 22 2015, 8:26 PM
epriestley edited edge metadata.
This revision is now accepted and ready to land.Jul 22 2015, 8:26 PM
This revision was automatically updated to reflect the committed changes.

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.