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

Handle errors when editing queries in admin editor #244

Merged

Conversation

markkelnar
Copy link
Contributor

When editing saved persisted queries in WP admin editor, do not allow invalid queries to be published. But allow invalid queries to be saved as drafts.

closes #243

@markkelnar markkelnar changed the title Bug/fix admin editor error message Handle errors when editing queries in admin editor Aug 8, 2023
Copy link
Collaborator

@jasonbahl jasonbahl left a comment

Choose a reason for hiding this comment

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

@markkelnar it seems like maybe the hook ordering still needs some work.

I'm able to add an invalid document and click "Publish" and I see the following:

CleanShot 2023-08-11 at 09 44 12

The post is published and WordPress responds with a confirmation that it was published, but we also respond with an error about it being invalid, and the status is then set back to draft.

The expectation is that WordPress should not publish this post at all if it doesn't meet the validation criteria to be published.

My hunch is that some logic needs to be applied in some different hooks that execute earlier in the post save lifecylce.

@markkelnar
Copy link
Contributor Author

I am not able to recreate an invalid query string being published. Do you have steps to recreate?

I do see the 'Post Published' message. That is default WP behavior. I can track that down to see if I can clobber that message.

@jasonbahl
Copy link
Collaborator

@markkelnar steps to reproduce:

  • Create a new "graphql_document" with an invalid query in the post_content field (i.e. "invalid query")
  • Click Publish
  • See the "Post Published" message
  • See that the published date has been updated (which happens after a post is published)

CleanShot 2023-08-14 at 08 27 31

What appears to be happening is that the validation is happening too late.

Post is published, we catch some validation then reset to draft.

What I would expect is the post to not be published in the first place. If I'm not mistaken, ACF has some functionality that adds validation for things like required fields, preventing posts from being published if certain fields aren't filled in. Perhaps we can take some ideas from how they're handling things?

@jasonbahl
Copy link
Collaborator

When I use ACF to add a required field to the graphql_document post type, and click "publish" without filling in the field, I get an error and I do not see the "Post Pubished" message nor is the "published" date updated:

CleanShot 2023-08-14 at 08 40 50

@markkelnar
Copy link
Contributor Author

Thanks @jasonbahl for the example test.

Acf does that validation using WP admin-ajax request and on error, displays the error you see and prevents the form submission for the edited post. That might be more work than we want to do right now.

I am going to test redirecting to the edit page for the post on error before the save_post action is invoked.

Copy link
Collaborator

@jasonbahl jasonbahl left a comment

Choose a reason for hiding this comment

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

I left a suggested change as the User Experience of clicking publish on an invalid document still appeared to publish the post even though it wasn't.


I also caught another bug.

When trashing a post of the graphql_document post type, if the document (post_content) is invalid, the error shows on the list posts screen after trashing:

I believe the validation is reaching further than it should, perhaps? I don't think we need to validate the document (post_content) when trashing/deleting a document.

CleanShot 2023-08-15 at 09 29 52

src/Admin/Editor.php Show resolved Hide resolved
@markkelnar
Copy link
Contributor Author

And similar behavior with the error message from validating the content when restoring the document from the trash. Adding tests for that too.

src/Admin/Editor.php Outdated Show resolved Hide resolved
src/Admin/Editor.php Show resolved Hide resolved
src/Admin/Editor.php Show resolved Hide resolved
@markkelnar markkelnar merged commit b13e132 into wp-graphql:main Aug 18, 2023
8 checks passed
@markkelnar markkelnar deleted the bug/fix-admin-editor-error-message branch August 18, 2023 13:05
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.

Validation for publishing a GraphQL Document
2 participants