Team_id-based Slack bot tokens and test updates (#91)#95
Merged
snowfox1003 merged 8 commits intocppalliance:developfrom Mar 9, 2026
Merged
Team_id-based Slack bot tokens and test updates (#91)#95snowfox1003 merged 8 commits intocppalliance:developfrom
snowfox1003 merged 8 commits intocppalliance:developfrom
Conversation
📝 WalkthroughWalkthroughReplace single-workspace Slack token usage with a multi-workspace mapping (SLACK_TEAM_IDS + per-team SLACK_BOT_TOKEN_), thread team_id through token/client acquisition, and move channel/user/message sync flows from file-based recovery to API-driven retrieval with merged raw messages by timestamp. Changes
Sequence Diagram(s)sequenceDiagram
participant Config as Config/Settings
participant TokenOps as TokenOps (tokens.py)
participant Client as ClientFactory
participant Fetcher as Fetcher/Sync
participant SlackAPI as Slack API
participant DB as Database
Note over Config,Fetcher: Multi-workspace token resolution + API-driven sync
Fetcher->>TokenOps: request token (team_id)
TokenOps->>Config: lookup SLACK_BOT_TOKEN[team_id] / SLACK_TEAM_ID fallback
alt token found
TokenOps-->>Fetcher: return token
else token missing
TokenOps-->>Fetcher: raise ValueError / log error
end
Fetcher->>Client: get_slack_client(team_id=..., bot_token=token)
Client->>Client: construct Slack API client with token
Client-->>Fetcher: client instance
Fetcher->>SlackAPI: call conversations.* / users.* / channels.* (team-scoped)
SlackAPI-->>Fetcher: responses (channels, users, messages)
Fetcher->>Fetcher: merge/process messages by ts
Fetcher->>DB: persist users/channels/messages
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Collaborator
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
cppa_slack_tracker/sync/sync_channel_user.py (1)
69-71: Remove redundant exception interpolation inlogger.exception.
logger.exception()automatically logs the active exception and traceback. Includingein the format string duplicates this information.♻️ Proposed cleanup
- logger.exception( - "Failed to fetch/sync members for %s: %s", channel.channel_id, e - ) + logger.exception( + "Failed to fetch/sync members for %s", channel.channel_id + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cppa_slack_tracker/sync/sync_channel_user.py` around lines 69 - 71, In sync_channel_user.py, inside the exception handling where logger.exception is called (the block that logs "Failed to fetch/sync members for %s: %s"), remove the redundant exception interpolation by passing only the channel identifier (e.g., channel.channel_id) as the formatting argument and let logger.exception capture the exception/traceback automatically; update the logger.exception call used in the member sync/fetch function so it reads a single-format-argument message referencing channel.channel_id (do not include the exception object `e` in the format args).cppa_slack_tracker/sync/sync_message.py (1)
187-189: Minor: Redundant conditional assignment.The expression
start_date = last_d if last_d is not None else Noneis equivalent tostart_date = last_dsincelast_dis alreadyNonewhen the condition fails.✨ Suggested simplification
- start_date = last_d if last_d is not None else None + start_date = last_d🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cppa_slack_tracker/sync/sync_message.py` around lines 187 - 189, The conditional assignment setting start_date is redundant; simply assign the result of _last_message_date(channel) directly to start_date (e.g., start_date = _last_message_date(channel)) or set start_date = last_d after calling _last_message_date(channel), removing the if-else expression that checks for None; update the assignment near the _last_message_date(channel) call and the start_date variable.cppa_slack_tracker/fetcher.py (1)
224-244: Consider consolidating the duplicated filter logic.The filter logic for bounded vs. unbounded
start_datehas similar structure that could potentially be refactored. However, given the different conditions (start_date <= created_d <= end_datevs.created_d <= end_date), keeping them separate for clarity is also reasonable.✨ Optional: Consolidated filter logic
- # Filter by date: [start_date, end_date] or <= end_date when start_date is None messages = [] for msg in all_messages: created_d = _ts_to_utc_date(msg.get("ts")) - if start_date is not None: - if created_d and start_date <= created_d <= end_date: - messages.append(msg) - continue - edited = msg.get("edited") or {} - edited_d = _ts_to_utc_date(edited.get("ts")) - if edited_d and start_date <= edited_d <= end_date: - messages.append(msg) - else: - if created_d and created_d <= end_date: - messages.append(msg) - continue - edited = msg.get("edited") or {} - edited_d = _ts_to_utc_date(edited.get("ts")) - if edited_d and edited_d <= end_date: - messages.append(msg) + edited = msg.get("edited") or {} + edited_d = _ts_to_utc_date(edited.get("ts")) + + def in_range(d: date | None) -> bool: + if d is None: + return False + if start_date is not None: + return start_date <= d <= end_date + return d <= end_date + + if in_range(created_d) or in_range(edited_d): + messages.append(msg) return messages🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cppa_slack_tracker/fetcher.py` around lines 224 - 244, The message date-filtering block duplicates nearly identical logic for created vs edited timestamps; refactor by extracting a small helper (e.g., _in_range(date, start_date, end_date) or _check_and_append(msg, timestamp_key)) and use it to evaluate both msg.get("ts") and msg.get("edited")["ts"] for each msg, replacing the two branches that check start_date is None vs bounded; keep using _ts_to_utc_date, all_messages, start_date, end_date and ensure behavior remains: when start_date is None treat check as date <= end_date, otherwise enforce start_date <= date <= end_date, appending msg if either created or edited timestamp matches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/settings.py`:
- Around line 175-192: get_slack_client currently fails for no-arg calls because
SLACK_BOT_TOKEN is a dict; update get_slack_client to accept optional team_id
and bot_token, and if bot_token is not provided and SLACK_BOT_TOKEN is a dict,
read a fallback SLACK_TEAM_ID env var (env("SLACK_TEAM_ID")) to pick the token
from SLACK_BOT_TOKEN[team_id]; if the fallback is missing or token lookup fails,
keep raising the ValueError as before. Ensure callers like SlackFetcher.__init__
and any no-arg get_slack_client() usage can rely on this environment fallback so
single-workspace setups work without changing every call site.
In `@operations/slack_ops/tokens.py`:
- Around line 17-53: The new strict requirement that
get_slack_bot_token(team_id) always receive a team_id breaks callers; change
get_slack_bot_token and its consumer get_slack_client to accept an optional
team_id and, when team_id is missing or empty, fall back to a default
SLACK_TEAM_ID from Django settings (getattr(settings, "SLACK_TEAM_ID", None))
before raising; update the lookup logic in get_slack_bot_token to use that
fallback, keep the same error logging and ValueError only if both team_id and
fallback are absent or the token is missing, and ensure callers that already
pass team_id continue to work unchanged (refer to get_slack_bot_token and
get_slack_client to locate code to modify).
---
Nitpick comments:
In `@cppa_slack_tracker/fetcher.py`:
- Around line 224-244: The message date-filtering block duplicates nearly
identical logic for created vs edited timestamps; refactor by extracting a small
helper (e.g., _in_range(date, start_date, end_date) or _check_and_append(msg,
timestamp_key)) and use it to evaluate both msg.get("ts") and
msg.get("edited")["ts"] for each msg, replacing the two branches that check
start_date is None vs bounded; keep using _ts_to_utc_date, all_messages,
start_date, end_date and ensure behavior remains: when start_date is None treat
check as date <= end_date, otherwise enforce start_date <= date <= end_date,
appending msg if either created or edited timestamp matches.
In `@cppa_slack_tracker/sync/sync_channel_user.py`:
- Around line 69-71: In sync_channel_user.py, inside the exception handling
where logger.exception is called (the block that logs "Failed to fetch/sync
members for %s: %s"), remove the redundant exception interpolation by passing
only the channel identifier (e.g., channel.channel_id) as the formatting
argument and let logger.exception capture the exception/traceback automatically;
update the logger.exception call used in the member sync/fetch function so it
reads a single-format-argument message referencing channel.channel_id (do not
include the exception object `e` in the format args).
In `@cppa_slack_tracker/sync/sync_message.py`:
- Around line 187-189: The conditional assignment setting start_date is
redundant; simply assign the result of _last_message_date(channel) directly to
start_date (e.g., start_date = _last_message_date(channel)) or set start_date =
last_d after calling _last_message_date(channel), removing the if-else
expression that checks for None; update the assignment near the
_last_message_date(channel) call and the start_date variable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 98e4d8e1-de90-4586-bccc-64adfc1c42c7
📒 Files selected for processing (13)
.env.exampleconfig/settings.pycppa_slack_tracker/fetcher.pycppa_slack_tracker/management/commands/run_cppa_slack_tracker.pycppa_slack_tracker/services.pycppa_slack_tracker/sync/__init__.pycppa_slack_tracker/sync/sync_channel.pycppa_slack_tracker/sync/sync_channel_user.pycppa_slack_tracker/sync/sync_message.pycppa_slack_tracker/sync/sync_user.pycppa_slack_tracker/tests/test_sync_message.pyoperations/slack_ops/tokens.pyoperations/tests/test_slack_tokens.py
Collaborator
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cppa_slack_transcript_tracker/runner.py (1)
34-38: Team-scoped token resolution works correctly, though explicit lookup is redundant.The code explicitly reads
SLACK_TEAM_IDand passes it toget_slack_bot_token(team_id=team_id). However,get_slack_bot_token()already has internal fallback logic via_slack_team_id_fallback()that performs the same lookup whenteam_idisNone.This redundancy is harmless and makes the intent clearer, so no change is required. The
ValueErrorcatch correctly handles missing token scenarios.♻️ Optional: Simplify by relying on internal fallback
try: - team_id = getattr(settings, "SLACK_TEAM_ID", None) or None - bot_token = bot_token or get_slack_bot_token(team_id=team_id) + bot_token = bot_token or get_slack_bot_token() except ValueError: bot_token = NoneThe internal fallback in
get_slack_bot_token()already readsSLACK_TEAM_IDfrom settings andos.environ.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cppa_slack_transcript_tracker/runner.py` around lines 34 - 38, The current code redundantly reads SLACK_TEAM_ID via getattr(settings, "SLACK_TEAM_ID", None) before calling get_slack_bot_token(team_id=team_id); simplify by letting get_slack_bot_token() perform its internal fallback: call get_slack_bot_token() without passing an explicit team_id (or pass None) and keep the existing ValueError exception handling so bot_token is set to None on missing token; update the try/except block around get_slack_bot_token() (referencing get_slack_bot_token and SLACK_TEAM_ID) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cppa_slack_tracker/management/commands/run_cppa_slack_tracker.py`:
- Around line 149-151: Update the stale dry-run/default messaging in
run_cppa_slack_tracker.py to reflect that the default execution runs users,
channels and messages (not "sync messages only"); change any help text, comments
or log messages that say "sync messages only (default)" to something like "sync
users, channels and messages (default)" and ensure any dry-run output that
prints the default path references the three actions invoked by sync_users,
sync_channels and sync_messages so messaging matches the actual behavior.
In `@cppa_slack_transcript_tracker/utils/slack_listener.py`:
- Around line 55-56: The code in slack_listener.py reads Django settings
unconditionally when computing team_id before honoring an explicit bot_token,
which can raise configuration errors in non-Django runtimes; change the logic in
the SlackListener initializer (where self.bot_token is set and team_id is
currently computed) to first check if bot_token was provided and if so skip any
access to django.conf.settings, otherwise only then call getattr(settings,
"SLACK_TEAM_ID", None) and get_slack_bot_token(team_id=team_id) to derive
self.bot_token.
---
Nitpick comments:
In `@cppa_slack_transcript_tracker/runner.py`:
- Around line 34-38: The current code redundantly reads SLACK_TEAM_ID via
getattr(settings, "SLACK_TEAM_ID", None) before calling
get_slack_bot_token(team_id=team_id); simplify by letting get_slack_bot_token()
perform its internal fallback: call get_slack_bot_token() without passing an
explicit team_id (or pass None) and keep the existing ValueError exception
handling so bot_token is set to None on missing token; update the try/except
block around get_slack_bot_token() (referencing get_slack_bot_token and
SLACK_TEAM_ID) accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a703599a-71bb-4206-ab0f-415a809c0879
📒 Files selected for processing (16)
.env.exampleconfig/settings.pycppa_slack_tracker/fetcher.pycppa_slack_tracker/management/commands/run_cppa_slack_tracker.pycppa_slack_tracker/services.pycppa_slack_tracker/sync/__init__.pycppa_slack_tracker/sync/sync_channel.pycppa_slack_tracker/sync/sync_channel_user.pycppa_slack_tracker/sync/sync_message.pycppa_slack_tracker/sync/sync_user.pycppa_slack_tracker/tests/test_sync_message.pycppa_slack_transcript_tracker/management/commands/run_cppa_slack_transcript_tracker.pycppa_slack_transcript_tracker/runner.pycppa_slack_transcript_tracker/utils/slack_listener.pyoperations/slack_ops/tokens.pyoperations/tests/test_slack_tokens.py
Collaborator
Author
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
cppa_slack_tracker/management/commands/run_cppa_slack_tracker.py (1)
130-133:⚠️ Potential issue | 🟡 MinorUpdate stale default-path messaging to match actual behavior.
Default execution now runs users + channels + messages, but comments and dry-run output still say “sync messages only (default)”.
🔧 Suggested fix
- # when set; multiple flags can be combined. If none are set, default to - # syncing messages only. + # when set; multiple flags can be combined. If none are set, default to + # syncing users, channels, and messages. @@ - self.stdout.write(" Would run: sync messages only (default)") + self.stdout.write(" Would run: sync users, channels, and messages (default)")Also applies to: 189-194
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cppa_slack_tracker/management/commands/run_cppa_slack_tracker.py` around lines 130 - 133, The inline comment and dry-run messaging in run_cppa_slack_tracker.py are out of date: update the comment block that currently states the default is "sync messages only (default)" to reflect the new default behavior of running users + channels + messages, and adjust any dry-run output text emitted by the Command class (e.g., in Command.handle or any helper method that prints the default-run summary) to say "default: sync users, channels, and messages" (or equivalent) so comments and runtime messages match actual behavior.cppa_slack_transcript_tracker/utils/slack_listener.py (1)
54-56:⚠️ Potential issue | 🟠 MajorHonor explicit
bot_tokenbefore reading Django settings.
team_idis resolved unconditionally, so explicit token usage still depends on settings access. Short-circuit onbot_tokenfirst.🔧 Suggested fix
def __init__(self, bot_token=None, app_token=None): - team_id = getattr(settings, "SLACK_TEAM_ID", None) or None - self.bot_token = bot_token or get_slack_bot_token(team_id=team_id) + if bot_token: + self.bot_token = bot_token + else: + try: + team_id = (getattr(settings, "SLACK_TEAM_ID", None) or "").strip() or None + except Exception: + team_id = None + self.bot_token = get_slack_bot_token(team_id=team_id)In Django 4.2, can `getattr(django.conf.settings, "SOME_KEY", None)` raise `ImproperlyConfigured` when settings are not configured?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cppa_slack_transcript_tracker/utils/slack_listener.py` around lines 54 - 56, In __init__ short-circuit when an explicit bot_token is provided: if bot_token is truthy set self.bot_token to it and avoid touching Django settings; otherwise resolve team_id from settings.SLACK_TEAM_ID and call get_slack_bot_token to populate self.bot_token. This ensures __init__ (and the get_slack_bot_token call) only accesses settings when necessary and honors an explicit bot_token.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cppa_slack_tracker/management/commands/run_cppa_slack_tracker.py`:
- Around line 130-133: The inline comment and dry-run messaging in
run_cppa_slack_tracker.py are out of date: update the comment block that
currently states the default is "sync messages only (default)" to reflect the
new default behavior of running users + channels + messages, and adjust any
dry-run output text emitted by the Command class (e.g., in Command.handle or any
helper method that prints the default-run summary) to say "default: sync users,
channels, and messages" (or equivalent) so comments and runtime messages match
actual behavior.
In `@cppa_slack_transcript_tracker/utils/slack_listener.py`:
- Around line 54-56: In __init__ short-circuit when an explicit bot_token is
provided: if bot_token is truthy set self.bot_token to it and avoid touching
Django settings; otherwise resolve team_id from settings.SLACK_TEAM_ID and call
get_slack_bot_token to populate self.bot_token. This ensures __init__ (and the
get_slack_bot_token call) only accesses settings when necessary and honors an
explicit bot_token.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4d5d7e71-ad25-4199-bb25-5e5e3ed74b0f
📒 Files selected for processing (16)
.env.exampleconfig/settings.pycppa_slack_tracker/fetcher.pycppa_slack_tracker/management/commands/run_cppa_slack_tracker.pycppa_slack_tracker/services.pycppa_slack_tracker/sync/__init__.pycppa_slack_tracker/sync/sync_channel.pycppa_slack_tracker/sync/sync_channel_user.pycppa_slack_tracker/sync/sync_message.pycppa_slack_tracker/sync/sync_user.pycppa_slack_tracker/tests/test_sync_message.pycppa_slack_transcript_tracker/management/commands/run_cppa_slack_transcript_tracker.pycppa_slack_transcript_tracker/runner.pycppa_slack_transcript_tracker/utils/slack_listener.pyoperations/slack_ops/tokens.pyoperations/tests/test_slack_tokens.py
…r explicit bot_token cppalliance#91
Collaborator
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
1 similar comment
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cppa_slack_tracker/fetcher.py (1)
197-244:⚠️ Potential issue | 🟠 MajorAvoid buffering full channel history before date filtering.
fetch_messagesstores every page inall_messagesand only filters afterward. Withstart_date=None, this can become unbounded memory growth on large channels.💡 Proposed fix (filter per page while paginating)
- all_messages = [] + messages = [] cursor = None while True: kwargs = { "channel": channel_id, "limit": 1000, "latest": latest_ts, "cursor": cursor, } if oldest_ts is not None: kwargs["oldest"] = oldest_ts data = client.conversations_history(**kwargs) if not data.get("ok"): logger.warning( "conversations.history failed for %s: %s", channel_id, data.get("error", "unknown"), ) break batch = data.get("messages", []) - all_messages.extend(batch) + for msg in batch: + created_d = _ts_to_utc_date(msg.get("ts")) + if start_date is not None: + if created_d and start_date <= created_d <= end_date: + messages.append(msg) + continue + edited = msg.get("edited") or {} + edited_d = _ts_to_utc_date(edited.get("ts")) + if edited_d and start_date <= edited_d <= end_date: + messages.append(msg) + else: + if created_d and created_d <= end_date: + messages.append(msg) + continue + edited = msg.get("edited") or {} + edited_d = _ts_to_utc_date(edited.get("ts")) + if edited_d and edited_d <= end_date: + messages.append(msg) if not batch: break cursor = (data.get("response_metadata") or {}).get("next_cursor") if not cursor: break - - # Filter by date: [start_date, end_date] or <= end_date when start_date is None - messages = [] - for msg in all_messages: - created_d = _ts_to_utc_date(msg.get("ts")) - if start_date is not None: - if created_d and start_date <= created_d <= end_date: - messages.append(msg) - continue - edited = msg.get("edited") or {} - edited_d = _ts_to_utc_date(edited.get("ts")) - if edited_d and start_date <= edited_d <= end_date: - messages.append(msg) - else: - if created_d and created_d <= end_date: - messages.append(msg) - continue - edited = msg.get("edited") or {} - edited_d = _ts_to_utc_date(edited.get("ts")) - if edited_d and edited_d <= end_date: - messages.append(msg) return messages🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cppa_slack_tracker/fetcher.py` around lines 197 - 244, The function fetch_messages currently accumulates every page into all_messages before date filtering, risking unbounded memory usage for large channels; change the pagination loop to filter each batch as it is received (using _ts_to_utc_date on msg["ts"] and msg.get("edited")["ts"]) and append only messages that meet the existing date logic to a persisted messages list (replace uses of all_messages with per-batch filtering), preserving the same start_date None behavior and the existing pagination/cursor handling (variables: client.conversations_history, kwargs, batch, cursor, start_date, end_date, _ts_to_utc_date).
🧹 Nitpick comments (2)
cppa_slack_tracker/sync/sync_message.py (1)
187-189: Minor: Simplify redundant conditional assignment.Line 189 can be simplified since
last_dis alreadyOptional[date]and the conditional is effectively a no-op.♻️ Proposed simplification
# Determine start_date: same day as last message, or None to fetch from beginning last_d = _last_message_date(channel) - start_date = last_d if last_d is not None else None + start_date = last_d🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cppa_slack_tracker/sync/sync_message.py` around lines 187 - 189, The assignment to start_date is redundant because _last_message_date(channel) already returns Optional[date]; replace the two-step last_d then conditional assignment with a single direct assignment by calling _last_message_date(channel) and assigning its result to start_date (use the existing symbol _last_message_date and variable start_date in sync_message.py).cppa_slack_tracker/sync/sync_channel_user.py (1)
68-72: Remove redundant exception object fromlogging.exceptioncall.The static analysis tool correctly flags that
eis redundant inlogger.exception()since the exception info is automatically included.♻️ Proposed fix
except Exception as e: logger.exception( - "Failed to fetch/sync members for %s: %s", channel.channel_id, e + "Failed to fetch/sync members for %s", channel.channel_id ) error_count += 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cppa_slack_tracker/sync/sync_channel_user.py` around lines 68 - 72, The logger.exception call passes the caught exception object redundantly; update the call in the except block that handles member sync for the channel (the logger.exception call referencing channel.channel_id and variable e) to omit e and only pass the formatted arguments (e.g., logger.exception("Failed to fetch/sync members for %s", channel.channel_id)), leaving error_count += 1 unchanged so exception info is still captured automatically.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@cppa_slack_tracker/fetcher.py`:
- Around line 197-244: The function fetch_messages currently accumulates every
page into all_messages before date filtering, risking unbounded memory usage for
large channels; change the pagination loop to filter each batch as it is
received (using _ts_to_utc_date on msg["ts"] and msg.get("edited")["ts"]) and
append only messages that meet the existing date logic to a persisted messages
list (replace uses of all_messages with per-batch filtering), preserving the
same start_date None behavior and the existing pagination/cursor handling
(variables: client.conversations_history, kwargs, batch, cursor, start_date,
end_date, _ts_to_utc_date).
---
Nitpick comments:
In `@cppa_slack_tracker/sync/sync_channel_user.py`:
- Around line 68-72: The logger.exception call passes the caught exception
object redundantly; update the call in the except block that handles member sync
for the channel (the logger.exception call referencing channel.channel_id and
variable e) to omit e and only pass the formatted arguments (e.g.,
logger.exception("Failed to fetch/sync members for %s", channel.channel_id)),
leaving error_count += 1 unchanged so exception info is still captured
automatically.
In `@cppa_slack_tracker/sync/sync_message.py`:
- Around line 187-189: The assignment to start_date is redundant because
_last_message_date(channel) already returns Optional[date]; replace the two-step
last_d then conditional assignment with a single direct assignment by calling
_last_message_date(channel) and assigning its result to start_date (use the
existing symbol _last_message_date and variable start_date in sync_message.py).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ab71d55d-b2bc-4b39-9a15-906b2584cf86
📒 Files selected for processing (16)
.env.exampleconfig/settings.pycppa_slack_tracker/fetcher.pycppa_slack_tracker/management/commands/run_cppa_slack_tracker.pycppa_slack_tracker/services.pycppa_slack_tracker/sync/__init__.pycppa_slack_tracker/sync/sync_channel.pycppa_slack_tracker/sync/sync_channel_user.pycppa_slack_tracker/sync/sync_message.pycppa_slack_tracker/sync/sync_user.pycppa_slack_tracker/tests/test_sync_message.pycppa_slack_transcript_tracker/management/commands/run_cppa_slack_transcript_tracker.pycppa_slack_transcript_tracker/runner.pycppa_slack_transcript_tracker/utils/slack_listener.pyoperations/slack_ops/tokens.pyoperations/tests/test_slack_tokens.py
There was a problem hiding this comment.
🧹 Nitpick comments (2)
cppa_slack_tracker/sync/sync_channel_user.py (1)
68-71: Redundant exception object inlogging.exceptioncall.The
eparameter is redundant sincelogger.exception()automatically includes the exception traceback. This is flagged by the static analysis hint (TRY401).♻️ Suggested fix
except Exception as e: logger.exception( - "Failed to fetch/sync members for %s: %s", channel.channel_id, e + "Failed to fetch/sync members for %s", channel.channel_id )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cppa_slack_tracker/sync/sync_channel_user.py` around lines 68 - 71, The logger.exception call in sync_channel_user.py is passing the caught exception object `e` redundantly; update the except block that surrounds the member sync (the logger.exception call referencing `channel.channel_id`) to remove the `e` argument and only log the channel id (e.g. logger.exception("Failed to fetch/sync members for %s", channel.channel_id)) so the traceback is included automatically by logger.exception.cppa_slack_tracker/fetcher.py (1)
224-244: Consider consolidating the filter logic to reduce duplication.The date filtering logic has two branches that are structurally similar. You could simplify this by using a helper or consolidating the condition.
♻️ Suggested consolidation
# Filter by date: [start_date, end_date] or <= end_date when start_date is None messages = [] for msg in all_messages: created_d = _ts_to_utc_date(msg.get("ts")) - if start_date is not None: - if created_d and start_date <= created_d <= end_date: - messages.append(msg) - continue - edited = msg.get("edited") or {} - edited_d = _ts_to_utc_date(edited.get("ts")) - if edited_d and start_date <= edited_d <= end_date: - messages.append(msg) - else: - if created_d and created_d <= end_date: - messages.append(msg) - continue - edited = msg.get("edited") or {} - edited_d = _ts_to_utc_date(edited.get("ts")) - if edited_d and edited_d <= end_date: - messages.append(msg) + edited = msg.get("edited") or {} + edited_d = _ts_to_utc_date(edited.get("ts")) + + def in_range(d: date | None) -> bool: + if d is None: + return False + if start_date is not None: + return start_date <= d <= end_date + return d <= end_date + + if in_range(created_d) or in_range(edited_d): + messages.append(msg) return messages🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cppa_slack_tracker/fetcher.py` around lines 224 - 244, The date-filtering branches duplicate logic; create a small helper (e.g., def _in_range(d, start_date, end_date): return d and (start_date is None and d <= end_date or start_date <= d <= end_date)) and replace the two branches by a single loop over all_messages where you compute created_d = _ts_to_utc_date(msg.get("ts")) and edited_d = _ts_to_utc_date((msg.get("edited") or {}).get("ts")) and append msg if _in_range(created_d, start_date, end_date) or _in_range(edited_d, start_date, end_date). This keeps the existing symbols (all_messages, start_date, end_date, _ts_to_utc_date) and preserves behavior while removing duplication.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cppa_slack_tracker/fetcher.py`:
- Around line 224-244: The date-filtering branches duplicate logic; create a
small helper (e.g., def _in_range(d, start_date, end_date): return d and
(start_date is None and d <= end_date or start_date <= d <= end_date)) and
replace the two branches by a single loop over all_messages where you compute
created_d = _ts_to_utc_date(msg.get("ts")) and edited_d =
_ts_to_utc_date((msg.get("edited") or {}).get("ts")) and append msg if
_in_range(created_d, start_date, end_date) or _in_range(edited_d, start_date,
end_date). This keeps the existing symbols (all_messages, start_date, end_date,
_ts_to_utc_date) and preserves behavior while removing duplication.
In `@cppa_slack_tracker/sync/sync_channel_user.py`:
- Around line 68-71: The logger.exception call in sync_channel_user.py is
passing the caught exception object `e` redundantly; update the except block
that surrounds the member sync (the logger.exception call referencing
`channel.channel_id`) to remove the `e` argument and only log the channel id
(e.g. logger.exception("Failed to fetch/sync members for %s",
channel.channel_id)) so the traceback is included automatically by
logger.exception.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cb6c225a-d198-4db5-8c26-052a0353fca9
📒 Files selected for processing (16)
.env.exampleconfig/settings.pycppa_slack_tracker/fetcher.pycppa_slack_tracker/management/commands/run_cppa_slack_tracker.pycppa_slack_tracker/services.pycppa_slack_tracker/sync/__init__.pycppa_slack_tracker/sync/sync_channel.pycppa_slack_tracker/sync/sync_channel_user.pycppa_slack_tracker/sync/sync_message.pycppa_slack_tracker/sync/sync_user.pycppa_slack_tracker/tests/test_sync_message.pycppa_slack_transcript_tracker/management/commands/run_cppa_slack_transcript_tracker.pycppa_slack_transcript_tracker/runner.pycppa_slack_transcript_tracker/utils/slack_listener.pyoperations/slack_ops/tokens.pyoperations/tests/test_slack_tokens.py
Collaborator
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Collaborator
Author
|
@coderabbitai review resume |
|
✅ Actions performedReviews resumed. |
Collaborator
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
config/settings.py (1)
181-188: Warn when a configured team ID has no matching token env var.Teams listed in
SLACK_TEAM_IDSbut missingSLACK_BOT_TOKEN_<team_id>are currently dropped silently, which can hide misconfiguration until runtime token lookup. Consider logging a startup warning for each missing token key.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/settings.py` around lines 181 - 188, The loop over ids_raw that builds out the out dict for SLACK_TEAM_IDS should emit a startup warning when a team id has no corresponding SLACK_BOT_TOKEN_<team_id> env var instead of silently dropping it; modify the block where key = f"SLACK_BOT_TOKEN_{tid}" and token = (env(key, default="") or "").strip() to check if token is empty and call the module/application logger (or warnings.warn) to log a clear warning referencing the missing key and tid before continuing so operators can detect misconfiguration at startup.cppa_slack_tracker/sync/sync_message.py (1)
187-189: Consider documenting the full-history fetch behavior.When
start_dateremainsNone(no DB messages), the API is called without anoldestparameter, potentially fetching the entire channel history. While this is the intended design per the docstring, consider whether this could cause rate-limiting or performance issues for channels with extensive history.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cppa_slack_tracker/sync/sync_message.py` around lines 187 - 189, The code sets start_date = _last_message_date(channel) or None which, when None, causes the API call to fetch the channel's full history; update the function docstring that contains this logic (and any public function that calls it, e.g., the sync routine around _last_message_date and start_date) to explicitly document that start_date == None triggers a full-history fetch, note potential performance/rate-limit implications, and add an inline comment next to the start_date assignment; optionally add a safety/logging measure (e.g., emit a warning via logger or enforce a configurable max_lookback) if full-history fetch is used so callers are aware and can opt into/avoid it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@config/settings.py`:
- Around line 181-188: The loop over ids_raw that builds out the out dict for
SLACK_TEAM_IDS should emit a startup warning when a team id has no corresponding
SLACK_BOT_TOKEN_<team_id> env var instead of silently dropping it; modify the
block where key = f"SLACK_BOT_TOKEN_{tid}" and token = (env(key, default="") or
"").strip() to check if token is empty and call the module/application logger
(or warnings.warn) to log a clear warning referencing the missing key and tid
before continuing so operators can detect misconfiguration at startup.
In `@cppa_slack_tracker/sync/sync_message.py`:
- Around line 187-189: The code sets start_date = _last_message_date(channel) or
None which, when None, causes the API call to fetch the channel's full history;
update the function docstring that contains this logic (and any public function
that calls it, e.g., the sync routine around _last_message_date and start_date)
to explicitly document that start_date == None triggers a full-history fetch,
note potential performance/rate-limit implications, and add an inline comment
next to the start_date assignment; optionally add a safety/logging measure
(e.g., emit a warning via logger or enforce a configurable max_lookback) if
full-history fetch is used so callers are aware and can opt into/avoid it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9c0e88ab-e5f5-49b2-b98c-433f2326bdd0
📒 Files selected for processing (16)
.env.exampleconfig/settings.pycppa_slack_tracker/fetcher.pycppa_slack_tracker/management/commands/run_cppa_slack_tracker.pycppa_slack_tracker/services.pycppa_slack_tracker/sync/__init__.pycppa_slack_tracker/sync/sync_channel.pycppa_slack_tracker/sync/sync_channel_user.pycppa_slack_tracker/sync/sync_message.pycppa_slack_tracker/sync/sync_user.pycppa_slack_tracker/tests/test_sync_message.pycppa_slack_transcript_tracker/management/commands/run_cppa_slack_transcript_tracker.pycppa_slack_transcript_tracker/runner.pycppa_slack_transcript_tracker/utils/slack_listener.pyoperations/slack_ops/tokens.pyoperations/tests/test_slack_tokens.py
leostar0412
requested review from
jonathanMLDev
and removed request for
jonathanMLDev
Collaborator
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
cppa_slack_transcript_tracker/runner.py (1)
35-36:⚠️ Potential issue | 🟠 MajorHonor explicit
bot_tokenbefore touching Django settings.At Line 35, settings are accessed unconditionally, even when
bot_tokenis already provided. That can fail in non-Django runtime contexts and block valid manual token usage.In Django, can `getattr(django.conf.settings, "SOME_KEY", None)` raise `ImproperlyConfigured` when settings are not configured?🔧 Proposed fix
- try: - team_id = getattr(settings, "SLACK_TEAM_ID", None) or None - bot_token = bot_token or get_slack_bot_token(team_id=team_id) - except ValueError: - bot_token = None + try: + if bot_token: + resolved_bot_token = bot_token + else: + try: + team_id = ( + getattr(settings, "SLACK_TEAM_ID", None) or "" + ).strip() or None + except Exception: + team_id = None + resolved_bot_token = get_slack_bot_token(team_id=team_id) + bot_token = resolved_bot_token + except ValueError: + bot_token = None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cppa_slack_transcript_tracker/runner.py` around lines 35 - 36, The code unconditionally accesses Django settings via getattr(settings, "SLACK_TEAM_ID", None) even when an explicit bot_token is provided; change the logic in runner.py so you first check if bot_token is truthy and return/use it as-is, and only call getattr(settings, "SLACK_TEAM_ID", None) and get_slack_bot_token(team_id=team_id) when bot_token is falsy/None; update the flow around the variables team_id, bot_token and the call to get_slack_bot_token to avoid touching settings when an explicit bot_token is supplied.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cppa_slack_tracker/sync/sync_channel.py`:
- Around line 81-83: The call to the Slack SDK method is using a positional arg
and will raise because conversations_info requires keyword-only parameters;
update the call in sync_channel.py where you call
get_slack_client(team_id=tid).conversations_info(channel_id) to use
conversations_info(channel=channel_id) so the method receives the channel
parameter correctly (keep get_slack_client and the tid/channel_id variables
unchanged).
In `@cppa_slack_tracker/sync/sync_message.py`:
- Around line 136-148: The merge function _merge_messages_by_ts currently uses
raw m.get("ts") for dict keys and sorting which can mix types and mis-order
values; normalize ts by coercing msg["ts"] to a consistent string key when
populating by_ts (e.g., str(msg["ts"])) and use a numeric sort key (e.g.,
float(ts) or int(...) fallback) when returning the sorted list so ordering is
numeric and stable; update code paths involving by_ts, msg and the final
sorted(..., key=...) accordingly and handle missing/invalid ts by falling back
to 0 or "" consistently.
---
Duplicate comments:
In `@cppa_slack_transcript_tracker/runner.py`:
- Around line 35-36: The code unconditionally accesses Django settings via
getattr(settings, "SLACK_TEAM_ID", None) even when an explicit bot_token is
provided; change the logic in runner.py so you first check if bot_token is
truthy and return/use it as-is, and only call getattr(settings, "SLACK_TEAM_ID",
None) and get_slack_bot_token(team_id=team_id) when bot_token is falsy/None;
update the flow around the variables team_id, bot_token and the call to
get_slack_bot_token to avoid touching settings when an explicit bot_token is
supplied.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e68d32d5-6a09-49ed-a841-0eb49d4e0d24
📒 Files selected for processing (17)
.env.exampleconfig/settings.pycppa_slack_tracker/fetcher.pycppa_slack_tracker/management/commands/run_cppa_slack_tracker.pycppa_slack_tracker/services.pycppa_slack_tracker/sync/__init__.pycppa_slack_tracker/sync/sync_channel.pycppa_slack_tracker/sync/sync_channel_user.pycppa_slack_tracker/sync/sync_message.pycppa_slack_tracker/sync/sync_user.pycppa_slack_tracker/tests/test_services.pycppa_slack_tracker/tests/test_sync_message.pycppa_slack_transcript_tracker/management/commands/run_cppa_slack_transcript_tracker.pycppa_slack_transcript_tracker/runner.pycppa_slack_transcript_tracker/utils/slack_listener.pyoperations/slack_ops/tokens.pyoperations/tests/test_slack_tokens.py
💤 Files with no reviewable changes (1)
- cppa_slack_tracker/tests/test_services.py
Collaborator
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cppa_slack_tracker/sync/sync_user.py (1)
39-55:⚠️ Potential issue | 🟠 Major
team_slugfallback can break API token resolution.At Line 54,
fetch_user_list(team_id or team_slug)passesteam_slugas_team_idwhenteam_idis missing. For API/token-scoped fetches, that value should be the Slack workspace ID, not a slug/name. This can cause avoidable sync failures.Proposed patch
def sync_users( team_slug: str, *, team_id: Optional[str] = None, include_bots: bool = True, ) -> tuple[int, int]: @@ - try: - members = fetch_user_list(team_id or team_slug) + tid = (team_id or "").strip() + if not tid: + logger.error("team_id is required for sync_users (team_slug=%s)", team_slug) + return success_count, error_count + 1 + + try: + members = fetch_user_list(tid)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cppa_slack_tracker/sync/sync_user.py` around lines 39 - 55, The code calls fetch_user_list(team_id or team_slug) which passes a slug into the _team_id parameter when team_id is None; change the call to pass explicit keyword args so the resolver gets the correct parameter: call fetch_user_list(team_id=team_id) when team_id is set, otherwise call fetch_user_list(team_slug=team_slug). Update the call site in the sync_users function (variables: team_id, team_slug, and fetch_user_list) so API token-scoped fetches receive the actual workspace ID instead of a slug.
🧹 Nitpick comments (1)
cppa_slack_transcript_tracker/runner.py (1)
35-38: Keep the token resolution error detail in logs.At Line 37,
ValueErroris swallowed and only a generic later message is emitted. Logging the exception withteam_idwill make misconfiguration debugging much faster.Proposed patch
- except ValueError: + except ValueError as e: + logger.error( + "Failed to resolve Slack bot token for team_id=%r: %s", + team_id, + e, + ) bot_token = None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cppa_slack_transcript_tracker/runner.py` around lines 35 - 38, When resolving bot_token (the call to get_slack_bot_token using team_id derived from settings.SLACK_TEAM_ID), don’t swallow the ValueError: update the except ValueError block to log the exception details and the team_id (e.g., via logger.exception or logging.exception) before setting bot_token = None so token resolution errors include the team_id and the original error for debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cppa_slack_transcript_tracker/utils/slack_listener.py`:
- Around line 55-65: Normalize the incoming bot_token by trimming whitespace
before the truthy check so whitespace-only strings don't bypass fallback
resolution: in the Slack listener constructor (where self.bot_token is set) call
bot_token = bot_token.strip() (or equivalent) and then if bot_token: set
self.bot_token = bot_token else resolve team_id and call
get_slack_bot_token(team_id=team_id); leave self.app_token assignment unchanged.
Ensure any exception handling for settings.SLACK_TEAM_ID remains intact and that
self.bot_token is never set to a whitespace-only string.
In `@operations/slack_ops/tokens.py`:
- Around line 48-64: The get_slack_bot_token path currently assumes Django
settings exist and treats failure to import settings as tokens_map=None, causing
a premature error even when SLACK_BOT_TOKEN_<tid> is present in the environment;
update the logic in operations/slack_ops/tokens.py (around the tokens_map
handling and the get_slack_bot_token flow) so that if importing
django.conf.settings fails or settings.SLACK_BOT_TOKEN is not a dict, you fall
back to checking os.environ for a key named SLACK_BOT_TOKEN_<tid> (and only
raise ValueError if neither settings.SLACK_BOT_TOKEN[tid] nor the corresponding
environment variable is found), keeping the existing logger.error message when
raising and referencing tid, tokens_map, and logger to locate the exact spots to
change.
---
Outside diff comments:
In `@cppa_slack_tracker/sync/sync_user.py`:
- Around line 39-55: The code calls fetch_user_list(team_id or team_slug) which
passes a slug into the _team_id parameter when team_id is None; change the call
to pass explicit keyword args so the resolver gets the correct parameter: call
fetch_user_list(team_id=team_id) when team_id is set, otherwise call
fetch_user_list(team_slug=team_slug). Update the call site in the sync_users
function (variables: team_id, team_slug, and fetch_user_list) so API
token-scoped fetches receive the actual workspace ID instead of a slug.
---
Nitpick comments:
In `@cppa_slack_transcript_tracker/runner.py`:
- Around line 35-38: When resolving bot_token (the call to get_slack_bot_token
using team_id derived from settings.SLACK_TEAM_ID), don’t swallow the
ValueError: update the except ValueError block to log the exception details and
the team_id (e.g., via logger.exception or logging.exception) before setting
bot_token = None so token resolution errors include the team_id and the original
error for debugging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cec89ea1-bac9-4411-b94f-864ec7c6dae0
📒 Files selected for processing (17)
.env.exampleconfig/settings.pycppa_slack_tracker/fetcher.pycppa_slack_tracker/management/commands/run_cppa_slack_tracker.pycppa_slack_tracker/services.pycppa_slack_tracker/sync/__init__.pycppa_slack_tracker/sync/sync_channel.pycppa_slack_tracker/sync/sync_channel_user.pycppa_slack_tracker/sync/sync_message.pycppa_slack_tracker/sync/sync_user.pycppa_slack_tracker/tests/test_services.pycppa_slack_tracker/tests/test_sync_message.pycppa_slack_transcript_tracker/management/commands/run_cppa_slack_transcript_tracker.pycppa_slack_transcript_tracker/runner.pycppa_slack_transcript_tracker/utils/slack_listener.pyoperations/slack_ops/tokens.pyoperations/tests/test_slack_tokens.py
💤 Files with no reviewable changes (1)
- cppa_slack_tracker/tests/test_services.py
Collaborator
Author
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cppa_slack_tracker/fetcher.py (1)
243-274:⚠️ Potential issue | 🔴 CriticalPrevent destructive membership updates when
conversations.membersfails.At Line 263, API failures
breakand return partial/empty member IDs. Downstream membership sync can treat that as authoritative and incorrectly soft-delete valid channel memberships. This should fail fast instead of returning degraded data.🛠️ Proposed fix
def fetch_channel_user_list( @@ while True: data = client.conversations_members( channel=channel_id, limit=1000, cursor=cursor, ) if not data.get("ok"): - logger.warning( - "conversations.members failed for %s: %s", - channel_id, - data.get("error", "unknown"), - ) - break + error = data.get("error", "unknown") + logger.warning("conversations.members failed for %s: %s", channel_id, error) + raise RuntimeError( + f"conversations.members failed for channel_id={channel_id}: {error}" + ) user_ids.extend(data.get("members", [])) cursor = (data.get("response_metadata") or {}).get("next_cursor") if not cursor: break return user_ids🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cppa_slack_tracker/fetcher.py` around lines 243 - 274, fetch_channel_user_list currently stops on a failed client.conversations_members call and returns whatever partial user_ids were collected, which can cause destructive downstream syncs; change the failure path in fetch_channel_user_list so that when data.get("ok") is false you raise an exception (e.g., RuntimeError or a Slack-specific error) including the channel_id and data.get("error")/response_metadata instead of breaking and returning partial results, ensuring no partial user_ids are returned to callers.
♻️ Duplicate comments (1)
cppa_slack_transcript_tracker/utils/slack_listener.py (1)
55-65:⚠️ Potential issue | 🟡 MinorTrim explicit tokens before truthiness checks.
At Line 55, whitespace-only
bot_tokenstill bypasses fallback resolution; same risk applies toapp_tokenat Line 65.🔧 Proposed fix
- if bot_token: - self.bot_token = bot_token + token = (bot_token or "").strip() + if token: + self.bot_token = token else: try: team_id = ( getattr(settings, "SLACK_TEAM_ID", None) or "" ).strip() or None except Exception: team_id = None self.bot_token = get_slack_bot_token(team_id=team_id) - self.app_token = app_token or get_slack_app_token() + self.app_token = (app_token or "").strip() or get_slack_app_token()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cppa_slack_transcript_tracker/utils/slack_listener.py` around lines 55 - 65, The code uses raw bot_token and app_token truthiness so whitespace-only strings bypass fallback; update the SlackListener initialization to trim and validate tokens before checking truthiness: for bot_token, call strip() (or normalize via token = bot_token.strip() if bot_token is not None and bot_token.strip() else None) and only use the provided value when non-empty, otherwise call get_slack_bot_token(team_id=...) (while deriving team_id from settings.SLACK_TEAM_ID as already done); do the same for app_token (use app_token.strip() and fallback to get_slack_app_token() when empty) so get_slack_bot_token and get_slack_app_token are only used when appropriate.
🧹 Nitpick comments (4)
cppa_slack_tracker/sync/sync_user.py (1)
53-55: Avoid usingteam_slugas a fallback for Slack API team resolution.Line 54 mixes slug/name fallback into a team-id API path; this can hide missing
team_idand fail token lookup indirectly. Prefer failing fast whenteam_idis absent.♻️ Proposed refactor
- try: - members = fetch_user_list(team_id or team_slug) + resolved_team_id = (team_id or "").strip() + if not resolved_team_id: + logger.error( + "team_id is required to sync users via Slack API (team_slug=%s)", + team_slug, + ) + return success_count, error_count + 1 + try: + members = fetch_user_list(resolved_team_id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cppa_slack_tracker/sync/sync_user.py` around lines 53 - 55, The code is incorrectly falling back to team_slug when calling fetch_user_list (mixing a slug into a team-id API path); change the logic in sync_user.py so fetch_user_list is only ever called with a valid team_id: validate team_id before the try/except, fail fast (raise or return an explicit error) if team_id is missing instead of using team_slug, and remove the fallback usage in the fetch_user_list call; ensure any downstream token lookup uses the validated team_id.config/settings.py (1)
192-197: Validate default team ID against token map at startup.
SLACK_TEAM_IDcan be set to a workspace that is not present inSLACK_BOT_TOKEN, which only fails later when client/token lookup runs. Consider emitting a startup warning (or raising) here for faster misconfiguration detection.♻️ Proposed improvement
SLACK_BOT_TOKEN = _slack_bot_token_from_env() SLACK_APP_TOKEN = (env("SLACK_APP_TOKEN", default="") or "").strip() # Optional: for cppa_slack_transcript_tracker (huddle transcript, token extraction) SLACK_TEAM_ID = (env("SLACK_TEAM_ID", default="") or "").strip() +if SLACK_TEAM_ID and SLACK_TEAM_ID not in SLACK_BOT_TOKEN: + import logging + logging.getLogger(__name__).warning( + "SLACK_TEAM_ID=%s is not configured in SLACK_BOT_TOKEN map. " + "Set SLACK_TEAM_IDS and SLACK_BOT_TOKEN_%s.", + SLACK_TEAM_ID, + SLACK_TEAM_ID, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/settings.py` around lines 192 - 197, After reading SLACK_TEAM_ID, validate it against the available bot-token mapping (the configured SLACK_BOT_TOKEN value or the parsed token map created from environment) and emit a clear startup warning or raise a ConfigurationError if the chosen SLACK_TEAM_ID is not present; implement this check immediately after SLACK_TEAM_ID is set in settings.py, referencing SLACK_TEAM_ID and SLACK_BOT_TOKEN (or the token map variable/function that parses SLACK_BOT_TOKEN) to confirm membership and log which team id and tokens were considered in the message.operations/slack_ops/tokens.py (1)
55-64: Separate “mapping missing” from “team missing” error paths.Right now both cases emit the same “team_id missing from map” message, which is misleading when
SLACK_BOT_TOKENitself is missing/malformed. Splitting these branches will make misconfiguration debugging faster.♻️ Proposed improvement
- if not isinstance(tokens_map, dict) or tid not in tokens_map: + if not isinstance(tokens_map, dict): + logger.error( + "SLACK_BOT_TOKEN is not configured as a team_id -> token mapping." + ) + raise ValueError( + "SLACK_BOT_TOKEN must be a dict-like mapping built from " + "SLACK_TEAM_IDS and SLACK_BOT_TOKEN_<team_id>." + ) + + if tid not in tokens_map: logger.error( "team_id %s is missing from SLACK_BOT_TOKEN. Set SLACK_TEAM_IDS and SLACK_BOT_TOKEN_%s in .env", tid, tid, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operations/slack_ops/tokens.py` around lines 55 - 64, The current check conflates a missing/malformed SLACK_BOT_TOKEN mapping and a missing team id because both branches log the same message; update the logic around tokens_map and tid so there are two clear branches: first check "if not isinstance(tokens_map, dict)" and log an error (via logger.error) indicating SLACK_BOT_TOKEN is missing or malformed and instruct to set SLACK_BOT_TOKEN_<TEAM> and SLACK_TEAM_IDS, then raise a ValueError with a message about the SLACK_BOT_TOKEN map being invalid; else (i.e., tokens_map is a dict) check "if tid not in tokens_map" and log a different error saying the specific team_id is missing and how to add SLACK_BOT_TOKEN_{tid} and SLACK_TEAM_IDS, and raise the existing ValueError referring to the missing team_id; adjust messages around the variables tokens_map, tid, logger.error and the raised ValueError to reflect these two distinct error paths.cppa_slack_tracker/sync/sync_message.py (1)
198-229: Consider adding a guardrail for first-run full-history fetches.At Line 200,
start_date=Nonetriggers a full-history API scan up toend_date. For large channels, this can be very expensive. Consider a configurable backfill window or checkpoint-based bootstrap mode to control first-run load.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cppa_slack_tracker/sync/sync_message.py` around lines 198 - 229, The current logic lets start_date=None trigger a full-history fetch via fetch_messages(channel_id, start_date, end_date, team_id=channel.team.team_id), which can be extremely expensive on first run; add a guardrail before calling fetch_messages: if start_date is None enforce a configurable backfill cap (e.g., MAX_BACKFILL_DAYS or BACKFILL_WINDOW) or switch to a checkpoint/bootstrap mode that limits the range (compute a new start_date = max(end_date - BACKFILL_WINDOW, some_minimum) or require an explicit flag to allow full backfill), and surface/log when the cap is applied (include channel_id and team_id) so that fetch_messages, the subsequent min_d/_ts_to_date logic, and downstream processing operate on the bounded window instead of the entire history.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cppa_slack_transcript_tracker/runner.py`:
- Around line 35-37: The code currently treats a whitespace-only explicit
bot_token as valid and thus skips fallback; update the normalization so that
before the fallback line (bot_token = bot_token or
get_slack_bot_token(team_id=team_id)) you strip and coerce empty/whitespace
strings to None (e.g., set bot_token = bot_token.strip() or None, then normalize
to None if empty) so get_slack_bot_token(team_id=team_id) will be called when an
explicit token is blank; apply this change around the bot_token variable in
runner.py where getattr(settings, "SLACK_TEAM_ID", None) and get_slack_bot_token
are used.
---
Outside diff comments:
In `@cppa_slack_tracker/fetcher.py`:
- Around line 243-274: fetch_channel_user_list currently stops on a failed
client.conversations_members call and returns whatever partial user_ids were
collected, which can cause destructive downstream syncs; change the failure path
in fetch_channel_user_list so that when data.get("ok") is false you raise an
exception (e.g., RuntimeError or a Slack-specific error) including the
channel_id and data.get("error")/response_metadata instead of breaking and
returning partial results, ensuring no partial user_ids are returned to callers.
---
Duplicate comments:
In `@cppa_slack_transcript_tracker/utils/slack_listener.py`:
- Around line 55-65: The code uses raw bot_token and app_token truthiness so
whitespace-only strings bypass fallback; update the SlackListener initialization
to trim and validate tokens before checking truthiness: for bot_token, call
strip() (or normalize via token = bot_token.strip() if bot_token is not None and
bot_token.strip() else None) and only use the provided value when non-empty,
otherwise call get_slack_bot_token(team_id=...) (while deriving team_id from
settings.SLACK_TEAM_ID as already done); do the same for app_token (use
app_token.strip() and fallback to get_slack_app_token() when empty) so
get_slack_bot_token and get_slack_app_token are only used when appropriate.
---
Nitpick comments:
In `@config/settings.py`:
- Around line 192-197: After reading SLACK_TEAM_ID, validate it against the
available bot-token mapping (the configured SLACK_BOT_TOKEN value or the parsed
token map created from environment) and emit a clear startup warning or raise a
ConfigurationError if the chosen SLACK_TEAM_ID is not present; implement this
check immediately after SLACK_TEAM_ID is set in settings.py, referencing
SLACK_TEAM_ID and SLACK_BOT_TOKEN (or the token map variable/function that
parses SLACK_BOT_TOKEN) to confirm membership and log which team id and tokens
were considered in the message.
In `@cppa_slack_tracker/sync/sync_message.py`:
- Around line 198-229: The current logic lets start_date=None trigger a
full-history fetch via fetch_messages(channel_id, start_date, end_date,
team_id=channel.team.team_id), which can be extremely expensive on first run;
add a guardrail before calling fetch_messages: if start_date is None enforce a
configurable backfill cap (e.g., MAX_BACKFILL_DAYS or BACKFILL_WINDOW) or switch
to a checkpoint/bootstrap mode that limits the range (compute a new start_date =
max(end_date - BACKFILL_WINDOW, some_minimum) or require an explicit flag to
allow full backfill), and surface/log when the cap is applied (include
channel_id and team_id) so that fetch_messages, the subsequent min_d/_ts_to_date
logic, and downstream processing operate on the bounded window instead of the
entire history.
In `@cppa_slack_tracker/sync/sync_user.py`:
- Around line 53-55: The code is incorrectly falling back to team_slug when
calling fetch_user_list (mixing a slug into a team-id API path); change the
logic in sync_user.py so fetch_user_list is only ever called with a valid
team_id: validate team_id before the try/except, fail fast (raise or return an
explicit error) if team_id is missing instead of using team_slug, and remove the
fallback usage in the fetch_user_list call; ensure any downstream token lookup
uses the validated team_id.
In `@operations/slack_ops/tokens.py`:
- Around line 55-64: The current check conflates a missing/malformed
SLACK_BOT_TOKEN mapping and a missing team id because both branches log the same
message; update the logic around tokens_map and tid so there are two clear
branches: first check "if not isinstance(tokens_map, dict)" and log an error
(via logger.error) indicating SLACK_BOT_TOKEN is missing or malformed and
instruct to set SLACK_BOT_TOKEN_<TEAM> and SLACK_TEAM_IDS, then raise a
ValueError with a message about the SLACK_BOT_TOKEN map being invalid; else
(i.e., tokens_map is a dict) check "if tid not in tokens_map" and log a
different error saying the specific team_id is missing and how to add
SLACK_BOT_TOKEN_{tid} and SLACK_TEAM_IDS, and raise the existing ValueError
referring to the missing team_id; adjust messages around the variables
tokens_map, tid, logger.error and the raised ValueError to reflect these two
distinct error paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a99de737-1d00-4d42-a0be-1f27907820d8
📒 Files selected for processing (17)
.env.exampleconfig/settings.pycppa_slack_tracker/fetcher.pycppa_slack_tracker/management/commands/run_cppa_slack_tracker.pycppa_slack_tracker/services.pycppa_slack_tracker/sync/__init__.pycppa_slack_tracker/sync/sync_channel.pycppa_slack_tracker/sync/sync_channel_user.pycppa_slack_tracker/sync/sync_message.pycppa_slack_tracker/sync/sync_user.pycppa_slack_tracker/tests/test_services.pycppa_slack_tracker/tests/test_sync_message.pycppa_slack_transcript_tracker/management/commands/run_cppa_slack_transcript_tracker.pycppa_slack_transcript_tracker/runner.pycppa_slack_transcript_tracker/utils/slack_listener.pyoperations/slack_ops/tokens.pyoperations/tests/test_slack_tokens.py
💤 Files with no reviewable changes (1)
- cppa_slack_tracker/tests/test_services.py
Collaborator
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cppa_slack_transcript_tracker/utils/slack_listener.py`:
- Around line 59-64: Remove the broad try/except that masks Django configuration
errors around the SLACK_TEAM_ID lookup; instead let errors propagate by
replacing the try/except block with a direct assignment using the same
expression: set team_id = (getattr(settings, "SLACK_TEAM_ID", None) or
"").strip() or None so ImproperlyConfigured (or other exceptions) from
getattr(settings, ...) are not swallowed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: dc586eae-74e0-4706-b3b2-4b8be948c0f1
📒 Files selected for processing (17)
.env.exampleconfig/settings.pycppa_slack_tracker/fetcher.pycppa_slack_tracker/management/commands/run_cppa_slack_tracker.pycppa_slack_tracker/services.pycppa_slack_tracker/sync/__init__.pycppa_slack_tracker/sync/sync_channel.pycppa_slack_tracker/sync/sync_channel_user.pycppa_slack_tracker/sync/sync_message.pycppa_slack_tracker/sync/sync_user.pycppa_slack_tracker/tests/test_services.pycppa_slack_tracker/tests/test_sync_message.pycppa_slack_transcript_tracker/management/commands/run_cppa_slack_transcript_tracker.pycppa_slack_transcript_tracker/runner.pycppa_slack_transcript_tracker/utils/slack_listener.pyoperations/slack_ops/tokens.pyoperations/tests/test_slack_tokens.py
💤 Files with no reviewable changes (1)
- cppa_slack_tracker/tests/test_services.py
Collaborator
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cppa_slack_transcript_tracker/utils/slack_listener.py (1)
54-70:⚠️ Potential issue | 🟡 MinorNormalize explicit
app_tokentoo.A whitespace-only
app_tokenstill bypassesget_slack_app_token()and theif not self.app_tokenguard, so the listener can be initialized with an invalid Socket Mode token.💡 Proposed fix
def __init__(self, bot_token=None, app_token=None): token = (bot_token or "").strip() if token: self.bot_token = token else: try: team_id = ( getattr(settings, "SLACK_TEAM_ID", None) or "" ).strip() or None except Exception: team_id = None self.bot_token = get_slack_bot_token(team_id=team_id) - self.app_token = app_token or get_slack_app_token() + app_token = (app_token or "").strip() + self.app_token = app_token or get_slack_app_token()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cppa_slack_transcript_tracker/utils/slack_listener.py` around lines 54 - 70, The constructor (__init__) currently treats a whitespace-only app_token as valid; normalize app_token like bot_token by stripping whitespace and falling back to get_slack_app_token() when the provided value is empty/whitespace. Specifically, change the logic around self.app_token so you set token_app = (app_token or "").strip() and if token_app assign self.app_token = token_app else call get_slack_app_token(); keep the existing ValueError guard for self.app_token unchanged. Use the existing symbols __init__, self.app_token, and get_slack_app_token to locate and update the code.
🧹 Nitpick comments (2)
cppa_slack_transcript_tracker/management/commands/run_cppa_slack_transcript_tracker.py (1)
33-34: Normalizeteam_idfor consistency with other callers.The runner.py (line 35) strips whitespace from
SLACK_TEAM_IDbefore use, but this command does not. A whitespace-only value would bypass fallback and potentially cause issues downstream.♻️ Proposed fix for consistency
- team_id = getattr(settings, "SLACK_TEAM_ID", None) or None + team_id = (getattr(settings, "SLACK_TEAM_ID", None) or "").strip() or None bot_token = get_slack_bot_token(team_id=team_id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cppa_slack_transcript_tracker/management/commands/run_cppa_slack_transcript_tracker.py` around lines 33 - 34, Normalize SLACK_TEAM_ID the same way as runner.py by stripping whitespace and treating empty/whitespace-only strings as None before calling get_slack_bot_token: replace the current team_id assignment (team_id = getattr(settings, "SLACK_TEAM_ID", None) or None) with logic that reads settings.SLACK_TEAM_ID, strips it (e.g., .strip()), and sets team_id to None if the stripped result is empty; keep the call to get_slack_bot_token(team_id=team_id) unchanged.cppa_slack_tracker/sync/sync_user.py (1)
54-68: Minor logging inconsistency.Line 60 uses
tid(stripped) for the API call, but line 66 logsteam_id(original parameter). If a whitespace-paddedteam_idis passed, the log would show the original value whiletid(the stripped version) is actually used.♻️ Proposed fix for consistent logging
logger.exception( "Failed to fetch users for team_slug=%s team_id=%s", team_slug, - team_id, + tid, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cppa_slack_tracker/sync/sync_user.py` around lines 54 - 68, The logs use the unstripped parameter team_id while the code calls fetch_user_list with the stripped tid; update the logging to reference tid for consistency. In the block that checks tid and the except block around fetch_user_list(tid), change logger.error and logger.exception messages to include tid (the stripped value) instead of team_id so the logged ID matches the value used in fetch_user_list; keep team_slug as-is. Ensure references to tid appear in both the "team_id is required" error and the "Failed to fetch users" exception log.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cppa_slack_tracker/fetcher.py`:
- Around line 200-208: The Slack API call in fetch_messages is missing the
inclusive flag so messages exactly at oldest_ts are excluded; update the call in
fetch_messages to include inclusive=True in the kwargs passed to
client.conversations_history, and then modify the
SlackAPIClient.conversations_history method signature to accept an inclusive
parameter and forward it to the underlying Slack API request so the API call
uses the inclusive option. Ensure the parameter name is inclusive (boolean) and
thread it through both the caller (fetch_messages) and the implementation in
SlackAPIClient.conversations_history.
In `@cppa_slack_tracker/management/commands/run_cppa_slack_tracker.py`:
- Around line 192-198: The dry-run logging in handle() currently omits the
presence of a supplied --messages-json even though the default (no flags) path
will call sync_messages() on a real run; update the dry-run logging block that
prints channel_id/start/end (the logger.info lines) to also detect
options.get("messages_json") and log that messages would be imported (e.g. "
messages_json=%s") so the dry-run shows this side effect; locate the default
branch in run_cppa_slack_tracker.py where handle() decides "Would run: sync
users, channels, and messages (default)" and add the additional logger.info for
messages_json (or otherwise ensure the dry-run prints that sync_messages would
be invoked) without changing sync_messages() behavior.
---
Outside diff comments:
In `@cppa_slack_transcript_tracker/utils/slack_listener.py`:
- Around line 54-70: The constructor (__init__) currently treats a
whitespace-only app_token as valid; normalize app_token like bot_token by
stripping whitespace and falling back to get_slack_app_token() when the provided
value is empty/whitespace. Specifically, change the logic around self.app_token
so you set token_app = (app_token or "").strip() and if token_app assign
self.app_token = token_app else call get_slack_app_token(); keep the existing
ValueError guard for self.app_token unchanged. Use the existing symbols
__init__, self.app_token, and get_slack_app_token to locate and update the code.
---
Nitpick comments:
In `@cppa_slack_tracker/sync/sync_user.py`:
- Around line 54-68: The logs use the unstripped parameter team_id while the
code calls fetch_user_list with the stripped tid; update the logging to
reference tid for consistency. In the block that checks tid and the except block
around fetch_user_list(tid), change logger.error and logger.exception messages
to include tid (the stripped value) instead of team_id so the logged ID matches
the value used in fetch_user_list; keep team_slug as-is. Ensure references to
tid appear in both the "team_id is required" error and the "Failed to fetch
users" exception log.
In
`@cppa_slack_transcript_tracker/management/commands/run_cppa_slack_transcript_tracker.py`:
- Around line 33-34: Normalize SLACK_TEAM_ID the same way as runner.py by
stripping whitespace and treating empty/whitespace-only strings as None before
calling get_slack_bot_token: replace the current team_id assignment (team_id =
getattr(settings, "SLACK_TEAM_ID", None) or None) with logic that reads
settings.SLACK_TEAM_ID, strips it (e.g., .strip()), and sets team_id to None if
the stripped result is empty; keep the call to
get_slack_bot_token(team_id=team_id) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4293706d-1958-40ce-910d-fe4de86d7ae2
📒 Files selected for processing (17)
.env.exampleconfig/settings.pycppa_slack_tracker/fetcher.pycppa_slack_tracker/management/commands/run_cppa_slack_tracker.pycppa_slack_tracker/services.pycppa_slack_tracker/sync/__init__.pycppa_slack_tracker/sync/sync_channel.pycppa_slack_tracker/sync/sync_channel_user.pycppa_slack_tracker/sync/sync_message.pycppa_slack_tracker/sync/sync_user.pycppa_slack_tracker/tests/test_services.pycppa_slack_tracker/tests/test_sync_message.pycppa_slack_transcript_tracker/management/commands/run_cppa_slack_transcript_tracker.pycppa_slack_transcript_tracker/runner.pycppa_slack_transcript_tracker/utils/slack_listener.pyoperations/slack_ops/tokens.pyoperations/tests/test_slack_tokens.py
💤 Files with no reviewable changes (1)
- cppa_slack_tracker/tests/test_services.py
Collaborator
Author
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cppa_slack_tracker/sync/sync_channel_user.py (1)
68-72: Consider removing redundant exception object fromlogger.exception.The
eargument is redundant sincelogger.exception()automatically includes the exception traceback.♻️ Proposed fix
except Exception as e: logger.exception( - "Failed to fetch/sync members for %s: %s", channel.channel_id, e + "Failed to fetch/sync members for %s", channel.channel_id )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cppa_slack_tracker/sync/sync_channel_user.py` around lines 68 - 72, In the except block inside sync_channel_user.py, remove the redundant exception argument from the logger.exception call so the traceback is logged automatically; change the call from logger.exception("Failed to fetch/sync members for %s: %s", channel.channel_id, e) to logger.exception("Failed to fetch/sync members for %s", channel.channel_id) and leave error_count += 1 unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cppa_slack_tracker/sync/sync_channel_user.py`:
- Around line 68-72: In the except block inside sync_channel_user.py, remove the
redundant exception argument from the logger.exception call so the traceback is
logged automatically; change the call from logger.exception("Failed to
fetch/sync members for %s: %s", channel.channel_id, e) to
logger.exception("Failed to fetch/sync members for %s", channel.channel_id) and
leave error_count += 1 unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: db0833da-f260-4954-bff9-6a49e0e19b38
📒 Files selected for processing (17)
.env.exampleconfig/settings.pycppa_slack_tracker/fetcher.pycppa_slack_tracker/management/commands/run_cppa_slack_tracker.pycppa_slack_tracker/services.pycppa_slack_tracker/sync/__init__.pycppa_slack_tracker/sync/sync_channel.pycppa_slack_tracker/sync/sync_channel_user.pycppa_slack_tracker/sync/sync_message.pycppa_slack_tracker/sync/sync_user.pycppa_slack_tracker/tests/test_services.pycppa_slack_tracker/tests/test_sync_message.pycppa_slack_transcript_tracker/management/commands/run_cppa_slack_transcript_tracker.pycppa_slack_transcript_tracker/runner.pycppa_slack_transcript_tracker/utils/slack_listener.pyoperations/slack_ops/tokens.pyoperations/tests/test_slack_tokens.py
💤 Files with no reviewable changes (1)
- cppa_slack_tracker/tests/test_services.py
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
3 participants
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.
Summary
Adds team_id-based Slack bot token resolution so the app can use different bot tokens per Slack workspace. Config, token helpers, and the CPPA Slack tracker are updated to use
team_id, and tests are aligned with the new API and missing-team_idbehavior.Fixes #91
What changed
Config & environment
config/settings.py—SLACK_BOT_TOKENis now a dict (team_id→ token) built from:SLACK_TEAM_IDS(comma-separated)SLACK_BOT_TOKEN_<team_id>per team.env.example— DocumentsSLACK_TEAM_IDSandSLACK_BOT_TOKEN_<team_id>for multi-workspace setupToken API (
operations/slack_ops/tokens.py)get_slack_bot_token(team_id)—team_idis required; raisesValueErrorwhen missing or when the team has no tokenget_slack_client(team_id=...)— When no explicit token is passed, usesget_slack_bot_token(team_id)so callers must supplyteam_idCPPA Slack tracker
team_id(from channel/workspace or options) and callget_slack_client(team_id=...)sync_channel,sync_channel_user,sync_message,sync_user) — Signatures and call sites updated to passteam_idthrough the pipelineTests
operations/tests/test_slack_tokens.py— Tests updated for the new API; new test ensures missing/emptyteam_idraises with a clear messagecppa_slack_tracker/tests/test_sync_message.py— Additional coverage (e.g. merge-by-ts behavior)Summary by CodeRabbit
New Features
Bug Fixes
Logging
Tests
Documentation