#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 by , 7 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:2 by , 7 years ago
Easy pickings: | unset |
---|---|
Summary: | Not 'Empty' Related Filter option → Add a 'Not Empty' option to admin's related filter |
Triage Stage: | Unreviewed → Accepted |
Type: | Uncategorized → Cleanup/optimization |
comment:3 by , 6 years ago
Has patch: | set |
---|---|
Patch needs improvement: | set |
comment:4 by , 6 years ago
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 by , 6 years ago
As for the display value, it should mimic the model_admin.get_empty_value_display()
behavior (get_not_empty_value_display()
).
comment:6 by , 6 years ago
Patch needs improvement: | set |
---|
comment:7 by , 6 years ago
Patch needs improvement: | unset |
---|---|
Triage Stage: | Accepted → Ready 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 by , 6 years ago
Patch needs improvement: | set |
---|---|
Triage Stage: | Ready for checkin → Accepted |
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 by , 6 years ago
Resolution: | → wontfix |
---|---|
Status: | assigned → closed |
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 by , 6 years ago
Resolution: | wontfix |
---|---|
Status: | closed → new |
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
follow-up: 13 comment:11 by , 6 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
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 by , 6 years ago
Resolution: | fixed → wontfix |
---|
comment:13 by , 6 years ago
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)
andself.lookup_val_isnull == 'False'
evaluate asTrue
in theNot Empty
case. (???) Surely the existingempty_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 by , 6 years ago
Cc: | added |
---|
Seems okay at first glance.