Opened 12 months ago

Closed 5 weeks ago

#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: 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 (9)

comment:1 Changed 12 months ago by Jake Barber

Owner: changed from nobody to Jake Barber
Status: newassigned

comment:2 Changed 9 months ago by Tim Graham

Easy pickings: unset
Summary: Not 'Empty' Related Filter optionAdd a 'Not Empty' option to admin's related filter
Triage Stage: UnreviewedAccepted
Type: UncategorizedCleanup/optimization

Seems okay at first glance.

comment:3 Changed 5 months ago by Carlton Gibson

Has patch: set
Patch needs improvement: set

comment:4 Changed 5 months ago by Carlton Gibson

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 Changed 5 months ago by Claude Paroz

As for the display value, it should mimic the model_admin.get_empty_value_display() behavior (get_not_empty_value_display()).

comment:6 Changed 5 months ago by Carlton Gibson

Patch needs improvement: set

comment:7 Changed 4 months ago by Carlton Gibson

Patch needs improvement: unset
Triage Stage: AcceptedReady 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 Changed 3 months ago by Tim Graham

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

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 Changed 5 weeks ago by Tim Graham

Resolution: wontfix
Status: assignedclosed

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.

Note: See TracTickets for help on using tickets.
Back to Top