Ref T8996, Convert badge recipients from Edges to actual BadgeAward objects
Details
Details
- Reviewers
epriestley - Group Reviewers
Blessed Reviewers - Maniphest Tasks
- T8996: Move Badge awards to be an actual object, not an edge
- Commits
- rP0330ea575db7: Converting badge recipients from Edge to BadgeAward table
Create badge, award it to recipient. Make sure adding/removing recipients works. (Still need to migrate exisiting recipients to new table and need to create activity feed blurbs)
Diff Detail
Diff Detail
- Repository
- rP Phabricator
- Branch
- badgesaward
- Lint
Lint Passed - Unit
Tests Passed - Build Status
Buildable 11278 Build 14016: Run Core Tests Build 14015: arc lint + arc unit
Event Timeline
Comment Actions
Nice, this looks good. I caught a few things inline (mostly style/modernization stuff). Once this is ready to go I'll write you the migration for it.
| src/applications/badges/controller/PhabricatorBadgesEditRecipientsController.php | ||
|---|---|---|
| 29–30 | I think this won't work -- if something is being removed, we need to apply two separate transactions (one TYPE_AWARD to award badges, one TYPE_REVOKE to revoke them). But I'm also not sure if this code is reachable, so we might be able to just delete it | |
| src/applications/badges/editor/PhabricatorBadgesEditor.php | ||
| 124 | It would be better for this transaction type to take a list of PHIDs instead of a single PHID, so future UI or API callers can bulk-remove a bunch of awards at once. | |
| 128–130 | For consistency with TYPE_AWARD, you might want to remove the deleted awards from the list and then re-attach the new, smaller list with attachAwards(), so the attached awards are always up to date. | |
| src/applications/badges/query/PhabricatorBadgesAwardQuery.php | ||
| 22 | I don't understand the [1] here, and it seems like it shouldn't work? | |
| 49–62 | Prefer to implement newResultObject() and then implement loadPage() as: return $this->loadStandardPage($this->newResultObject()); This has the same effect, but is a little simpler. | |
| 64 | There's a slightly newer style for these which is preferred -- you should implement buildWhereClauseParts() instead of buildWhereClause(). You can search for other implementations. The implementation is nearly the same, you just don't have to call buildPagingClause() or formatWhereClause(), and the infrastructure can do some other kinds of work for you (which aren't relevant here) automatically. | |
| src/applications/badges/query/PhabricatorBadgesQuery.php | ||
| 62–64 | You should be able to omit either the array() default to idx() or the explicit === null check, since they both do the same thing (give you an array() value if the key is missing). | |
| src/applications/badges/storage/PhabricatorBadgesAward.php | ||
| 17 | I think this can have a PhabricatorBadgesBadge typehint? | |
| 19–22 | $app appears unused. | |
| 60–66 | BadgesBadge should have new code in its destroyObjectPermanently() method to remove all awards, too (so when you destroy a badge, you destroy all the awards of that badge). | |
| src/applications/badges/query/PhabricatorBadgesAwardQuery.php | ||
|---|---|---|
| 22 | Otherwise I get a: Argument 1 passed to PhabricatorBadgesAward::attachBadge() must be an instance of PhabricatorBadgesBadge, array given, called in /Users/lkassianik/phacility/phabricator/src/applications/badges/query/PhabricatorBadgesAwardQuery.php on line 27 | |
| src/applications/badges/query/PhabricatorBadgesAwardQuery.php | ||
|---|---|---|
| 18 | Aha! Probably rename $query to $badges, then this should be mpull($badges, null, 'getPHID'), not mgroup(...). That should solve the [1] issue. | |
Comment Actions
This looks great. I caught a few more little things, but looks solid otherwise.
| resources/sql/autopatches/20160102.badges.award.sql | ||
|---|---|---|
| 9–10 | We should also have a separate key on just recipientPHID. Maybe call this key_badge and the other one key_recipient: UNIQUE KEY `key_badge` (badgePHID, recipientPHID), KEY `key_recipient` (recipientPHID) We issue two types of queries: on the badge page, we issue ... WHERE badgePHID = X to find all awards of a particular badge. On profile pages, we issue ... WHERE recipientPHID = X to find all badges a particular user has been awarded. MySQL can use any prefix of a key to improve query performance, but the <badgePHID, recipientPHID> key doesn't have a useful prefix for optimizing the recipientPHID = X case. Adding a second key will let both kinds of queries use a key, and execute quickly. | |
| src/applications/badges/editor/PhabricatorBadgesEditor.php | ||
| 50 | I'm surprised this works if you award a badge which has already been awarded to at least one user. I'd expect this to return a list of objects, which can not be JSON serialized and thus can not be stored in the database, so I'd expect this to produce an exception like "Unable to JSON encode transaction old value" or similar (possibly something less specific). Instead, this should probably just return the list of awardee PHIDs. | |
| src/applications/badges/storage/PhabricatorBadgesAward.php | ||
| 33–38 | (When you fiddle with the keys, fiddle with this too.) | |
| src/applications/badges/storage/PhabricatorBadgesBadge.php | ||
| 200 | I think this probably won't work -- use bin/remove destroy <phid> to destroy a badge with at least one award. I'd expect you to get an error like "badges are not attached!", because the DestructionEngine does not know that you expect it to call needRecipients(). Instead, I'd just use PhabricatorBadgesAwardQuery explicitly to load the awards here. | |
| 202 | This should be $engine->destroyObject($award); to make sure any related objects get cleaned up (in this case, there aren't any today). | |
Comment Actions
Oh, it looks like there's an edge query in PHUITimelineView that this doesn't deal with. I'd expect that to prevent badges from appearing on timelines.
Comment Actions
This should handle the migration (just drop it into autopatches/ as a new migration patch):
20160308.badgemigrate.sql
/* PhabricatorBadgeHasRecipientEdgeType::TYPECONST = 59 */
INSERT INTO {$NAMESPACE}_badges.badges_award
(badgePHID, recipientPHID, awarderPHID, dateCreated, dateModified)
SELECT src, dst, 'PHID-VOID-00000000000000000000', dateCreated, dateCreated
FROM {$NAMESPACE}_badges.edge WHERE type = 59;Comment Actions
Upshot:
- Fix the remaining edge query in PHUITimelineView.
- Add that migration (and run it to make sure I didn't break anything).
Then I think we're good to go.
Comment Actions
Oh, I think you only got that error because you'd already used the new code to award some badges. That's fine, INSERT IGNORE ... is reasonable in this case anyway (it just makes us ignore rows which would fail to insert because of duplicate key errors).
I think I caught one more minor issue inline.
| src/view/phui/PHUITimelineView.php | ||
|---|---|---|
| 262–266 | This probably needs to be idx($awards, $event->getAuthorPHID(), array()) because it's possible the author has no badges, and thus won't have an entry in the table. I'd expect you to see an error if you look at a comment made by a user who has been awarded no badges. | |
Comment Actions
Using correct way to find timeline event author badge award, in case author has no badge awards
Comment Actions
Caught a couple more issues.
| src/applications/badges/storage/PhabricatorBadgesAward.php | ||
|---|---|---|
| 27–31 | Very minor, but these can be omitted because any column named somethingPHID defaults to phid as a type. | |
| 39 | This key is improperly specified as unique, which prevents the same user from receiving more than one badge. To reproduce:
That raises an error because both awards have the same recipientPHID:  To fix this:
Then the steps above should work properly. | |
| src/applications/badges/storage/PhabricatorBadgesTransaction.php | ||
| 41 | The AWARD and REVOKE stories currently use generic rendering in timelines and feed -- here's timeline:  Here's feed:  These should say something like "alice awarded this badge to: bob, clara." You can fix this by providing strings here (in getTitle()) and below (in getTitleForFeed()). | |
| src/view/phui/PHUITimelineView.php | ||
| 266 | Oh, we lost this, which changed behavior a little bit: previously, we only showed active badges on timeline. Now, we'll show all badges. To reproduce this:
Prior to this change, archiving a badge hid it; now it is shown. The easiest way to fix this is probably to just make sure the badge is active below, before you add it to the $badges array. | |
Comment Actions
Don't show archived badges in timeline view, allow users to have multipls badge awards (saving transaction titles for another commit)
| src/view/phui/PHUITimelineView.php | ||
|---|---|---|
| 268–279 | This isn't quite right, since we'll slice first, then skip archived badges. So if you have 3 badges but the first 2 are archived, the third one will be thrown away and then the two archived ones will be hidden. But this will almost always do something reasonable in actual cases, and we can clean it up later. | |