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

feat: add search operator and fields arguments #321

Closed
wants to merge 1 commit into from

Conversation

dopry
Copy link
Collaborator

@dopry dopry commented Mar 8, 2023

Add parameters for search_operator and search_fields when search is enabled on a query.

@dopry dopry force-pushed the feat/search_operator-and-fields branch 3 times, most recently from c3397c7 to ca511f9 Compare March 10, 2023 16:45
@dopry dopry force-pushed the feat/search_operator-and-fields branch from ca511f9 to 33e8135 Compare May 2, 2023 14:21
@dopry dopry force-pushed the feat/search_operator-and-fields branch 2 times, most recently from 8fabb87 to 3bf9d79 Compare May 12, 2023 03:10
@dopry
Copy link
Collaborator Author

dopry commented May 12, 2023

@zerolab what documentation are you looking for here? I added some inline the graphql type definitions so it shows up in graphiql.

@dopry dopry force-pushed the feat/search_operator-and-fields branch from 3bf9d79 to 6c79caf Compare May 12, 2023 03:16
@dopry dopry changed the title WIP feat: add search operator and fields arguments feat: add search operator and fields arguments May 12, 2023
@zerolab
Copy link
Member

zerolab commented May 12, 2023

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

@dopry
Copy link
Collaborator Author

dopry commented May 12, 2023

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...

@zerolab
Copy link
Member

zerolab commented May 12, 2023

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
Copy link
Collaborator Author

dopry commented May 12, 2023

Alright I'll work on tests next week. :)

@dopry dopry force-pushed the feat/search_operator-and-fields branch from 6c79caf to 54ef10e Compare May 12, 2023 17:06
@zerolab
Copy link
Member

zerolab commented Sep 4, 2023

@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

@dopry
Copy link
Collaborator Author

dopry commented Sep 15, 2023

@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.

@dopry
Copy link
Collaborator Author

dopry commented Oct 5, 2023

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.

@dopry dopry force-pushed the feat/search_operator-and-fields branch 7 times, most recently from f816e48 to 200b4b5 Compare September 1, 2024 18:02
@dopry
Copy link
Collaborator Author

dopry commented Sep 1, 2024

@zerolab I added tests. Let me know if you want to see any additional changes.

@dopry dopry force-pushed the feat/search_operator-and-fields branch from 200b4b5 to 5735dc9 Compare September 1, 2024 20:11
@dopry
Copy link
Collaborator Author

dopry commented Sep 1, 2024

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.

@dopry dopry force-pushed the feat/search_operator-and-fields branch from 5735dc9 to ad0ab8a Compare September 1, 2024 21:15
@dopry dopry force-pushed the feat/search_operator-and-fields branch 3 times, most recently from 2274926 to 6c837ab Compare September 18, 2024 14:17
@dopry dopry force-pushed the feat/search_operator-and-fields branch from 6c837ab to 0d28ccd Compare September 20, 2024 13:46
allow for more specific search queries by specifying the search_operator
and search_fields.
@dopry dopry force-pushed the feat/search_operator-and-fields branch from 0d28ccd to fb63350 Compare September 20, 2024 13:48
@dopry
Copy link
Collaborator Author

dopry commented Sep 20, 2024

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.

  1. merge as is and document the limitations of search fields.
  2. implement support for an alternate root model, then merge this.
  3. 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?

@zerolab
Copy link
Member

zerolab commented Sep 20, 2024

Thanks @dopry. Marked it as a draft so I don't foget

@zerolab
Copy link
Member

zerolab commented Sep 20, 2024

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

@dopry
Copy link
Collaborator Author

dopry commented Sep 20, 2024

superceded by #406 and #407

@dopry dopry closed this Sep 20, 2024
@dopry dopry deleted the feat/search_operator-and-fields branch September 20, 2024 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants