[WEB-2631] chore: changed the cascading logic for soft delete #5829
Conversation
WalkthroughThe changes in this pull request involve modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 1
🧹 Outside diff range and nitpick comments (3)
apiserver/plane/bgtasks/deletion_task.py (3)
Line range hint
46-47: Implement therestore_related_objectsfunctionThe
restore_related_objectsfunction has been added but is currently unimplemented. This function seems intended to complement the soft delete functionality by providing a way to restore related objects.To complete this feature:
- Implement the logic to restore soft-deleted related objects.
- Ensure it handles all cases covered by
soft_delete_related_objects.- Add appropriate error handling and logging.
- Write unit tests to verify the restoration process.
Would you like assistance in drafting an initial implementation for this function?
Line range hint
1-165: Summary of review and next stepsThis review has identified several areas for improvement in the deletion tasks:
- The changes to
soft_delete_related_objectsneed revision to ensure correct handling ofon_deletebehaviors.- The new
restore_related_objectsfunction needs to be implemented.- The
hard_deletefunction, while unchanged, could benefit from refactoring for improved maintainability.To improve the overall design and reliability of these background tasks:
- Implement comprehensive error handling and logging throughout these functions.
- Consider adding transaction management to ensure data consistency during these operations.
- Implement the
restore_related_objectsfunction with proper testing.- Refactor the
hard_deletefunction as suggested for better maintainability.- Add integration tests to verify the entire deletion and restoration workflow.
These improvements will enhance the robustness and maintainability of the deletion and restoration processes in the system.
The proposed refactor of
hard_deletewould miss several models currently handled by the functionWhile the refactoring suggestion aims to improve maintainability by dynamically identifying models with a
deleted_atfield through inheritance, the current implementation ofhard_deletehandles numerous models that do not inherit from the specified base classes (AuditModel,TimeAuditModel,UserAuditModel,SoftDeleteModel). As a result, the proposed approach would exclude these models, leading to incomplete hard deletion operations.Models not covered by the proposed refactor:
- Workspace
- Project
- Cycle
- Module
- Issue
- Page
- IssueView
- Label
- State
- IssueActivity
- IssueComment
- IssueLink
- IssueReaction
- UserFavorite
- ModuleIssue
- CycleIssue
- Estimate
- EstimatePoint
To ensure comprehensive coverage, consider updating the refactor to include all models currently managed by
hard_delete, possibly by:
- Expanding the list of base classes to include all relevant classes that define
deleted_at.- Implementing a search mechanism that directly identifies all models with a
deleted_atfield, regardless of inheritance.🔗 Analysis chain
Line range hint
50-165: Consider refactoring thehard_deletefunction for improved maintainabilityWhile the
hard_deletefunction is unchanged in this PR, its current implementation has some potential issues:
- It explicitly lists many models, which could become difficult to maintain as the number of models grows.
- It uses a fixed number of days (
HARD_DELETE_AFTER_DAYS) for all models, which might not be flexible enough for different data retention requirements.Consider refactoring this function to:
- Use Django's content types framework to dynamically get all models with a
deleted_atfield.- Allow for model-specific retention periods.
Here's a sketch of how this could look:
from django.contrib.contenttypes.models import ContentType @shared_task def hard_delete(): for content_type in ContentType.objects.all(): model = content_type.model_class() if hasattr(model, 'deleted_at'): retention_days = getattr(model, 'HARD_DELETE_AFTER_DAYS', settings.HARD_DELETE_AFTER_DAYS) model.all_objects.filter( deleted_at__lt=timezone.now() - timezone.timedelta(days=retention_days) ).delete()This approach would be more maintainable and flexible. It would automatically handle new models with
deleted_atfields and allow for model-specific retention periods.To ensure this refactoring wouldn't miss any current models, let's check which models have a
deleted_atfield:Compare the results of this search with the models explicitly listed in the current
hard_deletefunction to ensure all are covered by the proposed refactoring.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for models with a deleted_at field rg --type python 'deleted_at\s*=\s*models\.DateTimeField' -g '!*/migrations/*'Length of output: 111
Script:
#!/bin/bash # Search for models with a deleted_at field using the correct file type identifier rg --type py 'deleted_at\s*=\s*models\.DateTimeField' -g '!*/migrations/*'Length of output: 144
Script:
#!/bin/bash # Identify base classes with 'deleted_at' in mixins.py base_classes=$(rg --type py 'class\s+(\w+)\(.*\):' apiserver/plane/db/mixins.py | sed -E 's/class\s+(\w+)\(.*:/\1/') # Search for models inheriting from these base classes for base in $base_classes; do rg --type py "class\s+\w+\(.*${base}.*\):" -g '!*/migrations/*' doneLength of output: 1701
Script:
#!/bin/bash # List of models handled by hard_delete models=( Workspace Project Cycle Module Issue Page IssueView Label State IssueActivity IssueComment IssueLink IssueReaction UserFavorite ModuleIssue CycleIssue Estimate EstimatePoint ) # Base classes that include 'deleted_at' base_classes=( AuditModel TimeAuditModel UserAuditModel SoftDeleteModel ) # Check inheritance for each model for model in "${models[@]}"; do # Search for the model class definition and its base classes inheritance=$(rg --type py "class\s+${model}\(.*\):" -g '!*/migrations/*' | grep -Eo "\(.*\)" | tr -d '()') # Check if any base class is in the list of base_classes inherited=false for base in "${base_classes[@]}"; do if echo "$inheritance" | grep -w "$base" > /dev/null; then inherited=true break fi done if [ "$inherited" = true ]; then echo "${model}: Inherits from a base class with 'deleted_at'" else echo "${model}: Does NOT inherit from a base class with 'deleted_at'" fi doneLength of output: 12341
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- apiserver/plane/app/views/workspace/draft.py (1 hunks)
- apiserver/plane/bgtasks/deletion_task.py (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
apiserver/plane/app/views/workspace/draft.py (1)
282-282: LGTM, but verify impact on activity logging.The change to serialize
created_recordsas a list looks good and likely aligns with the expected data structure for theissue_activitytask. However, please ensure the following:
- Verify that the
issue_activitytask and any downstream processes correctly handle this list structure.- Check if similar changes are needed in other parts of the codebase for consistency.
To verify the consistency of this change across the codebase, please run the following script:
This will help identify if similar changes are needed elsewhere in the codebase.
| not hasattr(field.remote_field, "on_delete") | ||
| or field.remote_field.on_delete == "CASCADE" |
There was a problem hiding this comment.
💡 Codebase verification
Incorrect usage of on_delete in deletion_task.py
The condition field.remote_field.on_delete == "CASCADE" uses a string literal instead of the models.CASCADE constant. This is likely incorrect and may prevent the condition from working as intended.
Suggested Revision:
if (
not hasattr(field.remote_field, "on_delete")
or field.remote_field.on_delete == models.CASCADE
):🔗 Analysis chain
Review the changes in the on_delete condition
The modification to the on_delete check introduces two potential issues:
-
The condition now includes cases where the
on_deleteattribute doesn't exist. This could lead to unexpected soft deletions for fields whereon_deletebehavior is not explicitly defined. -
The comparison
field.remote_field.on_delete == "CASCADE"uses a string literal "CASCADE" instead ofmodels.CASCADE. This is likely incorrect and won't match the actualmodels.CASCADEvalue.
Consider revising the condition as follows:
if (
hasattr(field.remote_field, "on_delete") and
field.remote_field.on_delete == models.CASCADE
):This maintains the original behavior of only soft-deleting related objects when CASCADE is explicitly specified.
To ensure we're not missing any custom on_delete implementations, let's check the codebase:
If this search returns results, we may need to adjust our approach to handle custom on_delete behaviors.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for custom on_delete implementations
rg --type python 'on_delete\s*=(?!.*models\.CASCADE).*' -g '!*/migrations/*'
Length of output: 331
Script:
#!/bin/bash
# Search for custom on_delete implementations using PCRE2 to support look-around
rg --pcre2 --type python 'on_delete\s*=(?!.*models\.CASCADE).*' -g '!*/migrations/*'
Length of output: 117
Script:
#!/bin/bash
# Search for custom on_delete implementations using PCRE2 and target Python files
rg --pcre2 'on_delete\s*=(?!.*models\.CASCADE).*' -g '*.py' -g '!*/migrations/*'
Length of output: 97472
chore:
Issue Link: WEB-2631
Summary by CodeRabbit
New Features
Bug Fixes