[WEB-2776] fix: restrict notifications #6081
Conversation
WalkthroughThe changes introduce a new import, Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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: 1
🧹 Outside diff range and nitpick comments (2)
apiserver/plane/bgtasks/notification_task.py (2)
249-251: Consider using a single database query for performanceThe current implementation first filters project members and then converts their IDs to strings in separate operations.
Consider combining these operations into a single query:
-new_mentions = list(ProjectMember.objects.filter( - project_id=project_id, member_id__in=new_mentions, is_active=True -).values_list("member_id", flat=True)) -new_mentions = [str(member_id) for member_id in new_mentions] +new_mentions = list( + ProjectMember.objects + .filter(project_id=project_id, member_id__in=new_mentions, is_active=True) + .annotate(member_str=Cast('member_id', output_field=CharField())) + .values_list('member_str', flat=True) +)
Line range hint
1-700: Consider adding tests for project member notification restrictionsThe changes introduce new business logic for notification restrictions, but there might not be sufficient test coverage.
Would you like me to help create test cases to verify:
- Notifications are only sent to active project members
- Notifications are not sent to inactive project members
- Notifications are properly handled when a project member becomes inactive
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
apiserver/plane/bgtasks/notification_task.py(4 hunks)
🧰 Additional context used
🪛 Ruff
apiserver/plane/bgtasks/notification_task.py
296-296: Line too long (137 > 88)
(E501)
🔇 Additional comments (3)
apiserver/plane/bgtasks/notification_task.py (3)
19-19: LGTM: Import addition aligns with the changes
The addition of ProjectMember import is necessary for the new project member validation logic.
98-99: LGTM: Project member validation added
Good addition of project member validation to ensure notifications are only sent to active project members.
Line range hint 1-700: Verify error handling for inactive project members
The changes ensure that notifications are only sent to active project members, but we should verify the error handling when a user becomes inactive.
✅ Verification successful
Let me gather more information about how notifications and mentions are handled with respect to project member status.
Let me try a different approach to check how notifications and project member status are handled.
Based on the findings, I can now provide a final response:
Proper handling of inactive project members is verified
The code already implements proper handling of inactive project members in notifications:
- New mentions are filtered to only include active project members through
ProjectMember.objects.filter(..., is_active=True) - Subscribers are only created for active project members via the
ProjectMember.objects.filter(..., is_active=True).exists()check - The filtering happens before any notification or mention processing, preventing notifications to inactive members
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there's proper error handling when a project member becomes inactive
# Look for any cleanup or status update logic
# Search for related model signals or handlers
rg -A 5 "pre_save|post_save|pre_delete|post_delete.*ProjectMember"
# Look for any cleanup tasks related to project member status changes
rg -A 5 "is_active.*ProjectMember|ProjectMember.*is_active"
Length of output: 1476
Script:
#!/bin/bash
# Check how notifications and mentions handle project member status
# Look for ProjectMember filters in notification related code
rg -A 5 "ProjectMember.*filter.*notification"
# Check for is_active checks in notification and mention related code
rg -A 5 "is_active.*notification|notification.*is_active"
# Look for any error handling in notification tasks
ast-grep --pattern 'try {
$$$
ProjectMember $$$
$$$
} catch ($$$) {
$$$
}'
# Check for any cleanup tasks when project member becomes inactive
rg -A 5 "def.*deactivate.*project.*member"
Length of output: 237
Script:
#!/bin/bash
# Search for ProjectMember related filters in the notification task file
rg -A 5 "ProjectMember" apiserver/plane/bgtasks/notification_task.py
# Look for any cleanup or status handling in ProjectMember model
fd ProjectMember.py -x cat {}
# Check for any notification cleanup tasks
rg -A 5 "notification.*clean|clean.*notification"
# Look for any task that handles project member status changes
rg -A 5 "@shared_task.*project.*member"
Length of output: 937
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
apiserver/plane/utils/issue_filters.py (2)
Line range hint
204-204: Consider index optimization for assignee filteringThe addition of
deleted_at__isnull=Truefilter for assignees is correct. However, this might impact query performance if thedeleted_atcolumn isn't properly indexed.Consider adding a database index on the
deleted_atcolumn of theissue_assigneetable to optimize these queries, especially if they're frequently used in the notification system. Example migration:from django.db import migrations class Migration(migrations.Migration): dependencies = [ ('your_app', 'previous_migration'), ] operations = [ migrations.AddIndex( model_name='IssueAssignee', index=migrations.Index(fields=['deleted_at'], name='idx_issue_assignee_deleted_at'), ), ]
188-188: Consider implementing a reusable filter for deleted entitiesThe same
deleted_at__isnull=Truecondition is repeated across multiple filter functions. This could be abstracted into a reusable function to maintain DRY principles.Consider implementing a helper function:
+ def add_not_deleted_filter(issue_filter, prefix, relation): + issue_filter[f"{prefix}{relation}__deleted_at__isnull"] = True + return issue_filter def filter_labels(params, issue_filter, method, prefix=""): # ... existing code ... - issue_filter[f"{prefix}label_issue__deleted_at__isnull"] = True + issue_filter = add_not_deleted_filter(issue_filter, prefix, "label_issue") return issue_filter def filter_assignees(params, issue_filter, method, prefix=""): # ... existing code ... - issue_filter[f"{prefix}issue_assignee__deleted_at__isnull"] = True + issue_filter = add_not_deleted_filter(issue_filter, prefix, "issue_assignee") return issue_filterAlso applies to: 204-204
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
apiserver/plane/utils/issue_filters.py(1 hunks)
🔇 Additional comments (2)
apiserver/plane/utils/issue_filters.py (2)
188-188: Ensure consistent handling of deleted labels
The addition of deleted_at__isnull=True filter is correct for excluding deleted labels. However, consider the following improvements:
- The filter should be applied before other label filters to optimize query performance
- This change might affect existing queries that intentionally included deleted labels
Let's verify if there are any direct queries that might be affected:
✅ Verification successful
The deleted_at__isnull=True filter for labels is already consistently implemented
The search results show that the label_issue__deleted_at__isnull=True condition is already consistently applied across various queries in the codebase. This includes:
- Direct issue filters
- Complex Q object queries
- Draft issue queries
- Related issue queries
The implementation aligns with the existing pattern and there's no evidence of queries intentionally including deleted labels. The change in issue_filters.py maintains this consistency.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct label queries that might be affected
rg -l "label_issue" | xargs -I {} rg "label_issue.*deleted_at" {} || echo "No direct queries found"
# Check if there are any tests that verify label filtering behavior
fd -e py test_.*issue.*filter
Length of output: 1800
Line range hint 188-204: Verify cascading delete behavior
The implementation of deleted_at filters for both labels and assignees raises a question about the cascading behavior when related entities are deleted.
Let's check the model definitions to ensure proper handling:
fix:
Issue Link: WEB-2776
Summary by CodeRabbit
New Features
Bug Fixes