baseapp-chats: update chats room title and image #179
Conversation
nossila
changed the title
Ronan-Fernandes
force-pushed
the
BA-1751-BE-update-room-name-and-image-url
branch
2 times, most recently
from
1f2be75 to
b3b969d
Compare
pt-tsl
force-pushed
the
BA-1737-profile-filter
branch
from
be6c2bb to
13ce906
Compare
pt-tsl
force-pushed
the
BA-1737-profile-filter
branch
from
13ce906 to
30aba02
Compare
WalkthroughThe changes introduce several enhancements to the Changes
Suggested reviewers
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 (
|
Ronan-Fernandes
force-pushed
the
BA-1751-BE-update-room-name-and-image-url
branch
from
f53c969 to
b3b969d
Compare
pt-tsl
force-pushed
the
BA-1737-profile-filter
branch
from
30aba02 to
143b6c8
Compare
pt-tsl
force-pushed
the
BA-1737-profile-filter
branch
from
143b6c8 to
a2f62ef
Compare
pt-tsl
force-pushed
the
BA-1751-BE-update-room-name-and-image-url
branch
from
b3b969d to
cfc4975
Compare
|
pt-tsl
force-pushed
the
BA-1751-BE-update-room-name-and-image-url
branch
from
9d2248f to
4ad772c
Compare
![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
🧹 Outside diff range and nitpick comments (10)
baseapp-chats/baseapp_chats/admin.py (2)
22-25: LGTM: Well-structured inline admin configurationThe TabularInline implementation with extra=0 is appropriate for managing related MessageStatus records.
Remove the extra blank line after the class definition to maintain consistent spacing with other classes in the file:
class MessageStatusInline(admin.TabularInline): model = MessageStatus extra = 0 -
22-24: Consider adding docstrings for better maintainabilityAdding docstrings to the MessageStatusInline class would help document its purpose and any special handling of message statuses.
Example addition:
class MessageStatusInline(admin.TabularInline): + """Inline admin interface for managing message read/delivery statuses. + + Allows viewing and editing message statuses directly within the message admin interface. + """ model = MessageStatus extra = 0baseapp-chats/baseapp_chats/graphql/filters.py (1)
29-41: Consider adding error handling and optimizing database queries.The implementation looks good overall, but there are a few potential improvements:
- The
user.profileaccess might cause an N+1 query problem- There's no explicit error handling for invalid profile_ids
- The distinct() call might impact performance on large datasets
Consider applying these improvements:
def filter_unread_messages(self, queryset, name, value): if value: profile_id = self.data.get("profile_id", None) - user_profile = self.request.user.profile - unread_messages_profile_pk = ( - get_pk_from_relay_id(profile_id) if profile_id else user_profile.pk - ) + try: + # Select profile_id in the same query to avoid N+1 + user_profile_id = self.request.user.profile.values_list('id', flat=True).get() + unread_messages_profile_pk = ( + get_pk_from_relay_id(profile_id) if profile_id else user_profile_id + ) + except Exception as e: + # Log the error and return empty queryset + logger.error(f"Error processing profile_id: {e}") + return queryset.none() return queryset.filter( unread_messages__profile_id=unread_messages_profile_pk, unread_messages__count__gt=0 ).distinct() return querysetbaseapp-chats/baseapp_chats/graphql/object_types.py (1)
72-83: Consider optimizing the resolve_is_read implementationThe implementation is functionally correct but could benefit from some improvements:
- Add error handling for invalid profile_id format
- Consider using select_related to optimize the database query
Here's a suggested optimization:
def resolve_is_read(self, info, profile_id=None, **kwargs): if profile_id: - profile_pk = get_pk_from_relay_id(profile_id) + try: + profile_pk = get_pk_from_relay_id(profile_id) + except Exception: + return None profile = Profile.objects.get_if_member(pk=profile_pk, user=info.context.user) if not profile: return None else: profile_pk = info.context.user.current_profile.pk - message_status = self.statuses.filter(profile_id=profile_pk).first() + message_status = self.statuses.select_related('profile').filter(profile_id=profile_pk).first() return message_status and message_status.is_readbaseapp-profiles/baseapp_profiles/tests/test_get_queries.py (6)
8-8: Remove unnecessary empty line between importsThis empty line breaks the logical grouping of imports. Consider removing it to maintain consistent import organization.
Line range hint
13-25: Rename query to match its functionalityThe query name
GET_PROFILE_BY_PATHsuggests path-based lookup, but it actually uses an ID. Consider renaming toGET_PROFILE_BY_IDfor clarity.
Line range hint
13-39: Add type hints to GraphQL query variablesConsider adding type hints to the query variables for better documentation and type safety:
- query Profile($id: ID!) { + query Profile($id: GlobalID!) { - query AllProfiles($q: String!) { + query AllProfiles($q: SearchString!) {
Line range hint
127-146: Fix incorrect assertion in permission testThe test
test_another_user_cant_view_membershas an incorrect assertion that contradicts its purpose. The test name suggests that another user shouldn't be able to view members, but the assertion expects the opposite.- assert content["data"]["profile"]["members"] + assert not content["data"]["profile"]["members"]
Line range hint
149-186: Improve test documentation and readabilityThe
test_search_profilesfunction would benefit from:
- A docstring explaining the test cases and search scenarios
- More descriptive variable names (e.g.,
profile_with_pathinstead ofprofile4)- Constants for test data (e.g.,
SEARCH_QUERY = "da")Example improvement:
def test_search_profiles(graphql_user_client): """Test profile search functionality. Tests: - Matches profiles with names starting with 'da' - Matches profiles with paths containing 'da' - Excludes non-matching profiles """ SEARCH_QUERY = "da" # Create test profiles matching_profile1 = ProfileFactory(name="David") matching_profile2 = ProfileFactory(name="Daniel") non_matching_profile1 = ProfileFactory(name="Mark") profile_with_path = ProfileFactory(name="John") non_matching_profile2 = ProfileFactory(name="Donald") # Set up URL path for one profile profile_path = URLPathFactory( path="danger.john", is_active=True, language=None, target_content_type=ContentType.objects.get_for_model(Profile), target_object_id=profile_with_path.id )
Line range hint
42-186: Consider adding tests for edge casesThe test suite would benefit from additional test cases:
- Error handling for invalid IDs
- Search with special characters or empty strings
- Pagination of search results
- Profile metadata with missing images
Would you like me to help generate these additional test cases?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📥 Commits
Reviewing files that changed from the base of the PR and between 1d303f8 and 9d2248f1780123810d15976567dd62dbb150cd53.
📒 Files selected for processing (8)
baseapp-chats/.coveragerc(1 hunks)baseapp-chats/baseapp_chats/admin.py(2 hunks)baseapp-chats/baseapp_chats/base.py(1 hunks)baseapp-chats/baseapp_chats/graphql/filters.py(2 hunks)baseapp-chats/baseapp_chats/graphql/object_types.py(6 hunks)baseapp-chats/baseapp_chats/migrations/0003_alter_unreadmessagecount_room.py(1 hunks)baseapp-chats/setup.cfg(1 hunks)baseapp-profiles/baseapp_profiles/tests/test_get_queries.py(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- baseapp-chats/.coveragerc
- baseapp-chats/setup.cfg
🔇 Additional comments (13)
baseapp-chats/baseapp_chats/migrations/0003_alter_unreadmessagecount_room.py (2)
1-25: LGTM! Migration is well-structured and follows Django conventions.
The migration is properly configured with correct dependencies and uses the swappable model pattern appropriately.
19-23: Verify CASCADE deletion behavior and related_name usage.
The migration introduces two significant changes that need verification:
on_delete=CASCADEwill delete unread message counts when a room is deletedrelated_name="unread_messages"enables reverse lookups from rooms
Let's verify the usage of these features in the codebase:
✅ Verification successful
Based on the results, I can now generate the final response:
CASCADE deletion and related_name implementation verified successfully
The codebase analysis confirms proper implementation:
- The
unread_messagesrelated_name is actively used in GraphQL queries and filters for managing unread message counts - No custom deletion handlers that could conflict with CASCADE behavior
- The related_name is properly referenced in filters using
unread_messages__profile_idandunread_messages__count__gt=0
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential issues with CASCADE deletion and related_name usage
# Check for any cleanup/deletion logic that might need updating
rg -A 3 "delete.*room" --type py
# Check if the new related_name is being used correctly
rg -A 3 "unread_messages" --type py
# Look for any existing room deletion handlers that might need updating
ast-grep --pattern 'def pre_delete($$$) { $$$room$$$}'
Length of output: 4960
baseapp-chats/baseapp_chats/admin.py (2)
7-7: LGTM: MessageStatus model import follows established patterns
The import is consistent with the file's existing model loading approach using swapper, which maintains model swappability.
32-32: LGTM: MessageStatus inline integration
Adding MessageStatusInline to MessageAdmin provides a good user experience for managing related message statuses.
Let's verify the relationship between Message and MessageStatus models:
✅ Verification successful
Relationship between Message and MessageStatus is properly configured
The relationship is well-defined:
- MessageStatus has a ForeignKey to Message with
related_name="statuses" - The relationship is properly constrained with
unique_together = ["message_id", "profile_id"] - The model includes relevant fields for tracking read status and timestamps
- Database triggers are set up to handle message status updates and unread counts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Message and MessageStatus relationship
# Check for ForeignKey or related field in MessageStatus model
ast-grep --pattern 'class MessageStatus$$$:
$$$
message = $$$ForeignKey($$$Message$$$)
$$$'
# Check for related_name in the relationship
rg -A 2 'message.*ForeignKey.*Message'
Length of output: 140
Script:
#!/bin/bash
# Let's try a broader search to find the MessageStatus model definition and its relationship
# Find files containing MessageStatus class definition
fd -e py | xargs rg -l "class MessageStatus"
# Search for any field definitions related to Message in those files
rg "message.*=.*models\."
# Also search for any reverse relationship from Message to MessageStatus
fd -e py | xargs rg -l "class Message"
rg "statuses.*=.*models\."
Length of output: 945
Script:
#!/bin/bash
# Let's examine the MessageStatus model definition in models.py
cat baseapp-chats/baseapp_chats/models.py
# Also check the factory to understand the relationship better
cat baseapp-chats/baseapp_chats/tests/factories.py
Length of output: 2978
Script:
#!/bin/bash
# Let's check the AbstractMessageStatus and AbstractBaseMessage definitions
cat baseapp-chats/baseapp_chats/base.py
Length of output: 7061
baseapp-chats/baseapp_chats/graphql/filters.py (3)
12-12: LGTM! Clear and well-structured filter declaration.
The boolean filter for unread messages follows Django Filter conventions and uses appropriate naming.
37-39: Verify the unread_messages related_name configuration.
The filter assumes the existence of an unread_messages related_name on the ChatRoom model. Let's verify this relationship is properly set up.
✅ Verification successful
Based on the verification results, I can now generate the final response:
The unread_messages related_name is properly configured
The relationship is correctly set up in both the model definition and migrations:
- The
AbstractUnreadMessageCountmodel inbase.pydefines the ForeignKey withrelated_name="unread_messages" - Migration
0003_alter_unreadmessagecount_room.pyconfirms this configuration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the unread_messages relationship configuration
# Check for the related_name in models
rg -A 5 'class.*UnreadMessageCount.*:|ForeignKey.*room'
# Check for any existing migrations that might conflict
fd -e py -x grep -l "unread_messages" {} \; -p "migrations"
Length of output: 1167
Script:
#!/bin/bash
# Let's verify the complete model structure and migrations
# Check the content of the migration file that modified unread_messages
cat baseapp-chats/baseapp_chats/migrations/0003_alter_unreadmessagecount_room.py
# Check the complete UnreadMessageCount model definition
ast-grep --pattern 'class AbstractUnreadMessageCount($_) {
$$$
}'
Length of output: 883
23-23: Verify the necessity of distinct() for query performance.
The addition of .distinct() prevents duplicate results from the OR condition with participant joins, which is good. However, let's verify if it's actually needed in common query patterns.
✅ Verification successful
The distinct() call is necessary and correctly implemented
The codebase analysis confirms that .distinct() is necessary because:
- The
filter_qmethod uses an OR condition withparticipants__profile__name__icontains, which can produce duplicate chat rooms when a room has multiple participants matching the search term - Similar patterns in the codebase (e.g., in profiles, notifications) consistently use
.distinct()when querying with OR conditions on related fields - The same file also uses
.distinct()in thefilter_unread_messagesmethod, showing a consistent pattern for handling potential duplicates in chat room queries
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the query patterns in the codebase commonly result in duplicate rows
# Look for GraphQL queries that might cause duplicates due to participant joins
# Search for GraphQL queries using this filter with participant-related fields
rg -A 5 'ChatRoomFilter.*participants'
# Check if there are other similar patterns in the codebase that might need distinct
ast-grep --pattern 'filter(
Q($$$) | Q($$$)
)'
Length of output: 91
Script:
#!/bin/bash
# Let's try a different approach to verify the query patterns
# Check the implementation of the filter method and surrounding context
rg -l "class ChatRoomFilter" | xargs cat
# Look for OR conditions in filter queries that might cause duplicates
rg -B 2 -A 2 "\|.*Q\("
# Check for participant-related joins or filters
rg -B 2 -A 2 "participants.*filter|filter.*participants"
# Look for distinct usage patterns
rg -B 2 -A 2 "\.distinct\(\)"
Length of output: 13289
baseapp-chats/baseapp_chats/graphql/object_types.py (4)
5-5: LGTM: Import added for new image field
The ThumbnailField import is correctly added and properly grouped with related imports.
42-42: LGTM: Well-structured is_read field addition
The is_read field is properly defined with an optional profile_id parameter and correctly included in Meta.fields.
Also applies to: 58-58
95-95: LGTM: Image field properly added
The image field is correctly defined as an optional ThumbnailField.
Let's verify the ThumbnailField implementation:
✅ Verification successful
ThumbnailField implementation verified and properly used
The ThumbnailField is correctly implemented in baseapp-core and properly used in the chat room type. It:
- Extends graphene.Field with width/height arguments
- Uses Django's easy-thumbnails library for image processing
- Implements caching for better performance
- Returns absolute URLs for the generated thumbnails
- Is consistently used across other parts of the codebase (auth, profiles, pages)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check ThumbnailField implementation and usage
ast-grep --pattern 'class ThumbnailField {
$$$
}'
Length of output: 53
Script:
#!/bin/bash
# Let's try searching for ThumbnailField using ripgrep to find its definition and imports
rg "ThumbnailField" -A 3
Length of output: 4556
Script:
#!/bin/bash
# Let's check the implementation of ThumbnailField in baseapp-core
cat baseapp-core/baseapp_core/graphql/fields.py
Length of output: 1870
130-130: Verify the impact of renaming 'name' to 'title'
This is a breaking change that will affect existing GraphQL queries using the 'name' field.
Let's check for existing usage:
Please ensure that:
- All clients are updated to use 'title' instead of 'name'
- A proper migration path is provided for this breaking change
baseapp-chats/baseapp_chats/base.py (2)
191-191: LGTM! Semantically correct related_name.
The related_name="unread_messages" is a good choice as it accurately describes the reverse relationship and follows Django naming conventions.
191-191: Verify backwards compatibility of the related_name change.
Adding a related_name where none existed before could break existing code that relies on Django's default reverse accessor. Please ensure no existing code uses the default related name.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📥 Commits
Reviewing files that changed from the base of the PR and between 9d2248f1780123810d15976567dd62dbb150cd53 and 4ad772c79510263c8207debdaedc665dc86def10.
📒 Files selected for processing (5)
baseapp-chats/baseapp_chats/admin.py(2 hunks)baseapp-chats/baseapp_chats/base.py(1 hunks)baseapp-chats/baseapp_chats/graphql/filters.py(2 hunks)baseapp-chats/baseapp_chats/graphql/object_types.py(6 hunks)baseapp-chats/baseapp_chats/migrations/0003_alter_unreadmessagecount_room.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- baseapp-chats/baseapp_chats/admin.py
- baseapp-chats/baseapp_chats/base.py
- baseapp-chats/baseapp_chats/graphql/object_types.py
- baseapp-chats/baseapp_chats/migrations/0003_alter_unreadmessagecount_room.py
🔇 Additional comments (3)
baseapp-chats/baseapp_chats/graphql/filters.py (3)
12-12: LGTM: Boolean filter declaration is appropriate
The unread_messages filter is correctly declared as a boolean filter with proper method reference.
22-23: Verify the performance impact of distinct()
The addition of .distinct() is necessary to prevent duplicate results from the OR condition and participant joins. However, this could impact performance on large datasets.
Let's check the query complexity:
✅ Verification successful
Let me gather more information about the ChatRoom model and its relationships to better assess the impact of distinct().
Based on the gathered information, I can now provide a final response about the distinct() performance concern:
The distinct() call is appropriate and well-optimized
The codebase analysis reveals that:
- The ChatRoom model has proper indexing on key fields (verb, room, action_object)
- The query is already optimized with ordering by "-last_message_time" and "-created"
- The distinct() operation is necessary to prevent duplicates from the OR condition with participant joins
- There are no reported performance issues or TODOs related to ChatRoom queries
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for potential performance bottlenecks in the codebase
# by checking for large ChatRoom queries or high participant counts
# Check for large queries involving ChatRoom
rg -B 3 -A 3 "ChatRoom\.objects\.filter|ChatRoom\.objects\.all"
# Look for any performance-related comments or TODOs
rg -i "todo.*performance|slow.*query|optimize" --type py
Length of output: 2394
Script:
#!/bin/bash
# Check the ChatRoom model definition and its relationships
ast-grep --pattern 'class ChatRoom($$$)'
# Look for indexes on the relevant fields
rg -B 2 -A 2 "class Meta|index_together|indexes" baseapp-chats/
# Check if there are any existing database optimizations
rg -B 2 -A 2 "db_index=True|unique=True" baseapp-chats/
Length of output: 11621
37-39: Verify related_name usage and test coverage
Let's verify:
- The correct usage of
unread_messagesrelated_name - Existence of proper test coverage for this filter
✅ Verification successful
Let me gather more information to verify the related_name usage and test coverage.
Let me check the model definition and relationship one final time to complete the verification.
The unread_messages filter implementation is properly tested and correctly used
The verification shows:
- The
unread_messagesrelated_name is properly defined in the model (found in base.py and migrations) - The filter is well-tested in
test_unread_messageswhich verifies:- Message creation scenario
- Unread count verification through GraphQL query
- Correct filtering behavior with profile-specific counts
The implementation is consistent with the test coverage and the related_name usage is verified across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify related_name usage and test coverage
# Check related_name definition
rg -A 2 "room.*=.*ForeignKey.*ChatRoom" --type py
# Look for test files covering this filter
rg -l "class.*ChatRoomFilter.*Test" --type py
# Check test coverage for unread_messages filter
rg "def test.*unread.*messages" --type py
Length of output: 268
Script:
#!/bin/bash
# Check the model definition for unread_messages
ast-grep --pattern 'class UnreadMessage'
# Check the test implementation for unread messages
rg -B 5 -A 10 "def test_unread_messages" --type py
# Look for any other usages of unread_messages in filters
rg "unread_messages" --type py
Length of output: 3099
Script:
#!/bin/bash
# Check the UnreadMessageCount model definition
ast-grep --pattern 'class UnreadMessageCount'
# Check the ChatRoom model definition
ast-grep --pattern 'class ChatRoom'
# Look for the full test implementation
cat baseapp-chats/baseapp_chats/tests/test_graphql_queries.py
Length of output: 8903
pt-tsl
force-pushed
the
BA-1751-BE-update-room-name-and-image-url
branch
from
4ad772c to
989cecc
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
baseapp-chats/baseapp_chats/graphql/object_types.py (1)
72-83: Consider adding error handling and query optimizationThe resolver implementation is good but could be enhanced:
- Add error handling for invalid profile_ids
- Consider optimizing the database query if the statuses relationship has foreign keys
def resolve_is_read(self, info, profile_id=None, **kwargs): if profile_id: - profile_pk = get_pk_from_relay_id(profile_id) + try: + profile_pk = get_pk_from_relay_id(profile_id) + except (ValueError, TypeError): + return None profile = Profile.objects.get_if_member(pk=profile_pk, user=info.context.user) if not profile: return None else: profile_pk = info.context.user.current_profile.pk - message_status = self.statuses.filter(profile_id=profile_pk).first() + message_status = self.statuses.select_related('profile').filter(profile_id=profile_pk).first() return message_status and message_status.is_read
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📥 Commits
Reviewing files that changed from the base of the PR and between 4ad772c79510263c8207debdaedc665dc86def10 and 989ceccf39c23cfbee9df0946cfaba5d273404d7.
📒 Files selected for processing (5)
baseapp-chats/baseapp_chats/admin.py(2 hunks)baseapp-chats/baseapp_chats/base.py(1 hunks)baseapp-chats/baseapp_chats/graphql/filters.py(2 hunks)baseapp-chats/baseapp_chats/graphql/object_types.py(6 hunks)baseapp-chats/baseapp_chats/migrations/0003_alter_unreadmessagecount_room.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- baseapp-chats/baseapp_chats/admin.py
- baseapp-chats/baseapp_chats/base.py
- baseapp-chats/baseapp_chats/graphql/filters.py
- baseapp-chats/baseapp_chats/migrations/0003_alter_unreadmessagecount_room.py
🔇 Additional comments (4)
baseapp-chats/baseapp_chats/graphql/object_types.py (4)
5-5: LGTM: Import added for new ThumbnailField
The import is correctly placed and necessary for the new image field functionality.
42-42: LGTM: Well-structured is_read field definition
The field is correctly defined with an optional profile_id parameter and properly included in the Meta fields list.
Also applies to: 58-58
95-95: LGTM: Well-defined optional image field
The image field is correctly implemented as an optional ThumbnailField.
130-130: Verify the impact of renaming 'name' to 'title'
The change from name to title in the fields tuple could be a breaking change. Please ensure:
- The corresponding database model field has been updated
- All GraphQL queries/mutations using this field have been updated
- Clients are aware of this breaking change
|
pt-tsl
force-pushed
the
BA-1751-BE-update-room-name-and-image-url
branch
from
59bb4ae to
6f116e5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
sonar-project.properties (1)
4-4: Note: Redundant migration exclusions.The
apps/**/migrations/**pattern appears in bothsonar.exclusionsandsonar.coverage.exclusions. While this redundancy doesn't cause issues, you might want to consider keeping it only insonar.exclusionssince migrations are already excluded from all analysis.Also applies to: 7-7
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📥 Commits
Reviewing files that changed from the base of the PR and between 989ceccf39c23cfbee9df0946cfaba5d273404d7 and 6f116e5f1a6a685f761732301b7ebfab79b1d798.
📒 Files selected for processing (1)
sonar-project.properties(1 hunks)
🔇 Additional comments (1)
sonar-project.properties (1)
7-7: LGTM! More specific exclusion pattern.
The updated pattern provides better granularity by specifically targeting test and migration files within the apps directory, rather than excluding all test files globally. This change allows for better coverage tracking of any implementation code that might exist in test files outside the apps directory.
pt-tsl
force-pushed
the
BA-1751-BE-update-room-name-and-image-url
branch
from
6f116e5 to
b2b1ff1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
baseapp-chats/baseapp_chats/graphql/object_types.py (1)
72-83: Consider improving error handling and query efficiencyWhile the resolver implementation is functional, consider these improvements:
- Add explicit error handling for invalid profile_id format
- Optimize the message status query by using select_related
- Consider returning a more explicit error for invalid profile_id instead of None
Here's a suggested implementation:
def resolve_is_read(self, info, profile_id=None, **kwargs): if profile_id: - profile_pk = get_pk_from_relay_id(profile_id) + try: + profile_pk = get_pk_from_relay_id(profile_id) + except (ValueError, TypeError): + raise GraphQLError("Invalid profile ID format") profile = Profile.objects.get_if_member(pk=profile_pk, user=info.context.user) if not profile: - return None + raise GraphQLError("Profile not found or access denied") else: profile_pk = info.context.user.current_profile.pk - message_status = self.statuses.filter(profile_id=profile_pk).first() + message_status = self.statuses.select_related('profile').filter(profile_id=profile_pk).first() return message_status and message_status.is_read
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📥 Commits
Reviewing files that changed from the base of the PR and between 6f116e5f1a6a685f761732301b7ebfab79b1d798 and b2b1ff1308597f721a0c7b7b51d247491672aa9e.
📒 Files selected for processing (5)
baseapp-chats/baseapp_chats/admin.py(2 hunks)baseapp-chats/baseapp_chats/base.py(1 hunks)baseapp-chats/baseapp_chats/graphql/filters.py(2 hunks)baseapp-chats/baseapp_chats/graphql/object_types.py(6 hunks)baseapp-chats/baseapp_chats/migrations/0003_alter_unreadmessagecount_room.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- baseapp-chats/baseapp_chats/admin.py
- baseapp-chats/baseapp_chats/base.py
- baseapp-chats/baseapp_chats/graphql/filters.py
- baseapp-chats/baseapp_chats/migrations/0003_alter_unreadmessagecount_room.py
🔇 Additional comments (4)
baseapp-chats/baseapp_chats/graphql/object_types.py (4)
5-5: LGTM: Import added for new image field
The ThumbnailField import is correctly added to support the new image field in BaseChatRoomObjectType.
42-42: LGTM: is_read field properly added
The is_read field is well-defined with an optional profile_id parameter and correctly included in Meta.fields.
Also applies to: 58-58
95-95: LGTM: Image field properly added
The ThumbnailField is correctly added as an optional field. However, let's verify the thumbnail configuration settings.
✅ Verification successful
ThumbnailField is properly configured and ready to use
The ThumbnailField implementation in baseapp-core is properly set up with:
- Easy-thumbnails integration for image processing
- Caching support through THUMBNAIL_CACHE setting
- Width and height parameters for dynamic resizing
- Absolute URL generation for the thumbnails
The field will work as expected in the chat room implementation without requiring additional configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for thumbnail configuration settings
rg -A 5 "THUMBNAIL_ALIASES|THUMBNAIL_PROCESSORS" settings.py config/*.py
Length of output: 180
Script:
#!/bin/bash
# Let's first find any settings or config files in the repository
fd -t f "settings|config" --full-path
# Also search for thumbnail related settings in Python files
rg -A 5 "THUMBNAIL_ALIASES|THUMBNAIL_PROCESSORS|THUMBNAIL" "*.py"
# Check if sorl-thumbnail is used
fd -t f "requirements.txt" --exec grep -i "sorl"
Length of output: 1646
Script:
#!/bin/bash
# Let's check the settings file in baseapp-chats and its dependencies
cat baseapp-chats/testproject/settings.py
# Let's also check what type of field ThumbnailField is by finding its definition
rg -A 5 "class ThumbnailField" baseapp-chats/
# Check if there are any thumbnail-related settings in baseapp-core settings
cat baseapp-core/baseapp_core/settings/__init__.py
Length of output: 1691
Script:
#!/bin/bash
# Let's find where ThumbnailField is defined in the codebase
fd -t f -e py . --exec rg -l "class ThumbnailField"
# Also check for any thumbnail-related imports
rg "from.*import.*Thumbnail" -A 2
Length of output: 1374
Script:
#!/bin/bash
# Let's check the implementation of ThumbnailField in baseapp-core
cat baseapp-core/baseapp_core/graphql/fields.py
# Also check if there are any thumbnail-related settings in baseapp-core
rg "THUMBNAIL" baseapp-core/baseapp_core/
Length of output: 2169
130-130:
Breaking Change: 'name' field renamed to 'title'
This change from 'name' to 'title' in the fields tuple is a breaking change that will affect existing GraphQL queries. Please ensure:
- All client applications are updated to use 'title' instead of 'name'
- A migration path is provided for existing clients
Ronan-Fernandes
force-pushed
the
BA-1751-BE-update-room-name-and-image-url
branch
from
b2b1ff1 to
0e310c2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
baseapp-chats/baseapp_chats/graphql/object_types.py (1)
72-83: Consider optimizing resolver performance and adding error handlingThe resolver implementation is logically correct but has two areas for improvement:
- Performance: Consider prefetching message statuses to avoid N+1 queries
- Error Handling: Add try-catch for invalid profile_id format
Consider this implementation:
def resolve_is_read(self, info, profile_id=None, **kwargs): + try: if profile_id: profile_pk = get_pk_from_relay_id(profile_id) profile = Profile.objects.get_if_member(pk=profile_pk, user=info.context.user) if not profile: return None else: profile_pk = info.context.user.current_profile.pk message_status = self.statuses.filter(profile_id=profile_pk).first() return message_status and message_status.is_read + except (ValueError, TypeError): + return NoneFor the N+1 query optimization, consider adding a DataLoader or using
select_related()at the query level.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📥 Commits
Reviewing files that changed from the base of the PR and between b2b1ff1308597f721a0c7b7b51d247491672aa9e and 0e310c22fab32b5d27cc9532d3700ba3c4ac64bf.
📒 Files selected for processing (5)
baseapp-chats/baseapp_chats/admin.py(2 hunks)baseapp-chats/baseapp_chats/base.py(1 hunks)baseapp-chats/baseapp_chats/graphql/filters.py(2 hunks)baseapp-chats/baseapp_chats/graphql/object_types.py(6 hunks)baseapp-chats/baseapp_chats/migrations/0003_alter_unreadmessagecount_room.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- baseapp-chats/baseapp_chats/admin.py
- baseapp-chats/baseapp_chats/base.py
- baseapp-chats/baseapp_chats/graphql/filters.py
- baseapp-chats/baseapp_chats/migrations/0003_alter_unreadmessagecount_room.py
🔇 Additional comments (3)
baseapp-chats/baseapp_chats/graphql/object_types.py (3)
5-5: LGTM: Import added for new image field
The ThumbnailField import is correctly added and properly grouped with other baseapp_core.graphql imports.
42-42: LGTM: is_read field properly added to schema
The field is correctly defined with an optional profile_id parameter and properly included in the fields list.
Also applies to: 58-58
95-95: Verify the impact of renaming 'name' to 'title'
The changes look correct, but renaming the field from 'name' to 'title' could be a breaking change for existing GraphQL queries.
Let's verify the usage of this field in the codebase:
Also applies to: 130-130
✅ Verification successful
Let me analyze the GraphQL schema and operations to verify if this is a breaking change.
Let me try to find GraphQL schema and query definitions to verify the impact of the field rename.
Let me try to find the model and GraphQL type definitions to verify the field rename impact.
Based on the search results, I can now provide a definitive response about the field rename impact:
The field rename from 'name' to 'title' is safe to proceed
The codebase search shows that:
- The field is already defined as
titlein the model (AbstractBaseChatRoom.title) - The GraphQL type correctly exposes
titlein its fields tuple (BaseChatRoomObjectType.Meta.fields) - The field is used consistently as
titlein filters (ChatRoomFilter.filter_qusestitle__icontains) - No references to a 'name' field were found in GraphQL queries or mutations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for GraphQL queries using the 'name' field
rg -t graphql -t js -t py 'name.*ChatRoom|ChatRoom.*name'
# Search for potential test files that might need updates
fd -e test.py -e spec.js | xargs rg 'name.*ChatRoom|ChatRoom.*name'
Length of output: 1036
Script:
#!/bin/bash
# Search for GraphQL queries and fragments in Python files
rg -t py 'class.*ChatRoom.*Fragment|class.*ChatRoom.*Query|query.*chatRoom|fragment.*chatRoom' -A 5
# Search for GraphQL operations in all files
rg 'query.*\{.*chatRoom|fragment.*\{.*chatRoom' -A 5
# Search for field name in object type definition
ast-grep --pattern 'class BaseChatRoomObjectType($$$) {
$$$
}'
Length of output: 223
Script:
#!/bin/bash
# Let's try a different approach to find GraphQL related files and their content
fd -e graphql . || true
fd -e gql . || true
# Search for ChatRoom related queries in Python files with more context
rg -t py "class.*ChatRoom" -A 10
# Look for any files that might contain GraphQL queries
fd . -t f -x file {} | grep -i "graphql"
Length of output: 57858
Script:
#!/bin/bash
# Let's try a simpler search for the field name in Python files
rg -t py "title.*=.*models\.CharField|name.*=.*models\.CharField" -B 2 -A 2
# Search for field references in GraphQL related files
rg -t py "class.*BaseChatRoom.*|class.*ChatRoom.*" -B 5 -A 15
# Look for any GraphQL schema definitions
rg -t py "class.*Meta.*fields.*=.*\(.*title.*\)|class.*Meta.*fields.*=.*\(.*name.*\)" -B 5 -A 2
Length of output: 35985
Ronan-Fernandes
force-pushed
the
BA-1751-BE-update-room-name-and-image-url
branch
from
0e310c2 to
2d4e314
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
baseapp-chats/baseapp_chats/graphql/filters.py (2)
Line range hint
19-23: Add docstring to explain the query filtering logicThe
filter_qmethod uses complex Q objects for filtering, but lacks documentation explaining its purpose and behavior.Consider adding a docstring:
def filter_q(self, queryset, name, value): + """Filter chat rooms by title or participant name. + + Args: + value (str): Search term to filter by + + Returns: + QuerySet: Filtered chat rooms matching title or participant name + """ return queryset.filter( Q(title__icontains=value) | Q(participants__profile__name__icontains=value) ).distinct()
29-50: Improve code structure and documentationConsider these good-to-have improvements:
- Add docstring explaining the filtering logic
- Simplify the profile_id handling logic
Apply these improvements:
def filter_unread_messages(self, queryset, name, value): + """Filter chat rooms by unread messages for a given profile. + + Args: + value (bool): When True, returns only rooms with unread messages + + Returns: + QuerySet: Filtered chat rooms + """ if not self.request.user.is_authenticated: return queryset.none() if value: - profile_id = self.data.get("profile_id", None) try: user_profile = self.request.user.profile except AttributeError: return queryset.none() - unread_messages_profile_pk = ( - get_pk_from_relay_id(profile_id) if profile_id else user_profile.pk - ) + profile_id = self.data.get("profile_id") + profile_pk = get_pk_from_relay_id(profile_id) if profile_id else user_profile.pk return ( queryset.select_related("unread_messages") .filter( - unread_messages__profile_id=unread_messages_profile_pk, + unread_messages__profile_id=profile_pk, unread_messages__count__gt=0, ) .distinct() ) return queryset
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📥 Commits
Reviewing files that changed from the base of the PR and between 0e310c22fab32b5d27cc9532d3700ba3c4ac64bf and 2d4e314.
📒 Files selected for processing (5)
baseapp-chats/baseapp_chats/admin.py(2 hunks)baseapp-chats/baseapp_chats/base.py(1 hunks)baseapp-chats/baseapp_chats/graphql/filters.py(2 hunks)baseapp-chats/baseapp_chats/graphql/object_types.py(6 hunks)baseapp-chats/baseapp_chats/migrations/0003_alter_unreadmessagecount_room.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- baseapp-chats/baseapp_chats/admin.py
- baseapp-chats/baseapp_chats/base.py
- baseapp-chats/baseapp_chats/graphql/object_types.py
- baseapp-chats/baseapp_chats/migrations/0003_alter_unreadmessagecount_room.py
🔇 Additional comments (2)
baseapp-chats/baseapp_chats/graphql/filters.py (2)
12-12: LGTM: Filter field declaration follows conventions
The unread_messages BooleanFilter is properly declared and linked to its filter method.
29-50:
Add authentication check and optimize query performance
The implementation still needs some critical improvements:
- Missing authentication validation before processing
prefetch_relatedmight not be the optimal choice here since we're filtering on the relation
Apply these improvements:
def filter_unread_messages(self, queryset, name, value):
+ if not self.request.user.is_authenticated:
+ return queryset.none()
+
if value:
profile_id = self.data.get("profile_id", None)
try:
user_profile = self.request.user.profile
except AttributeError:
return queryset.none()
unread_messages_profile_pk = (
get_pk_from_relay_id(profile_id) if profile_id else user_profile.pk
)
return (
- queryset.prefetch_related("unread_messages")
+ queryset.select_related("unread_messages")
.filter(
unread_messages__profile_id=unread_messages_profile_pk,
unread_messages__count__gt=0,
)
.distinct()
)
return queryset
|
Updates the BA Chats room title and room image query on graphql
Summary by CodeRabbit
Release Notes
New Features
MessageStatusentries within theMessageadmin panel.unread_messagesin chat room queries to easily identify rooms with unread messages.is_readfor messages andimagefor chat rooms.roomfield in the unread message count model for better reverse access.Chores