Implement Slack member count sync with rate-limit handling#1560
Implement Slack member count sync with rate-limit handling#1560shdwcodr wants to merge 1 commit intoOWASP:mainfrom
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Summary by CodeRabbit
Summary by CodeRabbit
WalkthroughThe Slack data sync command was refactored for modularity and improved error handling, introducing helper methods for Slack API calls with rate limit handling. The logic for fetching conversations and members was moved into dedicated methods. The Changes
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested labels
Suggested reviewers
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
backend/apps/slack/management/commands/slack_sync_data.py (1)
84-95:⚠️ Potential issueCritical: Remove merge artifacts.
The code contains merge artifacts (branch names) that break the syntax.
Remove lines 84 and 95 which contain branch names:
- ) - slack-member-count-sync - - - if members: - Member.bulk_save(members) - - # Update the workspace with the total members count. - workspace.total_members_count = total_members - workspace.save(update_fields=["total_members_count"]) - - self.stdout.write(self.style.SUCCESS(f"Populated {total_members} members")) - main - + )🧰 Tools
🪛 Ruff (0.11.9)
84-84: SyntaxError: Expected
exceptorfinallyaftertryblock
87-87: SyntaxError: Unexpected indentation
95-95: SyntaxError: unindent does not match any outer indentation level
🪛 Pylint (3.3.7)
[error] 84-84: Parsing failed: 'expected 'except' or 'finally' block (apps.slack.management.commands.slack_sync_data, line 84)'
(E0001)
🧹 Nitpick comments (1)
backend/apps/slack/migrations/0010_alter_workspace_slack_workspace_id.py (1)
9-9: Consider using double quotes for consistency.The migration logic is correct. However, if your project follows a style guide that prefers double quotes, consider updating the string literals.
- ('slack', '0009_rename_channel_conversation_and_more'), + ("slack", "0009_rename_channel_conversation_and_more"),- model_name='workspace', - name='slack_workspace_id', - field=models.CharField(max_length=100, unique=True, verbose_name='Workspace ID'), + model_name="workspace", + name="slack_workspace_id", + field=models.CharField(max_length=100, unique=True, verbose_name="Workspace ID"),Also applies to: 14-16
🧰 Tools
🪛 Ruff (0.11.9)
9-9: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
9-9: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/apps/slack/management/commands/slack_sync_data.py(4 hunks)backend/apps/slack/migrations/0010_alter_workspace_slack_workspace_id.py(1 hunks)backend/apps/slack/models/workspace.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/apps/slack/management/commands/slack_sync_data.py (2)
backend/apps/slack/models/conversation.py (3)
Conversation(11-90)update_data(68-90)bulk_save(63-65)backend/apps/slack/models/member.py (3)
update_data(66-78)bulk_save(61-63)Member(10-78)
🪛 Ruff (0.11.9)
backend/apps/slack/migrations/0010_alter_workspace_slack_workspace_id.py
9-9: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
9-9: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
14-14: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
15-15: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
16-16: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
backend/apps/slack/models/workspace.py
18-18: SyntaxError: Unexpected indentation
23-23: SyntaxError: Unexpected indentation
backend/apps/slack/management/commands/slack_sync_data.py
84-84: SyntaxError: Expected except or finally after try block
95-95: SyntaxError: unindent does not match any outer indentation level
97-97: SyntaxError: Unexpected indentation
104-104: Line too long (102 > 99)
(E501)
107-107: Line too long (112 > 99)
(E501)
119-119: SyntaxError: unindent does not match any outer indentation level
119-119: SyntaxError: Expected a statement
119-119: SyntaxError: Expected a statement
119-120: SyntaxError: Expected an expression
120-120: SyntaxError: Unexpected indentation
122-122: SyntaxError: unindent does not match any outer indentation level
🪛 Pylint (3.3.7)
backend/apps/slack/migrations/0010_alter_workspace_slack_workspace_id.py
[refactor] 6-6: Too few public methods (0/2)
(R0903)
backend/apps/slack/models/workspace.py
[error] 18-18: Parsing failed: 'unexpected indent (apps.slack.models.workspace, line 18)'
(E0001)
backend/apps/slack/management/commands/slack_sync_data.py
[error] 84-84: Parsing failed: 'expected 'except' or 'finally' block (apps.slack.management.commands.slack_sync_data, line 84)'
(E0001)
🔇 Additional comments (1)
backend/apps/slack/models/workspace.py (1)
47-48:⚠️ Potential issueFix syntax error in replace function.
Missing space after comma in the
replacefunction call.- env_key = f"SLACK_BOT_TOKEN_{self.slack_workspace_id.upper().replace('-','_')}" + env_key = f"SLACK_BOT_TOKEN_{self.slack_workspace_id.upper().replace('-', '_')}"Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
backend/apps/slack/management/commands/slack_sync_data.py (3)
99-100: Verify member count persistence in database.Ensure the
Conversation.update_datamethod properly handles thenum_membersfield and maps it to the model'stotal_members_countfield for database persistence.#!/bin/bash # Verify if Conversation.update_data handles num_members field ast-grep --pattern 'def update_data($_, $_) -> $_: $$$ }' # Check if total_members_count is set in the method rg -A 10 "def update_data" backend/apps/slack/models/conversation.py
46-66:⚠️ Potential issueCritical: Duplicate retry counter increment still exists.
The retry counter is incremented twice - once inside the rate limit handling block (line 60) and again outside the exception handling (line 63), causing premature retry failure.
Apply this fix to prevent double incrementing:
- except SlackApiError as e: - if e.response.status_code == 429: # Rate limit hit - retry_after = int(e.response.headers.get("Retry-After", 1)) - self.stdout.write(self.style.WARNING(f"Rate limited. Sleeping {retry_after}s...")) - time.sleep(retry_after) - retries += 1 - else: - raise e - retries += 1 - backoff = 2 ** retries + random.random() - time.sleep(backoff) + except SlackApiError as e: + if e.response.status_code == 429: # Rate limit hit + retry_after = int(e.response.headers.get("Retry-After", 1)) + self.stdout.write(self.style.WARNING(f"Rate limited. Sleeping {retry_after}s...")) + time.sleep(retry_after) + retries += 1 + else: + raise e + else: + # Exponential backoff for other transient errors + retries += 1 + backoff = 2 ** retries + random.random() + time.sleep(backoff)🧰 Tools
🪛 Ruff (0.11.9)
47-47: First line of docstring should be in imperative mood: "Handles Slack API calls with rate limit retries."
(D401)
53-53: Abstract
raiseto an inner function(TRY301)
53-53: Avoid specifying long messages outside the exception class
(TRY003)
53-53: Exception must not use an f-string literal, assign to variable first
Assign to variable; remove f-string literal
(EM102)
54-54: Consider moving this statement to an
elseblock(TRY300)
56-56: Magic value used in comparison, consider replacing
429with a constant variable(PLR2004)
58-58: Line too long (102 > 99)
(E501)
62-62: Use
raisewithout specifying exception nameRemove exception name
(TRY201)
64-64: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
66-66: Avoid specifying long messages outside the exception class
(TRY003)
66-66: Exception must not use a string literal, assign to variable first
Assign to variable; remove string literal
(EM101)
84-97: 🛠️ Refactor suggestionOptimization not implemented: Still making individual API calls for member counts.
The code continues to make individual
conversations.infocalls for each conversation, which will quickly hit rate limits with ~824 channels. Theconversations.listendpoint already includesnum_membersfor most conversation types.Use the existing
num_membersfromconversations.listresponse:- for conversation_data in response["channels"]: - if "num_members" not in conversation_data: - try: - info = self._call_slack_api( - client.conversations_info, - channel=conversation_data["id"], - include_num_members=True, - ) - conversation_data["num_members"] = info["channel"].get("num_members") - except SlackApiError as e: - self.stdout.write( - self.style.WARNING(f"Failed to get member count for {conversation_data['id']}: {e}") - ) - conversation_data["num_members"] = None + for conversation_data in response["channels"]: + # conversations.list already includes num_members for most types + if "num_members" not in conversation_data: + # Only fallback for types without num_members (e.g., DMs) + try: + info = self._call_slack_api( + client.conversations_info, + channel=conversation_data["id"], + include_num_members=True, + ) + conversation_data["num_members"] = info["channel"].get("num_members") + except SlackApiError as e: + self.stdout.write( + self.style.WARNING(f"Failed to get member count for {conversation_data['id']}: {e}") + ) + conversation_data["num_members"] = None🧰 Tools
🪛 Ruff (0.11.9)
95-95: Line too long (116 > 99)
(E501)
🧹 Nitpick comments (1)
backend/apps/slack/management/commands/slack_sync_data.py (1)
20-21: Improve argument documentation.The help text removal makes the command less user-friendly. Consider adding descriptive help text for better usability.
- parser.add_argument("--batch-size", type=int, default=1000) - parser.add_argument("--delay", type=float, default=0.5) + parser.add_argument("--batch-size", type=int, default=1000, help="Number of items to fetch per API call") + parser.add_argument("--delay", type=float, default=0.5, help="Delay in seconds between paginated requests")
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/apps/slack/management/commands/slack_sync_data.py(3 hunks)backend/apps/slack/migrations/0014_merge_20250609_1446.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.11.9)
backend/apps/slack/management/commands/slack_sync_data.py
47-47: First line of docstring should be in imperative mood: "Handles Slack API calls with rate limit retries."
(D401)
53-53: Abstract raise to an inner function
(TRY301)
53-53: Avoid specifying long messages outside the exception class
(TRY003)
53-53: Exception must not use an f-string literal, assign to variable first
Assign to variable; remove f-string literal
(EM102)
54-54: Consider moving this statement to an else block
(TRY300)
56-56: Magic value used in comparison, consider replacing 429 with a constant variable
(PLR2004)
58-58: Line too long (102 > 99)
(E501)
62-62: Use raise without specifying exception name
Remove exception name
(TRY201)
64-64: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
66-66: Avoid specifying long messages outside the exception class
(TRY003)
66-66: Exception must not use a string literal, assign to variable first
Assign to variable; remove string literal
(EM101)
95-95: Line too long (116 > 99)
(E501)
backend/apps/slack/migrations/0014_merge_20250609_1446.py
9-9: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
9-9: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
10-10: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
10-10: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
🪛 Pylint (3.3.7)
backend/apps/slack/migrations/0014_merge_20250609_1446.py
[refactor] 6-6: Too few public methods (0/2)
(R0903)
🔇 Additional comments (3)
backend/apps/slack/migrations/0014_merge_20250609_1446.py (1)
1-15: LGTM! Standard Django merge migration.This merge migration correctly resolves the migration history between two branches and follows Django conventions.
🧰 Tools
🪛 Ruff (0.11.9)
9-9: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
9-9: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
10-10: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
10-10: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
🪛 Pylint (3.3.7)
[refactor] 6-6: Too few public methods (0/2)
(R0903)
backend/apps/slack/management/commands/slack_sync_data.py (2)
3-3: Good addition for exponential backoff jitter.The
randomimport supports the jitter implementation in the retry logic.
9-9: Inconsistency: Workspace model still imported despite summary claiming removal.The AI summary states the
Workspacemodel was removed entirely, but the code still imports and uses it extensively (lines 27, 33, 35, etc.).Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
backend/apps/slack/management/commands/slack_sync_data.py (3)
101-102: Ensure member counts are persisted to database (related to previous feedback).The fetched
num_membersdata needs to be saved to the database. As previously noted, verify that theConversation.update_dataorfrom_slackmethod properly handles thenum_membersfield and maps it to thetotal_members_countdatabase field.#!/bin/bash # Verify if update_data method handles num_members properly ast-grep --pattern 'def update_data($_, $_) -> $_: $$$ }' # Check if from_slack method was updated to handle num_members rg -A 10 "def from_slack" backend/apps/slack/models/conversation.py
46-68:⚠️ Potential issueFix duplicate retry counter increment (duplicate issue).
The retry counter is still being incremented twice - once inside the rate limit handling and once at the end of the loop, causing premature retry failure.
Apply the previously suggested fix:
if e.response.status_code == 429: # Rate limit hit retry_after = int(e.response.headers.get("Retry-After", 1)) self.stdout.write(self.style.WARNING(f"Rate limited. Sleeping {retry_after}s...")) time.sleep(retry_after) retries += 1 else: raise e - retries += 1 - backoff = 2 ** retries + random.random() - time.sleep(backoff) + else: + # Exponential backoff for other transient errors + retries += 1 + backoff = 2 ** retries + random.random() + time.sleep(backoff)🧰 Tools
🪛 Ruff (0.11.9)
47-47: First line of docstring should be in imperative mood: "Handles Slack API calls with rate limit retries."
(D401)
53-53: Abstract
raiseto an inner function(TRY301)
53-53: Avoid specifying long messages outside the exception class
(TRY003)
53-53: Exception must not use an f-string literal, assign to variable first
Assign to variable; remove f-string literal
(EM102)
54-54: Consider moving this statement to an
elseblock(TRY300)
56-56: Magic value used in comparison, consider replacing
429with a constant variable(PLR2004)
58-58: Line too long (102 > 99)
(E501)
62-62: Use
raisewithout specifying exception nameRemove exception name
(TRY201)
66-66: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
68-68: Avoid specifying long messages outside the exception class
(TRY003)
68-68: Exception must not use a string literal, assign to variable first
Assign to variable; remove string literal
(EM101)
86-99: 🛠️ Refactor suggestionOptimize member count fetching to reduce API calls (duplicate suggestion).
The code still makes individual
conversations.infocalls for each conversation. As previously suggested, use thenum_membersfield directly from theconversations.listresponse when available.The previously suggested optimization remains valid:
- for conversation_data in response["channels"]: - if "num_members" not in conversation_data: - try: - info = self._call_slack_api( - client.conversations_info, - channel=conversation_data["id"], - include_num_members=True, - ) - conversation_data["num_members"] = info["channel"].get("num_members") - except SlackApiError as e: - self.stdout.write( - self.style.WARNING(f"Failed to get member count for {conversation_data['id']}: {e}") - ) - conversation_data["num_members"] = None + for conversation_data in response["channels"]: + # Use member count from conversations.list when available + if "num_members" not in conversation_data: + # Fallback for types without num_members (e.g., DMs) + try: + info = self._call_slack_api( + client.conversations_info, + channel=conversation_data["id"], + include_num_members=True, + ) + conversation_data["num_members"] = info["channel"].get("num_members") + except SlackApiError as e: + self.stdout.write( + self.style.WARNING(f"Failed to get member count for {conversation_data['id']}: {e}") + ) + conversation_data["num_members"] = None🧰 Tools
🪛 Ruff (0.11.9)
97-97: Line too long (116 > 99)
(E501)
🧹 Nitpick comments (1)
backend/apps/slack/management/commands/slack_sync_data.py (1)
47-47: Address static analysis issues for better code quality.Several static analysis issues should be addressed:
- Fix docstring to use imperative mood
- Address line length violations
- Consider using constants for magic numbers
- def _call_slack_api(self, func, *args, **kwargs): - """Handles Slack API calls with rate limit retries.""" + def _call_slack_api(self, func, *args, **kwargs): + """Handle Slack API calls with rate limit retries."""For line length issues, consider breaking long lines:
- self.stdout.write(self.style.WARNING(f"Rate limited. Sleeping {retry_after}s...")) + self.stdout.write( + self.style.WARNING(f"Rate limited. Sleeping {retry_after}s...") + )- self.stdout.write( - self.style.WARNING(f"Failed to get member count for {conversation_data['id']}: {e}") - ) + self.stdout.write( + self.style.WARNING( + f"Failed to get member count for {conversation_data['id']}: {e}" + ) + )Also applies to: 58-58, 97-97
🧰 Tools
🪛 Ruff (0.11.9)
47-47: First line of docstring should be in imperative mood: "Handles Slack API calls with rate limit retries."
(D401)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/apps/slack/management/commands/slack_sync_data.py(3 hunks)
🧰 Additional context used
🪛 Ruff (0.11.9)
backend/apps/slack/management/commands/slack_sync_data.py
47-47: First line of docstring should be in imperative mood: "Handles Slack API calls with rate limit retries."
(D401)
53-53: Abstract raise to an inner function
(TRY301)
53-53: Avoid specifying long messages outside the exception class
(TRY003)
53-53: Exception must not use an f-string literal, assign to variable first
Assign to variable; remove f-string literal
(EM102)
54-54: Consider moving this statement to an else block
(TRY300)
56-56: Magic value used in comparison, consider replacing 429 with a constant variable
(PLR2004)
58-58: Line too long (102 > 99)
(E501)
62-62: Use raise without specifying exception name
Remove exception name
(TRY201)
66-66: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
68-68: Avoid specifying long messages outside the exception class
(TRY003)
68-68: Exception must not use a string literal, assign to variable first
Assign to variable; remove string literal
(EM101)
97-97: Line too long (116 > 99)
(E501)
🔇 Additional comments (3)
backend/apps/slack/management/commands/slack_sync_data.py (3)
3-3: LGTM! Import and constant are correctly implemented.The
randomimport is appropriately used for retry jitter in the backoff algorithm, and theMAX_RETRIESconstant improves code maintainability.Also applies to: 13-14
41-44: Verify Workspace model usage - inconsistency with summary.The modular approach with helper methods improves code organization. However, the AI summary states that the
Workspacemodel was removed, but the code still references it extensively.#!/bin/bash # Check if Workspace model still exists and is being used rg -A 5 "class Workspace" backend/apps/slack/models/ rg -n "from.*Workspace\|import.*Workspace" backend/apps/slack/Likely an incorrect or invalid review comment.
116-145: LGTM! Well-structured member fetching implementation.The method properly handles pagination, error handling, and bulk operations. The consistent pattern with
_fetch_conversationsimproves code maintainability.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/apps/slack/management/commands/slack_sync_data.py (1)
46-68:⚠️ Potential issueCritical: Fix duplicate retry counter increment and improve error handling.
This method has several issues that need to be addressed:
- Duplicate retry increment: The retry counter is incremented on both line 60 (rate limit case) and line 63 (general case), causing premature failure.
- Magic number: The HTTP status code 429 should be a named constant.
- Exception handling: Multiple style violations with exception creation and raising.
+# Add constant at module level +HTTP_TOO_MANY_REQUESTS = 429 + def _call_slack_api(self, func, *args, **kwargs): - """Handles Slack API calls with rate limit retries.""" + """Handle Slack API calls with rate limit retries.""" retries = 0 while retries < MAX_RETRIES: try: response = func(*args, **kwargs) if not response.get("ok", False): - raise SlackApiError(f"{func.__name__} failed", response) + error_msg = f"{func.__name__} failed" + raise SlackApiError(error_msg, response) return response except SlackApiError as e: - if e.response.status_code == 429: # Rate limit hit + if e.response.status_code == HTTP_TOO_MANY_REQUESTS: retry_after = int(e.response.headers.get("Retry-After", 1)) - self.stdout.write(self.style.WARNING(f"Rate limited. Sleeping {retry_after}s...")) + warning_msg = f"Rate limited. Sleeping {retry_after}s..." + self.stdout.write(self.style.WARNING(warning_msg)) time.sleep(retry_after) retries += 1 else: - raise e - retries += 1 - #This randomness is non-crypto and only used for retry jitter. - # nosec - backoff = 2 ** retries + random.random() - time.sleep(backoff) - raise SlackApiError("Max retries exceeded", {}) + raise + continue # Skip the general retry logic for rate limits + + # General retry logic (only for non-SlackApiError exceptions) + retries += 1 + # This randomness is non-crypto and only used for retry jitter. + # nosec + backoff = 2 ** retries + random.random() + time.sleep(backoff) + + error_msg = "Max retries exceeded" + raise SlackApiError(error_msg, {})🧰 Tools
🪛 Ruff (0.11.9)
47-47: First line of docstring should be in imperative mood: "Handles Slack API calls with rate limit retries."
(D401)
53-53: Abstract
raiseto an inner function(TRY301)
53-53: Avoid specifying long messages outside the exception class
(TRY003)
53-53: Exception must not use an f-string literal, assign to variable first
Assign to variable; remove f-string literal
(EM102)
54-54: Consider moving this statement to an
elseblock(TRY300)
56-56: Magic value used in comparison, consider replacing
429with a constant variable(PLR2004)
58-58: Line too long (102 > 99)
(E501)
62-62: Use
raisewithout specifying exception nameRemove exception name
(TRY201)
66-66: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
68-68: Avoid specifying long messages outside the exception class
(TRY003)
68-68: Exception must not use a string literal, assign to variable first
Assign to variable; remove string literal
(EM101)
🧹 Nitpick comments (2)
backend/apps/slack/management/commands/slack_sync_data.py (2)
20-21: Fix line length violations while keeping the helpful descriptions.The help text additions are great for usability, but these lines exceed the 99 character limit.
- parser.add_argument("--batch-size", type=int, default=1000, help="Number of items to fetch per API request") - parser.add_argument("--delay", type=float, default=0.5, help="Delay in seconds between paginated requests") + parser.add_argument( + "--batch-size", type=int, default=1000, + help="Number of items to fetch per API request" + ) + parser.add_argument( + "--delay", type=float, default=0.5, + help="Delay in seconds between paginated requests" + )🧰 Tools
🪛 Ruff (0.11.9)
20-20: Line too long (116 > 99)
(E501)
21-21: Line too long (115 > 99)
(E501)
70-115: Verify the necessity of num_members fallback calls and fix line length.The implementation correctly adds a fallback for missing
num_members, but this should rarely be needed sinceconversations.listtypically includes this field for public/private channels.Consider adding logging to track when the fallback is actually used:
for conversation_data in response["channels"]: if "num_members" not in conversation_data: + self.stdout.write( + self.style.WARNING( + f"Missing num_members for {conversation_data.get('name', 'unknown')} " + f"(type: {conversation_data.get('conversation_type', 'unknown')})" + ) + ) try: info = self._call_slack_api( client.conversations_info, channel=conversation_data["id"], include_num_members=True, ) conversation_data["num_members"] = info["channel"].get("num_members") except SlackApiError as e: - self.stdout.write( - self.style.WARNING(f"Failed to get member count for {conversation_data['id']}: {e}") - ) + warning_msg = ( + f"Failed to get member count for {conversation_data['id']}: {e}" + ) + self.stdout.write(self.style.WARNING(warning_msg)) conversation_data["num_members"] = NoneThis will help verify when the fallback logic is actually needed and identify any patterns in missing data.
🧰 Tools
🪛 Ruff (0.11.9)
97-97: Line too long (116 > 99)
(E501)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/apps/slack/management/commands/slack_sync_data.py(3 hunks)
🧰 Additional context used
🪛 Ruff (0.11.9)
backend/apps/slack/management/commands/slack_sync_data.py
20-20: Line too long (116 > 99)
(E501)
21-21: Line too long (115 > 99)
(E501)
47-47: First line of docstring should be in imperative mood: "Handles Slack API calls with rate limit retries."
(D401)
53-53: Abstract raise to an inner function
(TRY301)
53-53: Avoid specifying long messages outside the exception class
(TRY003)
53-53: Exception must not use an f-string literal, assign to variable first
Assign to variable; remove f-string literal
(EM102)
54-54: Consider moving this statement to an else block
(TRY300)
56-56: Magic value used in comparison, consider replacing 429 with a constant variable
(PLR2004)
58-58: Line too long (102 > 99)
(E501)
62-62: Use raise without specifying exception name
Remove exception name
(TRY201)
66-66: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
68-68: Avoid specifying long messages outside the exception class
(TRY003)
68-68: Exception must not use a string literal, assign to variable first
Assign to variable; remove string literal
(EM101)
97-97: Line too long (116 > 99)
(E501)
🔇 Additional comments (3)
backend/apps/slack/management/commands/slack_sync_data.py (3)
1-14: LGTM! Good addition of retry constants and imports.The new
randomimport andMAX_RETRIESconstant support the improved retry logic with jitter, which is a best practice for API rate limiting.🧰 Tools
🪛 Ruff (0.11.9)
1-1: Missing docstring in public module
(D100)
23-44: Excellent refactoring for better maintainability.The extraction of conversation and member fetching into dedicated methods greatly improves code organization and readability. The error handling for missing workspaces and bot tokens is also well-implemented.
116-146: Well-implemented member fetching with efficient pagination.The method correctly implements cursor-based pagination, uses bulk operations for performance, and provides clear progress feedback. The error handling is appropriate and doesn't interrupt the overall sync process.
shdwcodr
force-pushed
the
slack-member-count-sync
branch
from
f7bd019 to
359f054
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
backend/apps/slack/management/commands/slack_sync_data.py (3)
21-22: Add help text back to CLI arguments for better user experience.The removal of help text makes the command less user-friendly and self-documenting.
49-69:⚠️ Potential issueFix duplicate retry counter increment and style issues.
The retry counter is incremented twice, causing premature failure of the retry mechanism. Additionally, address multiple style violations for better code quality.
def _call_slack_api(self, func, *args, **kwargs): - """Handles Slack API calls with rate limit retries.""" + """Handle Slack API calls with rate limit retries.""" + HTTP_TOO_MANY_REQUESTS = 429 retries = 0 while retries < MAX_RETRIES: try: response = func(*args, **kwargs) if not response.get("ok", False): - raise SlackApiError(f"{func.__name__} failed", response) + error_msg = f"{func.__name__} failed" + raise SlackApiError(error_msg, response) return response except SlackApiError as e: - if e.response.status_code == 429: # Rate limit hit + if e.response.status_code == HTTP_TOO_MANY_REQUESTS: retry_after = int(e.response.headers.get("Retry-After", 1)) - self.stdout.write(self.style.WARNING(f"Rate limited. Sleeping {retry_after}s...")) + warning_msg = f"Rate limited. Sleeping {retry_after}s..." + self.stdout.write(self.style.WARNING(warning_msg)) time.sleep(retry_after) retries += 1 else: - raise e - retries += 1 - backoff = 2 ** retries + random.random() - time.sleep(backoff) - raise SlackApiError("Max retries exceeded", {}) + raise + else: + # Exponential backoff for other transient errors + retries += 1 + backoff = 2 ** retries + random.random() + time.sleep(backoff) + error_msg = "Max retries exceeded" + raise SlackApiError(error_msg, {})🧰 Tools
🪛 Ruff (0.11.9)
50-50: First line of docstring should be in imperative mood: "Handles Slack API calls with rate limit retries."
(D401)
56-56: Abstract
raiseto an inner function(TRY301)
56-56: Avoid specifying long messages outside the exception class
(TRY003)
56-56: Exception must not use an f-string literal, assign to variable first
Assign to variable; remove f-string literal
(EM102)
57-57: Consider moving this statement to an
elseblock(TRY300)
59-59: Magic value used in comparison, consider replacing
429with a constant variable(PLR2004)
61-61: Line too long (102 > 99)
(E501)
65-65: Use
raisewithout specifying exception nameRemove exception name
(TRY201)
67-67: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
69-69: Avoid specifying long messages outside the exception class
(TRY003)
69-69: Exception must not use a string literal, assign to variable first
Assign to variable; remove string literal
(EM101)
102-103:⚠️ Potential issueEnsure fetched member counts are persisted to the database.
The member counts are being fetched but not saved to the database because the
Conversation.from_slackmethod doesn't handle thenum_membersfield.The
from_slackmethod inbackend/apps/slack/models/conversation.pyneeds to be updated to settotal_members_countfrom thenum_membersdata.
🧹 Nitpick comments (2)
backend/apps/slack/management/commands/slack_sync_data.py (2)
36-37: Fix line length violation.Break the long line for better readability and compliance with style guidelines.
- # Fallback to environment variable if DB token is missing - bot_token = getattr(workspace, "bot_token", None) or os.environ.get("DJANGO_SLACK_BOT_TOKEN") + # Fallback to environment variable if DB token is missing + bot_token = ( + getattr(workspace, "bot_token", None) + or os.environ.get("DJANGO_SLACK_BOT_TOKEN") + )🧰 Tools
🪛 Ruff (0.11.9)
37-37: Line too long (105 > 99)
(E501)
87-100: Fix line length violations and verify member count persistence.Several lines exceed the 99-character limit and need formatting fixes.
for conversation_data in response["channels"]: if "num_members" not in conversation_data: try: info = self._call_slack_api( client.conversations_info, channel=conversation_data["id"], include_num_members=True, ) - conversation_data["num_members"] = info["channel"].get("num_members") + conversation_data["num_members"] = info["channel"].get( + "num_members" + ) except SlackApiError as e: - self.stdout.write( - self.style.WARNING(f"Failed to get member count for {conversation_data['id']}: {e}") - ) + warning_msg = ( + f"Failed to get member count for " + f"{conversation_data['id']}: {e}" + ) + self.stdout.write(self.style.WARNING(warning_msg)) conversation_data["num_members"] = None🧰 Tools
🪛 Ruff (0.11.9)
98-98: Line too long (116 > 99)
(E501)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/apps/slack/management/commands/slack_sync_data.py(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/apps/slack/management/commands/slack_sync_data.py (2)
backend/apps/slack/models/conversation.py (3)
Conversation(11-90)update_data(68-90)bulk_save(63-65)backend/apps/slack/models/member.py (3)
update_data(66-78)bulk_save(61-63)Member(10-78)
🪛 Ruff (0.11.9)
backend/apps/slack/management/commands/slack_sync_data.py
37-37: Line too long (105 > 99)
(E501)
50-50: First line of docstring should be in imperative mood: "Handles Slack API calls with rate limit retries."
(D401)
56-56: Abstract raise to an inner function
(TRY301)
56-56: Avoid specifying long messages outside the exception class
(TRY003)
56-56: Exception must not use an f-string literal, assign to variable first
Assign to variable; remove f-string literal
(EM102)
57-57: Consider moving this statement to an else block
(TRY300)
59-59: Magic value used in comparison, consider replacing 429 with a constant variable
(PLR2004)
61-61: Line too long (102 > 99)
(E501)
65-65: Use raise without specifying exception name
Remove exception name
(TRY201)
67-67: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
69-69: Avoid specifying long messages outside the exception class
(TRY003)
69-69: Exception must not use a string literal, assign to variable first
Assign to variable; remove string literal
(EM101)
98-98: Line too long (116 > 99)
(E501)
🔇 Additional comments (1)
backend/apps/slack/management/commands/slack_sync_data.py (1)
117-146: Well-structured member fetching implementation.The method correctly implements cursor-based pagination and bulk saving. The use of the walrus operator in the list comprehension is clean and modern.
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
I don't think this code works. It also has some unrelated breaking changes. Also no test.
Sorry, mentors are busy w/ GSoC and don't have time to guide contributors w/ detailed implementation.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/apps/slack/management/commands/slack_sync_data.py (1)
50-70: Retry counter is still incremented twice
See earlier review – the doubleretries += 1causes the loop to terminate after ⌈MAX_RETRIES / 2⌉ attempts.@@ - retries += 1 else: raise e - retries += 1 + # Exponential back-off for non-rate-limit errors or after waiting + retries += 1🧰 Tools
🪛 Ruff (0.11.9)
51-51: First line of docstring should be in imperative mood: "Handles Slack API calls with rate limit retries."
(D401)
57-57: Abstract
raiseto an inner function(TRY301)
57-57: Avoid specifying long messages outside the exception class
(TRY003)
57-57: Exception must not use an f-string literal, assign to variable first
Assign to variable; remove f-string literal
(EM102)
58-58: Consider moving this statement to an
elseblock(TRY300)
60-60: Magic value used in comparison, consider replacing
429with a constant variable(PLR2004)
62-62: Line too long (102 > 99)
(E501)
66-66: Use
raisewithout specifying exception nameRemove exception name
(TRY201)
68-68: Comment contains ambiguous
–(EN DASH). Did you mean-(HYPHEN-MINUS)?(RUF003)
70-70: Avoid specifying long messages outside the exception class
(TRY003)
70-70: Exception must not use a string literal, assign to variable first
Assign to variable; remove string literal
(EM101)
🧹 Nitpick comments (2)
backend/apps/slack/management/commands/slack_sync_data.py (2)
3-4: Drop unusedrandomimport
randomis imported but never referenced. Remove to keep the module clean.-import random🧰 Tools
🪛 Ruff (0.11.9)
3-3:
randomimported but unusedRemove unused import:
random(F401)
21-23: Restore help text for CLI argumentsThe previous review already pointed out that removing
help=strings degrades UX.
Adding them is trivial and entirely backwards-compatible.- parser.add_argument("--batch-size", type=int, default=1000) - parser.add_argument("--delay", type=float, default=0.5) + parser.add_argument("--batch-size", type=int, default=1000, + help="Max items per Slack API page") + parser.add_argument("--delay", type=float, default=0.5, + help="Seconds to sleep between paginated requests")
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/apps/slack/management/commands/slack_sync_data.py(3 hunks)
🧰 Additional context used
🪛 Ruff (0.11.9)
backend/apps/slack/management/commands/slack_sync_data.py
3-3: random imported but unused
Remove unused import: random
(F401)
38-38: Line too long (105 > 99)
(E501)
51-51: First line of docstring should be in imperative mood: "Handles Slack API calls with rate limit retries."
(D401)
57-57: Abstract raise to an inner function
(TRY301)
57-57: Avoid specifying long messages outside the exception class
(TRY003)
57-57: Exception must not use an f-string literal, assign to variable first
Assign to variable; remove f-string literal
(EM102)
58-58: Consider moving this statement to an else block
(TRY300)
60-60: Magic value used in comparison, consider replacing 429 with a constant variable
(PLR2004)
62-62: Line too long (102 > 99)
(E501)
66-66: Use raise without specifying exception name
Remove exception name
(TRY201)
68-68: Comment contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF003)
70-70: Avoid specifying long messages outside the exception class
(TRY003)
70-70: Exception must not use a string literal, assign to variable first
Assign to variable; remove string literal
(EM101)
99-99: Line too long (116 > 99)
(E501)
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
backend/apps/slack/management/commands/slack_sync_data.py (2)
18-20: CLI flags lost their help text – restore usability
(This was raised earlier and is still present.)
47-66:⚠️ Potential issueRetry logic: double-increment & traceback loss
retriesis incremented inside the 429 branch (line 60) and again unconditionally at line 63 → premature “max retries exceeded”.raise ediscards the original traceback; use bareraise.Fix:
- retries += 1 - else: - raise e - retries += 1 + retries += 1 + else: + raise + # single increment per loop-iteration + retries += 1🧰 Tools
🪛 Ruff (0.11.9)
53-53: Abstract
raiseto an inner function(TRY301)
53-53: Avoid specifying long messages outside the exception class
(TRY003)
53-53: Exception must not use an f-string literal, assign to variable first
Assign to variable; remove f-string literal
(EM102)
54-54: Consider moving this statement to an
elseblock(TRY300)
56-56: Magic value used in comparison, consider replacing
429with a constant variable(PLR2004)
58-58: Line too long (102 > 99)
(E501)
62-62: Use
raisewithout specifying exception nameRemove exception name
(TRY201)
64-64: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
66-66: Avoid specifying long messages outside the exception class
(TRY003)
66-66: Exception must not use a string literal, assign to variable first
Assign to variable; remove string literal
(EM101)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/apps/slack/management/commands/slack_sync_data.py(2 hunks)backend/apps/slack/migrations/0014_remove_workspace_total_members_count.py(1 hunks)backend/apps/slack/tests/test_slack_sync_data.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/apps/slack/management/commands/slack_sync_data.py (2)
backend/apps/slack/models/conversation.py (3)
Conversation(11-90)update_data(68-90)bulk_save(63-65)backend/apps/slack/models/member.py (3)
Member(10-78)update_data(66-78)bulk_save(61-63)
🪛 Ruff (0.11.9)
backend/apps/slack/tests/test_slack_sync_data.py
1-1: File backend/apps/slack/tests/test_slack_sync_data.py is part of an implicit namespace package. Add an __init__.py.
(INP001)
57-57: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
57-57: Boolean positional value in function call
(FBT003)
backend/apps/slack/management/commands/slack_sync_data.py
34-34: Line too long (119 > 99)
(E501)
53-53: Abstract raise to an inner function
(TRY301)
53-53: Avoid specifying long messages outside the exception class
(TRY003)
53-53: Exception must not use an f-string literal, assign to variable first
Assign to variable; remove f-string literal
(EM102)
54-54: Consider moving this statement to an else block
(TRY300)
56-56: Magic value used in comparison, consider replacing 429 with a constant variable
(PLR2004)
58-58: Line too long (102 > 99)
(E501)
62-62: Use raise without specifying exception name
Remove exception name
(TRY201)
64-64: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
66-66: Avoid specifying long messages outside the exception class
(TRY003)
66-66: Exception must not use a string literal, assign to variable first
Assign to variable; remove string literal
(EM101)
94-94: Line too long (116 > 99)
(E501)
backend/apps/slack/migrations/0014_remove_workspace_total_members_count.py
9-9: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
9-9: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
14-14: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
15-15: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
🪛 Pylint (3.3.7)
backend/apps/slack/migrations/0014_remove_workspace_total_members_count.py
[refactor] 6-6: Too few public methods (0/2)
(R0903)
🔇 Additional comments (2)
backend/apps/slack/migrations/0014_remove_workspace_total_members_count.py (1)
8-17: Check migration ordering – thisRemoveFieldmay reference a soon-to-be-deleted modelIf the subsequent migration removes the entire
Workspacemodel, thisRemoveFieldwill become a no-op (or error) when run after that deletion.
Confirm the migration graph so that the model is still present at the point this migration executes, or merge the field removal into the model-deletion migration to avoid brittle intermediate states.🧰 Tools
🪛 Ruff (0.11.9)
9-9: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
9-9: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
14-14: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
15-15: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
backend/apps/slack/management/commands/slack_sync_data.py (1)
34-37: Single env var fallback may mix tokens across multiple workspaces
DJANGO_SLACK_BOT_TOKENis global; when syncing several workspaces you could accidentally use the same token for all. Prefer a per-workspace env var convention (e.g.DJANGO_SLACK_BOT_TOKEN_<WORKSPACE_ID>), or fail fast whenworkspace.bot_tokenis empty.🧰 Tools
🪛 Ruff (0.11.9)
34-34: Line too long (119 > 99)
(E501)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
backend/apps/slack/management/commands/slack_sync_data.py (2)
18-20: Add argument help-texts for better CLI UX.Help strings were removed but are still missing. Retain discoverability by restoring them.
118-136: Duplicate DB writes for members – passsave=Falsetoupdate_data.As flagged earlier, every
Member.update_datacall saves immediately and you save again viabulk_save, doubling traffic.- member - for member_data in response["members"] - if (member := Member.update_data(member_data, workspace)) + member + for member_data in response["members"] + if ( + member := Member.update_data( + member_data, workspace, save=False + ) + )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/apps/slack/management/commands/slack_sync_data.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/apps/slack/management/commands/slack_sync_data.py (2)
backend/apps/slack/models/conversation.py (3)
Conversation(11-90)update_data(68-90)bulk_save(63-65)backend/apps/slack/models/member.py (3)
Member(10-78)update_data(66-78)bulk_save(61-63)
🪛 Ruff (0.11.9)
backend/apps/slack/management/commands/slack_sync_data.py
34-34: Line too long (119 > 99)
(E501)
53-53: Abstract raise to an inner function
(TRY301)
53-53: Avoid specifying long messages outside the exception class
(TRY003)
53-53: Exception must not use an f-string literal, assign to variable first
Assign to variable; remove f-string literal
(EM102)
54-54: Consider moving this statement to an else block
(TRY300)
56-56: Magic value used in comparison, consider replacing 429 with a constant variable
(PLR2004)
58-58: Line too long (102 > 99)
(E501)
62-62: Use raise without specifying exception name
Remove exception name
(TRY201)
64-64: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
66-66: Avoid specifying long messages outside the exception class
(TRY003)
66-66: Exception must not use a string literal, assign to variable first
Assign to variable; remove string literal
(EM101)
94-94: Line too long (116 > 99)
(E501)
🔇 Additional comments (1)
backend/apps/slack/management/commands/slack_sync_data.py (1)
34-37: Per-workspace token fallback may leak data between workspaces.All workspaces fall back to the single
DJANGO_SLACK_BOT_TOKENvalue. If multiple workspaces are processed, the same token will be reused and you may end up writing data from workspace A into workspace B (or simply get 403 errors).Please confirm that:
- You never run the command with >1 workspace when env-fallback is used, or
- You provide distinct per-workspace env vars (e.g.
DJANGO_SLACK_BOT_TOKEN_<WS_ID>).
shdwcodr
force-pushed
the
slack-member-count-sync
branch
from
432db49 to
ec85936
Compare
|
2 participants
Resolves #1518
Hey team 👋
This PR fully implements syncing Slack channel member counts via
conversations.infoand integrates that data cleanly into our system.I’ve tested it on a workspace with 848 channels and over 38,000+ members, and the sync now runs completely end-to-end — no missing data, no partial syncs, no crashes.
✅ What’s Done
num_membersfor every Slack channeltest_slack_sync_data.py) usingunittest.mockto fully simulate Slack responses — CI-ready🧠 Notes
Thanks for the support!
— shdwcodr