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

fix: previews for unpublished pages #277

Merged
merged 1 commit into from
Jan 13, 2023

Conversation

dopry
Copy link
Collaborator

@dopry dopry commented Nov 29, 2022

applies to #275

I couldn't find any tests for the token calls and don't have a clue where to start generating them.

@dopry
Copy link
Collaborator Author

dopry commented Nov 29, 2022

I am testing with the following graphql query, we'll need to setup some sort of similar test scenario. note in both scenarios my frontend preview code has to copy the id or page_type out of the token to pass to the graphql query. I feel like maybe I should go deeper and just have the token take precendence over everything else if it is present.

{
  previewUnpublishedSavedDraft: page(
    id: 367
    token: "id=367:1p05oi:jDIi4YDGXs-6YgoOVz05HgyWrEwnOEoP3PTsEEF8RD4"
  ) {
    title
  }
  
  previewUnsaved: page(
    contentType: "typenetwork.TnPage"
    token: "parent_id=367;page_type=typenetwork.TnPage:1p05wC:jJsi2R0bD_IOHDMbZ4kKc34lH2myylKY_vGQ7_T_Hlk"
  ) {
    title
  }
  
  page(id: 367) {
    title
  }
}

@dopry dopry force-pushed the fix/275-unpublished-previews branch from ba2ccf3 to b67f98a Compare November 29, 2022 19:49
@dopry
Copy link
Collaborator Author

dopry commented Nov 29, 2022

I force pushed an update where token takes priority over all other args is present and the params required for fulfillment of a token query are extracted from the token.

My update test graphql looks more like

{
  "data": {
    "previewUnpublishedSavedDraft": {
      "title": "Not Published"
    },
    "previewUnsaved": {
      "title": "Title"
    },
    "page": null
  }
}

I still need help figuring out how to set the test scenarios for 2 token types (saved and unpublished, unsaved and unpublished), all the published scenarios should be covered by existing tests. We may also want to add tests to ensure unpublished content isn't accessible without the token. I'm not sure that those tests should necessarily hold up this PR since we don't already have coverage on those feature and this is a bug that could be impacting production sites.

@dopry dopry force-pushed the fix/275-unpublished-previews branch from b67f98a to 30838a1 Compare November 29, 2022 20:23
Copy link
Member

@zerolab zerolab left a comment

Choose a reason for hiding this comment

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

@dopry do you still need help with generating a token for new pages?

https://github.com/torchbox/wagtail-headless-preview/blob/main/src/wagtail_headless_preview/models.py#L75-L80

unpublished pages are no more special than others. You can just do token = PageModelWithPreviewMixin.create_page_preview().token

grapple/types/pages.py Outdated Show resolved Hide resolved
grapple/types/pages.py Outdated Show resolved Hide resolved
"""
is there a particular version of wagtail that requires checking that
get_page_from_preview_token exists? is there a version of wagtail
at which we can drop this conditional safely?
Copy link
Member

Choose a reason for hiding this comment

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

get_page_from_preview_token comes for wagtail-headless-preview and is Wagtail-versiona agnostic

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So this is really just a check to see if headless preview is installed and enabled?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

would we be better served with an isHeadlessPreviewEnabled() function?

return cls.get_page_from_preview_token(token)

"""
we get parent_id and page_type when previewing a page that has not been
Copy link
Member

Choose a reason for hiding this comment

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

parent_id is added for convenience. if I recall correctly we needed to vary things a bit when generating the token. When you have an ID that gives enough to go on.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we could eliminate the if id: code path here if the page_type was always sent. It might not be as 'fast', but it would reduce the amount of code we need to maintain.

@dopry
Copy link
Collaborator Author

dopry commented Jan 2, 2023

I'll try to get another pass on this by EOM.

@dopry dopry force-pushed the fix/275-unpublished-previews branch 4 times, most recently from 9f7d06e to 879c10c Compare January 6, 2023 20:07
@dopry
Copy link
Collaborator Author

dopry commented Jan 6, 2023

@zerolab I got the tests setup. I'm not happy with how I'm mocking the unsaved page as per my haiku, but it seems to work. I feel like wagtail-headless-preview is actually a better place for much of this code. I'll try to make a PR against it in the next few weeks to improve the DX here a little and for anyone else building on it.

@dopry dopry force-pushed the fix/275-unpublished-previews branch 4 times, most recently from df2c199 to 059910f Compare January 6, 2023 20:32
@dopry dopry force-pushed the fix/275-unpublished-previews branch from 059910f to 6cf227c Compare January 11, 2023 23:15
@zerolab
Copy link
Member

zerolab commented Jan 11, 2023

uh-oh, I missed this one for 0.19. Will review on Friday and make 0.19.1 release 🙈

@dopry
Copy link
Collaborator Author

dopry commented Jan 12, 2023

I was just about to ask. :) I'd love to see previews fixed. At some point in the future I'll try to take a pass on wagtail-headless-preview, but I'd rather those concerns not block this now.

@zerolab zerolab merged commit 8de5a12 into torchbox:main Jan 13, 2023
@zerolab
Copy link
Member

zerolab commented Jan 13, 2023

@dopry
Copy link
Collaborator Author

dopry commented Jan 13, 2023

🎉 hooray 🎉

@dopry dopry deleted the fix/275-unpublished-previews branch January 13, 2023 19:10
@zerolab
Copy link
Member

zerolab commented Jan 14, 2023

Thank you for your PRs, much appreciated!

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