Prevent bot replies from increasing unread reply count when bot accounts are not shown (#4747)

* Prevent bot replies from increasing unread reply count when bot accounts are not shown

* Pass LocalUser for unread replies count query

* Prevent bot mentions from increasing unread reply count when bot accounts are not shown
This commit is contained in:
Richard Schwab 2024-05-29 23:55:15 +02:00 committed by GitHub
parent 7d80a3c7d6
commit 91e57ff954
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
7 changed files with 189 additions and 25 deletions

1
Cargo.lock generated
View file

@ -2946,6 +2946,7 @@ dependencies = [
"diesel", "diesel",
"diesel-async", "diesel-async",
"lemmy_db_schema", "lemmy_db_schema",
"lemmy_db_views",
"lemmy_utils", "lemmy_utils",
"pretty_assertions", "pretty_assertions",
"serde", "serde",

View file

@ -37,8 +37,9 @@ import {
followCommunity, followCommunity,
blockCommunity, blockCommunity,
delay, delay,
saveUserSettings,
} from "./shared"; } from "./shared";
import { CommentView, CommunityView } from "lemmy-js-client"; import { CommentView, CommunityView, SaveUserSettings } from "lemmy-js-client";
let betaCommunity: CommunityView | undefined; let betaCommunity: CommunityView | undefined;
let postOnAlphaRes: PostResponse; let postOnAlphaRes: PostResponse;
@ -443,6 +444,59 @@ test("Reply to a comment from another instance, get notification", async () => {
assertCommentFederation(alphaReply, replyRes.comment_view); assertCommentFederation(alphaReply, replyRes.comment_view);
}); });
test("Bot reply notifications are filtered when bots are hidden", async () => {
const newAlphaBot = await registerUser(alpha, alphaUrl);
let form: SaveUserSettings = {
bot_account: true,
};
await saveUserSettings(newAlphaBot, form);
const alphaCommunity = (
await resolveCommunity(alpha, "!main@lemmy-alpha:8541")
).community;
if (!alphaCommunity) {
throw "Missing alpha community";
}
await alpha.markAllAsRead();
form = {
show_bot_accounts: false,
};
await saveUserSettings(alpha, form);
const postOnAlphaRes = await createPost(alpha, alphaCommunity.community.id);
// Bot reply to alpha's post
let commentRes = await createComment(
newAlphaBot,
postOnAlphaRes.post_view.post.id,
);
expect(commentRes).toBeDefined();
let alphaUnreadCountRes = await getUnreadCount(alpha);
expect(alphaUnreadCountRes.replies).toBe(0);
let alphaUnreadRepliesRes = await getReplies(alpha, true);
expect(alphaUnreadRepliesRes.replies.length).toBe(0);
// This both restores the original state that may be expected by other tests
// implicitly and is used by the next steps to ensure replies are still
// returned when a user later decides to show bot accounts again.
form = {
show_bot_accounts: true,
};
await saveUserSettings(alpha, form);
alphaUnreadCountRes = await getUnreadCount(alpha);
expect(alphaUnreadCountRes.replies).toBe(1);
alphaUnreadRepliesRes = await getReplies(alpha, true);
expect(alphaUnreadRepliesRes.replies.length).toBe(1);
expect(alphaUnreadRepliesRes.replies[0].comment.id).toBe(
commentRes.comment_view.comment.id,
);
});
test("Mention beta from alpha", async () => { test("Mention beta from alpha", async () => {
if (!betaCommunity) throw Error("no community"); if (!betaCommunity) throw Error("no community");
const postOnAlphaRes = await createPost(alpha, betaCommunity.community.id); const postOnAlphaRes = await createPost(alpha, betaCommunity.community.id);

View file

@ -364,10 +364,13 @@ export async function getUnreadCount(
return api.getUnreadCount(); return api.getUnreadCount();
} }
export async function getReplies(api: LemmyHttp): Promise<GetRepliesResponse> { export async function getReplies(
api: LemmyHttp,
unread_only: boolean = false,
): Promise<GetRepliesResponse> {
let form: GetReplies = { let form: GetReplies = {
sort: "New", sort: "New",
unread_only: false, unread_only,
}; };
return api.getReplies(form); return api.getReplies(form);
} }

View file

@ -11,9 +11,12 @@ pub async fn unread_count(
) -> LemmyResult<Json<GetUnreadCountResponse>> { ) -> LemmyResult<Json<GetUnreadCountResponse>> {
let person_id = local_user_view.person.id; let person_id = local_user_view.person.id;
let replies = CommentReplyView::get_unread_replies(&mut context.pool(), person_id).await?; let replies =
CommentReplyView::get_unread_replies(&mut context.pool(), &local_user_view.local_user).await?;
let mentions = PersonMentionView::get_unread_mentions(&mut context.pool(), person_id).await?; let mentions =
PersonMentionView::get_unread_mentions(&mut context.pool(), &local_user_view.local_user)
.await?;
let private_messages = let private_messages =
PrivateMessageView::get_unread_messages(&mut context.pool(), person_id).await?; PrivateMessageView::get_unread_messages(&mut context.pool(), person_id).await?;

View file

@ -40,6 +40,7 @@ serial_test = { workspace = true }
tokio = { workspace = true } tokio = { workspace = true }
pretty_assertions = { workspace = true } pretty_assertions = { workspace = true }
url.workspace = true url.workspace = true
lemmy_db_views.workspace = true
lemmy_utils.workspace = true lemmy_utils.workspace = true
[package.metadata.cargo-machete] [package.metadata.cargo-machete]

View file

@ -31,6 +31,7 @@ use lemmy_db_schema::{
person_block, person_block,
post, post,
}, },
source::local_user::LocalUser,
utils::{get_conn, limit_and_offset, DbConn, DbPool, ListFn, Queries, ReadFn}, utils::{get_conn, limit_and_offset, DbConn, DbPool, ListFn, Queries, ReadFn},
CommentSortType, CommentSortType,
}; };
@ -193,6 +194,8 @@ fn queries<'a>() -> Queries<
}; };
let list = move |mut conn: DbConn<'a>, options: CommentReplyQuery| async move { let list = move |mut conn: DbConn<'a>, options: CommentReplyQuery| async move {
// These filters need to be kept in sync with the filters in
// CommentReplyView::get_unread_replies()
let mut query = all_joins(comment_reply::table.into_boxed(), options.my_person_id); let mut query = all_joins(comment_reply::table.into_boxed(), options.my_person_id);
if let Some(recipient_id) = options.recipient_id { if let Some(recipient_id) = options.recipient_id {
@ -204,7 +207,7 @@ fn queries<'a>() -> Queries<
} }
if !options.show_bot_accounts { if !options.show_bot_accounts {
query = query.filter(person::bot_account.eq(false)); query = query.filter(not(person::bot_account));
}; };
query = match options.sort.unwrap_or(CommentSortType::New) { query = match options.sort.unwrap_or(CommentSortType::New) {
@ -246,24 +249,33 @@ impl CommentReplyView {
/// Gets the number of unread replies /// Gets the number of unread replies
pub async fn get_unread_replies( pub async fn get_unread_replies(
pool: &mut DbPool<'_>, pool: &mut DbPool<'_>,
my_person_id: PersonId, local_user: &LocalUser,
) -> Result<i64, Error> { ) -> Result<i64, Error> {
use diesel::dsl::count; use diesel::dsl::count;
let conn = &mut get_conn(pool).await?; let conn = &mut get_conn(pool).await?;
comment_reply::table let mut query = comment_reply::table
.inner_join(comment::table) .inner_join(comment::table)
.left_join( .left_join(
person_block::table.on( person_block::table.on(
comment::creator_id comment::creator_id
.eq(person_block::target_id) .eq(person_block::target_id)
.and(person_block::person_id.eq(my_person_id)), .and(person_block::person_id.eq(local_user.person_id)),
), ),
) )
// Dont count replies from blocked users .inner_join(person::table.on(comment::creator_id.eq(person::id)))
.into_boxed();
// These filters need to be kept in sync with the filters in queries().list()
if !local_user.show_bot_accounts {
query = query.filter(not(person::bot_account));
}
query
// Don't count replies from blocked users
.filter(person_block::person_id.is_null()) .filter(person_block::person_id.is_null())
.filter(comment_reply::recipient_id.eq(my_person_id)) .filter(comment_reply::recipient_id.eq(local_user.person_id))
.filter(comment_reply::read.eq(false)) .filter(comment_reply::read.eq(false))
.filter(comment::deleted.eq(false)) .filter(comment::deleted.eq(false))
.filter(comment::removed.eq(false)) .filter(comment::removed.eq(false))
@ -301,13 +313,15 @@ mod tests {
comment_reply::{CommentReply, CommentReplyInsertForm, CommentReplyUpdateForm}, comment_reply::{CommentReply, CommentReplyInsertForm, CommentReplyUpdateForm},
community::{Community, CommunityInsertForm}, community::{Community, CommunityInsertForm},
instance::Instance, instance::Instance,
person::{Person, PersonInsertForm}, local_user::{LocalUser, LocalUserInsertForm, LocalUserUpdateForm},
person::{Person, PersonInsertForm, PersonUpdateForm},
person_block::{PersonBlock, PersonBlockForm}, person_block::{PersonBlock, PersonBlockForm},
post::{Post, PostInsertForm}, post::{Post, PostInsertForm},
}, },
traits::{Blockable, Crud}, traits::{Blockable, Crud},
utils::build_db_pool_for_tests, utils::build_db_pool_for_tests,
}; };
use lemmy_db_views::structs::LocalUserView;
use lemmy_utils::{error::LemmyResult, LemmyErrorType}; use lemmy_utils::{error::LemmyResult, LemmyErrorType};
use pretty_assertions::assert_eq; use pretty_assertions::assert_eq;
use serial_test::serial; use serial_test::serial;
@ -331,11 +345,15 @@ mod tests {
.name("terrylakes recipient".into()) .name("terrylakes recipient".into())
.public_key("pubkey".to_string()) .public_key("pubkey".to_string())
.instance_id(inserted_instance.id) .instance_id(inserted_instance.id)
.local(Some(true))
.build(); .build();
let inserted_recipient = Person::create(pool, &recipient_form).await?; let inserted_recipient = Person::create(pool, &recipient_form).await?;
let recipient_id = inserted_recipient.id; let recipient_id = inserted_recipient.id;
let recipient_local_user =
LocalUser::create(pool, &LocalUserInsertForm::test_form(recipient_id), vec![]).await?;
let new_community = CommunityInsertForm::builder() let new_community = CommunityInsertForm::builder()
.name("test community lake".to_string()) .name("test community lake".to_string())
.title("nada".to_owned()) .title("nada".to_owned())
@ -386,7 +404,7 @@ mod tests {
CommentReply::update(pool, inserted_reply.id, &comment_reply_update_form).await?; CommentReply::update(pool, inserted_reply.id, &comment_reply_update_form).await?;
// Test to make sure counts and blocks work correctly // Test to make sure counts and blocks work correctly
let unread_replies = CommentReplyView::get_unread_replies(pool, recipient_id).await?; let unread_replies = CommentReplyView::get_unread_replies(pool, &recipient_local_user).await?;
let query = CommentReplyQuery { let query = CommentReplyQuery {
recipient_id: Some(recipient_id), recipient_id: Some(recipient_id),
@ -409,11 +427,44 @@ mod tests {
PersonBlock::block(pool, &block_form).await?; PersonBlock::block(pool, &block_form).await?;
let unread_replies_after_block = let unread_replies_after_block =
CommentReplyView::get_unread_replies(pool, recipient_id).await?; CommentReplyView::get_unread_replies(pool, &recipient_local_user).await?;
let replies_after_block = query.list(pool).await?; let replies_after_block = query.clone().list(pool).await?;
assert_eq!(0, unread_replies_after_block); assert_eq!(0, unread_replies_after_block);
assert_eq!(0, replies_after_block.len()); assert_eq!(0, replies_after_block.len());
// Unblock user so we can reuse the same person
PersonBlock::unblock(pool, &block_form).await?;
// Turn Terry into a bot account
let person_update_form = PersonUpdateForm {
bot_account: Some(true),
..Default::default()
};
Person::update(pool, inserted_terry.id, &person_update_form).await?;
let recipient_local_user_update_form = LocalUserUpdateForm {
show_bot_accounts: Some(false),
..Default::default()
};
LocalUser::update(
pool,
recipient_local_user.id,
&recipient_local_user_update_form,
)
.await?;
let recipient_local_user_view = LocalUserView::read(pool, recipient_local_user.id)
.await?
.ok_or(LemmyErrorType::CouldntFindLocalUser)?;
let unread_replies_after_hide_bots =
CommentReplyView::get_unread_replies(pool, &recipient_local_user_view.local_user).await?;
let mut query_without_bots = query.clone();
query_without_bots.show_bot_accounts = false;
let replies_after_hide_bots = query_without_bots.list(pool).await?;
assert_eq!(0, unread_replies_after_hide_bots);
assert_eq!(0, replies_after_hide_bots.len());
Comment::delete(pool, inserted_comment.id).await?; Comment::delete(pool, inserted_comment.id).await?;
Post::delete(pool, inserted_post.id).await?; Post::delete(pool, inserted_post.id).await?;
Community::delete(pool, inserted_community.id).await?; Community::delete(pool, inserted_community.id).await?;

View file

@ -31,6 +31,7 @@ use lemmy_db_schema::{
person_mention, person_mention,
post, post,
}, },
source::local_user::LocalUser,
utils::{get_conn, limit_and_offset, DbConn, DbPool, ListFn, Queries, ReadFn}, utils::{get_conn, limit_and_offset, DbConn, DbPool, ListFn, Queries, ReadFn},
CommentSortType, CommentSortType,
}; };
@ -192,6 +193,8 @@ fn queries<'a>() -> Queries<
}; };
let list = move |mut conn: DbConn<'a>, options: PersonMentionQuery| async move { let list = move |mut conn: DbConn<'a>, options: PersonMentionQuery| async move {
// These filters need to be kept in sync with the filters in
// PersonMentionView::get_unread_mentions()
let mut query = all_joins(person_mention::table.into_boxed(), options.my_person_id); let mut query = all_joins(person_mention::table.into_boxed(), options.my_person_id);
if let Some(recipient_id) = options.recipient_id { if let Some(recipient_id) = options.recipient_id {
@ -203,7 +206,7 @@ fn queries<'a>() -> Queries<
} }
if !options.show_bot_accounts { if !options.show_bot_accounts {
query = query.filter(person::bot_account.eq(false)); query = query.filter(not(person::bot_account));
}; };
query = match options.sort.unwrap_or(CommentSortType::Hot) { query = match options.sort.unwrap_or(CommentSortType::Hot) {
@ -247,23 +250,32 @@ impl PersonMentionView {
/// Gets the number of unread mentions /// Gets the number of unread mentions
pub async fn get_unread_mentions( pub async fn get_unread_mentions(
pool: &mut DbPool<'_>, pool: &mut DbPool<'_>,
my_person_id: PersonId, local_user: &LocalUser,
) -> Result<i64, Error> { ) -> Result<i64, Error> {
use diesel::dsl::count; use diesel::dsl::count;
let conn = &mut get_conn(pool).await?; let conn = &mut get_conn(pool).await?;
person_mention::table let mut query = person_mention::table
.inner_join(comment::table) .inner_join(comment::table)
.left_join( .left_join(
person_block::table.on( person_block::table.on(
comment::creator_id comment::creator_id
.eq(person_block::target_id) .eq(person_block::target_id)
.and(person_block::person_id.eq(my_person_id)), .and(person_block::person_id.eq(local_user.person_id)),
), ),
) )
// Dont count replies from blocked users .inner_join(person::table.on(comment::creator_id.eq(person::id)))
.into_boxed();
// These filters need to be kept in sync with the filters in queries().list()
if !local_user.show_bot_accounts {
query = query.filter(not(person::bot_account));
}
query
// Don't count replies from blocked users
.filter(person_block::person_id.is_null()) .filter(person_block::person_id.is_null())
.filter(person_mention::recipient_id.eq(my_person_id)) .filter(person_mention::recipient_id.eq(local_user.person_id))
.filter(person_mention::read.eq(false)) .filter(person_mention::read.eq(false))
.filter(comment::deleted.eq(false)) .filter(comment::deleted.eq(false))
.filter(comment::removed.eq(false)) .filter(comment::removed.eq(false))
@ -300,7 +312,8 @@ mod tests {
comment::{Comment, CommentInsertForm}, comment::{Comment, CommentInsertForm},
community::{Community, CommunityInsertForm}, community::{Community, CommunityInsertForm},
instance::Instance, instance::Instance,
person::{Person, PersonInsertForm}, local_user::{LocalUser, LocalUserInsertForm, LocalUserUpdateForm},
person::{Person, PersonInsertForm, PersonUpdateForm},
person_block::{PersonBlock, PersonBlockForm}, person_block::{PersonBlock, PersonBlockForm},
person_mention::{PersonMention, PersonMentionInsertForm, PersonMentionUpdateForm}, person_mention::{PersonMention, PersonMentionInsertForm, PersonMentionUpdateForm},
post::{Post, PostInsertForm}, post::{Post, PostInsertForm},
@ -308,6 +321,7 @@ mod tests {
traits::{Blockable, Crud}, traits::{Blockable, Crud},
utils::build_db_pool_for_tests, utils::build_db_pool_for_tests,
}; };
use lemmy_db_views::structs::LocalUserView;
use lemmy_utils::{error::LemmyResult, LemmyErrorType}; use lemmy_utils::{error::LemmyResult, LemmyErrorType};
use pretty_assertions::assert_eq; use pretty_assertions::assert_eq;
use serial_test::serial; use serial_test::serial;
@ -337,6 +351,9 @@ mod tests {
let inserted_recipient = Person::create(pool, &recipient_form).await?; let inserted_recipient = Person::create(pool, &recipient_form).await?;
let recipient_id = inserted_recipient.id; let recipient_id = inserted_recipient.id;
let recipient_local_user =
LocalUser::create(pool, &LocalUserInsertForm::test_form(recipient_id), vec![]).await?;
let new_community = CommunityInsertForm::builder() let new_community = CommunityInsertForm::builder()
.name("test community lake".to_string()) .name("test community lake".to_string())
.title("nada".to_owned()) .title("nada".to_owned())
@ -387,7 +404,8 @@ mod tests {
PersonMention::update(pool, inserted_mention.id, &person_mention_update_form).await?; PersonMention::update(pool, inserted_mention.id, &person_mention_update_form).await?;
// Test to make sure counts and blocks work correctly // Test to make sure counts and blocks work correctly
let unread_mentions = PersonMentionView::get_unread_mentions(pool, recipient_id).await?; let unread_mentions =
PersonMentionView::get_unread_mentions(pool, &recipient_local_user).await?;
let query = PersonMentionQuery { let query = PersonMentionQuery {
recipient_id: Some(recipient_id), recipient_id: Some(recipient_id),
@ -410,11 +428,44 @@ mod tests {
PersonBlock::block(pool, &block_form).await?; PersonBlock::block(pool, &block_form).await?;
let unread_mentions_after_block = let unread_mentions_after_block =
PersonMentionView::get_unread_mentions(pool, recipient_id).await?; PersonMentionView::get_unread_mentions(pool, &recipient_local_user).await?;
let mentions_after_block = query.list(pool).await?; let mentions_after_block = query.clone().list(pool).await?;
assert_eq!(0, unread_mentions_after_block); assert_eq!(0, unread_mentions_after_block);
assert_eq!(0, mentions_after_block.len()); assert_eq!(0, mentions_after_block.len());
// Unblock user so we can reuse the same person
PersonBlock::unblock(pool, &block_form).await?;
// Turn Terry into a bot account
let person_update_form = PersonUpdateForm {
bot_account: Some(true),
..Default::default()
};
Person::update(pool, inserted_person.id, &person_update_form).await?;
let recipient_local_user_update_form = LocalUserUpdateForm {
show_bot_accounts: Some(false),
..Default::default()
};
LocalUser::update(
pool,
recipient_local_user.id,
&recipient_local_user_update_form,
)
.await?;
let recipient_local_user_view = LocalUserView::read(pool, recipient_local_user.id)
.await?
.ok_or(LemmyErrorType::CouldntFindLocalUser)?;
let unread_mentions_after_hide_bots =
PersonMentionView::get_unread_mentions(pool, &recipient_local_user_view.local_user).await?;
let mut query_without_bots = query.clone();
query_without_bots.show_bot_accounts = false;
let replies_after_hide_bots = query_without_bots.list(pool).await?;
assert_eq!(0, unread_mentions_after_hide_bots);
assert_eq!(0, replies_after_hide_bots.len());
Comment::delete(pool, inserted_comment.id).await?; Comment::delete(pool, inserted_comment.id).await?;
Post::delete(pool, inserted_post.id).await?; Post::delete(pool, inserted_post.id).await?;
Community::delete(pool, inserted_community.id).await?; Community::delete(pool, inserted_community.id).await?;