Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements soft delete functionality for discussion threads, responses, and comments. Users with appropriate permissions (Staff, Admins, Moderators, TAs) can now delete content without permanently removing it from the database, and can restore previously deleted content. The implementation adds a new Active/Deleted filter in the Learner tab, displays deleted status indicators, and supports bulk delete/restore operations.
Changes:
- Added soft delete and restore actions for threads, responses, and comments with confirmation dialogs
- Implemented Active/Deleted content filtering with dedicated API endpoints for deleted content
- Added UI indicators for deleted content including badges, "Deleted by" information, and visual styling (opacity, line-through text)
Reviewed changes
Copilot reviewed 46 out of 47 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/index.scss | Added styling for deleted content indicators, learner message displays, and submenu containers |
| src/discussions/utils.js | Added restore action to actions list with permissions checking and disabled state logic |
| src/discussions/posts/post/messages.js | Added i18n messages for deleted badges and restore confirmations |
| src/discussions/posts/post/PostLink.jsx | Added deleted badge display, navigation logic for responses/comments, and visual indicators |
| src/discussions/posts/post/Post.jsx | Added restore confirmation dialog and "deleted by" information display |
| src/discussions/posts/post-filter-bar/messages.js | Added filter messages for active and deleted content |
| src/discussions/posts/index.js | Exported PostLink component |
| src/discussions/posts/data/thunks.js | Added performRestoreThread thunk and API filtering for deleted content |
| src/discussions/posts/data/slices.js | Changed default filter to ACTIVE, added deleted view state management |
| src/discussions/posts/data/selectors.js | Added selector for deleted view state |
| src/discussions/posts/data/api.js | Added restoreThread API function and isDeleted parameter support |
| src/discussions/posts/data/factories/threads.factory.js | Added is_deleted field to factory |
| src/discussions/posts/PostsView.test.jsx | Updated test assertions for new filter defaults |
| src/discussions/posts/NoResults.jsx | Fixed learners filter null check |
| src/discussions/post-comments/messages.js | Added i18n messages for deleted comments/responses |
| src/discussions/post-comments/data/thunks.js | Added performRestoreComment thunk and showDeleted parameter support |
| src/discussions/post-comments/data/hooks.js | Added useShowDeletedContent hook and showDeleted parameter passing |
| src/discussions/post-comments/data/api.js | Added restoreComment API and showDeleted parameter support |
| src/discussions/post-comments/data/factories/comments.factory.js | Added is_deleted field to factory |
| src/discussions/post-comments/comments/comment/Reply.jsx | Added restore confirmation and "deleted by" display for replies |
| src/discussions/post-comments/comments/comment/CommentHeader.jsx | Added flex-wrap to prevent layout issues |
| src/discussions/post-comments/comments/comment/Comment.jsx | Added restore functionality, deleted content display, and showDeleted parameter |
| src/discussions/post-comments/PostCommentsView.test.jsx | Updated test to expect show_deleted parameter |
| src/discussions/messages.js | Added restore action and bulk operation messages |
| src/discussions/learners/utils.js | Added restore actions and useLearnerActionsMenu hook for submenu support |
| src/discussions/learners/messages.js | Added messages for deleted activity, restore actions, and confirmation dialogs |
| src/discussions/learners/learner/proptypes.js | Added deleted count properties to learner shape |
| src/discussions/learners/learner/LearnerFooter.jsx | Added deleted activity count display with icon |
| src/discussions/learners/learner/LearnerFilterBar.jsx | Added deleted activity sorting option |
| src/discussions/learners/learner/LearnerCard.jsx | Passed deleted count props to footer |
| src/discussions/learners/learner-post-filter-bar/LearnerPostFilterBar.test.jsx | Updated test assertions for new radiogroup count |
| src/discussions/learners/learner-post-filter-bar/LearnerPostFilterBar.jsx | Added contentStatus filter for active/deleted content |
| src/discussions/learners/data/thunks.js | Added undeleteUserPosts thunk and getDeletedContent API usage |
| src/discussions/learners/data/slices.js | Added contentStatus to postFilter and undelete action reducers |
| src/discussions/learners/data/redux.test.jsx | Updated test to check contentStatus default value |
| src/discussions/learners/data/api.js | Added getDeletedContent and restoreUserPostsApi functions |
| src/discussions/learners/LearnerPostsView.test.jsx | Added mouseEnter events for submenu testing |
| src/discussions/learners/LearnerPostsView.jsx | Added restore confirmation dialog and navigation handling |
| src/discussions/learners/LearnerActionsDropdown.test.jsx | Updated tests for new submenu interaction |
| src/discussions/learners/LearnerActionsDropdown.jsx | Implemented submenu for delete/restore actions using portal |
| src/discussions/data/selectors.js | Updated filter check to treat ACTIVE same as ALL |
| src/discussions/data/constants.js | Added THREAD_FILTER_TYPES constant (unused) |
| src/discussions/common/HoverCard.jsx | Disabled interaction buttons for deleted content |
| src/discussions/common/ActionsDropdown.jsx | Added disabled state handling for actions |
| src/data/constants.js | Added RESTORE and bulk restore action constants |
| src/components/FilterBar.jsx | Added support for separated filter sections with active/deleted options |
| src/assets/undelete.svg | Added restore icon SVG |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| logError(error); | ||
| } | ||
| hideRestoreConfirmation(); | ||
| }, [id, courseId, threadId, dispatch, hideRestoreConfirmation]); |
There was a problem hiding this comment.
The dependency array includes threadId but it's not used in the callback function. This could cause unnecessary re-renders. Remove threadId from the dependency array.
| }, [id, courseId, threadId, dispatch, hideRestoreConfirmation]); | |
| }, [id, courseId, dispatch, hideRestoreConfirmation]); |
| logError(error); | ||
| } | ||
| hideRestoreConfirmation(); | ||
| }, [id, courseId, threadId, dispatch, hideRestoreConfirmation]); |
There was a problem hiding this comment.
The dependency array includes threadId but it's not used in the callback function. This could cause unnecessary re-renders. Remove threadId from the dependency array.
| }, [id, courseId, threadId, dispatch, hideRestoreConfirmation]); | |
| }, [id, courseId, dispatch, hideRestoreConfirmation]); |
| // Navigate back to learners list after deletion | ||
| navigate({ ...discussionsPath(Routes.LEARNERS.PATH, { courseId })(location) }); | ||
| } | ||
| }, [courseId, username, hideDeleteConfirmation, dispatchDelete, navigate, location, postId, dispatch, loadMorePosts]); |
There was a problem hiding this comment.
The dependency array includes dispatchDelete which is created by useDispatchWithState hook and may change on every render. This could cause unnecessary re-renders or stale closures. Consider using useCallback for dispatchDelete or restructure the dependencies.
|
|
||
| export const THREAD_FILTER_TYPES = { | ||
| ACTIVE: 'active', | ||
| DELETED: 'deleted', | ||
| }; |
There was a problem hiding this comment.
The THREAD_FILTER_TYPES constant is defined but never used in the codebase. The code uses PostsStatusFilter.ACTIVE and PostsStatusFilter.DELETED instead. Consider removing this unused constant or updating the code to use it consistently.
| export const THREAD_FILTER_TYPES = { | |
| ACTIVE: 'active', | |
| DELETED: 'deleted', | |
| }; |
| <LearnerAvatar username={username} /> | ||
| <div className="d-flex flex-column flex-fill" style={{ minWidth: 0 }}> | ||
| <div className="d-flex flex-column justify-content-start mw-100 flex-fill"> | ||
| <div className="d-flex flex-column flex-fill" style={{ overflow: 'visible' }}> |
There was a problem hiding this comment.
The change from minWidth: 0 to overflow: 'visible' might cause layout issues with text overflow or clipping. Ensure this change is intentional and doesn't break existing text truncation behavior.
| <div className="d-flex flex-column flex-fill" style={{ overflow: 'visible' }}> | |
| <div className="d-flex flex-column flex-fill" style={{ minWidth: 0 }}> |
| postStatus: RequestStatus.SUCCESSFUL, | ||
| filters: { | ||
| status: PostsStatusFilter.ALL, | ||
| status: PostsStatusFilter.ACTIVE, |
There was a problem hiding this comment.
Changing the default filter from ALL to ACTIVE is a breaking change that alters the default view for all users. This could be surprising to existing users who expect to see all posts by default. Consider whether this should be behind a feature flag or communicated as a breaking change.
| contentType, | ||
| } = {}) { | ||
| const params = snakeCaseObject({ | ||
| authorId: author, // The backend expects author_id |
There was a problem hiding this comment.
The parameter mapping is inconsistent - the function parameter is named author but the API expects authorId. Consider renaming the function parameter to authorId for clarity and consistency with the API contract.
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