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 , 6 years ago
Summary: | "Empty value selected check in Admin Filter prevents subclassing" → Empty value selected check in Admin Filter prevents subclassing |
---|
follow-up: 4 comment:3 by , 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).
`
comment:4 by , 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 , 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:7 by , 8 months ago
Cc: | added |
---|
Hi Sardorbek,
As I commented on #28687, I'm not understanding your issue exactly:
I'm not seeing how this comes up:
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
.