#36 Let community announce posts

Merged
nutomic merged 1 commits from announce-posts into federation 8 months ago
nutomic commented 9 months ago
Owner

Not finished yet, and the tests are failing.

Not finished yet, and the tests are failing.
nutomic reviewed 9 months ago
server/src/apub/community.rs Outdated
"".to_string()
}
})
.map(|c| get_shared_inbox(&c.user_actor_id))
nutomic commented 9 months ago

Honestly I have no clue what your code is doing, but I dont think its needed.

Honestly I have no clue what your code is doing, but I dont think its needed.
nutomic changed title from Let community announce posts to WIP: Let community announce posts 9 months ago
nutomic commented 9 months ago
Poster
Owner

Need to change this so that comments, votes and undos are federated (probably by having the community announce all of them?).

Need to change this so that comments, votes and undos are federated (probably by having the community announce all of them?).
Poster
Owner

Yeah, everything in the receive needs to be "forwarded" / announced to the other correct servers.

To test this correctly, we should also make sure the federation-test adds a third server.

Yeah, everything in the receive needs to be "forwarded" / announced to the other correct servers. To test this correctly, we should also make sure the federation-test adds a third server.
nutomic reviewed 9 months ago
server/src/db/code_migrations.rs Outdated
for cpost in &incorrect_posts {
Post::update_ap_id(&conn, cpost.id)?;
Post::update_ap_id(&conn, cpost.id, todo!())?;
nutomic commented 9 months ago

Dont forget to implement this properly.

Dont forget to implement this properly.
nutomic commented 9 months ago
Poster
Owner

I'm not sure where exactly MentionsAndAddresses::inboxes was used before, but I'm getting warnings now that its unused. Any advice how to integrate it again?

Edit: I think I got it, will add another commit.

Edit 2: Done, in the remote case comment notifications will be handled just like before, but I'm not sure if the local case is handled correctly.

I'm not sure where exactly `MentionsAndAddresses::inboxes` was used before, but I'm getting warnings now that its unused. Any advice how to integrate it again? Edit: I think I got it, will add another commit. Edit 2: Done, in the remote case comment notifications will be handled just like before, but I'm not sure if the local case is handled correctly.
nutomic changed title from WIP: Let community announce posts to Let community announce posts 9 months ago
nutomic reviewed 9 months ago
Ok(Activity::create(&conn, &activity_form)?)
debug!("inserting activity for user {}, data {:?}", user_id, data);
// TODO: this is broken
//Activity::create(&conn, &activity_form)?;
nutomic commented 9 months ago

As I wrote in the chat, this is breaking with errors like Key (user_id)=(-1) is not present in table "user_". We should probably add foreign keys user_id and community_id and require one of them to be correct. Or instead a column like actor_id?

As I wrote in the chat, this is breaking with errors like `Key (user_id)=(-1) is not present in table "user_"`. We should probably add foreign keys user_id and community_id and require one of them to be correct. Or instead a column like `actor_id`?
dessalines commented 9 months ago

I'll read through again, the user_id shouldn't ever be -1, but should be the local DB's id for that user.

I'll read through again, the user_id shouldn't ever be -1, but should be the local DB's id for that user.
nutomic commented 9 months ago

That doesnt work if the action is taken by the community (eg announce).

That doesnt work if the action is taken by the community (eg announce).
dessalines commented 9 months ago

One level inside announce tho, the activity will have the creator / user that did the action.

One level inside announce tho, the activity will have the creator / user that did the action.
nutomic commented 9 months ago

Okay I went with community.creator_id, which is the same thing we use for the accept follow activity. Still seems wrong.

Okay I went with community.creator_id, which is the same thing we use for the accept follow activity. Still seems wrong.
Poster
Owner

Oh also, the federation tests did pass, but we really didn't test out 3 instance thing which this really requires. I can write those tests tho, probably will be able to get to it tmrw.

Oh also, the federation tests did pass, but we really didn't test out 3 instance thing which this really requires. I can write those tests tho, probably will be able to get to it tmrw.
dessalines reviewed 9 months ago
server/src/apub/comment.rs Outdated
send_activity(&activity, creator, to)?;
}
Ok(())
}
dessalines commented 9 months ago

This is the same as what's in the impl Post, and since it doesn't use self, you could move this up to mod.rs

This is the same as what's in the `impl Post`, and since it doesn't use `self`, you could move this up to `mod.rs`
nutomic commented 9 months ago

Right, good catch. I moved it to activities::send_activity_to_community().

Right, good catch. I moved it to activities::send_activity_to_community().
dessalines reviewed 9 months ago
where
A: Activity + Base + Serialize + Debug,
{
insert_activity(&conn, -1, &activity, is_local_activity)?;
dessalines commented 9 months ago

There's a user_id that's doing the activity, so that needs to get passed through. Instead of sender: &str, use ``creator: &User_`, then you'll have both the actor_id, and the user_id.

There's a user_id that's doing the activity, so that needs to get passed through. Instead of `sender: &str`, use ``creator: &User_`, then you'll have both the actor_id, and the user_id.
dessalines commented 9 months ago

Also just to add, part of my thinking in making this a necessary column in the activity table, is to ensure that federated users also have local ids.

Also just to add, part of my thinking in making this a necessary column in the activity table, is to ensure that federated users also have local ids.
dessalines reviewed 9 months ago
dessalines reviewed 9 months ago
server/src/apub/community.rs Outdated
// dont send to the instance where the activity originally came from, because that would result
// in a database error (same data inserted twice)
let mut to = community.get_follower_inboxes(&conn)?;
let sending_user = get_or_fetch_and_upsert_remote_user(&sender, conn)?;
dessalines commented 9 months ago

Now you won't have to do this, because you already have the sending user, which already implements ActorType and you can get the shared_inbox that way.

Now you won't have to do this, because you already have the sending user, which already implements `ActorType` and you can get the shared_inbox that way.
nutomic commented 9 months ago

Right, done. There is a similar issue in shared_inbox but that is trickier to fix (I left a todo).

Right, done. There is a similar issue in shared_inbox but that is trickier to fix (I left a todo).
dessalines reviewed 9 months ago
.set_actor_xsd_any_uri(community.actor_id.to_owned())?
.set_object_base_box(BaseBox::from_concrete(activity)?)?;
insert_activity(&conn, -1, &announce, true)?;
dessalines commented 9 months ago

Thinking about it, this is correct, it does make sense to insert both the activity, and the announce.

Thinking about it, this is correct, it does make sense to insert both the activity, and the announce.
dessalines reviewed 9 months ago
return Err(format_err!(
"Received activity is addressed to remote community {}",
&community.actor_id
));
dessalines commented 9 months ago

Good call, I didn't think about this.

Good call, I didn't think about this.
dessalines reviewed 9 months ago
.unwrap()
.next()
.unwrap()
.to_string()
dessalines commented 9 months ago

I have to think about this one. The first item of the array is community/followers, but the other ones might be mentions and such.

I'm pretty sure in my code I just scan the comment text again anyway to handle mentions, but it would be good to make sure this returns the full array.

I have to think about this one. The first item of the array is `community/followers`, but the other ones might be mentions and such. I'm pretty sure in my code I just scan the comment text again anyway to handle mentions, but it would be good to make sure this returns the full array.
dessalines reviewed 9 months ago
// TODO: there is probably an easier way to do this
let oprops = match self {
SharedAcceptedObjects::Create(c) => &c.object_props,
SharedAcceptedObjects::Update(u) => &u.object_props,
dessalines commented 9 months ago

There's gotta be a cleaner way to do all of these, I'll play around a bit.

There's gotta be a cleaner way to do all of these, I'll play around a bit.
nutomic commented 9 months ago

I hope the new apub library will make it unnecessary.

I hope the new apub library will make it unnecessary.
dessalines reviewed 9 months ago
server/src/apub/shared_inbox.rs Outdated
let c = get_or_fetch_and_upsert_remote_community(&sender.to_string(), &conn)?;
verify(&request, &c)
}
}?;
dessalines commented 9 months ago

I can't think of a good cross-compatible way to do this either. We can't use urls because other future group-implementers will probably not use /c/...

I can't think of a good cross-compatible way to do this either. We can't use urls because other future group-implementers will probably not use `/c/`...
nutomic commented 9 months ago

We could move the check into the match because announce is the only activity that comes from a community. But then we probably have to duplicate the verify a lot more times. Lets see.

We could move the check into the match because announce is the only activity that comes from a community. But then we probably have to duplicate the verify a lot more times. Lets see.
dessalines reviewed 9 months ago
server/src/apub/shared_inbox.rs Outdated
if !community.local {
// ignore this object
}
Community::do_announce(activity, &community, sender, conn, false)
dessalines commented 9 months ago

Shouldn't this go in if community.local {

Shouldn't this go in `if community.local {`
nutomic commented 9 months ago

Right, I think I forgot to finish this.

Right, I think I forgot to finish this.
dessalines reviewed 9 months ago
.get_object_base_box()
.unwrap()
.to_owned();
// TODO: too much copy paste
dessalines commented 9 months ago

I'll have to play around and see if there's a cleaner way to do it too, but I doubt there is.

I'll have to play around and see if there's a cleaner way to do it too, but I doubt there is.
nutomic commented 9 months ago

I guess we can leave the refactoring until after we switch to the new library version.

I guess we can leave the refactoring until after we switch to the new library version.
dessalines commented 9 months ago

Good call.

Good call.
nutomic closed this pull request 8 months ago
The pull request has been merged as 1aa30d855e.
Sign in to join this conversation.
No reviewers
No Label
No Milestone
No Assignees
2 Participants
Notifications
Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Loading…
There is no content yet.