fix: rendered the analytics for labels #5906
Conversation
WalkthroughThe changes in this pull request involve modifications to the filtering logic within the 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: 3
🧹 Outside diff range and nitpick comments (1)
apiserver/plane/bgtasks/analytic_plot_export.py (1)
Line range hint
359-363: Fix incorrect collection usage in module name mapping.There's a bug in the module name mapping logic. The code is incorrectly using
label_detailsinstead ofmodule_detailswhen mapping module names in segmented rows.Apply this fix:
module = next( ( mod - for mod in label_details + for mod in module_details if str(mod[MODULE_ID]) == str(segm) ), None, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- apiserver/plane/app/views/analytic/base.py (1 hunks)
- apiserver/plane/bgtasks/analytic_plot_export.py (1 hunks)
- apiserver/plane/settings/common.py (1 hunks)
🔇 Additional comments (2)
apiserver/plane/settings/common.py (1)
442-442: LGTM with security considerations.The addition of "text/csv" MIME type is appropriate for analytics export functionality. However, ensure that proper input validation and sanitization are in place to prevent CSV injection attacks when processing uploaded CSV files.
Let's verify if CSV validation is implemented:
✅ Verification successful
Let me try a different search strategy to verify CSV handling in the codebase.
This will help us understand:
- How CSV files are handled in general
- If there's any generic file validation that would apply to CSV uploads
Let me try a more focused search to find CSV validation logic.
This will help us understand:
- Any validation logic in Python backend code
- How MIME types are validated/restricted
Based on the search results, I can now provide a definitive response about the CSV MIME type validation:
CSV file type is properly validated through MIME type checks
The codebase has a centralized MIME type validation system in
settings/common.pywheretext/csvis now part of theATTACHMENT_MIME_TYPESlist. This list is actively enforced in the attachment upload flow through the validation check:if not type or type not in settings.ATTACHMENT_MIME_TYPES: return Response({"error": "Invalid file type."})This ensures that only files with approved MIME types, including CSV files, can be uploaded. The validation happens at the API level before any file processing occurs.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for CSV validation or sanitization logic rg -i "csv.*validation|sanitize.*csv|clean.*csv"Length of output: 50
Script:
#!/bin/bash # Search for CSV file handling patterns rg -i "csv" -A 3 -B 3 # Also check for file upload validation patterns rg -i "file.*validation|validate.*file|clean.*file" -A 3 -B 3Length of output: 198003
Script:
#!/bin/bash # Search for file validation patterns in Python files rg -t py "validate|sanitize|clean" -A 3 -B 3 # Also check for MIME type validation patterns rg -t py "ATTACHMENT_MIME_TYPES|ALLOWED_MIME_TYPES" -A 3 -B 3Length of output: 107285
apiserver/plane/bgtasks/analytic_plot_export.py (1)
135-136: LGTM! Improved filter readability.The separation of filter conditions into distinct clauses is a more idiomatic Django approach and improves code readability while maintaining the same functionality. The filters correctly handle:
- Ensuring only labeled issues are included (
labels__id__isnull=False)- Excluding deleted label associations (
label_issue__deleted_at__isnull=True)
| **filters, | ||
| labels__id__isnull=False | ||
| & Q(label_issue__deleted_at__isnull=True), | ||
| labels__id__isnull=False, | ||
| label_issue__deleted_at__isnull=True, | ||
| ) | ||
| .distinct("labels__id") | ||
| .order_by("labels__id") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring repeated filtering patterns.
The filtering pattern for handling soft deletes and null checks is repeated across different entities (labels, cycles, modules). Consider extracting this into a reusable query manager or utility function to maintain consistency and reduce duplication.
Example implementation:
def get_entity_details(queryset, entity_type):
"""
Generic function to fetch entity details with proper null and soft delete handling
:param queryset: Base queryset to filter
:param entity_type: String indicating the entity type (labels, cycles, modules)
:return: Filtered queryset with appropriate values
"""
entity_configs = {
'labels': {
'id_field': 'labels__id',
'deleted_at_field': 'label_issue__deleted_at',
'values': ['labels__id', 'labels__color', 'labels__name']
},
'cycles': {
'id_field': 'issue_cycle__cycle_id',
'deleted_at_field': 'issue_cycle__deleted_at',
'values': ['issue_cycle__cycle_id', 'issue_cycle__cycle__name']
},
# Add other entities...
}
config = entity_configs[entity_type]
return (
queryset.filter(**{
f"{config['id_field']}__isnull": False,
f"{config['deleted_at_field']}__isnull": True
})
.distinct(config['id_field'])
.order_by(config['id_field'])
.values(*config['values'])
)| labels__id__isnull=False, | ||
| label_issue__deleted_at__isnull=True, |
There was a problem hiding this comment.
💡 Codebase verification
Based on the search results, I can now generate the final response as I have sufficient information to verify the concern.
The change in analytics endpoint is inconsistent with label filtering patterns across the codebase
The search results reveal that most of the codebase uses a consistent pattern of combining label filters with bitwise AND (&), while the analytics endpoint now uses comma-separated conditions. This inconsistency could lead to:
- Maintenance challenges due to different filtering patterns for the same logic
- Potential behavioral differences in edge cases
Key findings:
- Files like
utils/grouper.py,views/issue/base.py, and others consistently use:~Q(labels__id__isnull=True) & Q(label_issue__deleted_at__isnull=True) - Only the analytics endpoint (
views/analytic/base.py) and the analytics export task use comma-separated conditions - The functionality remains the same, but the implementation pattern differs
Note: While both approaches work in Django, maintaining consistency across the codebase is important for long-term maintainability. Consider aligning the analytics endpoint with the rest of the codebase by using the bitwise AND pattern: ~Q(labels__id__isnull=True) & Q(label_issue__deleted_at__isnull=True).
🔗 Analysis chain
LGTM! The filter conditions are now properly separated.
The change from bitwise AND to comma-separated conditions follows Django's best practices for applying multiple filters. This should correctly filter out null labels and deleted label associations.
Let's verify the fix works as expected:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the analytics endpoint correctly handles label filtering
# by checking for any remaining issues with null labels or deleted associations
# Test 1: Check for any remaining issues with null labels in analytics
rg -A 5 "labels__id__isnull.*True" .
# Test 2: Check for consistency in label filtering across the codebase
rg -A 5 "label_issue__deleted_at__isnull.*True" .
# Test 3: Look for any similar patterns that might need the same fix
ast-grep --pattern 'Issue.objects.filter($$$).distinct("labels__id")'
Length of output: 21611
chore:
Summary by CodeRabbit
New Features
"text/csv"in the allowed attachment types.Bug Fixes
Documentation