Conversation
1d1ac54 to
8711640
Compare
d997095 to
fd9406c
Compare
There was a problem hiding this comment.
Pull request overview
This PR implements comprehensive soft delete functionality for discussion threads, responses, and comments, allowing content to be marked as deleted rather than permanently removed from the database. The implementation includes UI indicators for deleted content, restore capabilities, and bulk operations for managing user contributions.
Key Changes:
- Added soft delete and restore actions with confirmation dialogs for threads, responses, and comments
- Implemented filtering between active and deleted content with dedicated views for moderators/staff
- Added bulk delete and restore operations for user posts at course and organization levels
Reviewed changes
Copilot reviewed 47 out of 48 changed files in this pull request and generated 26 comments.
Show a summary per file
| File | Description |
|---|---|
src/index.scss |
Added styling for deleted content indicators, learner message badges, and submenu containers |
src/discussions/utils.js |
Extended action permissions to support restore operations and disabled actions for deleted items |
src/discussions/posts/post/messages.js |
Added internationalized messages for delete/restore confirmations and deleted content badges |
src/discussions/posts/post/PostLink.jsx |
Enhanced post links to display deleted state with visual indicators and handle response/comment navigation |
src/discussions/posts/post/Post.jsx |
Integrated restore confirmation dialogs and deleted-by attribution display |
src/discussions/posts/post-filter-bar/messages.js |
Added filter labels for active vs deleted content status |
src/discussions/posts/index.js |
Exported PostLink component for reuse in learner views |
src/discussions/posts/data/thunks.js |
Implemented thread restore thunk and filter logic for active/deleted content |
src/discussions/posts/data/slices.js |
Changed default filter to active content and added deleted view state management |
src/discussions/posts/data/selectors.js |
Added selector for deleted view state |
src/discussions/posts/data/api.js |
Created API endpoint for restoring deleted threads |
src/discussions/posts/data/__factories__/threads.factory.js |
Added is_deleted field to thread factory for testing |
src/discussions/posts/PostsView.test.jsx |
Updated tests to reflect new default "active" filter |
src/discussions/posts/NoResults.jsx |
Fixed optional chaining for learner state access |
src/discussions/post-comments/messages.js |
Added restore confirmation messages for responses and comments |
src/discussions/post-comments/data/thunks.js |
Extended fetch operations to support showDeleted parameter and added restore thunk |
src/discussions/post-comments/data/hooks.js |
Implemented deleted content visibility logic based on filter state |
src/discussions/post-comments/data/api.js |
Added showDeleted parameter to API calls and restore comment endpoint |
src/discussions/post-comments/data/__factories__/comments.factory.js |
Added is_deleted field to comment factory |
src/discussions/post-comments/comments/comment/Reply.jsx |
Integrated restore functionality and deleted-by attribution for replies |
src/discussions/post-comments/comments/comment/CommentHeader.jsx |
Added flex-wrap to header for better responsive layout |
src/discussions/post-comments/comments/comment/Comment.jsx |
Implemented restore logic and deleted state handling for comments and responses |
src/discussions/post-comments/PostCommentsView.test.jsx |
Updated test expectations for show_deleted parameter |
src/discussions/messages.js |
Added restore action message and bulk operation labels |
src/discussions/learners/utils.js |
Created learner actions menu with nested delete/restore submenus |
src/discussions/learners/messages.js |
Added messages for deleted activity stats and restore operations |
src/discussions/learners/learner/proptypes.js |
Extended learner shape with deleted content counts |
src/discussions/learners/learner/LearnerFooter.jsx |
Added deleted activity indicator for staff/moderators |
src/discussions/learners/learner/LearnerFilterBar.jsx |
Added deleted activity sorting option |
src/discussions/learners/learner/LearnerCard.jsx |
Passed deleted stats to footer component |
src/discussions/learners/learner-post-filter-bar/LearnerPostFilterBar.test.jsx |
Updated tests for new content status filter radiogroup |
src/discussions/learners/learner-post-filter-bar/LearnerPostFilterBar.jsx |
Added content status filter for active/deleted distinction |
src/discussions/learners/data/thunks.js |
Implemented deleted content fetching and restore operations |
src/discussions/learners/data/slices.js |
Added restore action reducers and merged filter updates |
src/discussions/learners/data/redux.test.jsx |
Added test assertion for default contentStatus |
src/discussions/learners/data/api.js |
Created APIs for bulk restore and fetching deleted content |
src/discussions/learners/LearnerPostsView.test.jsx |
Updated tests to handle submenu interactions |
src/discussions/learners/LearnerPostsView.jsx |
Integrated restore confirmations and post refresh logic |
src/discussions/learners/LearnerActionsDropdown.test.jsx |
Added submenu hover interactions to tests |
src/discussions/learners/LearnerActionsDropdown.jsx |
Implemented nested submenu with portal rendering for delete/restore actions |
src/discussions/data/thunks.js |
Added performRestoreThread thunk |
src/discussions/data/selectors.js |
Updated filter logic to treat ACTIVE status as unfiltered |
src/discussions/data/constants.js |
Added THREAD_FILTER_TYPES constants |
src/discussions/common/HoverCard.jsx |
Disabled interactive actions for deleted content |
src/discussions/common/ActionsDropdown.jsx |
Added disabled state handling for action items |
src/data/constants.js |
Added RESTORE action and ACTIVE/DELETED status filter constants |
src/components/FilterBar.jsx |
Enhanced filter bar to support separated filter sections |
src/assets/undelete.svg |
Added restore/undelete icon SVG |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| import FilterBar from '../../../components/FilterBar'; | ||
| import { PostsStatusFilter, ThreadType } from '../../../data/constants'; | ||
| // import { PostsStatusFilter, ThreadType } from '../../../data/constants'; |
There was a problem hiding this comment.
The commented-out import statement should be removed rather than commented. If these imports are needed, they should be active; if not, remove them entirely. Commented code creates confusion and clutter in the codebase.
| // import { PostsStatusFilter, ThreadType } from '../../../data/constants'; |
src/discussions/utils.js
Outdated
| ).map(action => ({ | ||
| ...action, | ||
| // For deleted items, disable all actions except 'copy-link' and 'restore' | ||
| disabled: content.isDeleted && action.id !== 'copy-link' && action.id !== 'restore', | ||
| })), [content]); |
There was a problem hiding this comment.
The action filtering logic now adds a disabled property based on isDeleted state. However, this mutates the action objects during rendering which could cause issues with memoization. Consider moving this logic into the checkPermissions or checkConditions functions instead, or creating new action objects rather than spreading and modifying existing ones.
src/discussions/posts/post/Post.jsx
Outdated
| if (result.success) { | ||
| window.location.reload(); |
There was a problem hiding this comment.
After restoring a thread, the code calls window.location.reload() which causes a full page refresh. This is not ideal for user experience as it loses the current state and causes a jarring transition. Consider using Redux state updates to refresh just the thread data instead of reloading the entire page.
src/discussions/posts/NoResults.jsx
Outdated
| const learnersFilter = useSelector(({ learners }) => learners?.usernameSearch); | ||
| const isFiltered = postsFiltered || (topicsFilter !== '') | ||
| || (learnersFilter !== null) || (inContextTopicsFilter !== ''); | ||
| || (learnersFilter) || (inContextTopicsFilter !== ''); |
There was a problem hiding this comment.
The optional chaining learners?.usernameSearch and learners?.postFilter suggests that the learners state might be undefined in some cases. However, this defensive programming is inconsistent - other parts of the codebase access state.learners directly without null checks. Consider either making the learners reducer always defined or consistently use optional chaining throughout.
| const { getDeletedContent } = await import('./api'); | ||
| data = await getDeletedContent(courseId, { | ||
| author, | ||
| page, | ||
| pageSize: 10, | ||
| }); | ||
| } else { | ||
| // Use regular learner posts endpoint for active content | ||
| const { getUserPosts } = await import('./api'); |
There was a problem hiding this comment.
The code dynamically imports API functions inside the thunk (const { getDeletedContent } = await import('./api')). While lazy loading can help with bundle size, this creates inconsistency since other API calls in the same file use static imports. Either use dynamic imports consistently or use static imports for all APIs in this file to avoid confusion and ensure predictable loading behavior.
94b4ad6 to
f296e1e
Compare
5adc134 to
2f2eff9
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 47 out of 48 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| import FilterBar from '../../../components/FilterBar'; | ||
| import { PostsStatusFilter, ThreadType } from '../../../data/constants'; | ||
| // import { PostsStatusFilter, ThreadType } from '../../../data/constants'; |
There was a problem hiding this comment.
Commented out import statement should be removed. The commented import is replaced by the new imports above it, so this line should be deleted to keep the code clean.
| // import { PostsStatusFilter, ThreadType } from '../../../data/constants'; |
| }, | ||
| { | ||
| name: 'status', | ||
| name: 'status', // secondary status |
There was a problem hiding this comment.
The inline comment 'secondary status' is potentially misleading. This comment suggests there's a distinction between primary and secondary status filters, but this isn't clearly documented elsewhere. Consider either adding more comprehensive documentation or removing the comment if it doesn't add clarity.
| if (filters.contentStatus === PostsStatusFilter.DELETED) { | ||
| const { getDeletedContent } = await import('./api'); | ||
| data = await getDeletedContent(courseId, { | ||
| author, | ||
| page, | ||
| pageSize: 10, | ||
| }); |
There was a problem hiding this comment.
Missing error handling for the dynamic import. If the import of 'getDeletedContent' fails, it will throw an unhandled error. Consider wrapping the import in a try-catch block or handling the error appropriately.
| const displayTitle = (type === 'response' || type === 'comment') && threadTitle ? threadTitle : title; | ||
|
|
||
| // For comments/responses, navigate to the parent thread instead of the comment itself | ||
| const navigationPostId = (type === 'response' || type === 'comment') && commentThreadId ? commentThreadId : postId; |
There was a problem hiding this comment.
The navigationPostId calculation could result in undefined if both commentThreadId and postId are falsy. This could lead to navigation errors or broken links. Add validation to ensure a valid post ID is always used.
2f2eff9 to
94b4ad6
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 46 out of 47 changed files in this pull request and generated 16 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/discussions/common/HoverCard.jsx
Outdated
| onClick={() => !isDeleted && handleResponseCommentButton()} | ||
| disabled={isClosed || isDeleted} | ||
| style={{ lineHeight: '20px', ...(isDeleted ? { opacity: 0.3, cursor: 'not-allowed' } : {}) }} |
There was a problem hiding this comment.
The onClick handler on line 54 checks !isDeleted before calling the function, but the button is also disabled when isDeleted is true (line 55). This creates redundant protection. Either the disabled state is sufficient, or if you need the onClick check for some reason, document why both are necessary.
src/discussions/utils.js
Outdated
| */ | ||
| export function checkPermissions(content, action) { | ||
| if (content.editableFields.includes(action)) { | ||
| if (content.editableFields && content.editableFields.includes(action)) { |
There was a problem hiding this comment.
Defensive check added for editableFields is good, but this suggests content.editableFields might be undefined or null. Consider whether this is expected behavior or if the API response should be normalized to always include editableFields as an empty array when not present.
| } | ||
|
|
||
| if (userHasModerationPrivileges || userIsGroupTa) { | ||
| // Add reported filter only for group TA and moderators |
There was a problem hiding this comment.
The comment says 'Add reported filter only for group TA and moderators' but the condition checks for both userHasModerationPrivileges OR userIsGroupTa. This means moderators AND group TAs will see this filter, not just group TAs and moderators as a combined role. The comment should be clarified to match the actual logic.
| contentType, | ||
| } = {}) { | ||
| const params = snakeCaseObject({ | ||
| authorId: author, // The backend expects author_id |
There was a problem hiding this comment.
The API parameter name has been changed from 'authorId' (with snake_case conversion to 'author_id') on line 153 to use 'authorId' which will be converted to 'author_id' by snakeCaseObject. However, the comment on line 153 says 'The backend expects author_id'. Verify that the backend actually accepts this parameter name, as the getUserPosts function uses 'username' (line 81) for the author parameter.
| useEffect(() => () => { | ||
| if (isOpen) { | ||
| close(); | ||
| setTarget(null); | ||
| setActiveSubmenu(null); | ||
| } |
There was a problem hiding this comment.
The cleanup effect on lines 49-55 will run on every render because it doesn't have a dependency array. This means it sets up a new cleanup function on every render, which is inefficient. Add an empty dependency array [] to ensure this cleanup only runs on unmount.
| useEffect(() => () => { | |
| if (isOpen) { | |
| close(); | |
| setTarget(null); | |
| setActiveSubmenu(null); | |
| } | |
| useEffect(() => { | |
| return () => { | |
| if (isOpen) { | |
| close(); | |
| setTarget(null); | |
| setActiveSubmenu(null); | |
| } | |
| }; |
| * @returns {Promise<{}>} | ||
| */ | ||
| export const restoreComment = async (commentId, courseId) => { | ||
| const url = `${getConfig().LMS_BASE_URL}/api/discussion/v1/restore_content`; |
There was a problem hiding this comment.
The API function restoreComment uses a hardcoded endpoint '/api/discussion/v1/restore_content'. This is the same endpoint as used in threads (src/discussions/posts/data/api.js line 227). Consider extracting this to a shared constant or helper function to avoid duplication and ensure consistency.
| <span className={classNames( | ||
| 'font-weight-500 text-primary-500 font-style align-bottom mr-1', | ||
| { 'font-weight-bolder': !read }, | ||
| { 'text-decoration-line-through': isDeleted }, // Line-through for deleted threads |
There was a problem hiding this comment.
The inline comment on line 138 says 'Line-through for deleted threads' but this style is applied to all deleted content (threads, responses, and comments based on the displayTitle logic). The comment should be updated to reflect this broader scope or be more generic.
ce670c9 to
b5256de
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 46 out of 47 changed files in this pull request and generated 21 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .learner-submenu-container { | ||
| position: absolute; | ||
| left: 100%; | ||
| top: 0; | ||
| min-width: 300px; | ||
| max-width: 360px; | ||
| z-index: 9999; | ||
| box-shadow: 0 2px 8px rgba(0, 0, 0, 0.15); | ||
| border: 1px solid var(--pgn-color-light-400); | ||
| overflow: visible; | ||
| } |
There was a problem hiding this comment.
There is duplicate CSS for .learner-submenu-container defined at both lines 603-610 and lines 897-907. The definitions have slightly different properties (e.g., different min-width values: 280px vs 300px, and different max-width values: 320px vs 360px). This duplication should be consolidated into a single definition to avoid confusion and potential styling conflicts.
| const isActionDisabled = useCallback((actionId, isDeleted) => ( | ||
| // For deleted items, disable all actions except 'copy-link' and 'restore' | ||
| isDeleted && actionId !== 'copy-link' && actionId !== 'restore' | ||
| ), []); |
There was a problem hiding this comment.
In the isActionDisabled callback, the logic disables all actions except 'copy-link' and 'restore' for deleted items. However, this function is defined within useActions which is called with content as a dependency. The function should also include content in its dependency array to ensure it updates when the content changes. Currently, the empty dependency array [] means this callback never updates even if the content's deleted state changes.
| const handleRestoreConfirmation = useCallback(async () => { | ||
| try { | ||
| const { performRestoreThread } = await import('../data/thunks'); | ||
| const result = await dispatch(performRestoreThread(postId, courseId)); | ||
| // Check if restore failed and log the error | ||
| if (result && !result.success) { | ||
| logError(`Failed to restore thread: ${result.error || 'Unknown error'}`); | ||
| } | ||
| } catch (error) { | ||
| logError(error); | ||
| } | ||
| hideRestoreConfirmation(); | ||
| }, [postId, courseId, dispatch, hideRestoreConfirmation]); |
There was a problem hiding this comment.
The restore handler on lines 116-128 checks if (result && !result.success) and logs an error, but it doesn't provide any user feedback about the failure. Users should be notified when a restore operation fails so they understand why the content wasn't restored. Consider adding a toast notification or error message display for failed restore operations.
| if (!action.disabled) { | ||
| handleActions(action.action); | ||
| } |
There was a problem hiding this comment.
The click handler on lines 80-83 prevents the action from executing if action.disabled is true, but this check is redundant since the button already has disabled={action.disabled} on line 87. The disabled attribute on the button should already prevent click events from firing. This redundant check adds unnecessary complexity.
| if (!action.disabled) { | |
| handleActions(action.action); | |
| } | |
| handleActions(action.action); |
| export function performRestoreThread(threadId, courseId) { | ||
| return async (dispatch) => { | ||
| try { | ||
| const { restoreThread } = await import('./api'); | ||
| const data = await restoreThread(threadId, courseId); | ||
| // Update the thread in Redux state to reflect the restore | ||
| dispatch(updateThreadSuccess(camelCaseObject(data))); | ||
| return { success: true }; | ||
| } catch (error) { | ||
| logError(error); | ||
| return { success: false, error: error.message }; | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
The performRestoreThread thunk on lines 327-340 returns an object with { success: true } or { success: false, error: error.message }. However, Redux thunks typically don't need to return success/failure indicators - they should dispatch actions that update the state accordingly. The calling code then checks this return value, which is an anti-pattern. Consider having the thunk dispatch success/failure actions and let components observe state changes instead of checking return values.
| } | ||
| } catch (error) { | ||
| logError(error); | ||
| } | ||
| hideRestoreConfirmation(); | ||
| }, [id, courseId, threadId, dispatch, hideRestoreConfirmation]); |
There was a problem hiding this comment.
The handleRestoreConfirmation function on lines 86-96 handles errors by logging them but doesn't provide user feedback when an error occurs. Similar to the thread restore operation, users should be notified if restoring a comment fails. Consider adding error notification to inform users of failures.
| } | |
| } catch (error) { | |
| logError(error); | |
| } | |
| hideRestoreConfirmation(); | |
| }, [id, courseId, threadId, dispatch, hideRestoreConfirmation]); | |
| } else { | |
| window.alert(intl.formatMessage({ | |
| id: 'discussions.comment.restore.error', | |
| defaultMessage: 'There was a problem restoring this comment. Please try again.', | |
| })); | |
| } | |
| } catch (error) { | |
| logError(error); | |
| window.alert(intl.formatMessage({ | |
| id: 'discussions.comment.restore.error', | |
| defaultMessage: 'There was a problem restoring this comment. Please try again.', | |
| })); | |
| } | |
| hideRestoreConfirmation(); | |
| }, [id, courseId, threadId, dispatch, hideRestoreConfirmation, intl]); |
| await dispatch(fetchThread(threadId, courseId)); | ||
| } | ||
| } catch (error) { | ||
| logError(error); |
There was a problem hiding this comment.
Similar to other restore operations, the handleRestoreConfirmation function on lines 142-154 logs errors but doesn't notify the user when a restore operation fails. Users need feedback about failed operations to understand what went wrong and potentially retry or take alternative action.
| logError(error); | |
| logError(error); | |
| window.alert('Unable to restore this comment. Please try again.'); |
| postStatus: RequestStatus.SUCCESSFUL, | ||
| filters: { | ||
| status: PostsStatusFilter.ALL, | ||
| status: PostsStatusFilter.ACTIVE, |
There was a problem hiding this comment.
The default status filter has been changed from PostsStatusFilter.ALL to PostsStatusFilter.ACTIVE on line 49. This is a significant behavior change that will affect all users - they will now only see active (non-deleted) posts by default. While this is likely intentional for the soft delete feature, this change should be clearly documented in the PR description as it changes the default user experience. Additionally, consider if there should be any migration or notification to existing users about this change.
| export function performRestoreComment(commentId, courseId) { | ||
| return async () => { | ||
| try { | ||
| const { restoreComment } = await import('./api'); | ||
| await restoreComment(commentId, courseId); | ||
| return { success: true }; | ||
| } catch (error) { | ||
| logError(error); | ||
| return { success: false, error: error.message }; | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
The performRestoreComment thunk on lines 194-205 follows the same anti-pattern as the thread restore thunk, returning success/failure objects instead of relying on Redux state management. Additionally, this thunk doesn't dispatch any actions to update the Redux state after successfully restoring a comment. This means the UI won't reflect the restored state until the parent component manually refreshes (as seen with the fetchThread call in the component). Consider dispatching an action to update the comment state directly.
| {activeSubmenu === item.id && ( | ||
| <div className="bg-white learner-submenu-container"> | ||
| {item.submenu.map(subItem => ( | ||
| <Dropdown.Item | ||
| key={subItem.id} | ||
| as={Button} | ||
| variant="tertiary" | ||
| size="inline" | ||
| onClick={() => handleActions(subItem.action)} | ||
| className="d-flex justify-content-start actions-dropdown-item" | ||
| data-testid={subItem.id} | ||
| > | ||
| <span className="font-weight-normal"> | ||
| {subItem.label} | ||
| </span> | ||
| </Dropdown.Item> | ||
| ))} | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
The submenu implementation on lines 104-122 shows the submenu when activeSubmenu === item.id. However, the submenu is positioned absolutely (position: absolute in CSS) and uses left: 100% to position it to the right of the parent menu. This positioning approach could cause the submenu to overflow off-screen on smaller viewports or when the dropdown is near the right edge of the window. Consider adding logic to detect available space and flip the submenu to the left side when necessary.
santhosh-apphelix-2u
left a comment
There was a problem hiding this comment.
As discussed, I am approving this PR for now, but it will require refactoring as discussed.
This reverts commit 9dc3367.
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