Page MenuHomePhabricator

Badges v0.1
ClosedPublic

Authored by chad on Jul 13 2015, 5:33 AM.
Tags
None
Referenced Files
F12807365: D13626.id32924.diff
Wed, Mar 27, 7:27 PM
F12807152: D13626.id32960.diff
Wed, Mar 27, 7:05 PM
F12806481: D13626.id32977.diff
Wed, Mar 27, 6:18 PM
F12806379: D13626.id32928.diff
Wed, Mar 27, 6:11 PM
F12806336: D13626.id32961.diff
Wed, Mar 27, 6:08 PM
F12806306: D13626.id32984.diff
Wed, Mar 27, 6:06 PM
F12806300: D13626.id32919.diff
Wed, Mar 27, 6:06 PM
F12806297: D13626.id.diff
Wed, Mar 27, 6:06 PM
Tokens
"Mountain of Wealth" token, awarded by thoughtpolice."Yellow Medal" token, awarded by scp.

Details

Summary

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

Repository
rP Phabricator
Branch
badges-app
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 7250
Build 7542: [Placeholder Plan] Wait for 30 Seconds
Build 7541: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
  • Write translations for transactions

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

Some inlines, but they're like 95% style/most-modern-way-to-accomplish-things nitpicks. Generally looks solid to me.

src/applications/badges/application/PhabricatorBadgesApplication.php
64–71

These should have a 'template' property now I think, pointing at the PHID type.

src/applications/badges/capability/PhabricatorBadgesDefaultEditCapability.php
10

This is really "default edit policy", right?

13–15

(You probably can't ever be rejected for this.)

src/applications/badges/controller/PhabricatorBadgesEditController.php
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.

src/applications/badges/controller/PhabricatorBadgesEditIconController.php
12–14 ↗(On Diff #32962)

handlerequest / getViewer (see below)

src/applications/badges/controller/PhabricatorBadgesEditRecipientsController.php
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(...).

src/applications/badges/controller/PhabricatorBadgesListController.php
16 ↗(On Diff #32962)

handleRequest (see bleow)

src/applications/badges/controller/PhabricatorBadgesRemoveRecipientsController.php
6–14 ↗(On Diff #32962)

handleRequest() / getViewer() (see below)

64–67 ↗(On Diff #32962)

Unused.

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

src/applications/badges/controller/PhabricatorBadgesViewController.php
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.

src/applications/badges/editor/PhabricatorBadgesEditor.php
162

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.

src/applications/badges/interface/PhabricatorBadgesInterface.php
4

I don't think we need this, and can remove it.

src/applications/badges/phid/PhabricatorBadgesPHIDType.php
44

You can omit this if it's the same as setName().

49–72

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.

src/applications/badges/query/PhabricatorBadgesQuery.php
46–57

This should be simplified to:

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

Then implement newResultObject() (as return new PhabricatorBadge();).

58

Unused.

59–76

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.

74

This is fine as written, but could possibly be simplified a bit with getDestinationPHIDs() on the EdgeQuery.

85

This can be simplified slightly by implementing buildWhereClauseParts() instead, in modern code.

src/applications/badges/storage/PhabricatorBadge.php
4

This should more properly be called PhabricatorBadgesBadge.

93–95

This key is probably unnecessary.

src/applications/tokens/application/PhabricatorTokensApplication.php
14

Does this conflict with Fund now?

src/view/phui/PHUIObjectItemView.php
597 ↗(On Diff #32962)

Prefer // comments inside methods.

epriestley edited edge metadata.

wherps

This revision now requires changes to proceed.Jul 16 2015, 8:38 PM
chad edited edge metadata.
chad marked 22 inline comments as done.
  • Updates per comments
src/applications/badges/controller/PhabricatorBadgesEditRecipientsController.php
22–24 ↗(On Diff #32962)
src/applications/badges/controller/PhabricatorBadgesRemoveRecipientsController.php
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 added inline comments.
src/applications/badges/controller/PhabricatorBadgesEditRecipientsController.php
67 ↗(On Diff #32976)

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

src/applications/badges/controller/PhabricatorBadgesRemoveRecipientsController.php
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.

src/applications/badges/edge/PhabricatorBadgeHasRecipientEdgeType.php
6 ↗(On Diff #32976)

I made these up?

chad marked an inline comment as done.
chad edited edge metadata.
  • another round of updates
chad marked 3 inline comments as done.
  • Final Cleanup

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

src/applications/badges/controller/PhabricatorBadgesListController.php
6–14 ↗(On Diff #33008)

Keep allowPublic, nuke + getURIData

src/applications/badges/controller/PhabricatorBadgesRemoveRecipientsController.php
6–11 ↗(On Diff #33008)

Nuke / use $request->getURIData().

src/applications/badges/controller/PhabricatorBadgesViewController.php
7–14 ↗(On Diff #33008)

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

src/applications/badges/edge/PhabricatorBadgeHasRecipientEdgeType.php
7 ↗(On Diff #33008)

Oh, haha. Let me shoot you a diff for T6859, which will show the right values.

src/applications/badges/query/PhabricatorBadgesSearchEngine.php
19–31

Since you're using modern approaches (notably, you've implemented buildCustomSearchFields()), you should be able to just remove this method without anything breaking/changing.

src/applications/badges/storage/PhabricatorBadgesBadge.php
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 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.

src/applications/badges/application/PhabricatorBadgesApplication.php
67

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

src/applications/badges/controller/PhabricatorBadgesEditController.php
6–12 ↗(On Diff #33008)

Per elsewhere, prefer getURIData().

43 ↗(On Diff #33008)

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

src/applications/badges/controller/PhabricatorBadgesEditIconController.php
15 ↗(On Diff #33008)

getURIData()

src/applications/badges/controller/PhabricatorBadgesEditRecipientsController.php
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 edited edge metadata.
chad marked 17 inline comments as done.
  • Updates per feedback
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.