Skip to content

Document how to use rest_framework.filters.SearchFilter #476

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

Merged
merged 4 commits into from
Sep 17, 2018

Conversation

n2ygk
Copy link
Contributor

@n2ygk n2ygk commented Sep 13, 2018

Fixes #432

Description of the Change

Documents how to use rest_framework.filters.SearchFilter along with the other filter backends.

Includes updates to the example app to demonstrate usage and test cases.

CHANGELOG.md remains unchanged because this is included in: "Add optional jsonapi-style filter backends"

I also documented some filterset_fields to complement the search_fields in the usage document showing how to configure filter backends.

Checklist

  • PR only contains one change (considered splitting up PR)
  • unit-test added
  • documentation updated
  • changelog entry added to CHANGELOG.md
  • author name in AUTHORS

Includes updates to the example app and tests.
@n2ygk n2ygk requested a review from sliverc September 13, 2018 18:33
"""
test for `filter[search]=keyword1...` (keyword1 [AND keyword2...])
"""
for keywords in ("research", "chemistry", "nonesuch",
Copy link
Member

Choose a reason for hiding this comment

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

This test is a bit difficult to comprehend. It would be much easier to write it with pytest parametrize.

I ask myself though do we really need a test for SearchFilter? It is DRF functionality and as such it is already tested in DRF itself and we just do same work again. Hence I think we could remove this test.

@sliverc sliverc added this to the 2.6.0 milestone Sep 14, 2018
@n2ygk
Copy link
Contributor Author

n2ygk commented Sep 14, 2018 via email

@sliverc
Copy link
Member

sliverc commented Sep 14, 2018

@n2ygk if this is the case then the test should be simplified where only one keyword is searched for and a simple assert result.json() == expected will do. This will show how it works and is readable by someone who wants to see how search filter is used.

@n2ygk
Copy link
Contributor Author

n2ygk commented Sep 14, 2018

@sliverc Sorry, I didn't notice your followup comment until just now... Please take a look at the test case again before we kill it:-(. I am also happy to add the result.json() assertion with static test values but still think it's valuable to demonstrate the multi-keyword searching across ORM-related fields.

…fragility of this test. Any future changes to unrelated parts will break it.
@sliverc
Copy link
Member

sliverc commented Sep 17, 2018

OK. Let's leave it this way and merge. Thanks.

@sliverc sliverc merged commit 5d98c0b into django-json-api:master Sep 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants