-
Notifications
You must be signed in to change notification settings - Fork 0
feat: added soft delete functionality #10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements soft delete functionality for discussion threads, responses, and comments using an is_deleted flag. Instead of permanently removing records, content is marked as deleted and can be restored later. The implementation includes deleted content tracking in user statistics, filtering capabilities, and bulk restore operations for privileged users.
Changes:
- Added soft delete fields (
is_deleted,deleted_at,deleted_by) to threads and comments with database migration - Implemented soft delete and restore operations across MySQL and MongoDB backends with stat tracking
- Added API endpoints to view and restore deleted content, plus filtering support in search and user queries
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 23 comments.
Show a summary per file
| File | Description |
|---|---|
| forum/init.py | Version bumped to 0.4.0 |
| forum/migrations/0006_comment_deleted_at_comment_deleted_by_and_more.py | Database migration adding soft delete fields to Comment and CommentThread models |
| forum/backends/mysql/models.py | Added soft delete fields and updated comment_count to exclude deleted comments |
| forum/backends/mysql/api.py | Implemented soft delete/restore logic, stat tracking, and deleted content queries for MySQL |
| forum/backends/mongodb/threads.py | Added soft delete/restore methods and deleted content queries for threads |
| forum/backends/mongodb/comments.py | Modified delete method to support soft delete mode and added restore functionality |
| forum/backends/mongodb/api.py | Implemented soft delete orchestration, stat updates, and deleted content filtering |
| forum/backends/backend.py | Added abstract methods for deleted content retrieval |
| forum/serializers/contents.py | Added serializer fields for soft delete metadata |
| forum/api/threads.py | Updated delete_thread to use soft delete and added restore endpoints |
| forum/api/comments.py | Updated delete_comment to use soft delete and added restore endpoints |
| forum/api/users.py | Added show_deleted parameter for viewing deleted threads in user stats |
| forum/api/search.py | Added is_deleted parameter to search functionality |
| forum/api/init.py | Exported new deleted content retrieval functions |
| forum/views/comments.py | Updated documentation for delete endpoint |
| tests/test_views/test_threads.py | Updated assertions to check for soft delete instead of hard delete |
| tests/test_views/test_comments.py | Updated assertions and added documentation about soft delete behavior |
| tests/test_backends/test_mongodb/test_comments.py | Updated expected return values for delete method |
| tests/e2e/test_users.py | Added deleted content stats and updated test assertions |
Comments suppressed due to low confidence (1)
forum/api/init.py:95
- The restore_comment, restore_user_deleted_comments, restore_thread, and restore_user_deleted_threads functions are imported but not included in the all list. This means they won't be exported when someone does "from forum.api import *". Either add them to all or remove the unused imports if they're meant to be internal-only functions.
__all__ = [
"create_child_comment",
"create_parent_comment",
"create_subscription",
"create_thread",
"create_user",
"delete_comment",
"delete_comment_vote",
"delete_subscription",
"delete_thread",
"delete_thread_vote",
"get_commentables_stats",
"get_course_id_by_comment",
"get_course_id_by_thread",
"get_parent_comment",
"get_thread",
"get_thread_subscriptions",
"get_user",
"get_user_active_threads",
"get_user_comments",
"get_user_course_stats",
"get_user_subscriptions",
"get_user_threads",
"get_deleted_comments_for_course",
"get_deleted_threads_for_course",
"mark_thread_as_read",
"pin_thread",
"retire_user",
"search_threads",
"unpin_thread",
"update_comment",
"update_comment_flag",
"update_comment_votes",
"update_thread",
"update_thread_flag",
"update_thread_votes",
"update_user",
"update_username",
"update_users_in_course",
]
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Update existing fields and add new fields if they don't exist | ||
| for k, v in kwargs.items(): | ||
| course_stat[k] = course_stat.get(k, 0) + v |
Copilot
AI
Jan 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The update_stats_for_course method in MongoDB has been modified to add new fields to course_stats if they don't exist (line 44: course_stat.get(k, 0)). However, this means that for legacy users with existing course_stats that don't have deleted_threads, deleted_responses, or deleted_replies fields, these will be initialized to 0 on the first update. This is correct, but consider whether a data migration is needed to ensure all existing course_stats have these fields set to 0 explicitly for consistency.
|
|
||
| # IMPORTANT: Restore replies (children) FIRST, then responses (parents) | ||
| # This prevents double-counting replies when both parent and children are restored | ||
|
|
||
| # First, restore all replies (comments with a parent) | ||
| replies = [c for c in deleted_comments if c.parent is not None] | ||
| for comment in replies: | ||
| if cls.restore_comment(str(comment.pk), restored_by=restored_by): | ||
| count += 1 | ||
|
|
||
| # Then, restore all responses (comments without a parent) | ||
| responses = [c for c in deleted_comments if c.parent is None] | ||
| for comment in responses: | ||
| if cls.restore_comment(str(comment.pk), restored_by=restored_by): | ||
| count += 1 | ||
|
|
||
| return count |
Copilot
AI
Jan 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The restore_user_deleted_comments method processes replies first, then responses. However, the comment states this prevents double-counting when both parent and children are restored. The logic here seems flawed: if a child reply is restored first (incrementing replies by 1), and then its parent response is restored, the parent restoration will count the child as "still deleted" (which it's not anymore after the first restore). This could lead to incorrect stats. Consider restoring parents first, or tracking which children have already been restored.
| backend = get_backend(course_id)() | ||
| sort_criterion = backend.get_user_sort_criterion(sort_key) | ||
| exclude_from_stats = ["_id", "course_id"] | ||
| exclude_from_stats = ["_id", "course_id", "deleted_count"] |
Copilot
AI
Jan 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The exclude_from_stats list now includes "deleted_count" which means this computed field won't be returned in the user stats API. However, deleted_count is computed in the to_dict method (line 88-90 in models.py) and appears to be a useful stat. Consider whether this exclusion is intentional or if deleted_count should be exposed to API consumers.
| exclude_from_stats = ["_id", "course_id", "deleted_count"] | |
| exclude_from_stats = ["_id", "course_id"] |
| # Hard delete: permanently remove | ||
| result = self._collection.delete_one({"_id": ObjectId(_id)}) | ||
| result_count = result.deleted_count | ||
| if mode == "hard": |
Copilot
AI
Jan 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the MongoDB delete method, there's duplicate logic checking mode == "hard" on line 302 and 289. The check on line 302 for updating parent child_count only happens in hard mode, but this logic block could be consolidated with the hard delete block above it (lines 299-301) to reduce code duplication and improve readability.
| if mode == "hard": |
| # Add soft delete filtering | ||
| kwargs["is_deleted"] = {"$ne": True} | ||
| contents = list(Contents().get_list(**kwargs)) | ||
|
|
||
| # Get all thread IDs mentioned in comments | ||
| comment_thread_ids = set() | ||
| for content in contents: | ||
| if content.get("_type") == "Comment" and content.get("comment_thread_id"): | ||
| comment_thread_ids.add(content["comment_thread_id"]) | ||
|
|
Copilot
AI
Jan 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The get_contents method now adds "is_deleted": {"$ne": True} filtering to exclude soft deleted content. However, it also collects comment_thread_ids but doesn't use them (lines 1873-1876). This unused variable and logic should be removed or the filtering should be completed to also exclude comments whose threads are deleted.
| # Add soft delete filtering | |
| kwargs["is_deleted"] = {"$ne": True} | |
| contents = list(Contents().get_list(**kwargs)) | |
| # Get all thread IDs mentioned in comments | |
| comment_thread_ids = set() | |
| for content in contents: | |
| if content.get("_type") == "Comment" and content.get("comment_thread_id"): | |
| comment_thread_ids.add(content["comment_thread_id"]) | |
| # Add soft delete filtering for contents themselves | |
| kwargs["is_deleted"] = {"$ne": True} | |
| contents = list(Contents().get_list(**kwargs)) | |
| # Exclude comments whose threads have been soft-deleted | |
| comment_thread_ids: set[Any] = set() | |
| for content in contents: | |
| if content.get("_type") == "Comment" and content.get("comment_thread_id"): | |
| comment_thread_ids.add(content["comment_thread_id"]) | |
| if comment_thread_ids: | |
| deleted_threads = CommentThread().get_list( | |
| _id={"$in": list(comment_thread_ids)}, | |
| is_deleted=True, | |
| ) | |
| deleted_thread_ids = {thread["_id"] for thread in deleted_threads} | |
| if deleted_thread_ids: | |
| contents = [ | |
| content | |
| for content in contents | |
| if not ( | |
| content.get("_type") == "Comment" | |
| and content.get("comment_thread_id") in deleted_thread_ids | |
| ) | |
| ] |
| def get_thread(thread_id: str) -> dict[str, Any] | None: | ||
| """Get thread from id.""" | ||
| thread = CommentThread().get(thread_id) |
Copilot
AI
Jan 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The get_thread method in MongoDB API no longer checks if thread is None before returning it. This means None will be returned for non-existent threads, which is correct, but the removed check was present for a reason. Verify that all callers of this method handle None correctly, especially since this is a behavior change.
| def get_thread(thread_id: str) -> dict[str, Any] | None: | |
| """Get thread from id.""" | |
| thread = CommentThread().get(thread_id) | |
| def get_thread(thread_id: str) -> dict[str, Any]: | |
| """Get thread from id.""" | |
| thread = CommentThread().get(thread_id) | |
| if thread is None: | |
| raise ObjectDoesNotExist(f"Thread with id {thread_id} does not exist") |
| ) | ||
|
|
||
| return result.modified_count | ||
|
|
Copilot
AI
Jan 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The delete method signature has changed but the type annotation shows 'type: ignore[override]'. This indicates the method signature doesn't match the parent class. Consider updating the parent class method signature in the abstract base to match, or document why the override is necessary.
| # NOTE: | |
| # The signature of this method intentionally differs from BaseContents.delete. | |
| # Comments support both soft and hard deletion as well as cascading deletes of | |
| # child comments, so we need extra parameters (mode, deleted_by) and a tuple | |
| # return value (parent and child deletion counts). The abstract base cannot be | |
| # broadened without impacting other backends, so we keep this override and | |
| # explicitly ignore the type-checker override warning. |
| # Count deleted content | ||
| deleted_threads = threads.filter(is_deleted=True).count() | ||
| deleted_responses = responses.filter(is_deleted=True).count() | ||
| deleted_replies = replies.filter(is_deleted=True).count() | ||
|
|
||
| stats, _ = CourseStat.objects.get_or_create(user=author, course_id=course_id) | ||
| stats.threads = threads.count() | ||
| stats.responses = responses.count() | ||
| stats.replies = replies.count() | ||
| stats.threads = threads.count() - deleted_threads | ||
| stats.responses = responses.count() - deleted_responses | ||
| stats.replies = replies.count() - deleted_replies | ||
| stats.deleted_threads = deleted_threads | ||
| stats.deleted_responses = deleted_responses | ||
| stats.deleted_replies = deleted_replies |
Copilot
AI
Jan 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The build_course_stats method rebuilds stats by counting all content (including deleted) and then subtracting deleted counts. This approach is correct for rebuilding stats from scratch. However, the stats could become out of sync if content is directly modified in the database without going through the API layer. Consider adding database constraints or triggers to maintain consistency, or document that stats should be periodically rebuilt.
| # Update stats based on what was actually deleted | ||
| if responses_deleted > 0: | ||
| # A response (parent comment) was deleted | ||
| backend.update_stats_for_course( | ||
| author_id, | ||
| comment_course_id, | ||
| responses=-responses_deleted, | ||
| deleted_responses=responses_deleted, | ||
| replies=-replies_deleted, | ||
| deleted_replies=replies_deleted, | ||
| ) | ||
| else: | ||
| backend.update_stats_for_course(author_id, comment_course_id, responses=-1) | ||
| # Only a reply was deleted (no response) | ||
| backend.update_stats_for_course( | ||
| author_id, | ||
| comment_course_id, | ||
| replies=-replies_deleted, | ||
| deleted_replies=replies_deleted, | ||
| ) |
Copilot
AI
Jan 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The delete_comment function updates stats for all comments regardless of whether they are anonymous. However, in the restore_comment and delete_thread functions, stats are only updated when content is not anonymous (checking anonymous or anonymous_to_peers flags). This inconsistency means that deleting anonymous content will incorrectly update stats, but restoring it won't restore those stats, leading to stat drift. The delete_comment function should check if the comment is anonymous before updating stats.
| comment_id: The ID of the comment to be deleted. | ||
| Body: | ||
| Empty. | ||
| deleted_by: Optional ID of the user performing the delete (defaults to authenticated user). |
Copilot
AI
Jan 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation on line 145 states that deleted_by is an optional parameter in the request body, but there's no visible code change to extract this parameter from the request and pass it to the delete_comment function. This means the deleted_by tracking feature won't work through the API unless the view implementation is updated.
Description
Implements soft delete functionality for discussion threads, responses, and comments using the
is_deletedflag instead of permanently deleting records.This enables safe deletion and restoration of discussion content while preserving existing data.
Changes Made
JIRA Tickets
Related Pull Requests
Merge checklist:
Check off if complete or not applicable: