Replace most usages of full names with display names

Review Request #9930 — Created May 11, 2018 and submitted

brennie
Review Board
release-3.0.x
9966
9924
16db2a2...
reviewboard
chipx86

We now only render a user's full name only if they have a public
profile. In other cases, we fall back to the username when appropriate
or render an empty string (such as in the full name column).

We now also provide the {% user_profile_display_name %} template tag,
which will render a user's full name is visible to the current user, or
their username if not. This should be used in place of the
user_displayname filter.

This change also addresses a regression introduced by Djblets in
https://reviews.reviewboard.org/r/9955/ where the avatar template
changed, causing test failures in Review Board. We now specify our own
avatar template (that uses our new template tag) and the unit tests have
been corrected

Ran unit tests.

  • 0
  • 0
  • 9
  • 0
  • 9
Description From Last Updated
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

brennie
brennie
brennie
brennie
chipx86
  1. 
      
  2. reviewboard/accounts/models.py (Diff revision 5)
     
     
     
     
     
     
     
     
     
     

    I'm concerned that we're going to be introducing a lot of unexpected query counts in the product with this.

    First, we'll be fetching profiles that maybe we weren't fetching before, and if we're doing this in any sort of loop, we're going to have unexpected queries.

    Second, that Local Site admin filter is not going to be efficient. If we call this 10 times on 3 different users, we end up with 10 queries, which is not good. Particularly since in the 99% case, there's no Local Site.

    Even if we did it once, we now have another potentially expensive lookup on each page load, particularly since we're fetching the results and checking for existence (which is what will happen under the hood here -- note that we're not calling .exists()).

    So what we need to do is ensure we're doing at most one lookup per page load per requesting user. I suggest splitting off that filter into a new function (is_admin_for_user()) and reworking this method and that to look like:

    def get_display_name(self, viewing_user):
        if viewing_user is None or viewing:
            return None
    
        display_name = None
    
        if (viewing_user is not None and (... or viewing_user.is_admin_for_user(self.user))):
            display_name = self.user.get_full_name()
    
        return display_name or self.user.username
    
    def is_admin_for_user(self, user):
        if self.is_staff:
            return True
    
        if not user or user.is_anonymous():
            return False
    
        if not hasattr(self, '_adminned_user_ids'):
            self._adminned_user_ids = cache_memoize(
                '%s-adminned-user-ids' % self.user.pk,
                lambda: tuple(User.objects.filter(local_site_admins=self.user).values_list('pk', flat=True)))
    
        return user.pk in self._adminned_user_ids
    
  3. The change description says {% get_display_name %}, and this says {% get_user_display_name %}. I feel that neither of those are really appropriate (the "get" isn't correct for this usage). I also feel that it's too similarly-named to the filter, but differs too much from its behavior.

    I don't know that I have a better name for it, off-hand, without it getting too lengthy... Maybe {% user_profile_display_name %}? It at least provides a little more context in how the name is computed... I don't know. Maybe {% user_visible_display_name %}.

  4. reviewboard/datagrids/columns.py (Diff revision 5)
     
     
     

    I feel like we should be using the new method on the profile for this.

    Either way, we need to be 100% sure we're not introducing a bunch of new queries here. I want to see unit test asserting query counts for this column. They'd need to test that the calls aren't introducing any new queries, since we can't have a query per row being introduced.

    1. This doesn't use the method because it returns '' if we don't have access to the full name, not the user name.

  5. reviewboard/datagrids/grids.py (Diff revision 5)
     
     

    This looks left over (it's not adding to the queryset).

  6. 
      
brennie
brennie
brennie
brennie
chipx86
  1. 
      
  2. I know we're down to the wire here, but can you add a unit test for is_admin_for_user? This is an important function to test thoroughly for security reasons.

  3. reviewboard/accounts/models.py (Diff revision 8)
     
     

    Can you rename the parameter to viewing_user? (I meant to mention this explicitly in a prior comment -- it was just in the example code). That'll help make this a lot more readable and self-documenting (user and self.user blur together).

  4. reviewboard/accounts/models.py (Diff revision 8)
     
     

    Typo: "for this users"

  5. 
      
brennie
chipx86
  1. Looking great. One small thing (hesitated bothering with it, but should be quick).

  2. reviewboard/datagrids/columns.py (Diff revision 9)
     
     
     

    Missing docstring.

  3. 
      
brennie
david
  1. Ship It!
  2. 
      
brennie
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (bba7004)
Loading...