Page MenuHomePhabricator

Adding awarder info to badge cards displayed on user profile pages
ClosedPublic

Authored by lpriestley on Apr 2 2016, 12:54 AM.

Details

Summary

Ref T8940

Test Plan

Award badge, open recipient profile page, badge should appear in badges list, and flipping the badge card should show who awarded it.

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

lpriestley updated this revision to Diff 37536.Apr 2 2016, 12:54 AM
lpriestley retitled this revision from to Adding awarder info to badge cards displayed on user profile pages.
lpriestley updated this object.
lpriestley edited the test plan for this revision. (Show Details)
lpriestley added a reviewer: epriestley.

This diff intentionally doesn't include the "recipient list on the badge page" part of this task:

epriestley requested changes to this revision.Apr 2 2016, 1:42 AM
epriestley edited edge metadata.

This is good, but it will load a lot more data than we need when some of a user's badges have also been awarded to a lot of other users.

The needRecipients(true) will make us load every award for the user's badges, which might be hundreds or thousands of rows/objects if a badge has been awarded to lots of users. We won't look at most of this data, and this could make profiles slow on large installs.

Instead, just use PhabricatorBadgesAwardQuery upfront to get the user's awards (instead of $user->getBadgePHIDs(), then use PhabricatorBadgesQuery without needRecipients() to get the badges. That should load only the data we actually need.

src/applications/people/controller/PhabricatorPeopleProfileViewController.php
198

Specifically, this makes us load more data than we need.

This revision now requires changes to proceed.Apr 2 2016, 1:42 AM
lpriestley updated this revision to Diff 37539.Apr 2 2016, 5:33 AM
lpriestley edited edge metadata.

Optimizing badges and awards queries to only fetch data that's relevant

lpriestley updated this revision to Diff 37540.Apr 2 2016, 5:44 AM
lpriestley edited edge metadata.

Fixing a regression - empty badges box if all badges are archived

lpriestley added inline comments.Apr 2 2016, 5:46 AM
src/applications/people/controller/PhabricatorPeopleProfileViewController.php
211

Turns out, because the card flips on click, you can't actually click the user handle on it. Is there away to make the handle unclickable?

lpriestley added inline comments.Apr 2 2016, 5:49 AM
src/applications/people/controller/PhabricatorPeopleProfileViewController.php
187–199

I know this seems like two queries right out of the gate might be aggressive, but my thinking was: if a user has badgePHIDs, then there are definitely awards, and we would still have to make a second query to get badges, even if all those badges were archived.

epriestley accepted this revision.Apr 2 2016, 1:45 PM
epriestley edited edge metadata.

Issuing the second query immediately seems generally reasonable to me.

There is a tiny chance that something like this could possibly happen, either now or in the future:

getBadgePHIDs() is cached, so it could return slightly out-of-date data, in theory. Even if it wasn't, there's a tiny but nonzero amount of time between the query powering getBadgePHIDs() and the query checking for the user's awards.

So getBadgePHIDs()could say the user has badges, but by the time the second query executes a millisecond later the user might not have any badges anymore (someone could have removed them).

Then we'd do a bogus query on no PHIDs, which would throw an exception.

This is probably impossible. If it isn't impossible, the window is so narrow that you probably couldn't hit it by trying. The badge cache on users may not even make sense soon, making it 100% impossible. Even if this is possible to hit, reloading the page would fix it. So waiting for this to actually present a problem seems fine to me.


For the clicking thing, I think we have to write custom JS. Currently, it's powered by a simple behavior (jx-toggle-class) in PHUIBadgeView, which just says "toggle this CSS class on/off when the thing is clicked".

There's no way to make this behavior more complex/subtle (e.g., ignore clicks on links) and it's important that it not ignore clicks on links in all cases because it also powers stuff like "Show/Hide Details" links elsewhere.

So this probably boils down to:

  • Write a new behavior which does the toggling.
  • It can mostly work like jx-toggle-class, but can probably be a bit simpler since it's custom.
  • It should ignore clicks on links inside the card.

This seems fine to push to a future diff to me.

To check if something is a click on a link, you'll do something like this:

JX.Stratcom.listen('badge-view', 'click', function(e) {
  if (e.getNode('tag:a')) {
    // If the event has a `tag:a` node on it, that means the user
    // either clicked a link or some other node inside a link.
    return;
  }
  // ...
});
This revision is now accepted and ready to land.Apr 2 2016, 1:45 PM

Do you think that until we implement a custom behavior, that we could make that handle not appear as a link? Or is that too involved?

chad added a comment.Apr 2 2016, 3:38 PM

Ideally you could click on the badge name and get to the badge's profile as well. I'm not super concerned about linking the awarder, since I don't think that's useful, but linking the badge name to see who else is awarded and why does seem beneficial.

If there was already a "don't link this" thing on the handles I think that would be fine to use, but we don't have one and don't have any other use cases for it that I can think of.

You could technically force the handle to load, then pull the name off it, but this is kind of a mess and probably more work to do + undo later than it's worth.

@chad's use case of linking the front of the badge makes sense to me, too.

This revision was automatically updated to reflect the committed changes.