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 advanced follow management #622

Draft
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

Menrath
Copy link
Contributor

@Menrath Menrath commented Dec 25, 2023

This Draft Pull request brings a follow request management. It allows for manual accepting and rejecting a follow request and sends the proper ActivityPub response to the followers inbox.

How its done

It introduces a new custom post type ap_follow_request which is used to store follow requests. Each follow request post is the child post of the follower post.

The follower table in the admin page (currently only hardcoded for the application actor) is now a join between the information saved in the post types ap_follow_request and ap_follower.

Fixes

  • Currently if a user deletes a follower, this follower is deleted for all other WordPress ActivityPub actors too.
  • The follow request (it's ID) is not saved, so no Accept or Reject Activity can be sent later (async).

Open discussions points

  • Should manualylApprovesFollowers be a setting that is available to each WordPress actor/user
  • Should a user that does not manually approve followers be able to remove/reject a follower posthum?
  • Make final decision of where to store follow relationships and follow-request ids

Tasks that can only be done once the above discussion points are solved

  • Depending on the issue above: write migrations/implement a fallback.
  • Remove overall complexity and cleanup
  • Write uni tests for all new features.
    Not yet. First I would like to discuss this proposal (next year, after the holidays of course). Also this is still a draft and proof of concept. There are many parts where things could and should be refactored as well as improved.

No Testing instructions, but a short video what can be done now

Bildschirmaufzeichnung.vom.2023-12-25.22-31-08.webm

- the application actor is managed manually by default
- no admin-options are included yet
- the old new follower table only is used hardcoded by the application actor
- no admin notifications are send yet
- todo: a lot more
@pfefferle
Copy link
Member

pfefferle commented Dec 25, 2023

Thanks for the PR and happy holidays ☺️

This is a lot of overhead to simply have a state like "pending" or "to approve"! The follow-request scheme felt like a complete copy of the follower structure (when reading the change on the phone), but it is indeed a way simpler solution, so forget my first comment! But still: If the current db follower scheme does allow this easily, feel free to rethink the structure completely (but I would still avoid custom tables).

@pfefferle
Copy link
Member

If this is a completely different thing maybe we should handle it completely different? Maybe as a "server follow" or "server connection"?

TODO: one should decide for a clear naming
@Menrath
Copy link
Contributor Author

Menrath commented Dec 26, 2023

I fixed a small typo, which may have lead to confusion, if anyone should have tested it.

If this is a completely different thing maybe we should handle it completely different? Maybe as a "server follow" or "server connection"?

I don't think it's any different. The main difference is that if we allow actors (something that can be followed within WordPress) to manually approve followers ( as:manuallyApprovesFollowers) then the follow process must be asynchronous. Therefore we need to store the follow request somewhere. Also, if an actor inside WordPress decides later not to allow a certain follower, the follower relationship should not just be deleted on the WordPress side, but the original follow request should be rejected (or the accept should be undone - still unclear to me), anyway the instance of the follower should be informed.

Currently all actors within WordPress have manuallyApprovesFollowers set to false, only the application user has set it so true. This is hard-coded and at the moment there is no option to change that. But there could be such a feature with ease, for example to let the admin set a default for the user-actors and let the users override it.

But still: If the current db follower scheme does allow this easily, feel free to rethink the structure completely (but I would still avoid custom tables).

I already have further ideas in mind. You were quite right with "at first it felt like a complete copy of the follower structure". It did to me too! 🥲

Maybe it's due to the fact that the Followers are being stored as Followers not as just actors with for example the post type being called ap_actors rather than ap_followers. Also note that now there are some other functions in the follower class that are now not needed anymore, I just did not have time yet to clear up things, the state is still more a "proof of concept".

Further I tried to have two things in mind:

I believe that between the lines I have expressed my idea, for the ActivityPub actors within WordPress to be decoupled from the WordPress user table. So if the followers should in the future be store with post type 'ap_actor' the followed WordPress actors could also just stored with this post type.

@pfefferle
Copy link
Member

I don't think it's any different. The main difference is that if we allow actors (something that can be followed within WordPress) to manually approve followers ( as:manuallyApprovesFollowers) then the follow process must be asynchronous.

I do not speak about the approval process in general. You began with the PR because we need an approval process at the application level for Mobilizon and maybe Peertube. This process does not have to be mixed up with the classical follow process. The application user should not be followed by normal users, and it should not follow normal users. Therefore, the question is: Does it make sense to have a different scheme for it if there is no overlap anyway?

Maybe it's due to the fact that the Followers are being stored as Followers not as just actors with for example the post type being called ap_actors rather than ap_followers.

It's not about the naming (we can easily change that later on); it's the persistence that gets hackier and hackier. Inspired by the followers/following model of the friends plugin, I started with a custom taxonomy, which worked perfectly. Sadly, we had issues with that on WordPress.com, so I changed it to a custom post type. But it always felt like a compromise, and it's getting worse the more we try to add to that construct.

I believe that between the lines I have expressed my idea, for the ActivityPub actors within WordPress to be decoupled from the WordPress user table. So if the followers should in the future be store with post type 'ap_actor' the followed WordPress actors could also just stored with this post type.

If we try to use one custom post type for Followers and Followings, and the same for the Application, we have to work with sub-posts and metas even more, making everything even more complex and slow!

@pfefferle
Copy link
Member

pfefferle commented Dec 26, 2023

I am still not sure about the sub-post solution (or the whole follower structure, like I described above), but regardless of that, I don't think it should be a different model or collection. It "only" adds a state to the follow process, so we should try to handle that in the follower class, in my opinion (even if it's technically a different post type).

@@ -0,0 +1,57 @@
<?php
Copy link
Member

Choose a reason for hiding this comment

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

I would not list both tables on one page!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree! Haven't thought of how to structure this well yet. Any ideas welcome!

Copy link
Member

Choose a reason for hiding this comment

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

I would add a tab for that, or a filter. That would be another argument for using the same custom post type as for the follower

@Menrath
Copy link
Contributor Author

Menrath commented Dec 27, 2023

I am still not sure about the sub-post solution (or the whole follower structure, like I described above), but regardless of that, I don't think it should be a different model or collection. It "only" adds a state to the follow process, so we should try to handle that in the follower class, in my opinion (even if it's technically a different post type).

I am open for discussions! :) I just had to decide for some structure to learn about everything involved in the follow handling process. I also thought about using terms/taxonomies or heavily use the post_meta. I should write those thoughts down. Please bear with me, I'm not yet an expert on the WordPress database structure and I can often only guess at the impact on performance. I am grateful to receive tips and will gladly change my code.

Does it make sense to have a different scheme for it if there is no overlap anyway?

If there are enough/any technical differences then I would answer yes. When receiving a follow I think there aren't any yet. Maybe one could think of only automatically rejecting all non- Application actors, or make that the actor is listed in the nodeinfo a condition too for even achieving the state where the WordPress admins can have an Accept option? But again this is IMHO just an extra condition in a handler, and not a real different process. Do you have other differences in mind?

About custom tables: People say to avoid them whenever you can. I basically just stick to that advise. I didn't reached the level of WordPress expertise to decide when using custom tables actually could make sense.

@pfefferle
Copy link
Member

pfefferle commented Dec 27, 2023

What about working with the post state (pending, approved, ...) instead of a new custom post type? I would also love to see the model and controller of the request merged into the follower structure (model & collection) if that's possible. To simplify things for implementers.

What do you think?

'post_name' => esc_url_raw( $this->get_id() ),
'post_excerpt' => sanitize_text_field( wp_kses( $this->get_summary(), 'user_description' ) ),
'post_status' => 'publish',
'post_content' => $this->to_json(),
Copy link
Member

Choose a reason for hiding this comment

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

We were not sure if the default filters used on the content would interfere with the JSON. Do you have some experience with using JSON as post-content?

Copy link
Contributor Author

@Menrath Menrath Dec 27, 2023

Choose a reason for hiding this comment

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

Default filters and also other plugins should not mess with post_types they don't know (in my opinio and in an ideal world) but it would be a case, where I would reach out to the community (via Mastodon) and see if I can gather some opinions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another point: the json representation of the follower actor which is saved in the post_content and the according inbox used by WordPress which is saved in the post_content_filtered are recoverable things. They are just cached. They can be retrieved at any time, as long as the actor id which is stored in the post_guid remains intact.

Copy link
Member

Choose a reason for hiding this comment

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

I also thought about adding the inbox to the to_ping field instead!?!

Copy link
Contributor Author

@Menrath Menrath Dec 27, 2023

Choose a reason for hiding this comment

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

The to_ping field is used heavily in the core. And to what I found in the internet posted by Automattic devs almost two decades ago, the post_content_filtered seems like the ideal place to store transformed/filtered data.

But I also liked the to_ping field, I just am not sure whether it may work.

@@ -0,0 +1,57 @@
<?php
Copy link
Member

Choose a reason for hiding this comment

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

I would add a tab for that, or a filter. That would be another argument for using the same custom post type as for the follower

@pfefferle
Copy link
Member

Or maybe we could simply work with metas, like activitypub_pending_user_id and rename it to activitypub_user_id when approved?

@Menrath
Copy link
Contributor Author

Menrath commented Dec 27, 2023

What about working with the post state (pending, approved, ...) instead of a new custom post type? I would also love to see the model and controller of the request merged into the follower structure (model & collection) if that's possible. To simplify things for implementers.

The main problem is that there can be multiple pending/approved states for each follower. For now I separated the logic from adding a follower and adding the follow relationship between the follower the targets. Now it should also be possible to save the follow requests in the post_meta for the follower. The information that has to be saved is the follow id (which is generated by the other instance and must not be lost) and the status. One could either use "named meta field keys" or a single meta field key with serialized json. Or one could use terms/taxonomies to assign this information to a follower. This would result in slow queries of course, but these queries would only be done when the user takes action in the WordPress admin section.

@Menrath
Copy link
Contributor Author

Menrath commented Dec 27, 2023

Or maybe we could simply work with metas, like activitypub_pending_user_id and rename it to activitypub_user_id when approved?

Here we would in both cases have to save the id of the follow object somewhere related. Any ideas? Thats why I am struggling for so long :/

@pfefferle
Copy link
Member

Because we had a lot of discussions and problems with taxonomies on WordPress.com I would avoid them for now. My problem is with the sub-type is, that it adds a lot of complexity that could break things.

Can you explain the current idea a bit more in detail? What if one mastodon user is trying to follow two WordPress users? We create a follower, but how many requests and how many user_id metas?

@pfefferle
Copy link
Member

Here we would in both cases have to save the id of the follow object somewhere related. Any ideas? Thats why I am struggling for so long :/

My idea was, to add a pending follower as meta activitypub_pending_user_id to the follower object. And if the user accepts we rename the meta to activitypub_user_id?!? Then we would know if it's pending or not through the relation between the follower and the user.

@Menrath
Copy link
Contributor Author

Menrath commented Dec 27, 2023

Because we had a lot of discussions and problems with taxonomies on WordPress.com I would avoid them for now. My problem is with the sub-type is, that it adds a lot of complexity that could break things.

O.K.!

Can you explain the current idea a bit more in detail? What if one mastodon user is trying to follow two WordPress users? We create a follower, but how many requests and how many user_id metas?

  1. When MastodonUser1 wants to follow WordPressUser1 the MastodonUser1 gets saved as a post with post type ap_follower if a post with the same guid does not exist yet. The guid reflects the Actor-id of the MastodonUser (it'S URL).

  2. Now the id of the Follow request has to be stored somewhere (this is still to be discussed). We need the id in case we want to do async Accept or Reject responses. Note that the id does not always tell us about the target actor or the actor issuing the follow request, so we maybe have to save more information).

My idea was, to add a pending follower as meta activitypub_pending_user_id to the follower object. And if the user accepts we rename the meta to activitypub_user_id?!? Then we would know if it's pending or not through the relation between the follower and the user.

This is the point were this solution seems not to be enough.

  1. If WordPressUser1 does not manually approve followers the follow relationship is added instantly via a postmeta for the Follower (in the post table as post type 'ap_follower', just like before this PR). Then the Accept response is sent.

When MastodonUser1 then sends a follow request to WordPressUser2 user nothing happens at step 1 cause the Follower is already known to WordPress. In step 2 we have a different Follow request id that also has a different target. If WordPressUser2 also does not manually approve followers the follow relationship is added via an additional entry in the postmeta table, but the same key ('activitypub_user_id') but a different value: the user id of WordPressUser2.

My intuition would also have been to store the followers follow relationships of a WordPress user in the usermeta table: save an array of the custom post type ids of the followers. This is because most of the time we know about the user first and then want get its followers, not the other way round: get a follower and then tell me all target users it has accepted follow relationships with.

Which type of query and when?

When dispatching posts for a WordPress user and the follow relationships haven't changed the cached list of inboxes is used.

If the follow relationship have changed since the last dispatch, first the follow relationships have to be looked up (that is because every time we edit the follow-relationship the cached target inboxes for this user get deleted). Then with that list a distinct list of inboxes can be generated again.

If the user looks into his admin menu to browse his followers the query is not for the follow relationships, but for all the follow requests that target this WordPress user. I wouldn't mind if this query is rather slow, when the trade of is that the other queries are faster.

Conclusion

The state of a follow request could also be just kept in the difference between "there is a follow request from actor x to user y" and "there is no active follow relationship between actor x and user y".

@pfefferle
Copy link
Member

pfefferle commented Dec 27, 2023

I started with storing all followers in one big user meta! I decided to have everything in the custom post and the post meta, to easily filter, sort and page the followers. If we add the ids to the user, we can't do that any more and have to use big lists of ids and query with IS IN or have to use plain SQL with joins instead of WP_Querys. We also have the problem that not every user is represented in the db (blog and application for example).

@pfefferle
Copy link
Member

pfefferle commented Dec 27, 2023

This is the point were this solution seems not to be enough.

  • Mastodon1 tries to follow Blog1 (manual approve enabled). The Follower Mastodon1 will be created with the post meta activitypub_pending_user_id with the id of Blog1.
  • Mastodon1 tries to follow Blog2 (manual approve enabled). Follower Mastodon1 already exists, so it only adds activitypub_pending_user_id with the id of Blog2.
  • Mastodon1 is following Blog3 (manual approve disabled). Follower Mastodon1 already exists, so it only adds activitypub_user_id (without pending prefix) with the id of Blog3.

So the current state of Mastodon1 is he follows 1 Blog and two are pending (waiting for approval).

If Blog2 decides to approve the request, we change the activitypub_pending_user_id with the id of Blog2 to activitypub_user_id.

Should be possible and easy to have queries based on the different types.

Still not voting for this beeing the best solution, just wanted to show that it works ☺️

TBD.

@Menrath
Copy link
Contributor Author

Menrath commented Dec 27, 2023

Thanks for reading through all my thoughts. 🙇

Doing what you proposed would work almost, but for storing the ids of the Follow request (not of the Follower actor) one would need something else too. For example a meta key called activitypub_follow_ids which contain a serialized array like

array( 
   '<some_follow_id>' => <target_wp_user_id>,
   ...
)

Or using a customized key like activitypub_follow_id_<user_id> which holds the id. Both approaches don't seem ideal to me, but they could be done.

@pfefferle
Copy link
Member

It seems hard to explain it in text.

It should work without helper tables, because I simply replaced the Follow_Request object (you introduced) with a activitypub_pending_user_id post meta for the follower. Because it is a post meta it has the follower id by default and maps it to a blog user and the name tells you that it's a pending connection. Currently you have activitypub_user_id to map a follower to a user. My proposal is to add activitypub_pending_user_id for pending users.

Maybe we have to have another call ☺️

@Menrath
Copy link
Contributor Author

Menrath commented Dec 27, 2023

It seems hard to explain it in text.

It should work without helper tables, because I simply replaced the Follow_Request object (you introduced) with a activitypub_pending_user_id post meta for the follower. Because it is a post meta it has the follower id by default and maps it to a blog user and the name tells you that it's a pending connection. Currently you have activitypub_user_id to map a follower to a user. My proposal is to add activitypub_pending_user_id for pending users.

Maybe we have to have another call ☺️

We definitely will! 😊 (...I thought I would do real holidays and take some time off but I working on this without pressure is quite relaxing too!)

Back to the topic: The ActivityPub id of the follower (actor of type Person, Groups, etc.) is different from the id of the ActivityPub follow request (activity of type Follow).

In Mobilizon the follow id looks like: https://mobilizon.lan/follow/228b35b8-b743-4bbd-a363-1214fba6c287 whereas the actor id of the application actor would look like: https://mobilizon.lan/relay.

But something that I don't know is how receiving an Accept is typically handled by other fediverse software: it theoretically could be handled by

  • solely having the follow id (e.g. Mobilizon: https://mobilizon.lan/follow/random-uuid)
  • or having both the target and the origin actor id. (e.g. Mobilizon https://mobilizon.lan/relay and https://wordpress.lan/username)

though the standard suggests using at least the first approach.

@pfefferle
Copy link
Member

Ah, because of the follow (activity) id!?!

@Menrath
Copy link
Contributor Author

Menrath commented Dec 27, 2023

If we then realize that nobody uses it we would maybe have a solution, too.

Edit: Seems like Mobilizon is not using the id, it is using the object. The follow (activity) id only gets used for error logging. @tcitworld

@pfefferle
Copy link
Member

One more idea: why not use that to start an inbox/outbox custom post type and use that to store and process the request?!?

@Menrath
Copy link
Contributor Author

Menrath commented Dec 29, 2023

One more idea: why not use that to start an inbox/outbox custom post type and use that to store and process the request?!?

At the moment I can't exactly assess what the benefits of this might be.

@pfefferle
Copy link
Member

One more idea: why not use that to start an inbox/outbox custom post type and use that to store and process the request?!?

At the moment I can't exactly assess what the benefits of this might be.

I am working on that anyway, because with comments support, we can no longer query the posts table for the Outbox Collection. A persistent Outbox is also needed for C2S support in the future. The Inbox was planned as second step, because of the same reasons and to also support Activities that are not directly interpreted by the plugin itself.

So to not have things twice or more, maybe we could re-use the Inbox idea?

@Menrath
Copy link
Contributor Author

Menrath commented Dec 29, 2023

I am working on that anyway, because with comments support, we can no longer query the posts table for the Outbox Collection.

For reference (I try to explain a bit more, for everyone following this tread): The Outbox is a OrderedCollection which contains everything a user has published through ActivityPub (their maybe differences depending on the audience of an activity and who queries the outbox). Getting a full outbox in the future would require fetching all of an actor's comments from the wp_comments table and all of their public activitypub-enabled posts from the wp_posts table and joining them together in order. I wonder where the "persistent" condition is not possible. Is there a better place to discuss this at the moment?

A persistent Outbox is also needed for C2S support in the future.

Client-to-server support: Interesting, but almost nobody implemented that (see https://socialhub.activitypub.rocks/t/activitypub-client-to-server-faq/1941). The Mastodon API kind of became the used alternative.


More related to this topic: If we would indeed abstract the actors in a custom post type, we could store follower_ids as a postmeta on the followed actor not the other way round. I think that then we could also store the follow requests as a (single) postmeta. Thats because one never needs to query the other way round and accepted relationship can be stored like before. If you still have doubts about adding too many custom post types (though I don't know of any disadvantages using custom post types for such things).

I just had the idea for drawing a graph for that via drawio, do you think this could be of help (especially for other open source community people joining the conversation?).

@pfefferle
Copy link
Member

pfefferle commented Dec 29, 2023

It's not really the number of CPTs, but the resulting complexity. That's why I also voted for merging the request logic into the follower collection/model and to handle it as a state, even if it is a different db-scheme. This way it would be easier to change the db structure later on.

If we really see them as "standalone Activities that needs an answer/reaction/interaction" instead, then I would prefer the inbox collection.

I am fine with changing the WordPress User structure. But I am not sure in which way!!! Either to have all users as Actors in a custom post type (that simplifies and unifies the handling of virtual users like the Application and the Blog User but makes syncing changes between both object complicated) or to create WP_Users also for the Application and Blog User that has a similar effect.

I am open to discuss all options. Maybe we should start them in the Discussion Board?

@Menrath
Copy link
Contributor Author

Menrath commented Dec 29, 2023

I am open to discuss all options. Maybe we should start them in the Discussion Board?

I created a board: #623. As this is an import task that affects a lot of future work, I would suggest to get as many help from others as we can get: the open source community and experts on WP_Queries at Automattic.

@Menrath
Copy link
Contributor Author

Menrath commented Jan 5, 2024

I am currently also preparing a pull request similar to this one with minimal changes to the codebase, but follow-management being restricted to the application actor.

Besides that I also started a poll to ask the community how important advanced follow management is for them: https://graz.social/@linos/111699325774324467

And I noticed: also the timestamp for a follow request is a nice thing to have, especially for the follower table.

@Menrath
Copy link
Contributor Author

Menrath commented Apr 15, 2024

@pfefferle Any new thoughts on this meanwhile? Now I could find some time to draft an easier version of this focusing on follow management solely for the application actor, which should always require approval I'd say. I would add an extra tab on the settings page and keep the interface similar. The follow request IDs I would then store as JSON-within the database, which is not good practise, but since the requests will be very very few and we can avoid introducing other ways of storing the data.

@pfefferle
Copy link
Member

We are currently discussing our possibilities with different DB schemes. I will post an update as soon as we are sure about our options.

@Menrath
Copy link
Contributor Author

Menrath commented Apr 15, 2024

We are currently discussing our possibilities with different DB schemes. I will post an update as soon as we are sure about our options.

Great! Than I'll focus on other issues and will come back to this one, once I hear an update from you! :)

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.

None yet

2 participants