[WEB-2680] chore: updated queryset for soft delete #5844
Conversation
WalkthroughThe changes in this pull request focus on updating the deletion behavior of related entities within several serializer classes, specifically for issues, drafts, and their associated labels and assignees. The serializers have been modified to implement both soft and hard deletion methods. Additionally, a new class for soft deletion has been introduced in the database layer, enhancing the overall management of deletion processes across the application. 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 (
|
![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 (4)
apiserver/plane/app/serializers/draft.py (1)
Security Concern:
description_htmlField Lacks SanitizationThe
description_htmlfield in theDraftIssueDetailSerializerdoes not appear to have any sanitization measures in place. This omission can lead to Cross-Site Scripting (XSS) vulnerabilities, allowing malicious HTML or scripts to be injected.To address this issue:
Implement Sanitization:
- Use a library like
bleachto sanitize thedescription_htmlcontent before saving or rendering.- Example:
import bleach class DraftIssueDetailSerializer(DraftIssueSerializer): description_html = serializers.CharField() def validate_description_html(self, value): return bleach.clean(value)Review Serializer Usages:
- Ensure that all instances where
DraftIssueDetailSerializeris used properly handle the sanitizeddescription_html.Update Frontend Components:
- Verify that frontend components rendering
description_htmlsafely handle HTML content, avoiding the execution of unintended scripts.Add Validation:
- Implement additional validation to enforce HTML content standards and size limits as per application requirements.
🔗 Analysis chain
Line range hint
291-298: LGTM. Consider security measures for HTML content.The addition of the
description_htmlfield enhances the functionality of theDraftIssueDetailSerializer. However, please consider the following:
- Implement appropriate security measures to sanitize the HTML content to prevent XSS attacks. Consider using a library like
bleachfor this purpose.- Update any frontend components that render this field to properly handle HTML content.
- If not already in place, add validation to ensure the HTML content meets your application's standards and size limits.
To ensure proper handling of the new field, you can run the following script:
This will help identify areas where the serializer is used, allowing you to verify that the new field is properly handled throughout the application.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of DraftIssueDetailSerializer in the codebase rg --type python 'DraftIssueDetailSerializer' .Length of output: 78
Script:
#!/bin/bash # Corrected search for usage of DraftIssueDetailSerializer with the right file type rg --type py 'DraftIssueDetailSerializer' .Length of output: 401
Script:
#!/bin/bash # Search for sanitization functions applied to description_html in the identified files rg --type py 'description_html.*bleach\.clean' ./apiserver/plane/app/views/workspace/draft.py rg --type py 'description_html.*bleach\.clean' ./apiserver/plane/app/serializers/draft.py rg --type py 'description_html.*bleach\.clean' ./apiserver/plane/app/serializers/__init__.pyLength of output: 282
apiserver/plane/api/serializers/issue.py (1)
215-215: Approved: Hard delete implementation aligns with PR objective.The changes to use
delete(soft=False)forIssueAssigneeandIssueLabelinstances align with the PR objective of refactoring the queryset for soft delete. This ensures that old assignees and labels are permanently removed when updating an issue.However, it's important to note that this change has implications on data retention and recovery:
- Historical data about previous assignees and labels will be permanently lost.
- This might affect the ability to audit changes or revert to previous states.
Consider implementing the following to mitigate potential data loss:
- A backup mechanism to store deleted assignees and labels information.
- Logging of deletion operations for auditing purposes.
- A "history" table to track changes to issue assignees and labels over time.
Example implementation for a history table:
class IssueAssigneeHistory(models.Model): issue = models.ForeignKey(Issue, on_delete=models.CASCADE) assignee = models.ForeignKey(User, on_delete=models.SET_NULL, null=True) assigned_at = models.DateTimeField(auto_now_add=True) unassigned_at = models.DateTimeField(null=True) class IssueLabelHistory(models.Model): issue = models.ForeignKey(Issue, on_delete=models.CASCADE) label = models.ForeignKey(Label, on_delete=models.SET_NULL, null=True) added_at = models.DateTimeField(auto_now_add=True) removed_at = models.DateTimeField(null=True)These models would allow tracking the history of assignees and labels for each issue without relying on soft delete.
Also applies to: 232-232
apiserver/plane/space/serializer/issue.py (1)
Line range hint
1-563: Recommendation: Comprehensive review of soft delete implementationWhile the changes in this file introduce a deletion mechanism for
IssueAssigneeandIssueLabelinstances, it appears that the soft delete functionality might not be fully implemented as intended across the codebase.Consider the following recommendations:
- Conduct a comprehensive review of all model-related operations in the codebase to ensure consistent implementation of the soft delete mechanism.
- Implement a base model or mixin that includes soft delete functionality, which can be inherited by all relevant models.
- Update all relevant serializers and views to handle soft-deleted records appropriately (e.g., filtering them out in list views unless explicitly requested).
- Ensure that any cascade delete operations also respect the soft delete mechanism.
Would you like assistance in creating a plan for implementing these recommendations across the codebase?
apiserver/plane/db/models/issue.py (1)
Line range hint
83-98: Consider the impact on accessing soft-deletedIssueinstances.With soft deletion implemented, any queries that need to include soft-deleted issues will require adjustments. Ensure that such use cases are accounted for by using a manager that includes soft-deleted records or by explicitly handling them in queries.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- apiserver/plane/api/serializers/issue.py (2 hunks)
- apiserver/plane/app/serializers/draft.py (2 hunks)
- apiserver/plane/app/serializers/issue.py (2 hunks)
- apiserver/plane/db/mixins.py (1 hunks)
- apiserver/plane/db/models/issue.py (2 hunks)
- apiserver/plane/space/serializer/issue.py (2 hunks)
🧰 Additional context used
🔇 Additional comments (9)
apiserver/plane/app/serializers/draft.py (1)
172-174: LGTM. Verify impact on data retention and update documentation.The change from soft delete to hard delete for
DraftIssueAssigneeandDraftIssueLabelaligns with the PR objective. However, please consider the following:
- Verify that this change doesn't conflict with any data retention policies.
- Update any relevant documentation to reflect this change in deletion behavior.
- Ensure that any code relying on soft delete for these models is updated accordingly.
To verify the impact of this change, you can run the following script:
This will help identify any other instances where soft delete is being used, ensuring consistency across the codebase.
Also applies to: 191-193
✅ Verification successful
Deletion Behavior Verified
All existing
delete()methods withsoft=Trueare only method definitions inapiserver/plane/db/mixins.pyand are not invoked elsewhere in the codebase. The changes indraft.pyto usesoft=FalseforDraftIssueAssigneeandDraftIssueLabeldo not conflict with other parts of the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any other occurrences of soft delete in the codebase rg --type python 'delete\(.*soft=True.*\)' .Length of output: 77
Script:
#!/bin/bash # Find all Python files and search for 'delete' calls with 'soft=True' fd --extension py --type file . | xargs grep -H 'delete(.*soft=True.*)'Length of output: 226
apiserver/plane/app/serializers/issue.py (1)
204-204:⚠️ Potential issueCaution: Changing from soft delete to hard delete
The modification from soft delete to hard delete for
IssueAssigneeandIssueLabelobjects aligns with the PR objective of refactoring the queryset for soft delete. However, this change has significant implications:
- Data Retention: Hard deletes permanently remove data, which may impact data retention policies and audit trails.
- Recoverability: Unlike soft deletes, hard-deleted data cannot be easily recovered.
- Cascading Effects: Ensure that any dependent data or relationships are properly handled to avoid orphaned records.
Consider the following actions:
- Update the documentation to reflect this change in deletion behavior.
- Review and update any code that relies on soft-deleted
IssueAssigneeandIssueLabelrecords.- Ensure that proper backups are in place before deploying this change to production.
- Consider adding a confirmation step or a grace period before permanent deletion if not already implemented.
To ensure this change doesn't introduce unintended consequences, please run the following verification script:
This script will help identify any potential areas in the codebase that might be affected by this change from soft delete to hard delete.
Also applies to: 221-221
apiserver/plane/db/mixins.py (3)
46-51: Implementation ofSoftDeletionQuerySetfor soft deletionThe
deletemethod inSoftDeletionQuerySetcorrectly handles soft deletion by updating thedeleted_atfield whensoft=True, and performs a hard delete by callingsuper().delete()whensoft=False. This provides flexibility in deletion operations at the queryset level.
56-58:SoftDeletionManagerfilters out soft-deleted recordsThe
get_querysetmethod inSoftDeletionManagerreturns aSoftDeletionQuerySetfiltered to exclude records wheredeleted_atis set, effectively omitting soft-deleted instances from regular queries.
Line range hint
65-86:SoftDeleteModelcorrectly manages soft and hard deletionThe
deletemethod inSoftDeleteModelappropriately handles soft deletion by settingdeleted_atto the current timestamp and saving the instance whensoft=True. It also enqueues a task to soft delete related objects, ensuring that cascading deletions are managed asynchronously. Whensoft=False, it performs a hard delete by invokingsuper().delete().apiserver/plane/db/models/issue.py (4)
Line range hint
83-98: Confirm the integration ofSoftDeletionManagerwithIssueManager.The
IssueManagernow inherits fromSoftDeletionManager, which introduces soft deletion functionality to theIssuemodel. This change should ensure that soft-deleted issues are appropriately handled in queries.
Line range hint
83-98: Ensure that queryset filters behave as expected with soft deletion.The
get_querysetmethod applies several filters and exclusions. Verify that these filters, in combination with soft deletion, return the correct set ofIssueinstances, especially considering fields likeissue_inbox__status,state__is_triage,archived_at,project__archived_at, andis_draft.Consider adding tests to validate the queryset results after the introduction of soft deletion.
Line range hint
83-98: Update all references toIssue.objectsin the codebase.Since
objects = models.Manager()is removed from theIssueclass, ensure that all references toIssue.objectsare updated to useIssue.issue_objectsor the appropriate manager to prevent potentialAttributeErrors.Run the following script to find occurrences of
Issue.objects:#!/bin/bash # Description: Find all references to Issue.objects in the codebase. # Test: Search for 'Issue.objects' in Python files. rg --type py 'Issue\.objects'
15-15: Verify the import ofSoftDeletionManager.The import statement adds
SoftDeletionManagerfromplane.db.mixins. Ensure that this class is correctly defined and available in the codebase to avoid import errors.Run the following script to confirm the existence of
SoftDeletionManager:✅ Verification successful
Verified:
SoftDeletionManageris correctly defined inplane/db/mixins.py.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if SoftDeletionManager is defined in plane/db/mixins.py # Test: Search for the class definition of SoftDeletionManager rg --type py --pcre2 'class\s+SoftDeletionManager' plane/db/mixins.pyLength of output: 14932
Script:
#!/bin/bash # Description: Search for the definition of SoftDeletionManager across the entire codebase # Test: Search for the class definition using ripgrep rg --type py --pcre2 'class\s+SoftDeletionManager' .Length of output: 128
| @@ -421,7 +421,7 @@ def update(self, instance, validated_data): | |||
| updated_by_id = instance.updated_by_id | |||
|
|
|||
| if assignees is not None: | |||
| IssueAssignee.objects.filter(issue=instance).delete() | |||
| IssueAssignee.objects.filter(issue=instance).delete(soft=False) | |||
There was a problem hiding this comment.
Inconsistency with PR objective: Hard delete implemented instead of soft delete
The changes in lines 424 and 441 are implementing a hard delete (soft=False) for IssueAssignee and IssueLabel instances. This contradicts the PR objective of implementing a soft delete mechanism.
Consider the following points:
- Using
soft=Falsewill permanently remove the data, which may not be the intended behavior for a soft delete implementation. - This change could lead to unintended data loss, making it difficult to recover or audit historical assignments and labels.
To align with the PR objective, consider changing soft=False to soft=True or removing the soft parameter altogether if the default behavior is soft delete. For example:
IssueAssignee.objects.filter(issue=instance).delete()and
IssueLabel.objects.filter(issue=instance).delete()Please verify if this aligns with the intended soft delete implementation across the project.
Also applies to: 441-441
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
apiserver/plane/api/serializers/issue.py (1)
215-215: LGTM! Consider adding a comment for clarity.The changes from
delete()todelete(soft=False)for bothIssueAssigneeandIssueLabelare correct and align with the PR objective of refactoring the queryset for soft delete functionality. This modification maintains the current hard deletion behavior while preparing the codebase for potential soft deletion in the future.For improved clarity and consistency, consider adding a brief comment explaining the reason for using
soft=False. For example:# Perform hard delete to maintain current behavior while supporting future soft delete functionality IssueAssignee.objects.filter(issue=instance).delete(soft=False)This comment would help other developers understand the intention behind the explicit
soft=Falseparameter.Also applies to: 232-232
apiserver/plane/db/mixins.py (1)
Line range hint
82-83: Pass*argsand**kwargstosave()MethodIn the
deletemethod ofSoftDeleteModel, when performing a soft delete, consider passing*argsand**kwargstoself.save()to ensure any additional arguments are properly handled. This is important if thesave()method has been overridden elsewhere and relies on these parameters.Apply this diff:
- self.save(using=using) + self.save(using=using, *args, **kwargs)apiserver/plane/app/serializers/draft.py (1)
Line range hint
238-244: Avoid redundant inclusion ofdescription_htmlinfieldsIn
DraftIssueDetailSerializer, you're addingdescription_htmlto thefieldslist:fields = DraftIssueSerializer.Meta.fields + [ "description_html", ]However,
description_htmlis already included inDraftIssueSerializer.Meta.fields. Adding it again results in duplication.Apply this diff to remove the redundant field addition:
class DraftIssueDetailSerializer(DraftIssueSerializer): description_html = serializers.CharField() class Meta(DraftIssueSerializer.Meta): - fields = DraftIssueSerializer.Meta.fields + [ - "description_html", - ] + fields = DraftIssueSerializer.Meta.fields read_only_fields = fieldsThis ensures that
description_htmlremains in thefieldswithout duplication.apiserver/plane/db/models/issue.py (1)
Removal of the default
objectsmanager impacts multiple areas in the codebase.The following files still use
Issue.objects:
- apiserver/back_migration.py
- apiserver/plane/license/bgtasks/tracer.py
- apiserver/plane/bgtasks/issue_automation_task.py
- apiserver/plane/bgtasks/notification_task.py
- apiserver/plane/bgtasks/export_task.py
- apiserver/plane/bgtasks/email_notification_task.py
- apiserver/plane/bgtasks/dummy_data_task.py
- apiserver/plane/bgtasks/issue_activities_task.py
- apiserver/plane/api/views/module.py
- apiserver/plane/api/views/issue.py
- ... [additional files listed in the shell script output]
Please update these usages to use
issue_objectsor consider redefining the default manager in theIssueclass if necessary.🔗 Analysis chain
Line range hint
118-118: Removal of defaultobjectsmanager may impact codebase.By not defining
objects = models.Manager()in theIssueclass, the default managerIssue.objectsis no longer available. This change could break any existing code that relies onIssue.objectsto query issues. Please verify all usages ofIssue.objectsacross the codebase and update them to useissue_objectsor consider redefining the default manager if necessary.To identify all occurrences where
Issue.objectsis used, run the following script:🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all usages of `Issue.objects` in the codebase. # Search for 'Issue.objects' in Python files. rg --type py --no-heading --line-number $'Issue\\.objects'Length of output: 15455
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- apiserver/plane/api/serializers/issue.py (2 hunks)
- apiserver/plane/app/serializers/draft.py (2 hunks)
- apiserver/plane/app/serializers/issue.py (2 hunks)
- apiserver/plane/db/mixins.py (1 hunks)
- apiserver/plane/db/models/issue.py (2 hunks)
- apiserver/plane/space/serializer/issue.py (2 hunks)
🧰 Additional context used
🔇 Additional comments (9)
apiserver/plane/api/serializers/issue.py (1)
215-215: Verify the impact of deletion changes on the codebase.While the changes to use
delete(soft=False)are correct and localized to theupdatemethod, it's important to ensure that this modification doesn't unintentionally affect other parts of the codebase that might rely on the hard deletion behavior ofIssueAssigneeandIssueLabelinstances.To confirm that these changes don't have unintended consequences, please run the following script:
This script will help identify any other locations in the codebase where
IssueAssigneeandIssueLabelinstances are being deleted. If any results are found, please review those occurrences to determine if they also need to be updated to usedelete(soft=False)for consistency.Also applies to: 232-232
✅ Verification successful
Deletion changes are localized and do not impact other parts of the codebase.
The verification scripts did not find any additional
delete()orbulk_delete()calls onIssueAssigneeorIssueLabeloutside ofapiserver/plane/api/serializers/issue.py. This confirms that the changes to usedelete(soft=False)are isolated to theupdatemethod ofIssueSerializerand do not affect other areas of the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other occurrences of delete() calls on IssueAssignee and IssueLabel # and verify if they need similar updates. echo "Checking for delete() calls on IssueAssignee:" rg --type python 'IssueAssignee.*delete\(\)' -g '!apiserver/plane/api/serializers/issue.py' echo "Checking for delete() calls on IssueLabel:" rg --type python 'IssueLabel.*delete\(\)' -g '!apiserver/plane/api/serializers/issue.py' echo "Checking for bulk_delete() calls on IssueAssignee:" rg --type python 'IssueAssignee.*bulk_delete\(\)' -g '!apiserver/plane/api/serializers/issue.py' echo "Checking for bulk_delete() calls on IssueLabel:" rg --type python 'IssueLabel.*bulk_delete\(\)' -g '!apiserver/plane/api/serializers/issue.py'Length of output: 915
Script:
#!/bin/bash # Description: Check for other occurrences of delete() and bulk_delete() calls on IssueAssignee and IssueLabel # and verify if they need similar updates. echo "Checking for delete() calls on IssueAssignee:" rg -t py 'IssueAssignee.*delete\(\)' -g '!apiserver/plane/api/serializers/issue.py' echo "Checking for delete() calls on IssueLabel:" rg -t py 'IssueLabel.*delete\(\)' -g '!apiserver/plane/api/serializers/issue.py' echo "Checking for bulk_delete() calls on IssueAssignee:" rg -t py 'IssueAssignee.*bulk_delete\(\)' -g '!apiserver/plane/api/serializers/issue.py' echo "Checking for bulk_delete() calls on IssueLabel:" rg -t py 'IssueLabel.*bulk_delete\(\)' -g '!apiserver/plane/api/serializers/issue.py'Length of output: 759
Script:
#!/bin/bash # Description: Broadly search for any delete() or bulk_delete() calls on IssueAssignee and IssueLabel # across the codebase, excluding the specified serializer file. echo "Searching for IssueAssignee.delete() calls:" rg -t py 'IssueAssignee\s*\.\s*delete\s*\(' -g '!apiserver/plane/api/serializers/issue.py' echo "Searching for IssueAssignee.bulk_delete() calls:" rg -t py 'IssueAssignee\s*\.\s*bulk_delete\s*\(' -g '!apiserver/plane/api/serializers/issue.py' echo "Searching for IssueLabel.delete() calls:" rg -t py 'IssueLabel\s*\.\s*delete\s*\(' -g '!apiserver/plane/api/serializers/issue.py' echo "Searching for IssueLabel.bulk_delete() calls:" rg -t py 'IssueLabel\s*\.\s*bulk_delete\s*\(' -g '!apiserver/plane/api/serializers/issue.py'Length of output: 771
apiserver/plane/app/serializers/issue.py (1)
Line range hint
1-1024: Ensure comprehensive testing for deletion behavior changesThe changes in this file are focused on modifying the deletion behavior in the
updatemethod ofIssueCreateSerializer. While these changes are minimal, they have a potentially significant impact on data management within the application.To ensure the changes don't introduce unintended consequences:
- Implement comprehensive unit tests for the
updatemethod, covering both the assignee and label deletion scenarios.- Perform integration tests to verify the cascading effects of these hard deletes on related parts of the system.
- Review and update any documentation related to issue management and data retention policies.
Run the following script to check for any other occurrences of soft delete in the codebase that might need similar updates:
This will help ensure consistency across the codebase and identify any other areas that might need attention following this change in deletion strategy.
✅ Verification successful
Run an updated search for soft delete instances in Python files
The previous script encountered an error due to an unrecognized file type. Please execute the following corrected script to search for other instances of soft delete in Python files:
No additional instances of soft delete found in Python files
The search did not find any other occurrences of
delete(soft=True)in Python files, confirming that the changes inIssueCreateSerializerare isolated and do not affect other parts of the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other instances of soft delete in Python files rg --type python 'delete\(\s*soft\s*=\s*True\s*\)' -g '!apiserver/plane/app/serializers/issue.py'Length of output: 130
Script:
#!/bin/bash # Search for other instances of soft delete in Python files excluding issue.py rg 'delete\(\s*soft\s*=\s*True\s*\)' -g '*.py' -g '!apiserver/plane/app/serializers/issue.py'Length of output: 95
apiserver/plane/db/mixins.py (2)
56-58: Ensure Custom QuerySet is Properly Utilized in ManagerThe
get_querysetmethod inSoftDeletionManagernow returns aSoftDeletionQuerySet. Ensure that all model managers intended to use soft deletion are usingSoftDeletionManagerto leverage the custom queryset behavior.
Line range hint
77-94: Potential Breaking Change: Modeldelete()Now Defaults to Soft DeletionIn
SoftDeleteModel, thedelete()method has been overridden withsoft=Trueby default. This alters the default behavior of model instances'delete()method, causing them to perform a soft delete unlesssoft=Falseis specified. Existing code that callsdelete()without thesoftparameter on models inheriting fromSoftDeleteModelwill now perform soft deletions instead of hard deletions. This could lead to unintended data retention and affect system behavior.To identify where
delete()is called withoutsoft=False, consider running the following script:#!/bin/bash # Description: Find all usages of `delete()` on instances of `SoftDeleteModel` that may be impacted. # Find all models inheriting from SoftDeleteModel rg --type=python -A5 'class .*SoftDeleteModel' | grep '^class' | awk '{print $2}' | cut -d'(' -f1 > soft_delete_models.txt # Search for delete() calls on instances of these models while read model; do # Look for delete() method calls on instances of the model rg --type=python -F "$model(" -A5 | rg '\.delete\(' done < soft_delete_models.txtThis script helps identify potential areas in the codebase where the change in default deletion behavior might cause issues.
apiserver/plane/app/serializers/draft.py (2)
172-174: Confirm the use of thesoftparameter in thedelete()methodThe
delete()method is being called withsoft=False:DraftIssueAssignee.objects.filter(draft_issue=instance).delete( soft=False )Please verify that the
delete()method forDraftIssueAssigneesupports thesoftkeyword argument. By default, Django'sdelete()method does not accept any parameters. If you have implemented a custom manager or queryset method that allows for this parameter to handle soft deletions, this is acceptable. Otherwise, this might raise an error.To confirm whether the
delete()method supports thesoftparameter, run the following script:
191-193: Confirm the use of thesoftparameter in thedelete()methodThe
delete()method is being called withsoft=False:DraftIssueLabel.objects.filter(draft_issue=instance).delete( soft=False )Please verify that the
delete()method forDraftIssueLabelaccepts thesoftkeyword argument. In Django's ORM, the standarddelete()method does not take any parameters. If you have a custom implementation that supports this parameter for soft deletion purposes, then this is appropriate. Otherwise, it may lead to errors during execution.To check if the
delete()method supports thesoftparameter, run the following script:✅ Verification successful
Confirmed: The
delete()method supports thesoftparameterThe
delete()method in theDraftIssueLabelmodel utilizes a custom manager and queryset that accept thesoftparameter for soft deletion.
- Location:
apiserver/plane/db/mixins.py- Implementation:
SoftDeletionManagerandSoftDeletionQuerySethandle thesoftparameter appropriately.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if the `delete` method in `DraftIssueLabel` supports the `soft` parameter. # Search for custom `delete` methods in `DraftIssueLabel` model or its manager. rg --type py --word-regex 'def delete\(' -A 5 -g '**/models/*.py' 'class.*DraftIssueLabel' # Search for custom managers or querysets associated with `DraftIssueLabel`. rg --type py 'objects = .*Manager\(\)' -A 5 -g '**/models/*.py' 'class.*DraftIssueLabel'Length of output: 785
Script:
#!/bin/bash # Description: Verify if the `delete` method in `DraftIssueLabel` supports the `soft` parameter. # Corrected search for custom `delete` methods in `DraftIssueLabel` model or its manager. rg --type py --word-regexp 'def delete\(' -A 5 -g 'models/*.py' 'class\s+DraftIssueLabel' # Corrected search for custom managers or querysets associated with `DraftIssueLabel`. rg --type py 'objects\s*=\s*.*Manager\(\)' -A 5 -g 'models/*.py' 'class\s+DraftIssueLabel'Length of output: 312
Script:
#!/bin/bash # Description: Locate the `DraftIssueLabel` class and verify if the `delete` method supports the `soft` parameter. # Step 1: Find all files containing the `DraftIssueLabel` class definition. rg --type py 'class\s+DraftIssueLabel' --color=never # Step 2: If the class is found, search for a custom `delete` method within those files. rg --type py 'def\s+delete\s*\(' --context 5 -g '*/DraftIssueLabel*.py' # Step 3: Search for custom managers or querysets associated with `DraftIssueLabel`. rg --type py 'objects\s*=\s*.*Manager\(\)' --context 5Length of output: 2224
Script:
#!/bin/bash # Description: Verify if the `SoftDeletionManager` class's `delete` method supports the `soft` parameter. # Step 1: Find the file containing the `SoftDeletionManager` class definition. rg --type py 'class\s+SoftDeletionManager' --color=never # Step 2: Once the file is identified (assuming it's in apiserver/plane/db/mixins.py), search for the `delete` method within this class. # Replace <path_to_file> with the actual path from Step 1. rg --type py 'def\s+delete\s*\(' -C 5 'class\s+SoftDeletionManager' <path_to_file> # Step 3: Specifically check if the `delete` method includes the `soft` parameter. rg --type py 'def\s+delete\s*\(.*soft\s*=' -C 2 'class\s+SoftDeletionManager' <path_to_file>Length of output: 356
Script:
#!/bin/bash # Description: Verify if the `SoftDeletionManager` class's `delete` method supports the `soft` parameter. # Step 1: Find the file containing the `SoftDeletionManager` class definition. rg --type py 'class\s+SoftDeletionManager' --color=never # Step 2: Once the file is identified, search for the `delete` method within this class. rg --type py 'def\s+delete\s*\(' -C 5 'class\s+SoftDeletionManager' apiserver/plane/db/mixins.py # Step 3: Specifically check if the `delete` method includes the `soft` parameter. rg --type py 'def\s+delete\s*\(.*soft\s*=' -C 2 'class\s+SoftDeletionManager' apiserver/plane/db/mixins.pyLength of output: 2243
apiserver/plane/db/models/issue.py (3)
15-15: ImportingSoftDeletionManagerenables soft deletion functionality.The addition of
from plane.db.mixins import SoftDeletionManageris appropriate for integrating soft deletion capabilities into theIssueManager.
Line range hint
83-96: UpdatingIssueManagerto useSoftDeletionManagerfor soft deletion.Changing the inheritance of
IssueManagerfrommodels.ManagertoSoftDeletionManagercorrectly incorporates soft deletion handling into the issue management. The overriddenget_querysetmethod will now appropriately include soft-deleted records as per the new manager's logic.
Line range hint
83-96: Ensureget_querysetaligns with soft deletion logic.Since
SoftDeletionManagerhandles the exclusion of soft-deleted records, double-check that the custom filters inget_querysetdo not conflict with or override the soft deletion behavior. This ensures that the queryset returns the correct set of issues, including or excluding soft-deleted ones as intended.Consider reviewing and updating any filters that may inadvertently include soft-deleted records.
| @@ -421,7 +421,7 @@ def update(self, instance, validated_data): | |||
| updated_by_id = instance.updated_by_id | |||
|
|
|||
| if assignees is not None: | |||
| IssueAssignee.objects.filter(issue=instance).delete() | |||
| IssueAssignee.objects.filter(issue=instance).delete(soft=False) | |||
There was a problem hiding this comment.
💡 Codebase verification
Soft delete implementation is inconsistent or missing in IssueAssignee and IssueLabel models
Our analysis reveals that the IssueAssignee and IssueLabel models lack fields typically associated with soft deletes (e.g., is_deleted, deleted_at). Despite using delete(soft=False) in serializers and views, the absence of these fields means that soft delete functionality may not be effectively implemented, leading to potential data loss.
Consider the following points:
- Missing Soft Delete Fields: Without fields like
is_deletedordeleted_at, the models cannot properly support soft deletion. - Hard Delete Usage: The use of
delete(soft=False)across various parts of the codebase bypasses any soft delete logic, resulting in permanent deletions. - Inconsistent Deletion Behavior: This approach contradicts the PR's objective of implementing soft deletes and can lead to data integrity issues.
🔗 Analysis chain
Reconsider the use of hard delete for related entities
The changes to use delete(soft=False) for IssueAssignee and IssueLabel instances contradict the PR objective of implementing soft delete. This approach may lead to unexpected data loss and inconsistency in deletion behavior across the application.
Consider the following points:
- Data Integrity: Permanently deleting related records during updates can lead to loss of historical data and potential integrity issues.
- Consistency: The use of hard delete here may be inconsistent with soft delete implementations elsewhere in the application.
- Recoverability: Soft deletes allow for potential data recovery, which is lost with hard deletes.
To align with the PR objectives and maintain data integrity, consider the following changes:
- IssueAssignee.objects.filter(issue=instance).delete(soft=False)
+ IssueAssignee.objects.filter(issue=instance).delete() # This will use the default soft delete
- IssueLabel.objects.filter(issue=instance).delete(soft=False)
+ IssueLabel.objects.filter(issue=instance).delete() # This will use the default soft deleteAdditionally, ensure that the queryset used to fetch these related entities is updated to exclude soft-deleted records, if not already implemented.
To confirm the current soft delete implementation, please run the following script:
This script will help us understand the current soft delete implementation and ensure that our changes are consistent with the existing codebase.
Also applies to: 441-441
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the soft delete implementation for IssueAssignee and IssueLabel models
# Check for a soft delete field (e.g., is_deleted, deleted_at) in the models
echo "Checking for soft delete fields in IssueAssignee and IssueLabel models:"
rg -A 5 "class (IssueAssignee|IssueLabel)\(" apiserver/plane/db/models/
# Check for custom manager implementation that might handle soft deletes
echo "\nChecking for custom manager implementations:"
rg -A 10 "class (IssueAssignee|IssueLabel)Manager\(" apiserver/plane/db/models/
# Check for any existing soft delete logic in views or serializers
echo "\nChecking for existing soft delete logic:"
rg "delete\(.*soft" apiserver/plane/
Length of output: 2863
| @@ -201,7 +201,7 @@ def update(self, instance, validated_data): | |||
| updated_by_id = instance.updated_by_id | |||
|
|
|||
| if assignees is not None: | |||
| IssueAssignee.objects.filter(issue=instance).delete() | |||
| IssueAssignee.objects.filter(issue=instance).delete(soft=False) | |||
There was a problem hiding this comment.
Consider the implications of switching to hard delete
The changes from delete() to delete(soft=False) for IssueAssignee and IssueLabel represent a shift from soft deletion to hard deletion. This modification has several important implications:
- Data Permanence: Hard deletion will permanently remove the data, which may not be recoverable.
- Audit Trail: It might affect the ability to maintain a complete audit trail of issue changes.
- Performance: Hard deletes might be faster and reduce database bloat, but at the cost of data retention.
Consider the following:
- Ensure this change aligns with the overall data management strategy of the application.
- Update any dependent code that might rely on soft-deleted records still being present.
- Verify that this change doesn't conflict with any data retention policies or regulatory requirements.
- Consider adding a confirmation step or logging mechanism for these hard deletes to maintain traceability.
Would you like assistance in implementing any of these suggestions?
Also applies to: 221-221
| class SoftDeletionQuerySet(models.QuerySet): | ||
| def delete(self, soft=True): | ||
| if soft: | ||
| return self.update(deleted_at=timezone.now()) | ||
| else: | ||
| return super().delete() |
There was a problem hiding this comment.
💡 Codebase verification
Potential Breaking Change Confirmed: Multiple delete() Calls Default to Soft Deletion
The analysis identified numerous instances where delete() is called without specifying soft=False. This means these deletions will now perform soft deletions by default, potentially leading to unintended behavior and accumulation of soft-deleted records.
Affected Files:
apiserver/plane/utils/cache.pyapiserver/plane/tests/api/test_authentication.pyapiserver/plane/space/views/inbox.pyapiserver/plane/space/views/issue.pyapiserver/plane/db/mixins.pyapiserver/plane/license/api/views/admin.pyapiserver/plane/db/migrations/0077_draftissue_cycle_user_timezone_project_user_timezone_and_more.pyapiserver/plane/bgtasks/apiserver/plane/api/views/apiserver/plane/app/views/apiserver/plane/authentication/utils/workspace_project_join.pyapiserver/plane/authentication/provider/credentials/magic_code.py
Please review these deletions to ensure they align with the intended behavior.
🔗 Analysis chain
Potential Breaking Change: QuerySet.delete() Now Defaults to Soft Deletion
The SoftDeletionQuerySet class overrides the delete() method with soft=True by default. This changes the behavior of QuerySet.delete(), so it performs a soft delete unless soft=False is explicitly provided. Existing code that relies on hard deletion through delete() may now perform soft deletions unintentionally, potentially leading to unexpected results or accumulation of soft-deleted records.
To verify where delete() is called without specifying soft=False, consider running the following script:
This script searches for all occurrences of delete() calls on QuerySets, helping to identify places where the default behavior change may impact the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of `delete()` on QuerySets that may be affected.
# Search for `.delete(` calls in Python files.
rg --type=python '\.delete\('
Length of output: 143
Script:
#!/bin/bash
# Description: Find all usages of `delete()` on QuerySets in Python files.
# Search for `.delete(` calls in all `.py` files.
rg '\.delete\(' --type py
Length of output: 10502
Script:
#!/bin/bash
# Description: Find all usages of `delete()` on QuerySets in Python files that do NOT specify `soft=False`.
# Search for `.delete(` calls not followed by `soft=False`.
rg '\.delete\((?!.*soft\s*=\s*False)' --type py
Length of output: 295
Script:
#!/bin/bash
# Description: Find all usages of `delete()` on QuerySets in Python files that do NOT specify `soft=False`.
# Search for `.delete(` calls in all `.py` files and exclude lines with `soft=False`.
rg '\.delete\(' --type py | grep -v 'soft\s*=\s*False'
Length of output: 9254
pushya22
changed the title
chore:
Summary by CodeRabbit
New Features
description_html, in the draft issue details for improved representation.Bug Fixes
Refactor