Page MenuHomePhabricator

Allow awarding Badges from the profile
ClosedPublic

Authored by chad on Mar 29 2016, 3:03 AM.

Details

Summary

[WIP] Allows awarding a badge from a user profile. Unsure of the interactions here if a user can't award any badges, or if we should just hide this.

Fixes T10688
Fixes T10318

Test Plan

Award some badges. Steal them back.

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

chad updated this revision to Diff 37477.Mar 29 2016, 3:03 AM
chad retitled this revision from to Allow awarding Badges from the profile.
chad updated this object.
chad edited the test plan for this revision. (Show Details)
chad added reviewers: epriestley, lpriestley.
chad added inline comments.Mar 29 2016, 3:08 AM
src/applications/badges/controller/PhabricatorBadgesAwardController.php
67

filter out awarded badges?

epriestley edited edge metadata.Mar 29 2016, 12:55 PM

I think the ideal interaction here is probably having a tokenizer datasource which customizes responses for the actor/user, so archived/awarded/not-awardable badges show up, but produce disabled tokens with notes like "Archived", "Already Awarded" and "No Permission to Award".

Then the user should be able to select them in the UI (no "where is the thing??!" confusion), but clearly understand why they won't work.

That doesn't need to happen in this diff, but I'd probably just keep showing all badges in the dropdown until we get around to refining it?


If you want to build the datasource, DrydockBlueprintDatasource and AlmanacServiceDatasource might be helpful to look at. The interaction would basically be:

  • Create a new Datasource class for badges (PhabricatorBadgesDatasource or similar).
  • Like DrydockBlueprintDatasource, it marks archived badges as "Archived".
  • Like AlmanacServiceDatasource, it takes a parameter to customize results.

In this case, it would take an awardToPHID parameter or similar, and you'd pass the PHID of the user being awarded badges. That would make the datasource change behavior:

  • Load that user's badges.
  • Mark any badges they already have as closed with a note like "Already Awarded".
  • Mark any badges the viewer can't edit as closed with a note like "No Permission to Award".

You should be able to visit /typeahead/class/?class=XXX&awardToPHID=YYY to test that the datasource works, then adjust the dialog to use it.


I think the only bad thing about this diff is the exception when a user double-awards a badge. An easy fix for that would be to just let the award go through -- we could refine that later. To do that, you should be able to put a try / catch around saving the award in PhabricatorBadgesEditor, near line 145.

$award->save();
$awards[] = $award;

If you try/catch that, catch AphrontDuplicateKeyQueryException, and just ignore it, that should give us reasonable overall behavior.

It will incorrectly write a transaction claiming that you awarded the badge, but that's not a big deal (that's purely cosmetic and can be fixed later).

src/applications/badges/storage/PhabricatorBadgesTransaction.php
212–223 ↗(On Diff #37477)

This stuff should be fixed elsewhere via T10688 -- real root cause is a translation data error.

There's no withName() sort of method on BadgesQuery right now but you can actually ignore that and everything will work fine for installs with fewer than 100 badges. We can add ngrams + querying in a future diff to page/filter more gracefully.

chad planned changes to this revision.Mar 29 2016, 4:42 PM
chad added a comment.Mar 31 2016, 8:26 PM

Not sure how to trigger the exception you mentioned, tried double awards and it just goes through like it would on a Badges Page.

chad updated this revision to Diff 37498.Mar 31 2016, 8:27 PM
chad edited edge metadata.
  • revert translation changes

Oh! That was probably me making stuff up, I think I had an old key on the table from locally patching an earlier version of the badge award migration. Sorry about that, let me take a look at this verison.

Yeah, behavior is fine. I just had a junk key.

epriestley accepted this revision.Mar 31 2016, 8:32 PM
epriestley edited edge metadata.

I think this is fine as-is if you want to kick it in. The datasource stuff would be nice at some point, but that's a good chunk of work and could reasonably wait until T10690.

This revision is now accepted and ready to land.Mar 31 2016, 8:32 PM
chad added a comment.Mar 31 2016, 8:37 PM

yeah i'd prefer a typeahead at some point. this is low tech, but very efficient

This revision was automatically updated to reflect the committed changes.