Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#16611 closed New feature (wontfix)

Contrib.auth.UserManager should allow filtering active/inactive users

Reported by: TomaszZielinski Owned by: TomaszZielinski
Component: contrib.auth Version: master
Severity: Normal Keywords:
Cc: twoolie Triage Stage: Design decision needed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no


This is something I do in most projects, so I think it's a nice small addition to UserManager.

Attachments (2)

auth.diff (3.4 KB) - added by TomaszZielinski 4 years ago.
16611.2.diff (4.3 KB) - added by aaugustin 4 years ago.

Download all attachments as: .zip

Change History (10)

Changed 4 years ago by TomaszZielinski

comment:1 Changed 4 years ago by TomaszZielinski

  • Needs documentation unset
  • Needs tests unset
  • Owner changed from nobody to TomaszZielinski
  • Patch needs improvement unset

comment:2 Changed 4 years ago by aaugustin

  • Triage Stage changed from Unreviewed to Accepted

It's such a thin wrapper that I initially wondered if it was really useful.

Upon further thought, it seems likely that, when developers build a QuerySet on User objects, they only want the active users (in most real-life cases). But it's easy to forget to filter on is_active=True.

So it's probably a good idea to promote as the "user manager you always use", example:

for user in

The patch is very good — I'm uploading a version with a very small change, namely using the new manager in PasswordResetForm.clean_email.

I vote +0. But I'd like other opinions before promoting this to RFC.

Version 0, edited 4 years ago by aaugustin (next)

Changed 4 years ago by aaugustin

comment:3 Changed 4 years ago by aaugustin

  • Triage Stage changed from Accepted to Design decision needed

comment:4 Changed 4 years ago by TomaszZielinski

It could also be a new manager, I'll happily update the patch.

If we go in that direction, it might be clever (or a bit too clever, but let's discuss this) to just add a "switch" to the default manager, so that its .all() method returns only active users. Personally I rarely need inactive users and if I forgot to filter them out manually, then e.g. they might accidentaly become "liked" or "followed" by other users (on a FB-style website).

comment:5 Changed 4 years ago by jacob

  • Resolution set to wontfix
  • Status changed from new to closed

I'm going to reject this idea; let me try to explain why.

Consider User.objects.filter(is_active=True). It's immediately clear to anyone with any passing familiarity with Django's model syntax what this is doing: it's fetching every User object whose is_active field is set to True. The code itself explains exactly what it's doing.

On the other hand, consider What does this do? The code hints that it returns "active" users -- but what does "active" mean? I'll have to dive into the code or documentation to see that "active" means "Users who have the is_active flag set to True" and not "Users who've logged in in the past week" or "Users who are logged into the site right now" (both perfectly reasonable translations of the word "active").

Yes, the second form saves about a dozen characters. Always remember, though: software is read far more often than it's written; saving a few characters once isn't an acceptable reason to introduce cognitive load every time that code is read.

Clarity and brevity sometimes are at odds. When they are, I choose clarity.

comment:6 Changed 4 years ago by twoolie

  • Cc twoolie added
  • Easy pickings set
  • milestone set to 1.4
  • Resolution wontfix deleted
  • Status changed from closed to reopened

Right up front: appologies if re-opening is not the correct etiquette, but i believe there is a use case that has not been considdered and i don't know if closed tickets will ever show up again in the core dev's ticket lists. Feel free to shut me down after hearing me out.

I independently opened a Pull Request for this feature a few days ago before I knew this issue existed. I just wanted to note that the main purpose of my patch was not to enable shortcutting in python when a more verbose approach is better. My patch was motivated by the following (admittedly generic) template use case which is enabled by setting use_for_related_fields = True on UserManager.

{% for moderator in %}
    {{ moderator|profilelink }}{% if not forloop.last %},{% endif %}
{% endfor %}

instead of

{% for moderator in forum.moderators.all %}
    {% if moderator.is_active %}
        {{ moderator|profilelink }}{% if not forloop.last %},{% endif %}
    {% endif %}
{% endfor %}

While it is perfectly verbose to put explicit filtering in the template, it seems to me that this is exactly the use case that custom manager methods (especially those that span related) were made for. This is a common use case that can be optimised in a short, semantic, performant way, and the benefit extends out to all models that reference User, making this a high ROI patch.

As far as potential confusion about the exact meaning of active(), this is the reason that the django project takes such pride in having comprehensive docs, is it not? Also, the custom method could be changed to be is_active() so as to explicitly mirror the property and hence suggest function.

comment:7 Changed 4 years ago by ptone

  • Resolution set to wontfix
  • Status changed from reopened to closed

In general - if a core developer closes a ticket - it should only be opened after a discussion on the mailing list where a core developer(s) changes their mind. There was a thread on the mailing list:

and it does not appear minds were changed - this ticket can be reopened if someone weighs in later, but for now, the original decision should be restored.

In general, restricting the set of moderators in your example should happen in view code, not in template code.

comment:8 Changed 4 years ago by jacob

  • milestone 1.4 deleted

Milestone 1.4 deleted

Note: See TracTickets for help on using tickets.
Back to Top