Replace most usages of full names with display names

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

Information

Review Board
release-3.0.x
16db2a2...

Reviewers

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.

Description From Last Updated

I know we're down to the wire here, but can you add a unit test for is_admin_for_user? This is an …

chipx86chipx86

F841 local variable 'site' is assigned to but never used

reviewbotreviewbot

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

chipx86chipx86

The change description says {% get_display_name %}, and this says {% get_user_display_name %}. I feel that neither of those are …

chipx86chipx86

I feel like we should be using the new method on the profile for this. Either way, we need to …

chipx86chipx86

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

chipx86chipx86

Can you rename the parameter to viewing_user? (I meant to mention this explicitly in a prior comment -- it was …

chipx86chipx86

Typo: "for this users"

chipx86chipx86

Missing docstring.

chipx86chipx86
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)
     
     
     
     
     
     
     
     
     
     
    Show all issues

    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. Show all issues

    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)
     
     
     
    Show all issues

    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)
     
     
    Show all issues

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

  6. 
      
brennie
brennie
brennie
brennie
chipx86
  1. 
      
  2. Show all issues

    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)
     
     
    Show all issues

    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)
     
     
    Show all issues

    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)
     
     
     
    Show all issues

    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...