chore: restrict email notifications for removed users #6100
Conversation
WalkthroughThe pull request introduces enhancements to the notification filtering logic across several components. In the Changes
Possibly related PRs
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (2)
apiserver/plane/app/views/notification/base.py (1)
65-68: Consider adding an index to optimize the new filtering conditions.Since we're now filtering on
project__project_projectmember__memberandproject__project_projectmember__is_active, consider adding a composite index to optimize these queries, especially if the notifications table is expected to grow large.Example Django migration to add the index:
class Migration(migrations.Migration): dependencies = [ ('app', 'previous_migration'), ] operations = [ migrations.AddIndex( model_name='ProjectMember', index=models.Index( fields=['member', 'is_active'], name='project_member_active_idx' ), ), ]apiserver/plane/bgtasks/notification_task.py (1)
248-250: Reuse fetched project member IDs to avoid redundant queriesSince
project_member_idsare fetched at the beginning, you can reuse this list in other parts of your code to avoid redundant queries. For example, when filtering mentions or subscribers, utilizeproject_member_idsfor consistency and efficiency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
apiserver/plane/app/views/notification/base.py(1 hunks)apiserver/plane/app/views/workspace/base.py(1 hunks)apiserver/plane/bgtasks/notification_task.py(5 hunks)
🔇 Additional comments (1)
apiserver/plane/app/views/workspace/base.py (1)
356-356: LGTM! Verify query performance impact.
The added filter correctly ensures that only activities from projects where the user is an active member are included, which aligns with the PR objective of restricting notifications for removed users.
Let's verify the query performance impact:
✅ Verification successful
Query performance impact is minimal - filter is widely used with proper indexes
The added filter project__project_projectmember__is_active=True is already extensively used across the codebase (50+ occurrences) in similar query patterns, particularly in views that handle user-specific data. The consistent usage pattern shows this is a well-established filtering approach in the codebase.
Key observations:
- The filter is commonly paired with
project__archived_at__isnull=Trueand other performance-optimized conditions - Most queries using this filter also use
select_related()for optimizing related field access - The query in question is already limited to 10000 records and uses
select_related()for performance
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the added filter significantly impacts the query performance
# by analyzing the query plan and checking for any related indexes.
# Check if there's an index on project_projectmember.is_active
ast-grep --pattern 'class Migration:
$$$
operations = [
$$$
migrations.AddIndex(
model_name="projectmember",
index=models.Index(
fields=["is_active"],
$$$
),
$$$
),
$$$
]'
# Look for similar patterns of filtering by is_active in other views
rg "project__project_projectmember__is_active\s*=" -A 2
Length of output: 15737
| .filter( | ||
| project__project_projectmember__member=request.user, | ||
| project__project_projectmember__is_active=True, | ||
| ) |
There was a problem hiding this comment.
💡 Codebase verification
The active member check is not consistently applied across all notification queries
The verification revealed that while the main list endpoint properly filters for active project members, other notification endpoints like UnreadNotificationEndpoint and MarkAllReadNotificationViewSet don't include the same active member check:
UnreadNotificationEndpoint.get()(lines 245-263): Missing active member checkMarkAllReadNotificationViewSet.create()(lines 284-286): Missing active member check
These endpoints should also include the same filtering to maintain consistency with the PR's objective of restricting notifications for removed users.
🔗 Analysis chain
LGTM! The changes effectively restrict notifications to active project members.
The added filters ensure that notifications are only shown to active project members, which aligns with the PR objective of restricting notifications for removed users.
Let's verify that the filters work as expected:
Let me gather more information to verify if there are any potential notification queries that might bypass the active member check.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that notifications are properly filtered for inactive project members
# Check if there are any notifications being sent to inactive project members
ast-grep --pattern 'Notification.objects.filter($$$).exclude(project__project_projectmember__is_active=True)' \
--pattern 'Notification.objects.filter($$$).exclude(project__project_projectmember__member=True)'
# Check for any other notification queries that might bypass these filters
rg -l "Notification.objects" | xargs ast-grep --pattern 'Notification.objects.filter($$$)'
Length of output: 2374
Script:
#!/bin/bash
# Let's check the content of the notification base file to understand the context of these queries
cat apiserver/plane/app/views/notification/base.py
Length of output: 13739
| new_mentions = [ | ||
| str(mention) for mention in new_mentions if mention in new_mentions | ||
| ] |
There was a problem hiding this comment.
Fix redundant and incorrect filtering of new mentions
The condition if mention in new_mentions in the list comprehension is redundant because each mention is inherently from new_mentions. This does not filter the mentions as intended. If the goal is to filter new_mentions to include only active project members, you should update the code to:
new_mentions = [
str(mention) for mention in new_mentions if mention in project_member_ids
]
chore:
Summary by CodeRabbit
New Features
Bug Fixes