Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: rendered the analytics for labels #5906

Merged
merged 2 commits into from
Oct 24, 2024
Merged

fix: rendered the analytics for labels #5906

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2500687
fix: rendered the analytics for labels
NarayanBavisetti Oct 24, 2024
07b6e9f
fix: analytics exports
NarayanBavisetti Oct 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions apiserver/plane/app/views/analytic/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,8 +112,8 @@ def get(self, request, slug):
Issue.objects.filter(
workspace__slug=slug,
**filters,
labels__id__isnull=False
& Q(label_issue__deleted_at__isnull=True),
labels__id__isnull=False,
label_issue__deleted_at__isnull=True,
Comment on lines +115 to +116
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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

)
.distinct("labels__id")
.order_by("labels__id")
Comment on lines 114 to 119
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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'])
    )

Expand Down
3 changes: 2 additions & 1 deletion apiserver/plane/bgtasks/analytic_plot_export.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,8 @@ def get_label_details(slug, filters):
Issue.objects.filter(
workspace__slug=slug,
**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")
Expand Down
1 change: 1 addition & 0 deletions apiserver/plane/settings/common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -439,5 +439,6 @@
"text/javascript",
"application/json",
"text/xml",
"text/csv",
pablohashescobar marked this conversation as resolved.
Show resolved Hide resolved
"application/xml",
]
Loading