Opened 6 years ago

Closed 6 years ago

Last modified 5 years ago

#14467 closed (duplicate)

AllValuesFilterSpec does not work with Null

Reported by: oyvind Owned by: nobody
Component: contrib.admin Version: master
Severity: Keywords: Filterspec
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

AllValuesFilterSpec does not make the proper __isnull query for nullable / None values.

<a href="?field=None">None</a>

Only redirects to ?e=1 

Would expect the link to be

<a href="?field__isnull=1">None</a>

Attachments (6)

fix_for_None_values.diff (415 bytes) - added by oyvind 6 years ago.
Fix without touching FilterSpec
AllValuesFilterSpec_Null.diff (2.0 KB) - added by oyvind 6 years ago.
Proper fix
AllValuesFilterSpec_Null_Tests.py (2.4 KB) - added by oyvind 6 years ago.
Start of tests for patch, not sure where to put them
AllValuesFilterSpec_Null.2.diff (5.3 KB) - added by oyvind 6 years ago.
Added patch with regressiontests, testing cl.get_query_set
AllValuesFilterSpec_Null.3.diff (5.2 KB) - added by Anssi Kääriäinen 6 years ago.
Small refactoring of .2.diff, now without test_sqlite changes!
AllValuesFilterSpec_Null.4.diff (5.0 KB) - added by oyvind 6 years ago.
New patch using RequestFactory, Python comments, no delete() in tearDown, added back include_none

Download all attachments as: .zip

Change History (17)

Changed 6 years ago by oyvind

Attachment: fix_for_None_values.diff added

Fix without touching FilterSpec

Changed 6 years ago by oyvind

Proper fix

comment:1 Changed 6 years ago by oyvind

Has patch: set
Needs documentation: unset
Needs tests: set
Patch needs improvement: unset

Changed 6 years ago by oyvind

Start of tests for patch, not sure where to put them

comment:2 Changed 6 years ago by Anssi Kääriäinen

Resolution: invalid
Status: newclosed

I am pretty sure that qs.filter(field=None) and qs.filter(field__isnull=1) are the exact same query. So, href="?field=None" should be fine.

comment:3 Changed 6 years ago by Anssi Kääriäinen

Resolution: invalid
Status: closedreopened
Triage Stage: UnreviewedAccepted

To clarify the problem: href="?field=None" will result in query qs.filter(field='None'). Reopening...

comment:4 Changed 6 years ago by oyvind

Needs tests: unset

comment:5 Changed 6 years ago by Anssi Kääriäinen

Component: Uncategorizeddjango.contrib.admin
Keywords: Filterspec added

Should the tests try to filter on some field containing None, and see that everything works all the way to database? That is, the mock request actually returns the right filtered list of objects when None filtering is requested. Test behavior, not implementation and all that...

comment:6 Changed 6 years ago by oyvind

How would i actually test that, there is no self.assertNotRaises, try / except in the test?

comment:7 Changed 6 years ago by Alex Gaynor

Just call the function, if it raises an exception the test will fail (technically error, but it amounts to the same thing).

Changed 6 years ago by oyvind

Added patch with regressiontests, testing cl.get_query_set

Changed 6 years ago by Anssi Kääriäinen

Small refactoring of .2.diff, now without test_sqlite changes!

comment:8 Changed 6 years ago by Anssi Kääriäinen

Triage Stage: AcceptedReady for checkin

IMHO this is now ready for committer. Please see the comment above lookup_choice_vals in choices().

comment:9 Changed 6 years ago by Łukasz Rekucki

Triage Stage: Ready for checkinAccepted

IMHO, the TestCase needs some code style changes:

  • Regression test can now use django.test.client.RequestFactory to mock a request instead of creating it's own.
  • You don't need to delete created ORM objects in tearDown() - the DB will get flushed anyway.
  • Python has comments, you don't need to put noop string expressions.

Also, looping through the list (using "in") to check if there is a None, when we just looped feels weird.

Changed 6 years ago by oyvind

New patch using RequestFactory, Python comments, no delete() in tearDown, added back include_none

comment:10 Changed 6 years ago by oyvind

Resolution: duplicate
Status: reopenedclosed

Duplicate of #8528

comment:11 Changed 5 years ago by Jacob

milestone: 1.3

Milestone 1.3 deleted

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