-
Notifications
You must be signed in to change notification settings - Fork 57
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
Only retrieve the pages that are registered in the settings #226
base: main
Are you sure you want to change the base?
Conversation
Hey @jffr, thank you for this. |
@zerolab Thank you for the feedback! I have fixed the failing tests and introduced two new tests for the app setting:
I think we're missing one last test, which is a query where we get more pages from different apps. But in order to write that test we have to include a new demo app within the example project. Do you think it is worth it or are the test cases explained above good enough? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @jffr,
thank you for this. left a few notes/suggestions
# Retrieve only the pages that are registered in the settings | ||
content_types = ContentType.objects.filter( | ||
app_label__in=grapple_settings.APPS | ||
) | ||
pages = pages.filter(content_type__in=content_types) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this could be simplified as:
pages = pages.filter(content_type__app_label__in=grapple_settings.APPS)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
additionally, we still support the old dict-style declaration. https://cs.github.com/GrappleGQL/wagtail-grapple/blob/9b8a49c05ddc74673e980e6a359c9f642aae8877/grapple/actions.py?q=grapple_settings.APPS#L55-L63
Perhaps use registry.apps
- https://cs.github.com/GrappleGQL/wagtail-grapple/blob/9b8a49c05ddc74673e980e6a359c9f642aae8877/grapple/registry.py#L62 for this
|
||
pages_data = executed["data"]["pages"] | ||
self.assertEquals(pages_data[0]["contentType"], "home.HomePage") | ||
self.assertEquals(pages_data[1]["contentType"], "home.BlogPage") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest adding news_post = NewsPageFactory(parent=self.home)
before the execute and add a check to ensure the news page doesn't appear in the results
@@ -0,0 +1,22 @@ | |||
# Generated by Django 3.2.13 on 2022-04-24 07:18 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't see the changes that triggered this migration 🤔
@jffr do you still have any inclination to work on this PR? |
A small fix for removing pages that are not registered in the GRAPPLE settings.
Having a project with two apps (for example home and blog). You'll get an error when we only expose the 'blog' app. Which returns the following output:
Where we have the following settings:
And the query: