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

[Tomasz Zieliński]

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

[Aymeric Augustin]

EDIT

I accepted the ticket because I thought the syntax would be User.active (adding a new manager); actually it's User.objects.active() (adding a method to the manager). This really adds little value compared to User.objects.filter(is_active=True), so I change my vote to -0.

---

ORIGINAL COMMENT

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 User.active as the "user manager you always use", example:

for user in User.active.filter(please_spam_me=True):
    spam()

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.

[Tomasz Zieliński]

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).

[Jacob]

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 User.objects.active(). 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.

[twoolie]

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 forum.moderators.active %}
    {{ 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.

[Preston Holmes]

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: ​http://groups.google.com/group/django-developers/browse_thread/thread/66eeaee690417d4f

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.

[Jacob]

Milestone 1.4 deleted