Fixes T12398. This adds withBadgeStatuses as a query parameter when searching for Awards to show. In most (all?) cases we currently only show active badges.
Details
Details
- Reviewers
epriestley - Maniphest Tasks
- T12398: Archived badges visible in comment form
- Commits
- rPfd69dfaa9a14: Allow searching for Badge Awards by Badge status
Assign myself a badge, archive it and verify it does not appear on profile, comment form, or timeline.
Diff Detail
Diff Detail
- Repository
- rP Phabricator
- Branch
- disabled-badge (branched from master)
- Lint
Lint Passed - Unit
Tests Passed - Build Status
Buildable 15989 Build 21192: Run Core Tests Build 21191: arc lint + arc unit
Event Timeline
Comment Actions
Top-level stuff looks good but couple of implementation details inline.
| src/applications/badges/query/PhabricatorBadgesAwardQuery.php | ||
|---|---|---|
| 17–33 | Doing this filtering here -- instead of in the query itself -- means that we may do a very large amount of unnecessary work. Suppose the user has been awarded 5,000 badges, and the first 4,995 have been archived (perhaps they were awarded in a terrible badge accident). We'll end up loading all 4,995 of those awards, then trying to load their badges, failing, and loading another group of awards. The page size for queries is normally 100, so this will issue 50 extra queries (with setLimit(2), it will issue about 2,500 extra queries). Instead, withBadgeStatuses() should cause us to ... JOIN badge ON badge.phid = award.badgePHID ... WHERE badge.status IN (%Ls) ... in the query. You can look at withAuditorPHIDs() / shouldJoinAuditor() in DiffusionCommitQuery for a similar example of this, I think. By using a JOIN, MySQL will do this work for us and status filtering won't require more queries. | |
| src/applications/transactions/view/PhabricatorApplicationTransactionCommentView.php | ||
| 532–539 | This change (away from setLimit()) means that we load every award the user has ever received (which could be thousands), then throw away all but two. What issue were you hitting that prompted you to remove setLimit()? | |
| src/applications/transactions/view/PhabricatorApplicationTransactionCommentView.php | ||
|---|---|---|
| 532–539 | does setLimit work with filterPage? I assumed if I had 4 badges, and 1 was archived, only 1 might be returned. | |
Comment Actions
The two work together, yeah. If you setLimit(100), you'll get 100 results back if possible -- even if we have to issue multiple queries to get that many visible results.
Normally this looks something like:
- issue a query for 100 results, filter the results, a few weren't visible, end up with, say, 83 results;
- issue another query for 17 more results, filter those results, this time they're all visible so we add them to the first set and return the full 100 results.
There are some cases where this gets complicated. T11773 discusses "overheating" (when we repeatedly filter every result over and over again, "overheating" stops us from continuing forever) and T12353 has some more detailed discussion about paging and how we pick the underlying query sizes and how we might choose different sizes in the future.
Comment Actions
I only half remember, but I think there is a way to just pull in status? I don't need the entire row of data?
Comment Actions
This is fully correct as written -- the JOIN doesn't really pull in the row on its own, unless you SELECT * to say that you want every possible column. With getPrimaryTableAlias() we end up doing something like SELECT badge_award.*, so the joined table isn't part of the result set and doesn't get returned.
The id ambiguous part is because the query ends up with a subclause like AND id > 3, but that could mean badges_badge.id or badges_award.id. With getPrimaryTableAlias() the subclause will generate AND badges_award.id > 3 and remove the ambiguity.
Comment Actions
The filter change is actually good -- I think we'll currently fatal if you award a user a badge, manually DELETE the badge in MySQL, then view the user's profile page. And the filter change fixes that. One inline about doing it in a slightly more consistent way.
Then one inline about a tricky case with setLimit().
But the meat of this is in good shape now.
| src/applications/badges/query/PhabricatorBadgesAwardQuery.php | ||
|---|---|---|
| 28 | Slightly more consistent is something like this: if (!$award_badge) {
unset($awards[$key]);
$this->didRejectResult($award);
continue;
}...and then return $awards; at the bottom, without using $results. The didRejectResult() will let us do a slightly better job in telling the user what happened in some situations. For example, Handles will (theoretically) be able to render "Restricted Badge Award" instead of "Invalid Badge Award". This will probably never matter in this case but does in some other cases. | |
| src/view/phui/PHUITimelineView.php | ||
| 250 | I think we can't do setLimit() here -- we'll end up with two total awards, not two awards per user, but this query is trying to fetch badges for every user who has made a comment. There's no way to query for two awards per user and this is hard to express in SQL -- at least, I'm not sure how to do it. For now, because there's no easy fix, I think it's OK to leave this as it was. The eventual fix is probably via T9006, where we'd build a cache that let us do this query efficiently and let users exercise control over which badges were displayed. | |