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.

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

Allow HTML to render when no filter_class is defined. #3560

Merged
merged 2 commits into from Nov 4, 2015

Conversation

ericholscher
Copy link
Contributor

@ericholscher ericholscher commented Oct 28, 2015

Previously it required a filter_class,
or else it would error when calling cls().
This now sets the filter context to None
if one does not exist.

This fixes #3559

Previously it required a filter_class,
or else it would error when calling `cls()`.
This now sets the `filter` context to `None`
if one does not exist.

Fixes encode#3559
@ericholscher
Copy link
Contributor Author

@ericholscher ericholscher commented Oct 28, 2015

Not sure what the best way to test this is, happy to add tests if someone points the way.

@jpadilla jpadilla added the Bug label Oct 28, 2015
@jpadilla jpadilla added this to the 3.3.1 Release milestone Oct 28, 2015
@@ -118,7 +118,10 @@ def filter_queryset(self, request, queryset, view):

def to_html(self, request, queryset, view):
cls = self.get_filter_class(view, queryset)
filter_instance = cls(request.query_params, queryset=queryset)
if cls:
Copy link
Member

@jpadilla jpadilla Oct 28, 2015

Choose a reason for hiding this comment

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

As a general note/nitpick can we go ahead and also make this look like the filter_query() method on top, basically just renaming cls for filter_class.

@jpadilla
Copy link
Member

@jpadilla jpadilla commented Oct 28, 2015

@ericholscher thanks for putting this together, looks good to me. We were already doing a similar check on the filter_queryset() method on top of that one. HTML rendering for filters has no tests that I know of yet, so I wouldn't really worry about it.

@jpadilla
Copy link
Member

@jpadilla jpadilla commented Oct 28, 2015

@ericholscher also I'm wondering what does that rendered HTML looks like with no filter since both filter templates use filter.form from context.

@tomchristie
Copy link
Member

@tomchristie tomchristie commented Nov 2, 2015

happy to add tests if someone points the way.

An example, along with screenshots demonstrating the behavior in the None case would prob be first point of call. We could figure out a sensible test case following on from that.

@tomchristie
Copy link
Member

@tomchristie tomchristie commented Nov 4, 2015

Gonna accept this for now, given bug fix nature of the 3.3.1 release.
Thanks @ericholscher!

tomchristie added a commit that referenced this issue Nov 4, 2015
Allow HTML to render when no filter_class is defined.
@tomchristie tomchristie merged commit 95f92e9 into encode:master Nov 4, 2015
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants