Details
- Reviewers
epriestley lpriestley - Maniphest Tasks
- T10318: Make it easier to award badges
T10688: Badge translation error - Commits
- rP2386705873bd: Allow awarding Badges from the profile
Award some badges. Steal them back.
Diff Detail
- Repository
- rP Phabricator
- Branch
- arcpatch-D15544
- Lint
Lint Passed - Unit
Tests Passed - Build Status
Buildable 11363 Build 14145: Run Core Tests Build 14144: arc lint + arc unit
Event Timeline
src/applications/badges/controller/PhabricatorBadgesAwardController.php | ||
---|---|---|
67 | filter out awarded badges? |
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.
Not sure how to trigger the exception you mentioned, tried double awards and it just goes through like it would on a Badges Page.
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.
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.