torchbox
feat: add search operator and fields arguments
Add parameters for search_operator and search_fields when search is enabled on a query.
@zerolab what documentation are you looking for here? I added some inline the graphql type definitions so it shows up in graphiql.
I was thinking developer docs (e.g. https://wagtail-grapple.readthedocs.io/en/latest/general-usage/model-types.html) so it is not buried in code
Are we creating documentation for consumers of of grapple or for grapple devs? We do not have any docs on the shared GraphQL Structures that this impacts and none of the enable_* options to the QuerySetList constructor or PaginatedQuerySet are exposed via the Decorators. I would propose the we create a new issue to document the structures... I feel like we need to document structures and how they are related to the decorators and GraphqlTypes... My understanding is that the structures are effectively base types for all of the generated GraphQL Types... so documenting those should probably go into GraphQL types. The new section of docs feels like a fairly large task on it's own... How do you feel about deferring this documentation effort to a separate task.... I found the current docs pretty intimidating as a new user and quite confusing... There is a lot of assumption that I would know what a field was and what the plural, singular, and paginated modified mean...
We want docs for both consumers and devs. For example, as an implementer/user I would like to know that I can change the search operator
Now, my previous comment was me cheekily hoping to get that started here 🙈 Happy for a separate task. We do need to rehaul the documentation and add a few other things (https://github.com/torchbox/wagtail-grapple/issues?q=is%3Aissue+is%3Aopen+label%3Adocumentation) so happy for that to be handled as a follow up
@dopry are you still keen to pursue this? We need some test, then I can do a full review/local testing pass and get it in
@zerolab I am, but I'm just getting back to work after summer break. It'll take me a few weeks at least to catch up on client work before I can take on any OSS work.
still on my radar, I'm wrapping up some work on django-oauth-toolkit that is a higher priority for me, so maybe a week or two out still.
@zerolab I added tests. Let me know if you want to see any additional changes.
Hrm. my searchFields tests don't seem to be working quite the way I'd like. I tried setting up a body field to specify alongside title, but it didn't work. At first I didn't realize why. In my customized grapple implementation, I use my own custom page type as the root for all queries rather the wagtail.model.Page that has all my common search_fields... I'm realizing that doesn't work with grapple atm as you can't specify an alternate root model. Where grapple uses a mixin with qs=WagtailPageObjects.all(), I use standalone function that looks like
def _resolve_search_pages(
info,
content_types=[],
page=1,
per_page=10,
root=TnPage.objects.live().specific(),
categories=[],
tags=[],
ancestor=None,
parent=None,
in_menu=None,
**kwargs
):
"""
root allows an alternative queryset to be passed in to allow
for inheriting types to filter the queryset first.
"""
I feel like it would be nice to pass a 'root' into PagesQuery to overide the base model for pages, but not sure that is in the scope of this... In the mean time. I've stripped down the test to just verify with title alone.
Based on my last comment I think we should hold off on merging this until I update the docs to reflect the limitations of search fields. Given the limitation to the title field, I'm not sure how valuable the search_fields feature would be at this juncture.
I think there are a few routes forward for this PR.
- merge as is and document the limitations of search fields.
- implement support for an alternate root model, then merge this.
- split out search_operator as it's own PR. I think it has value in isolation. Then I can continue on search_fields in isolation.
The first option could be confusing to users and might be more of a support headache than it's worth. The second option I feel like would hold up the PR and prevent us from getting the search_operator feature. I think the third option as best.
@zerolab thoughts?
Did you merge a PR that allows specifying an alternative root mode?
I have not. The recent merges were the fix fo custom page interfaces and your in_menu PR