baseapp-chats: filter past messages for new chat group participant #208
Conversation
|
Warning Rate limit exceeded@pt-tsl has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 33 minutes and 48 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📥 CommitsReviewing files that changed from the base of the PR and between 63386f0c7ccaf6ee939feb2cc84ee08fc8be2c16 and 80101a4. 📒 Files selected for processing (2)
WalkthroughThe changes introduce functionality to handle group chat message visibility. A new Changes
Sequence DiagramsequenceDiagram
participant User1 as Existing User
participant User2 as New User
participant ChatRoom as Group Chat
participant MessageService as Message Service
User1->>ChatRoom: Send messages before User2 joins
User2->>ChatRoom: Join chat
User2->>MessageService: Request messages
MessageService-->>User2: Return only messages after join time
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 0
🧹 Nitpick comments (3)
baseapp-chats/baseapp_chats/graphql/object_types.py (1)
101-113: Simplify profile retrieval logic.The nested conditional for profile retrieval can be simplified for better readability.
Consider this refactoring:
- profile = ( - info.context.user.current_profile - if hasattr(info.context.user, "current_profile") - else ( - info.context.user.profile.pk if hasattr(info.context.user, "profile") else None - ) - ) + profile = getattr(info.context.user, 'current_profile', None) or \ + getattr(info.context.user, 'profile', None) + profile_id = profile.pk if profile else Nonebaseapp-chats/baseapp_chats/graphql/mutations.py (1)
118-123: Consider bulk creation for better performance.The current implementation creates participants one by one. For better performance, consider using bulk creation.
- for participant in participants: - ChatRoomParticipant.objects.create( - profile=participant, room=room, accepted_at=timezone.now() - ) + now = timezone.now() + ChatRoomParticipant.objects.bulk_create([ + ChatRoomParticipant(profile=participant, room=room, accepted_at=now) + for participant in participants + ])baseapp-chats/baseapp_chats/tests/test_graphql_queries.py (1)
276-320: Test implementation looks good, consider additional test cases.The test effectively verifies the core functionality. Consider adding these test cases:
- Test when a participant leaves and rejoins the group
- Test with multiple participants joining at different times
- Test edge case when
participants_countis exactly 2Example test case for rejoining scenario:
def test_participant_can_see_previous_messages_when_rejoining(graphql_user_client): with freeze_time("2024-12-01 10:00:00") as frozen_time: # Initial setup with participant # Leave group # Send messages # Rejoin group # Verify all messages are visible pass
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 3fc7a38 and 37fc8c4943e34871b602a5c48acfad9cc6b99a2e.
📒 Files selected for processing (4)
baseapp-chats/baseapp_chats/graphql/mutations.py(1 hunks)baseapp-chats/baseapp_chats/graphql/object_types.py(1 hunks)baseapp-chats/baseapp_chats/tests/test_graphql_queries.py(2 hunks)baseapp-chats/setup.cfg(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- baseapp-chats/setup.cfg
🔇 Additional comments (1)
baseapp-chats/baseapp_chats/graphql/object_types.py (1)
109-113: Verify message filtering behavior for edge cases.
The current implementation might have edge cases:
- What happens if
participantis None? - Should we return an empty list instead of all messages when no participant is found?
✅ Verification successful
Let me check the broader context of this code to better understand the message filtering behavior.
Let me try a different approach to get the context of the messages method.
The message filtering behavior is correctly implemented for edge cases
Looking at the full implementation in resolve_all_messages, the code handles edge cases appropriately:
- When
participantis None, the code falls through to the default return statementreturn self.messages.all().order_by("-created"), which returns all messages. - The filtering by
accepted_atis only applied for chat rooms with more than 2 participants (if self.participants_count > 2), which is a deliberate design choice to show the full message history for direct messages (2 participants) while filtering group chat messages based on when a participant joined.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential edge case handling in other implementations
rg -A 5 "filter.*accepted_at"
Length of output: 558
Script:
#!/bin/bash
# Get more context around the message filtering implementation
ast-grep --pattern 'def messages($$$) {
$$$
}'
Length of output: 50
Script:
#!/bin/bash
# Get the full context of the file containing the message filtering logic
cat baseapp-chats/baseapp_chats/graphql/object_types.py
Length of output: 5309
nossila
changed the title
pt-tsl
force-pushed
the
BA-1943-hide-past-group-chat-messages
branch
from
37fc8c4 to
7cec752
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
baseapp-chats/baseapp_chats/base.py (1)
32-32: Consider documenting the relationship betweenis_groupandparticipants_countThe model now has both
is_groupandparticipants_countfields. Whileis_groupis used for filtering messages, it would be helpful to document how these fields relate to each other and whether there are any invariants (e.g., is a room withparticipants_count > 2always a group?).baseapp-chats/baseapp_chats/tests/test_graphql_queries.py (1)
276-321: Test implementation looks good but could be more explicitThe test effectively validates that new participants can only see messages after joining. However, consider adding explicit assertions to verify that the earlier messages are not visible.
assert len(content["data"]["chatRoom"]["allMessages"]["edges"]) == 2 +# Verify these are only the messages sent after joining +messages = content["data"]["chatRoom"]["allMessages"]["edges"] +assert all(message["node"]["content"] == messages[0]["node"]["content"] + for message in messages)baseapp-chats/baseapp_chats/graphql/mutations.py (1)
88-88: Simplify the title retrievalThe
Nonedefault value is redundant sincedict.get()returnsNoneby default.- title = input.get("title", None) + title = input.get("title")🧰 Tools
🪛 Ruff (0.8.2)
88-88: Use
input.get("title")instead ofinput.get("title", None)Replace
input.get("title", None)withinput.get("title")(SIM910)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 37fc8c4943e34871b602a5c48acfad9cc6b99a2e and 7cec75238f7813e349eb9daa886891635d517def.
📒 Files selected for processing (7)
baseapp-chats/baseapp_chats/base.py(1 hunks)baseapp-chats/baseapp_chats/graphql/mutations.py(3 hunks)baseapp-chats/baseapp_chats/graphql/object_types.py(2 hunks)baseapp-chats/baseapp_chats/migrations/0004_chatroom_is_group.py(1 hunks)baseapp-chats/baseapp_chats/tests/test_graphql_mutations.py(3 hunks)baseapp-chats/baseapp_chats/tests/test_graphql_queries.py(2 hunks)baseapp-chats/setup.cfg(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- baseapp-chats/setup.cfg
🧰 Additional context used
🪛 Ruff (0.8.2)
baseapp-chats/baseapp_chats/graphql/mutations.py
88-88: Use input.get("title") instead of input.get("title", None)
Replace input.get("title", None) with input.get("title")
(SIM910)
🔇 Additional comments (6)
baseapp-chats/baseapp_chats/migrations/0004_chatroom_is_group.py (1)
13-17: LGTM! Clean migration for adding the is_group field
The migration is well-structured and maintains backward compatibility with the default value.
baseapp-chats/baseapp_chats/graphql/object_types.py (1)
149-157: Verify coordination with parallel PR introducing is_group
A previous review comment mentions another PR (#198) also introducing the is_group field. We should ensure these changes are coordinated.
✅ Verification successful
PR #198 and current PR are properly coordinated
Based on the search results, both PRs are well-coordinated:
- PR #198 "baseapp-chats: create chat group" is currently open and introduces the same
is_groupfunctionality - The field is properly integrated across the codebase:
- Model definition in
base.py - Migration file
0004_chatroom_is_group.py - GraphQL mutations with proper validation logic
- GraphQL object types (this PR)
- Test coverage in
test_graphql_queries.py
- Model definition in
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if PR #198 also modifies these files
gh pr view 198 --json files -q '.files[].path' | grep -E 'baseapp-chats/(base\.py|object_types\.py|migrations/)'
Length of output: 114
Script:
#!/bin/bash
# Let's first check if PR #198 exists and its current status
gh pr view 198 --json number,title,state,isDraft
# Let's also search for any mentions of is_group in the codebase
rg "is_group" -C 2
# And check for any recent migrations that might have added this field
fd . "baseapp-chats/migrations" --type f --exec head -n 20 {}
Length of output: 5018
baseapp-chats/baseapp_chats/graphql/mutations.py (2)
87-97: Group chat validation logic is well implemented
The validation ensures that group chats require a title, which is a good UX decision.
🧰 Tools
🪛 Ruff (0.8.2)
88-88: Use input.get("title") instead of input.get("title", None)
Replace input.get("title", None) with input.get("title")
(SIM910)
137-142: Efficient use of bulk_create for participants
Good use of bulk_create for better performance when adding multiple participants.
baseapp-chats/baseapp_chats/tests/test_graphql_mutations.py (2)
437-464: Well-structured test for group chat creation
The test thoroughly validates all aspects of group chat creation including:
- Title setting
- Group flag
- Participant count
- Image upload
466-489: Good negative test case for group creation
Properly validates that group creation fails without a required title.
pt-tsl
changed the base branch from
master
to
BA-1935-BE-chats-update-mutation-to-create-groups
Ronan-Fernandes
force-pushed
the
BA-1935-BE-chats-update-mutation-to-create-groups
branch
2 times, most recently
from
548fb49 to
80b0e3b
Compare
pt-tsl
force-pushed
the
BA-1943-hide-past-group-chat-messages
branch
from
7cec752 to
2670f2a
Compare
Ronan-Fernandes
force-pushed
the
BA-1935-BE-chats-update-mutation-to-create-groups
branch
2 times, most recently
from
c9756e7 to
adced51
Compare
anicioalexandre
force-pushed
the
BA-1943-hide-past-group-chat-messages
branch
from
2670f2a to
73f2e64
Compare
pt-tsl
force-pushed
the
BA-1943-hide-past-group-chat-messages
branch
from
0ab9588 to
6ab4213
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
baseapp-chats/baseapp_chats/graphql/object_types.py (1)
122-134: 🛠️ Refactor suggestionSimplify profile resolution and add error handling
The current implementation needs improvement in several areas:
- Profile resolution is complex and could be simplified
- Missing error handling for edge cases
- No logging for debugging issues
Apply this refactoring to improve error handling and maintainability:
def resolve_all_messages(self, info, **kwargs): if self.is_group: - profile = ( - info.context.user.current_profile - if hasattr(info.context.user, "current_profile") - else ( - info.context.user.profile.pk if hasattr(info.context.user, "profile") else None - ) - ) + try: + profile = getattr(info.context.user, 'current_profile', None) or info.context.user.profile + except AttributeError: + logger.debug("No profile found for user %s", info.context.user.id) + return self.messages.none() # Return empty queryset if no profile + participant = self.participants.filter(profile=profile).first() - if participant: - return self.messages.filter(created__gte=participant.accepted_at).order_by( - "-created" - ) + if not participant: + logger.debug("User %s is not a participant in group %s", info.context.user.id, self.id) + return self.messages.none() + + if not participant.accepted_at: + logger.debug("Participant %s has no accepted_at timestamp in group %s", participant.id, self.id) + return self.messages.none() + + return self.messages.filter(created__gte=participant.accepted_at).order_by("-created") return self.messages.all().order_by("-created")
🧹 Nitpick comments (2)
baseapp-chats/baseapp_chats/graphql/mutations.py (1)
147-149: Consider storing participant counts in a more robust manner.Saving
participants_countas a static field may lead to inconsistencies if participants are added or removed later. You might avoid storing derived data like this, or ensure it is always updated in lockstep with participant modifications (e.g., using signals, triggers, or a single transaction).baseapp-chats/baseapp_chats/graphql/object_types.py (1)
122-134: Add type hints for better code maintainabilityConsider adding type hints to improve code maintainability:
- def resolve_all_messages(self, info, **kwargs): + def resolve_all_messages(self, info: ResolveInfo, **kwargs) -> QuerySet[Message]:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 4dfd35f4f79c2a1f8f31e4375ac573f6e290df24 and 6ab42139798bd2b95f9dcfba62e56f0974133996.
📒 Files selected for processing (4)
baseapp-chats/baseapp_chats/graphql/mutations.py(1 hunks)baseapp-chats/baseapp_chats/graphql/object_types.py(1 hunks)baseapp-chats/baseapp_chats/tests/test_graphql_queries.py(2 hunks)baseapp-chats/setup.cfg(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- baseapp-chats/baseapp_chats/tests/test_graphql_queries.py
- baseapp-chats/setup.cfg
⏰ Context from checks skipped due to timeout of 90000ms (17)
- GitHub Check: baseapp-payments / project-workflow (3.12, 5.0.8)
- GitHub Check: baseapp-pages / project-workflow (3.12, 5.0.8)
- GitHub Check: baseapp-profiles / project-workflow (3.12, 4.2.15)
- GitHub Check: baseapp-payments / project-workflow (3.12, 4.2.15)
- GitHub Check: baseapp-pages / project-workflow (3.12, 4.2.15)
- GitHub Check: baseapp-organizations / project-workflow (3.12, 5.0.8)
- GitHub Check: baseapp-message-templates / project-workflow (3.12, 5.0.8)
- GitHub Check: baseapp-notifications / project-workflow (3.12, 5.0.8)
- GitHub Check: baseapp-organizations / project-workflow (3.12, 4.2.15)
- GitHub Check: baseapp-message-templates / project-workflow (3.12, 4.2.15)
- GitHub Check: baseapp-follows / project-workflow (3.12, 5.0.8)
- GitHub Check: baseapp-notifications / project-workflow (3.12, 4.2.15)
- GitHub Check: baseapp-follows / project-workflow (3.12, 4.2.15)
- GitHub Check: baseapp-comments / project-workflow (3.12, 5.0.8)
- GitHub Check: baseapp-comments / project-workflow (3.12, 4.2.15)
- GitHub Check: baseapp-auth-baseapp_auth / project-workflow (3.12, 5.0.8)
- GitHub Check: baseapp-auth-baseapp_auth / project-workflow (3.12, 4.2.15)
🔇 Additional comments (1)
baseapp-chats/baseapp_chats/graphql/object_types.py (1)
122-134: Verify edge cases in the implementationThe current implementation needs verification for several edge cases:
- When a participant's
accepted_atis None- When messages exist with
createdexactly equal toaccepted_at- Race conditions between message creation and participant acceptance
Let's verify these edge cases with the following script:
✅ Verification successful
Edge cases are properly handled in the implementation
The test implementation demonstrates that:
- Participants always have
accepted_atset during creation- Messages created at exactly
accepted_attime are included (due to>=comparison)- Race conditions are prevented by atomic participant creation with deterministic timestamps
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential edge cases in the implementation # Check for participants with null accepted_at echo "Checking for participants with null accepted_at..." rg "accepted_at.*=.*None" -g "test_*.py" # Check for message filtering logic in tests echo "Checking message filtering test coverage..." rg "test.*message.*filter|test.*participant.*message" -g "test_*.py" # Check for race condition handling echo "Checking race condition handling..." rg "created.*=.*accepted_at|accepted_at.*=.*created" -g "test_*.py"Length of output: 608
Script:
#!/bin/bash # Check the test implementation echo "Test implementation:" rg -A 10 "test_new_participant_cant_list_previous_messages_when_joining_group_room" baseapp-chats/baseapp_chats/tests/test_graphql_queries.py # Look for participant model definition echo -e "\nParticipant model definition:" ast-grep --pattern 'class Participant($$$):' # Check for database migrations containing accepted_at echo -e "\nDatabase migrations for accepted_at:" rg "accepted_at" -g "*migrations*.py"Length of output: 948
Script:
#!/bin/bash # Check complete test implementation with more context echo "Complete test implementation:" rg -B 5 -A 20 "test_new_participant_cant_list_previous_messages_when_joining_group_room" baseapp-chats/baseapp_chats/tests/test_graphql_queries.py # Look for participant model in models.py echo -e "\nParticipant model:" rg -A 10 "class ChatRoomParticipant" baseapp-chats/baseapp_chats/models.py # Look for message creation in tests echo -e "\nMessage creation in tests:" rg "Message.*create|message.*=.*Message" -g "test_*.py" # Search for accepted_at field definition echo -e "\nAccepted at field definition:" rg "accepted_at.*=.*models\." baseapp-chats/baseapp_chats/models.pyLength of output: 3681
Script:
#!/bin/bash # Check abstract participant model echo "Abstract participant model:" rg -A 15 "class AbstractChatRoomParticipant" baseapp-chats/baseapp_chats/models.py # Get complete test implementation echo -e "\nComplete test implementation (including assertions):" rg -B 5 -A 30 "test_new_participant_cant_list_previous_messages_when_joining_group_room" baseapp-chats/baseapp_chats/tests/test_graphql_queries.py # Check for model validators echo -e "\nModel validators:" rg "def clean|def validate" baseapp-chats/baseapp_chats/models.pyLength of output: 1914
pt-tsl
force-pushed
the
BA-1943-hide-past-group-chat-messages
branch
from
dcbaa90 to
6ab4213
Compare
pt-tsl
force-pushed
the
BA-1943-hide-past-group-chat-messages
branch
from
4d98c5c to
6cd0e30
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 4d98c5cdf1a08352b988011dc9a6ad213d9be9ba and 6cd0e30907bfae5c9628c7bc8edfc488e1cb6365.
📒 Files selected for processing (1)
baseapp-core/setup.cfg(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: baseapp-drf-view-action-permissions / project-workflow (3.12, 5.0.8)
- GitHub Check: baseapp-e2e / project-workflow (3.12, 4.2.15)
- GitHub Check: baseapp-drf-view-action-permissions / project-workflow (3.12, 4.2.15)
- GitHub Check: baseapp-core / project-workflow (3.12, 5.0.8)
- GitHub Check: baseapp-comments / project-workflow (3.12, 5.0.8)
- GitHub Check: baseapp-core / project-workflow (3.12, 4.2.15)
- GitHub Check: baseapp-cloudflare-stream-field / project-workflow (3.12, 5.0.8)
- GitHub Check: baseapp-comments / project-workflow (3.12, 4.2.15)
- GitHub Check: baseapp-chats / project-workflow (3.12, 5.0.8)
- GitHub Check: baseapp-cloudflare-stream-field / project-workflow (3.12, 4.2.15)
- GitHub Check: baseapp-chats / project-workflow (3.12, 4.2.15)
- GitHub Check: baseapp / project-workflow (3.12, 5.0.8)
- GitHub Check: baseapp-auth-baseapp_auth / project-workflow (3.12, 5.0.8)
- GitHub Check: baseapp / project-workflow (3.12, 4.2.15)
- GitHub Check: baseapp-blocks / project-workflow (3.12, 5.0.8)
- GitHub Check: baseapp-auth-baseapp_referrals / project-workflow (3.12, 5.0.8)
- GitHub Check: baseapp-auth-baseapp_auth / project-workflow (3.12, 4.2.15)
- GitHub Check: baseapp-blocks / project-workflow (3.12, 4.2.15)
- GitHub Check: baseapp-auth-baseapp_referrals / project-workflow (3.12, 4.2.15)
|
pt-tsl
force-pushed
the
BA-1943-hide-past-group-chat-messages
branch
from
6cd0e30 to
8f8c8d2
Compare
pt-tsl
force-pushed
the
BA-1943-hide-past-group-chat-messages
branch
from
8f8c8d2 to
63386f0
Compare
pt-tsl
force-pushed
the
BA-1943-hide-past-group-chat-messages
branch
from
63386f0 to
80101a4
Compare
|
Acceptance Criteria
Display Messages based on time Participant Joined
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Migrations
is_groupfield to chat room model