• 
      

    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:
    Completed
    Change Summary:
    Pushed to release-3.0.x (bba7004)