LemmyNet
/
lemmy
3
2
Fork 2
Code Pull Requests 2 Releases 151 Wiki Activity
Labels Milestones
New Pull Request

#33 Implement deleting communities

Closed
nutomic wants to merge 0 commits from delete-community into federation
Conversation 2 Commits N/A Files Changed 173
nutomic commented 10 months ago
Owner

I'm not sure how this should deal with removed/deleted, so I'm just treating both in the same way.

I'm not sure how this should deal with removed/deleted, so I'm just treating both in the same way.
nutomic commented 10 months ago
Poster
Owner

Okay this is not entirely correct, the tombstone should be served with http status 410. And there is also a Remove activity that we could use, but I would just go with one of them for now to keep things simpler.

Okay this is not entirely correct, the tombstone should be served with http status 410. And there is also a `Remove` activity that we could use, but I would just go with one of them for now to keep things simpler.
dessalines requested changes 10 months ago
dessalines left a comment

rev

dessalines commented 10 months ago
Poster
Owner

Oops, still figuring out reviews.

Oops, still figuring out reviews.
dessalines requested changes 10 months ago
dessalines left a comment

Review

server/src/api/community.rs
expires,
};
ModRemoveCommunity::create(&conn, &form)?;
updated_community.send_delete(&conn)?;
dessalines commented 10 months ago

Ya this should be a send_remove. Just remove this line for now, lets leave all mod actions for later, those will get complicated.

Ya this should be a `send_remove`. Just remove this line for now, lets leave all mod actions for later, those will get complicated.
nutomic commented 10 months ago

So you want to use only Remove activity for now and not Delete? I'm not entirely sure how to handle that, the spec mentions nothing about a Tombstone for Remove.

So you want to use only `Remove` activity for now and not `Delete`? I'm not entirely sure how to handle that, the spec mentions nothing about a Tombstone for Remove.
nutomic commented 10 months ago

And I think we can just use the same activity (whether that is Delete or Remove) for both actions for now. Later we can add the different functionality for each.

And I think we can just use the same activity (whether that is Delete or Remove) for both actions for now. Later we can add the different functionality for each.
server/src/api/community.rs Outdated
}
if let Some(_deleted) = data.deleted.to_owned() {
updated_community.send_delete(&conn)?;
dessalines commented 10 months ago

No reason to use if let here unless you're using the variable it extracts. Just do

if data.deleted {...

No reason to use if let here unless you're using the variable it extracts. Just do `if data.deleted {...`
nutomic commented 10 months ago

Error: expected bool, found enum std::option::Option``

Error: `expected `bool`, found enum `std::option::Option``
nutomic commented 10 months ago

But your right the code is wrong, it doesnt even check the value of deleted. But I dont know how to do that without using a match or a second if statement.

But your right the code is wrong, it doesnt even check the value of `deleted`. But I dont know how to do that without using a match or a second if statement.
dessalines commented 10 months ago

Sry, data.deleted.is_some()

Sry, data.deleted.is_some()
nutomic commented 10 months ago

That doesnt check whether the boolean is true either...

That doesnt check whether the boolean is true either...
server/src/apub/comment.rs Outdated
type Response = Note;
fn to_apub(&self, conn: &PgConnection) -> Result<Note, Error> {
fn to_apub(&self, conn: &PgConnection) -> Result<ResponseOrTombstone<Note>, Error> {
dessalines commented 10 months ago

Hrm, I don't like this.

Two ideas:

Just add a to_apub_tombstone to that trait, that returns Result<Tombstone, Error>. Then you can still use the associated responsetype trait.

Or, maybe since the return type can either be multiple types, either the thing, or a tombstone of the thing, maybe the ToApub trait should be made generic.

pub trait ToApub<Response>{
  fn to_apub(&self, conn: &PgConnection) -> Result<Response, Error>;
}

And implement it for both the tombstone and the regular object.

Hrm, I don't like this. Two ideas: Just add a `to_apub_tombstone` to that trait, that returns `Result<Tombstone, Error>`. Then you can still use the associated responsetype trait. Or, maybe since the return type can either be multiple types, either the thing, or a tombstone of the thing, maybe the ToApub trait should be made generic. ``` pub trait ToApub<Response>{ fn to_apub(&self, conn: &PgConnection) -> Result<Response, Error>; } ``` And implement it for both the tombstone and the regular object.
nutomic commented 10 months ago

Okay I will go with a seperate ToTombstone trait, that seems easiest. Dont think the ToApub would work because we need to return tombstones for a lot of different object types.

Okay I will go with a seperate ToTombstone trait, that seems easiest. Dont think the ToApub<Response> would work because we need to return tombstones for a lot of different object types.
server/src/apub/mod.rs Outdated
}
}
}
dessalines commented 10 months ago

I don't really like using enums as types, and doing switching like this. Use one of the suggestions above to have to_apub return different / generic types.

I don't really like using enums as types, and doing switching like this. Use one of the suggestions above to have `to_apub` return different / generic types.
server/src/apub/mod.rs Outdated
pub trait ApubObjectType {
fn send_create(&self, creator: &User_, conn: &PgConnection) -> Result<(), Error>;
fn send_update(&self, creator: &User_, conn: &PgConnection) -> Result<(), Error>;
//fn send_delete(&self, creator: &User_, conn: &PgConnection) -> Result<(), Error>;
dessalines commented 10 months ago

This should be uncommented, and maybe added to the ActorType trait too.

This should be uncommented, and maybe added to the `ActorType` trait too.
nutomic commented 10 months ago

I have it in the ActorType already. Will probably handle posts etc seperately.

I have it in the ActorType already. Will probably handle posts etc seperately.
nutomic commented 10 months ago

Okay guess I will handle posts etc in this PR as well because there isnt much else to do.

Okay guess I will handle posts etc in this PR as well because there isnt much else to do.
server/src/apub/mod.rs Outdated
Err(format_err!("Accept not implemented."))
}
fn send_delete(&self, conn: &PgConnection) -> Result<(), Error>;
dessalines commented 10 months ago

Ya move this into the trait functions.

Ya move this into the trait functions.
nutomic commented 10 months ago

Not sure what you mean.

Not sure what you mean.
dessalines commented 10 months ago

Basically just removing that line and uncommenting:

pub trait ApubObjectType {
  fn send_create(&self, creator: &User_, conn: &PgConnection) -> Result<(), Error>;
  fn send_update(&self, creator: &User_, conn: &PgConnection) -> Result<(), Error>;
  ---->  //fn send_delete(&self, creator: &User_, conn: &PgConnection) -> Result<(), Error>;
}

So apub.post.rs, comment.rs has a send_delete also.

Basically just removing that line and uncommenting: ``` pub trait ApubObjectType { fn send_create(&self, creator: &User_, conn: &PgConnection) -> Result<(), Error>; fn send_update(&self, creator: &User_, conn: &PgConnection) -> Result<(), Error>; ----> //fn send_delete(&self, creator: &User_, conn: &PgConnection) -> Result<(), Error>; } ``` So apub.post.rs, comment.rs has a send_delete also.
server/src/apub/shared_inbox.rs
removed: None,
published: None,
updated: None,
deleted: Some(true),
dessalines commented 10 months ago

It seems weird to do all this tombstone work to just set it like this, but I had to do the same thing with likes and dislikes.

It seems weird to do all this tombstone work to just set it like this, but I had to do the same thing with likes and dislikes.
nutomic commented 10 months ago

How else could we possibly do it? I guess ideally this would be handled by the API/wrapper, and we just call remove(community), but for now we dont have much of a choice.

How else could we possibly do it? I guess ideally this would be handled by the API/wrapper, and we just call `remove(community)`, but for now we dont have much of a choice.
dessalines commented 10 months ago

You're right, I can't think of another way either, its unavoidable.

You're right, I can't think of another way either, its unavoidable.
nutomic reviewed 10 months ago
server/src/apub/shared_inbox.rs Outdated
}
(SharedAcceptedObjects::Delete(d), Some("Tombstone")) => {
// TODO: is this deleting a community, post, comment or what?
receive_delete_community(&d, &request, &conn, chat_server)
nutomic commented 10 months ago

Thoughts on this?

Thoughts on this?
nutomic commented 10 months ago

Really I dont think it makes any sense to send the tombstone here, we should send an empty group/page/note.

Really I dont think it makes any sense to send the tombstone here, we should send an empty group/page/note.
dessalines commented 10 months ago

https://www.w3.org/TR/activitystreams-vocabulary/#dfn-formertype

It looks like we can set a formertype on the tombstone object and switch on that.

https://www.w3.org/TR/activitystreams-vocabulary/#dfn-formertype It looks like we can set a formertype on the tombstone object and switch on that.
nutomic closed this pull request 10 months ago
This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
Request review
No reviewers
Labels
Apply labels
Clear labels
No items
No Label
Milestone
Set milestone
Clear milestone
No items
No Milestone
Assignees
Assign users
Clear assignees
No Assignees
2 Participants
Notifications
Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Write Preview
Loading…
Cancel
Save
There is no content yet.
Delete Branch 'delete-community'

Deleting a branch is permanent. It CANNOT be undone. Continue?

No
Yes