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

Rework related filtering #199

Merged
merged 7 commits into from
Jul 15, 2018
Merged

Conversation

rpkilby
Copy link
Collaborator

@rpkilby rpkilby commented Oct 26, 2017

Alternative implementation to #197. This also needs further testing, but in general I'm happier with it.

The major difference is how data is passed between filtersets. In the current implementation (and #197), related filtersets are provided with a transformed data dictionary where their relationship name has been stripped from the data keys. The downside to #197 is that it's not possible to render related filterset forms.

In contrast, this approach passes the same incoming data to its related filtersets, untransformed. As a result, users should have a way to render related filterset forms with no conflict.

Overall, here is what this PR would enable:

  • Correct behavior when filtering across to-many relationships (ref).
  • Related Filters can now enforce queryset/choices restrictions. (eg, only display choices for related articles that have been published or are authored by the request user).
  • No longer necessary to customize method filters (previously requires munging the field_name).
  • Filtering against related annotations. Previously this was not possible, as the related filters were applied to the root queryset, and not the nested/related queryset.

Changes:

  • Add FilterSet.relationship, which allows the filterset to parse it's params across relationships. This is compatible with form_prefix, although that's not usually relevant with API filtersets. It turns out, form prefixes aren't directly compatible, as the subsetting happens before parent initialization/filter deepcopy'ing, and prefix/relationship detection only occurs after. This could be supported, but it doesn't seem worth the trouble. i.e., we don't need prefixes to support rendering related filtersets.
  • Add the FilterSet.get_related_filtersets() method and it's corresponding FilterSet.related_filtersets attribute. The latter is simply the cached result of the former. This method constructs a dict of related filtersets, keyed by their relationship name.
  • Add the FilterSet.filter_related_filtersets() method, which is a hook for defining the behavior of how related filtersets are filtered.
  • Add templates that display the first level of related filtersets.

TODO:

  • Test FilterSet.get_related_filtersets()
  • Test FilterSet.filter_related_filtersets() Implicit through existing tests
  • Test templates (test post data)
  • Test related filterset errors
  • Ensure there are no tests marked as expected failures

@rpkilby rpkilby added this to the v1.0.0 milestone Oct 26, 2017
@codecov-io
Copy link

codecov-io commented Oct 26, 2017

Codecov Report

Merging #199 into master will increase coverage by 0.42%.
The diff coverage is 100%.

Impacted file tree graph

@rpkilby rpkilby force-pushed the rework-related-filtering-v2 branch 2 times, most recently from f651e3d to 2a1bb6b Compare October 26, 2017 13:30
@rpkilby rpkilby force-pushed the rework-related-filtering-v2 branch 3 times, most recently from 9471a95 to a40e3d4 Compare December 30, 2017 17:54
@rpkilby rpkilby force-pushed the rework-related-filtering-v2 branch 9 times, most recently from fbbad5b to 8741f69 Compare January 15, 2018 13:48
@rpkilby rpkilby force-pushed the rework-related-filtering-v2 branch 2 times, most recently from f57073e to 6fc6ee1 Compare July 14, 2018 05:18
@rpkilby rpkilby changed the title [wip] Rework related filtering, v2 Rework related filtering Jul 14, 2018
@rpkilby
Copy link
Collaborator Author

rpkilby commented Jul 14, 2018

Alright, I'm largely satisfied with this PR. I need to carefully review the code and ensure the tests adequately cover the new behavior, but otherwise, this PR accomplishes what I want it to.

In addition to the general improvements to filtering across relationships, it's worth noting that this PR enables annotation support for related filtersets, as well as form rendering for first-level relationships.

@rpkilby rpkilby force-pushed the rework-related-filtering-v2 branch 9 times, most recently from 976b16d to f7a4884 Compare July 15, 2018 03:35
@rpkilby rpkilby force-pushed the rework-related-filtering-v2 branch 12 times, most recently from c677ddb to 87cf88f Compare July 15, 2018 19:18
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