Opened 20 months ago

Closed 4 months ago

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

Owner: changed from nobody to Jake Barber
Status: newassigned

comment:2 Changed 17 months 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 13 months ago by Carlton Gibson

Has patch: set
Patch needs improvement: set

comment:4 Changed 13 months 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 13 months 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 13 months ago by Carlton Gibson

Patch needs improvement: set

comment:7 Changed 12 months 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 11 months 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 9 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 4 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 4 months ago by Sardorbek Imomaliev (previous) (diff)

comment:11 Changed 4 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 4 months ago by Tim Graham

Resolution: fixedwontfix

comment:13 in reply to:  11 Changed 4 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 3 months ago by Sardorbek Imomaliev

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