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

Preview: get "parent" post ID for non-hierarchical posts when using asPreview #2876

Open
renatonascalves opened this issue Aug 1, 2023 · 10 comments
Labels
has: workaround A temporary workaround for this issue has been provided. Status: 🚀 Actionable Issues that have been curated, have enough info to take action, and are ready to be worked on Type: Enhancement New feature or request

Comments

@renatonascalves
Copy link
Collaborator

renatonascalves commented Aug 1, 2023

What problem does this address?

Currently, if you try to preview a non-hierarchical post, the Previewable interface does not return the post ID of the "parent" post.

Screenshot 2023-08-01 at 11 22 50

What is your proposed solution?

There are a few options here:

  • We could add a new field for the Previewable interface, exposing the "parent" post ID;
  • We could add a new custom interface for non-hierarchical posts exposing the "parent" post ID;
  • We could let API consumers to figure this out by themselves.

What alternatives have you considered?

Currently, API consumers need to circumvent this by getting the id in the params and swap for the one in the response in their apps.

But it is my understanding tools are not doing that and exposing a broken experience. (See the additional context below).

Additional Context

This is particularly helpful in a toolbar. See this Github issue, wpengine/faustjs#1515, for an example where this problem reflects.

@jasonbahl
Copy link
Collaborator

jasonbahl commented Aug 1, 2023

@renatonascalves you have the id in the input already, eh?


edit: ah, you call this out already 🤦

@renatonascalves
Copy link
Collaborator Author

@jasonbahl I do! And keeping as is and leaving API consumers to handle it in their apps is a valid approach as well.

I just think the current approach is not obvious. And the example shared shows people has expectations it will return the right databaseId when it doesn't.

@jasonbahl
Copy link
Collaborator

@renatonascalves I think perhaps the solution is maybe clients changing from querying asPreview: true to instead querying the node they want, and querying the connected preview node:

CleanShot 2023-08-02 at 14 00 11

This way you pass the ID of something, you get that something, then you specify you want the connected preview of it. It's a bit more explicit that the client wants 2 nodes, the published node and the preview of the node.

I believe the asPreview is working as intended. The asPreview argument allows the consumer to explicitly ask for the preview node, and that node is returned.

Perhaps in the same way we have a preview connection, we could introduce an inverse of that. Something like previewOf connection.

That way we could query the node that a preview is the preview of.

For example:

{
  post(id: 3192, idType: DATABASE_ID) {
    ...PostTemplateData
    preview { ## The preview node (contains unpublished data )
      node {
        ...PostTemplateData
        previewOf { ## the origin node the preview is a preview of
          node {
            ...PostTemplateData
          }
        }
      }
    }
  }
}

fragment PostTemplateData on Post {
  id
  databaseId
  title
  content
  isPreview
}

CleanShot 2023-08-02 at 14 18 53

Here's the code to register the connection if you want to play with it and see what you think.

add_action( 'graphql_register_types', function() {

	$post_types = WPGraphQL::get_allowed_post_types( 'objects' );

	foreach ( $post_types as $post_type ) {

		// only define the connection for public/publicly_queryable post types
		if ( true !== $post_type->publicly_queryable && true !== $post_type->public ) {
			continue;
		}

		register_graphql_connection( [
			'fromType' => ucfirst( $post_type->graphql_single_name ),
			'toType' => ucfirst( $post_type->graphql_single_name ),
			'description' => __( 'The published node the revision is associated with', 'wp-graphql' ),
			'fromFieldName' => 'previewOf',
			'oneToOne' => true,
			'resolve' => function( \WPGraphQL\Model\Post $preview, $args, $context, $info ) {

				// if the current node is not a preview, return null.
				// this connection is intended to return the published node
				// of the preview
				if ( ! $preview->isPreview ) {
					return null;
				}

				$resolver = new \WPGraphQL\Data\Connection\PostObjectConnectionResolver( $preview, $args, $context, $info );

				// resolve the connection using the preview's parentDatabaseId
				$resolver->set_query_arg( 'p', $preview->parentDatabaseId );

				// return the connection as a one to one connection
				return $resolver->one_to_one()->get_connection();

			}
		] );

	}

} );

Let me know what you think and we can include a connection like this in the core WPGraphQL Schema!

@jasonbahl
Copy link
Collaborator

Here is the same previewOf connection simplified with an asPreview query:

CleanShot 2023-08-02 at 14 26 50

@jasonbahl jasonbahl added the Type: Enhancement New feature or request label Aug 2, 2023
@renatonascalves
Copy link
Collaborator Author

I'll play around with it. But a quick look tells me that a previewOf connection seems ideal.

I do wonder however what can be done to avoid this mistake from consumers. It was not obvious to me at first, and others apparently, that using asPreview returns a different object.

I believe the asPreview is working as intended. The asPreview argument allows the consumer to explicitly ask for the preview node, and that node is returned.

And I agree! It is not a bug per se. The issue is the change in the response is not obvious.

@jasonbahl
Copy link
Collaborator

jasonbahl commented Aug 2, 2023

I do wonder however what can be done to avoid this mistake from consumers. It was not obvious to me at first, and others apparently, that using asPreview returns a different object.

Ya, perhaps more clarity on the description of the asPreview argument.

Right now it says: Whether to return the node as a preview instance

Whether to return the Preview Node instead of the Published Node. When the ID of a Node is provided along with asPreview being set to true, the preview node with un-published changes will be returned instead of the published node. If no preview node exists or the requestor doesn't have proper capabilities to preview, no node will be returned.

@renatonascalves
Copy link
Collaborator Author

renatonascalves commented Aug 27, 2023

@jasonbahl Tks for improving the description of the argument. 💪🏾

I liked the suggestions here. And it'd be great to have them in the core plugin.

@justlevine justlevine added Status: Discussion Requires a discussion to proceed has: workaround A temporary workaround for this issue has been provided. labels Dec 11, 2023
@jeremyProwseYS
Copy link

jeremyProwseYS commented Dec 13, 2023

Just chiming in on this off the back of a related ticket #3001 (any context for my below thoughts can be found there) I raised the other day. In short here are the thoughts I had regarding the current behaviour of the parent key when viewing content in preview mode:

I've managed to find a work-around using the revisionOf prop to traverse the true parent structure for the post. However this is just that, a "workaround" and I was wondering if there were any plans to reintroduce expected behaviour for the parent prop when preview is concerned. Surely the previewOf prop suggested by @jasonbahl in this post would make more sense to house the info currently returned in the parent prop? I'm just not sure changing the return value / behaviour of a prop based on a variable, such as preview, makes logical sense.

revisionOf could be used in place of adding a new prop (previewOf), as it seems to house all the info you would expect.

@renatonascalves
Copy link
Collaborator Author

I'm just not sure changing the return value / behaviour of a prop based on a variable, such as preview, makes logical sense.

So, in a nutshell, WordPress saves revision post types in different post ids. So when a user uses the asPreview, aware of it or not, the user is requesting for a different post, not the "main" post, but the revision post.

So having access to the "parent" post via previewOf or revisionOf makes sense to me. I think it comes down to intention. The intention of using asPreview is to grab the revision post, not the main post. So it doesn't make sense to return the main post when trying to preview it.

@justlevine
Copy link
Collaborator

justlevine commented Dec 15, 2023

My understanding of the underlying source of contention is that we don't have a PostRevision type, we have a Post type, which when using the asPreview argument coerces the revision into a Post.

With that in mind, the discrepancy becomes a lot more clear:

image

The parentDatabaseId is intended to return the parent of a Post in a HierarchicalNode tree, not of a revision, while the revisionOf is meant to contain the Post that the revision will be used for.

As such, IMO the bug is that when previewed, HierarchicalNode.parent (in the screenshot: previewPage.parentDatabaseId) refers to the Post this is a revision of when it should return the same value as the livePage, and revision traversal should be limited to the Previewable and NodeWithRevisions interfaces, instead of coopting the semantics.

(I do see the benefit of adding NodeWithRevisions.revisionOfDatabaseId to add with that traversal)

@jasonbahl jasonbahl added Status: 🚀 Actionable Issues that have been curated, have enough info to take action, and are ready to be worked on and removed Status: Discussion Requires a discussion to proceed labels Dec 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has: workaround A temporary workaround for this issue has been provided. Status: 🚀 Actionable Issues that have been curated, have enough info to take action, and are ready to be worked on Type: Enhancement New feature or request
Projects
Status: 💬 In Discussion
Development

No branches or pull requests

4 participants