-
-
Notifications
You must be signed in to change notification settings - Fork 905
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
Filter_removed_comments_from_search #4634
Filter_removed_comments_from_search #4634
Conversation
Should deleted comments also be filtered out? |
This is the wrong place to change it as its also used to get the comments for a post among other things. Its necessary to show removed comments there so they can be restored. As its only about search you should filter them in search.rs instead. Or possibly add a parameter |
This reverts commit c6d6490.
@Nutomic got your point. Added filter for removed comments in search.rs . Deleted comments are still shown in search results, should they remain or not? |
It should be fine to exclude them as well. |
crates/apub/src/api/search.rs
Outdated
let comments = comments | ||
.into_iter() | ||
.filter(|c| !c.comment.removed && !c.comment.deleted) | ||
.collect(); |
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 isn't the right way to handle it either, filtering needs to be in SQL.
In the comment query logic, you can check for search_term
(since if that's provided, you know its a search), and filter out deleted and removed based on that.
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.
In this case, we can do it this way:
in comment_veiw
we can add a new option to CommentQuery
sctruct
like
pub struct CommentQuery<'a> {
...
pub filter_removed_and_deleted: bool,
...
}
and in fn queries
we can add a corresponding check if filter
if options.filter_removed_and_deleted {
query = query.filter(comment::deleted.eq(false).and(comment::removed.eq(false)));
}
and in search.rs
we just need to add this option to filter out deleted and removed comments
comments = CommentQuery {
sort: (sort.map(post_to_comment_sort_type)),
listing_type: (listing_type),
search_term: (Some(q)),
filter_removed_and_deleted: (false), // < - adding option to filter deleted and removed comments
community_id: (community_id),
creator_id: (creator_id),
local_user: (local_user_view.as_ref()),
page: (page),
limit: (limit),
..Default::default()
}
.list(&mut context.pool())
.await?;
@dessalines what do you think about 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.
First try to add that here, into the existing if let Some(search)
.
If that fails tho, your approach looks good also.
As proposed in #4576