Replace most usages of full names with display names
Review Request #9930 — Created May 11, 2018 and submitted
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 … |
chipx86 | |
F841 local variable 'site' is assigned to but never used |
reviewbot | |
I'm concerned that we're going to be introducing a lot of unexpected query counts in the product with this. First, … |
chipx86 | |
The change description says {% get_display_name %}, and this says {% get_user_display_name %}. I feel that neither of those are … |
chipx86 | |
I feel like we should be using the new method on the profile for this. Either way, we need to … |
chipx86 | |
This looks left over (it's not adding to the queryset). |
chipx86 | |
Can you rename the parameter to viewing_user? (I meant to mention this explicitly in a prior comment -- it was … |
chipx86 | |
Typo: "for this users" |
chipx86 | |
Missing docstring. |
chipx86 |
- Description:
-
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
{% get_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. - Commit:
-
bfac82d334fcbdd0860d832627b49379eb7af43360c5c180ae896c0b0d47ff7e82d6412ee5fbe0ba
- Diff:
-
Revision 2 (+314 -12)
Checks run (2 succeeded)
- Commit:
-
60c5c180ae896c0b0d47ff7e82d6412ee5fbe0ba0420408acc303b1b3c2e5499c57cb39f542bf6ac
- Diff:
-
Revision 3 (+314 -12)
Checks run (2 succeeded)
- Commit:
-
0420408acc303b1b3c2e5499c57cb39f542bf6ac32ea2b8809a61c852d6b7427e6555db3534c805a
- Diff:
-
Revision 4 (+315 -12)
Checks run (2 succeeded)
- Commit:
-
32ea2b8809a61c852d6b7427e6555db3534c805a4576df41fcabfb947a420457bb9861a1bda2c45e
- Diff:
-
Revision 5 (+315 -12)
Checks run (2 succeeded)
-
-
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
-
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 %}
. -
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.
-
- Change Summary:
-
Addressed feedback.
- Description:
-
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
{% get_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. ~ 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. - Commit:
-
4576df41fcabfb947a420457bb9861a1bda2c45e406442b347ab92f50ab328a06c49276bb2f9ffee
- People:
-
- Diff:
Revision 6 (+579 -21)
Checks run (2 succeeded)
flake8 passed.JSHint passed.
- Change Summary:
-
Fix some regressions caused by Djblets
- Description:
-
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 - Commit:
-
406442b347ab92f50ab328a06c49276bb2f9ffeeea3fe22258eda3ecac0ca5a49ed62550910433b5
- Diff:
-
Revision 7 (+584 -26)
Checks run (2 succeeded)
- Commit:
-
ea3fe22258eda3ecac0ca5a49ed62550910433b5d1745035c92973d02a0c90db47563f397f51bd06
- Diff:
-
Revision 8 (+584 -26)
Checks run (2 succeeded)
-
-
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. -
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
andself.user
blur together). -
- Change Summary:
-
Add unit tests, address feedback.
- Commit:
-
d1745035c92973d02a0c90db47563f397f51bd0644f65f0af1cba15120259dc9fd92892536c21ac0
- Diff:
-
Revision 9 (+655 -26)
Checks run (2 succeeded)
- Change Summary:
-
Fixed issues
- Commit:
-
44f65f0af1cba15120259dc9fd92892536c21ac016db2a2b9c8355f49e3bfe1231b9ba0bae7b623d
- Diff:
-
Revision 10 (+671 -26)