Page MenuHomePhabricator

Badges v0.1
ClosedPublic

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

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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

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.

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
13–15

handleRequest / getViewer() (see below)

51

Never used.

122–128

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

133

Never used.

135–136

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

src/applications/badges/controller/PhabricatorBadgesEditIconController.php
13–15

handlerequest / getViewer (see below)

src/applications/badges/controller/PhabricatorBadgesEditRecipientsController.php
13–15

handleRequest / getViewer (see below)

23–25

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

67

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

110

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
17

handleRequest (see bleow)

src/applications/badges/controller/PhabricatorBadgesRemoveRecipientsController.php
7–15

handleRequest() / getViewer() (see below)

65–68

Unused.

70

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

81

In modern code, just return $dialog;.

src/applications/badges/controller/PhabricatorBadgesViewController.php
17

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

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

134

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
3 ↗(On Diff #32962)

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
3 ↗(On Diff #32962)

This should more properly be called PhabricatorBadgesBadge.

92–94 ↗(On Diff #32962)

This key is probably unnecessary.

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

Does this conflict with Fund now?

src/view/phui/PHUIObjectItemView.php
597

Prefer // comments inside methods.

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

wherps

This revision now requires changes to proceed.Jul 16 2015, 8:38 PM
chad edited edge metadata.EditedJul 16 2015, 10:17 PM
chad marked 22 inline comments as done.
chad updated this revision to Diff 32976.
  • Updates per comments
chad added inline comments.Jul 16 2015, 10:20 PM
src/applications/badges/controller/PhabricatorBadgesEditRecipientsController.php
23–25
src/applications/badges/controller/PhabricatorBadgesRemoveRecipientsController.php
70

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

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

src/applications/badges/controller/PhabricatorBadgesRemoveRecipientsController.php
71

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
7

I made these up?

chad marked an inline comment as done.Jul 16 2015, 11:08 PM
chad edited edge metadata.
chad updated this revision to Diff 32977.
  • another round of updates
chad marked 3 inline comments as done.Jul 16 2015, 11:29 PM
chad updated this revision to Diff 32978.
  • 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...

src/applications/badges/controller/PhabricatorBadgesListController.php
7–15

Keep allowPublic, nuke + getURIData

src/applications/badges/controller/PhabricatorBadgesRemoveRecipientsController.php
7–12

Nuke / use $request->getURIData().

src/applications/badges/controller/PhabricatorBadgesViewController.php
8–15

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

src/applications/badges/edge/PhabricatorBadgeHasRecipientEdgeType.php
8

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
30–36

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.Jul 21 2015, 12:02 PM
epriestley requested changes to this revision.

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
7–13

Per elsewhere, prefer getURIData().

44

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

src/applications/badges/controller/PhabricatorBadgesEditIconController.php
16

getURIData()

src/applications/badges/controller/PhabricatorBadgesEditRecipientsController.php
7–11

Remove now that you're using getURIData().

This revision now requires changes to proceed.Jul 21 2015, 12:02 PM
chad edited edge metadata.Jul 21 2015, 4:19 PM
chad marked 17 inline comments as done.
chad updated this revision to Diff 33033.
  • Updates per feedback
epriestley edited edge metadata.Jul 22 2015, 8:26 PM
epriestley accepted this revision.
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.