Saving entire body of messsages#1621
Conversation
Summary by CodeRabbit
Summary by CodeRabbit
WalkthroughThis change updates the Slack message synchronization process to store the entire raw message body in the database, modifies the message model to allow messages without an author, and adjusts related tests accordingly. It also removes filtering for certain message subtypes and improves author resolution logic with retries and error handling. Changes
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🧪 Generate Unit Tests
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
backend/apps/slack/models/message.py (1)
47-49: Renamerelated_namefor theinviterFKUsing
related_name="inviter"will createmember.inviterwhich collides conceptually with theMemberobject itself and reads ambiguously. Consider something likerelated_name="invited_messages"for clarity and to avoid accidental shadowing.backend/apps/slack/management/commands/slack_sync_messages.py (2)
352-357: Remove unreachable duplicate author-check
authoris already validated just above; these lines will never be hit and add noise.- if not author: - self.stdout.write( - self.style.WARNING(f"Could not fetch user {slack_user_id}, skipping message") - ) - return None
370-415: Tidy_get_or_create_member: naming & needless initial sleep
- Variable
authoractually holds aMember; rename tomemberfor readability.- The unconditional
time.sleep(delay)before the first API call slows the happy path without need. Sleep only after a rate-limit response.- author = None + member = None ... - time.sleep(delay) ... - author = Member.update_data(...) + member = Member.update_data(...) ... - return author + return memberThis also satisfies the pylint “too many arguments” warning if you move
delayto an internal constant or default.🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 370-370: Too many arguments (6/5)
(R0913)
[refactor] 370-370: Too many positional arguments (6/5)
(R0917)
backend/tests/slack/models/message_test.py (1)
212-219: Assert theinviterwas persistedThe test verifies the plumbing but never checks the field value:
-assert result is mock_message_instance +assert result is mock_message_instance +assert result.inviter is mock_inviterHelps catch regressions where the FK assignment is accidentally dropped.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/apps/slack/management/commands/slack_sync_messages.py(1 hunks)backend/apps/slack/migrations/0016_message_blocks_message_client_message_id_and_more.py(1 hunks)backend/apps/slack/models/message.py(5 hunks)backend/tests/slack/commands/management/slack_sync_messages_test.py(9 hunks)backend/tests/slack/models/message_test.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/apps/slack/management/commands/slack_sync_messages.py (3)
backend/apps/slack/models/message.py (2)
Message(13-149)update_data(111-149)backend/apps/slack/models/conversation.py (1)
update_data(68-90)backend/apps/slack/models/member.py (1)
Member(10-78)
🪛 Pylint (3.3.7)
backend/apps/slack/management/commands/slack_sync_messages.py
[refactor] 370-370: Too many arguments (6/5)
(R0913)
[refactor] 370-370: Too many positional arguments (6/5)
(R0917)
backend/apps/slack/migrations/0016_message_blocks_message_client_message_id_and_more.py
[refactor] 7-7: Too few public methods (0/2)
(R0903)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Run frontend unit tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (1)
backend/tests/slack/commands/management/slack_sync_messages_test.py (1)
234-260: Looks good – updated subtype tests cover the new ignore-logic.
backend/apps/slack/migrations/0016_message_blocks_message_client_message_id_and_more.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/apps/slack/migrations/0016_message_blocks_message_client_message_id_and_more.py(1 hunks)backend/apps/slack/models/message.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/apps/slack/models/message.py
🧰 Additional context used
🪛 Pylint (3.3.7)
backend/apps/slack/migrations/0016_message_blocks_message_client_message_id_and_more.py
[refactor] 7-7: Too few public methods (0/2)
(R0903)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (2)
backend/apps/slack/migrations/0016_message_blocks_message_client_message_id_and_more.py (2)
74-81: 👍parent_user_id,subtype, andteamnow includemax_lengthThis addresses the divergence warning noted in the previous review. No further action needed here.
28-34: Re-evaluateon_delete=models.CASCADEfor theinviterrelationIf a
Memberis deleted, all messages they invited will also be deleted.
ConsiderPROTECTorSET_NULLinstead to preserve historical messages.[ suggest_optional_refactor ]
backend/apps/slack/migrations/0016_message_blocks_message_client_message_id_and_more.py
Outdated
Show resolved
Hide resolved
Dishant1804
force-pushed
the
fetch_entire_message_body
branch
from
b3a940e to
3ce2b3c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/apps/slack/migrations/0016_message_message_body.py (1)
12-16: Addingmessage_bodyJSONField
The new field withdefault=dictand a descriptiveverbose_namewill store full Slack payloads as intended.
Suggestion: if you need historical message data backfilled (instead of{}), consider adding aRunPythondata migration or allowingnull=Trueuntil real payloads are populated.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/apps/slack/admin.py(1 hunks)backend/apps/slack/management/commands/slack_sync_messages.py(1 hunks)backend/apps/slack/migrations/0016_message_message_body.py(1 hunks)backend/apps/slack/models/message.py(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- backend/apps/slack/admin.py
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/apps/slack/management/commands/slack_sync_messages.py
- backend/apps/slack/models/message.py
🧰 Additional context used
🪛 Pylint (3.3.7)
backend/apps/slack/migrations/0016_message_message_body.py
[refactor] 6-6: Too few public methods (0/2)
(R0903)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
- GitHub Check: Run backend tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (3)
backend/apps/slack/migrations/0016_message_message_body.py (3)
1-2: Auto-generated migration header is correct
The timestamp and Django version comment follow conventions and need no changes.
3-3: Imports are appropriate
Usingmigrationsandmodelsfromdjango.dbaligns with standard Django migration patterns.
7-9: Dependency declaration is valid
Dependence on0015_remove_message_is_thread_parent_message_has_replies_and_morecorrectly sequences this migration.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
backend/apps/slack/management/commands/slack_sync_messages.py (2)
338-341: Remove unconditionalsleepbefore firstusers_infocall
time.sleep(delay)is executed unconditionally before the first attempt to call the Slack API, slowing down normal execution paths.
Move the sleep inside the rate-limited branch where it is actually needed.- try: - time.sleep(delay) + try: user_info = client.users_info(user=slack_user_id)
352-360: Use dict-style access consistently forSlackResponseEverywhere else you use
e.response["error"]; here you switch to.get().
SlackResponsemimics a dict but does not guarantee.get(). Stick to the bracket form for consistency and to avoid surprises if the SDK changes.- if e.response.get("error") == "ratelimited": + if e.response["error"] == "ratelimited":backend/apps/slack/migrations/0016_message_is_bot_message_raw_data_alter_message_author.py (1)
20-22: Guard against storing unbounded raw payloads
raw_datawill store the full Slack payload, which can occasionally exceed several hundred kilobytes (large file attachments, blocks, etc.).
Consider adding a DB-level size check or an application-level trim/compression step to avoid bloating the table.This can be deferred, but worth tracking.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/apps/slack/management/commands/slack_sync_messages.py(1 hunks)backend/apps/slack/migrations/0016_message_is_bot_message_raw_data_alter_message_author.py(1 hunks)backend/apps/slack/models/message.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/apps/slack/models/message.py
🧰 Additional context used
🪛 Pylint (3.3.7)
backend/apps/slack/migrations/0016_message_is_bot_message_raw_data_alter_message_author.py
[refactor] 7-7: Too few public methods (0/2)
(R0903)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run frontend e2e tests
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
In general is you are writing the same code (especially in the same file) more than once you probably implementing something wrong.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
backend/apps/slack/management/commands/slack_sync_messages.py (2)
274-274: Consider parameter consolidation to address complexity.The method has 7 parameters, which exceeds best practices and triggers pylint warnings. While all parameters appear necessary, consider consolidating them into a parameter object or dataclass to improve maintainability.
Example refactoring approach:
+from dataclasses import dataclass + +@dataclass +class MessageCreationParams: + client: WebClient + conversation: Conversation + delay: float + max_retries: int + -def _create_message( - self, - client: WebClient, - message_data: dict, - conversation: Conversation, - delay: float, - max_retries: int, - *, - parent_message: Message | None = None, -) -> Message | None: +def _create_message( + self, + params: MessageCreationParams, + message_data: dict, + *, + parent_message: Message | None = None, +) -> Message | None:
285-338: Simplify the nested retry logic for better maintainability.While the logic correctly handles both users and bots, the nested retry structure makes it difficult to follow. Consider extracting the member creation logic to reduce complexity.
+def _create_member_with_retry(self, client, slack_user_id, message_data, workspace, delay, max_retries): + """Create member with retry logic.""" + for retry_count in range(max_retries): + try: + if message_data.get("user"): + user_info = client.users_info(user=slack_user_id) + self._handle_slack_response(user_info, "users_info") + return Member.update_data(user_info["user"], workspace, save=True) + else: + bot_info = client.bots_info(bot=slack_user_id) + self._handle_slack_response(bot_info, "bots_info") + bot_data = { + "id": slack_user_id, + "is_bot": True, + "name": bot_info["bot"].get("name"), + "real_name": bot_info["bot"].get("name"), + } + return Member.update_data(bot_data, workspace, save=True) + except SlackApiError as e: + if e.response.get("error") == "ratelimited": + retry_after = int(e.response.headers.get("Retry-After", delay)) + self.stdout.write(self.style.WARNING("Rate limited on member info")) + time.sleep(retry_after) + else: + self.stdout.write(self.style.ERROR(f"Failed to fetch member data for {slack_user_id}")) + break + return None def _create_message(self, ...): # ... existing code ... except Member.DoesNotExist: - retry_count = 0 - while retry_count < max_retries: - # ... complex nested logic ... + author = self._create_member_with_retry( + client, slack_user_id, message_data, conversation.workspace, delay, max_retries + )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
backend/apps/slack/admin.py(2 hunks)backend/apps/slack/management/commands/slack_sync_messages.py(7 hunks)backend/apps/slack/migrations/0017_remove_message_text.py(1 hunks)backend/apps/slack/models/conversation.py(2 hunks)backend/apps/slack/models/member.py(1 hunks)backend/apps/slack/models/message.py(3 hunks)backend/tests/slack/commands/management/slack_sync_messages_test.py(5 hunks)backend/tests/slack/models/message_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/apps/slack/models/message.py
- backend/tests/slack/commands/management/slack_sync_messages_test.py
🧰 Additional context used
🪛 Pylint (3.3.7)
backend/apps/slack/management/commands/slack_sync_messages.py
[refactor] 274-274: Too many arguments (7/5)
(R0913)
[refactor] 274-274: Too many positional arguments (6/5)
(R0917)
backend/apps/slack/migrations/0017_remove_message_text.py
[refactor] 6-6: Too few public methods (0/2)
(R0903)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
- GitHub Check: Run backend tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (13)
backend/apps/slack/migrations/0017_remove_message_text.py (1)
6-16: LGTM! Migration structure is correct.The migration properly removes the
textfield and correctly depends on the previous migration that introducedraw_data. The structure follows Django migration best practices.backend/apps/slack/models/conversation.py (2)
4-4: Good use of TYPE_CHECKING to prevent circular imports.The conditional import pattern properly avoids runtime circular dependencies while maintaining type hints.
Also applies to: 11-12
43-46: Efficient implementation of latest_message property.The property correctly uses Django ORM ordering to get the most recent message. The return type annotation properly indicates it can return None when no messages exist.
backend/apps/slack/models/member.py (1)
52-52: Excellent defensive programming improvement.Using nested
.get()calls preventsKeyErrorexceptions when the"profile"key is missing frommember_data, making the code more robust when processing Slack member data.backend/tests/slack/models/message_test.py (1)
181-182: Test correctly updated to align with model changes.The test properly uses
raw_data={"text": "Short message"}instead of the removedtextfield, maintaining the same test logic while adapting to the new data structure.backend/apps/slack/admin.py (2)
93-96: Useful admin filters added for Member model.Adding filters for
is_botandworkspacewill improve the admin interface usability for managing Slack members.
136-154: Admin interface properly updated to align with model changes.The changes correctly:
- Update
list_displayto showcreated_atinstead of the removedtextfield- Add
authorandconversationto provide better context in the admin list view- Change search from
"text"to"raw_data__text"to maintain search functionality with the new data structure- Add useful filters for
has_repliesandconversationbackend/apps/slack/management/commands/slack_sync_messages.py (6)
23-23: Good performance optimization.Increasing the default batch size from 200 to 999 will reduce the number of API calls needed while staying within Slack's API limits.
115-124: Excellent architectural improvements.The refactoring to use bulk saves and immediate reply fetching significantly improves performance and aligns well with the goal of saving complete message data. The void method approach with bulk saves is much more efficient than individual saves.
Also applies to: 130-138, 141-153, 173-191
202-272: Consistent and well-implemented reply fetching.The bulk save approach and improved parameter handling align well with the main message fetching logic. Good consistency in the codebase.
339-345: Correct implementation for bulk save strategy.The
save=Falseparameter correctly supports the bulk save approach, and the method properly handles cases whereauthormight beNone.
274-345: Successfully addresses past review feedback.The refactored
_create_messagemethod properly handles the bot_id vs user distinction that was flagged in previous reviews and aligns well with the PR objective of saving entire message bodies without unnecessary filtering.
312-320: Verify bot_data structure matches Member model expectations.The custom
bot_datadictionary creation looks functional, but please verify that the field mapping aligns with theMembermodel structure, particularly the mapping of bot name to bothnameandreal_namefields.#!/bin/bash # Description: Check the Member model structure to verify bot_data field mapping # Expected: Find the Member model definition and its field structure ast-grep --pattern 'class Member($$$): $$$'
2 participants
Resolves #1613