Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Abstract order user by name #3529

Merged
merged 3 commits into from
Sep 11, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions jobserver/migrations/0007_add_User_order_by_name.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
# Generated by Django 4.2.5 on 2023-09-06 16:15

from django.db import migrations

import jobserver.models.core


class Migration(migrations.Migration):

dependencies = [
("jobserver", "0006_move_to_new_release_flow"),
]

operations = [
migrations.AlterModelManagers(
name="user",
managers=[
("objects", jobserver.models.core.UserManager()),
],
),
]
30 changes: 29 additions & 1 deletion jobserver/models/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,13 @@

import structlog
from django.contrib.auth.hashers import check_password, make_password
from django.contrib.auth.models import AbstractBaseUser, UserManager
from django.contrib.auth.models import AbstractBaseUser
from django.contrib.auth.models import UserManager as DjangoUserManager
from django.contrib.auth.validators import UnicodeUsernameValidator
from django.contrib.postgres.fields import ArrayField
from django.db import models, transaction
from django.db.models import Min, Q, prefetch_related_objects
from django.db.models.functions import Lower, NullIf
from django.urls import reverse
from django.utils import timezone
from django.utils.functional import cached_property
Expand Down Expand Up @@ -767,6 +769,32 @@ def human_memorable_token(size=8):
return xkcd_password.generate_xkcdpassword(WORDLIST, numwords=3)


class UserQuerySet(models.QuerySet):
def order_by_name(self):
"""
Order Users by their "name"

we don't have fullname populated for all users yet and having some
users at the top of the list with just usernames looks fairly odd.
We've modelled our text fields in job-server such that they're not
nullable because we treat empty string as the only empty case. The
NullIf() call lets us tell the database to treat empty strings as NULL
for the purposes of this ORDER BY, using nulls_last=True

TODO: remove this method in favour of order_by(Lower("fullname")) once
all users have fullname populated
"""
return self.order_by(
NullIf(
Lower("fullname"), models.Value(""), output_field=models.TextField()
).asc(nulls_last=True)
)


class UserManager(DjangoUserManager.from_queryset(UserQuerySet)):
use_in_migrations = True


class User(AbstractBaseUser):
"""
A custom User model used throughout the codebase
Expand Down
25 changes: 7 additions & 18 deletions jobserver/views/users.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@
from django.contrib.auth import login
from django.contrib.auth.decorators import login_required
from django.core.signing import BadSignature, SignatureExpired, TimestampSigner
from django.db.models import Count, Q, TextField, Value
from django.db.models.functions import Lower, NullIf
from django.db.models import Count, Q
from django.db.models.functions import Lower
from django.shortcuts import get_object_or_404, redirect
from django.template.response import TemplateResponse
from django.urls import reverse
Expand Down Expand Up @@ -359,20 +359,9 @@ class UserList(ListView):
template_name = "user_list.html"

def get_queryset(self):
qs = super().get_queryset().annotate(project_count=Count("projects"))

# we don't have all full names for all users yet and having some users
# at the top of the list with just usernames looks fairly odd. We've
# modelled our text fields in job-server such that they're not nullable
# because we treat empty string as they only empty case. The NullIf()
# call lets us tell the database to treat empty strings as NULL for the
# purposes of this ORDER BY, using nulls_last=True
# TODO: switch this to just order on Lower("fullname") once all users
# have fullname filled in
qs = qs.order_by(
NullIf(Lower("fullname"), Value(""), output_field=TextField()).asc(
nulls_last=True
)
return fetch(
super()
.get_queryset()
.annotate(project_count=Count("projects"))
.order_by_name()
)

return fetch(qs)
4 changes: 1 addition & 3 deletions staff/forms.py
Original file line number Diff line number Diff line change
Expand Up @@ -97,9 +97,7 @@ class ProjectAddMemberForm(PickUsersMixin, RolesForm):

class ProjectCreateForm(forms.Form):
application_url = forms.URLField()
copilot = UserModelChoiceField(
queryset=User.objects.order_by(Lower("fullname"), "username")
)
copilot = UserModelChoiceField(queryset=User.objects.order_by_name())
name = forms.CharField()
number = forms.IntegerField()
org = forms.ModelChoiceField(queryset=Org.objects.order_by("name"))
Expand Down