Display deleted or removed comments when they have children #3965
Conversation
SleeplessOne1917
requested review from
Nutomic,
dessalines and
phiresky
as code owners
crates/db_views/src/comment_view.rs
Outdated
| // only show deleted comments to creator, or if they have children | ||
| if let Some(local_user) = options.local_user { | ||
| query = query.filter( | ||
| comment::deleted | ||
| .eq(false) | ||
| .or(comment::creator_id.eq(local_user.person.id)) | ||
| .or(comment_aggregates::child_count.gt(0)), | ||
| ); | ||
| } else { | ||
| query = query.filter( | ||
| comment::deleted | ||
| .eq(false) | ||
| .or(comment_aggregates::child_count.gt(0)), | ||
| ); |
There was a problem hiding this comment.
The old logic for this only showed deleted comments to the creator of the comment when creator ID was specified. This mean the only way to see them is if you use the search page and filter by creator, or if you are viewing your own profile. It also had the likely unintentional side effect of showing deleted comments to unauthenticated users. For example:
There was a problem hiding this comment.
Can you add a test case to make sure this wont get broken in the future again? Either a Rust test in the same file, or a Typescript test under api_tests folder.
There was a problem hiding this comment.
There are 2 blocks that check for deleted and removed in this file. IMO those blocks should be entirely removed... its too difficult and pointless to try to remove data in trees, and still have the rest of the tree render. I'm not sure why I would have approved adding those for comment_views. (post_views are a different story since they're flat)
It should be left to front ends to make sure to render deleted and removed (IE show that they're deleted / removed, and don't show the content).
There was a problem hiding this comment.
It looks like the logic was originally added around a year ago.
crates/db_views/src/comment_view.rs
Outdated
| @@ -202,7 +212,11 @@ fn queries<'a>() -> Queries< | |||
| .unwrap_or(false); | |||
| // only show removed comments to admin when viewing user profile | |||
| if !(options.is_profile_view && is_admin) { | |||
There was a problem hiding this comment.
I'm wondering if it makes sense to only show removed comments to an admin if they're looking at the profile view. I would imagine it would be useful to for an admin to see the comments in the context of the post they were removed from.
In addition, I believe this logic still hides deleted comments from non-admin moderators.
There was a problem hiding this comment.
Thats true. Problem is that its very annoying when you browse the site and see offensive, removed comments everywhere. I guess ideally removed comments would be returned by the API, but the ui would hide them by default, with a button "click to view".
The profile view doesnt belong to any community, so theres no good way to show it to mods there (unless you show it to anyone who moderates any community).
There was a problem hiding this comment.
Agree, this should be left up to the UIs, and we shouldn't be messing with comment trees.
There was a problem hiding this comment.
@dessalines Just to make sure we're on the same page, you don't want to filter deleted or removed comments at all on the backend and leave that decision of how to display them up to the UI?
Edit: Should UIs also handle filtering comments from blocked users or instances, bot accounts, or posts in languages that a user doesn't want to see?
There was a problem hiding this comment.
Correct. At least in the case of deleted and removed, that info is directly on the CommentView, so UI's can just choose how they want to render it, while still not messing with the rest of the tree.
The blocked communities / persons/ instances one tho... I'm honestly not sure. My gut reaction is that those should stay the way they are, even if it does wipe out branches of the tree ( in the case of person and instance blocks). But I'm open to either way.
There was a problem hiding this comment.
Please lets not add more complication with potentially removing branches of the trees from results. We already have ppl trying to add extremely complicated hacks into the tree-building code because of missing branches.
If anything, the correct way to do it would be to make sure all comments are returned (even blocked ones), but let the front ends read vars on the CommentView 's like deleted, removed, blocked, so they can choose to hide them in the front end, but still build the tree correctly.
There was a problem hiding this comment.
At this point, maybe we should ask for input from other devs. Ultimately it's going to be UI maintainers who are most affected by this.
There was a problem hiding this comment.
@MV-GH is working through a case rn, where they're having to "fake" missing comments in trees because of this issue. The solutions are not clean.
There was a problem hiding this comment.
Well, I do prefer that it would return something for blocked comments. So the UI can inform the user, that the reason they can't view this this comment because the creator is blocked. (And if they wish to see to comment anyway they could override it)
I do not think its possible for the server to insure that it always returns complete trees. And thus the clients will have to handle this edgecase regardless of these changes.
Changing it so that the server still fully returns deleted data (and thus keeps that data). Might have some bad implications, I doubt admins would still want to store and return illegal content. Think of CSAM, but this could be anything really that is declared illegal, and which could bring them into legal trouble. Or to conform to GDPR, where a user removes his comment it should be completely gone. To be in compliance with GDPR, see "The right to erasure" Articles 17 & 19 of GDPR. All though I am no legal expert.
Other cases might be, what happens to comments from deleted users. I am not sure what we currently do, but I think the current approach is to completely remove them?
Or what about the case, where the admins straight up delete entries/comments from within the DB. (This happened during the CSAM scare)
The solution, I have now is not ideal and a bit more complex. But I do think is necessary. Maybe the server can return these "fake" missing comments to offload the complexity from the clients. But I rather not if this has a performance impact.
There was a problem hiding this comment.
Purged content (as in DB deleted), shouldn't be an issue, because a purge of a comment will wipe out all its children (IE the entire tree branch).
Other cases might be, what happens to comments from deleted users. I am not sure what we currently do, but I think the current approach is to completely remove them?
The deleted flag should get updated on those CommentViews, and also overwritten (either after a month, or if they've deleted their account, immediately). So the tree is intact in that case.
| @@ -190,21 +190,6 @@ fn queries<'a>() -> Queries< | |||
| query = query.filter(comment_like::score.eq(-1)); | |||
| } | |||
|
|
|||
| let is_creator = options.creator_id == options.local_user.map(|l| l.person.id); | |||
There was a problem hiding this comment.
Good. I'm sure these just got copy-pasted from the post_view ones, where it does make sense to have them.
|
Hey I just wanted to double back here (now that a lot of instances are on 0.19) and thank you @SleeplessOne1917 for this change! Without any changes on our part in Thunder, it "works better" now that comment trees with deleted comments can be fully rendered with placeholders. Thanks again! |
Currently if a comment is deleted or removed the entire thread under that comment will be inaccessible. Take this comment for example. The child comment of this comment was removed by a mod, but that child also had children. If you try clicking on the button to show children, you will get an endless loading spinner. You can still get to the existing child thread if you have a direct link (like so), but there is no way to get there from the top level comment. Also note in the child thread that clicking on "Show context" does nothing.
I originally thought this was a frontend issue see this, however investigation revelead this requires a backend change.
I also took the liberty of updating the translation submodule and making the dev docker compose file use the latest lemmy ui docker image.