-
-
Notifications
You must be signed in to change notification settings - Fork 916
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 columns post.pending, comment.pending #5499
Conversation
11cab5a
to
47a938a
Compare
Post::update(&mut context.pool(), post.id, &form).await?; | ||
} | ||
return Err(e.into()); | ||
} |
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.
This code is rather awkward at the moment...
@@ -145,7 +145,16 @@ impl Object for ApubComment { | |||
context, | |||
)) | |||
.await?; | |||
verify_is_remote_object(¬e.id, context)?; | |||
if let Err(e) = verify_is_remote_object(¬e.id, context) { |
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.
if let Err(e) = verify_is_remote_object(¬e.id, context) { | |
if let Err(e) = verify_is_remote_object(¬e.id, context) { | |
// The comment was created locally and sent back, indicating that the other instance did not reject it |
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.
Thanks, added
@@ -163,7 +163,16 @@ impl Object for ApubPost { | |||
context: &Data<Self::DataType>, | |||
) -> LemmyResult<()> { | |||
verify_domains_match(page.id.inner(), expected_domain)?; | |||
verify_is_remote_object(&page.id, context)?; | |||
if let Err(e) = verify_is_remote_object(&page.id, context) { |
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.
if let Err(e) = verify_is_remote_object(&page.id, context) { | |
if let Err(e) = verify_is_remote_object(&page.id, context) { | |
// The post was created locally and sent back, indicating that the other instance did not reject it |
What about all the other items: private messages, reports, communities? These too could have been created by banned users. If the root issue is that federated site or community banned users should not be able to create any content on our instance / community, then that should be handled by properly federating those bans, and checking the |
Bans are handled by #5515. But there are also other reasons why a post may not get published, for example plugins may reject posts if they contain blocked words (and in fact the builtin slur filter as well). Unlike bans it is impossible for us to know this in advance, so we need to wait if the post gets anounced by the community instance or not. Private messages, reports and communities are different. In those cases it only matters if the user was banned on his home instance or on our instance, which is already handled. But in case of posts and comments, it is also possible that the user is banned on a third instance (where the community is hosted), so we need to handle it differently. |
Don't plugins by nature have to take action on items that already exist via create hooks? Otherwise adding a before create / validation hook would be better for that use case.
If #5515 also federated information about bans to all servers (not just the one where the community lives), then that shouldn't be a problem (at least after federation). The reason I'm wary about pending, is now we'll have more cases of posts / comments missing and not showing up, with unclear rules and cases as to why they might not be showing up. Not to mention that now we'll have to account for pending as a filter in all the DB queries, so this would probably have to alter most indexes, especially on the post table. If we add this lets at least limit it only the |
The plugin feature also includes before hooks to reject posts (eg for spam filter). See here in the example plugin. I agree that this PR has some risks, and cases where it wont work properly if the
Not sure what you mean, simply rename the db columns? |
It's up to you if you want to rename I spose, but just make sure that column is well commented in the rust struct. |
Renamed the column to federation_pending and added comments. Also added helper method to set pending false, and call this method when receiving a federated vote. This way if the Announce from the community is not delivered for some reason, the post will still be published once it gets voted on. |
62225a7
to
50beb2a
Compare
When posting to a remote community, this post is immediately visible to users on the same instance as the post creator. However there are many reasons why the post may not get accepted by the community (eg user ban or rejected by plugin #5498). With this PR new posts and comments to remote communities are marked as pending, and only visible to the creator. Once the post/comment is federated back from the community to our instance, it is made public.
This approach can only be used if the remote community has at least one local subscriber. Otherwise the post wont be federated back to us, so we dont mark it as pending in the first place (this should only affect small instances/communities).
Previous discussion in #5415