Change admin search_fields to favor USERNAME_FIELD instead of "email".

[charettes]

Description of the Change

Per this request.

---

First nothing guarantees that the user model has a field named "email" as it can be set to a different name using EMAIL_FIELD. At the very least the get_email_field_name method should have been used.

Secondly nothing guarantees that EMAIL_FIELD is going to be indexed and thus suitable for search purposes. On the other hand USERNAME_FIELD must be unique and thus indexed in order to enforce this constraint.

For these reasons USERNAME_FIELD represents a better choice to allow the different toolkit models to be searched by through the admin.

Checklist

• PR only contains one change (considered splitting up PR)
• unit-test added (not sure if needed given upgrades to admin: search_fields, list_filters and raw_id_field #1041 didn't include any and Django enforce admin checks)
• documentation updated (I assume nothing is needed here either)
• CHANGELOG.md updated (only for user relevant changes)
• author name in AUTHORS

[n2ygk]

In further thinking about this, I see a problem with this approach:

By unilaterally changing search behavior from user__email to user__username this creates a breaking change for anybody already expecting the admin search to use user__email.

How about doing this:

1. Implement the fix you recommend of calling the get_email_field_name method instead of looking for the email attribute (which in all fairness is how it is documented in the contrib admin models documentation!)
2. Add (and document) a configuration setting that lists the fields to search.
3. Add a corresponding setting (or code) to set search_help_text that describes what is searched.

This will allow you to configure searching only by username for your performance reason use case while retaining the existing code default of searching by email and further allowing other users to expand the list of admin search fields for their particular use cases.

[charettes]

(which in all fairness is how it is documented in the contrib admin models documentation!)

That's a valid point that might be worth raising upstream. I'll see what I can do.

I'm happy to submit a PR for the first point as it's unambiguously a good change but I'm not convinced about the other ones.

There doesn't exist a setting today and Django already provides a way to override registered Django admins

class ApplicationAdmin(admin.ApplicationAdmin):
    search_fields = ("name", "user__email")
    search_help_text = "Search applications by name and user email"

admin.site.register(application_model, ApplicationAdmin)

In fact that's the approach we used at $JOB when we had to make sure user__email was not used as it was causing a lot of contention on one of our databases.

IMO settings like APPLICATION_ADMIN_CLASS are an anti-pattern as Django already has a pattern to override per-model admin classes and adding extra settings to target specific model admin options is a slippery slope so I don't think 2. and 3. are worth doing.

To summarize I think we should either proceed with this PR as it is as user__username constitute a better default than user__email or close this PR and only do 1.

[jaap3]

In my case the User model has an email field, but doesn't have a username field. This issue is still relevant to me, because this field uses a case-insensitive non-deterministic collation. The way Django Admin implements search_fields doesn't work with this type of field. (See also: https://adamj.eu/tech/2023/02/23/migrate-django-postgresql-ci-fields-case-insensitive-collation/#adjust-queries).

It's possible to work around this by using admin.unregister and registering a customised version of DOT admin classes.

However, maybe DOT should just drop the ability to search by email. Instead it can document how to customize the admin classes instead, using search_fields as an example. DOT doesn't really need to make any assumptions about the user model.

It would be a breaking change, but it is not a "mission critical" feature. Those that need it can bring it back using their own code.

The PR that first tried to introduce this had a fitting comment by @IvanAnishchuk :

Not sure if assuming things regarding user model is a good approach here

#723 (review)

P.S. @charettes I didn't even think about the APPLICATION_*_CLASS settings, surely those have to be deprecated at some point? It does imply that subclassing the admin classes is an expected use-case.