Code

Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#14880 closed (fixed)

raw_id_fields in admin does not work when limit_choices_to dictionary has value=False

Reported by: smallming@… Owned by: lukeplant
Component: contrib.admin Version: 1.2
Severity: Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

The related-lookup popup does not properly filter the queryset when the limit_choices_to dictionary has value=False. The models.py and admin.py demonstrate the problem.


To reproduce:

  1. Using the following models.py and admin.py, do
    Publisher(name='local', overseas=False).save()
    Publisher(name='overseas', overseas=True).save()
    
  2. Inside admin interface > add book, select related-lookup link to display popup. Only publisher "overseas" is available. Should display publisher "local" only.


Work-around: setting limit_choices_to={'overseas': 0} works, but this work-around is less intuitive than a fix.


models.py

from django.db import models

class Publisher(models.Model):
    name        = models.CharField(max_length=20)
    overseas    = models.BooleanField()

class Book(models.Model):
    title           = models.CharField(max_length=20)
    local_publisher = models.ForeignKey(Publisher, limit_choices_to={'overseas': False})

admin.py

from testproj.testapp.models import *
from django.contrib import admin

class BookAdmin(admin.ModelAdmin):
    raw_id_fields = ['local_publisher',]

class PublisherAdmin(admin.ModelAdmin):
    list_display = ['name', 'overseas']

admin.site.register(Book, BookAdmin)
admin.site.register(Publisher, PublisherAdmin)

Attachments (4)

ticket_14880_tests_1.2.3.diff (3.2 KB) - added by lukeplant 3 years ago.
Tests - patch against 1.2.3
ticket_14880_tests_1.2.4.diff (3.1 KB) - added by lukeplant 3 years ago.
Tests - patch against 1.2.4
ticket_14880_tests_1.2.X.diff (3.2 KB) - added by lukeplant 3 years ago.
Tests - patch against 1.2.X
ticket_14880_tests_trunk.diff (3.2 KB) - added by lukeplant 3 years ago.
Tests - patch against trunk

Download all attachments as: .zip

Change History (15)

comment:1 follow-up: Changed 4 years ago by julien

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

I tried your example, but when clicking the hourglass to select a publisher, the popup crashes with an error: SuspiciousOperation: Filtering by overseas not allowed. What version of Django are you using?

comment:2 in reply to: ↑ 1 Changed 4 years ago by smallming@…

Replying to julien:

I tried your example, but when clicking the hourglass to select a publisher, the popup crashes with an error: SuspiciousOperation: Filtering by overseas not allowed. What version of Django are you using?

I'm using 1.2.1. Tested using manage.py runserver.

comment:3 Changed 4 years ago by russellm

  • milestone set to 1.3
  • Triage Stage changed from Unreviewed to Accepted

Confirmed that this worked in 1.2.3, but doesn't in 1.2.4 or trunk. This makes it a release blocker for 1.3.

comment:4 Changed 4 years ago by russellm

  • Keywords blocker, regression added

comment:5 Changed 3 years ago by lukeplant

I don't think this was ever working. I have tried with a real life project, and with tests, and I get failure on 1.2.3. On 1.2.4 I get the suspicious operation thing. The original reporter was using 1.2.1 apparently, so this doesn't look like a regression.

I will attach patches for the tests - against 1.2.3, 1.2.4, most recent 1.2.X and most recent trunk, all slightly different.

Changed 3 years ago by lukeplant

Tests - patch against 1.2.3

Changed 3 years ago by lukeplant

Tests - patch against 1.2.4

Changed 3 years ago by lukeplant

Tests - patch against 1.2.X

Changed 3 years ago by lukeplant

Tests - patch against trunk

comment:6 Changed 3 years ago by smallming

I will like to add a line to admin.py in the initial bug report so it properly reflects the bug and "skip" SuspiciousOperation exception. Reason below.

class PublisherAdmin(admin.ModelAdmin):
    list_display = ['name', 'overseas']
    list_filter = ['overseas'] # Added for illustration

I was tracing the bug using 1.2.4, and found that the SuspiciousOperation is due to the lack of corresponding
ModelAdmin.list_filter specified. Adding list_filter = ['overseas'] to PublisherAdmin will lead to the original bug reported (at least in 1.2.4).

Perhaps a separate bug can be filed under "raw_id_fields in admin raise SuspiciousOperation when limit_choices_to is used without corresponding ModelAdmin.list_filter".

comment:7 Changed 3 years ago by ramiro

The SuspiciousOperation error you see in 1.2.4 is because of a problem in the security fix that triggered its release, it was fixed in r15140 and it is also already fixed in trunk. We can ignore the noise introduced by that.

Regarding he original report of this ticket, I can trace back this problem showing itself as far as Django 1.1 (and even 1.0, but there bugs like #9561 introduce additional noise)(tested with sqlite3).

In such environment, clicking in the magnifying glass icon opens a popup where only the overseas publisher is shown. Later, when trying to save the PublishedBook instance, validation rejects it so it seems there the filter overseas=False filter/validation is working in that stage.

comment:8 Changed 3 years ago by lukeplant

  • Keywords blocker, regression removed
  • milestone 1.3 deleted

OK, so this isn't a regression, we can remove the blocker/regression keywords. It would still be nice to be fixed for 1.3, but we don't have to.

For those looking to fix it, there are probably two routes:

  1. Change the generated query string to use '0' instead of 'False', since '0' does work.
  2. Fix the code that reads the query string to treat 'False' as boolean False (but obviously only for boolean fields).

I have no idea at the moment which is the 'right' way to fix it, one of them is likely to be cleaner that the other.

comment:9 Changed 3 years ago by lukeplant

  • Owner changed from nobody to lukeplant
  • Status changed from new to assigned

I will look at this, as it is related to the fix in #15103

comment:10 Changed 3 years ago by lukeplant

  • Resolution set to fixed
  • Status changed from assigned to closed

(In [15348]) Fixed #14880 - raw_id_fields in admin does not work when limit_choices_to dictionary has value=False

Thanks to smallming for the report.

comment:11 Changed 3 years ago by lukeplant

(In [15351]) [1.2.X] Fixed #14880 - raw_id_fields in admin does not work when limit_choices_to dictionary has value=False

Thanks to smallming for the report.

Backport of [15348] from trunk.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.