Opened 7 years ago

Closed 5 years ago

Last modified 5 years ago

#28687 closed Cleanup/optimization (wontfix)

Add a 'Not Empty' option to admin's related filter

Reported by: Brillgen Developers Owned by: Jake Barber
Component: contrib.admin Version: 2.0
Severity: Normal Keywords:
Cc: Sardorbek Imomaliev Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

Currently the admin related filter has an 'All' and a 'Empty Value' option ...however it does not have an equally easy to implement and performant NOT 'Empty' option for those cases where we want to filter like that. It maps cleanly onto isnull = True/False.

Would this be accepted if a patch was added since its rather easy to do but the core needs to be ok with it first.

Change History (14)

comment:1 by Jake Barber, 7 years ago

Owner: changed from nobody to Jake Barber
Status: new → assigned

comment:2 by Tim Graham, 6 years ago

Easy pickings: unset
Summary: Not 'Empty' Related Filter option → Add a 'Not Empty' option to admin's related filter
Triage Stage: Unreviewed → Accepted
Type: Uncategorized → Cleanup/optimization

Seems okay at first glance.

comment:3 by Carlton Gibson, 6 years ago

Has patch: set
Patch needs improvement: set

comment:4 by Carlton Gibson, 6 years ago

Patch needs improvement: unset

PR is functionally OK.

It hardcodes a (translatable) "Not Empty" string. I'm not sure if we need to do something more sophisticated there (or take a different approach, such as using a filter subclass if you want this behaviour) or if it's OK as it is.

comment:5 by Claude Paroz, 6 years ago

As for the display value, it should mimic the model_admin.get_empty_value_display() behavior (get_not_empty_value_display()).

comment:6 by Carlton Gibson, 6 years ago

Patch needs improvement: set

comment:7 by Carlton Gibson, 6 years ago

Patch needs improvement: unset
Triage Stage: Accepted → Ready for checkin

OK, patch looks good.

I have a small reservation about whether we want yet more API on ModelAdmin to add the not_empty_value_display option, rather than steering users to subclassing FieldListFilter themselves, but bar that...

comment:8 by Tim Graham, 6 years ago

Patch needs improvement: set
Triage Stage: Ready for checkin → Accepted

I agree -- get_not_empty_value_display() looks heavy handed for this use case. get_empty_value_display() is used elsewhere besides the RelatedFieldListFilter and it corresponds to the values displayed in the changelist -- that's not the case for the "not empty" case.

comment:9 by Tim Graham, 6 years ago

Resolution: → wontfix
Status: assigned → closed

After some discussion on the pull request, I think it's better if users subclass the relevant list filter and add the "not empty" option if they desire it. It doesn't seem like a particularly common need and that way it's not cluttering things for everyone else.

comment:10 by Sardorbek Imomaliev, 5 years ago

Resolution: wontfix
Status: closed → new

I think we should reopen this and fix an issue with current RelatedFieldListFilter. For example currently to add Not Empty choice you need to do this

Code highlighting: 

class NotEmptyFilter(admin.RelatedFieldListFilter):
    @property
    def include_empty_choice(self):
        # FIXME: empty value selected incorrectly override in choices
        return False

    def has_output(self):
        return super().has_output() + 2

    def choices(self, changelist):
        yield from super().choices(changelist)
        yield {
            'selected': self.lookup_val_isnull == 'True',
            'query_string': changelist.get_query_string(
                {self.lookup_kwarg_isnull: 'True'}, [self.lookup_kwarg]
            ),
            'display': _('Empty'),
        yield {
            'selected': self.lookup_val_isnull == 'False',
            'query_string': changelist.get_query_string(
                {self.lookup_kwarg_isnull: 'False'}, [self.lookup_kwarg]
            ),
            'display': _('Not Empty'),
        }

But you should be able to do just

Code highlighting: 

class NotEmptyFilter(admin.RelatedFieldListFilter):
    def has_output(self):
        return super().has_output() + 1

    def choices(self, changelist):
        yield from super().choices(changelist)
        yield {
            'selected': self.lookup_val_isnull == 'False',
            'query_string': changelist.get_query_string(
                {self.lookup_kwarg_isnull: 'False'}, [self.lookup_kwarg]
            ),
            'display': _('Not Empty'),
        }

Currently this is not possible because of bool(self.lookup_val_isnull) check in selected for Empty choice, and if we add Not Empty choice like in second example we will get both Empty and Not Empty choices rendered as selected on Not Empty

This is easy to fix, and I would like to provide PR if this change is OK

Last edited 5 years ago by Sardorbek Imomaliev (previous) (diff)

comment:11 by Carlton Gibson, 5 years ago

Resolution: → fixed
Status: new → closed

Hi Sardorbek.

This looks like a (related but) separate issue. Can you submit a new ticket, something along the lines of "Empty value selected check in Admin Filter prevents subclassing" and we can have a look.

At first glance I can't see how both bool(self.lookup_val_isnull) and self.lookup_val_isnull == 'False' evaluate as True in the Not Empty case. (???) Surely the existing empty_choice logic would already be broken if they did.

If you have a PR (with a test case showing the broken example) available (or can provide one when opening the ticket) it makes it much easier to assess.

Thanks.

comment:12 by Tim Graham, 5 years ago

Resolution: fixed → wontfix

in reply to:  11 comment:13 by Sardorbek Imomaliev, 5 years ago

Hi Carlton,

Thanks for reply, it is not broken because there is no Not Empty choice in django itself
created new issue https://code.djangoproject.com/ticket/30149

Replying to Carlton Gibson:

Hi Sardorbek.

This looks like a (related but) separate issue. Can you submit a new ticket, something along the lines of "Empty value selected check in Admin Filter prevents subclassing" and we can have a look.

At first glance I can't see how both bool(self.lookup_val_isnull) and self.lookup_val_isnull == 'False' evaluate as True in the Not Empty case. (???) Surely the existing empty_choice logic would already be broken if they did.

If you have a PR (with a test case showing the broken example) available (or can provide one when opening the ticket) it makes it much easier to assess.

Thanks.

comment:14 by Sardorbek Imomaliev, 5 years ago

Cc: Sardorbek Imomaliev added
Note: See TracTickets for help on using tickets.
Back to Top