modlinltd / django-advanced-filters

Add advanced filtering abilities to Django admin
https://pypi.org/project/django-advanced-filters/
MIT License
771 stars 173 forks source link

Fix of Max Choices bug #32

Closed pjpassa closed 7 years ago

pjpassa commented 8 years ago

Max choices used to only send data to the front end if the total number of instances of the model was less than the max choices setting.

I refactored the code so that the set was created earlier. This way the view will return data if the number of unique values is less than the max choices setting.

coveralls commented 8 years ago

Coverage Status

Coverage decreased (-0.007%) to 96.19% when pulling 2b124aa75688e9260291f3d1df8d9830a7d149d0 on worthwhile:feat/max-choices-fix into 119c76d7198b84148f5662face3e62af9241895b on modlinltd:develop.

asfaltboy commented 8 years ago

Hi @pjpassa, thanks for the contribution! This fix makes sense but, I'm worried about potentially harmful performance if the query will return a very large number of values. That was the reason we used query.count() instead, to avoid potentially fetching large amount of values just to count them.

What if we order_by(field.name).distinct().count() instead? For our simple query this should work correctly I believe.

Additionally, I would add a more specific test case, for example leaving the existing name with identical 5 names, and asserting the view returns the 1 distinct name. Later, randomizing the names (or using ID field), and asserting the max limit is reached.

coveralls commented 8 years ago

Coverage Status

Coverage decreased (-2.1%) to 94.095% when pulling dbed972ee7b524c86224ce5c30989e7637fa2e9b on worthwhile:feat/max-choices-fix into 119c76d7198b84148f5662face3e62af9241895b on modlinltd:develop.

coveralls commented 8 years ago

Coverage Status

Coverage decreased (-0.007%) to 96.19% when pulling dbed972ee7b524c86224ce5c30989e7637fa2e9b on worthwhile:feat/max-choices-fix into 119c76d7198b84148f5662face3e62af9241895b on modlinltd:develop.