Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix of Max Choices bug #32

Merged
merged 8 commits into from
Nov 1, 2016
Merged

Fix of Max Choices bug #32

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
cc19130
select2 only initializes if there are choices available. otherwise, t…
Jun 7, 2016
63fd630
Revert "select2 only initializes if there are choices available. othe…
Jun 7, 2016
0fb02dc
updated query for choices for select2 field so that it will take only…
Jun 7, 2016
9d41c04
fixed failing test to account for new distinct for max choices
Jun 7, 2016
898581b
Changes URL declaration to avoid deprecated pattern
hermanocabral Jul 12, 2016
2b124aa
removed print statement from test
Aug 18, 2016
2c10219
refactored retrieval of choices so that the db is getting distinct va…
Aug 19, 2016
dbed972
pep8
Aug 19, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion advanced_filters/tests/test_views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,14 @@ def test_database_choices(self):
def test_more_than_max_database_choices(self):
factories.Client.create_batch(5, assigned_to=self.user)
view_url = reverse(self.url_name, kwargs=dict(
model='customers.Client', field_name='first_name'))
model='customers.Client', field_name='id'))
res = self.client.get(view_url)
self.assert_json(res, {'results': []})

@override_settings(ADVANCED_FILTERS_MAX_CHOICES=4)
def test_distinct_database_choices(self):
factories.Client.create_batch(5, assigned_to=self.user, email="foo@bar.com")
view_url = reverse(self.url_name, kwargs=dict(
model='customers.Client', field_name='email'))
res = self.client.get(view_url)
self.assert_json(res, {'results': [{'id': 'foo@bar.com', 'text': 'foo@bar.com'}]})
8 changes: 3 additions & 5 deletions advanced_filters/urls.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,8 @@
from django.conf.urls import patterns, url
from django.conf.urls import url

from advanced_filters.views import GetFieldChoices

urlpatterns = patterns(
# API
'',
urlpatterns = [
url(r'^field_choices/(?P<model>.+)/(?P<field_name>.+)/?',
GetFieldChoices.as_view(),
name='afilters_get_field_choices'),
Expand All @@ -13,4 +11,4 @@
url(r'^field_choices/$',
GetFieldChoices.as_view(),
name='afilters_get_field_choices'),
)
]
5 changes: 2 additions & 3 deletions advanced_filters/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,9 +69,8 @@ def get(self, request, model=None, field_name=None):
logger.debug('No choices calculated for field %s of type %s',
field, type(field))
else:
choices = model_obj.objects.values_list(field.name, flat=True)
if choices.count() < max_choices:
choices = set(choices)
choices = model_obj.objects.values_list(field.name, flat=True).distinct()
Copy link
Member

@asfaltboy asfaltboy Aug 19, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hope you don't mind me commenting while you work, I think order_by explicitly here is really a must, since the model may have default ordering that differs from our field.name, which may break the distinct query, as noted here.

... if you use a values() query to restrict the columns selected, the columns used in any order_by() (or default model ordering) will still be involved and may affect uniqueness of the results.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should add an explicit order_by in case the model's default queryset manager doesn't order_by a field outside of our values list. This is explained in distinct() docs - first note.

if choices.count() <= max_choices:
choices = zip(choices, choices)
logger.debug('Choices found for field %s: %s',
field.name, choices)
Expand Down