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

Add replies collection #876

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

Menrath
Copy link
Contributor

@Menrath Menrath commented Sep 1, 2024

Add replies collection to posts and federated comments.

Solves Issue #138

Description

The replies collection offer a neat way to sync replies over multiple instances. Mostly on demand and sometimes also client side on demand. They are not yet frequently used because of the moderation issues that arise with them. But since WordPress already got a good approve-system for comments this should not be too problematic here.

Rest Endpoint

To make sure the ID's of the collection and the collections page are resolveable.
It also adds a new rest endpoint /activitypub/1.0/(posts|comments)/<id>/replies which returns a Collection. Adding the parameter ?page=0 returns the CollectionPage directly.
I decided not to use something like <the_permalink>/replies because it is only needed for activity+json and has no frontend view. Seemed consistent to me.

Unit tests:

I started writing new unit tests, but was not sure how deep to test this. I'm happy for some discussions.

Manual testing:

Write federated comments for a post, or receive comments and make sure those comments are approved.
When querying the ActivbityPub's JSON representation of the post the replies key exists, e.g.:

...
 "replies": {
    "id": "https://wp.lan/wp-json/activitypub/1.0/posts/49/replies",
    "type": "Collection",
    "first": {
      "id": "https://wp.lan/wp-json/activitypub/1.0/posts/49/replies?page=0",
      "type": "CollectionPage",
      "partOf": "https://wp.lan/wp-json/activitypub/1.0/posts/49/replies",
      "items": [
        "https://mz.lan/comments/8be36040-5a02-4335-917f-4c5dcc988e1f",
        "https://mz.lan/comments/de26c344-8d7d-400f-a8f5-5b89307bb72a",
        "https://mz.lan/comments/abce32af-a59e-41c6-8b47-a8925980f1ef",
        "https://wp.lan/?c=13",
        "https://mz.lan/comments/fa3d2ee2-147b-4eee-b682-98b0b7115603"
      ]
    }
  },
...

Changing the global option comments_per_page to for example "3" would change the response to this:

...
"replies": {
    "id": "https://wp.lan/wp-json/activitypub/1.0/posts/49/replies",
    "type": "Collection",
    "first": {
      "id": "https://wp.lan/wp-json/activitypub/1.0/posts/49/replies?page=0",
      "type": "CollectionPage",
      "partOf": "https://wp.lan/wp-json/activitypub/1.0/posts/49/replies",
      "items": [
        "https://mz.lan/comments/8be36040-5a02-4335-917f-4c5dcc988e1f",
        "https://mz.lan/comments/de26c344-8d7d-400f-a8f5-5b89307bb72a"
      ],
      "next": "https://wp.lan/wp-json/activitypub/1.0/posts/49/replies?page=1"
    }
  },
...

Note that the items only contains 2 IDs. This is because the code currently uses core functions to query the paginated comments and later filters out the local-only ones. Because we have more pages also the key next is present.

It (https://wp.lan/wp-json/activitypub/1.0/posts/49/replies?page=1) resolves to:

**https://wp.lan/wp-json/activitypub/1.0/posts/49/replies?page=1**
{
  "@context": [
    "https://www.w3.org/ns/activitystreams",
    {
      "Hashtag": "as:Hashtag"
    }
  ],
  "id": "https://wp.lan/wp-json/activitypub/1.0/posts/49/replies?page=1",
  "type": "CollectionPage",
  "partOf": "https://wp.lan/wp-json/activitypub/1.0/posts/49/replies",
  "items": [
    "https://mz.lan/comments/abce32af-a59e-41c6-8b47-a8925980f1ef",
    "https://wp.lan/?c=13"
  ],
  "next": "https://wp.lan/wp-json/activitypub/1.0/posts/49/replies?page=2"
}

Unapproved or local only comments should never be listed.

Discussion points

  • This draft has always functions handle both replies for comments and posts rather than splitting up the code in the beginning. I just chose this construct to get to a working prototype and I already fear that there might be good arguments to do it differently.
  • The code which gets the federated comment IDs: https://github.com/Menrath/wordpress-activitypub/blob/replies_collection/includes/collection/class-replies.php#L108 These lines are duplicated or have very similar occurences, more than once in the code.
  • Maybe use a direct meta-query to get all approved-federated comments in a paginated way.
  • I am happy to hear any ideas and thoughts.
  • Mastodon for example lists all self-replies on the first page of the collection, and external replies later, this would also be possible, to ensure threads are displayed. Threads are possible now also in WordPress, but maybe not that important.
  • Mastodon includes local replies fully, not only the ID. Within WordPress this in theory could get difficult because of the JSON-LD context (different post types with different ActivityPub object types at worst).

@pfefferle
Copy link
Member

Awesome 😍

@Menrath Menrath marked this pull request as ready for review September 5, 2024 08:07
@pfefferle
Copy link
Member

pfefferle commented Sep 13, 2024

@Menrath Is this ready to merge at least as a first attempt?

@Menrath
Copy link
Contributor Author

Menrath commented Sep 14, 2024

I would love to see some more unit tests, but besides that I feel from my point of view I have nothing to add atm :)

Copy link
Contributor Author

@Menrath Menrath left a comment

Choose a reason for hiding this comment

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

Seems all good to me.

if ( is_local_comment( $comment ) ) {
continue;
}
$comment_meta = \get_comment_meta( $comment->comment_ID );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This "if elseif" clause is now present 3-5 times in the code, sometimes slightly different. I have not fully understood why it is slightly different in some cases. But it seems this could be modularised into a function somewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pfefferle Should someone have a look at this before this PR gets merged? I could find time in the next days.

Copy link
Member

Choose a reason for hiding this comment

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

but be aware that there are two different usecases:

  1. We want to have the ID with URL as fallback.
  2. We want to have the URL with ID as fallback.

includes/rest/class-collection.php Show resolved Hide resolved
includes/collection/class-replies.php Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants