Opened 5 years ago

Closed 5 years ago

Last modified 4 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 5 years ago.
Fix without touching FilterSpec
AllValuesFilterSpec_Null.diff (2.0 KB) - added by oyvind 5 years ago.
Proper fix
AllValuesFilterSpec_Null_Tests.py (2.4 KB) - added by oyvind 5 years ago.
Start of tests for patch, not sure where to put them
AllValuesFilterSpec_Null.2.diff (5.3 KB) - added by oyvind 5 years ago.
Added patch with regressiontests, testing cl.get_query_set
AllValuesFilterSpec_Null.3.diff (5.2 KB) - added by akaariai 5 years ago.
Small refactoring of .2.diff, now without test_sqlite changes!
AllValuesFilterSpec_Null.4.diff (5.0 KB) - added by oyvind 5 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 5 years ago by oyvind

Fix without touching FilterSpec

Changed 5 years ago by oyvind

Proper fix

comment:1 Changed 5 years ago by oyvind

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

Changed 5 years ago by oyvind

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

comment:2 Changed 5 years ago by akaariai

  • Resolution set to invalid
  • Status changed from new to closed

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 5 years ago by akaariai

  • Resolution invalid deleted
  • Status changed from closed to reopened
  • Triage Stage changed from Unreviewed to Accepted

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

comment:4 Changed 5 years ago by oyvind

  • Needs tests unset

comment:5 Changed 5 years ago by akaariai

  • Component changed from Uncategorized to django.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 5 years ago by oyvind

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

comment:7 Changed 5 years ago by Alex

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

Changed 5 years ago by oyvind

Added patch with regressiontests, testing cl.get_query_set

Changed 5 years ago by akaariai

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

comment:8 Changed 5 years ago by akaariai

  • Triage Stage changed from Accepted to Ready for checkin

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

comment:9 Changed 5 years ago by lrekucki

  • Triage Stage changed from Ready for checkin to Accepted

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

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

comment:10 Changed 5 years ago by oyvind

  • Resolution set to duplicate
  • Status changed from reopened to closed

Duplicate of #8528

comment:11 Changed 4 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

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