chore: soft deletion of cycle and module #5884
Conversation
WalkthroughThe changes in this pull request primarily focus on enhancing the management of cycle issues and related entities across various API endpoints. Key modifications include shifting from soft to hard deletion for cycle issues, implementing stricter validation for issue creation, and refining filtering logic to exclude deleted entities in multiple contexts. New endpoints for cycle progress and analytics have been introduced, and existing serializers and views have been updated to improve data handling and integrity. 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: 5
🧹 Outside diff range and nitpick comments (14)
apiserver/plane/utils/grouper.py (1)
38-38: Approved: Simplified module deletion checkThe change from
Q(issue_module__module__deleted_at__isnull=True)toQ(issue_module__deleted_at__isnull=True)aligns with the PR objective of implementing soft deletion for modules. This modification simplifies the condition by directly checking theissue_module's deletion status instead of relying on the related module's status.Consider adding a brief comment explaining the rationale behind this condition for better code maintainability:
# Check if the issue_module is not soft-deleted Q(issue_module__deleted_at__isnull=True)apiserver/plane/app/views/issue/sub_issue.py (1)
Line range hint
1-224: Summary of changes and potential impactThe modifications in this file focus on simplifying the annotation conditions for
cycle_idandmodule_ids. These changes align with similar modifications in other files, suggesting a broader effort to streamline how cycles and modules are filtered across the application.While these changes appear to be part of a consistent update, it's crucial to:
- Ensure that the simplified conditions don't unintentionally include cycles or modules that should be excluded.
- Verify that these changes don't break any existing functionality or tests.
- Update any related documentation to reflect these changes in filtering behavior.
Consider adding comments explaining the rationale behind these changes to improve code maintainability. Additionally, it might be beneficial to create or update unit tests specifically for these annotation conditions to ensure the desired behavior is maintained.
apiserver/plane/app/views/workspace/draft.py (1)
Line range hint
63-98: Overall: Consistent implementation of soft deletionThe changes in this file consistently implement soft deletion handling for both cycles and modules in the
get_querysetmethod. This improves data integrity by ensuring that soft-deleted records are excluded from the queryset.To further enhance code readability and maintainability, consider extracting the soft deletion conditions into a separate method or constant. For example:
SOFT_DELETE_CONDITION = Q(deleted_at__isnull=True) # Then use it in annotations: .annotate( cycle_id=Case( When( draft_issue_cycle__deleted_at__isnull=True, then=F("draft_issue_cycle__cycle_id"), ), default=None, ) ) .annotate( module_ids=Coalesce( ArrayAgg( "draft_issue_module__module_id", distinct=True, filter=~Q(draft_issue_module__module_id__isnull=True) & Q(draft_issue_module__module__archived_at__isnull=True) & SOFT_DELETE_CONDITION, ), Value([], output_field=ArrayField(UUIDField())), ), )This approach would make it easier to maintain consistent soft deletion logic across the codebase.
apiserver/plane/app/views/module/issue.py (1)
73-73: LGTM! Consider enhancing readability.The change from
issue_cycle__cycle__deleted_at__isnull=Truetoissue_cycle__deleted_at__isnull=Truealigns with the PR objective of implementing soft deletion for cycles. This modification ensures that thecycle_idis annotated based on whether theissue_cycleitself is not deleted, rather than checking the associated cycle's deletion status.To improve code readability, consider adding a comment explaining the logic behind this condition:
# Annotate cycle_id only if the issue_cycle is not soft-deleted issue_cycle__deleted_at__isnull=True,apiserver/plane/app/views/cycle/issue.py (1)
Line range hint
1-361: Summary of changes related to soft deletion strategyThis pull request introduces two significant changes in the
CycleIssueViewSet:
- Modified the condition for checking deleted cycles in the
listmethod.- Changed the deletion behavior in the
destroymethod to perform hard deletes.These changes appear to be part of a broader effort to refine the soft deletion strategy in the application. While they seem logical, they may have significant impacts on data management, query behavior, and potentially on data retention policies.
Consider the following recommendations:
- Document the rationale behind these changes in the code comments or project documentation.
- Update any relevant data management policies or documentation to reflect the new deletion behavior.
- Ensure that these changes are consistently applied across all related parts of the codebase.
- Consider adding appropriate logging or audit trails for permanent deletions, if not already in place.
- Review and update any data backup or recovery processes that might be affected by these changes.
apiserver/plane/app/views/issue/archive.py (2)
Line range hint
232-239: LGTM: Added state validation before archivingThe new condition to check the state group of the issue before archiving is a good addition. It ensures that only issues in the "completed" or "cancelled" state groups can be archived, which improves data integrity and aligns with business logic.
Suggestion: Consider using an f-string for the error message to make it more dynamic:
return Response( { "error": f"Can only archive issues in 'completed' or 'cancelled' state groups. Current state group: {issue.state.group}" }, status=status.HTTP_400_BAD_REQUEST, )This would provide more context about the current state group of the issue.
Line range hint
315-359: LGTM: Enhanced bulk archive functionalityThe changes in the
postmethod ofBulkArchiveIssuesEndpointare well-implemented:
- The validation for issue IDs improves the API's robustness.
- Checking each issue's state group ensures consistency with the single issue archive logic.
- Using error codes allows for more standardized error handling on the client side.
Suggestion for optimization: Consider using a single database query to filter issues by their state group, rather than checking each issue individually in the loop. This could improve performance for large batches:
issues = Issue.objects.filter( workspace__slug=slug, project_id=project_id, pk__in=issue_ids, state__group__in=["completed", "cancelled"] ).select_related("state") if issues.count() != len(issue_ids): return Response( { "error_code": ERROR_CODES["INVALID_ARCHIVE_STATE_GROUP"], "error_message": "Some issues are not in 'completed' or 'cancelled' state groups", }, status=status.HTTP_400_BAD_REQUEST, )This approach reduces database queries and moves the state group check to the database level.
apiserver/plane/api/views/module.py (1)
509-509: LGTM! Consider adding a comment for clarity.The change from
module_issue.delete()tomodule_issue.delete(soft=False)aligns with the PR objective of implementing soft deletion. This ensures that the module issue is permanently deleted rather than soft-deleted.Consider adding a brief comment explaining why
soft=Falseis used here, for better code readability and to prevent future confusion. For example:# Perform hard delete to permanently remove the module issue module_issue.delete(soft=False)apiserver/plane/db/models/issue.py (1)
94-94: LGTM! Consider adding a comment for clarity.The addition of
.filter(deleted_at__isnull=True)correctly implements soft deletion filtering for issues. This change aligns well with the PR objective of implementing soft deletion for cycles and modules.Consider adding a brief comment above this line to explain the purpose of this filter, e.g.:
# Filter out soft-deleted issues .filter(deleted_at__isnull=True)This would enhance code readability and make the soft deletion logic more explicit.
apiserver/plane/space/views/issue.py (1)
Line range hint
709-742: Approved: Improved filtering for module_ids annotation.The addition of
& Q(issue_module__deleted_at__isnull=True)in themodule_idsannotation is a good improvement. It ensures that only non-deleted issue_module relationships are considered when aggregating module IDs, enhancing data integrity.Consider combining the conditions for better readability:
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__deleted_at__isnull=True), + filter=Q( + issue_module__module_id__isnull=False, + issue_module__module__archived_at__isnull=True, + issue_module__deleted_at__isnull=True + ), ), Value([], output_field=ArrayField(UUIDField())), ),This change combines the conditions into a single Q object, which may be slightly more efficient and easier to read.
apiserver/plane/app/serializers/draft.py (1)
Line range hint
85-88: Redundant assignment ofmodulesvariableIn the
createmethod,modulesis assigned fromvalidated_data.pop("module_ids", None)and then immediately overwritten withself.initial_data.get("module_ids", None). This makes the first assignment redundant. Please verify whether the initial assignment is necessary, or consider removing it to avoid confusion and potential bugs.apiserver/plane/api/views/issue.py (3)
Line range hint
141-157: Handle potentialDoesNotExistexception when fetching issuesIn the
getmethod, fetching an issue withIssue.objects.get()may raise aDoesNotExistexception if no matching issue is found. To prevent unhandled exceptions and return an appropriate HTTP 404 response, consider wrapping the query in atry-exceptblock.Apply this diff to handle the exception:
def get(self, request, slug, project_id, pk=None): external_id = request.GET.get("external_id") external_source = request.GET.get("external_source") if external_id and external_source: + try: issue = Issue.objects.get( external_id=external_id, external_source=external_source, workspace__slug=slug, project_id=project_id, ) return Response( IssueSerializer( issue, fields=self.fields, expand=self.expand, ).data, status=status.HTTP_200_OK, ) + except Issue.DoesNotExist: + return Response( + {"detail": "Not found."}, + status=status.HTTP_404_NOT_FOUND + )
Line range hint
252-300: Refactor duplicated logic inputandpostmethodsThe
putmethod contains logic similar to thepostmethod, especially when creating a new issue when one does not exist. Consider refactoring to extract common functionality into helper methods or services to reduce code duplication and enhance maintainability.
Issue Found: The
Issuemodel does not implement soft deletion.
- The
deletemethod is not overridden inplane/db/models/issue.py, leading to permanent deletion of issues.🔗 Analysis chain
Line range hint
379-397: Ensure consistent soft deletion of issuesIn the
deletemethod ofIssueAPIEndpoint, the call toissue.delete()should perform a soft delete to align with the soft deletion implementation for cycles and modules. Verify that theIssuemodel'sdeletemethod is overridden to handle soft deletion; otherwise, the issue may be permanently deleted, which could lead to data integrity issues.Run the following script to confirm the
Issuemodel implements soft deletion:Expected result: The script should find a
deletemethod in theIssuemodel implementing soft deletion logic.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the Issue model uses soft deletion. # Search for the custom delete method in the Issue model. ast-grep --pattern $'class Issue($$$_): $$$ def delete($_$): $$$ $$$' plane/db/models/issue.pyLength of output: 187
Script:
#!/bin/bash # Description: Locate the Issue model class definition. ast-grep --pattern $'class Issue($$$_): $$$' $(fd --extension py)Length of output: 32694
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (18)
- apiserver/plane/api/views/cycle.py (1 hunks)
- apiserver/plane/api/views/issue.py (1 hunks)
- apiserver/plane/api/views/module.py (1 hunks)
- apiserver/plane/app/serializers/draft.py (2 hunks)
- apiserver/plane/app/views/cycle/issue.py (2 hunks)
- apiserver/plane/app/views/dashboard/base.py (2 hunks)
- apiserver/plane/app/views/inbox/base.py (3 hunks)
- apiserver/plane/app/views/issue/archive.py (1 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/module/issue.py (3 hunks)
- apiserver/plane/app/views/view/base.py (4 hunks)
- apiserver/plane/app/views/workspace/draft.py (2 hunks)
- apiserver/plane/app/views/workspace/user.py (1 hunks)
- apiserver/plane/db/models/issue.py (1 hunks)
- apiserver/plane/space/views/issue.py (3 hunks)
- apiserver/plane/utils/grouper.py (1 hunks)
🧰 Additional context used
🔇 Additional comments (30)
apiserver/plane/utils/grouper.py (1)
Line range hint
1-238: Summary: Improved handling of soft-deleted modules in issue queryingThe change in this file enhances the
issue_queryset_grouperfunction to better support soft deletion of modules. By modifying the condition formodule_idsin theannotations_map, the function now directly checks the deletion status ofissue_moduleinstead of its related module. This change is consistent with the PR objectives and similar modifications in other files.The rest of the file, including other utility functions like
issue_on_resultsandissue_group_values, remains unchanged and continues to support the overall functionality of grouping and querying issues.apiserver/plane/app/views/issue/sub_issue.py (2)
112-112: Verify the impact of modified module_ids annotation condition.The condition for
module_idsannotation has been changed from checking bothissue_module__module__deleted_at__isnull=Trueandissue_module__module__archived_at__isnull=Trueto onlyissue_module__deleted_at__isnull=True. While this is consistent with changes in other files, it's important to verify that this modification doesn't unintentionally include modules that should be excluded.To ensure this change doesn't introduce any unintended side effects, please run the following verification:
#!/bin/bash # Description: Check for any instances where the new condition might differ from the old one # Search for other occurrences of module_ids annotations rg -A 5 "module_ids.*ArrayAgg" --glob '!apiserver/plane/app/views/issue/sub_issue.py' # Check for any related tests that might need updating rg -A 5 "test.*module.*deleted" --glob '*test*.py'
56-56: Verify the impact of simplified cycle_id annotation condition.The condition for
cycle_idannotation has been simplified from checking bothissue_cycle__cycle__deleted_at__isnull=Trueandissue_cycle__cycle__deleted_at__isnull=Trueto onlyissue_cycle__deleted_at__isnull=True. While this aligns with changes in other files, it's crucial to verify that this simplification doesn't unintentionally include cycles that should be excluded.To ensure this change doesn't introduce any unintended side effects, please run the following verification:
apiserver/plane/app/views/issue/relation.py (1)
99-99: LGTM! Verify impact and update tests.This change aligns with the PR objective of implementing soft deletion for cycles. The condition now checks if the
issue_cyclerelation is not deleted, rather than checking the associated cycle's deletion status.Please ensure that:
- This change doesn't unintentionally alter the behavior of the
listmethod.- The tests are updated to cover this new condition.
To verify the impact, you can run the following script:
This will help ensure consistency across the codebase and identify any tests that may need updating.
apiserver/plane/app/views/workspace/draft.py (2)
63-66: LGTM: Improved handling of soft-deleted cyclesThis change enhances the
get_querysetmethod by only considering non-deleted draft issue cycles when annotating thecycle_id. This aligns with the PR's objective of implementing soft deletion and improves data integrity in the queryset.
98-98: LGTM: Consistent handling of soft-deleted modulesThis change in the
module_idsannotation is consistent with the previous modification and the PR's objective. It ensures that only non-deleted draft issue modules are included in themodule_idsannotation, improving data integrity in the queryset.apiserver/plane/app/views/module/issue.py (2)
344-344: Consistent with previous change. Verify implications.The modification from
module_issue.delete()tomodule_issue.delete(soft=False)in thedestroymethod is consistent with the similar change in thecreate_issue_modulesmethod. This ensures permanent deletion ofmodule_issue.Please refer to the previous comment regarding the verification of implications and consistency across the codebase. The same considerations apply here.
319-319: Verify implications of permanent deletion.The change from
module_issue.delete()tomodule_issue.delete(soft=False)ensures that the deletion ofmodule_issueis permanent, aligning with the PR objective. However, this modification has implications on data retention and recovery capabilities.Please verify:
- This change is consistent with the deletion behavior in other parts of the codebase.
- The implications of permanent deletion have been considered and approved by the team.
- Any dependent features or reports that might rely on soft-deleted data have been updated accordingly.
To ensure consistency across the codebase, run the following script:
This script will help identify any inconsistencies in the deletion methods used across the codebase.
apiserver/plane/app/views/cycle/issue.py (2)
361-361: Confirm the intention for permanent deletion of cycle issues.The deletion behavior for
cycle_issuehas been changed to usesoft=False, which means it will perform a permanent deletion instead of a soft delete.Please confirm:
- Is this change intentional and aligned with the project's data management strategy?
- Are there any data retention policies or audit requirements that might be affected by this change?
- Has this change been consistently applied across all relevant parts of the codebase?
To verify the consistency of this change, you can run the following command:
#!/bin/bash # Description: Check for inconsistencies in deletion behavior across the codebase echo "Searching for soft delete calls on CycleIssue:" rg "CycleIssue.*delete\(" -g "*.py" echo "Searching for hard delete calls on CycleIssue:" rg "CycleIssue.*delete\(.*soft=False" -g "*.py" echo "Note: Review the results to ensure consistent deletion behavior for CycleIssue across the codebase."
108-108: Verify the impact of the modified deletion check.The condition for checking if a cycle is deleted has been changed from
issue_cycle__cycle__deleted_at__isnull=Truetoissue_cycle__deleted_at__isnull=True. This simplifies the logic but potentially changes the query behavior.Please confirm:
- Is this change intentional?
- Does it align with the soft deletion strategy for cycles?
- Are there any scenarios where this might produce different results compared to the previous implementation?
To help verify, you can run the following query to check for any discrepancies:
apiserver/plane/app/views/issue/archive.py (3)
70-73: LGTM: Improved cycle_id annotation logicThe change from
issue_cycle__cycle__deleted_at__isnull=Truetoissue_cycle__deleted_at__isnull=Truealigns with the PR objective of implementing soft deletion for cycles. This modification ensures that thecycle_idis annotated based on the deletion status of theissue_cyclerelation itself, rather than the associated cycle. This approach is more direct and consistent with similar changes in other files.
Line range hint
275-281: LGTM: Improved unarchive queryThe addition of
archived_at__isnull=Falsein the query is a good improvement. It ensures that only archived issues can be unarchived, preventing potential errors or inconsistencies in the data. This change enhances the robustness of the unarchive operation.
Line range hint
360-361: LGTM: Optimized bulk update operationThe bulk update operation now only updates the
archived_atfield, which is more efficient and consistent with the changes made in the single issue archive method. This optimization reduces unnecessary database writes and improves performance.apiserver/plane/app/views/workspace/user.py (1)
126-126: LGTM. Verify impact on cycle-related queries.The change from
issue_cycle__cycle__deleted_at__isnull=Truetoissue_cycle__deleted_at__isnull=Truelooks good. This modification alters how thecycle_idis determined for issues, now checking if theissue_cycleitself is not deleted rather than the associated cycle.Please verify that this change doesn't negatively impact any cycle-related queries or functionalities. Run the following script to check for any other occurrences of
issue_cycle__cycle__deleted_atthat might need similar updates:Also, ensure that the documentation is updated to reflect this change in cycle deletion handling if necessary.
apiserver/plane/app/views/dashboard/base.py (4)
243-243: Approved: Improved module deletion handlingThis change enhances the
module_idsannotation by checking thedeleted_atfield of theissue_moduleassociation instead of the module itself. This aligns with the implementation of soft deletion for modules, ensuring that only active module associations are considered when aggregating issues.
412-412: Approved: Consistent module deletion handlingThis change in the
dashboard_created_issuesfunction mirrors the modification made indashboard_assigned_issues. It ensures consistent handling of soft-deleted modules across both assigned and created issues, maintaining the integrity of the dashboard data.
Line range hint
1-824: Overall improvement in soft deletion handling for modulesThe changes in this file enhance the dashboard functionality by properly handling soft-deleted modules. By modifying the
module_idsannotation in bothdashboard_assigned_issuesanddashboard_created_issuesfunctions, the code now correctly excludes issues associated with soft-deleted modules from the dashboard data. This aligns well with the PR objective of implementing soft deletion for modules and cycles.These modifications ensure that the dashboard accurately reflects the current state of issues and their associations with active modules, improving data consistency and user experience.
Line range hint
1-824: Consider reviewing module handling across the codebaseWhile the changes in this file are well-implemented and consistent, it might be beneficial to review other parts of the codebase that deal with module associations. This would ensure that the soft deletion logic for modules is consistently applied throughout the application, maintaining data integrity and preventing potential inconsistencies.
To assist with this review, you can run the following script to find other occurrences of module-related queries:
This will help identify any other locations where similar changes might be necessary.
✅ Verification successful
No additional module handling found in the codebase
The verification process did not identify any other occurrences of module-related queries. Therefore, the module handling changes are localized and consistent.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other occurrences of module-related queries rg -n "issue_module__module__deleted_at" --type pyLength of output: 50
apiserver/plane/app/views/issue/base.py (5)
91-93: LGTM: Improved cycle filtering for soft deletionThis change enhances the filtering condition for
cycle_idannotation to support soft deletion of cycles. It now correctly considers only non-deleted cycles by checkingissue_cycle__deleted_at__isnull=True.
514-516: LGTM: Enhanced module filtering for soft deletionThis modification improves the filtering condition for
module_idsannotation to support soft deletion of module associations. The addition ofQ(issue_module__deleted_at__isnull=True)ensures that only non-deleted module associations are considered.
615-617: LGTM: Consistent module filtering in partial updateThis change mirrors the modification made earlier in the file, applying the same soft deletion logic for module associations in the
partial_updatemethod. This ensures consistency in handling soft-deleted modules across different operations.
781-783: LGTM: Consistent soft deletion handling in paginated viewThese changes apply the same soft deletion logic for cycles and modules in the
IssuePaginatedViewSetclass. This ensures consistency in handling soft-deleted data across different views and operations, maintaining data integrity throughout the application.Also applies to: 915-917
91-93: Overall: Effective implementation of soft deletion for cycles and modulesThe changes in this file consistently implement soft deletion for cycles and modules across various methods and classes related to issue management. This approach ensures that:
- Only non-deleted cycles are considered when annotating
cycle_id.- Only non-deleted module associations are included when annotating
module_ids.- The soft deletion logic is applied consistently in different contexts (list view, detail view, updates, and paginated view).
These modifications enhance data integrity and provide a more robust handling of deleted entities in the issue management system.
Also applies to: 514-516, 615-617, 781-783, 915-917
apiserver/plane/space/views/issue.py (1)
112-112: Verify the impact of the cycle deletion condition change.The condition for checking if a cycle is deleted has been simplified. This change might affect how deleted cycles are handled in the issue retrieval process.
Please confirm that this change aligns with the intended behavior for handling deleted cycles. Run the following script to check for any inconsistencies:
apiserver/plane/app/views/view/base.py (3)
213-213: LGTM: Correct filtering of active issue cyclesThe addition of
issue_cycle__deleted_at__isnull=Trueensures that only issues associated with active (not soft-deleted) cycles are retrieved. This aligns with the soft deletion implementation for cycles.
269-269: LGTM: Correct filtering of active issue modulesIncluding
issue_module__deleted_at__isnull=Truein the filter ensures that only issues associated with active (not soft-deleted) modules are considered. This change is appropriate for the soft deletion of modules.
291-291: LGTM: Consistent filtering for soft-deleted cyclesThe filter
issue_cycle__deleted_at__isnull=Trueis consistently applied here to exclude issues linked to soft-deleted cycles. This maintains the integrity of issue listings by omitting those associated with deleted cycles.apiserver/plane/app/views/inbox/base.py (2)
118-118: Filtering issues to include only those with active cyclesThe condition
issue_cycle__deleted_at__isnull=Truecorrectly filters issues to include only those associated with cycles that have not been soft-deleted. This aligns with the PR objective of implementing soft deletion for cycles.
174-174: Filtering modules to include only those that are activeIncluding
& Q(issue_module__deleted_at__isnull=True)ensures that only issues linked to modules that have not been soft-deleted are retrieved. This change supports the soft deletion feature for modules as outlined in the PR objectives.apiserver/plane/api/views/issue.py (1)
208-208: Approve filtering of issues from soft-deleted cyclesThe addition of
Q(issue_cycle__deleted_at__isnull=True)in the annotation ensures that issues associated with soft-deleted cycles are excluded from query results. This change correctly aligns with the intended behavior of hiding issues linked to deleted cycles.
apiserver/plane/api/views/cycle.py
Outdated
| @@ -761,7 +761,7 @@ def delete(self, request, slug, project_id, cycle_id, issue_id): | |||
| cycle_id=cycle_id, | |||
| ) | |||
| issue_id = cycle_issue.issue_id | |||
| cycle_issue.delete() | |||
| cycle_issue.delete(soft=False) | |||
There was a problem hiding this comment.
💡 Codebase verification
Multiple Hard Deletes Detected Across the Codebase
The usage of delete(soft=False) is prevalent in several parts of the codebase, which may conflict with the PR's objective of implementing soft deletion for cycles and modules.
apiserver/plane/space/serializer/issue.pyapiserver/plane/app/views/module/base.pyapiserver/plane/app/views/project/base.pyapiserver/plane/app/views/module/issue.pyapiserver/plane/app/serializers/draft.pyapiserver/plane/app/serializers/issue.pyapiserver/plane/app/views/view/base.pyapiserver/plane/app/views/workspace/favorite.pyapiserver/plane/app/views/page/base.pyapiserver/plane/api/serializers/issue.pyapiserver/plane/app/views/issue/relation.pyapiserver/plane/app/views/cycle/base.pyapiserver/plane/app/views/cycle/issue.pyapiserver/plane/api/views/module.pyapiserver/plane/api/views/cycle.py
Please review these instances to ensure that hard deletions are intentional and align with the overall data management strategy. If soft deletions are preferred, consider refactoring these delete operations accordingly.
🔗 Analysis chain
Verify the intention of hard delete for cycle issues
The change cycle_issue.delete(soft=False) implements a hard delete for cycle issues. This seems to contradict the PR objective of implementing soft deletion for cycles and modules.
Could you please clarify if this is intentional? If not, consider changing it to cycle_issue.delete() for soft deletion or provide a rationale for using hard deletion in this specific case.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other occurrences of hard delete in the codebase
rg "delete\(soft=False\)" --type py
Length of output: 2104
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (8)
apiserver/plane/db/migrations/0082_alter_issue_managers_alter_cycleissue_issue_and_more.py (2)
21-25: Approve ForeignKey relationship change with a suggestion.The modification to the 'issue' field in the 'cycleissue' model looks good. The CASCADE delete behavior and the 'issue_cycle' related_name are appropriate for maintaining data integrity and ease of access.
Consider adding a
db_index=Trueparameter to the ForeignKey for potential performance improvements on queries involving this relationship:field=models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='issue_cycle', to='db.issue', db_index=True)
26-30: Approve ForeignKey relationship change with a suggestion.The modification to the 'draft_issue' field in the 'draftissuecycle' model is consistent with the changes made to the 'cycleissue' model. This ensures a uniform approach to handling both regular and draft issues.
As suggested for the 'cycleissue' model, consider adding a
db_index=Trueparameter here as well:field=models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='draft_issue_cycle', to='db.draftissue', db_index=True)apiserver/plane/bgtasks/deletion_task.py (1)
Line range hint
1-180: Summary of review and final recommendationsThis review has covered the following key points:
- The hardcoding of the deletion period to 60 days, which impacts flexibility and potentially compliance.
- The need for improved error handling and logging throughout the deletion tasks.
- Potential inconsistencies with task scheduling changes mentioned in the AI summary.
- Opportunities for refactoring to improve maintainability and performance.
Final recommendations:
- Reconsider the hardcoded deletion period and implement a more flexible solution.
- Implement comprehensive error handling and logging across all deletion-related functions.
- Ensure consistency between this file and the changes in task scheduling mentioned in the AI summary.
- Consider implementing model-specific deletion periods and a dry-run mode for testing.
- Review and implement the
restore_related_objectsfunction if it's needed for the project.These changes will enhance the robustness, maintainability, and flexibility of the deletion tasks, ensuring better alignment with potential compliance requirements and operational needs.
To further improve the overall architecture, consider:
- Implementing a configuration file for deletion periods and other settings.
- Creating a separate logging module for centralized log management.
- Developing unit tests for the deletion tasks to ensure reliability.
Please review these suggestions and implement those that align with the project's goals and best practices.
apiserver/plane/api/serializers/issue.py (1)
Line range hint
215-232: Summary of changes to IssueSerializer deletion behavior.The changes in the
updatemethod ofIssueSerializerremove thesoft=Falseargument from thedelete()method calls for bothIssueAssigneeandIssueLabel. This modification appears to be part of the broader implementation of soft deletion for issues and related entities.Please consider the following points:
- Ensure that these changes are consistent with the deletion behavior implemented in other parts of the application.
- Verify that the default deletion behavior (now used) aligns with the intended soft deletion strategy.
- Update any related documentation or comments to reflect these changes in deletion behavior.
- Consider adding tests to verify the new deletion behavior for both
IssueAssigneeandIssueLabel.To maintain consistency and clarity across the codebase, consider:
- Implementing a base model or mixin that standardizes the soft deletion behavior for all relevant models.
- Adding a configuration option to easily switch between soft and hard deletion globally or per model.
- Updating any queries that fetch related objects to account for the new soft deletion behavior, ensuring that "deleted" objects are not inadvertently included in results.
apiserver/plane/bgtasks/analytic_plot_export.py (1)
133-135: LGTM! Consider a minor optimization.The addition of
label_issue__deleted_at__isnull=Trueto the filter condition is correct and aligns with the PR objective of implementing soft deletion. This ensures that only labels associated with non-deleted issues are returned.For a slight performance optimization, consider combining the conditions:
labels__id__isnull=False, label_issue__deleted_at__isnull=True,This avoids the use of
&operator and may be more readable:workspace__slug=slug, **filters, labels__id__isnull=False, label_issue__deleted_at__isnull=True,apiserver/plane/app/views/analytic/base.py (1)
113-116: LGTM! Consider optimizing the query.The addition of
& Q(label_issue__deleted_at__isnull=True)effectively implements soft deletion for labels, aligning with the PR objectives. The indentation correction also improves code readability.Consider optimizing the query by combining the conditions:
Issue.objects.filter( workspace__slug=slug, **filters, labels__id__isnull=False, label_issue__deleted_at__isnull=True, )This approach may be more efficient as it avoids the use of the
&operator and keeps all conditions within a singlefiltercall.apiserver/plane/app/serializers/issue.py (1)
Line range hint
472-496: Approve URL validation and uniqueness checks for IssueLinkSerializer.The changes in the
IssueLinkSerializerare well-implemented and improve data integrity:
- The new
validate_urlmethod ensures only valid URLs are accepted.- The
createandupdatemethods now prevent duplicate URLs for the same issue.These modifications enhance the overall robustness of the serializer.
Consider standardizing the error messages for consistency:
- raise serializers.ValidationError({"error": "URL already exists for this Issue"}) + raise serializers.ValidationError({"error": "This URL already exists for the current issue."})Apply this change to both the
createandupdatemethods.apiserver/plane/db/models/cycle.py (1)
113-115: Consider renamingrelated_nameto reflect the many-to-one relationshipNow that the
issuefield inCycleIssuehas been changed from aOneToOneFieldto aForeignKey, it allows multipleCycleIssueinstances to relate to a singleIssue. It would be clearer to update therelated_nameto a plural form likeissue_cyclesto accurately reflect this relationship in reverse lookups.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (29)
- apiserver/.env.example (0 hunks)
- apiserver/plane/api/serializers/issue.py (2 hunks)
- apiserver/plane/api/views/cycle.py (1 hunks)
- apiserver/plane/api/views/inbox.py (1 hunks)
- apiserver/plane/app/serializers/draft.py (3 hunks)
- apiserver/plane/app/serializers/issue.py (2 hunks)
- apiserver/plane/app/views/analytic/base.py (1 hunks)
- apiserver/plane/app/views/cycle/base.py (2 hunks)
- apiserver/plane/app/views/cycle/issue.py (1 hunks)
- apiserver/plane/app/views/dashboard/base.py (6 hunks)
- apiserver/plane/app/views/inbox/base.py (10 hunks)
- apiserver/plane/app/views/issue/base.py (12 hunks)
- apiserver/plane/app/views/issue/relation.py (4 hunks)
- apiserver/plane/app/views/issue/sub_issue.py (4 hunks)
- apiserver/plane/app/views/module/issue.py (1 hunks)
- apiserver/plane/app/views/view/base.py (6 hunks)
- apiserver/plane/app/views/workspace/draft.py (4 hunks)
- apiserver/plane/bgtasks/analytic_plot_export.py (1 hunks)
- apiserver/plane/bgtasks/deletion_task.py (1 hunks)
- apiserver/plane/celery.py (0 hunks)
- apiserver/plane/db/migrations/0082_alter_issue_managers_alter_cycleissue_issue_and_more.py (1 hunks)
- apiserver/plane/db/models/cycle.py (1 hunks)
- apiserver/plane/db/models/draft.py (2 hunks)
- apiserver/plane/settings/common.py (0 hunks)
- apiserver/plane/space/serializer/issue.py (2 hunks)
- apiserver/plane/space/utils/grouper.py (1 hunks)
- apiserver/plane/space/views/issue.py (5 hunks)
- apiserver/plane/utils/grouper.py (1 hunks)
- apiserver/plane/utils/issue_filters.py (5 hunks)
💤 Files with no reviewable changes (3)
- apiserver/.env.example
- apiserver/plane/celery.py
- apiserver/plane/settings/common.py
✅ Files skipped from review due to trivial changes (1)
- apiserver/plane/utils/issue_filters.py
🚧 Files skipped from review as they are similar to previous changes (11)
- apiserver/plane/app/serializers/draft.py
- apiserver/plane/app/views/cycle/issue.py
- apiserver/plane/app/views/dashboard/base.py
- apiserver/plane/app/views/inbox/base.py
- apiserver/plane/app/views/issue/relation.py
- apiserver/plane/app/views/issue/sub_issue.py
- apiserver/plane/app/views/module/issue.py
- apiserver/plane/app/views/view/base.py
- apiserver/plane/app/views/workspace/draft.py
- apiserver/plane/space/views/issue.py
- apiserver/plane/utils/grouper.py
🧰 Additional context used
🔇 Additional comments (25)
apiserver/plane/db/migrations/0082_alter_issue_managers_alter_cycleissue_issue_and_more.py (4)
1-39: Overall, the migration looks good with some points to verify.This migration file successfully implements the necessary changes to support soft deletion for cycles and modules. The alterations to model managers, foreign key relationships, and unique constraints are consistent and well-structured.
Key points to address:
- Verify the implementation and purpose of the new 'issue_objects' manager.
- Consider adding
db_index=Trueto the foreign key fields for potential performance benefits.- Ensure that the soft deletion logic is correctly implemented in the corresponding models and views.
Given the significance of these changes, it's crucial to thoroughly test the following scenarios:
- Soft deletion of issues, cycles, and draft issues.
- Querying and filtering of soft-deleted records.
- Potential impact on existing data and queries.
Consider running comprehensive integration tests to validate the behavior of the entire issue and cycle management system with these new changes.
15-20: Verify the purpose and implementation of the new 'issue_objects' manager.A new model manager 'issue_objects' has been added to the 'issue' model. While this can be useful for customizing queryset behavior, it's important to ensure its implementation aligns with the soft deletion feature mentioned in the PR objectives.
Please confirm:
- The purpose of this new manager in relation to soft deletion.
- That the implementation of 'issue_objects' in the model file correctly handles soft-deleted records.
#!/bin/bash # Description: Verify the implementation of the 'issue_objects' manager # Test: Search for the 'issue_objects' manager implementation ast-grep --pattern $'class Issue(models.Model): $$$ issue_objects = $_($$) $$$'
35-38: Approve unique constraint addition with verification suggestion.The unique constraint on ('draft_issue', 'cycle', 'deleted_at') for the 'draftissuecycle' model is consistent with the changes made to the 'cycleissue' model. This ensures uniform handling of both regular and draft issues in relation to cycles and soft deletion.
Please ensure that the soft deletion logic for draft issues is implemented consistently with regular issues. Verify that:
- Soft deletion for draft issues updates the 'deleted_at' field.
- Queries for draft issues properly exclude soft-deleted records.
#!/bin/bash # Description: Verify the soft deletion implementation for draftissuecycle # Test: Search for soft deletion logic in draftissuecycle-related files rg -A 5 $'class DraftIssueCycle|def soft_delete|deleted_at'
31-34: Approve unique constraint addition with verification suggestion.The unique constraint on ('issue', 'cycle', 'deleted_at') for the 'cycleissue' model is a good addition. It prevents duplicate entries while supporting soft deletion by allowing entries with different deletion timestamps.
Please ensure that the soft deletion logic in the corresponding model and views correctly utilizes this constraint. Verify that:
- Soft deletion updates the 'deleted_at' field instead of removing the record.
- Queries filtering out soft-deleted records are implemented correctly.
apiserver/plane/bgtasks/deletion_task.py (3)
Line range hint
1-180: Enhance error handling and logging in deletion tasksThe current implementation of the deletion tasks could benefit from improved error handling and logging:
- The
hard_deletefunction lacks specific error handling, which could lead to silent failures.- The
restore_related_objectsfunction is empty, potentially indicating incomplete functionality.- There's no logging of deletion operations, which could be useful for auditing and debugging.
Consider the following improvements:
- Add error handling and logging to the
hard_deletefunction:import logging logger = logging.getLogger(__name__) @shared_task def hard_delete(): try: # ... existing deletion logic ... logger.info(f"Successfully completed hard delete operation") except Exception as e: logger.error(f"Error during hard delete operation: {str(e)}") # Optionally, re-raise the exception if you want the task to be marked as failed raise
- Implement the
restore_related_objectsfunction or remove it if not needed:@shared_task def restore_related_objects(app_label, model_name, instance_pk, using=None): try: model_class = apps.get_model(app_label, model_name) instance = model_class.all_objects.get(pk=instance_pk) instance.deleted_at = None instance.save(using=using) logger.info(f"Successfully restored {model_name} with pk {instance_pk}") except Exception as e: logger.error(f"Error restoring {model_name} with pk {instance_pk}: {str(e)}") raise
- Add more granular error handling in
soft_delete_related_objects:@shared_task def soft_delete_related_objects(app_label, model_name, instance_pk, using=None): try: model_class = apps.get_model(app_label, model_name) instance = model_class.all_objects.get(pk=instance_pk) # ... existing logic ... except ObjectDoesNotExist: logger.warning(f"{model_name} with pk {instance_pk} not found for soft deletion") except Exception as e: logger.error(f"Error during soft delete of {model_name} with pk {instance_pk}: {str(e)}") raiseTo ensure these changes align with the project's logging practices, please run:
#!/bin/bash # Description: Check current logging practices in the project echo "Checking for logging import and configuration:" rg "import logging" --type py rg "logging.getLogger" --type py echo "Checking for existing error handling patterns:" rg "try:" --type py -A 5
Line range hint
74-180: Review the overall impact of the fixed deletion periodThe change to a fixed 60-day period for all model deletions has significant implications:
- Uniform treatment: All models now have the same deletion period, which may not be appropriate for all data types.
- Compliance: This change might affect data retention policies and compliance requirements.
- Performance: The deletion task's performance might be impacted if 60 days is significantly different from the previous setting.
- Inconsistency with task scheduling: The AI summary mentioned the removal of the "check-every-day-to-delete-hard-delete" task, but this file doesn't reflect that change.
Consider the following recommendations:
- Implement model-specific deletion periods if required:
deletion_periods = { Workspace: 60, Project: 90, # ... other models } for model, period in deletion_periods.items(): model.all_objects.filter( deleted_at__lt=timezone.now() - timezone.timedelta(days=period) ).delete()
- Add logging to track the number of objects deleted for each model:
import logging logger = logging.getLogger(__name__) for model in [Workspace, Project, ...]: deleted_count = model.all_objects.filter( deleted_at__lt=timezone.now() - timezone.timedelta(days=days) ).delete()[0] logger.info(f"Deleted {deleted_count} {model.__name__} objects")
- Consider implementing a dry-run mode for testing the impact of changes:
def hard_delete(dry_run=False): # ... existing code ... for model in all_models: if hasattr(model, "deleted_at"): queryset = model.all_objects.filter( deleted_at__lt=timezone.now() - timezone.timedelta(days=days) ) if dry_run: print(f"Would delete {queryset.count()} {model.__name__} objects") else: queryset.delete()To ensure consistency with the task scheduling change, please run:
74-74:⚠️ Potential issueConsider the implications of hardcoding the deletion period
The
daysvariable has been hardcoded to 60, replacing the previoussettings.HARD_DELETE_AFTER_DAYS. This change has several implications:
- Reduced flexibility: The deletion period is no longer configurable through settings.
- Consistency: All models now use the same 60-day period for deletion.
- Potential oversight: This change might have been unintentional or not fully considered.
- Performance impact: A fixed 60-day period might not be optimal for all environments or use cases.
- Compliance concerns: Some data retention policies might require different deletion periods.
Consider the following options:
- Revert to using a configurable setting:
days = getattr(settings, 'HARD_DELETE_AFTER_DAYS', 60) # Default to 60 if not set
- If the intention is to use a fixed period, add a comment explaining the rationale:
# Fixed 60-day period for all deletions as per [reference to decision/policy] days = 60
- If different models require different retention periods, consider using a dictionary:
deletion_periods = { Workspace: 60, Project: 90, # ... other models }To ensure this change was intentional and its impact is understood, please run the following script:
apiserver/plane/db/models/draft.py (2)
237-240: LGTM! Changes improve flexibility and data integrity.The modification of
draft_issuefromOneToOneFieldtoForeignKeyallows multipleDraftIssueCycleinstances to be associated with a singleDraftIssue. This change, along with the newunique_togetherconstraint, improves flexibility while maintaining data integrity.Also applies to: 247-247
237-240: Verify impact on related code and existing data.These changes might affect code that assumes a one-to-one relationship between
DraftIssueandDraftIssueCycle. Additionally, the new constraint could impact existing data if there are any duplicate entries.Please run the following script to check for potential issues:
Also applies to: 247-247
apiserver/plane/space/utils/grouper.py (1)
38-47: Excellent implementation of soft deletion filtering!The changes to the
annotations_mapdictionary effectively implement soft deletion filtering for assignees and labels. This enhancement ensures that only active (non-deleted) associations are included in the issue annotations, which aligns perfectly with the PR's objective of implementing soft deletion for cycles and modules.These modifications significantly improve data integrity and consistency across the application. Great job on considering the implications of soft deletion in this utility function!
apiserver/plane/api/serializers/issue.py (2)
232-232: Verify the deletion behavior for IssueLabel.Similar to the change for IssueAssignee, the removal of
soft=Falsefrom thedelete()method call might change the deletion behavior for issue labels. Please confirm if this aligns with the intended soft deletion implementation for issues.To check the current deletion behavior, run the following script:
215-215: Verify the deletion behavior for IssueAssignee.The removal of
soft=Falsefrom thedelete()method call might change the deletion behavior for issue assignees. Please confirm if this aligns with the intended soft deletion implementation for issues.To check the current deletion behavior, run the following script:
apiserver/plane/app/serializers/issue.py (3)
Line range hint
1-1024: Summary of changes in issue.py
- Removal of soft deletion for issue assignees and labels (lines 204 and 221).
- Enhanced URL validation and uniqueness checks in
IssueLinkSerializer(lines 472-496).The changes to
IssueLinkSerializerare well-implemented and improve data integrity. However, the shift from soft to hard deletion for issue-related entities is a significant change that requires careful consideration.Please ensure that:
- The removal of soft deletion for issue assignees and labels is intentional and aligns with the project's data management strategy.
- Any dependent systems or processes that relied on soft deletion for these entities have been updated accordingly.
- Data migration or cleanup processes are in place to handle any potential inconsistencies that may arise from this change.
221-221: Confirm the intentional removal of soft deletion for issue labels.The removal of the
soft=Falseparameter from thedelete()call indicates a change from soft deletion to hard deletion for issue labels. This change is consistent with the modification made for issue assignees but could potentially result in data loss if not managed properly throughout the system.Please verify if this change is intentional and aligns with the project's overall soft deletion strategy. Consider running the following script to check for any remaining soft deletion logic for issue labels:
#!/bin/bash # Description: Check for any remaining soft deletion logic for issue labels # Search for soft deletion related to IssueLabel rg -i 'IssueLabel.*soft.*delete' --type py
204-204: Verify the intentional removal of soft deletion for issue assignees.The removal of the
soft=Falseparameter from thedelete()call suggests a shift from soft deletion to hard deletion for issue assignees. This change could potentially lead to data loss if not handled carefully in other parts of the system.Please confirm if this change is intentional and aligned with the overall soft deletion strategy for the project. Consider the following script to check for any remaining soft deletion logic for issue assignees:
apiserver/plane/app/views/issue/base.py (6)
91-91: Improved cycle filtering logicThe change from
issue_cycle__cycle__deleted_at__isnull=Truetoissue_cycle__deleted_at__isnull=Truesimplifies and improves the cycle filtering logic. This update focuses on the association between the issue and cycle rather than the cycle itself, which is likely more accurate for determining valid issue-cycle relationships.
223-223: Enhanced filtering for issue associationsThe changes in the
IssueViewSetclass improve the filtering logic for various issue associations:
- The
cycle_idannotation now correctly checks for non-deleted issue-cycle associations.- The
label_idsannotation adds a check for non-deleted label associations.- The
assignee_idsannotation ensures only active project members and non-deleted assignee associations are included.- The
module_idsannotation adds a check for non-deleted module associations.These updates provide more accurate results by excluding invalid or deleted associations.
Also applies to: 494-505, 515-515
Line range hint
596-617: Consistent improvements in association filtering, but potential oversightThe changes in the
partial_updatemethod are consistent with those in theretrievemethod, improving the filtering logic for labels, assignees, and modules. This consistency is good for maintaining accurate issue data across different operations.However, there's a commented-out line in the
assignee_idsannotation:# & Q(issue_assignee__deleted_at__isnull=True)This line is present and uncommented in the
retrievemethod. Was this intentionally left out here, or is it an oversight?Could you please clarify the intention behind commenting out this line? If it was unintentional, consider uncommenting it to maintain consistency with the
retrievemethod.
783-783: Consistent cycle filtering improvementThe change in the
IssuePaginatedViewSetclass'sget_querysetmethod is consistent with the earlier modifications to thecycle_idannotation. This ensures that the improved logic for filtering cycles based on non-deleted issue-cycle associations is maintained across different view sets.
Line range hint
897-918: Consistent improvements in association filteringThe changes in the
listmethod of theIssuePaginatedViewSetclass are consistent with the earlier modifications in theIssueViewSetclass. These improvements in the filtering logic forlabel_ids,assignee_ids, andmodule_idsensure that:
- Only non-deleted label associations are included.
- Only active project members and non-deleted assignee associations are considered.
- Only non-deleted module associations are included.
This consistency across different parts of the codebase helps maintain accurate and up-to-date issue data in various contexts.
Line range hint
1-918: Overall improvement in issue association filteringThe changes throughout this file consistently enhance the filtering logic for issue associations, including cycles, labels, assignees, and modules. These improvements focus on ensuring that only active, non-deleted associations are considered when retrieving or updating issue data.
Key benefits of these changes:
- More accurate representation of current issue relationships
- Reduced likelihood of displaying or processing outdated or invalid data
- Consistent handling of associations across different views and operations
These enhancements should lead to a more reliable and up-to-date issue management system, improving the overall user experience and data integrity.
One minor point to verify is the commented-out line in the
partial_updatemethod of theIssueViewSetclass, as mentioned in a previous comment.apiserver/plane/api/views/inbox.py (2)
239-242: Verify the correctness of theissue_assignee__deleted_atfield referenceEnsure that
issue_assignee__deleted_ataccurately references thedeleted_atfield on the issue-assignee relationship. If the related name is notissue_assignee, the filter may not exclude deleted assignees as intended.To confirm the related field name in the
Issuemodel, you can run:#!/bin/bash # Description: Verify the related_name for assignees in the Issue model # Expectation: The 'assignees' ManyToManyField should have a related_name that matches 'issue_assignee' ast-grep --pattern $'class Issue { $$$ assignees = ManyToManyField($_, $_, through=$_, related_name="issue_assignee") $$$ }'
230-231: Verify the correctness of thelabel_issue__deleted_atfield referencePlease ensure that
label_issue__deleted_atcorrectly references thedeleted_atfield on the label-issue relationship. If the related name is notlabel_issue, this filter may not work as intended, and deleted labels might not be properly excluded.To confirm the related field name in the
Issuemodel, you can run:apiserver/plane/api/views/cycle.py (1)
407-407: Potential Hard Deletion DetectedThe usage of
cycle.delete()may result in a hard deletion of the cycle, which could conflict with the PR's objective of implementing soft deletion for cycles. This issue has been previously raised and is still applicable.apiserver/plane/app/views/cycle/base.py (1)
147-151: No issues found with the filter updateThe added filter correctly ensures that only non-deleted assignees are included in the
assignee_idsannotation.
apiserver/plane/db/models/cycle.py
Outdated
| unique_together = ["issue", "cycle", "deleted_at"] | ||
| verbose_name = "Cycle Issue" | ||
| verbose_name_plural = "Cycle Issues" |
There was a problem hiding this comment.
Re-evaluate including deleted_at in the unique constraint
Including deleted_at in the unique_together = ["issue", "cycle", "deleted_at"] constraint allows multiple entries of the same issue and cycle combination as long as deleted_at differs. This could lead to unexpected duplicates if deleted_at is not consistently managed. Consider whether the uniqueness should be enforced only on issue and cycle when deleted_at is null to prevent duplicates of active records.
| @@ -491,7 +496,7 @@ def destroy(self, request, slug, project_id, pk): | |||
| origin=request.META.get("HTTP_ORIGIN"), | |||
| ) | |||
| # TODO: Soft delete the cycle break the onetoone relationship with cycle issue | |||
| cycle.delete(soft=False) | |||
| cycle.delete() | |||
There was a problem hiding this comment.
Hard deletion of cycles may lead to data loss; consider addressing soft delete issues
Changing to cycle.delete() now performs a hard delete of the cycle, which may result in permanent data loss. The existing TODO comment indicates that soft deletion breaks the one-to-one relationship with CycleIssue. It would be better to resolve the soft deletion issue rather than switching to a hard delete as a workaround.
Would you like assistance in resolving the soft deletion issue? I can help investigate why soft deletion is causing issues with the one-to-one relationship and propose a solution.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
apiserver/plane/db/migrations/0082_alter_issue_managers_alter_cycleissue_issue_and_more.py (2)
15-20: LGTM! Consider adding documentation for the new manager.The addition of the
issue_objectsmanager to theissuemodel is a good practice for implementing custom query logic, potentially for soft deletion.Consider adding a comment or documentation to explain the purpose and functionality of this new manager, either in this migration file or in the corresponding model file.
47-62: LGTM! Consider adding explanatory comments.The additional constraints for
cycleissueanddraftissuecycleeffectively implement soft deletion logic by ensuring uniqueness for active (non-deleted) entries.Consider adding comments to explain the purpose of these constraints, especially how they work in conjunction with the previous unique constraints to implement soft deletion.
apiserver/plane/db/models/cycle.py (1)
Line range hint
113-172: Summary of changes and recommendation for soft deletion implementationThe changes in both
CycleIssueandCycleUserPropertiesmodels consistently implement constraints for soft deletion. However, there's a potential risk of creating duplicate entries with differentdeleted_atvalues.To mitigate this risk and ensure data integrity:
- Implement a custom manager or override the
savemethod to handle soft deletion consistently.- Consider adding a database-level trigger to prevent the creation of duplicate active records.
- Thoroughly test the soft deletion and restoration processes to ensure they behave as expected.
Would you like assistance in implementing these safeguards or creating comprehensive test cases for the soft deletion functionality?
apiserver/plane/app/views/issue/base.py (4)
Line range hint
398-421: Enhanced activity tracking for issue creationThe addition of issue and model activity tracking improves the system's ability to log and monitor changes. The use of asynchronous processing (
delaycalls) is a good practice for performance optimization.For consistency, consider extracting the common data preparation (like
json.dumps(request.data, cls=DjangoJSONEncoder)) into a variable to be used in bothissue_activity.delay()andmodel_activity.delay()calls.Also applies to: 441-449
Line range hint
598-625: Consistent soft deletion and enhanced activity tracking in partial updatesThe changes in the
partial_updatemethod maintain consistency with the soft deletion implementation and improve activity tracking. The use ofjson.dumps()for data serialization is a good practice.To improve code reusability, consider extracting the common annotation logic (for
label_ids,assignee_ids, andmodule_ids) into a separate method or queryset annotation that can be reused across different views.Also applies to: 656-677
791-793: Consistent soft deletion checks in paginated viewThe changes in the
IssuePaginatedViewSetclass maintain consistency with the soft deletion implementation seen throughout the file. This ensures that pagination only considers active relationships.To potentially optimize performance, consider using subqueries instead of annotations for
label_ids,assignee_ids, andmodule_idsif these fields are not always needed. This could reduce the complexity of the main query when these fields are not required.Also applies to: 903-930
Line range hint
1-930: Overall implementation of soft deletion and activity trackingThe changes in this file consistently implement soft deletion checks across various issue-related operations, aligning well with the PR objectives. The addition of activity tracking enhances the system's ability to monitor changes.
Key points to address:
- Consider extracting common annotation logic into reusable methods or queryset annotations.
- Clarify the intention behind removing the
soft=Falseargument in thedestroymethod, as it seems to contradict the soft deletion strategy.- Evaluate the suggestion to use subqueries instead of annotations in the
IssuePaginatedViewSetfor potential performance optimization.These improvements will further enhance the consistency and efficiency of the implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
- apiserver/plane/api/views/inbox.py (1 hunks)
- apiserver/plane/app/views/dashboard/base.py (4 hunks)
- apiserver/plane/app/views/inbox/base.py (9 hunks)
- apiserver/plane/app/views/issue/base.py (9 hunks)
- apiserver/plane/app/views/issue/relation.py (4 hunks)
- apiserver/plane/app/views/issue/sub_issue.py (3 hunks)
- apiserver/plane/app/views/view/base.py (5 hunks)
- apiserver/plane/app/views/workspace/draft.py (3 hunks)
- apiserver/plane/db/migrations/0082_alter_issue_managers_alter_cycleissue_issue_and_more.py (1 hunks)
- apiserver/plane/db/models/cycle.py (1 hunks)
- apiserver/plane/db/models/draft.py (2 hunks)
- apiserver/plane/space/views/issue.py (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- apiserver/plane/api/views/inbox.py
- apiserver/plane/app/views/dashboard/base.py
- apiserver/plane/app/views/inbox/base.py
- apiserver/plane/app/views/issue/relation.py
- apiserver/plane/app/views/issue/sub_issue.py
- apiserver/plane/app/views/view/base.py
- apiserver/plane/app/views/workspace/draft.py
- apiserver/plane/db/models/draft.py
- apiserver/plane/space/views/issue.py
🧰 Additional context used
🔇 Additional comments (10)
apiserver/plane/db/migrations/0082_alter_issue_managers_alter_cycleissue_issue_and_more.py (5)
30-38: LGTM! Consistent with previous change.The modification of the
draft_issuefield in thedraftissuecyclemodel mirrors the previous change, ensuring consistency in the data model.
43-46: LGTM! Consistent with previous constraint.The unique constraint on (
draft_issue,cycle,deleted_at) for thedraftissuecyclemodel is consistent with the previous constraint, supporting soft deletion.
1-63: Overall, the migration implements soft deletion effectively.This migration file successfully implements the necessary database changes to support soft deletion for cycles and modules. The combination of altered fields, unique constraints, and additional constraints provides a robust framework for managing deleted entries while maintaining data integrity.
To ensure the changes are fully effective:
- Add documentation for the new
issue_objectsmanager and the purpose of the constraints.- Verify that related views properly handle the new cascade deletion behavior and unique constraints.
- Consider adding tests to validate the soft deletion functionality in practice.
39-42: LGTM! Verify unique constraint handling in views.The unique constraint on (
issue,cycle,deleted_at) for thecycleissuemodel supports soft deletion implementation.Ensure that the views handling cycleissue creation and updates properly handle this unique constraint. Run the following script to verify:
#!/bin/bash # Description: Check for unique constraint handling in cycleissue-related views # Test: Search for cycleissue creation and update logic rg -i 'cycleissue.*create|update.*cycleissue' apiserver/plane/api/views/
21-29: LGTM! Verify cascade behavior in related views.The modification of the
issuefield in thecycleissuemodel with CASCADE deletion and arelated_nameis appropriate for maintaining data integrity.Please ensure that the cascade behavior is properly handled in related views, particularly in
apiserver/plane/api/views/cycle.pyandapiserver/plane/api/views/issue.py. Run the following script to verify:apiserver/plane/db/models/cycle.py (2)
Line range hint
165-172: Consistent changes inCycleUserPropertiesmodel constraintsThe changes in the
CycleUserPropertiesmodel are consistent with those in theCycleIssuemodel:
- The existing
unique_togetherconstraint is retained, ensuring uniqueness of thecycle,user, anddeleted_atcombination.- A new
UniqueConstraintis added to enforce uniqueness ofcycleanduserwhendeleted_atis null.These changes align with the PR objective of implementing soft deletion. However, the same potential issue exists here:
Including
deleted_atin theunique_togetherconstraint might allow multiple entries of the samecycleandusercombination with differentdeleted_atvalues. This could lead to unexpected duplicates ifdeleted_atis not consistently managed.To verify this concern, let's check if there are any safeguards in place:
Consider whether uniqueness should be enforced only on
cycleanduserwhendeleted_atis null to prevent duplicates of active records.
113-128: Significant changes inCycleIssuemodel structure and constraintsThe changes in the
CycleIssuemodel introduce more flexibility in the relationship between cycles and issues while implementing soft deletion:
- Changing
issuefromOneToOneFieldtoForeignKeyallows multipleCycleIssueinstances to relate to a singleIssue.- The
unique_togetherconstraint ensures uniqueness of theissue,cycle, anddeleted_atcombination.- The new
UniqueConstraintenforces uniqueness ofcycleandissuewhendeleted_atis null.These changes align with the PR objective of implementing soft deletion. However, there's a potential issue to consider:
Including
deleted_atin theunique_togetherconstraint might allow multiple entries of the sameissueandcyclecombination with differentdeleted_atvalues. This could lead to unexpected duplicates ifdeleted_atis not consistently managed.To verify this concern, let's check if there are any safeguards in place:
Consider whether uniqueness should be enforced only on
issueandcyclewhendeleted_atis null to prevent duplicates of active records.apiserver/plane/app/views/issue/base.py (3)
91-93: Improved cycle_id annotation logicThe simplification of the
cycle_idannotation condition enhances efficiency and correctly implements soft deletion at the issue-cycle relationship level. This change aligns well with the PR's objective of implementing soft deletion.
223-225: Consistent implementation of soft deletion checksThe changes in the
cycle_id,label_ids,assignee_ids, andmodule_idsannotations consistently implement soft deletion checks across different relationships. This ensures that only active (non-deleted) relationships are considered when querying issues, improving data consistency and aligning with the PR's soft deletion objectives.Also applies to: 492-507, 515-519
Line range hint
679-694: Clarification needed on deletion strategyThe removal of the
soft=Falseargument from thedelete()call suggests a shift towards hard deletion. This seems to contradict the PR objective of implementing soft deletion for cycles and modules. Could you please clarify if this change is intentional and how it aligns with the overall soft deletion strategy?
chore:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements
Documentation