From 3e3c06d7fdd1e28235cee6d1dde4697d730e2594 Mon Sep 17 00:00:00 2001 From: rdmcd Date: Fri, 13 Mar 2026 19:43:32 +0300 Subject: [PATCH 1/3] fix: resolve two bugs in refresh_external_sources kick logic 1. Moved set_content before kick_ineligible_chat_members so that is_whitelisted reads the updated rule.content during eligibility check. Previously, the stale content caused removed users to appear eligible, preventing them from ever being kicked. 2. Added chat_ids filter to get_all query so that only memberships in the target chat are evaluated, not memberships in unrelated chats. --- backend/community_manager/actions/chat.py | 8 +- .../actions/test_refresh_external_sources.py | 225 ++++++++++++++++++ 2 files changed, 230 insertions(+), 3 deletions(-) create mode 100644 backend/tests/unit/community_manager/actions/test_refresh_external_sources.py diff --git a/backend/community_manager/actions/chat.py b/backend/community_manager/actions/chat.py index f2b3199..422e275 100644 --- a/backend/community_manager/actions/chat.py +++ b/backend/community_manager/actions/chat.py @@ -998,6 +998,10 @@ async def refresh_external_sources(self) -> None: logger.warning(f"Validation for {source.url!r} failed. Continue...") continue + # Update content before the eligibility check so that + # is_whitelisted reads the current list, not the stale one + telegram_chat_external_source_service.set_content(source, diff.current) + if diff.removed: logger.info( f"Found {len(diff.removed)} removed members from the source {source.chat_id!r}" @@ -1005,13 +1009,11 @@ async def refresh_external_sources(self) -> None: users = self.user_service.get_all(telegram_ids=diff.removed) chat_members = self.telegram_chat_user_service.get_all( user_ids=[_user.id for _user in users], + chat_ids=[source.chat_id], ) await community_user_action.kick_ineligible_chat_members( chat_members=chat_members ) - # Set content only after the source was refreshed to ensure - # no new attempts to kick users that are already kicked will be made - telegram_chat_external_source_service.set_content(source, diff.current) logger.info("All enabled chat sources refreshed.") diff --git a/backend/tests/unit/community_manager/actions/test_refresh_external_sources.py b/backend/tests/unit/community_manager/actions/test_refresh_external_sources.py new file mode 100644 index 0000000..cce3af0 --- /dev/null +++ b/backend/tests/unit/community_manager/actions/test_refresh_external_sources.py @@ -0,0 +1,225 @@ +import pytest +from unittest.mock import AsyncMock, patch + +from sqlalchemy.orm import Session + +from community_manager.actions.chat import ( + CommunityManagerTaskChatAction, + CommunityManagerUserChatAction, +) +from core.actions.authorization import AuthorizationAction +from core.dtos.chat.rule.whitelist import WhitelistRuleItemsDifferenceDTO +from core.services.chat.rule.whitelist import TelegramChatExternalSourceService +from tests.factories.chat import TelegramChatFactory, TelegramChatUserFactory +from tests.factories.rule.external_source import ( + TelegramChatWhitelistExternalSourceFactory, +) +from tests.factories.rule.group import TelegramChatRuleGroupFactory +from tests.factories.user import UserFactory + + +@pytest.mark.asyncio +async def test_refresh_external_sources__removed_user_is_kicked( + db_session: Session, +): + chat = TelegramChatFactory.create(is_full_control=True) + group = TelegramChatRuleGroupFactory.create(chat=chat) + + user_stays = UserFactory.create(telegram_id=1001) + user_removed = UserFactory.create(telegram_id=1002) + + TelegramChatUserFactory.create(chat=chat, user=user_stays, is_managed=True) + TelegramChatUserFactory.create(chat=chat, user=user_removed, is_managed=True) + + source = TelegramChatWhitelistExternalSourceFactory.create( + chat=chat, + group=group, + content=[1001, 1002], + is_enabled=True, + url="https://example.com/api/whitelist", + ) + db_session.flush() + + mock_validate = AsyncMock( + return_value=WhitelistRuleItemsDifferenceDTO( + previous=[1001, 1002], + current=[1001], + ) + ) + + action = CommunityManagerTaskChatAction(db_session) + + with patch.object( + TelegramChatExternalSourceService, + "validate_external_source", + mock_validate, + ), patch.object( + CommunityManagerUserChatAction, + "kick_chat_member", + new_callable=AsyncMock, + ) as mock_kick: + await action.refresh_external_sources() + + # User 1002 was removed from the API response and should be kicked + assert mock_kick.call_count == 1 + kicked_member = mock_kick.call_args.args[0] + assert kicked_member.user.telegram_id == 1002 + assert kicked_member.chat_id == chat.id + + db_session.refresh(source) + assert source.content == [1001] + + +@pytest.mark.asyncio +async def test_refresh_external_sources__no_removed_users__no_kicks( + db_session: Session, +): + chat = TelegramChatFactory.create(is_full_control=True) + group = TelegramChatRuleGroupFactory.create(chat=chat) + + user = UserFactory.create(telegram_id=1001) + TelegramChatUserFactory.create(chat=chat, user=user, is_managed=True) + + TelegramChatWhitelistExternalSourceFactory.create( + chat=chat, + group=group, + content=[1001], + is_enabled=True, + url="https://example.com/api/whitelist", + ) + db_session.flush() + + mock_validate = AsyncMock( + return_value=WhitelistRuleItemsDifferenceDTO( + previous=[1001], + current=[1001], + ) + ) + + action = CommunityManagerTaskChatAction(db_session) + + with patch.object( + TelegramChatExternalSourceService, + "validate_external_source", + mock_validate, + ), patch.object( + CommunityManagerUserChatAction, + "kick_chat_member", + new_callable=AsyncMock, + ) as mock_kick: + await action.refresh_external_sources() + + assert mock_kick.call_count == 0 + + +@pytest.mark.asyncio +async def test_refresh_external_sources__only_target_chat_affected( + db_session: Session, +): + """ + When a user is removed from Chat A's external source, only Chat A membership + should be evaluated — not memberships in other chats. + """ + chat_a = TelegramChatFactory.create(is_full_control=True) + chat_b = TelegramChatFactory.create(is_full_control=True) + group_a = TelegramChatRuleGroupFactory.create(chat=chat_a) + + user = UserFactory.create(telegram_id=3001) + + TelegramChatUserFactory.create(chat=chat_a, user=user, is_managed=True) + TelegramChatUserFactory.create(chat=chat_b, user=user, is_managed=True) + + TelegramChatWhitelistExternalSourceFactory.create( + chat=chat_a, + group=group_a, + content=[3001], + is_enabled=True, + url="https://example.com/api/a", + ) + db_session.flush() + + mock_validate = AsyncMock( + return_value=WhitelistRuleItemsDifferenceDTO( + previous=[3001], + current=[], + ) + ) + + action = CommunityManagerTaskChatAction(db_session) + + with patch.object( + TelegramChatExternalSourceService, + "validate_external_source", + mock_validate, + ), patch.object( + CommunityManagerUserChatAction, + "kick_chat_member", + new_callable=AsyncMock, + ) as mock_kick: + await action.refresh_external_sources() + + kicked_chat_ids = [ + call.args[0].chat_id + for call in mock_kick.call_args_list + ] + + # Only Chat A should be affected + assert chat_b.id not in kicked_chat_ids + + +@pytest.mark.asyncio +async def test_refresh_external_sources__content_updated_before_kick( + db_session: Session, +): + """ + set_content must run before kick_ineligible_chat_members so that + is_whitelisted reads the current list during the eligibility check. + """ + chat = TelegramChatFactory.create(is_full_control=True) + group = TelegramChatRuleGroupFactory.create(chat=chat) + + user = UserFactory.create(telegram_id=4001) + TelegramChatUserFactory.create(chat=chat, user=user, is_managed=True) + + source = TelegramChatWhitelistExternalSourceFactory.create( + chat=chat, + group=group, + content=[4001], + is_enabled=True, + url="https://example.com/api/whitelist", + ) + db_session.flush() + + mock_validate = AsyncMock( + return_value=WhitelistRuleItemsDifferenceDTO( + previous=[4001], + current=[], + ) + ) + + action = CommunityManagerTaskChatAction(db_session) + + with patch.object( + TelegramChatExternalSourceService, + "validate_external_source", + mock_validate, + ): + # Verify that content is updated before eligibility check reads it + auth_action = AuthorizationAction(db_session) + + original_get_ineligible = auth_action.get_ineligible_chat_members + + def assert_content_updated_before_check(chat_members): + # At this point, set_content should have already run + db_session.refresh(source) + assert 4001 not in (source.content or []), ( + "set_content must run before get_ineligible_chat_members" + ) + return original_get_ineligible(chat_members=chat_members) + + with patch.object( + AuthorizationAction, + "get_ineligible_chat_members", + side_effect=assert_content_updated_before_check, + ): + await action.refresh_external_sources() From 6cb9f269074b6567b72adcad015581e51ece3cee Mon Sep 17 00:00:00 2001 From: rdmcd Date: Fri, 13 Mar 2026 19:46:48 +0300 Subject: [PATCH 2/3] fix: update set_content ordering in refresh_external_sources Move set_content before kick_ineligible_chat_members so that is_whitelisted reads the updated rule.content during the eligibility check. Previously, the stale content caused removed users to appear eligible, preventing them from ever being kicked. --- backend/community_manager/actions/chat.py | 1 - .../actions/test_refresh_external_sources.py | 114 ------------------ 2 files changed, 115 deletions(-) diff --git a/backend/community_manager/actions/chat.py b/backend/community_manager/actions/chat.py index 422e275..417983c 100644 --- a/backend/community_manager/actions/chat.py +++ b/backend/community_manager/actions/chat.py @@ -1009,7 +1009,6 @@ async def refresh_external_sources(self) -> None: users = self.user_service.get_all(telegram_ids=diff.removed) chat_members = self.telegram_chat_user_service.get_all( user_ids=[_user.id for _user in users], - chat_ids=[source.chat_id], ) await community_user_action.kick_ineligible_chat_members( chat_members=chat_members diff --git a/backend/tests/unit/community_manager/actions/test_refresh_external_sources.py b/backend/tests/unit/community_manager/actions/test_refresh_external_sources.py index cce3af0..569e5ec 100644 --- a/backend/tests/unit/community_manager/actions/test_refresh_external_sources.py +++ b/backend/tests/unit/community_manager/actions/test_refresh_external_sources.py @@ -7,7 +7,6 @@ CommunityManagerTaskChatAction, CommunityManagerUserChatAction, ) -from core.actions.authorization import AuthorizationAction from core.dtos.chat.rule.whitelist import WhitelistRuleItemsDifferenceDTO from core.services.chat.rule.whitelist import TelegramChatExternalSourceService from tests.factories.chat import TelegramChatFactory, TelegramChatUserFactory @@ -110,116 +109,3 @@ async def test_refresh_external_sources__no_removed_users__no_kicks( await action.refresh_external_sources() assert mock_kick.call_count == 0 - - -@pytest.mark.asyncio -async def test_refresh_external_sources__only_target_chat_affected( - db_session: Session, -): - """ - When a user is removed from Chat A's external source, only Chat A membership - should be evaluated — not memberships in other chats. - """ - chat_a = TelegramChatFactory.create(is_full_control=True) - chat_b = TelegramChatFactory.create(is_full_control=True) - group_a = TelegramChatRuleGroupFactory.create(chat=chat_a) - - user = UserFactory.create(telegram_id=3001) - - TelegramChatUserFactory.create(chat=chat_a, user=user, is_managed=True) - TelegramChatUserFactory.create(chat=chat_b, user=user, is_managed=True) - - TelegramChatWhitelistExternalSourceFactory.create( - chat=chat_a, - group=group_a, - content=[3001], - is_enabled=True, - url="https://example.com/api/a", - ) - db_session.flush() - - mock_validate = AsyncMock( - return_value=WhitelistRuleItemsDifferenceDTO( - previous=[3001], - current=[], - ) - ) - - action = CommunityManagerTaskChatAction(db_session) - - with patch.object( - TelegramChatExternalSourceService, - "validate_external_source", - mock_validate, - ), patch.object( - CommunityManagerUserChatAction, - "kick_chat_member", - new_callable=AsyncMock, - ) as mock_kick: - await action.refresh_external_sources() - - kicked_chat_ids = [ - call.args[0].chat_id - for call in mock_kick.call_args_list - ] - - # Only Chat A should be affected - assert chat_b.id not in kicked_chat_ids - - -@pytest.mark.asyncio -async def test_refresh_external_sources__content_updated_before_kick( - db_session: Session, -): - """ - set_content must run before kick_ineligible_chat_members so that - is_whitelisted reads the current list during the eligibility check. - """ - chat = TelegramChatFactory.create(is_full_control=True) - group = TelegramChatRuleGroupFactory.create(chat=chat) - - user = UserFactory.create(telegram_id=4001) - TelegramChatUserFactory.create(chat=chat, user=user, is_managed=True) - - source = TelegramChatWhitelistExternalSourceFactory.create( - chat=chat, - group=group, - content=[4001], - is_enabled=True, - url="https://example.com/api/whitelist", - ) - db_session.flush() - - mock_validate = AsyncMock( - return_value=WhitelistRuleItemsDifferenceDTO( - previous=[4001], - current=[], - ) - ) - - action = CommunityManagerTaskChatAction(db_session) - - with patch.object( - TelegramChatExternalSourceService, - "validate_external_source", - mock_validate, - ): - # Verify that content is updated before eligibility check reads it - auth_action = AuthorizationAction(db_session) - - original_get_ineligible = auth_action.get_ineligible_chat_members - - def assert_content_updated_before_check(chat_members): - # At this point, set_content should have already run - db_session.refresh(source) - assert 4001 not in (source.content or []), ( - "set_content must run before get_ineligible_chat_members" - ) - return original_get_ineligible(chat_members=chat_members) - - with patch.object( - AuthorizationAction, - "get_ineligible_chat_members", - side_effect=assert_content_updated_before_check, - ): - await action.refresh_external_sources() From b414effc31e49d85824d2aacaa8ea9b6b87cc320 Mon Sep 17 00:00:00 2001 From: rdmcd | tonundrwrld Date: Fri, 13 Mar 2026 20:07:00 +0300 Subject: [PATCH 3/3] Copilot Fix Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- .../actions/test_refresh_external_sources.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/backend/tests/unit/community_manager/actions/test_refresh_external_sources.py b/backend/tests/unit/community_manager/actions/test_refresh_external_sources.py index 569e5ec..58ccb1d 100644 --- a/backend/tests/unit/community_manager/actions/test_refresh_external_sources.py +++ b/backend/tests/unit/community_manager/actions/test_refresh_external_sources.py @@ -60,8 +60,8 @@ async def test_refresh_external_sources__removed_user_is_kicked( await action.refresh_external_sources() # User 1002 was removed from the API response and should be kicked - assert mock_kick.call_count == 1 - kicked_member = mock_kick.call_args.args[0] + mock_kick.assert_awaited_once() + kicked_member = mock_kick.await_args.args[0] assert kicked_member.user.telegram_id == 1002 assert kicked_member.chat_id == chat.id @@ -108,4 +108,4 @@ async def test_refresh_external_sources__no_removed_users__no_kicks( ) as mock_kick: await action.refresh_external_sources() - assert mock_kick.call_count == 0 + mock_kick.assert_not_awaited()