[Django] #30149: "Empty value selected check in Admin Filter prevents subclassing"
Django
-----------------------------------------------+------------------------
Reporter: Sardorbek Imomaliev | Owner: nobody
Type: Uncategorized | Status: new
Component: contrib.admin | Version: 2.1
Severity: Normal | Keywords: admin
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-----------------------------------------------+------------------------
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
{{{
#!div style="font-size: 80%"
Code highlighting:
{{{#!python
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
{{{
#!div style="font-size: 80%"
Code highlighting:
{{{#!python
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
--
Ticket URL: <https://code.djangoproject.com/ticket/30149>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Sardorbek Imomaliev | Owner: nobody
Type: Uncategorized | Status: new
Component: contrib.admin | Version: 2.1
Keywords: admin | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--
Ticket URL: <https://code.djangoproject.com/ticket/30149#comment:1>
Django
Reporter: Sardorbek Imomaliev | Owner: nobody
Type: Uncategorized | Status: new
Component: contrib.admin | Version: 2.1
Keywords: admin | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
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`.
--
Ticket URL: <https://code.djangoproject.com/ticket/30149#comment:2>
Django
Reporter: Sardorbek Imomaliev | Owner: nobody
Type: Uncategorized | Status: new
Component: contrib.admin | Version: 2.1
Keywords: admin | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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 part at least then makes sense).
`
--
Ticket URL: <https://code.djangoproject.com/ticket/30149#comment:3>
Django
Reporter: Sardorbek Imomaliev | Owner: nobody
Type: Uncategorized | Status: new
Component: contrib.admin | Version: 2.1
Keywords: admin | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Sardorbek Imomaliev):
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 [comment:3 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).
> `
--
Ticket URL: <https://code.djangoproject.com/ticket/30149#comment:4>
Django
Reporter: Sardorbek Imomaliev | Owner: nobody
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: admin | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* version: 2.1 => master
* type: Uncategorized => Cleanup/optimization
* stage: Unreviewed => Accepted
Comment:
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!
--
Ticket URL: <https://code.djangoproject.com/ticket/30149#comment:5>
Django
Reporter: Sardorbek Imomaliev | Owner: nobody
Component: contrib.admin | Version: dev
Severity: Normal | Resolution:
Keywords: admin | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Natalia Bidart):
[https://github.com/django/django/pull/10951|PR] is 4 years old, I
requested a refresh.
--
Ticket URL: <https://code.djangoproject.com/ticket/30149#comment:6>
Django
Severity: Normal | Resolution:
Keywords: admin | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Ülgen Sarıkavak):
* cc: Ülgen Sarıkavak (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/30149#comment:7>
Django
Component: contrib.admin | Version: dev
Severity: Normal | Resolution:
Keywords: admin | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by VIZZARD-X):
I have submitted a pull request that fixes this issue.
The fix updates `RelatedFieldListFilter.__init__` to explicitly handle
"false-y" string values (like `'False'` and `'0'`) in the query
parameters, ensuring they are interpreted as a boolean `False` rather than
`True`.
I have also added a regression test in `tests/admin_filters/tests.py` to
verify the fix.
PR: https://github.com/django/django/pull/20489
--
Ticket URL: <https://code.djangoproject.com/ticket/30149#comment:8>
Django
Component: contrib.admin | Version: dev
Severity: Normal | Resolution:
Keywords: admin | Triage Stage: Accepted
Easy pickings: 0 | UI/UX: 0
Changes (by VIZZARD-X):
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/30149#comment:9>