-
-
Notifications
You must be signed in to change notification settings - Fork 941
Remove Children #6319
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
base: main
Are you sure you want to change the base?
Remove Children #6319
Conversation
| let report_alias = diesel::alias!(comment_report as cr); | ||
| let report_subquery = report_alias | ||
| .inner_join(comment::table.on(comment::id.eq(report_alias.field(comment_report::comment_id)))) | ||
| .filter(comment::path.contained_by(comment_path)); |
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.
Should check .starts_with instead of .contained_by (or same thing via .ilike). But it seems that none of these are available for LTree.
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.
No, this is right. contained_by runs the <@ operator that does what we want.
| build_post_response(&context, community.id, local_user_view, post_id).await | ||
| } | ||
|
|
||
| pub async fn remove_post_with_children( |
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.
Maybe subjective but I prefer to use with_replies everywhere.
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.
I've removed this function, but added back the remove_children fields. I can change this to with_replies, I think both make sense.
|
I deployed |
| }, | ||
| ) | ||
| .await?; | ||
| let orig_comment_id = orig_comment.comment.id; |
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.
We already have comment_id above.
| let updated_comment = updated_comments | ||
| .iter() | ||
| .find(|c| c.id == orig_comment_id) | ||
| .ok_or(LemmyErrorType::CouldntUpdate)?; |
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 avoids an extra API call / read so its fine.
| } else { | ||
| // Don't allow removing or restoring comment which was deleted by user, as it would reveal | ||
| // the comment text in mod log. | ||
| if orig_comment.comment.deleted { | ||
| return Err(LemmyErrorType::CouldntUpdate.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.
Why is this in an else block now? Then you'll be able to reveal the comment text in the modlog as long as delete children is true.
Probably just keep this top level.
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.
I figured it'd be useful to be able to remove the replies to a deleted comment.
| let form = ModlogInsertForm::mod_remove_comment( | ||
| local_user_view.person.id, | ||
| &orig_comment.comment, | ||
| removed, | ||
| &data.reason, | ||
| ); | ||
| let actions = Modlog::create(&mut context.pool(), &[form]).await?; |
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.
Then we aren't creating a bunch of rows for each of these? I notice lock also works like this so its probably fine. I guess that's #6323
| local_user_view: LocalUserView, | ||
| ) -> LemmyResult<Json<PostResponse>> { | ||
| let post_id = data.post_id; | ||
| let removed = data.remove_children.unwrap_or(data.removed); |
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.
Potentially dangerous, you can remove, see below comment.
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.
unwrap_or is safe, though? I do this here to enforce the aforementioned problem with federation, otherwise only the comments will get removed locally but on federated instances the post be removed as well.
| if data.remove_children.is_some() { | ||
| remove_or_restore_post_comments( | ||
| &post, | ||
| local_user_view.person.id, | ||
| removed, | ||
| &data.reason, | ||
| &context, | ||
| ) | ||
| .await?; |
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.
Use the if let Some(remove_children) = data.remove_children, its much safer.
crates/api/api_utils/src/utils.rs
Outdated
| let removed_comments: Vec<Comment> = | ||
| Comment::update_removed_for_comment_and_children(&mut context.pool(), &comment.path, removed) | ||
| .await? | ||
| // Filter out deleted comments here so their content doesn't show up in the modlog. |
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.
👍
crates/api/api_utils/src/utils.rs
Outdated
| .filter(|c| !c.deleted) | ||
| .collect(); | ||
|
|
||
| let actions = create_modlog_entries_for_removed_or_restored_comments( |
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 is really confusing to have the modlog entries being written at two levels, both in this function, and in the top level api action. In the API action it seemed like none of these were being written.
Another problem: I think this will probably also dupe a mod remove entry for the original comment
Since this function is only ever called from the api action, how about just copy-pasting its internals to inside the if let Some(remove_children) block. Then we'll clearly see everything that's being done.
If you look at how you did crates/api/api/src/comment/lock.rs, its much cleaner: no duped actions, everything occurs at the same level.
Its probably better to group the actions by their type also. Do all the modlog inserts in one place, all the comment updates in one place, etc. BC right now its confusing to trace all this down.
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.
Since this function is only ever called from the api action
This function is also called from the APub side, as I wanted to avoid duping a bunch of code. It's ~22 lines and causing more confusion than I thought it would, so I'll just copy them everywhere.
Another problem: I think this will probably also dupe a mod remove entry for the original comment
The modlog entry for a single comment only gets written if remove_children is None.
Its probably better to group the actions by their type also. Do all the modlog inserts in one place, all the comment updates in one place, etc. BC right now its confusing to trace all this down.
I don't know how I'd do that. We're dealing with two data types here, Comment and Vec<Comment>, and mixing data types in rust requires a bunch of boilerplate.
crates/api/api_utils/src/utils.rs
Outdated
| .filter(|c| !c.deleted) | ||
| .collect(); | ||
|
|
||
| let actions = create_modlog_entries_for_removed_or_restored_comments( |
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 is really confusing to have the modlog entries being written at two levels, both in this function, and in the top level api action. In the API action it seemed like none of these were being written.
Another problem: I think this will probably also dupe a mod remove entry for the original comment
Since this function is only ever called from the api action, how about just copy-pasting its internals to inside the if let Some(remove_children) block. Then we'll clearly see everything that's being done.
If you look at how you did crates/api/api/src/comment/lock.rs, its much cleaner: no duped actions, everything occurs at the same level.
I'd also move the modlog inserts to right next to the existing one at the bottom of the API action.
Fixes #3841
Depends on LemmyNet/lemmy-js-client#844
This adds new API endpoints
post/remove_with_childrenandcomment/remove_with_children. I originally had this be an optionalremove_childrenon the plain remove endpoints, but I couldn't figure out a good way to represent only removing the replies to something on the ActivityPub side. This made the option confusing as it overrode the actualremove, so instead I moved this to its own endpoint so it's not as confusing for client developers.