Page MenuHomePhabricator

Allow searching for Badge Awards by Badge status
ClosedPublic

Authored by chad on Mar 15 2017, 5:47 PM.

Details

Summary

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.

Test Plan

Assign myself a badge, archive it and verify it does not appear on profile, comment form, or timeline.

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 created this revision.Mar 15 2017, 5:47 PM
chad edited the summary of this revision. (Show Details)Mar 15 2017, 5:51 PM
epriestley requested changes to this revision.Mar 15 2017, 6:20 PM

Top-level stuff looks good but couple of implementation details inline.

src/applications/badges/query/PhabricatorBadgesAwardQuery.php
17–34

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()?

This revision now requires changes to proceed.Mar 15 2017, 6:20 PM
chad added inline comments.Mar 15 2017, 6:22 PM
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.

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.

chad edited edge metadata.Mar 15 2017, 7:22 PM
chad updated this revision to Diff 42081.
  • move to join
chad added a comment.Mar 15 2017, 7:22 PM

Stuck at #1052: Column 'id' in order clause is ambiguous

chad added a comment.Mar 15 2017, 7:24 PM

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?

Add getPrimaryTableAlias() and have it return badges_award, I think?

chad updated this revision to Diff 42082.Mar 15 2017, 7:27 PM
  • fix table alias

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.

chad updated this revision to Diff 42083.Mar 15 2017, 7:30 PM
  • cleaner filter
epriestley accepted this revision.Mar 15 2017, 7:35 PM

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
32

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
251

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.

This revision is now accepted and ready to land.Mar 15 2017, 7:35 PM
chad updated this revision to Diff 42084.Mar 15 2017, 7:40 PM
  • comments
This revision was automatically updated to reflect the committed changes.