Opened 3 years ago

Closed 17 months ago

Last modified 16 months 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 Changed 3 years ago by Jake Barber

Owner: changed from nobody to Jake Barber
Status: newassigned

comment:2 Changed 2 years ago by Tim Graham

Easy pickings: unset
Summary: Not 'Empty' Related Filter optionAdd a 'Not Empty' option to admin's related filter
Triage Stage: UnreviewedAccepted
Type: UncategorizedCleanup/optimization

Seems okay at first glance.

comment:3 Changed 2 years ago by Carlton Gibson

Has patch: set
Patch needs improvement: set

comment:4 Changed 2 years ago by Carlton Gibson

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 Changed 2 years ago by Claude Paroz

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

comment:6 Changed 2 years ago by Carlton Gibson

Patch needs improvement: set

comment:7 Changed 2 years ago by Carlton Gibson

Patch needs improvement: unset
Triage Stage: AcceptedReady 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 Changed 2 years ago by Tim Graham

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

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 Changed 22 months ago by Tim Graham

Resolution: wontfix
Status: assignedclosed

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 Changed 17 months ago by Sardorbek Imomaliev

Resolution: wontfix
Status: closednew

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 17 months ago by Sardorbek Imomaliev (previous) (diff)

comment:11 Changed 17 months ago by Carlton Gibson

Resolution: fixed
Status: newclosed

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 Changed 17 months ago by Tim Graham

Resolution: fixedwontfix

comment:13 in reply to:  11 Changed 16 months ago by Sardorbek Imomaliev

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 Changed 16 months ago by Sardorbek Imomaliev

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