[Django] #28687: Not 'Empty' Related Filter option
Django
-----------------------------------------------+------------------------
Reporter: Brillgen Developers | Owner: nobody
Type: Uncategorized | Status: new
Component: contrib.admin | Version: 2.0
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 1
UI/UX: 0 |
-----------------------------------------------+------------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/28687>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Brillgen Developers | Owner: Jake
| Barber
Type: Uncategorized | Status: assigned
Component: contrib.admin | Version: 2.0
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by Jake Barber):
* owner: nobody => Jake Barber
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/28687#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: Brillgen Developers | Owner: Jake
Cleanup/optimization | Status: assigned
Component: contrib.admin | Version: 2.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Changes (by Tim Graham):
* type: Uncategorized => Cleanup/optimization
* easy: 1 => 0
* stage: Unreviewed => Accepted
Comment:
Seems okay at first glance.
--
Ticket URL: <https://code.djangoproject.com/ticket/28687#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: Brillgen Developers | Owner: Jake
Type: | Barber
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/28687#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: Brillgen Developers | Owner: Jake
Type: | Barber
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Changes (by Carlton Gibson):
* needs_better_patch: 1 => 0
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/28687#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: Brillgen Developers | Owner: Jake
Type: | Barber
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Comment (by Claude Paroz):
As for the display value, it should mimic the
`model_admin.get_empty_value_display()` behavior
(`get_not_empty_value_display()`).
--
Ticket URL: <https://code.djangoproject.com/ticket/28687#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: Brillgen Developers | Owner: Jake
Type: | Barber
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/28687#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: Brillgen Developers | Owner: Jake
Type: | Barber
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Changes (by Carlton Gibson):
* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin
Comment:
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...
--
Ticket URL: <https://code.djangoproject.com/ticket/28687#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Brillgen Developers | Owner: Jake
Type: | Barber
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
* stage: Ready for checkin => Accepted
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/28687#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Brillgen Developers | Owner: Jake
Type: | Barber
Component: contrib.admin | Version: 2.0
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Tim Graham):
* status: assigned => closed
* resolution: => wontfix
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/28687#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: Brillgen Developers | Owner: Jake
Type: | Barber
Component: contrib.admin | Version: 2.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: closed => new
* resolution: wontfix =>
Comment:
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
{{{
#!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`
--
Ticket URL: <https://code.djangoproject.com/ticket/28687#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: Brillgen Developers | Owner: Jake
Type: | Barber
Component: contrib.admin | Version: 2.0
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => fixed
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/28687#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: Brillgen Developers | Owner: Jake
Type: | Barber
Cleanup/optimization | Status: closed
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* resolution: fixed => wontfix
--
Ticket URL: <https://code.djangoproject.com/ticket/28687#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: Brillgen Developers | Owner: Jake
Type: | Barber
Cleanup/optimization | Status: closed
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (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 [comment:11 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/28687#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: Brillgen Developers | Owner: Jake
Type: | Barber
Cleanup/optimization | Status: closed
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Sardorbek Imomaliev (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/28687#comment:14>