Skip to content

Update TokenAdmin to respect USERNAME_FIELD of the user model. #9836

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+28 −3
Open

Update TokenAdmin to respect USERNAME_FIELD of the user model. #9836

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 3 additions & 2 deletions rest_framework/authtoken/admin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,11 @@ def url_for_result(self, result):

class TokenAdmin(admin.ModelAdmin):
list_display = ('key', 'user', 'created')
list_filter = ('created',)
fields = ('user',)
search_fields = ('user__username',)
search_fields = ('user__%s' % User.USERNAME_FIELD,)
search_help_text = _('Username')
ordering = ('-created',)
Copy link
Contributor

@p-r-a-v-i-n p-r-a-v-i-n Nov 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think we should keep this ordering = ('-created',) ? what's your thoughts

Copy link
Author

@m000 m000 Nov 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think default ordering by username combined with a creation date filter makes more sense for several reasons:

  • You usually know which user's token you are looking for. So when you search for part of the username, it's easier to scan the search results if they are ordered by username.
  • When you have a date in mind, it is usually in the recent past (which is covered by the options of the created filter).
  • Manually ordering by created is always possible if desired.
  • created is not indexed while the username field typically should be. So, if anyone deals with a lot of tokens, loading the page should be slower.

But no hard opinion on this. We can stick with -created ordering if you find that better.

Copy link
Contributor

@p-r-a-v-i-n p-r-a-v-i-n Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, created is not indexed while the username field typically should be

this make sense to me, and if this is the case then i don't have any problem.

Copy link
Author

@m000 m000 Nov 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Marking as resolved.

Source reference for the fields:

# rest_framework/authtoken/models.py::Token
    created = models.DateTimeField(_("Created"), auto_now_add=True)

# django/contrib/auth/models.py::AbstractUser
    username = models.CharField(
        _("username"),
        max_length=150,
        unique=True,  # --> adds index
        help_text=_(
            "Required. 150 characters or fewer. Letters, digits and @/./+/-/_ only."
        ),
        validators=[username_validator],
        error_messages={
            "unique": _("A user with that username already exists."),
        },
    )

Copy link
Collaborator

@browniebroke browniebroke Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I'm late to the party here, but one thing that comes to mind is how this will look in the UI. The ordering is by username but AFAIK the username isn't displayed on the page. If the email field is configured as username, the values could be quite different from the user display, making the ordering looks almost random.

Not sure if that's a big problem, though. And the point you raised about the lack of index is a good one.

Copy link
Contributor

@p-r-a-v-i-n p-r-a-v-i-n Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the email field is configured as username, the values could be quite different from the user display, making the ordering looks almost random.

Yes that sorting might create confusion in user mind. This will be surprise for the users.
if there is a performance problem then they can override the indexing and if require we can create seperate PR for adding index on created_at field if we have strong use case in future ( my opinion).

Copy link
Author

@m000 m000 Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@browniebroke The user is displayed. Here is a reference screenshot after the changes.

tokenadmin

Apart from the indexing issue, I would prefer ordering by username because if you do a partial match search (e.g. adam in the screenshot), it is easier to scan through the result list if it is ordered by username.

OTOH, if tokens are long-lived, the creation date soon becomes irrelevant: Created at Sept. 2022 vs Oct. 2022 is pretty much the same thing in 2025.

But I think we have already spent more energy on this than its worth. Let me know of your final decision on ordering and I'll roll with it.

I can also follow-up with adding indexing to created_at, if you feel this is needed. Anecdotally, for the ~500 tokens in our app, the admin loading difference is not noticeable. But I don't know how many tokens other apps may have.

Copy link
Collaborator

@browniebroke browniebroke Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I see, the default __str__ for the user uses username:

https://github.com/django/django/blob/bd7940982d6cab386dae7698ab097b91e5d8145e/django/contrib/auth/base_user.py#L58-L59

I can see some apps where it might be customised as first_name last_name but that's perhaps fine

ordering = ('user__%s' % User.USERNAME_FIELD,)
Comment on lines +28 to +30
Copy link

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes to use USERNAME_FIELD instead of hardcoded 'username' lack test coverage. Consider adding tests that verify the admin configuration works correctly with a custom user model that uses a different USERNAME_FIELD (e.g., 'email'), ensuring search and ordering function as expected.

Copilot uses AI. Check for mistakes.
Copy link
Contributor

@p-r-a-v-i-n p-r-a-v-i-n Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@m000 it would be great to add test coverage for the changes.

Copy link
Author

@m000 m000 Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added. It took a while to figure it out. Admin tests are a major PITA because modules include code that runs on load.

actions = None # Actions not compatible with mapped IDs.

def get_changelist(self, request, **kwargs):
Expand Down
26 changes: 25 additions & 1 deletion tests/test_authtoken.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import importlib
from io import StringIO
from unittest.mock import patch

import pytest
from django.contrib.admin import site
Expand All @@ -11,7 +12,7 @@
from rest_framework.authtoken.admin import TokenAdmin
from rest_framework.authtoken.management.commands.drf_create_token import \
Command as AuthTokenCommand
from rest_framework.authtoken.models import Token
from rest_framework.authtoken.models import Token, TokenProxy
from rest_framework.authtoken.serializers import AuthTokenSerializer
from rest_framework.exceptions import ValidationError

Expand All @@ -36,6 +37,29 @@ def test_model_admin_displayed_fields(self):
token_admin = TokenAdmin(self.token, self.site)
assert token_admin.get_fields(mock_request) == ('user',)

@patch('django.contrib.admin.site.register') # avoid duplicate registrations
def test_model_admin__username_field(self, mock_register):
import rest_framework.authtoken.admin as authtoken_admin_m

class EmailUser(User):
USERNAME_FIELD = 'email'
username = None

for user_model in (User, EmailUser):
with (
self.subTest(user_model=user_model),
patch('django.contrib.auth.get_user_model', return_value=user_model) as get_user_model
):
importlib.reload(authtoken_admin_m) # reload after patching
assert get_user_model.call_count == 1

mock_request = object()
token_admin = authtoken_admin_m.TokenAdmin(TokenProxy, self.site)
assert token_admin.get_search_fields(mock_request) == (f'user__{user_model.USERNAME_FIELD}',)
assert token_admin.get_ordering(mock_request) == (f'user__{user_model.USERNAME_FIELD}',)

importlib.reload(authtoken_admin_m) # restore after testing

def test_token_string_representation(self):
assert str(self.token) == 'test token'

Expand Down