-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Add REST API for Annotations (Collaborative Comments) #3807
Conversation
@ someone with tag access. Please tag this as 'in progress' for me? Thanks :-) |
@ someone with tag access, please tag this as 'ready for review' for me please. |
These are my initial thoughts without looking at the code. I think from a user experience perspective, having annotations be hidden from contributors as soon as their post is published isn't desired behaviour. One use-case I can already think of is when a post is published but the original author wants to change something. Instead of changing the published post, they should be able to comment on the post so the editor can actually change the post. @omarreiss and @hedgefield can you comment? Have you considered that annotations can span multiple blocks? I think restricting annotations to one block is very limiting. I would like to be able to say: "Remove this section" in an annotation. That section could include a heading, 3 paragraphs and an image. Furthermore i could also see the selection starting and/or ending halfway into a block. How would you represent this in data? Or do you see this changing as we flesh out the JavaScript APIs necessary to create annotations? |
Thanks for doing the groundwork on this @jaswrks. This is an extensive PR already 👍 I can't comment much on the code, I'm no developer, but I'll say from a UX perspective that anyone that can comment should always be allowed to comment, regardless of which state a post is in. In our designs we also explored the idea of letting plugins hook into this system to give feedback via annotations, those would not all be 'solved' when a post is published. Keep up the good work! |
parent::__construct( WP_Annotation_Utils::$post_type ); | ||
|
||
// `rest_base` is already configured via register_post_type(). | ||
$this->namespace = 'gutenberg/v1'; // @codingStandardsIgnoreLine - PHPCS false positive on 'namespace'. |
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 feel it might be best to specify this string in some global config/variable instead, and reuse it from there i.e. if "gutenberg/v1" needs to change over time, then it can easily be overlooked (or hard to maintain) if it's sitting inside some class. Not sure if this is a convention though.
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.
No question. Thanks for spotting that.
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.
Took a second look at this. It does appear to be a convention followed by WP core and the reusable blocks controller also. And I suppose it does makes sense to leave it in the class definition, which needs to remain in support of the previous version; i.e., in the event that a new version of the API is released in the future.
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.
Ok - got it. Thanks for letting me know :-)
lib/class-wp-annotation-utils.php
Outdated
|
||
if ( $new_history_entry ) { | ||
$history[] = $new_history_entry; | ||
$history = array_slice( $history, -25 ); // Last 25 changes only. |
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.
Same as in my other comment. Is it possible to load "25" from some global configuration instead? It would help with maintenance and synchronisation of this class with whatever else is going on.
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 think it might be good to do an apply_filters
on this.
@atimmer writes...
Great point. Thanks! I'll see what I can do about that. |
@atimmer writes...
Agreed. I read over your thoughts on this in #3628 also. So the The selection object I'm proposing is based on your work: ...
"selection": {
"ranges": [
{
"begin": {
"offset": 12
},
"end": {
"offset": 493
}
}
]
} ... which could easily be expanded to support a much more elaborate JS endeavor. "selection": {
"ranges": [
{
"begin": {
"block": "da4e1738-adef-4f04-9790-9ad477e61adc",
"offset": 12
},
"end": {
"block": "0498de08-96f7-47c7-8450-6f23c38d767e",
"offset": 493
}
}, {
"begin": {
"block": "b48a805d-3ed9-46e2-84a7-03f78c192397",
"offset": 73
},
"end": {
"block": "b48a805d-3ed9-46e2-84a7-03f78c192397",
"offset": 345
}
}
]
}, Do you feel that covers all currently conceivable use cases? |
@hedgefield writes...
Cool. Did you see the cc @atimmer Ways to add, edit, delete annotations:
Ways to modify annotator data via filters: |
Awesome work on the PR! Mostly agree with what @atimmer and @hedgefield have stated. Annotations are useful regardless of the published state. (They are useful for much more than to highlight comments, but comments are probably a useful default UI in providing context for any type of annotation.) A couple of considerations:
|
@omarreiss writes...
Yes, I agree with this. Likely to be archived and not deleted. Whether it be a comment in WordPress, a conversation in Slack, or an annotation. Conversations should be archived not deleted. @omarreiss writes...
I share some of your concern there. To your point about them being admin-only... Raises New Questions
A deeper discussion about front-end vs. back-end post types seems a little outside the scope of this particular endeavor, but at the same time, still highly relevant in terms of performance. My feeling is that it would be better for annotations to be built on top of existing extensibility features, and then allow core to catch up and expose ways to optimize back-end-only post types. |
Thanks again for the feedback thus far. 2f7889d loosens permissions ever so slightly. Now allowing contributors to read, edit, and delete annotations in any post they've authored, even after it is published. There is more room to bend on this. If it seems I'm being stingy with permissions it's because I'd like to error on the side of caution. We can always tweak this further as additional use cases are identified that might require less restrictive permissions in a future iteration. |
Thanks, @jaswrks, great progress so far 🚀 I have two high-level questions (and some code comment, but will leave those inline):
|
@nb Thanks for the review 😃 My thought process went like this:
None of that is insurmountable and it may have been the same or less work than a custom post type. What worried me most is that comments are generally treated as public by WordPress core and many plugins. In other words, a 'comment' is most often a front-end content type that is authored by a user visiting the site, a note left by the post author, a live blog entry, or a ping. We can certainly change that! Hmm, but given the number of plugins that adjust, enhance, and add functionality to comments — That's not such a risk in the Liveblog plugin because liveblog entries are written as public postings. In the case of annotations, one author is having a private discussion with another. I'd like to secure that in the best way possible and I'm somewhat hesitant to throw a private comment type into the comments table. The |
@nb writes...
RationaleA good way to think of 'substatus', as it exists now, it to imagine you were using something like ZenHub or waffle.io on top of GitHub issues. With any of those, it suddenly it becomes possible for GitHub issues to not only have tags, but also have 'state'. Substatus is the 'state' of an annotation, and that state doesn't change the fact that it is still published. Right now, annotations can have an 'archived' substatus (state). In the future it might be nice to get more specific and support a 'resolved' substatus or a 'rejected' substatus. Instead of substatus, we could register a new post status Pros
Cons
Other ThoughtsA better name for substatus might be 'state', but not sure it's any less confusing. Another option would be to register a taxonomy for this, such as |
Another option worth investigating, which would steer this into an entirely new direction. Someone from hypothes.is reached out to me about this and suggested we take a look.
I don't know a whole lot about the standard yet (just scratching the surface at the moment), but I did join hypothes.is on Slack and I'll share anything that looks promising as I collect more information from them. They too are an open source project. |
Annotation Is Now a Web StandardI just took a closer look at the W3C: Web Annotation Data Model 💯 published earlier this year. It seems quite comprehensive, having been given a lot of attention by these fine folks. It appears to support everything discussed so far with respect to annotations in Gutenberg, and then some. Following that standard would include these benefits:
I suggest we take a deeper dive into this (the vocab and protocol too), then discuss it. See also: https://news.ycombinator.com/item?id=13729525 |
@jaswrks The annotation spec looks really promising. Selectors look a lot like discussions we've had with @youknowriad and @aduth. If we give every block an HTML |
Very happy to see this happening! Yay for interoperable standards! |
@atimmer Agree, definitely. If I'm reading the FragmentSelector spec correctly, it looks like a The CSSSelector could work that way too, only expressed as a CSS selector ... and then be further refined by a TextPositionSelector, or any other. "selector": {
"type": "CSSSelector",
"value": "#foo > ul:nth-child(2) > li:nth-child(3)",
"refinedBy": {
"type": "TextPositionSelector",
"start": 412,
"end": 667
}
} The RangeSelector looks interesting too, and it also reminds me of ideas that you presented. The difference being that a RangeSelector seems to be a way to start/end any other selector. And the SvgSelector looks like it would offer some very powerful functionality once we are able to implement this in an annotation client. For example, making it possible to select an area of an image. Here's an annotation using the WADM (Web Annotation Data Model) and based on my rudimentary interpretation of it at this time. Feedback and corrections welcome. {
"@context": "http://www.w3.org/ns/anno.jsonld",
"type": "Annotation",
"id": "http://example.com/wp-admin/post.php?post=123#annotation/123",
"canonical": "urn:uuid:dbfb1861-0ecf-41ad-be94-a584e5c4f1df",
"motivation": "editing",
"purpose": "commenting",
"creator": {
"id": "http://example.com/author/johndoe",
"type": "Person",
"name": "John Doe",
"nickname": "johndoe"
},
"created": "2017-12-20T00:00:00Z",
"modified": "2017-12-20T00:00:00Z",
"generator": {
"id": "https://wordpress.org/",
"type": "Software",
"name": "WordPress",
"homepage": "https://wordpress.org/"
},
"generated": "2017-12-20T00:00:00Z",
"body": {
"type" : "TextualBody",
"value" : "<p>Please fix this.</p>",
"format" : "text/html"
},
"target": {
"type": "SpecificResource",
"source": "http://example.com/wp-admin/post.php?post=123",
"styleClass": "yellow-highlight",
"selector": {
"type": "RangeSelector",
"startSelector": {
"type": "CssSelector",
"value": "#foo"
},
"endSelector": {
"type": "CssSelector",
"value": "#bar"
}
},
"renderedVia": {
"id": "https://github.com/WordPress/gutenberg",
"type": "Software",
"schema:softwareVersion": "1.9.1"
}
},
"stylesheet": {
"type": "CssStylesheet",
"value": ".yellow-highlight { background: yellow }"
}
} |
- Register new post type: wp_annotation - Add REST API controller: WP_REST_Annotations_Controller - Add annotation utilties: WP_Annotation_Utils - Add unit tests: REST_Annotations_Controller_Test referencing: #3026 Spaces to tabs. Hierarchical responses for annotations via REST API. - `hierarchical=flat` - `hierarchical=threaded` - Enhance existing unit tests. - Add unit tests for hierarchical responses. - fix: Allow `parent=0` when checking permissions. Delete annotations on parent deletion. - Delete a post's annotations whenever the post itself is permanently deleted from the database. Attached to `delete_post`. - `WP_Annotation_Utils::$post_type` property for faster interal reads. New unit tests; enhance existing unit tests - Add `class-annotations-test.php` to cover `WP_Annotation_Utils` - Generate additional fake test data for all annotation unit tests. - Enhance unit tests that cover `WP_REST_Annotations_Controller`. - fix: In call to `register_post_type()`, the `read` cap should map to `edit_posts` for annotations. fix: Don't die on nonexistent parent. fix: typo in docblock Add substatus history length filter Allows annotation substatus history length to be increased or decreased using a filter: `gutenberg_rest_annotation_substatus_history_length` Loosen permissions slightly for contributors - If a user is the author of a post, and the only reason they can't edit the post is because they can't edit_published_posts, and the parent post's current status is `publish` or `future`, then allow the user to annotate. - Update unit tests to reflect revised permission strategy. Adjust annotation object model. - Rename and repurpose `md5_email`. Now `image_url` with avatar URL. - Add readonly `author_meta` property with display name and avatar URL. - Allow `additionalProperties` in `annotator_meta` schema definition.
- Plus a minor fix to correct a failing PHPUnit test in blocks.
Closing this in favor of: |
Referencing: #3026
New Annotation Post Type
My work in this PR explores the use of a custom post type to store annotations that will power collaborative commenting in a future version of Gutenberg. I start from the premise that an annotation can be a child of any other post type in WordPress.
For this to work effectively in Gutenberg I am focused here on developing a REST API and the object model associated with annotations (see JSON examples below).
Progress, todo list
wp_annotation
WP_REST_Annotations_Controller
WP_Annotation_Utils
REST_Annotations_Controller_Test
hierarchical=flat
hierarchical=threaded
parent_post_id
is deleted.WP_Annotation_Utils
md5_email
toimage_url
and use avatar URL by default."author_meta": {}
withdisplay_name
andimage_url
props.additionalProperties
inannotator_meta
schema definition.A UI that utilizes the annotation object model.I will work on this in a separate PR.
Checklist:
How to Test Annotation Endpoint
I'm providing a few examples as a Postman Collection that can be used to run tests against the new REST API endpoint for annotations. You'll need the Basic Auth plugin for the WP REST API.
Annotation Object Model (REST API)
Here is the annotation object model I've implemented in this PR, which extends the object model for any custom post type in the WordPress REST API. I've given a brief description of the additional custom REST fields below; e.g.,
parent_post_id
,substatus
, and others.full REST API Schema generated by WordPress.
Quick Overview of Annotation Properties
parent_post_id
(required)Every annotation is associated with a parent post that's not another annotation. So as you might have guessed,
parent_post_id
points to a 'post', 'page', or another custom post type. Theparent_post_id
is a reference back to the annotation's top-level non-annotation parent, and every annotation receives this property, regardless of depth.parent
(optional, default=0)This property is standard in all post types and establishes a parent/child relationship between posts of the same type. It's the same with annotations.
parent
allows for threaded conversations just like comments in WordPress. Thus, top-level annotations on a post haveparent=0
, and any replies to that annotation haveparent=[parent annotation ID]
, which is what makes it a reply.author
(strongly suggested)A
WP_User->ID
- the user who created the annotation.selection
(optional, default=null)This identifies one or more selection ranges associated with the annotation; e.g., a text or markup selection in Gutenberg. I'm going to remain vague about this for the time being, because this field is the one most likely to bend a little — to meet the needs of a UI powered by JavaScript.
substatus
(optional, default=)One of the following:
''
archived
Every post type in WordPress has a status, and an annotation has one too. In addition, an annotation has a
substatus
. This allows an annotation to remain published, but be flagged as archived — i.e., still available, but perhaps less obvious and not in your way while writing.last_substatus_time
(readonly)last_substatus_time
is the last timesubstatus
changed.substatus_history
(readonly)A history of the most recent 25
substatus
changes, including which author or annotator performed thesubstatus
change.annotator
andannotator_meta
(optional)While these are both optional, if one is given, the other is required.
What is an Annotator? It's a plugin that creates bot-like annotations. If both
annotator
andannotator_meta
are set, a UI built on this object model should then recognize the annotator as the author (like a bot message in Slack). So these two fields are an easy way for plugins to create annotations on behalf of a user working in the WP Dashboard.Here's one way a plugin might go about doing that in the future.
An annotator is identified by the
annotator
property, set to a non-numeric slug; e.g.,yoast
,grammarly
,my-cool-plugin
. An annotator must be accompanied byannotator_meta
, which supplies adisplay_name
for the annotator and animage_url
property, which a plugin should fill with their product or company icon.annotator_meta
is also designed with extensibility in mind, so it allows any additional arbitrary properties too, and there are also some nice filters in this PR.How about using the comment post type instead?
That was my first instinct. WordPress comments and much of their existing functionality matches that of annotations in Gutenberg. I even started down that path, but quickly decided it was the wrong approach. Why? Custom post types are an extensibility feature. Comments are not. And comments are almost always public too, which is significantly different from annotations. Especially when you consider that WordPress core tends to treat comments as public postings, as do many popular plugins. So trying to mold a comment post type that is generally treated as public, into an annotation that is always private — it's asking for trouble IMO. I ran into technical limitations right from the start. A new post type for annotations is much for flexible.
Connecting Annotations to Gutenberg Blocks
The current plan is not to do that, not exactly, that is. Instead, a block will own the annotations that it has. So a block will carry all top-level annotations (those with
parent=0
) with it as attributes. A query can be made for all top-level annotations, and a hierarchical thread will be returned by this new API.Threaded Annotations via REST API
This PR adds support for hierarchical annotations in REST API responses. There are currently two supported hierarchical options, which match the options currently in WP core for comments.
hierarchical=flat
- All child annotations are appended to the response in a flat structure.hierarchical=threaded
is the most useful. This gives each annotation in the response achildren
property, which is an array of all replies, and those replies may havechildren
too.Threaded Annotations Example
.../gutenberg/v1/annotations?hierarchical=threaded
WP Annotations: Postman Collection Examples
Annotation Permissions Diagram
Update: Diagram now slightly outdated. Following some initial feedback, Contributors now retain access to their annotations even after their post is published; i.e., they can still read, create, edit, and delete annotations. I will update the diagram soon.
Deleting a Parent Post
Permanently deleting a post results in all of that post's annotations being deleted along with it.