[WEB-2693] chore: removed the deleted cycles from the issue list#5868
[WEB-2693] chore: removed the deleted cycles from the issue list#5868
Conversation
WalkthroughThe pull request introduces modifications across multiple classes related to issue and cycle management within the API. Key changes involve the implementation of conditional annotations for the 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (10)
apiserver/plane/app/views/workspace/draft.py (1)
Line range hint
60-100: LGTM: Improved queryset annotations.The changes in the
get_querysetmethod enhance data integrity and readability:
The new
cycle_idannotation ensures that only non-deleted cycles are considered, which is a good practice for data consistency.The
module_idsannotation has been reformatted for better readability, and additional conditions have been added to filter out archived and deleted modules.These changes improve the overall quality of the queryset and the data it returns.
Consider adding a comment explaining the purpose of the
cycle_idannotation for better code documentation:# Annotate cycle_id, ensuring only non-deleted cycles are considered .annotate( cycle_id=Case( When( issue_cycle__cycle__deleted_at__isnull=True, then=F("issue_cycle__cycle_id"), ), default=None, ) )apiserver/plane/app/views/module/issue.py (1)
9-10: LGTM! Consider a minor readability improvement.The changes to the
get_querysetmethod effectively implement the removal of deleted cycles from the issue list. The use ofCaseandWhenfor conditional annotation is a robust approach.To slightly improve readability, consider extracting the
Casestatement into a separate variable:cycle_id_annotation = Case( When( issue_cycle__cycle__deleted_at__isnull=True, then=F("issue_cycle__cycle_id"), ), default=None, ) # Then in the annotate call: .annotate(cycle_id=cycle_id_annotation)This separation can make the query structure clearer, especially if you need to reuse or modify the annotation in the future.
Also applies to: 70-78
apiserver/plane/app/views/cycle/issue.py (1)
105-113: LGTM: Annotation logic forcycle_idimproved.The new annotation for
cycle_idusingCaseandWheneffectively filters out deleted cycles, aligning with the PR objective. This change ensures that only active cycles are considered when listing issues.For improved readability, consider adding a brief comment explaining the purpose of this annotation:
# Annotate cycle_id, setting it to None for deleted cycles cycle_id=Case( When( issue_cycle__cycle__deleted_at__isnull=True, then=F("issue_cycle__cycle_id"), ), default=None, )This minor addition would make the intention clearer for future maintainers.
apiserver/plane/app/views/issue/archive.py (1)
67-75: Approve the changes with a minor suggestion.The modification to the
cycle_idannotation is a good improvement. It ensures that only non-deleted cycles are considered when annotatingcycle_id, which enhances data consistency in the issue list.Consider adding a comment explaining the purpose of this conditional annotation, as it might not be immediately clear to other developers why this change was necessary. For example:
# Annotate cycle_id only for non-deleted cycles to maintain consistency in the issue list cycle_id=Case( When( issue_cycle__cycle__deleted_at__isnull=True, then=F("issue_cycle__cycle_id"), ), default=None, )This comment will help future maintainers understand the rationale behind this specific implementation.
apiserver/plane/app/views/workspace/user.py (1)
123-131: Improved cycle_id annotation logicThe changes to the
cycle_idannotation are well-implemented and align with the PR objective. By using aCasestatement to check if the related cycle'sdeleted_atis null, the code now ensures that only valid (non-deleted) cycle IDs are returned. This improvement helps maintain data integrity and prevents issues associated with deleted cycles from appearing in the list.Consider adding a comment explaining the purpose of this conditional annotation to improve code readability and maintainability for future developers.
apiserver/plane/app/views/issue/base.py (1)
778-786: Approved: Consistent implementation ofcycle_idannotation.The conditional annotation for
cycle_idis correctly implemented here, consistent with the previous occurrence. This ensures uniform behavior across different parts of the code.Consider refactoring this repeated annotation into a reusable method or queryset annotation to reduce code duplication. This would improve maintainability and ensure consistency if changes are needed in the future.
For example, you could create a method like:
def annotate_cycle_id(queryset): return queryset.annotate( cycle_id=Case( When( issue_cycle__cycle__deleted_at__isnull=True, then=F("issue_cycle__cycle_id"), ), default=None, ) )Then use it in both places:
queryset = annotate_cycle_id(queryset)This would centralize the logic and make it easier to maintain.
apiserver/plane/app/views/inbox/base.py (2)
Line range hint
225-233: Handle potentialNoneTypeerror when retrievinginbox_idThe
inbox_idobtained with.first()may beNoneif no matchingInboxis found. PassingNonetoInboxIssue.objects.get()could raise an error.Consider adding a check to ensure
inbox_idis notNonebefore proceeding. If it isNone, return an appropriate error response.Apply this diff to handle the potential error:
def partial_update(self, request, slug, project_id, pk): inbox_id = Inbox.objects.filter( workspace__slug=slug, project_id=project_id ).first() + if inbox_id is None: + return Response( + {"error": "Inbox not found"}, + status=status.HTTP_400_BAD_REQUEST, + ) inbox_issue = InboxIssue.objects.get( issue_id=pk, workspace__slug=slug,
Line range hint
228-233: Ensure correct assignment ofinbox_idwhen fetchingInboxIssueIn the
InboxIssue.objects.get()call, ensure thatinbox_idis correctly referenced usinginbox_id.idinstead of the entire object.Update the
inbox_idparameter to use the ID attribute:inbox_issue = InboxIssue.objects.get( issue_id=pk, workspace__slug=slug, project_id=project_id, - inbox_id=inbox_id, + inbox_id=inbox_id.id, )apiserver/plane/app/views/view/base.py (2)
Line range hint
138-144: Duplicate Annotation ofsub_issues_countThe
sub_issues_countannotation is defined twice in theget_querysetmethod—once at lines 138-144 and again at lines 225-231. This duplication is unnecessary and may affect query efficiency.Remove the duplicate annotation of
sub_issues_countto streamline the queryset.Also applies to: 225-231
Line range hint
153-165: Simplify Permission Checks in thelistMethodThe permission checks within the
listmethod ofWorkspaceViewViewSetinvolve nestedifstatements, which can be simplified for better readability and maintainability.Consider refactoring the permission logic to reduce nesting and improve clarity. For example, you can combine conditions or use early returns to make the code more concise.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
- apiserver/plane/api/views/issue.py (1 hunks)
- apiserver/plane/app/views/cycle/issue.py (2 hunks)
- apiserver/plane/app/views/inbox/base.py (2 hunks)
- apiserver/plane/app/views/issue/archive.py (2 hunks)
- apiserver/plane/app/views/issue/base.py (4 hunks)
- apiserver/plane/app/views/issue/relation.py (2 hunks)
- apiserver/plane/app/views/issue/sub_issue.py (2 hunks)
- apiserver/plane/app/views/module/issue.py (2 hunks)
- apiserver/plane/app/views/view/base.py (3 hunks)
- apiserver/plane/app/views/workspace/draft.py (3 hunks)
- apiserver/plane/app/views/workspace/user.py (1 hunks)
- apiserver/plane/space/views/issue.py (2 hunks)
- packages/editor/src/core/extensions/custom-image/components/image-block.tsx (1 hunks)
🧰 Additional context used
🔇 Additional comments (23)
apiserver/plane/app/views/issue/sub_issue.py (2)
13-14: LGTM: New imports for Case statement.The addition of
CaseandWhenimports fromdjango.db.modelsis appropriate for the newcycle_idannotation logic.
53-61: Excellent: Improved cycle_id annotation logic.The updated
cycle_idannotation using aCasestatement effectively addresses the PR objective of removing deleted cycles from the issue list. This change ensures that:
cycle_idis set toNoneif the associated cycle has been deleted.- The filtering of deleted cycles is handled efficiently at the database level.
This implementation maintains the existing functionality while improving data consistency.
apiserver/plane/app/views/issue/relation.py (2)
6-16: LGTM: Import statements updated correctly.The addition of
CaseandWhenimports fromdjango.db.modelsis appropriate for the new functionality implemented in thelistmethod.
96-104: LGTM: Cycle annotation updated to exclude deleted cycles.The new annotation for
cycle_ideffectively addresses the PR objective of removing deleted cycles from the issue list. The implementation correctly usesCaseandWhento set thecycle_idonly when the cycle is not deleted, defaulting toNoneotherwise. This change ensures that only valid cycles are considered in the issue list.packages/editor/src/core/extensions/custom-image/components/image-block.tsx (3)
59-59: Approve changes in image source handling and UI conditionals.The refactoring to use
remoteImageSrcconsistently throughout the component is a positive change. It simplifies the source of truth for the image source and updates the conditions for showing the image loader, utils, and resizer accordingly. This change improves the overall clarity and maintainability of the component.These changes look good and align well with the PR objectives of removing deleted cycles from the issue list. The consistent use of
remoteImageSrcshould make the code more robust and easier to maintain.Also applies to: 201-213
Line range hint
1-324: Summary of changes and their impact.The modifications in this file are focused on simplifying the handling of the image source in the
CustomImageBlockcomponent. The main changes include:
- Removal of the
srcprop from the component's props.- Consistent use of
remoteImageSrcderived fromnode.attrs.src.- Updated conditions for rendering image-related UI elements.
These changes align with the PR objectives and the AI-generated summary. They improve the component's consistency and maintainability without introducing new issues or altering its core functionality.
The changes in this file are well-implemented and contribute positively to the codebase. They simplify the component's logic and improve its overall structure.
59-59: Verify the removal ofsrcprop and its impact.The
srcprop has been commented out in the destructuring. This change aligns with the removal of thesrcproperty fromCustomImageBlockPropsas mentioned in the AI-generated summary. Now,remoteImageSrcis derived solely fromnode.attrs.src.Please ensure that:
- This change is intentional and aligns with the PR objectives.
- All places where
CustomImageBlockis used have been updated to remove thesrcprop.- The logic depending on
remoteImageSrcthroughout the component still works as expected.You can use the following script to check for any remaining usage of the
srcprop withCustomImageBlock:apiserver/plane/app/views/workspace/draft.py (2)
15-16: LGTM: New imports for conditional annotation.The addition of
CaseandWhenimports fromdjango.db.modelsis appropriate for the new conditional annotation in theget_querysetmethod.
Line range hint
1-100: Summary: Improved queryset handling for draft issues.The changes in this file focus on enhancing the
get_querysetmethod of theWorkspaceDraftIssueViewSetclass. The main improvements are:
- Addition of a conditional annotation for
cycle_idto ensure only non-deleted cycles are considered.- Refinement of the
module_idsannotation to exclude archived and deleted modules.These changes contribute to better data integrity and accuracy in the draft issue queryset. The overall structure and functionality of the class remain unchanged, maintaining consistency with the existing codebase.
apiserver/plane/app/views/module/issue.py (1)
Line range hint
1-378: Overall impact assessment: Changes are well-contained and aligned with PR objectives.The modifications to the
get_querysetmethod effectively implement the removal of deleted cycles from the issue list without affecting other parts of theModuleIssueViewSetclass. The changes are well-contained and don't require alterations to other methods or class structure.The new imports (
CaseandWhen) are correctly placed and used appropriately. This implementation aligns well with the PR objective of removing deleted cycles from the issue list, improving data consistency in the application.apiserver/plane/app/views/cycle/issue.py (2)
6-6: LGTM: Import statement updated correctly.The import statement has been appropriately updated to include
CaseandWhenfrom Django's ORM, which are used in the new annotation logic forcycle_id.
Line range hint
1-359: Summary: Effective implementation of cycle filtering.The changes in this file successfully implement the filtering of deleted cycles from the issue list. The new annotation logic for
cycle_idensures that only active cycles are considered when listing issues, which aligns perfectly with the PR objective.Key points:
- The import statement has been updated correctly to include the necessary Django ORM functions.
- The
listmethod now includes aCasestatement to handle deleted cycles appropriately.- The changes are focused and don't introduce any apparent side effects to other parts of the class.
These modifications should improve the accuracy of the issue list by excluding issues associated with deleted cycles.
apiserver/plane/app/views/issue/archive.py (1)
Line range hint
1-375: Summary of changes and potential impactThe main change in this file is the modification of the
cycle_idannotation in theget_querysetmethod of theIssueArchiveViewSetclass. This change improves data consistency by excluding deleted cycles from thecycle_idannotation.Key points:
- The change ensures that only non-deleted cycles are considered when annotating
cycle_id.- This modification enhances data consistency in the issue list.
- Other methods in this file are not directly affected by this change.
To ensure a smooth integration of this change:
- Review the results of the verification script provided earlier.
- Update any code that relies on the presence of
cycle_idfor deleted cycles.- Test thoroughly to confirm that this change doesn't introduce unexpected behavior in other parts of the application that interact with archived issues or cycles.
These steps will help maintain the integrity of the application while benefiting from the improved data consistency introduced by this change.
apiserver/plane/app/views/issue/base.py (3)
17-18: LGTM: New imports for conditional annotations.The addition of
WhenandCaseimports from django.db.models is appropriate for implementing the conditional annotations for thecycle_idfield.
88-96: Excellent: Conditional annotation forcycle_id.This change effectively implements the PR objective of removing deleted cycles from the issue list. The use of
CaseandWhenensures thatcycle_idis only set when the associated cycle is not deleted (deleted_atis null). This is a robust way to handle soft-deleted cycles in the issue list.
Line range hint
1-896: Summary: Effective implementation of cycle removal logic.The changes in this file successfully address the PR objective of removing deleted cycles from the issue list. The consistent implementation of the conditional annotation for
cycle_idensures that deleted cycles are not included in issue queries. This improves data integrity and user experience by preventing issues from being associated with non-existent cycles.Key points:
- Appropriate imports have been added for the new functionality.
- The conditional annotation is correctly implemented using
CaseandWhen.- The changes are consistent across different methods in the file.
While the implementation is solid, consider the refactoring suggestion to reduce code duplication and improve maintainability.
Overall, these changes represent a significant improvement in handling deleted cycles within the issue management system.
apiserver/plane/space/views/issue.py (2)
109-117: LGTM! This change effectively filters out deleted cycles.The introduction of the
Casestatement forcycle_idannotation is a good improvement. It ensures that only cycles that haven't been deleted (i.e.,deleted_atis null) are associated with the issues. This aligns well with the PR objective of removing deleted cycles from the issue list.
706-714: LGTM! Consistent implementation for single issue retrieval.This change mirrors the modification made in the
ProjectIssuesPublicEndpointclass. By applying the sameCasestatement forcycle_idannotation, it ensures consistency in how deleted cycles are handled across both bulk issue listing and single issue retrieval. This is a good practice that maintains code coherence and prevents potential discrepancies in behavior.apiserver/plane/app/views/inbox/base.py (2)
6-6: Import 'Case' and 'When' for conditional annotationsThe addition of
CaseandWhento the imports is appropriate, as they are used for conditional annotations in the queryset to handle conditional logic.
115-123: Conditional annotation ofcycle_idto exclude deleted cyclesThe
annotatemethod correctly usesCaseandWhento setcycle_idonly if the related cycle is not deleted (deleted_at__isnull=True). This ensures that issues associated with deleted cycles do not incorrectly retain acycle_id, aligning with the PR objective to remove deleted cycles from the issue list.apiserver/plane/api/views/issue.py (1)
205-213: Correctly excludes deleted cycles from the issue listThe updated annotation for
cycle_idensures that only cycles that are not deleted (deleted_at__isnull=True) are associated with issues. This change aligns with the PR objective of removing deleted cycles from the issue list.apiserver/plane/app/views/view/base.py (2)
12-13: Approved: Added necessary imports for conditional annotationsThe imports of
CaseandWhenare required for the conditional annotations used later in the code.
210-218: Ensure Consistent Handling of Deleted CyclesThe conditional annotation for
cycle_idusingCaseandWhenis added in bothget_querysetandlistmethods. While addressing deleted cycles is important, ensure that this logic is consistently applied and tested across all relevant querysets.Run the following script to check for other querysets where
cycle_idshould be conditionally annotated:Also applies to: 288-296
✅ Verification successful
Consistent Handling of Deleted Cycles Verified
The conditional annotation for
cycle_idusingCaseandWhenis consistently applied only within theget_querysetandlistmethods as intended. No additional querysets requiring this annotation were found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all occurrences of `cycle_id` annotation in the codebase. # Search for annotations of `cycle_id` using `Case` and `When`. rg --type python 'annotate\(\s*cycle_id=Case\(' -A 5Length of output: 85
Script:
#!/bin/bash # Description: Find all occurrences of `cycle_id` annotation in Python files. # Search for annotations of `cycle_id` using `Case` and `When` in .py files. rg --glob "*.py" 'annotate\(\s*cycle_id=Case\(' -A 5Length of output: 54
Script:
#!/bin/bash # Description: Find all occurrences of `cycle_id` annotation using `Case` and `When` in Python files with ast-grep. # Use ast-grep to search for `cycle_id=Case(` patterns in Python files. ast-grep --lang python --pattern '_.annotate(cycle_id=Case($_, $_))'Length of output: 70
| .annotate( | ||
| cycle_id=Case( | ||
| When( | ||
| issue_cycle__cycle__deleted_at__isnull=True, | ||
| then=F("issue_cycle__cycle_id"), | ||
| ), | ||
| default=None, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
💡 Codebase verification
Issues Found with cycle_id Handling.
The recent change allowing cycle_id to be None has introduced potential issues in both serializers and query filters. The following areas need attention to ensure proper handling of None values for cycle_id:
-
Serializers:
apiserver/plane/app/serializers/draft.py:cycle_id = serializers.PrimaryKeyRelatedField(read_only=True)
apiserver/plane/app/serializers/issue.py:cycle_id = serializers.PrimaryKeyRelatedField(read_only=True)
Action: Add
allow_null=Trueto thecycle_idfields to permitNonevalues. -
Query Filters:
apiserver/plane/bgtasks/issue_activities_task.py:cycle = Cycle.objects.filter(pk=cycle_id).first()
apiserver/plane/api/views/cycle.py:.filter(cycle_id=self.kwargs.get("cycle_id")) Issue.issue_objects.filter(issue_cycle__cycle_id=cycle_id)
apiserver/plane/app/views/cycle/issue.py:.filter(cycle_id=self.kwargs.get("cycle_id")) Issue.issue_objects.filter(issue_cycle__cycle_id=cycle_id)
Action: Update these query filters to handle cases where
cycle_idisNone. For example, include checks likecycle_id__isnull=Truewhere appropriate.
Please address these issues to ensure that the application correctly handles scenarios where cycle_id can be None, thereby maintaining data consistency and preventing potential runtime errors.
🔗 Analysis chain
Verify the impact of the cycle_id annotation change.
While the change in the cycle_id annotation improves data consistency, it's important to ensure that this modification doesn't unintentionally affect other parts of the codebase that might rely on the cycle_id field, especially for deleted cycles.
Please run the following script to identify any potential issues:
This script will help identify areas of the codebase that might need attention due to the cycle_id annotation change. Please review the results and update any affected code as necessary.
Further verification needed for cycle_id null handling.
To ensure that the recent change allowing cycle_id to be None does not introduce issues, please run the following scripts to verify that serializers and query filters handle cycle_id appropriately.
This script performs the following checks:
-
Serializers Null Handling:
- Searches for
cycle_idfields in serializers. - Ensures that
allow_null=Trueis set, indicating that the field can acceptNonevalues.
- Searches for
-
Query Filters Null Handling:
- Looks for query filters that involve
cycle_id. - Checks whether these filters include conditions to handle
Nonevalues, such ascycle_id__isnull=True.
- Looks for query filters that involve
-
Direct Access Checks (Optional):
- Identifies places where
cycle_idis accessed directly. - Ensures that there are checks in place to handle scenarios where
cycle_idmight beNone.
- Identifies places where
Please review the results of these scripts and update the serializers or query conditions as necessary to handle cycle_id being None.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for usages of cycle_id that might be affected by the new annotation logic
# Search for cycle_id usages in Python files
echo "Searching for cycle_id usages:"
rg "cycle_id" --type py -g "!*/migrations/*"
# Search for potential filter conditions on cycle_id that might need updating
echo "\nSearching for potential cycle_id filters:"
rg "filter\(.*cycle_id" --type py -g "!*/migrations/*"
# Search for serializers that include cycle_id to ensure they handle None values
echo "\nSearching for serializers including cycle_id:"
rg "class.*Serializer" -A 10 -g "!*/migrations/*" | rg "cycle_id"
Length of output: 21795
Script:
#!/bin/bash
# Description: Verify serializers allow null values for cycle_id and query filters handle None appropriately
# Check serializers for cycle_id fields allowing nulls
echo "Checking serializers for cycle_id fields allowing null values:"
rg 'cycle_id\s*=\s*serializers\.PrimaryKeyRelatedField' --type py -g "!*/migrations/*" | grep -v 'allow_null=True'
# Check query filters using cycle_id without handling None
echo "\nChecking query filters for cycle_id without handling nulls:"
rg '\.filter\([^)]*cycle_id[^)]*\)' --type py -g "!*/migrations/*" | grep -v 'cycle_id__isnull'
# Optionally, check for direct accesses to cycle_id that might not account for None
echo "\nChecking for direct cycle_id accesses without None checks:"
rg 'cycle_id' --type py -g "!*/migrations/*" | grep -E 'if\s+cycle_id\s+|cycle_id\s*='
Length of output: 10160
| .annotate( | ||
| cycle_id=Case( | ||
| When( | ||
| issue_cycle__cycle__deleted_at__isnull=True, | ||
| then=F("issue_cycle__cycle_id"), | ||
| ), | ||
| default=None, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring to reduce code duplication.
The Case statement for cycle_id annotation is identical in both ProjectIssuesPublicEndpoint and IssueRetrievePublicEndpoint classes. To improve maintainability and reduce duplication, consider extracting this annotation logic into a separate method or utility function. This would make it easier to update the logic in the future if needed and ensure consistency across the codebase.
Example refactoring:
def annotate_cycle_id(queryset):
return queryset.annotate(
cycle_id=Case(
When(
issue_cycle__cycle__deleted_at__isnull=True,
then=F("issue_cycle__cycle_id"),
),
default=None,
)
)
# Then in both classes:
issue_queryset = annotate_cycle_id(issue_queryset)This refactoring would make the code more DRY (Don't Repeat Yourself) and easier to maintain.
Also applies to: 706-714
| .annotate( | ||
| cycle_id=Case( | ||
| When( | ||
| issue_cycle__cycle__deleted_at__isnull=True, | ||
| then=F("issue_cycle__cycle_id"), | ||
| ), | ||
| default=None, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Duplicate Annotation of cycle_id in get_queryset
The cycle_id annotation is being added in the get_queryset method. However, the same annotation is also present in the list method starting at line 288. This duplication may lead to redundant computations and could impact performance.
Consider removing the redundant annotation from either get_queryset or list to optimize the query performance.
| .annotate( | ||
| cycle_id=Case( | ||
| When( | ||
| issue_cycle__cycle__deleted_at__isnull=True, | ||
| then=F("issue_cycle__cycle_id"), | ||
| ), | ||
| default=None, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Redundant Annotation of cycle_id in list Method
The cycle_id annotation is being reapplied in the list method even though it was already annotated in get_queryset. Reapplying annotations can cause unnecessary overhead.
Since get_queryset already includes the cycle_id annotation, you can remove it from the list method to avoid redundancy.
chore:
Issue Link: WEB-2693
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
CustomImageBlockcomponent.Chores