encode / django-rest-framework

Web APIs for Django. 🎸
https://www.django-rest-framework.org
Other
28.4k stars 6.84k forks source link

Filter form rendering multiple fields #4434

Closed scottrus closed 8 years ago

scottrus commented 8 years ago

Checklist

Using the example from the current docs, if providing multiple search methods on a field then the field is rendered multiple times in the filter form html. Given the example below, two copies of the 'category' field will be rendered for each filter type.

import django_filters
from myapp.models import Product
from myapp.serializers import ProductSerializer
from rest_framework import filters
from rest_framework import generics

class ProductFilter(filters.FilterSet):
    min_price = django_filters.NumberFilter(name="price", lookup_expr='gte')
    max_price = django_filters.NumberFilter(name="price", lookup_expr='lte')
    class Meta:
        model = Product
        fields = {
                'category': ['exact', 'contains'], 
                'in_stock': ['exact'],
                'min_price': ['exact'],
                'max_price': ['exact']:
        }

class ProductList(generics.ListAPIView):
    queryset = Product.objects.all()
    serializer_class = ProductSerializer
    filter_backends = (filters.DjangoFilterBackend,)
    filter_class = ProductFilter

Expected behavior

A single field should be rendered. An for the match type should be given as drop down or other form element.

Actual behavior

Multiple fields rendered confusingly to the end user.

carltongibson commented 8 years ago

Hmmm.

This isn't an issue with DRF. At best it's a Django Filter issue. I'm really sure it's even that...

You're defining multiple filters, each has a form field, each is rendered in the form. That's just How it worksβ„’

I'm not sure what to say beyond that...

You'll need to see how close you get experimenting.

I'm happy for you to open an issue on Django Filter if there's some specific change you think we might make.

tomchristie commented 8 years ago

Presumably the issue here is that this: 'category': ['exact', 'contains'], renders as two separate but identically displayed fields, that cannot be differentiated between by the end user, right?

carltongibson commented 8 years ago

Yeah. There's an open PR about improving the form labelling.

tomchristie commented 8 years ago

Gotcha, good stuff! πŸ‘

We should push for screenshots a little more on issues like these - immensely more descriptive medium for UI issues! πŸ˜„

carltongibson commented 8 years ago

We should push for screenshots a little more on issues like these...

Indeed. +11111

Gotcha, good stuff! πŸ‘

Approaching Inbox Zero on Django Filter. (After β‰ˆ2 years.)

carltongibson commented 8 years ago

@scottrus Does the example passing a list to the filter constructor in the docs do what you're after?

Something like:

    category = django_filters.CharFilter(lookup_expr=['exact', 'contains'])
scottrus commented 8 years ago

Hey, thanks for the feedback. I totally would have pushed a screenshot except I cheated and modified the doc example rather than building one. 😊

Yes, the issue is the 'category': ['exact', 'contains'] results in duplicate fields and I totally understand why. It's just really not an ideal user experience. I thought it was DRF that was doing this interpretation, not django-filter, so apologies the issue report is in the wrong place.

Let me see where I end up by passing the list to the filter constructor. I hadn't found that example or considered that as a solution yet.

scottrus commented 8 years ago

Yup, the syntax proposed does indeed put a select widget for either 'exact' or 'contains' into the form instead of having multiple char fields by the same name. However this seems to also change the filter expression so that ?category__contains=blah is no longer valid. Instead I see ?category_0=blah&category_1=contains in the resulting URL.

scottrus commented 8 years ago

I'm not yet sure I understand what the intended differences is between specifying the filter details as a meta option with field = {} instead of as a filter constructor with lookup_expr. I would think that either syntax should create the same form widgets and allow for the same filter expressions in the URL.

Instead there seems to be some intentional subtle differences between using the Meta class and using the filter constructor to define lookup_expr. I'll try to dig into the code later today after work.

carltongibson commented 8 years ago

Hey @scottrus.

Yup, the syntax proposed does indeed ...

Great. πŸ™‚

...the intended differences...

This is just an accident of history.

There should be one-- and preferably only one --obvious way to do it.

Alas, this isn't quite the case.

Declaring the filters explicitly is the preferred, canonical method. The fields meta dict is a convenience. It has limitations. As soon as they are hit, stop using it. In another world, I'd not again add the fields shortcut but it was a good PR back when there was a long backlog of very old issues. Sometimes these things are only clear with hindsight. (Plus the fields shortcut is kind of cool, except when it leads to confusions...)

scottrus commented 8 years ago

Sounds good @carltongibson. I agree, the fields short cut is awesome when it works. 😊 I'm good declaring the filters as constructors but still expecting field__modifier=value style parameters to work in URLs. That's what I'll look into the code for tonight.

carltongibson commented 8 years ago

Oooh. Ouch.

That comes from Django's multi-widget... of course you can change it but, it's a lot of layers deep.

rpkilby commented 8 years ago

Approaching Inbox Zero on Django Filter. (After β‰ˆ2 years.)

I'm always happy to create new PRs to keep you busy πŸ˜„

@scottrus while https://github.com/carltongibson/django-filter/pull/437 churns, you might be able to hack out some better labels that are compatible with the Meta.fields option. This should be possible by combining humanized versions of the field name and the lookup_expr. something like:

from django.forms.utils import pretty_name
import django_filters as filters

class BaseFilterSet(filters.FilterSet):
    def __init__(self, *args, **kwargs):
        super(BaseFilterSet, self).__init__(*args, **kwargs)
        for f in self.filters.values():
            f.label = pretty_name(' '.join(self.name, self.lookup_expr))