Opened 6 years ago

Last modified 8 months ago

#30149 new Cleanup/optimization

Empty value selected check in Admin Filter prevents subclassing

Reported by: Sardorbek Imomaliev Owned by: nobody
Component: contrib.admin Version: dev
Severity: Normal Keywords: admin
Cc: Ülgen Sarıkavak Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

as per https://code.djangoproject.com/ticket/28687?cnum_edit=10#comment:11 creating new issue, will get to writing test and PR next week

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

Change History (7)

comment:1 by Sardorbek Imomaliev, 6 years ago

Summary: "Empty value selected check in Admin Filter prevents subclassing" → Empty value selected check in Admin Filter prevents subclassing

comment:2 by Carlton Gibson, 6 years ago

Hi Sardorbek,

As I commented on #28687, I'm not understanding your issue exactly:

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

I'm not seeing how this comes up:

...we will get both Empty and Not Empty choices rendered as selected on Not Empty.

Can you provide a test case or sample project that shows exactly what you mean here?

Sorry if I'm just being slow. I'll leave this open for a few days more to await your reply. Otherwise I'll close as needsinfo.

Last edited 6 years ago by Carlton Gibson (previous) (diff)

comment:3 by Carlton Gibson, 6 years ago

OK. Typing that post, and correcting my backticks, I see the issue with bool(self.lookup_val_isnull) is that you want to compare against the string 'False'... (that part at least then makes sense).
`

Last edited 6 years ago by Carlton Gibson (previous) (diff)

in reply to:  3 comment:4 by Sardorbek Imomaliev, 6 years ago

Yes this is exactly the issue because, self.lookup_val_isnull can be either 'True' or 'False' as strings, and if they are stored as strings they both will evaluate as True if you do bool(self.lookup_val_isnull). So we need to do string comparison, or change how self.lookup_val_isnull is stored, which is not backwards compatible. Would it be ok if I provide fix?

Replying to Carlton Gibson:

OK. Typing that post, and correcting my backticks, I see the issue with bool(self.lookup_val_isnull) is that you want to compare against the string 'False'... (that part at least then makes sense).
`

comment:5 by Carlton Gibson, 6 years ago

Triage Stage: Unreviewed → Accepted
Type: Uncategorized → Cleanup/optimization
Version: 2.1 → master

Hmmm. Not sure...

  • Given that you can currently set any value at all in the query string to have Empty filtering working, it's guaranteed that people will be doing just that. So if we start comparing against the string 'True' we'll be breaking someone's site.

We might argue, "well they shouldn't do that" but I look at this, which is what we're doing (in e.g. RelatedFieldListFilter.__init__()): 

        self.lookup_val_isnull = request.GET.get(self.lookup_kwarg_isnull)

...and I think we should be using a form to process the raw GET, validate it and extract the value as a boolean.
(I know we trust users in the admin but it's raw user input...)

So there's various shoulds floating around. I'm not clear that one trumps the other at this point in time.

So questions:

  • what does the subclass look like overriding __init__() to use a form to process the GET parameter as boolean?

If it's not too bad, we might still say wontfix. However...

  • Does anything break in the test suite if we make that change in the existing filters?

If not, we could probably make the change.
If there are BC issues, we could maybe use a widget that accepted Obviously False values as False (e.g. 'False', '0' el al) with everything else as True.

Would it be ok if I provide fix?

Yep. Always. 🙂

I'll Accept this now, on the basis that you want to work on it, and that the implementation doesn't raise any issues. (It may be we have to decline in the end, but it should be OK, at first glance.)

Thanks!

comment:6 by Natalia Bidart, 19 months ago

​https://github.com/django/django/pull/10951|PR is 4 years old, I requested a refresh.

Version 0, edited 19 months ago by Natalia Bidart (next)

comment:7 by Ülgen Sarıkavak, 8 months ago

Cc: Ülgen Sarıkavak added
Note: See TracTickets for help on using tickets.
Back to Top