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

UI Changes; Support for lt, lte, gt, gte; and Advanced Filter Admin queryset filtering by User #25

Closed pjpassa closed 8 years ago

asfaltboy commented 8 years ago

Hello @pjpassa, thank you for contributing!

I like most of the changes, but please take note of my comments. Also, the tests now need correction; looks like it has to do with the queryset/change/delete permission checks.

pjpassa commented 8 years ago

@asfaltboy I've added/updated tests as requested and made the changes you requested, too. There is one more bug that I'm trying to track down, and I'll need to figure out what failed in the latest build.

1

When you change the field you get a dropdown for value that is unusable.

2

But when you add another filter everything fixes itself.

3

I haven't been able to track down the issue, but if you can help me track down what is wrong, I'd be glad to fix it for this pull request.

asfaltboy commented 8 years ago

Hi @pjpassa, thank you again for your work.

The fancy select box is an easy-select2 widget. It is indeed initialised on field selection, in a method called OperatorHandlers.initialize_select2.

That code issues a request to query for available value choices for a given field and displays a smart select box which should also accept custom values.

Honestly I don’t know of an issue with this, but you can disable it for given fields by updating settings.ADVANCED_FILTERS_DISABLE_FOR_FIELDS = (‘email’, ). Please refer to the GetFieldChoices view for more info.

If you do find an issue that you can easily reproduce, kindly open an issue.

On a side note, we're definitely lacking on usability, documentation and tests for the frontend features, including this widget, other javascript and styles.

Correction: I am aware of a number of easy-select2 issues as well as other frontend bugs.

asfaltboy commented 8 years ago

Regarding the failing test, I have a feeling this has to do with "extra forms fix". You can try reverting only that change and re-run the tests. I think it would be easier to pinpoint if each feature was a separate pull request, for next time 😄

pjpassa commented 8 years ago

I believe those last 2 commits cover everything. The QuantifiedCode issue is related to the new test I created, and I don't see anything wrong with the problems it lists, to be honest. Especially seeing that the same "problems" exist in other places in the code base and I would break consistency if I fixed them.

I'll continue to do testing on the front-end problem and create an issue as necessary. Or if I can fix it, I'll create a new pull request. 😉

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.1%) to 96.198% when pulling 5cdcbe98eb4eac571b9b2dfea552bac76ccf88ec on worthwhile:develop into 5fc8c65259406a2a6098446d72de4810805624db on modlinltd:develop.

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.1%) to 96.198% when pulling 4f928dafb37c555c72f052f7cdcc41fd54258f2b on worthwhile:develop into 5fc8c65259406a2a6098446d72de4810805624db on modlinltd:develop.

asfaltboy commented 8 years ago

Looks great! Thanks again @pjpassa