[WEB-2691] chore: filtered the deleted labels and modules #5860
Conversation
WalkthroughThe pull request introduces significant modifications to the filtering logic across various views related to issues, including dashboards, inboxes, and issue relations. It enhances the conditions for filtering Changes
Possibly related PRs
Suggested labels
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: 6
🧹 Outside diff range and nitpick comments (9)
apiserver/plane/utils/grouper.py (1)
29-32: Approved: Enhanced filtering for labels and modulesThe changes effectively filter out deleted labels and modules from the issue queryset, aligning with the PR objectives. This improvement ensures that only active entities are considered in the grouping process.
A minor suggestion for consistency:
Consider reordering the conditions for
module_idsto match the order used forlabel_ids. This would improve readability:"module_ids": ( "issue_module__module_id", ( ~Q(issue_module__module_id__isnull=True) & Q(issue_module__module__deleted_at__isnull=True) & Q(issue_module__module__archived_at__isnull=True) ), ),This change is optional and doesn't affect functionality.
Also applies to: 33-39
apiserver/plane/app/views/issue/relation.py (1)
115-118: Approve changes with a minor suggestion for consistencyThe modifications to
label_idsandassignee_idsannotations effectively filter out deleted labels and inactive assignees, improving data integrity and aligning with the PR objectives. These changes ensure that only relevant and active data is returned in the API response.For consistency, consider applying a similar filter to
module_idsif modules can also be deleted or deactivated in your system.If applicable, you might want to add a similar filter for
module_ids:module_ids=Coalesce( ArrayAgg( "issue_module__module__id", distinct=True, filter=( ~Q(issue_module__module__id__isnull=True) & Q(issue_module__module__deleted_at__isnull=True) ), ), Value([], output_field=ArrayField(UUIDField())), ),This would ensure consistency across all related entities (labels, assignees, and modules).
Also applies to: 123-126
apiserver/plane/app/views/workspace/draft.py (1)
63-66: Approved: Improved filtering for deleted labels and modulesThe changes effectively filter out deleted labels and modules from the queryset, aligning with the PR objective. This enhancement improves data integrity by ensuring only active entities are considered.
For consistency, consider applying a similar filter for
assignee_idsto exclude deleted or inactive assignees:assignee_ids=Coalesce( ArrayAgg( "assignees__id", distinct=True, filter=~Q(assignees__id__isnull=True) & Q(assignees__member_project__is_active=True) & Q(assignees__deleted_at__isnull=True), # Add this line ), Value([], output_field=ArrayField(UUIDField())), ),This change would ensure consistency across all entity types (labels, modules, and assignees) in filtering out deleted or inactive items.
Also applies to: 84-85
apiserver/plane/app/views/dashboard/base.py (1)
390-393: Consistent improvement in filtering logic for created issues.The changes to the filtering logic for
label_idsandmodule_idsin thedashboard_created_issuesfunction are consistent with the improvements made in thedashboard_assigned_issuesfunction. This consistency is good for maintaining code quality and behavior across different parts of the dashboard.Consider refactoring the common filtering logic for
label_ids,assignee_ids, andmodule_idsinto a separate function or method. This would reduce code duplication and make future maintenance easier. Here's a suggested approach:def get_filtered_annotations(): return { 'label_ids': Coalesce( ArrayAgg( 'labels__id', distinct=True, filter=( ~Q(labels__id__isnull=True) & Q(labels__deleted_at__isnull=True) ), ), Value([], output_field=ArrayField(UUIDField())), ), 'assignee_ids': Coalesce( ArrayAgg( 'assignees__id', distinct=True, filter=~Q(assignees__id__isnull=True) & Q(assignees__member_project__is_active=True), ), Value([], output_field=ArrayField(UUIDField())), ), 'module_ids': Coalesce( ArrayAgg( 'issue_module__module_id', distinct=True, filter=~Q(issue_module__module_id__isnull=True) & Q(issue_module__module__archived_at__isnull=True) & Q(issue_module__module__deleted_at__isnull=True), ), Value([], output_field=ArrayField(UUIDField())), ), } # Then in both dashboard_assigned_issues and dashboard_created_issues: issues = issues.annotate(**get_filtered_annotations())This refactoring would make the code more DRY (Don't Repeat Yourself) and easier to maintain in the future.
Also applies to: 410-412
apiserver/plane/app/views/view/base.py (1)
Line range hint
330-335: Replace magic numbers with role constants for clarityThe code uses hardcoded role numbers like
role=5androle=20. For better readability and maintainability, consider replacing these with the corresponding constants from theROLEenumeration (e.g.,ROLE.GUEST,ROLE.ADMIN).Apply this change to use role constants:
# Example in the `list` method - role=5, + role=ROLE.GUEST, # Example in the `destroy` method - role=20, + role=ROLE.ADMIN,Also applies to: 360-365
apiserver/plane/app/views/issue/base.py (4)
475-475: Simplify the condition by usingisnull=False.For clarity and readability, replace
~Q(labels__id__isnull=True)withQ(labels__id__isnull=False).Apply this diff:
filter=( - ~Q(labels__id__isnull=True) + Q(labels__id__isnull=False) & Q(labels__deleted_at__isnull=True) & Q(labels__archived_at__isnull=True) ),
576-576: Simplify the condition by usingisnull=False.Replace
~Q(labels__id__isnull=True)withQ(labels__id__isnull=False)for better readability.Apply this diff:
filter=( - ~Q(labels__id__isnull=True) + Q(labels__id__isnull=False) & Q(labels__deleted_at__isnull=True) & Q(labels__archived_at__isnull=True) ),
595-597: Add parentheses to clarify filter conditions.To improve readability and ensure proper operator precedence, add parentheses around the combined filter conditions.
Apply this diff:
-filter=~Q(issue_module__module_id__isnull=True) +filter=( + ~Q(issue_module__module_id__isnull=True) & Q(issue_module__module__archived_at__isnull=True) & Q(issue_module__module__deleted_at__isnull=True) +),
868-868: Simplify the condition by usingisnull=False.For clarity, replace
~Q(labels__id__isnull=True)withQ(labels__id__isnull=False).Apply this diff:
filter=( - ~Q(labels__id__isnull=True) + Q(labels__id__isnull=False) & Q(labels__deleted_at__isnull=True) & Q(labels__archived_at__isnull=True) ),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- apiserver/plane/app/views/dashboard/base.py (4 hunks)
- apiserver/plane/app/views/inbox/base.py (4 hunks)
- apiserver/plane/app/views/issue/base.py (6 hunks)
- apiserver/plane/app/views/issue/relation.py (1 hunks)
- apiserver/plane/app/views/issue/sub_issue.py (2 hunks)
- apiserver/plane/app/views/view/base.py (2 hunks)
- apiserver/plane/app/views/workspace/draft.py (2 hunks)
- apiserver/plane/space/views/issue.py (2 hunks)
- apiserver/plane/utils/grouper.py (1 hunks)
🧰 Additional context used
🔇 Additional comments (13)
apiserver/plane/utils/grouper.py (1)
Line range hint
1-214: Summary: Effective implementation of label and module filteringThe changes in this file successfully implement the PR objective of filtering out deleted labels and modules from the issue queryset. The modifications are localized to the
issue_queryset_grouperfunction and do not introduce any breaking changes or unnecessary complexities.Key improvements:
- Labels are now filtered to exclude deleted ones.
- Modules are filtered to exclude both deleted and archived ones.
These changes enhance data integrity in issue grouping and should improve the overall user experience by ensuring only relevant, active entities are considered.
apiserver/plane/app/views/issue/sub_issue.py (1)
80-83: Excellent improvements to filtering logic!The changes to the
label_idsandmodule_idsannotations are well-implemented and align perfectly with the PR objectives. By adding conditions to filter out deleted labels and archived/deleted modules, you've enhanced data integrity and improved the clarity of the issues list.Specifically:
- For
label_ids, the addition ofQ(labels__deleted_at__isnull=True)ensures only active labels are included.- For
module_ids, the new conditionsQ(issue_module__module__archived_at__isnull=True)andQ(issue_module__module__deleted_at__isnull=True)filter out both archived and deleted modules.These changes will effectively streamline the issues list by removing references to deleted labels and modules, improving the overall user experience.
Also applies to: 100-102
apiserver/plane/app/views/dashboard/base.py (2)
Line range hint
1-824: Overall improvement in dashboard issue filtering.The changes made to this file focus on enhancing the filtering logic for labels and modules in both assigned and created issues. These improvements ensure that only active and non-deleted entities are included in the dashboard queries, which is a positive change for data integrity and user experience.
While the changes are good, consider the following suggestions for further improvement:
- Implement the refactoring suggestion to reduce code duplication between
dashboard_assigned_issuesanddashboard_created_issuesfunctions.- Verify that similar filtering logic is applied consistently across the codebase where labels and modules are queried.
- Consider adding comments to explain the purpose of the new filtering conditions, especially if they address specific edge cases or requirements.
These changes align well with the PR objective of filtering out deleted labels and modules, improving the overall quality of the dashboard data.
221-224: Improved filtering logic for assigned issues.The changes to the filtering logic for
label_idsandmodule_idsin thedashboard_assigned_issuesfunction are good improvements. They ensure that only active and non-deleted labels and modules are included in the query results, which enhances data integrity and user experience.To ensure consistency, let's verify if similar filtering logic is applied in other parts of the codebase where labels and modules are queried:
Also applies to: 241-243
apiserver/plane/space/views/issue.py (2)
704-707: Improved filtering for labels and modulesThe changes to the
label_idsandmodule_idsannotations enhance data integrity by excluding deleted or archived entities. This aligns well with the PR objective of filtering out deleted labels and modules.For
label_ids:filter=( ~Q(labels__id__isnull=True) & Q(labels__deleted_at__isnull=True) ),For
module_ids:filter=~Q(issue_module__module_id__isnull=True) & Q(issue_module__module__archived_at__isnull=True) & Q(issue_module__module__deleted_at__isnull=True),These modifications ensure that only active and non-deleted labels and modules are included in the query results, improving the overall data quality and consistency.
Also applies to: 724-726
Line range hint
704-726: Summary: Improved filtering for public issue retrievalThe changes in this file enhance the
IssueRetrievePublicEndpointclass by improving the filtering of labels, assignees, and modules. These modifications ensure that only active and non-deleted entities are included in the API response, which aligns with the PR objective of filtering out deleted labels and modules.The implementation is consistent and well-structured, improving data integrity and the overall quality of the public API. No further issues or improvements were identified in this review.
apiserver/plane/app/views/view/base.py (2)
258-259: LGTM: Correctly filtering out archived and deleted modulesThe code properly filters out modules that are archived or deleted when aggregating
module_ids. This ensures that only active modules are considered.
Line range hint
280-295: Verify the correctness of role-based filtering logicIn the issue filtering logic within the
listmethod, the condition usesrole__gt=5to filter issues for roles higher than guest. Please verify that this logic aligns with the intended role hierarchy. If higher role numbers represent lower privileges (e.g., guest users), the condition might need adjustment torole__lt=5or use explicit role constants for clarity.To check the role hierarchy, you can run the following script:
apiserver/plane/app/views/inbox/base.py (3)
144-147: Correctly exclude deleted labels from 'label_ids' aggregationThe added filter
Q(labels__deleted_at__isnull=True)ensures that only labels that are not marked as deleted are included in thelabel_idsaggregation. This improves data integrity by preventing deleted labels from being associated with issues.
165-166: Include only active modules in 'module_ids' aggregationBy adding
Q(issue_module__module__archived_at__isnull=True)andQ(issue_module__module__deleted_at__isnull=True)to the filter, themodule_idsaggregation now includes only modules that are neither archived nor deleted. This ensures that issues are associated with valid modules.
193-194: Exclude deleted labels from 'label_ids' in inbox issuesIncluding
Q(issue__labels__deleted_at__isnull=True)in the filter ensures that only non-deleted labels are aggregated inlabel_idsfor inbox issues. This maintains accurate labeling and prevents inconsistencies in the issue data.apiserver/plane/app/views/issue/base.py (2)
495-496: No issues found with module filters.The filter for
module_idscorrectly checks botharchived_at__isnull=Trueanddeleted_at__isnull=True, ensuring only active modules are included.
888-889: No issues found with module filters.The filtering for
module_idsappropriately includes botharchived_at__isnull=Trueanddeleted_at__isnull=True, ensuring only active modules are included.
| filter=( | ||
| ~Q(labels__id__isnull=True) | ||
| & Q(labels__deleted_at__isnull=True) | ||
| ), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Suggestion: Also filter out archived labels in label_ids aggregation
Currently, the label_ids aggregation filters out deleted labels but not archived ones. For consistency with modules and to ensure that only active labels are included, consider adding a filter for archived labels.
Apply this change to include the archived filter:
filter=(
~Q(labels__id__isnull=True)
& Q(labels__deleted_at__isnull=True)
+ & Q(labels__archived_at__isnull=True)
),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| filter=( | |
| ~Q(labels__id__isnull=True) | |
| & Q(labels__deleted_at__isnull=True) | |
| ), | |
| filter=( | |
| ~Q(labels__id__isnull=True) | |
| & Q(labels__deleted_at__isnull=True) | |
| & Q(labels__archived_at__isnull=True) | |
| ), |
| filter=( | ||
| ~Q(labels__id__isnull=True) | ||
| & Q(labels__deleted_at__isnull=True) | ||
| ), |
There was a problem hiding this comment.
Inconsistent field references in 'label_ids' aggregation filter
There is a potential issue in the label_ids aggregation filter: the fields in the filter should match the annotated fields. Currently, the filter uses labels__id and labels__deleted_at, but the aggregation is over issue__labels__id. This mismatch may lead to incorrect filtering.
Apply this diff to correct the filter field references:
- filter=(
- ~Q(labels__id__isnull=True)
- & Q(labels__deleted_at__isnull=True)
+ filter=(
+ ~Q(issue__labels__id__isnull=True)
+ & Q(issue__labels__deleted_at__isnull=True)
),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| filter=( | |
| ~Q(labels__id__isnull=True) | |
| & Q(labels__deleted_at__isnull=True) | |
| ), | |
| filter=( | |
| ~Q(issue__labels__id__isnull=True) | |
| & Q(issue__labels__deleted_at__isnull=True) | |
| ), |
| filter=~Q(assignees__id__isnull=True) | ||
| & Q(assignees__member_project__is_active=True), |
There was a problem hiding this comment.
Ensure consistent field references in 'assignee_ids' aggregation filter
Similar to the previous comment, the filter for assignee_ids uses assignees__id and assignees__member_project__is_active, whereas the aggregation is over issue__assignees__id. To ensure correct filtering, the field references should include the issue__ prefix.
Apply this diff to update the filter field references:
- filter=~Q(assignees__id__isnull=True)
- & Q(assignees__member_project__is_active=True),
+ filter=~Q(issue__assignees__id__isnull=True)
+ & Q(issue__assignees__member_project__is_active=True),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| filter=~Q(assignees__id__isnull=True) | |
| & Q(assignees__member_project__is_active=True), | |
| filter=~Q(issue__assignees__id__isnull=True) | |
| & Q(issue__assignees__member_project__is_active=True), |
| filter=( | ||
| ~Q(labels__id__isnull=True) | ||
| & Q(labels__deleted_at__isnull=True) | ||
| ), |
There was a problem hiding this comment.
Archived labels are not excluded in the filter.
In the aggregation of label_ids, the filter does not exclude archived labels. Adding labels__archived_at__isnull=True ensures that only active labels are included.
Apply this diff:
filter=(
~Q(labels__id__isnull=True)
& Q(labels__deleted_at__isnull=True)
+ & Q(labels__archived_at__isnull=True)
),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| filter=( | |
| ~Q(labels__id__isnull=True) | |
| & Q(labels__deleted_at__isnull=True) | |
| ), | |
| filter=( | |
| ~Q(labels__id__isnull=True) | |
| & Q(labels__deleted_at__isnull=True) | |
| & Q(labels__archived_at__isnull=True) | |
| ), |
| filter=( | ||
| ~Q(labels__id__isnull=True) | ||
| & Q(labels__deleted_at__isnull=True) | ||
| ), |
There was a problem hiding this comment.
Archived labels are not excluded in the filter.
In the aggregation of label_ids, the filter checks for labels where deleted_at is null but does not exclude archived labels. This means archived labels may unintentionally be included in the results. To ensure only active labels are aggregated, add labels__archived_at__isnull=True to the filter condition.
Apply this diff to update the filter condition:
filter=(
~Q(labels__id__isnull=True)
& Q(labels__deleted_at__isnull=True)
+ & Q(labels__archived_at__isnull=True)
),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| filter=( | |
| ~Q(labels__id__isnull=True) | |
| & Q(labels__deleted_at__isnull=True) | |
| ), | |
| filter=( | |
| ~Q(labels__id__isnull=True) | |
| & Q(labels__deleted_at__isnull=True) | |
| & Q(labels__archived_at__isnull=True) | |
| ), |
| filter=( | ||
| ~Q(labels__id__isnull=True) | ||
| & Q(labels__deleted_at__isnull=True) | ||
| ), |
There was a problem hiding this comment.
Archived labels are not excluded in the filter.
Similar to the previous occurrence, the filter for label_ids does not exclude archived labels. To prevent archived labels from being aggregated, add labels__archived_at__isnull=True to the filter condition.
Apply this diff:
filter=(
~Q(labels__id__isnull=True)
& Q(labels__deleted_at__isnull=True)
+ & Q(labels__archived_at__isnull=True)
),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| filter=( | |
| ~Q(labels__id__isnull=True) | |
| & Q(labels__deleted_at__isnull=True) | |
| ), | |
| filter=( | |
| ~Q(labels__id__isnull=True) | |
| & Q(labels__deleted_at__isnull=True) | |
| & Q(labels__archived_at__isnull=True) | |
| ), |
pushya22
changed the title
chore:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor