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

REST API: Post locking endpoint #37789

Open
wants to merge 4 commits into
base: trunk
Choose a base branch
from
Open

Conversation

spacedmonkey
Copy link
Member

Description

In this Gutenberg PR, we're trying to introduce post locking #4217. However this uses admin ajax and other legacy javascript. Post locking should use the REST API. This REST API endpoint was originally requested by @youknowriad in #44862.

How it works.

This PR dynamically added new endpoints for all post types that support the REST API. This work similar to how revisions and autosaves works in core. The structure of the endpoint is as follows.

GET wp/v2/{base}/{id}/lock - Get lock data.
POST wp/v2/{base}/{id}/lock - Update / Create lock. No data required. User id defaults to currently logged in user.
DELETE wp/v2/{base}/{id}/lock - Delete lock. User can not delete other locks.

Example wp/v2/posts/999/lock

The structure of the response is pretty small. With date, id and author being returned. If you wish to get more author information, you can simply embed the request, like so

GET wp/v2/posts/999/lock?_embed=author.

This PR is based on the Web stories REST API endpoint found here. This endpoint designed to be CRUD endpoint, the logic of handling hand off between users, if for client / javascript to hand.

How has this been tested?

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).
  • I've updated related schemas if appropriate. <!-- Reference: https://github.com/WordPress/gutenberg/tree/trunk/schemas ->

@youknowriad
Copy link
Contributor

This would be a good code quality improvements for the block editor. But I also wonder about the next steps (collaborative editing) and whether this endpoint could be impacted. It seems locking won't mean the same thing.

@spacedmonkey
Copy link
Member Author

This would be a good code quality improvements for the block editor. But I also wonder about the next steps (collaborative editing) and whether this endpoint could be impacted. It seems locking won't mean the same thing.

I think we can get this merged in it's current form. I agree that post locking in collaborative editing mode would look very different, liking being stored and accessed differently. That would require a new and different endpoint. But we can cross that bridge when we come to it, it doesn't mean this PR can / should be blocked.

@swissspidy
Copy link
Member

Wouldn't it make more sense to actually make use of the endpoint in the editor to verify that it actually works as intended?

@spacedmonkey
Copy link
Member Author

Wouldn't it make more sense to actually make use of the endpoint in the editor to verify that it actually works as intended?

This is not how menu endpoints were merged. We can merge this and follow up with another PR with it's usage. Any tweaks that need to be made, can be made in that PR.

Copy link
Contributor

@anton-vlasenko anton-vlasenko left a comment

Choose a reason for hiding this comment

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

LGTM. I only have one small remark.

lib/class-wp-rest-post-lock-controller.php Outdated Show resolved Hide resolved
@enejb
Copy link
Contributor

enejb commented Jan 14, 2022

Thanks for introducing this.

🤔 I am not sure we need the GET wp/v2/{base}/{id}/lock endpoint, expect for completeness.

Instead, I think the lock info should be available on the single post and post query endpoints. This way when we display the posts in different contexts we can show who is already editing them and prevent the user from going into the editor in the first place.

cc: @hypest and @mkevins for thoughts and mobile app perspective.

@swissspidy
Copy link
Member

Thanks for introducing this.

🤔 I am not sure we need the GET wp/v2/{base}/{id}/lock endpoint, expect for completeness.

Instead, I think the lock info should be available on the single post and post query endpoints. This way when we display the posts in different contexts we can show who is already editing them and prevent the user from going into the editor in the first place.

Thanks to this endpoint the lock information can be _embedded in those other endpoints‘ responses to achieve exactly that.

@enejb
Copy link
Contributor

enejb commented Jan 14, 2022

Thanks to this endpoint the lock information can be _embedded in those other endpoints‘ responses to achieve exactly that.

Thanks, I didn't know about this.

@spacedmonkey
Copy link
Member Author

🤔 I am not sure we need the GET wp/v2/{base}/{id}/lock endpoint, expect for completeness.

The GET endpoint should used for dialogs like this.

Screenshot 2022-01-12 at 12 36 29

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core REST API Task Task for Core REST API efforts REST API Interaction Related to REST API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants