fix: update set_content ordering in refresh_external_sources#232
Open
rdmcd wants to merge 3 commits intoOpenBuilders:devfrom
Open
fix: update set_content ordering in refresh_external_sources#232rdmcd wants to merge 3 commits intoOpenBuilders:devfrom
rdmcd wants to merge 3 commits intoOpenBuilders:devfrom
Conversation
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.
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.
There was a problem hiding this comment.
Pull request overview
Fixes a correctness issue in refresh_external_sources where members removed from an external whitelist were not being kicked because the DB-backed content used during eligibility evaluation was updated too late.
Changes:
- Update external-source
contentbefore running the kick/eligibility path so eligibility checks use the latest list. - Add unit coverage for “removed user is kicked” and “no diff means no kicks”.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
backend/community_manager/actions/chat.py |
Reorders external source content persistence ahead of eligibility/kick evaluation. |
backend/tests/unit/community_manager/actions/test_refresh_external_sources.py |
Adds async unit tests verifying kicks happen for removed users and do not happen when nothing changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
1005
to
1007
| if diff.removed: | ||
| logger.info( | ||
| f"Found {len(diff.removed)} removed members from the source {source.chat_id!r}" |
backend/tests/unit/community_manager/actions/test_refresh_external_sources.py
Show resolved
Hide resolved
| ) as mock_kick: | ||
| await action.refresh_external_sources() | ||
|
|
||
| assert mock_kick.call_count == 0 |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Author
|
Initial PR was coauthored by Claude. So it can be labelled |
scutuatua-crypto
approved these changes
Mar 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes a bug in
refresh_external_sourceswhere users removed from an external API whitelist are never kicked.set_contentwas called afterkick_ineligible_chat_members. The eligibility check inside the kick path readsrule.contentfrom the DB, which still contains the old list (including the removed user). Sois_whitelistedreturnsTrue, the user appears eligible, andkick_chat_memberis never called. After that,set_contentupdates the DB — and the next cycle sees no diff, so the user is never kicked.Checklist
Changes
set_contentbefore kick block: Content is updated before the eligibility check runs, sois_whitelistedreads the current list instead of the stale one. If a kick fails mid-way, the periodic compliance check (check_chat_members_compliance) will catch the user on the next run.How Has This Been Tested?
Unit tests added in
tests/unit/community_manager/actions/test_refresh_external_sources.py:MODE=test ./docker.sh run --rm -it test pytest tests/unit/community_manager/actions/test_refresh_external_sources.py -vAdditional Notes
Existing users who should have been kicked but weren't can be cleaned up by running
check-target-chat-membersfor each chat with external source rules — the updatedrule.contentalready excludes them, so the compliance check will correctly identify them as ineligible.