Page MenuHomePhabricator

Update DiffusionLastModifiedController to use identities
ClosedPublic

Authored by amckinley on Aug 13 2018, 11:42 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Apr 29, 4:21 PM
Unknown Object (File)
Sat, Apr 27, 10:19 PM
Unknown Object (File)
Thu, Apr 25, 12:01 AM
Unknown Object (File)
Fri, Apr 19, 7:51 PM
Unknown Object (File)
Tue, Apr 16, 6:10 AM
Unknown Object (File)
Thu, Apr 11, 8:18 AM
Unknown Object (File)
Apr 1 2024, 5:13 PM
Unknown Object (File)
Mar 21 2024, 12:51 AM
Subscribers

Details

Summary

Ref T12164. Updates another controller to use identities.

Test Plan

Pretty ad-hoc, but loaded the main pages of several different repos with and without repo identities. I'm not totally convinced the author from this data structure is actually being used:

$return = array(
  'commit'    => $modified,
  'date'      => $date,
  'author'    => $author,
  'details'   => $details,
);

Diff Detail

Repository
rP Phabricator
Branch
commit-cleanup2
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 20611
Build 28004: Run Core Tests
Build 28003: arc lint + arc unit

Event Timeline

I think the "Graph" tab may still use this, maybe check that?

Screen Shot 2018-08-14 at 8.01.07 AM.png (1×1 px, 267 KB)

We could also consider just moving DiffusionView::renderName() into renderAuthor()/renderCommitter(), perhaps -- should be safe since the path which uses Identities definitely returns markup.

This revision is now accepted and ready to land.Aug 14 2018, 3:02 PM

I think the "Graph" tab may still use this, maybe check that?

Hmmm, I wonder why your screenshot has clickable "epriestley" links but my full name is showing up as text? Here's my version of the same page:

Screen Shot 2018-08-14 at 8.28.23 AM.png (948×2 px, 310 KB)

Oh -- my screenshot is on my local install, and I haven't made you an account there, so there's no Phabricator account to associate the commit with. I get the same view as you do here on secure.

We could also consider just moving DiffusionView::renderName() into renderAuthor()/renderCommitter(), perhaps -- should be safe since the path which uses Identities definitely returns markup.

How exactly would this work? In the case where renderXXX() ends up trying to return a string, just pass it through renderName() first? I guess having that nice tooltip would be strictly better than plain text. Is it possible that it might render on some page where the CSS isn't expecting it, though?

In the case where renderXXX() ends up trying to return a string, just pass it through renderName() first?

Yeah. Or possibly the renderName() logic can just move to Commit entirely, depending on how many callsites we're left with after converting things.

Is it possible that it might render on some page where the CSS isn't expecting it, though?

Since we'll sometimes return a complex piece of HTML now from renderAuthor() anyway (when we have an Identity and the Identity is bound to a user, we link to their profile) and generally did that before anyway (just with a lot more if (authorPHID) checks) I suspect we won't run into any problems. Definitely possible, but it's likely we'll catch it while converting to Identities if we do run afoul of some CSS.

  • Migrate DiffusionBlameController to use repo identities

Or possibly the renderName() logic can just move to Commit entirely, depending on how many callsites we're left with after converting things.

There are no other callsites aside from this controller, but I don't feel great about moving that kind of UI-specific logic into Commit. Here's what we'd be moving:

final public static function renderName($name) {
  $email = new PhutilEmailAddress($name);
  if ($email->getDisplayName() && $email->getDomainName()) {
    Javelin::initBehavior('phabricator-tooltips', array());
    require_celerity_resource('aphront-tooltip-css');
    return javelin_tag(
      'span',
      array(
        'sigil' => 'has-tooltip',
        'meta'  => array(
          'tip'   => $email->getAddress(),
          'align' => 'S',
          'size'  => 'auto',
        ),
      ),
      $email->getDisplayName());
  }
  return hsprintf('%s', $name);
}

If you think we should move that into Commit, let me know and I'll put up a new diff for that.

This revision was automatically updated to reflect the committed changes.