Opened 13 years ago

Closed 13 years ago

Last modified 12 years ago

#14467 closed (duplicate)

AllValuesFilterSpec does not work with Null

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

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

Download all attachments as: .zip

Change History (17)

by oyvind, 13 years ago

Attachment: fix_for_None_values.diff added

Fix without touching FilterSpec

by oyvind, 13 years ago

Proper fix

comment:1 by oyvind, 13 years ago

Has patch: set
Needs tests: set

by oyvind, 13 years ago

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

comment:2 by Anssi Kääriäinen, 13 years ago

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 by Anssi Kääriäinen, 13 years ago

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 by oyvind, 13 years ago

Needs tests: unset

comment:5 by Anssi Kääriäinen, 13 years ago

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 by oyvind, 13 years ago

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

comment:7 by Alex Gaynor, 13 years ago

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

by oyvind, 13 years ago

Added patch with regressiontests, testing cl.get_query_set

by Anssi Kääriäinen, 13 years ago

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

comment:8 by Anssi Kääriäinen, 13 years ago

Triage Stage: AcceptedReady for checkin

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

comment:9 by Łukasz Rekucki, 13 years ago

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.

by oyvind, 13 years ago

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

comment:10 by oyvind, 13 years ago

Resolution: duplicate
Status: reopenedclosed

Duplicate of #8528

comment:11 by Jacob, 12 years ago

milestone: 1.3

Milestone 1.3 deleted

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