UI Changes; Support for lt, lte, gt, gte; and Advanced Filter Admin queryset filtering by User #25
Conversation
…perators to 'Equals'
…to check Delete is not clear to end users.
…iew/edit filters that they create (unless they are a superuser)
| @@ -230,7 +230,7 @@ def forms(self): | |||
|
|
|||
| AFQFormSet = formset_factory( | |||
| AdvancedFilterQueryForm, formset=AdvancedFilterFormSet, | |||
| extra=1, can_delete=True) | |||
| extra=0, can_delete=True) | |||
|
|
|||
| AFQFormSetNoExtra = formset_factory( | |||
There was a problem hiding this comment.
AFQFormSetNoExtra, defined just below (line 235), already has 0 extra forms. And I believe the current default is used when creating a new filter, which would make sense wince a filter with 0 rules is not really interesting.
Perhaps it would be better to add an require an extra form only when adding, but not when editing rules? We can toggle between the forms by passing extra_form=True/False in admin.py.
|
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. |
…ermissions to change or delete their filters
…form checks for falsy value for extra rather than extra just being None
|
@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.
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. |
| @@ -361,7 +361,7 @@ def initialize_form(self, instance, model, data=None, extra=None): | |||
| AdvancedFilterQueryForm._parse_query_dict( | |||
| field_data, model)) | |||
|
|
|||
| formset = AFQFormSetNoExtra if extra is None else AFQFormSet | |||
| formset = AFQFormSetNoExtra if not extra or not instance else AFQFormSet | |||
There was a problem hiding this comment.
You're right about not extra, good catch!
However, I'm not sure if I like or not instance here. If we're editing a new form and get an error on submit, the instance is still None, but I think the user expects to see an extra form row (if it was there before the error).
|
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 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 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. |
|
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 😄 |
|
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. 😉 |
|
Looks great! Thanks again @pjpassa |
No description provided.