Gathering slack messages from conversations #1513
Conversation
Summary by CodeRabbit
Summary by CodeRabbit
WalkthroughA new Django model for Slack messages was introduced, along with corresponding admin registration, migration, and utility methods for bulk saving and updating from data. Two management commands were implemented for syncing Slack workspace data and messages, with comprehensive unit tests for both commands and the new model. The Makefile was updated to run both sync commands. Changes
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes were found. Suggested reviewers
✨ Finishing Touches
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
Documentation and Community
|
Dishant1804
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (7)
backend/apps/slack/models/message.py (4)
3-4: Update deprecated type imports.The
typing.Listimport is deprecated in favor of the built-inlisttype for Python 3.9+.-from typing import List +from typing import TYPE_CHECKING + +if TYPE_CHECKING: + from typing import AnyNote: Since
listis now preferred for type annotations, we can use it directly in the type hints below.🧰 Tools
🪛 Ruff (0.11.9)
4-4:
typing.Listis deprecated, uselistinstead(UP035)
24-28: Replace magic number with a constant.The magic number
50should be extracted to a named constant for better maintainability.+# Constants +TEXT_PREVIEW_LENGTH = 50 + class Message(TimestampedModel): # ... existing code ... def __str__(self): """Human readable representation.""" - text_preview = self.text[:50] + "..." if len(self.text) > 50 else self.text + text_preview = self.text[:TEXT_PREVIEW_LENGTH] + "..." if len(self.text) > TEXT_PREVIEW_LENGTH else self.text return f"{text_preview}"🧰 Tools
🪛 Ruff (0.11.9)
26-26: Magic value used in comparison, consider replacing
50with a constant variable(PLR2004)
50-50: Update type annotation to use modern syntax.Replace deprecated
Listwith built-inlisttype annotation.- def bulk_save(messages: List['Message'], fields=None) -> None: + def bulk_save(messages: list['Message'], fields=None) -> None:🧰 Tools
🪛 Ruff (0.11.9)
50-50: Use
listinstead ofListfor type annotationReplace with
list(UP006)
50-50: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
79-79: Use datetime.UTC alias.The
timezone.utccan be replaced with the more moderndatetime.UTCalias.- message.timestamp = datetime.fromtimestamp(float(retrieved_timestamp), tz=timezone.utc) + message.timestamp = datetime.fromtimestamp(float(retrieved_timestamp), tz=datetime.UTC)🧰 Tools
🪛 Ruff (0.11.9)
79-79: Use
datetime.UTCaliasConvert to
datetime.UTCalias(UP017)
backend/apps/slack/management/commands/slack_sync_messages.py (3)
126-126: Update type annotation to modern syntax.Use the modern union type annotation syntax.
- def _create_message_from_data(self, message_data: dict, conversation: Conversation) -> Optional[Message]: + def _create_message_from_data(self, message_data: dict, conversation: Conversation) -> Message | None:🧰 Tools
🪛 Ruff (0.11.9)
126-126: Use
X | Yfor type annotationsConvert to
X | Y(UP007)
126-126: Line too long (109 > 99)
(E501)
149-149: Use lazy logging formatting.Avoid f-strings in logging statements for better performance.
- logger.warning(f"Invalid message data, missing {e}: {message_data}") + logger.warning("Invalid message data, missing %s: %s", e, message_data)🧰 Tools
🪛 Ruff (0.11.9)
149-149: Logging statement uses f-string
(G004)
85-97: Consider adding error handling for latest timestamp.The
latestparameter might cause issues if the timestamp format is incorrect. Consider validation or error handling.last_message = ( Message.objects.filter(conversation=conversation) .order_by("-timestamp") .first() ) - latest = last_message.timestamp if last_message else None + latest = None + if last_message and last_message.timestamp: + try: + # Convert to Slack timestamp format if needed + latest = last_message.timestamp.timestamp() + except (AttributeError, ValueError): + latest = None🧰 Tools
🪛 Ruff (0.11.9)
85-85: Line too long (107 > 99)
(E501)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
backend/Makefile(1 hunks)backend/apps/slack/admin.py(3 hunks)backend/apps/slack/management/commands/slack_sync_messages.py(1 hunks)backend/apps/slack/migrations/0010_message.py(1 hunks)backend/apps/slack/models/__init__.py(1 hunks)backend/apps/slack/models/conversation.py(1 hunks)backend/apps/slack/models/message.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
backend/apps/slack/admin.py (1)
backend/apps/slack/models/message.py (1)
Message(9-84)
backend/apps/slack/models/__init__.py (1)
backend/apps/slack/models/message.py (1)
Message(9-84)
backend/apps/slack/management/commands/slack_sync_messages.py (3)
backend/apps/slack/models/conversation.py (3)
Conversation(11-90)bulk_save(63-65)update_data(68-90)backend/apps/slack/models/message.py (3)
Message(9-84)bulk_save(50-52)update_data(55-84)backend/apps/slack/models/workspace.py (2)
Workspace(10-32)bot_token(25-32)
🪛 Ruff (0.11.9)
backend/apps/slack/models/message.py
4-4: typing.List is deprecated, use list instead
(UP035)
11-11: Blank line contains whitespace
Remove whitespace from blank line
(W293)
17-17: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
21-21: Avoid using null=True on string-based fields such as CharField
(DJ001)
26-26: Magic value used in comparison, consider replacing 50 with a constant variable
(PLR2004)
43-43: Blank line contains whitespace
Remove whitespace from blank line
(W293)
47-47: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
50-50: Use list instead of List for type annotation
Replace with list
(UP006)
50-50: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
55-55: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
57-57: Blank line contains whitespace
Remove whitespace from blank line
(W293)
79-79: Use datetime.UTC alias
Convert to datetime.UTC alias
(UP017)
84-84: No newline at end of file
Add trailing newline
(W292)
backend/apps/slack/management/commands/slack_sync_messages.py
85-85: Line too long (107 > 99)
(E501)
104-104: Use a list comprehension to create a transformed list
(PERF401)
118-118: Line too long (109 > 99)
(E501)
123-123: Line too long (108 > 99)
(E501)
126-126: Use X | Y for type annotations
Convert to X | Y
(UP007)
126-126: Line too long (109 > 99)
(E501)
127-127: No blank lines allowed after function docstring (found 1)
Remove blank line(s) after function docstring
(D202)
132-132: Blank line contains whitespace
Remove whitespace from blank line
(W293)
147-147: Blank line contains whitespace
Remove whitespace from blank line
(W293)
149-149: Logging statement uses f-string
(G004)
158-158: Blank line contains whitespace
Remove whitespace from blank line
(W293)
158-158: No newline at end of file
Add trailing newline
(W292)
backend/apps/slack/migrations/0010_message.py
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)
15-15: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
17-17: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
17-17: Line too long (117 > 99)
(E501)
17-17: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
18-18: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
19-19: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
20-20: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
20-20: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
20-20: Line too long (100 > 99)
(E501)
21-21: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
21-21: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
21-21: Line too long (103 > 99)
(E501)
22-22: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
22-22: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
23-23: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
23-23: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
23-23: Line too long (111 > 99)
(E501)
24-24: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
24-24: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
24-24: Line too long (109 > 99)
(E501)
25-25: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
25-25: Line too long (147 > 99)
(E501)
25-25: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
25-25: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
28-28: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
28-28: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
29-29: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
29-29: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
30-30: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
30-30: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
30-30: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
🪛 GitHub Actions: Run CI/CD
backend/apps/slack/models/message.py
[error] 23-23: Ruff DJ001: Avoid using null=True on string-based fields such as CharField.
[error] 28-63: Ruff PLR2004: Magic value used in comparison, consider replacing 50 with a constant variable.
[error] 59-59: Ruff W293: Blank line contains whitespace.
backend/apps/slack/management/commands/slack_sync_messages.py
[error] 84-100: Ruff E501: Line too long (107 > 99).
[error] 103-103: Ruff PERF401: Use a list comprehension to create a transformed list instead of appending in a for loop.
[error] 117-117: Ruff E501: Line too long (109 > 99).
[error] 122-122: Ruff E501: Line too long (108 > 99).
[error] 125-125: Ruff E501: Line too long (106 > 99).
[error] 147-147: Ruff G004: Logging statement uses f-string; consider using lazy formatting.
backend/apps/slack/migrations/0010_message.py
[error] 17-17: Ruff E501: Line too long (117 > 99).
[error] 20-20: Ruff E501: Line too long (100 > 99).
[error] 21-21: Ruff E501: Line too long (103 > 99).
[error] 23-23: Ruff E501: Line too long (111 > 99).
[error] 24-24: Ruff E501: Line too long (109 > 99).
[error] 25-25: Ruff E501: Line too long (147 > 99).
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (4)
backend/apps/slack/models/conversation.py (1)
1-1: LGTM: Documentation improvement.Good change to align the docstring with the actual model name "Conversation" rather than "Channel".
backend/apps/slack/models/__init__.py (1)
4-4: LGTM: Standard model import pattern.The Message import follows the existing pattern and properly exposes the new model from the package.
backend/Makefile (1)
151-153: LGTM: Consistent Makefile target pattern.The new
slack-sync-messagestarget follows the established patterns and naming conventions in the Makefile.backend/apps/slack/admin.py (1)
8-8: LGTM: Standard admin model registration.The Message model import and registration follow Django admin best practices.
Also applies to: 145-145
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
backend/tests/slack/models/message_test.py (2)
7-7: Replace magic number constant with descriptive name or literal value.
CONSTANT_2 = 2is not descriptive and doesn't add value. Consider using the literal2directly in tests or a more descriptive name likeEXPECTED_REPLY_COUNT.-CONSTANT_2 = 2Then update line 73:
- assert result.reply_count == CONSTANT_2 + assert result.reply_count == 2
138-138: Simplify method binding in test.The manual method binding
Message.get_thread_messages.__get__(mock_parent, Message)is overly complex for a test. Consider testing the method directly on a real Message instance or mocking more simply.- mock_parent.get_thread_messages = Message.get_thread_messages.__get__(mock_parent, Message) + # Create a real Message instance and patch the queryset + message = Message( + conversation=mock_conversation, + slack_message_id="123456.000", + thread_timestamp="123456.000" + )backend/tests/slack/commands/management/slack_sync_data_test.py (1)
13-17: Consider consolidating test constants.Multiple token constants could be organized better for maintainability.
-CONSTANT_2 = 2 -CONSTANT_3 = 3 -TEST_TOKEN = "xoxb-test-token" # noqa: S105 -TEST_TOKEN_1 = "xoxb-token-1" # noqa: S105 -TEST_TOKEN_2 = "xoxb-token-2" # noqa: S105 +# Test constants +EXPECTED_API_CALLS = 2 +EXPECTED_ITEMS_COUNT = 3 +TEST_TOKENS = { + "default": "xoxb-test-token", # noqa: S105 + "workspace1": "xoxb-token-1", # noqa: S105 + "workspace2": "xoxb-token-2", # noqa: S105 +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/Makefile(1 hunks)backend/tests/slack/commands/management/slack_sync_data_test.py(1 hunks)backend/tests/slack/models/message_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/Makefile
🧰 Additional context used
🪛 GitHub Actions: Run CI/CD
backend/tests/slack/commands/management/slack_sync_data_test.py
[error] 136-136: CSpell: Unknown word 'UBOT' found during spelling check.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (5)
backend/tests/slack/models/message_test.py (2)
162-169: LGTM! Clean string representation test.The test properly verifies both short and long text truncation scenarios for the
__str__method.
42-44:✅ Verification successful
Verify timestamp conversion logic.
The test converts a string timestamp to datetime, but ensure this matches the actual model implementation. The timestamp format suggests it might be a Unix timestamp as a string.
🏁 Script executed:
#!/bin/bash # Description: Check how timestamp is handled in the Message model ast-grep --pattern $'class Message($$$) { $$$ def update_data($$$) { $$$ timestamp = $$$ $$$ } $$$ }'Length of output: 122
🏁 Script executed:
#!/bin/bash # Locate Message model file(s) and inspect timestamp parsing logic files=$(fd --type f --strip-cwd-prefix message.py) if [ -z "$files" ]; then echo "No message.py files found" exit 0 fi for file in $files; do echo "=== Inspecting $file ===" rg -A20 "class Message" -n "$file" rg -n "thread_ts|ts|timestamp" -n "$file" doneLength of output: 2732
🏁 Script executed:
#!/bin/bash # Show the context around the timestamp test in message_test.py sed -n '1,200p' backend/tests/slack/models/message_test.py | sed -n '1,100p'Length of output: 3565
No changes needed—timestamp conversion is correct
The
Message.update_datamethod parses the string timestamp viamessage.timestamp = datetime.fromtimestamp(float(retrieved_timestamp), tz=UTC)and the tests assert against
datetime.fromtimestamp(1605000000, tz=UTC)so the implementation and test are in sync.
backend/tests/slack/commands/management/slack_sync_data_test.py (3)
215-265: LGTM! Comprehensive API interaction test.The test thoroughly verifies:
- Correct API client initialization
- Proper pagination handling with cursors
- Correct parameter passing to Slack API
- Model update and bulk save operations
- Output verification
This is a well-structured integration test that covers the happy path comprehensively.
294-334: Excellent error handling test coverage.The test properly verifies that Slack API errors are handled gracefully, with appropriate error messages logged. The mock setup correctly simulates a SlackApiError scenario.
374-410: Good edge case coverage for None returns.This test ensures the command handles gracefully when
update_datamethods returnNone, preventing bulk save operations with empty data. This is important for data integrity.
backend/tests/slack/commands/management/slack_sync_data_test.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
backend/tests/slack/commands/management/slack_sync_data_test.py (2)
13-14: Remove unnecessary constant abstractions.The constants
CONSTANT_2andCONSTANT_3add unnecessary indirection. Using literal values2and3directly in the assertions would be more readable and clearer.-CONSTANT_2 = 2 -CONSTANT_3 = 3Then replace usages throughout the file:
- assert mock_client.conversations_list.call_count == CONSTANT_2 + assert mock_client.conversations_list.call_count == 2
569-569: Add missing newline at end of file.The file is missing a trailing newline, which is a common convention in Python files.
assert "Processing workspace: Workspace 2" in output +
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/tests/slack/commands/management/slack_sync_data_test.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
- GitHub Check: Run backend tests
🔇 Additional comments (4)
backend/tests/slack/commands/management/slack_sync_data_test.py (4)
136-136: Spelling issue resolved.Good fix! Changing from "UBOT123" to "BOT123" addresses the CI spelling check failure mentioned in the previous review. The new ID is still appropriate for testing bot functionality.
160-273: Well-structured comprehensive test with good mock usage.This test method effectively covers the happy path scenario with proper mocking of Slack API responses, pagination handling, and verification of all expected calls and data processing. The use of side_effect for simulating multiple API responses is particularly well done.
294-334: Good error handling test coverage.This test properly verifies that Slack API errors are handled gracefully, with appropriate error messages logged while still processing other successful operations (conversations in this case).
512-568: Excellent multi-workspace test coverage.This test ensures the command properly handles multiple workspaces by creating separate Slack clients for each workspace and processing data independently. The verification of call counts and token usage is thorough.
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
backend/apps/slack/management/commands/slack_sync_messages.py (1)
224-224:⚠️ Potential issueAdd missing newline at end of file.
The file is missing a trailing newline.
Add a newline character at the end of line 224.
♻️ Duplicate comments (1)
backend/apps/slack/management/commands/slack_sync_messages.py (1)
218-223: 🛠️ Refactor suggestionImprove error handling to include the actual error message.
The error handling could be more informative by including the actual error from the Slack response.
Apply this diff to improve error handling:
def _handle_slack_response(self, response, api_method): """Handle Slack API response and raise exception if needed.""" if not response["ok"]: - error_message = f"{api_method} API call failed" + error_message = f"{api_method} API call failed: {response.get('error', 'Unknown error')}" logger.error(error_message) self.stdout.write(self.style.ERROR(error_message))
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/apps/slack/admin.py(3 hunks)backend/apps/slack/management/commands/slack_sync_messages.py(1 hunks)backend/apps/slack/migrations/0014_message.py(1 hunks)backend/apps/slack/models/message.py(1 hunks)backend/tests/slack/commands/management/slack_sync_messages_test.py(1 hunks)backend/tests/slack/models/message_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/apps/slack/admin.py
- backend/tests/slack/models/message_test.py
- backend/apps/slack/models/message.py
🧰 Additional context used
🪛 Ruff (0.11.9)
backend/tests/slack/commands/management/slack_sync_messages_test.py
549-549: Local variable mock_bulk_save is assigned to but never used
Remove assignment to unused variable mock_bulk_save
(F841)
638-638: No newline at end of file
Add trailing newline
(W292)
backend/apps/slack/migrations/0014_message.py
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)
15-15: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
17-17: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
17-17: Line too long (117 > 99)
(E501)
17-17: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
18-18: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
19-19: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
20-20: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
20-20: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
21-21: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
21-21: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
21-21: Line too long (103 > 99)
(E501)
22-22: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
22-22: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
23-23: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
23-23: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
24-24: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
24-24: Line too long (147 > 99)
(E501)
24-24: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
24-24: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
27-27: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
27-27: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
28-28: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
28-28: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
29-29: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
29-29: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
29-29: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
🪛 GitHub Actions: Run CI/CD
backend/tests/slack/commands/management/slack_sync_messages_test.py
[error] 549-549: Ruff F841: Local variable mock_bulk_save is assigned to but never used at line 549.
backend/apps/slack/migrations/0014_message.py
[error] 17-17: Ruff E501: Line too long (117 > 99) at line 17.
[error] 21-21: Ruff E501: Line too long (103 > 99) at line 21.
[error] 24-24: Ruff E501: Line too long (147 > 99) at line 24.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
backend/tests/slack/commands/management/slack_sync_messages_test.py
Outdated
Show resolved
Hide resolved
backend/tests/slack/commands/management/slack_sync_messages_test.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/test-schema.yaml (1)
107-107: Optimize cache-to for buildx layer persistence
To ensure newly built layers are pushed back into the GitHub Actions cache and maximize cache hits, consider addingmode=max. Example diff:-cache-to: | - type=gha,compression=zstd +cache-to: | + type=gha,mode=max,compression=zstd
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/run-ci-cd.yaml(7 hunks).github/workflows/test-schema.yaml(1 hunks).github/workflows/update-nest-test-images.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- .github/workflows/run-ci-cd.yaml
- .github/workflows/update-nest-test-images.yaml
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run frontend e2e tests
🔇 Additional comments (1)
.github/workflows/test-schema.yaml (1)
104-104: Switch to GitHub Actions native cache for buildx
Great improvement—removing local directory caching in favor oftype=ghasimplifies the CI environment and standardizes caching across workflows. Please verify that the pinneddocker/build-push-actioncommit supports thetype=ghacache driver.
Dishant1804
force-pushed
the
message_model
branch
from
735c923 to
9c737e4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
backend/apps/slack/management/commands/slack_sync_messages.py (1)
210-216: 🛠️ Refactor suggestionImprove error handling and add missing newline.
The method logs errors but doesn't raise exceptions, potentially leading to silent failures. Also, the file is missing a trailing newline.
def _handle_slack_response(self, response, api_method): """Handle Slack API response and raise exception if needed.""" if not response["ok"]: - error_message = f"{api_method} API call failed" + error_message = f"{api_method} API call failed: {response.get('error', 'Unknown error')}" logger.error(error_message) self.stdout.write(self.style.ERROR(error_message)) + raise SlackApiError(message=error_message, response=response) +
♻️ Duplicate comments (4)
backend/apps/slack/management/commands/slack_sync_messages.py (4)
122-122: Use Message.update_data for consistency with other data syncing.Per previous review feedback, this should use
Message.update_datafor consistency with other data syncing operations.
142-149: Consider moving this logic to Message.from_slack method.This method contains message creation logic that would be better placed in the
Messagemodel as a class method for better separation of concerns.
179-203: Consider deferring thread fetching to a separate pass.Fetching thread replies inline during message processing could impact performance and makes the logic more complex. Consider processing main messages first, then fetching threads in a separate pass.
206-208: 🛠️ Refactor suggestionReplace broad KeyError handling with specific field validation.
The generic KeyError exception handling doesn't indicate which required field is missing, making debugging difficult.
- except KeyError: - logger.warning("Invalid message data") + except KeyError as e: + logger.warning(f"Invalid message data: missing required field {e}") return None
🧹 Nitpick comments (1)
backend/apps/slack/management/commands/slack_sync_messages.py (1)
15-16: Add class docstring for better documentation.The class is missing a docstring that would help other developers understand its purpose and usage.
class Command(BaseCommand): + """Django management command to synchronize Slack messages for all conversations. + + This command fetches messages from Slack API for all conversations in all workspaces, + optionally including thread replies, and stores them in the database. + """ help = "Populate messages for all Slack conversations"🧰 Tools
🪛 Pylint (3.3.7)
[convention] 15-15: Missing class docstring
(C0115)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/apps/slack/management/commands/slack_sync_messages.py(1 hunks)
🧰 Additional context used
🪛 Pylint (3.3.7)
backend/apps/slack/management/commands/slack_sync_messages.py
[error] 6-6: Unable to import 'django.core.management.base'
(E0401)
[error] 7-7: Unable to import 'slack_sdk'
(E0401)
[error] 8-8: Unable to import 'slack_sdk.errors'
(E0401)
[convention] 15-15: Missing class docstring
(C0115)
[convention] 38-38: Missing function or method docstring
(C0116)
[warning] 38-38: Unused argument 'args'
(W0613)
[refactor] 75-75: Too many arguments (6/5)
(R0913)
[refactor] 75-75: Too many local variables (16/15)
(R0914)
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (7)
backend/apps/slack/admin.py (1)
131-140:⚠️ Potential issueAdd missing class docstring.
The MessageAdmin class is missing a docstring as required by the coding standards.
class MessageAdmin(admin.ModelAdmin): + """Admin interface for Slack Message model.""" search_fields = (Note: For comprehensive admin interface improvements (list_display, list_filter, readonly_fields, etc.), please refer to the previous review comment which provides detailed enhancement suggestions.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 131-131: Missing class docstring
(C0115)
[refactor] 131-131: Too few public methods (0/2)
(R0903)
backend/apps/slack/models/message.py (1)
67-68: Add error handling for timestamp conversion.The timestamp conversion could fail if the Slack timestamp is not a valid float, which would cause the entire message processing to fail.
As mentioned in previous reviews, consider adding error handling:
- if ts := message_data.get("ts"): - self.timestamp = datetime.fromtimestamp(float(ts), tz=UTC) + if ts := message_data.get("ts"): + try: + self.timestamp = datetime.fromtimestamp(float(ts), tz=UTC) + except (ValueError, TypeError, OSError): + logger.warning(f"Invalid timestamp value: {ts}") + self.timestamp = Nonebackend/apps/slack/management/commands/slack_sync_messages.py (5)
18-37: Add missing command line argument for include_threads.The
include_threadsflag is hardcoded toTruein the handle method (line 42), but there's no corresponding command line argument to control this behavior.Add the missing argument as suggested in previous reviews:
parser.add_argument( "--channel-id", type=str, help="Specific channel ID to fetch messages from", ) + parser.add_argument( + "--include-threads", + action="store_true", + help="Fetch and combine thread messages (default: False)", + )
38-42: Use command line argument for include_threads.The
include_threadsvariable is hardcoded instead of reading from command line options.batch_size = options["batch_size"] channel_id = options["channel_id"] delay = options["delay"] - include_threads = True + include_threads = options.get("include_threads", False)🧰 Tools
🪛 Pylint (3.3.7)
[convention] 38-38: Missing function or method docstring
(C0116)
[warning] 38-38: Unused argument 'args'
(W0613)
58-62: Add error handling for non-existent channel ID.The code could raise
Conversation.DoesNotExistif the providedchannel_iddoesn't exist in the database.- conversations = ( - Conversation.objects.filter(slack_channel_id=channel_id) - if channel_id - else Conversation.objects.filter(workspace=workspace) - ) + if channel_id: + try: + conversations = [Conversation.objects.get(slack_channel_id=channel_id)] + except Conversation.DoesNotExist: + self.stdout.write( + self.style.ERROR(f"Channel ID {channel_id} not found") + ) + continue + else: + conversations = Conversation.objects.filter(workspace=workspace)
121-124: Fix line length violation.The line assigning
oldestexceeds the character limit and should be broken into multiple lines.- last_message = ( - Message.objects.filter(conversation=conversation).order_by("-timestamp").first() - ) - oldest = last_message.timestamp.timestamp() if last_message else None + last_message = ( + Message.objects.filter(conversation=conversation) + .order_by("-timestamp") + .first() + ) + oldest = ( + last_message.timestamp.timestamp() if last_message else None + )
301-307: Improve error handling and add missing newline.The response handling should include the actual error message and the file needs a trailing newline.
def _handle_slack_response(self, response, api_method): """Handle Slack API response and raise exception if needed.""" if not response["ok"]: - error_message = f"{api_method} API call failed" + error_message = f"{api_method} API call failed: {response.get('error', 'Unknown error')}" logger.error(error_message) self.stdout.write(self.style.ERROR(error_message)) + raise SlackApiError(message=error_message, response=response) +
🧹 Nitpick comments (5)
backend/apps/slack/models/message.py (3)
46-54: Consider reducing method argument count.The method has 6 arguments which exceeds the typical recommendation of 5. Consider if some parameters could be combined or if the method signature could be simplified.
You could potentially combine thread-related parameters into a single object or use keyword-only arguments more extensively to improve readability.
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 46-46: Too many arguments (6/5)
(R0913)
75-84: Consider reducing method argument count.Similar to the
from_slackmethod, this static method also has 6 arguments. The current design is functional but could benefit from parameter grouping.Consider if thread-related parameters (
is_thread_reply,parent_message) could be encapsulated in a single parameter or if the method interface could be simplified.🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 76-76: Too many arguments (6/5)
(R0913)
99-106: Consider optimizing database query.The current implementation uses separate
get()and fallback to creating a new instance. For bulk operations, this could be optimized usingget_or_create()or handling the logic at a higher level.- slack_message_id = data.get("ts") - try: - message = Message.objects.get( - slack_message_id=slack_message_id, conversation=conversation - ) - except Message.DoesNotExist: - message = Message(slack_message_id=slack_message_id, conversation=conversation) + slack_message_id = data.get("ts") + message, created = Message.objects.get_or_create( + slack_message_id=slack_message_id, + conversation=conversation, + defaults={'slack_message_id': slack_message_id, 'conversation': conversation} + )backend/apps/slack/management/commands/slack_sync_messages.py (2)
253-253: Remove unused client parameter.The
clientparameter is not used in this method according to static analysis.def _create_message_from_data( self, - client: WebClient, message_data: dict, conversation: Conversation, *, is_thread_reply: bool, parent_message: Message | None = None, ) -> Message | None:And update the call sites accordingly (lines 142, 225).
🧰 Tools
🪛 Pylint (3.3.7)
[warning] 253-253: Unused argument 'client'
(W0613)
192-200: Optimize database query for latest reply.The query structure could be simplified and the variable naming could be more consistent with the parent messages logic.
- latest_reply = ( - Message.objects.filter( - conversation=conversation, - parent_message=parent_message, - ) - .order_by("-timestamp") - .first() - ) - oldest_ts = latest_reply.timestamp.timestamp() if latest_reply else None + latest_reply = ( + Message.objects.filter( + conversation=conversation, + parent_message=parent_message, + ) + .order_by("-timestamp") + .first() + ) + oldest_ts = ( + latest_reply.timestamp.timestamp() if latest_reply else None + )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/apps/slack/admin.py(3 hunks)backend/apps/slack/management/commands/slack_sync_messages.py(1 hunks)backend/apps/slack/migrations/0014_message.py(1 hunks)backend/apps/slack/models/message.py(1 hunks)backend/tests/slack/commands/management/slack_sync_messages_test.py(1 hunks)backend/tests/slack/models/message_test.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
backend/tests/slack/commands/management/slack_sync_messages_test.py (5)
backend/apps/slack/management/commands/slack_sync_messages.py (6)
Command(15-306)handle(38-73)_create_message_from_data(251-299)_fetch_thread_replies(174-249)_handle_slack_response(301-306)add_arguments(18-36)backend/apps/slack/models/conversation.py (1)
Conversation(11-90)backend/apps/slack/models/member.py (1)
Member(10-78)backend/apps/slack/models/message.py (1)
Message(12-118)backend/apps/slack/models/workspace.py (2)
Workspace(10-43)bot_token(36-43)
backend/apps/slack/models/message.py (3)
backend/apps/common/models.py (2)
BulkSaveModel(8-30)TimestampedModel(33-40)backend/apps/slack/models/conversation.py (5)
Conversation(11-90)Meta(14-16)from_slack(39-60)bulk_save(63-65)update_data(68-90)backend/apps/slack/models/member.py (1)
Member(10-78)
backend/apps/slack/management/commands/slack_sync_messages.py (4)
backend/apps/slack/models/conversation.py (3)
Conversation(11-90)bulk_save(63-65)update_data(68-90)backend/apps/slack/models/member.py (1)
Member(10-78)backend/apps/slack/models/message.py (3)
Message(12-118)bulk_save(71-73)update_data(76-118)backend/apps/slack/models/workspace.py (2)
Workspace(10-43)bot_token(36-43)
backend/apps/slack/admin.py (1)
backend/apps/slack/models/message.py (1)
Message(12-118)
backend/tests/slack/models/message_test.py (4)
backend/apps/slack/models/conversation.py (1)
Conversation(11-90)backend/apps/slack/models/member.py (1)
Member(10-78)backend/apps/slack/models/message.py (4)
Message(12-118)bulk_save(71-73)update_data(76-118)from_slack(46-68)backend/tests/slack/commands/management/slack_sync_messages_test.py (1)
mock_conversation(50-56)
🪛 Pylint (3.3.7)
backend/tests/slack/commands/management/slack_sync_messages_test.py
[error] 7-7: Unable to import 'django.core.management'
(E0401)
[error] 9-9: Unable to import 'apps.slack.management.commands.slack_sync_messages'
(E0401)
[error] 10-10: Unable to import 'apps.slack.models'
(E0401)
[warning] 149-149: Unused argument 'mock_conversation'
(W0613)
[refactor] 165-165: Too many arguments (10/5)
(R0913)
[refactor] 165-165: Too many positional arguments (10/5)
(R0917)
[refactor] 165-165: Too many local variables (18/15)
(R0914)
[warning] 167-167: Unused argument 'mock_sleep'
(W0613)
[warning] 228-228: Access to a protected member _create_message_from_data of a client class
(W0212)
[warning] 245-245: Access to a protected member _create_message_from_data of a client class
(W0212)
[warning] 262-262: Access to a protected member _create_message_from_data of a client class
(W0212)
[warning] 283-283: Access to a protected member _create_message_from_data of a client class
(W0212)
[warning] 310-310: Access to a protected member _create_message_from_data of a client class
(W0212)
[warning] 335-335: Access to a protected member _fetch_thread_replies of a client class
(W0212)
[refactor] 346-346: Too many arguments (6/5)
(R0913)
[refactor] 346-346: Too many positional arguments (6/5)
(R0917)
[warning] 369-369: Access to a protected member _fetch_thread_replies of a client class
(W0212)
[warning] 347-347: Unused argument 'mock_sleep'
(W0613)
[warning] 385-385: Access to a protected member _handle_slack_response of a client class
(W0212)
[warning] 393-393: Access to a protected member _handle_slack_response of a client class
(W0212)
[refactor] 23-23: Too many public methods (22/20)
(R0904)
backend/apps/slack/migrations/0014_message.py
[convention] 1-1: Missing module docstring
(C0114)
[convention] 1-1: Module name "0014_message" doesn't conform to snake_case naming style
(C0103)
[error] 3-3: Unable to import 'django.db.models.deletion'
(E0401)
[error] 4-4: Unable to import 'django.db'
(E0401)
[convention] 7-7: Missing class docstring
(C0115)
[refactor] 7-7: Too few public methods (0/2)
(R0903)
backend/apps/slack/models/message.py
[error] 5-5: Unable to import 'django.db'
(E0401)
[convention] 15-15: Missing class docstring
(C0115)
[refactor] 15-15: Too few public methods (0/2)
(R0903)
[refactor] 46-46: Too many arguments (6/5)
(R0913)
[refactor] 76-76: Too many arguments (6/5)
(R0913)
backend/apps/slack/management/commands/slack_sync_messages.py
[error] 6-6: Unable to import 'django.core.management.base'
(E0401)
[error] 7-7: Unable to import 'slack_sdk'
(E0401)
[error] 8-8: Unable to import 'slack_sdk.errors'
(E0401)
[convention] 15-15: Missing class docstring
(C0115)
[convention] 38-38: Missing function or method docstring
(C0116)
[warning] 38-38: Unused argument 'args'
(W0613)
[refactor] 75-75: Too many arguments (6/5)
(R0913)
[refactor] 174-174: Too many local variables (20/15)
(R0914)
[refactor] 251-251: Too many arguments (6/5)
(R0913)
[warning] 297-297: Catching too general exception Exception
(W0718)
[warning] 253-253: Unused argument 'client'
(W0613)
backend/apps/slack/admin.py
[convention] 131-131: Missing class docstring
(C0115)
[refactor] 131-131: Too few public methods (0/2)
(R0903)
backend/tests/slack/models/message_test.py
[convention] 1-1: Missing module docstring
(C0114)
[error] 3-3: Unable to import 'apps.slack.models.conversation'
(E0401)
[error] 4-4: Unable to import 'apps.slack.models.member'
(E0401)
[error] 5-5: Unable to import 'apps.slack.models.message'
(E0401)
[convention] 8-8: Missing function or method docstring
(C0116)
[warning] 10-10: Access to a protected member _state of a client class
(W0212)
[convention] 15-15: Missing class docstring
(C0115)
[convention] 16-16: Missing function or method docstring
(C0116)
[convention] 22-22: Missing function or method docstring
(C0116)
[convention] 53-53: Missing function or method docstring
(C0116)
[convention] 87-87: Missing function or method docstring
(C0116)
[convention] 119-119: Missing function or method docstring
(C0116)
[convention] 157-157: Missing function or method docstring
(C0116)
[convention] 188-188: Missing function or method docstring
(C0116)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run frontend e2e tests
🔇 Additional comments (16)
backend/apps/slack/admin.py (2)
8-8: LGTM: Clean import addition.The import follows the existing pattern and properly imports the new Message model for admin registration.
153-153: LGTM: Proper admin registration.The Message model registration follows the existing pattern and correctly pairs the model with its admin class.
backend/tests/slack/models/message_test.py (8)
8-13: LGTM: Useful mock helper function.The
create_model_mockhelper function provides a clean way to create consistent mock instances for testing. This reduces code duplication across test methods.🧰 Tools
🪛 Pylint (3.3.7)
[convention] 8-8: Missing function or method docstring
(C0116)
[warning] 10-10: Access to a protected member _state of a client class
(W0212)
16-21: LGTM: Effective delegation test.The test properly verifies that
Message.bulk_savedelegates toBulkSaveModel.bulk_savewith the correct parameters.🧰 Tools
🪛 Pylint (3.3.7)
[convention] 16-16: Missing function or method docstring
(C0116)
22-52: LGTM: Comprehensive new message test.The test thoroughly covers the new message creation scenario, including proper attribute assignment and save behavior verification.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 22-22: Missing function or method docstring
(C0116)
53-86: LGTM: Thorough existing message update test.The test effectively verifies the existing message update path, including proper delegation to
from_slackwith the correct parameters.🧰 Tools
🪛 Pylint (3.3.7)
[convention] 53-53: Missing function or method docstring
(C0116)
87-118: LGTM: Important save control test.The test verifies the
save=Falsebehavior, which is crucial for bulk operations where individual saves should be avoided.🧰 Tools
🪛 Pylint (3.3.7)
[convention] 87-87: Missing function or method docstring
(C0116)
119-156: LGTM: Essential thread reply test.The test properly verifies thread reply functionality, including parent message assignment and correct thread flag handling.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 119-119: Missing function or method docstring
(C0116)
157-187: LGTM: Complete thread parent test.The test effectively verifies thread parent detection based on reply_count, completing the thread functionality test coverage.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 157-157: Missing function or method docstring
(C0116)
188-196: LGTM: Effective string representation test.The test covers both short and long message scenarios, verifying the 50-character truncation limit and ellipsis addition for long messages.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 188-188: Missing function or method docstring
(C0116)
backend/tests/slack/commands/management/slack_sync_messages_test.py (5)
1-21: LGTM: Well-structured test module setup.The module has clear imports, useful constants for test data, and follows good testing practices.
🧰 Tools
🪛 Pylint (3.3.7)
[error] 7-7: Unable to import 'django.core.management'
(E0401)
[error] 9-9: Unable to import 'apps.slack.management.commands.slack_sync_messages'
(E0401)
[error] 10-10: Unable to import 'apps.slack.models'
(E0401)
26-136: LGTM: Excellent fixture design.The fixtures provide comprehensive mock data for different test scenarios while maintaining clear separation of concerns. This supports robust and maintainable tests.
138-219: LGTM: Comprehensive command execution tests.The tests effectively cover the main execution paths including error conditions (no workspaces, no token) and successful synchronization scenarios.
🧰 Tools
🪛 Pylint (3.3.7)
[warning] 149-149: Unused argument 'mock_conversation'
(W0613)
[refactor] 165-165: Too many arguments (10/5)
(R0913)
[refactor] 165-165: Too many positional arguments (10/5)
(R0917)
[refactor] 165-165: Too many local variables (18/15)
(R0914)
[warning] 167-167: Unused argument 'mock_sleep'
(W0613)
220-394: LGTM: Thorough edge case coverage.The tests excellently cover various edge cases including different message subtypes, missing content, missing users, and error handling scenarios.
🧰 Tools
🪛 Pylint (3.3.7)
[warning] 228-228: Access to a protected member _create_message_from_data of a client class
(W0212)
[warning] 245-245: Access to a protected member _create_message_from_data of a client class
(W0212)
[warning] 262-262: Access to a protected member _create_message_from_data of a client class
(W0212)
[warning] 283-283: Access to a protected member _create_message_from_data of a client class
(W0212)
[warning] 310-310: Access to a protected member _create_message_from_data of a client class
(W0212)
[warning] 335-335: Access to a protected member _fetch_thread_replies of a client class
(W0212)
[refactor] 346-346: Too many arguments (6/5)
(R0913)
[refactor] 346-346: Too many positional arguments (6/5)
(R0917)
[warning] 369-369: Access to a protected member _fetch_thread_replies of a client class
(W0212)
[warning] 347-347: Unused argument 'mock_sleep'
(W0613)
[warning] 385-385: Access to a protected member _handle_slack_response of a client class
(W0212)
[warning] 393-393: Access to a protected member _handle_slack_response of a client class
(W0212)
395-432: LGTM: Complete command interface testing.The tests effectively cover argument parsing and Django management command integration, providing comprehensive test coverage for the entire command.
backend/apps/slack/migrations/0014_message.py (1)
7-64: LGTM: Well-structured database migration.The migration properly creates the Message model with:
- Appropriate field types and constraints
- Correct foreign key relationships to Member and Conversation
- Self-referential parent_message for thread support
- Unique constraint ensuring message uniqueness per conversation
- Proper database table naming and verbose names
The line formatting issues mentioned in previous reviews appear to be resolved.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 7-7: Missing class docstring
(C0115)
[refactor] 7-7: Too few public methods (0/2)
(R0903)
| """Human readable representation.""" | ||
| text_display_limit = 50 | ||
| text_preview = ( | ||
| self.text[:text_display_limit] + "..." |
There was a problem hiding this comment.
Try common::utils::truncate()
|
|
||
| def _create_message_from_data( | ||
| self, | ||
| client: WebClient, |
There was a problem hiding this comment.
client seems to be unused here.
| author = Member.objects.get( | ||
| slack_user_id=slack_user_id, workspace=conversation.workspace | ||
| ) | ||
| except Member.DoesNotExist: |
There was a problem hiding this comment.
Can we create a user instead?
There was a problem hiding this comment.
I think we are already syncing the members of the workspace -- so if the member is able to message he/she will de facto be a part of workspace and they will already be synced as we are syncing the members as well in slack_sync_data here
Creating a user will be redundant here
There was a problem hiding this comment.
Consider a case when user joins after the slack_sync_data is finished but before the messages sync. It's also important to remember that those 2 sync will not always go together.
There was a problem hiding this comment.
This is such a great case I missed it completely
Thanks for explaining I'll implement it
Resolves #1475