From d77b956c812df4ca61ede3319d3b00f941248f0f Mon Sep 17 00:00:00 2001 From: Jesper Hodge Date: Thu, 23 Apr 2026 21:07:23 -0400 Subject: [PATCH 1/9] feat: delete tag --- src/taxonomy/data/api.ts | 1 + src/taxonomy/data/apiHooks.ts | 20 ++ src/taxonomy/tag-list/Actions.tsx | 138 ++++++++++ src/taxonomy/tag-list/TagListTable.test.jsx | 253 ++++++++++++++----- src/taxonomy/tag-list/TagListTable.tsx | 133 ++++------ src/taxonomy/tag-list/hooks.test.tsx | 28 +- src/taxonomy/tag-list/hooks.ts | 68 ++++- src/taxonomy/tag-list/messages.ts | 27 ++ src/taxonomy/tag-list/tagColumns.tsx | 159 ++---------- src/taxonomy/tree-table/CreateRow.test.tsx | 73 ++++-- src/taxonomy/tree-table/CreateRow.tsx | 35 ++- src/taxonomy/tree-table/DraftRow.tsx | 9 +- src/taxonomy/tree-table/EditRow.tsx | 20 +- src/taxonomy/tree-table/NestedRows.test.tsx | 104 +++++--- src/taxonomy/tree-table/NestedRows.tsx | 81 +----- src/taxonomy/tree-table/SaveErrorAlert.tsx | 13 +- src/taxonomy/tree-table/TableBody.tsx | 97 ++----- src/taxonomy/tree-table/TableView.test.tsx | 80 ++++-- src/taxonomy/tree-table/TableView.tsx | 116 +++------ src/taxonomy/tree-table/TreeTableContext.tsx | 64 +++++ src/taxonomy/tree-table/index.ts | 1 + 21 files changed, 881 insertions(+), 639 deletions(-) create mode 100644 src/taxonomy/tag-list/Actions.tsx create mode 100644 src/taxonomy/tree-table/TreeTableContext.tsx diff --git a/src/taxonomy/data/api.ts b/src/taxonomy/data/api.ts index f2de425ed5..4d552f9da3 100644 --- a/src/taxonomy/data/api.ts +++ b/src/taxonomy/data/api.ts @@ -99,6 +99,7 @@ export const apiUrls = { tagsPlanImport: (taxonomyId: number) => makeUrl(`${taxonomyId}/tags/import/plan/`), createTag: (taxonomyId: number) => makeUrl(`${taxonomyId}/tags/`), updateTag: (taxonomyId: number) => makeUrl(`${taxonomyId}/tags/`), + deleteTag: (taxonomyId: number) => makeUrl(`${taxonomyId}/tags/`), } satisfies Record string>; /** diff --git a/src/taxonomy/data/apiHooks.ts b/src/taxonomy/data/apiHooks.ts index b47ee23f1b..fd40a10097 100644 --- a/src/taxonomy/data/apiHooks.ts +++ b/src/taxonomy/data/apiHooks.ts @@ -269,3 +269,23 @@ export const useUpdateTag = (taxonomyId: number) => { }, }); }; + +export const useDeleteTag = (taxonomyId: number) => { + const queryClient = useQueryClient(); + + return useMutation({ + mutationFn: async ({ value, withSubtags }: { value: string; withSubtags: boolean; }) => { + const body = { tags: [value], with_subtags: withSubtags }; + await getAuthenticatedHttpClient().delete(apiUrls.deleteTag(taxonomyId), { + data: body, + }); + }, + onSuccess: () => { + queryClient.invalidateQueries({ + queryKey: taxonomyQueryKeys.taxonomyTagList(taxonomyId), + }); + // In the metadata, 'tagsCount' (and possibly other fields) will have changed: + queryClient.invalidateQueries({ queryKey: taxonomyQueryKeys.taxonomyMetadata(taxonomyId) }); + }, + }); +}; diff --git a/src/taxonomy/tag-list/Actions.tsx b/src/taxonomy/tag-list/Actions.tsx new file mode 100644 index 0000000000..0740ed39ef --- /dev/null +++ b/src/taxonomy/tag-list/Actions.tsx @@ -0,0 +1,138 @@ +import { + Icon, + IconButton, + IconButtonWithTooltip, + Dropdown, +} from '@openedx/paragon'; +import { + AddCircle, + MoreVert, +} from '@openedx/paragon/icons'; +import { useIntl } from '@edx/frontend-platform/i18n'; +import type { Row } from '@tanstack/react-table'; + +import type { + RowId, + TreeRowData, +} from '@src/taxonomy/tree-table/types'; +import type { TagListRowData } from './types'; +import messages from './messages'; + +interface ActionsHeaderProps { + onStartDraft: () => void; + setDraftError: (error: string) => void; + setIsCreatingTopRow: (isCreating: boolean) => void; + setEditingRowId: (id: RowId | null) => void; + setActiveActionMenuRowId: (id: RowId | null) => void; + hasOpenDraft: boolean; + draftInProgressHintId: string; + canAddTag: boolean; +} + +const ActionsHeader = ({ + onStartDraft, + setDraftError, + setIsCreatingTopRow, + setEditingRowId, + setActiveActionMenuRowId, + hasOpenDraft, + canAddTag, + draftInProgressHintId, +}: ActionsHeaderProps) => { + const intl = useIntl(); + return ( +
+ {intl.formatMessage(messages.createNewTagTooltip)}
} + src={AddCircle} + alt={intl.formatMessage(messages.createTagButtonLabel)} + size="inline" + onClick={() => { + onStartDraft(); + setDraftError(''); + setIsCreatingTopRow(true); + setEditingRowId(null); + setActiveActionMenuRowId(null); + }} + disabled={hasOpenDraft || !canAddTag} + aria-describedby={hasOpenDraft ? draftInProgressHintId : undefined} + /> + + ); +}; + +interface ActionsMenuProps { + rowData: TagListRowData; + startSubtagDraft: () => void; + disableAddSubtag: boolean; + startEditRow: () => void; + disableEditRow: boolean; + reachedMaxDepth: (row: Row) => boolean; + startDeleteRow: (row: Row) => void; + disableDeleteRow: boolean; + row: Row; +} + +const ActionsMenu = ({ + rowData, + row, + startSubtagDraft, + disableAddSubtag, + startEditRow, + disableEditRow, + reachedMaxDepth, + startDeleteRow, + disableDeleteRow, +}: ActionsMenuProps) => { + const intl = useIntl(); + + const deleteRowMenuItem = ( + startDeleteRow(row)} + disabled={disableDeleteRow} + > + {intl.formatMessage(messages.deleteTag)} + + ); + + const editRowMenuItem = ( + + {intl.formatMessage(messages.renameTag)} + + ); + + return ( + + + + + {intl.formatMessage(messages.addSubtag)} + + {editRowMenuItem} + {deleteRowMenuItem} + + + ); +}; + +const Actions = { + Header: ActionsHeader, + Menu: ActionsMenu, +}; + +export default Actions; diff --git a/src/taxonomy/tag-list/TagListTable.test.jsx b/src/taxonomy/tag-list/TagListTable.test.jsx index d9022c84fb..0f31b59189 100644 --- a/src/taxonomy/tag-list/TagListTable.test.jsx +++ b/src/taxonomy/tag-list/TagListTable.test.jsx @@ -1,5 +1,6 @@ import React from 'react'; import { AxiosError } from 'axios'; +import userEvent from '@testing-library/user-event'; import { render, waitFor, @@ -86,21 +87,12 @@ const mockTagsResponse = { const mockTagResponseDisallowingEdits = { ...mockTagsResponse, - results: mockTagsResponse.results.map(tag => ({ + results: mockTagsResponse.results.map((tag) => ({ ...tag, can_change_tag: false, can_delete_tag: false, })), }; -const mockTagsPaginationResponse = { - next: null, - previous: null, - count: 103, - num_pages: 2, - current_page: 1, - start: 0, - results: [], -}; const rootTagsListUrl = 'http://localhost:18010/api/content_tagging/v1/taxonomies/1/tags/?full_depth_threshold=10000&include_counts=true'; const subTagsResponse = { @@ -124,6 +116,8 @@ const subTagsResponse = { const subTagsUrl = 'http://localhost:18010/api/content_tagging/v1/taxonomies/1/tags/?full_depth_threshold=10000&parent_tag=root+tag+1'; const createTagUrl = 'http://localhost:18010/api/content_tagging/v1/taxonomies/1/tags/'; +const deleteTagUrl = createTagUrl; +const deleteConfirmMessage = 'Warning: are you sure you want to delete this tag and all its subtags and descendants? Any tags applied to course content will be deleted.'; const renderTagListTable = (maxDepth = 3) => render(); @@ -198,6 +192,25 @@ const openRenameDraftRow = async (tagName = 'root tag 1') => { }; }; +const buildTagsResponse = (results) => ({ + ...mockTagsResponse, + count: results.filter((tag) => tag.depth === 0).length, + results, +}); + +const expectDeleteRequest = async ({ tagName, withSubtags }) => { + await waitFor(() => { + expect(axiosMock.history.delete.length).toBe(1); + expect(axiosMock.history.delete[0].url).toBe(deleteTagUrl); + expect(axiosMock.history.delete[0].data).toEqual( + JSON.stringify({ + tags: [tagName], + with_subtags: withSubtags, + }), + ); + }); +}; + describe('', () => { beforeEach(async () => { ({ axiosMock } = initializeMocks({ user: adminUser })); @@ -281,24 +294,6 @@ describe('', () => { }); }); - // temporarily skipped because pagination is not implemented yet - it.skip('should render pagination footer', async () => { - axiosMock.onGet(rootTagsListUrl).reply(200, mockTagsPaginationResponse); - renderTagListTable(); - const tableFooter = await screen.findAllByRole('navigation', { - name: /table pagination/i, - }); - expect(tableFooter[0]).toBeInTheDocument(); - }); - - // temporarily skipped because pagination is not implemented yet - it.skip('should render correct number of items in pagination footer', async () => { - axiosMock.onGet(rootTagsListUrl).reply(200, mockTagsPaginationResponse); - renderTagListTable(); - const paginationButtons = await screen.findByText('Page 1 of 2'); - expect(paginationButtons).toBeInTheDocument(); - }); - describe('Create a new top-level tag', () => { it('should disable tag creation buttons if the taxonomy includes `can_add_tag: false`', async () => { axiosMock.onGet(rootTagsListUrl).reply(200, { @@ -460,38 +455,6 @@ describe('', () => { expect(temporaryRow).toBeInTheDocument(); }); - // temporarily skipped because pagination is not implemented yet - it.skip('should refresh the table and remove the temporary row when a pagination button is clicked', async () => { - axiosMock.onPost(createTagUrl).reply(201, { - ...tagDefaults, - value: 'xyz tag', - child_count: 0, - _id: 1234, - }); - const { creatingRow, input } = await openTopLevelDraftRow(); - - fireEvent.change(input, { target: { value: 'xyz tag' } }); - const saveButton = within(creatingRow).getByRole('button', { name: 'Save' }); - fireEvent.click(saveButton); - const temporaryRow = await screen.findByText('xyz tag'); - // temporaryRow should be at the top of the table, that is, the first row after the header - const rows = screen.getAllByRole('row'); - expect(rows[1]).toContainElement(temporaryRow); - - // Simulate clicking a pagination button - const paginationButton = await screen.findByRole('button', { name: 'Go to page 2' }); - fireEvent.click(paginationButton); - - await waitFor(() => { - // A get request should have refreshed the table data - expect(axiosMock.history.get.length).toBeGreaterThan(1); - const xyzTagRow = screen.queryByText('xyz tag'); - expect(xyzTagRow).toBeInTheDocument(); - // expect the row to not be the first row after the header - expect(rows[1]).not.toContainElement(xyzTagRow); - }); - }); - // a bit flaky when ran together with other tests - any way to improve this? it('should allow adding multiple tags consecutively without a page refresh', async () => { // clear axios mock history @@ -991,6 +954,165 @@ describe('', () => { expect(within(grandchildTagRow).getByText('Add Subtag')).toHaveAttribute('aria-disabled', 'true'); }); }); + + describe('Delete Tags', () => { + let confirmSpy; + + beforeEach(() => { + confirmSpy = jest.spyOn(window, 'confirm').mockReturnValue(true); + }); + + afterEach(() => { + confirmSpy.mockRestore(); + }); + + const tagDepthScenarios = [ + { + description: 'Delete a top-level tag', + tagName: 'root tag 1', + }, + { description: 'Delete a sub-tag', tagName: 'the child tag' }, + { description: 'Delete a grandchild tag', tagName: 'the grandchild tag' }, + ]; + + tagDepthScenarios.forEach(({ description, tagName }) => { + describe(description, () => { + beforeEach(async () => { + axiosMock.resetHistory(); + }); + + it('should disable delete action if tag includes `can_delete: false`', async () => { + axiosMock.reset(); + axiosMock + .onGet(rootTagsListUrl) + .reply(200, mockTagResponseDisallowingEdits); + axiosMock.onGet(subTagsUrl).reply(200, subTagsResponse); + cleanup(); + ({ axiosMock } = initializeMocks({ user: adminUser })); + axiosMock + .onGet(rootTagsListUrl) + .reply(200, mockTagResponseDisallowingEdits); + axiosMock.onGet(subTagsUrl).reply(200, subTagsResponse); + renderTagListTable(); + await waitForRootTag(); + + openActionsMenuForTag(tagName); + const deleteButton = screen.getByRole('button', { name: /Delete/i }); + expect(deleteButton).toBeInTheDocument(); + expect(deleteButton).toHaveAttribute('aria-disabled', 'true'); + }); + }); + }); + + it('asks for browser confirmation before deleting a leaf tag and deletes after acceptance', async () => { + axiosMock.reset(); + axiosMock.onDelete(deleteTagUrl).reply(204); + axiosMock + .onGet(rootTagsListUrl) + .reply( + 200, + buildTagsResponse( + mockTagsResponse.results.filter( + (tag) => tag.value !== 'root tag 2', + ), + ), + ); + + expect(screen.queryByText('root tag 2')).toBeInTheDocument(); + + openActionsMenuForTag('root tag 2'); + fireEvent.click(screen.getByRole('button', { name: /^Delete$/i })); + + expect(confirmSpy).toHaveBeenCalledWith(deleteConfirmMessage); + await expectDeleteRequest({ tagName: 'root tag 2', withSubtags: false }); + expect( + await screen.findByText( + '1 tag(s) deleted. This change will be applied across all tagged content.', + ), + ).toBeInTheDocument(); + await waitFor(() => { + expect(screen.queryByText('root tag 2')).not.toBeInTheDocument(); + }); + }); + + it('deletes a parent tag together with all rendered descendants after browser confirmation', async () => { + fireEvent.click(screen.getAllByText('Expand All')[0]); + await screen.findByText('the child tag'); + await screen.findByText('the grandchild tag'); + + axiosMock.reset(); + axiosMock.onDelete(deleteTagUrl).reply(204); + axiosMock + .onGet(rootTagsListUrl) + .reply( + 200, + buildTagsResponse( + mockTagsResponse.results.filter( + (tag) => + !['root tag 1', 'the child tag', 'the grandchild tag'].includes( + tag.value, + ), + ), + ), + ); + + openActionsMenuForTag('root tag 1'); + fireEvent.click(screen.getByRole('button', { name: /^Delete$/i })); + + expect(confirmSpy).toHaveBeenCalledWith(deleteConfirmMessage); + await expectDeleteRequest({ tagName: 'root tag 1', withSubtags: true }); + expect( + await screen.findByText( + '3 tag(s) deleted. This change will be applied across all tagged content.', + ), + ).toBeInTheDocument(); + await waitFor(() => { + expect(screen.queryByText('root tag 1')).not.toBeInTheDocument(); + expect(screen.queryByText('the child tag')).not.toBeInTheDocument(); + expect( + screen.queryByText('the grandchild tag'), + ).not.toBeInTheDocument(); + expect(screen.getByText('root tag 2')).toBeInTheDocument(); + }); + }); + + it('does not issue a delete request when browser confirmation is canceled and leaves the table unchanged', async () => { + confirmSpy.mockReturnValue(false); + + openActionsMenuForTag('root tag 1'); + fireEvent.click(screen.getByRole('button', { name: /^Delete$/i })); + + expect(confirmSpy).toHaveBeenCalledWith(deleteConfirmMessage); + expect(axiosMock.history.delete.length).toBe(0); + expect(screen.getByText('root tag 1')).toBeInTheDocument(); + }); + + it('surfaces a failed delete through both the persistent error alert and the delete-error toast while leaving the row visible', async () => { + axiosMock.reset(); + axiosMock.onDelete(deleteTagUrl).reply(() => { + const error = new AxiosError('Request failed with status code 500'); + error.response = { + data: { + value: ['Delete failed'], + }, + }; + return Promise.reject(error); + }); + + openActionsMenuForTag('root tag 1'); + fireEvent.click(screen.getByRole('button', { name: /^Delete$/i })); + + expect(confirmSpy).toHaveBeenCalledWith(deleteConfirmMessage); + await expectDeleteRequest({ tagName: 'root tag 1', withSubtags: true }); + expect( + await screen.findByText('Error saving changes'), + ).toBeInTheDocument(); + expect( + await screen.findByText('Error deleting tag: Delete failed'), + ).toBeInTheDocument(); + expect(screen.getByText('root tag 1')).toBeInTheDocument(); + }); + }); }); // These async creation flows are intentionally isolated because they pass individually @@ -1206,12 +1328,15 @@ describe(' pagination transition behavior', () => { mutateAsync: jest.fn(), }); jest.spyOn(hooksModule, 'useEditActions').mockReturnValue({ - handleCreateTag: jest.fn(), - handleUpdateTag: jest.fn(), + handleCreateRow: jest.fn(), + handleUpdateRow: jest.fn(), + startSubtagDraft: jest.fn(), + startEditRow: jest.fn(), + startDeleteRow: jest.fn(), validate: jest.fn(() => true), }); - jest.spyOn(treeTableModule, 'TableView').mockImplementation((props) => { - tableViewProps = props; + jest.spyOn(treeTableModule, 'TableView').mockImplementation(() => { + tableViewProps = React.useContext(treeTableModule.TreeTableContext); return
; }); }); diff --git a/src/taxonomy/tag-list/TagListTable.tsx b/src/taxonomy/tag-list/TagListTable.tsx index c2d00319d8..eb5317e96c 100644 --- a/src/taxonomy/tag-list/TagListTable.tsx +++ b/src/taxonomy/tag-list/TagListTable.tsx @@ -4,18 +4,17 @@ import React, { useEffect, } from 'react'; import type { PaginationState } from '@tanstack/react-table'; -import { TableView } from '@src/taxonomy/tree-table'; -import { useTagListData, useCreateTag, useUpdateTag } from '@src/taxonomy/data/apiHooks'; -import { TagTree } from './tagTree'; +import { useTagListData, useCreateTag, useUpdateTag, useDeleteTag } from '@src/taxonomy/data/apiHooks'; +import { TableView, TreeTableContext } from '@src/taxonomy/tree-table'; import type { RowId, - TreeColumnDef, TreeRowData, } from '../tree-table/types'; +import { TagTree } from './tagTree'; +import { getColumns } from './tagColumns'; import { TABLE_MODES, } from './constants'; -import { getColumns } from './tagColumns'; import { useTableModes, useEditActions } from './hooks'; interface TagListTableProps { @@ -47,7 +46,7 @@ const TagListTable = ({ taxonomyId, maxDepth }: TagListTableProps) => { const [toast, setToast] = useState({ show: false, message: '', variant: 'success' }); const [tagTree, setTagTree] = useState(null); const [isCreatingTopTag, setIsCreatingTopTag] = useState(false); - const [activeActionMenuRowId, setActiveActionMenuRowId] = useState(null); + const [, setActiveActionMenuRowId] = useState(null); const [draftError, setDraftError] = useState(''); const treeData = (tagTree?.getAllAsDeepCopy() || []) as unknown as TreeRowData[]; const hasOpenDraft = isCreatingTopTag || creatingParentId !== null || editingRowId !== null; @@ -83,59 +82,29 @@ const TagListTable = ({ taxonomyId, maxDepth }: TagListTableProps) => { }); const createTagMutation = useCreateTag(taxonomyId); const updateTagMutation = useUpdateTag(taxonomyId); + const deleteTagMutation = useDeleteTag(taxonomyId); const pageCount = tagList?.numPages ?? -1; - - // TODO: to make this more readable, introduce a React context for the TagListTable instead of passing props. + const canAddTag = tagList?.canAddTag !== false; // Custom Edit Actions Hook - handles table mode transitions, API calls, // and updating the table without a full data reload when creating or editing tags. - const { handleCreateTag, handleUpdateTag, validate } = useEditActions({ - setTagTree, - setDraftError, - createTagMutation, - updateTagMutation, - enterPreviewMode, - setToast, - setIsCreatingTopTag, - setCreatingParentId, - exitDraftWithoutSave, - setEditingRowId, - }); - - const columns = useMemo( - () => - getColumns({ - setIsCreatingTopTag, - setCreatingParentId, - handleUpdateTag, - setEditingRowId, - onStartDraft: enterDraftMode, - setActiveActionMenuRowId, - hasOpenDraft, - canAddTag: tagList?.canAddTag !== false, - draftError, - setDraftError, - isSavingDraft: createTagMutation.isPending, - maxDepth, - }), - [ - isCreatingTopTag, - tableMode, - activeActionMenuRowId, - hasOpenDraft, - creatingParentId, - tagList?.canAddTag, - draftError, - createTagMutation.isPending, - maxDepth, + const editActions = useEditActions( + { + enterDraftMode, + enterPreviewMode, + enterViewMode, + setTagTree, + setDraftError, + createTagMutation, + updateTagMutation, + setToast, setIsCreatingTopTag, setCreatingParentId, - handleUpdateTag, + exitDraftWithoutSave, setEditingRowId, - enterDraftMode, setActiveActionMenuRowId, - setDraftError, - ], + deleteTagMutation, + }, ); // RELOAD DATA IN VIEW MODE @@ -150,33 +119,43 @@ const TagListTable = ({ taxonomyId, maxDepth }: TagListTableProps) => { } }, [tagList?.results, tableMode]); + // TreeTable context + const contextValueArgs = { + ...editActions, + treeData, + pageCount, + pagination, + handlePaginationChange, + isLoading, + isCreatingTopRow: isCreatingTopTag, + draftError, + createRowMutation: createTagMutation, + updateRowMutation: updateTagMutation, + toast, + setToast, + setIsCreatingTopRow: setIsCreatingTopTag, + exitDraftWithoutSave, + creatingParentId, + setCreatingParentId, + setDraftError, + editingRowId, + setEditingRowId, + onStartDraft: enterDraftMode, + setActiveActionMenuRowId, + hasOpenDraft, + canAddTag, + maxDepth, + }; + const contextValue = { + ...contextValueArgs, + columns: getColumns(contextValueArgs), + table: null, + }; + return ( - + + + ); }; diff --git a/src/taxonomy/tag-list/hooks.test.tsx b/src/taxonomy/tag-list/hooks.test.tsx index 89fb70c0c6..f9e8ec4f0e 100644 --- a/src/taxonomy/tag-list/hooks.test.tsx +++ b/src/taxonomy/tag-list/hooks.test.tsx @@ -44,28 +44,35 @@ describe('useTableModes', () => { describe('useEditActions', () => { const buildActions = (overrides = {}) => { const createTagMutation = { mutateAsync: jest.fn() }; - // mock updateTagMutation to have a function `mutateAsync` that returns a resolved promise const updateTagMutation = { mutateAsync: jest.fn() }; + const deleteTagMutation = { mutateAsync: jest.fn() }; const setTagTree = jest.fn(); const setDraftError = jest.fn(); + const enterDraftMode = jest.fn(); const enterPreviewMode = jest.fn(); + const enterViewMode = jest.fn(); const setToast = jest.fn(); const setIsCreatingTopTag = jest.fn(); const setCreatingParentId = jest.fn(); const exitDraftWithoutSave = jest.fn(); const setEditingRowId = jest.fn(); + const setActiveActionMenuRowId = jest.fn(); const params = { setTagTree, setDraftError, createTagMutation: createTagMutation as any, + enterDraftMode, enterPreviewMode, + enterViewMode, setToast, setIsCreatingTopTag, setCreatingParentId, exitDraftWithoutSave, setEditingRowId, updateTagMutation: updateTagMutation as any, + deleteTagMutation: deleteTagMutation as any, + setActiveActionMenuRowId, ...(overrides as any), }; @@ -74,14 +81,18 @@ describe('useEditActions', () => { return { actions: result.current, createTagMutation, + deleteTagMutation, setTagTree, setDraftError, + enterDraftMode, enterPreviewMode, + enterViewMode, setToast, setIsCreatingTopTag, setCreatingParentId, exitDraftWithoutSave, setEditingRowId, + setActiveActionMenuRowId, }; }; @@ -122,7 +133,7 @@ describe('useEditActions', () => { } = buildActions(); await act(async () => { - await actions.handleUpdateTag(' same value ', 'same value'); + await actions.handleUpdateRow(' same value ', 'same value'); }); expect(enterPreviewMode).not.toHaveBeenCalled(); @@ -139,7 +150,7 @@ describe('useEditActions', () => { } = buildActions(); await act(async () => { - await actions.handleUpdateTag('updated', 'original'); + await actions.handleUpdateRow('updated', 'original'); }); await waitFor(() => { @@ -152,17 +163,12 @@ describe('useEditActions', () => { expect(setEditingRowId).toHaveBeenCalledWith(null); }); - it('keeps draft open and shows failure toast when createTag request fails', async () => { - const { - actions, - createTagMutation, - setDraftError, - setToast, - } = buildActions(); + it('keeps draft open and shows failure toast when createRow request fails', async () => { + const { actions, createTagMutation, setDraftError, setToast } = buildActions(); createTagMutation.mutateAsync.mockRejectedValue(new Error('server failed')); await act(async () => { - await actions.handleCreateTag('new tag'); + await actions.handleCreateRow('new tag'); }); expect(setDraftError).toHaveBeenCalledWith('server failed'); diff --git a/src/taxonomy/tag-list/hooks.ts b/src/taxonomy/tag-list/hooks.ts index a1518b9de9..d0bbb942f1 100644 --- a/src/taxonomy/tag-list/hooks.ts +++ b/src/taxonomy/tag-list/hooks.ts @@ -3,8 +3,8 @@ import { AxiosError } from 'axios'; import { useIntl } from '@edx/frontend-platform/i18n'; import globalMessages from '@src/messages'; -import { useCreateTag, useUpdateTag } from '@src/taxonomy/data/apiHooks'; -import type { RowId } from '@src/taxonomy/tree-table/types'; +import { useCreateTag, useDeleteTag, useUpdateTag } from '@src/taxonomy/data/apiHooks'; +import type { RowId, TreeRowData } from '@src/taxonomy/tree-table/types'; import { TagTree } from './tagTree'; import { TagListTableError } from './errors'; import { @@ -15,6 +15,10 @@ import { } from './constants'; import messages from './messages'; +import { getTagListRowData, getTagWithDescendantsCount } from './utils'; +import { Row } from '@tanstack/react-table'; + +const DELETE_CONFIRM_MESSAGE = 'Warning: are you sure you want to delete this tag and all its subtags and descendants? Any tags applied to course content will be deleted.'; /** Interface for table mode actions for React's `useReducer` hook. * @@ -41,13 +45,17 @@ interface UseEditActionsParams { setTagTree: React.Dispatch>; setDraftError: React.Dispatch>; createTagMutation: ReturnType; + enterDraftMode: () => void; enterPreviewMode: () => void; + enterViewMode: () => void; setToast: React.Dispatch>; setIsCreatingTopTag: React.Dispatch>; setCreatingParentId: React.Dispatch>; exitDraftWithoutSave: () => void; setEditingRowId: React.Dispatch>; updateTagMutation: ReturnType; + setActiveActionMenuRowId: React.Dispatch>; + deleteTagMutation: ReturnType; } const getInlineValidationMessage = (value: string, intl: ReturnType): string => { @@ -111,16 +119,19 @@ const useTableModes = (): UseTableModesReturn => { }; const useEditActions = ({ + enterDraftMode, + enterPreviewMode, + enterViewMode, setTagTree, setDraftError, createTagMutation, - enterPreviewMode, setToast, setIsCreatingTopTag, setCreatingParentId, exitDraftWithoutSave, setEditingRowId, updateTagMutation, + deleteTagMutation, }: UseEditActionsParams) => { const intl = useIntl(); @@ -255,10 +266,57 @@ const useEditActions = ({ } }; + const startSubtagDraft = (row: Row) => { + const rowData = getTagListRowData(row); + enterDraftMode(); + setDraftError(''); + setCreatingParentId(rowData.id); + row.toggleExpanded(true); + }; + + const startEditTag = (row: Row) => { + const rowData = getTagListRowData(row); + enterDraftMode(); + setDraftError(''); + setEditingRowId(`${rowData.id}:${rowData.value}`); + }; + + const startDeleteTag = (row: Row) => { + if (window.confirm(DELETE_CONFIRM_MESSAGE)) { + void handleDeleteTag(row); + } + }; + + const handleDeleteTag = async (row: Row) => { + const rowData = getTagListRowData(row); + const count = getTagWithDescendantsCount(rowData); + // If the tag in the frontend state does not have subtags, + // don't allow the backend to delete subtags. + // That prevents problems in case of stale frontend state. + const shouldDeleteSubtags = count > 1; + try { + // In view mode, the table reloads on change, reflecting the deletion + // without needing to manually update the table state + enterViewMode(); + await deleteTagMutation.mutateAsync({ value: rowData.value, withSubtags: shouldDeleteSubtags }); + setToast({ + show: true, + message: intl.formatMessage(messages.tagsDeleteSuccessMessage, { count }), + }); + } catch (error) { + const errorMessage = getErrorMessage(error); + setDraftError(errorMessage); + setToast({ show: true, message: intl.formatMessage(messages.tagDeleteErrorMessage, { errorMessage }) }); + } + }; + return { updateTableWithoutDataReload, - handleCreateTag, - handleUpdateTag, + handleCreateRow: handleCreateTag, + handleUpdateRow: handleUpdateTag, + startSubtagDraft, + startEditRow: startEditTag, + startDeleteRow: startDeleteTag, validate, }; }; diff --git a/src/taxonomy/tag-list/messages.ts b/src/taxonomy/tag-list/messages.ts index 4ba77837d9..cc338f4607 100644 --- a/src/taxonomy/tag-list/messages.ts +++ b/src/taxonomy/tag-list/messages.ts @@ -100,6 +100,33 @@ const messages = defineMessages({ deleteTagConfirmationEmphasizedPart: { id: 'course-authoring.tag-list.delete-tag-confirmation-bold-part', defaultMessage: 'Any tags applied to course content will be removed across all assigned organizations.', + deleteTag: { + id: 'course-authoring.tag-list.delete-tag', + defaultMessage: 'Delete', + }, + deleteTagDisabledTooltip: { + id: 'course-authoring.tag-list.delete-tag-disabled-tooltip', + defaultMessage: 'This tag does not allow deletion', + }, + tagEditForbidden: { + id: 'course-authoring.tag-list.system-defined-tag-edit-disabled', + defaultMessage: 'Disabled because this is not allowed to be changed', + }, + tagDeleteForbidden: { + id: 'course-authoring.tag-list.system-defined-tag-delete-disabled', + defaultMessage: 'Disabled because this is not allowed to be deleted', + }, + hasOpenDraft: { + id: 'course-authoring.tag-list.has-open-draft', + defaultMessage: 'Disabled because tag creation or edit is in progress', + }, + tagsDeleteSuccessMessage: { + id: 'course-authoring.tag-list.delete-success', + defaultMessage: '{count} tag(s) deleted. This change will be applied across all tagged content.', + }, + tagDeleteErrorMessage: { + id: 'course-authoring.tag-list.delete-error', + defaultMessage: 'Error deleting tag: {errorMessage}', }, }); diff --git a/src/taxonomy/tag-list/tagColumns.tsx b/src/taxonomy/tag-list/tagColumns.tsx index 7b8874331f..800355a761 100644 --- a/src/taxonomy/tag-list/tagColumns.tsx +++ b/src/taxonomy/tag-list/tagColumns.tsx @@ -1,14 +1,4 @@ -import { - Icon, - IconButton, - IconButtonWithTooltip, - Dropdown, -} from '@openedx/paragon'; -import { - AddCircle, - MoreVert, -} from '@openedx/paragon/icons'; -import { FormattedMessage, useIntl } from '@edx/frontend-platform/i18n'; +import { FormattedMessage } from '@edx/frontend-platform/i18n'; import type { Row } from '@tanstack/react-table'; import type { @@ -16,128 +6,35 @@ import type { TreeColumnDef, TreeRowData, } from '@src/taxonomy/tree-table/types'; -import type { TagListRowData } from './types'; import messages from './messages'; import OptionalExpandLink from './OptionalExpandLink'; import UsageCountDisplay from './UsageCountDisplay'; import { getTagListRowData } from './utils'; +import Actions from './Actions'; const EDITABLE_COLUMNS = ['value']; interface GetColumnsArgs { - setIsCreatingTopTag: (isCreating: boolean) => void; - setCreatingParentId: (id: RowId | null) => void; - handleUpdateTag: (value: string, originalValue: string) => void; + setIsCreatingTopRow: (isCreating: boolean) => void; setEditingRowId: (id: RowId | null) => void; onStartDraft: () => void; setActiveActionMenuRowId: (id: RowId | null) => void; hasOpenDraft: boolean; canAddTag: boolean; - draftError: string; setDraftError: (error: string) => void; - isSavingDraft: boolean; maxDepth: number; + startSubtagDraft: (row: Row) => void; + startEditRow: (row: Row) => void; + startDeleteRow: (row: Row) => void; } -interface ActionsHeaderProps { - onStartDraft: () => void; - setDraftError: (error: string) => void; - setIsCreatingTopTag: (isCreating: boolean) => void; - setEditingRowId: (id: RowId | null) => void; - setActiveActionMenuRowId: (id: RowId | null) => void; - hasOpenDraft: boolean; - draftInProgressHintId: string; - canAddTag: boolean; -} - -const ActionsHeader = ({ - onStartDraft, - setDraftError, - setIsCreatingTopTag, - setEditingRowId, - setActiveActionMenuRowId, - hasOpenDraft, - canAddTag, - draftInProgressHintId, -}: ActionsHeaderProps) => { - const intl = useIntl(); - return ( -
- {intl.formatMessage(messages.createNewTagTooltip)}
} - src={AddCircle} - alt={intl.formatMessage(messages.createTagButtonLabel)} - size="inline" - onClick={() => { - onStartDraft(); - setDraftError(''); - setIsCreatingTopTag(true); - setEditingRowId(null); - setActiveActionMenuRowId(null); - }} - disabled={hasOpenDraft || !canAddTag} - aria-describedby={hasOpenDraft ? draftInProgressHintId : undefined} - /> -
- ); -}; - -interface ActionsMenuProps { - rowData: TagListRowData; - startSubtagDraft: () => void; - disableAddSubtag: boolean; - editTag: () => void; - disableEditTag: boolean; - reachedMaxDepth: (row: Row) => boolean; - row: Row; -} - -const ActionsMenu = ({ - rowData, - row, - startSubtagDraft, - disableAddSubtag, - editTag, - disableEditTag, - reachedMaxDepth, -}: ActionsMenuProps) => { - const intl = useIntl(); - - return ( - - - - - {intl.formatMessage(messages.addSubtag)} - - - {intl.formatMessage(messages.renameTag)} - - - - ); -}; - function getColumns({ - setIsCreatingTopTag, - setCreatingParentId, + setIsCreatingTopRow, setEditingRowId, onStartDraft, + startSubtagDraft, + startEditRow, + startDeleteRow, setActiveActionMenuRowId, hasOpenDraft, canAddTag, @@ -172,10 +69,10 @@ function getColumns({ { id: 'actions', header: () => ( - { - onStartDraft(); - setDraftError(''); - setCreatingParentId(rowData.id); - setEditingRowId(null); - setIsCreatingTopTag(false); - setActiveActionMenuRowId(null); - row.toggleExpanded(true); - }; - - const editTag = () => { - onStartDraft(); - setDraftError(''); - setEditingRowId(`${rowData.id}:${rowData.value}`); - setCreatingParentId(null); - setIsCreatingTopTag(false); - setActiveActionMenuRowId(null); - }; + const disableEditRow = hasOpenDraft || rowData.canChangeTag === false; + const disableDeleteRow = hasOpenDraft || rowData.canDeleteTag === false; return (
- startSubtagDraft(row)} disableAddSubtag={disableAddSubtag} - editTag={editTag} - disableEditTag={disableEditTag} + startEditRow={() => startEditRow(row)} + disableEditRow={disableEditRow} reachedMaxDepth={reachedMaxDepth} + startDeleteRow={() => startDeleteRow(row)} + disableDeleteRow={disableDeleteRow} />
); diff --git a/src/taxonomy/tree-table/CreateRow.test.tsx b/src/taxonomy/tree-table/CreateRow.test.tsx index 57a7840441..77a1ff30e1 100644 --- a/src/taxonomy/tree-table/CreateRow.test.tsx +++ b/src/taxonomy/tree-table/CreateRow.test.tsx @@ -3,30 +3,49 @@ import { IntlProvider } from '@edx/frontend-platform/i18n'; import { fireEvent, render, screen } from '@testing-library/react'; import CreateRow from './CreateRow'; +import { TreeTableContext } from './TreeTableContext'; const wrapper = ({ children }: { children: React.ReactNode; }) => ( {children} ); -const baseProps = () => ({ +const baseContextValue = () => ({ + treeData: [], + columns: [], + pageCount: -1, + pagination: { pageIndex: 0, pageSize: 10 }, + handlePaginationChange: jest.fn(), + isLoading: false, + isCreatingTopRow: false, draftError: '', setDraftError: jest.fn(), handleCreateRow: jest.fn(), setIsCreatingTopRow: jest.fn(), exitDraftWithoutSave: jest.fn(), createRowMutation: { isPending: false }, + updateRowMutation: {}, + toast: { show: false, message: '', variant: 'success' }, + setToast: jest.fn(), + creatingParentId: null, + setCreatingParentId: jest.fn(), validate: jest.fn((value: string) => value.trim().length > 0), + handleUpdateRow: jest.fn(), + editingRowId: null, + setEditingRowId: jest.fn(), + table: null, }); describe('CreateRow', () => { it('saves on Enter when value is valid', () => { - const props = baseProps(); + const contextValue = baseContextValue(); render( - - - - -
, + + + + + +
+ , { wrapper }, ); @@ -34,19 +53,21 @@ describe('CreateRow', () => { fireEvent.change(input, { target: { value: ' new tag ' } }); fireEvent.keyDown(input, { key: 'Enter' }); - expect(props.handleCreateRow).toHaveBeenCalledWith('new tag'); + expect(contextValue.handleCreateRow).toHaveBeenCalledWith('new tag'); }); it('does not save on Enter when mutation is pending', () => { - const props = baseProps(); - props.createRowMutation = { isPending: true }; + const contextValue = baseContextValue(); + contextValue.createRowMutation = { isPending: true }; render( - - - - -
, + + + + + +
+ , { wrapper }, ); @@ -54,18 +75,20 @@ describe('CreateRow', () => { fireEvent.change(input, { target: { value: 'pending tag' } }); fireEvent.keyDown(input, { key: 'Enter' }); - expect(props.handleCreateRow).not.toHaveBeenCalled(); + expect(contextValue.handleCreateRow).not.toHaveBeenCalled(); }); it('cancels on Escape and resets draft state', () => { - const props = baseProps(); + const contextValue = baseContextValue(); render( - - - - -
, + + + + + +
+ , { wrapper }, ); @@ -73,8 +96,8 @@ describe('CreateRow', () => { fireEvent.change(input, { target: { value: 'will cancel' } }); fireEvent.keyDown(input, { key: 'Escape' }); - expect(props.setDraftError).toHaveBeenCalledWith(''); - expect(props.setIsCreatingTopRow).toHaveBeenCalledWith(false); - expect(props.exitDraftWithoutSave).toHaveBeenCalled(); + expect(contextValue.setDraftError).toHaveBeenCalledWith(''); + expect(contextValue.setIsCreatingTopRow).toHaveBeenCalledWith(false); + expect(contextValue.exitDraftWithoutSave).toHaveBeenCalled(); }); }); diff --git a/src/taxonomy/tree-table/CreateRow.tsx b/src/taxonomy/tree-table/CreateRow.tsx index f130560b02..d16399f77b 100644 --- a/src/taxonomy/tree-table/CreateRow.tsx +++ b/src/taxonomy/tree-table/CreateRow.tsx @@ -1,42 +1,41 @@ -import React from 'react'; -import type { CreateRowMutationState } from './types'; +import React, { useContext } from 'react'; import DraftRow from './DraftRow'; +import { TreeTableContext } from './TreeTableContext'; interface CreateRowProps { - draftError: string; - setDraftError: (error: string) => void; - handleCreateRow: (value: string) => void; - setIsCreatingTopRow: (isCreating: boolean) => void; - exitDraftWithoutSave: () => void; - createRowMutation: CreateRowMutationState; + handleCreateRow?: (value: string) => void; + exitDraftWithoutSave?: () => void; indent?: number; - validate: (value: string, mode?: 'soft' | 'hard') => boolean; } const CreateRow: React.FC = ({ - draftError, - setDraftError, handleCreateRow, - setIsCreatingTopRow, exitDraftWithoutSave, - createRowMutation, indent = 0, - validate, }) => { + const { + setDraftError, + handleCreateRow: contextHandleCreateRow, + setIsCreatingTopRow, + exitDraftWithoutSave: contextExitDraftWithoutSave, + createRowMutation, + } = useContext(TreeTableContext); + + const onCreateRow = handleCreateRow ?? contextHandleCreateRow; + const onExitDraftWithoutSave = exitDraftWithoutSave ?? contextExitDraftWithoutSave; + const handleCancel = () => { setDraftError(''); setIsCreatingTopRow(false); - exitDraftWithoutSave(); + onExitDraftWithoutSave(); }; return ( diff --git a/src/taxonomy/tree-table/DraftRow.tsx b/src/taxonomy/tree-table/DraftRow.tsx index 1be3b01a2c..57b0b7ef1f 100644 --- a/src/taxonomy/tree-table/DraftRow.tsx +++ b/src/taxonomy/tree-table/DraftRow.tsx @@ -1,4 +1,4 @@ -import React, { useState } from 'react'; +import React, { useContext, useState } from 'react'; import { useIntl } from '@edx/frontend-platform/i18n'; import { Button, Spinner } from '@openedx/paragon'; import { Row } from '@tanstack/react-table'; @@ -7,15 +7,14 @@ import UsageCountDisplay from '@src/taxonomy/tag-list/UsageCountDisplay'; import { EditableCell } from './EditableCell'; import type { CreateRowMutationState, TreeRowData } from './types'; import messages from './messages'; +import { TreeTableContext } from './TreeTableContext'; interface DraftRowProps { - draftError: string; initialValue?: string; onSave: (value: string) => void; onCancel: () => void; mutationState: CreateRowMutationState; indent?: number; - validate: (value: string, mode?: 'soft' | 'hard') => boolean; requireValueChangeToEnableSave?: boolean; rowTestId?: string; rowId?: string; @@ -23,13 +22,11 @@ interface DraftRowProps { } const DraftRow: React.FC = ({ - draftError, initialValue = '', onSave, onCancel, mutationState, indent = 0, - validate, requireValueChangeToEnableSave = false, rowTestId, rowId, @@ -39,6 +36,8 @@ const DraftRow: React.FC = ({ const [saveDisabled, setSaveDisabled] = useState(true); const intl = useIntl(); + const { draftError, validate } = useContext(TreeTableContext); + const updateSaveDisabled = (value: string) => { const trimmedValue = value.trim(); const isValid = validate(value, 'soft'); diff --git a/src/taxonomy/tree-table/EditRow.tsx b/src/taxonomy/tree-table/EditRow.tsx index 12a83fa055..d4ef94d9c3 100644 --- a/src/taxonomy/tree-table/EditRow.tsx +++ b/src/taxonomy/tree-table/EditRow.tsx @@ -1,31 +1,29 @@ -import React from 'react'; +import React, { useContext } from 'react'; import { Row } from '@tanstack/react-table'; -import type { CreateRowMutationState, TreeRowData } from './types'; +import type { TreeRowData } from './types'; import DraftRow from './DraftRow'; +import { TreeTableContext } from './TreeTableContext'; interface EditRowProps { - draftError: string; - setDraftError: (error: string) => void; initialValue: string; handleUpdateRow: (value: string) => void; cancelEditRow: () => void; - updateRowMutation: CreateRowMutationState; indent?: number; - validate: (value: string, mode?: 'soft' | 'hard') => boolean; row: Row; } const EditRow: React.FC = ({ - draftError, - setDraftError, initialValue, handleUpdateRow, cancelEditRow, - updateRowMutation, indent = 0, - validate, row, }) => { + const { + setDraftError, + updateRowMutation, + } = useContext(TreeTableContext); + const handleCancel = () => { setDraftError(''); cancelEditRow(); @@ -33,13 +31,11 @@ const EditRow: React.FC = ({ return ( diff --git a/src/taxonomy/tree-table/NestedRows.test.tsx b/src/taxonomy/tree-table/NestedRows.test.tsx index ceeecf0395..4e512dea63 100644 --- a/src/taxonomy/tree-table/NestedRows.test.tsx +++ b/src/taxonomy/tree-table/NestedRows.test.tsx @@ -3,21 +3,37 @@ import { IntlProvider } from '@edx/frontend-platform/i18n'; import { fireEvent, render, screen } from '@testing-library/react'; import NestedRows from './NestedRows'; +import { TreeTableContext } from './TreeTableContext'; const wrapper = ({ children }: { children: React.ReactNode; }) => ( {children} ); -const defaultRequiredProps = { - setIsCreatingTopRow: jest.fn(), +const baseContextValue: any = () => ({ + treeData: [], + columns: [], + pageCount: -1, + pagination: { pageIndex: 0, pageSize: 10 }, + handlePaginationChange: jest.fn(), + isLoading: false, + isCreatingTopRow: false, + draftError: '', createRowMutation: {}, updateRowMutation: {}, + toast: { show: false, message: '', variant: 'success' }, + setToast: jest.fn(), + setIsCreatingTopRow: jest.fn(), + exitDraftWithoutSave: jest.fn(), + handleCreateRow: jest.fn(), + creatingParentId: null, + setCreatingParentId: jest.fn(), + setDraftError: jest.fn(), + validate: jest.fn(() => true), handleUpdateRow: jest.fn(), editingRowId: null, setEditingRowId: jest.fn(), - exitDraftWithoutSave: jest.fn(), - validate: () => true, -}; + table: null, +}); const makeCell = (id: string, content: string) => ({ id, @@ -46,16 +62,18 @@ const makeRow = ({ describe('NestedRows', () => { it('renders nothing when parent row is collapsed', () => { const parent = makeRow({ id: 1, value: 'parent', expanded: false }); + const contextValue = baseContextValue(); const { container } = render( - - - - -
, + + + + + +
+ , { wrapper }, ); @@ -70,30 +88,31 @@ describe('NestedRows', () => { expanded: true, subRows: [nestedChild], }); - const setCreatingParentId = jest.fn(); + const contextValue = baseContextValue(); + contextValue.setCreatingParentId = jest.fn(); + contextValue.creatingParentId = 2; + contextValue.createRowMutation = { isPending: false }; const onCancelCreation = jest.fn(); render( - - - - -
, + + + + + +
+ , { wrapper }, ); fireEvent.click(screen.getByText('Cancel')); - expect(setCreatingParentId).toHaveBeenCalledWith(null); + expect(contextValue.setCreatingParentId).toHaveBeenCalledWith(null); expect(onCancelCreation).toHaveBeenCalled(); }); @@ -106,18 +125,21 @@ describe('NestedRows', () => { subRows: [nestedChild], }); + const contextValue = baseContextValue(); + contextValue.editingRowId = '2:child'; + render( - - - - -
, + + + + + +
+ , { wrapper }, ); diff --git a/src/taxonomy/tree-table/NestedRows.tsx b/src/taxonomy/tree-table/NestedRows.tsx index c2647f9f98..25f3669d02 100644 --- a/src/taxonomy/tree-table/NestedRows.tsx +++ b/src/taxonomy/tree-table/NestedRows.tsx @@ -1,14 +1,10 @@ -import React from 'react'; +import React, { useContext } from 'react'; import { flexRender } from '@tanstack/react-table'; -import type { - RowId, - TreeRow, - TreeColumnDef, - CreateRowMutationState, -} from './types'; +import type { TreeRow } from './types'; import CreateRow from './CreateRow'; import EditRow from './EditRow'; +import { TreeTableContext } from './TreeTableContext'; interface NestedRowsProps { /** The parent row object from TanStack React Table */ @@ -25,33 +21,6 @@ interface NestedRowsProps { childRowsData?: TreeRow[]; /** Current nesting depth level (used for indentation calculation) */ depth?: number; - /** Error message to display in draft creation form */ - draftError?: string; - /** Setter function for draft error state */ - setDraftError?: (error: string) => void; - /** ID of the row currently in creation mode */ - creatingParentId?: RowId | null; - /** Setter function for which row is in creation mode */ - setCreatingParentId?: (value: RowId | null) => void; - /** Callback to set whether top-level row creation is active */ - setIsCreatingTopRow: (isCreating: boolean) => void; - /** State object for the row creation mutation (isPending, isError, error) */ - createRowMutation: CreateRowMutationState; - /** State object for the row update mutation (isPending, isError, error) */ - updateRowMutation: CreateRowMutationState; - /** Callback when an existing row is updated (receives new value and original value) */ - handleUpdateRow: (value: string, originalValue: string) => void; - /** ID of the row currently in edit mode */ - editingRowId: RowId | null; - /** Setter function for which row is in edit mode */ - setEditingRowId: (id: RowId | null) => void; - /** Callback to exit draft mode without saving changes */ - exitDraftWithoutSave: () => void; - /** Column definitions for rendering cells in edit mode */ - columns?: TreeColumnDef[]; - /** Validation function for new row values (receives value and optional 'soft' or 'hard' mode; - * in 'hard' mode an exception is thrown on validation failure) */ - validate: (value: string, mode?: 'soft' | 'hard') => boolean; } /** @@ -75,20 +44,16 @@ const NestedRows = ({ onCancelCreation = () => {}, childRowsData = [], depth = 1, - draftError = '', - setDraftError = () => {}, - creatingParentId = null, - setCreatingParentId = () => {}, - setIsCreatingTopRow, - createRowMutation, - updateRowMutation, - handleUpdateRow, - editingRowId, - setEditingRowId, - exitDraftWithoutSave, - columns = [], - validate, }: NestedRowsProps) => { + const { + creatingParentId, + setCreatingParentId, + handleUpdateRow, + editingRowId, + setEditingRowId, + exitDraftWithoutSave, + } = useContext(TreeTableContext); + if (!parentRow.getIsExpanded()) { return null; } @@ -98,14 +63,9 @@ const NestedRows = ({ <> {isCreating && ( onSaveNewChildRow(value, parentRowValue)} - setIsCreatingTopRow={setIsCreatingTopRow} exitDraftWithoutSave={onCancelCreation} - createRowMutation={createRowMutation} indent={indent} - validate={validate} /> )} {childRowsData?.map(row => { @@ -115,17 +75,13 @@ const NestedRows = ({ {editingRowId === `${row.original.id}:${String(row.original.value)}` ? ( handleUpdateRow(value, String(row.original.value))} cancelEditRow={() => { setEditingRowId(null); exitDraftWithoutSave(); }} - updateRowMutation={updateRowMutation} indent={indent} - validate={validate} row={row} /> ) : @@ -159,20 +115,7 @@ const NestedRows = ({ setCreatingParentId(null); onCancelCreation(); }} - creatingParentId={creatingParentId} - setCreatingParentId={setCreatingParentId} depth={depth + 1} - draftError={draftError} - setDraftError={setDraftError} - setIsCreatingTopRow={setIsCreatingTopRow} - createRowMutation={createRowMutation} - updateRowMutation={updateRowMutation} - handleUpdateRow={handleUpdateRow} - editingRowId={editingRowId} - setEditingRowId={setEditingRowId} - exitDraftWithoutSave={exitDraftWithoutSave} - columns={columns} - validate={validate} /> ); diff --git a/src/taxonomy/tree-table/SaveErrorAlert.tsx b/src/taxonomy/tree-table/SaveErrorAlert.tsx index ef0788140f..2b94d9286f 100644 --- a/src/taxonomy/tree-table/SaveErrorAlert.tsx +++ b/src/taxonomy/tree-table/SaveErrorAlert.tsx @@ -5,6 +5,7 @@ import { import { Info } from '@openedx/paragon/icons'; import { useIntl } from '@edx/frontend-platform/i18n'; +// @ts-ignore import './TableView.scss'; import messages from './messages'; @@ -12,15 +13,21 @@ interface SaveErrorAlertProps { draftError: string | undefined; isError: boolean | undefined; isUpdateError: boolean | undefined; + isAdditionalError?: boolean; } -const SaveErrorAlert = ({ draftError, isError, isUpdateError }: SaveErrorAlertProps) => { +const SaveErrorAlert = ({ + draftError, + isError, + isUpdateError, + isAdditionalError = false, +}: SaveErrorAlertProps) => { const intl = useIntl(); - const hasError: boolean = Boolean((isError || isUpdateError) && !!draftError); + const hasError: boolean = Boolean((isError || isUpdateError || isAdditionalError) && !!draftError); const [alertOpen, setAlertOpen] = React.useState(hasError); useEffect(() => { setAlertOpen(hasError); - }, [hasError, isError, isUpdateError, draftError]); + }, [hasError, isError, isUpdateError, isAdditionalError, draftError]); if (!alertOpen) { return null; } diff --git a/src/taxonomy/tree-table/TableBody.tsx b/src/taxonomy/tree-table/TableBody.tsx index 0962c4c6c7..c46df26d10 100644 --- a/src/taxonomy/tree-table/TableBody.tsx +++ b/src/taxonomy/tree-table/TableBody.tsx @@ -1,4 +1,4 @@ -import React from 'react'; +import React, { useContext } from 'react'; import { useIntl } from '@edx/frontend-platform/i18n'; import { flexRender } from '@tanstack/react-table'; @@ -6,56 +6,31 @@ import { LoadingSpinner } from '@src/generic/Loading'; import NestedRows from './NestedRows'; import messages from './messages'; +import { TreeTableContext } from './TreeTableContext'; -import type { - CreateRowMutationState, - RowId, - TreeColumnDef, - TreeTable, -} from './types'; import CreateRow from './CreateRow'; import EditRow from './EditRow'; -interface TableBodyProps { - columns: TreeColumnDef[]; - isCreatingTopRow: boolean; - draftError: string; - setIsCreatingTopRow: (isCreating: boolean) => void; - exitDraftWithoutSave: () => void; - handleCreateRow: (value: string, parentRowValue?: string) => void; - creatingParentId: RowId | null; - setCreatingParentId: (id: RowId | null) => void; - setDraftError: (error: string) => void; - createRowMutation: CreateRowMutationState; - updateRowMutation: CreateRowMutationState; - table: TreeTable; - isLoading: boolean; - validate: (value: string, mode?: 'soft' | 'hard') => boolean; - handleUpdateRow: (value: string, originalValue: string) => void; - editingRowId: RowId | null; - setEditingRowId: (id: RowId | null) => void; -} - -const TableBody = ({ - columns, - isCreatingTopRow, - draftError, - handleCreateRow, - setIsCreatingTopRow, - exitDraftWithoutSave, - creatingParentId, - setCreatingParentId, - setDraftError, - createRowMutation, - updateRowMutation, - table, - isLoading, - validate, - handleUpdateRow, - editingRowId, - setEditingRowId, -}: TableBodyProps) => { +const TableBody = () => { const intl = useIntl(); + const { + columns, + isCreatingTopRow, + handleCreateRow, + exitDraftWithoutSave, + creatingParentId, + setCreatingParentId, + setDraftError, + table, + isLoading, + handleUpdateRow, + editingRowId, + setEditingRowId, + } = useContext(TreeTableContext); + + if (!table) { + return null; + } if (isLoading) { return ( @@ -79,33 +54,19 @@ const TableBody = ({ )} - {isCreatingTopRow && ( - - )} + {isCreatingTopRow && } {table.getRowModel().rows.filter(row => row.depth === 0).map(row => ( {editingRowId === `${row.original.id}:${String(row.original.value)}` ? ( handleUpdateRow(value, String(row.original.value))} cancelEditRow={() => { setEditingRowId(null); exitDraftWithoutSave(); }} - updateRowMutation={updateRowMutation} - validate={validate} row={row} /> ) : @@ -130,20 +91,6 @@ const TableBody = ({ setCreatingParentId(null); exitDraftWithoutSave(); }} - creatingParentId={creatingParentId} - setCreatingParentId={setCreatingParentId} - depth={1} - draftError={draftError} - createRowMutation={createRowMutation} - setDraftError={setDraftError} - setIsCreatingTopRow={setIsCreatingTopRow} - validate={validate} - updateRowMutation={updateRowMutation} - handleUpdateRow={handleUpdateRow} - editingRowId={editingRowId} - setEditingRowId={setEditingRowId} - exitDraftWithoutSave={exitDraftWithoutSave} - columns={columns} /> ))} diff --git a/src/taxonomy/tree-table/TableView.test.tsx b/src/taxonomy/tree-table/TableView.test.tsx index eca55131b7..83a4751775 100644 --- a/src/taxonomy/tree-table/TableView.test.tsx +++ b/src/taxonomy/tree-table/TableView.test.tsx @@ -2,6 +2,7 @@ import React from 'react'; import { IntlProvider } from '@edx/frontend-platform/i18n'; import { fireEvent, render, screen } from '@testing-library/react'; +import { TreeTableContext } from './TreeTableContext'; import { TableView } from './TableView'; jest.mock('./TableBody', () => { @@ -19,7 +20,7 @@ const wrapper = ({ children }: { children: React.ReactNode; }) => ( {children} ); -const baseProps = () => ({ +const baseContextValue = () => ({ treeData: [{ id: 1, value: 'root' }], columns: [{ accessorKey: 'value', header: 'Tag name', cell: (info: any) => info.getValue() }], pageCount: 3, @@ -42,15 +43,27 @@ const baseProps = () => ({ handleUpdateRow: jest.fn(), editingRowId: null, setEditingRowId: jest.fn(), + table: null, }); +const renderTableView = ( + contextValue = baseContextValue(), + props: React.ComponentProps = {}, +) => + render( + + + , + { wrapper }, + ); + describe('TableView', () => { it('shows and dismisses save error banner', () => { - const props = baseProps(); - props.createRowMutation = { isPending: false, isError: true }; - props.draftError = 'Request failed with status code 500'; + const contextValue = baseContextValue(); + contextValue.createRowMutation = { isPending: false, isError: true }; + contextValue.draftError = 'Request failed with status code 500'; - render(, { wrapper }); + renderTableView(contextValue); expect(screen.getByText('Error saving changes')).toBeInTheDocument(); fireEvent.click(screen.getByRole('button', { name: /dismiss/i })); @@ -58,42 +71,73 @@ describe('TableView', () => { }); it('keeps pagination hidden by default even when multiple pages are reported', () => { - const props = baseProps(); - render(, { wrapper }); + renderTableView(); expect(screen.queryByRole('navigation', { name: /table pagination/i })).not.toBeInTheDocument(); }); it('renders pagination and updates page selection when explicitly enabled', () => { - const props = baseProps(); - render(, { wrapper }); + const contextValue = baseContextValue(); + renderTableView(contextValue, { enablePagination: true }); expect(screen.getByText('Page 1 of 3')).toBeInTheDocument(); fireEvent.click(screen.getByRole('button', { name: /^page 2$/i })); - expect(props.handlePaginationChange).toHaveBeenCalled(); + expect(contextValue.handlePaginationChange).toHaveBeenCalled(); }); it('hides pagination when there is only one page', () => { - const props = baseProps(); - props.pageCount = 1; - render(, { wrapper }); + const contextValue = baseContextValue(); + contextValue.pageCount = 1; + renderTableView(contextValue); expect(screen.queryByRole('navigation', { name: /table pagination/i })).not.toBeInTheDocument(); }); it('closes toast by setting show to false', () => { - const props = baseProps(); - props.toast = { show: true, message: 'created', variant: 'success' }; + const contextValue = baseContextValue(); + contextValue.toast = { show: true, message: 'created', variant: 'success' }; - render(, { wrapper }); + renderTableView(contextValue); fireEvent.click(screen.getByRole('button', { name: /close/i })); - expect(props.setToast).toHaveBeenCalled(); - const updater = props.setToast.mock.calls[0][0]; + expect(contextValue.setToast).toHaveBeenCalled(); + const updater = contextValue.setToast.mock.calls[0][0]; expect(updater({ show: true, message: 'created', variant: 'success' })).toEqual({ show: false, message: 'created', variant: 'success', }); }); + + it('shows the save error alert when a delete mutation fails and draftError is present', () => { + const contextValue = baseContextValue(); + contextValue.draftError = 'Delete request failed'; + + renderTableView(contextValue, { hasAdditionalError: true }); + + expect(screen.getByText('Error saving changes')).toBeInTheDocument(); + expect(screen.getByText('Delete request failed. Please try again.')).toBeInTheDocument(); + }); + + it('reopens the save error alert when a new delete error arrives after the user previously dismissed it', () => { + const contextValue = baseContextValue(); + contextValue.draftError = 'First delete failure'; + + const { rerender } = renderTableView(contextValue, { hasAdditionalError: true }); + + fireEvent.click(screen.getByRole('button', { name: /dismiss/i })); + expect(screen.queryByText('Error saving changes')).not.toBeInTheDocument(); + + const nextContextValue = baseContextValue(); + nextContextValue.draftError = 'Second delete failure'; + + rerender( + + + , + ); + + expect(screen.getByText('Error saving changes')).toBeInTheDocument(); + expect(screen.getByText('Second delete failure. Please try again.')).toBeInTheDocument(); + }); }); diff --git a/src/taxonomy/tree-table/TableView.tsx b/src/taxonomy/tree-table/TableView.tsx index 707792f452..9f2d13f637 100644 --- a/src/taxonomy/tree-table/TableView.tsx +++ b/src/taxonomy/tree-table/TableView.tsx @@ -1,4 +1,4 @@ -import React from 'react'; +import React, { useContext, useMemo } from 'react'; import { Button, Toast, @@ -13,78 +13,45 @@ import { getCoreRowModel, getExpandedRowModel, flexRender, - type OnChangeFn, - type PaginationState, } from '@tanstack/react-table'; import { ArrowDropUpDown } from '@openedx/paragon/icons'; import { useIntl } from '@edx/frontend-platform/i18n'; import TableBody from './TableBody'; +// @ts-ignore import './TableView.scss'; -import type { - CreateRowMutationState, - RowId, - ToastState, - TreeColumnDef, - TreeRowData, -} from './types'; import messages from './messages'; import SaveErrorAlert from './SaveErrorAlert'; +import { TreeTableContext } from './TreeTableContext'; +import { TreeTable } from './types'; interface TableViewProps { - treeData: TreeRowData[]; - columns: TreeColumnDef[]; - pageCount: number; enablePagination?: boolean; - pagination: PaginationState; - handlePaginationChange: OnChangeFn; - isLoading: boolean; - isCreatingTopRow: boolean; - draftError: string; - createRowMutation: CreateRowMutationState; - updateRowMutation: CreateRowMutationState; - toast: ToastState; - setToast: React.Dispatch>; - setIsCreatingTopRow: (isCreating: boolean) => void; - exitDraftWithoutSave: () => void; - handleCreateRow: (value: string, parentRowValue?: string) => void; - creatingParentId: RowId | null; - setCreatingParentId: (id: RowId | null) => void; - setDraftError: (error: string) => void; - validate: (value: string, mode?: 'soft' | 'hard') => boolean; - handleUpdateRow: (value: string, originalValue: string) => void; - editingRowId: RowId | null; - setEditingRowId: (id: RowId | null) => void; + hasAdditionalError?: boolean; } const TableView = ({ - treeData, - columns, - pageCount, enablePagination = false, - pagination, - handlePaginationChange, - isLoading, - isCreatingTopRow, - draftError, - createRowMutation, - updateRowMutation, - handleCreateRow, - toast, - setToast, - setIsCreatingTopRow, - exitDraftWithoutSave, - creatingParentId, - setCreatingParentId, - setDraftError, - validate, - handleUpdateRow, - editingRowId, - setEditingRowId, + hasAdditionalError = false, }: TableViewProps) => { const intl = useIntl(); - const table = useReactTable({ + const contextValue = useContext(TreeTableContext); + + const { + treeData, + columns, + pageCount, + pagination, + handlePaginationChange, + draftError, + createRowMutation, + updateRowMutation, + toast, + setToast, + } = contextValue; + + const table: TreeTable = useReactTable({ data: treeData, columns, getCoreRowModel: getCoreRowModel(), @@ -98,14 +65,27 @@ const TableView = ({ getSubRows: (row) => row?.subRows || undefined, }); + const nestedContextValue = useMemo(() => ({ + ...contextValue, + table, + }), [contextValue, table]); + const currentPageIndex = table.getState().pagination.pageIndex + 1; const { isError } = createRowMutation; const { isError: isUpdateError } = updateRowMutation; return ( - <> - + // This is a nested context provider. It is a valid pattern in React even if it looks odd, + // and the purpose here is to add the react-table instance to the overall context. + // Note that there is an outer context provider higher up in `TagListTable` as well. + +
@@ -145,25 +125,7 @@ const TableView = ({ ))} - + @@ -200,7 +162,7 @@ const TableView = ({ {toast.message} - + ); }; diff --git a/src/taxonomy/tree-table/TreeTableContext.tsx b/src/taxonomy/tree-table/TreeTableContext.tsx new file mode 100644 index 0000000000..da6dcdcedb --- /dev/null +++ b/src/taxonomy/tree-table/TreeTableContext.tsx @@ -0,0 +1,64 @@ +import { createContext } from 'react'; +import type { Dispatch, SetStateAction } from 'react'; +import type { OnChangeFn, PaginationState } from '@tanstack/react-table'; + +import type { + CreateRowMutationState, + RowId, + ToastState, + TreeColumnDef, + TreeTable, + TreeRowData, +} from './types'; + +export interface TreeTableContextValue { + treeData: TreeRowData[]; + columns: TreeColumnDef[]; + pageCount: number; + pagination: PaginationState; + handlePaginationChange: OnChangeFn; + isLoading: boolean; + isCreatingTopRow: boolean; + draftError: string; + createRowMutation: CreateRowMutationState; + updateRowMutation: CreateRowMutationState; + toast: ToastState; + setToast: Dispatch>; + setIsCreatingTopRow: (isCreating: boolean) => void; + exitDraftWithoutSave: () => void; + handleCreateRow: (value: string, parentRowValue?: string) => void; + creatingParentId: RowId | null; + setCreatingParentId: (id: RowId | null) => void; + setDraftError: (error: string) => void; + validate: (value: string, mode?: 'soft' | 'hard') => boolean; + handleUpdateRow: (value: string, originalValue: string) => void; + editingRowId: RowId | null; + setEditingRowId: (id: RowId | null) => void; + table: TreeTable | null; +} + +export const TreeTableContext = createContext({ + treeData: [], + columns: [], + pageCount: -1, + pagination: { pageIndex: 0, pageSize: 0 }, + handlePaginationChange: () => {}, + isLoading: false, + isCreatingTopRow: false, + draftError: '', + createRowMutation: {}, + updateRowMutation: {}, + toast: { show: false, message: '', variant: 'success' }, + setToast: () => {}, + setIsCreatingTopRow: () => {}, + exitDraftWithoutSave: () => {}, + handleCreateRow: () => {}, + creatingParentId: null, + setCreatingParentId: () => {}, + setDraftError: () => {}, + validate: () => true, + handleUpdateRow: () => {}, + editingRowId: null, + setEditingRowId: () => {}, + table: null, +}); diff --git a/src/taxonomy/tree-table/index.ts b/src/taxonomy/tree-table/index.ts index eca12d684c..44b27a7709 100644 --- a/src/taxonomy/tree-table/index.ts +++ b/src/taxonomy/tree-table/index.ts @@ -1,2 +1,3 @@ export { EditableCell } from './EditableCell'; export { TableView } from './TableView'; +export { TreeTableContext } from './TreeTableContext'; From 4f8d1b7b1222c4c5c35c43e8f1a1129f98d632df Mon Sep 17 00:00:00 2001 From: Jesper Hodge Date: Thu, 23 Apr 2026 21:15:05 -0400 Subject: [PATCH 2/9] fix: lint --- src/taxonomy/tag-list/TagListTable.test.jsx | 8 ++++---- src/taxonomy/tag-list/hooks.ts | 3 ++- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/taxonomy/tag-list/TagListTable.test.jsx b/src/taxonomy/tag-list/TagListTable.test.jsx index 0f31b59189..c74b325755 100644 --- a/src/taxonomy/tag-list/TagListTable.test.jsx +++ b/src/taxonomy/tag-list/TagListTable.test.jsx @@ -1,6 +1,5 @@ import React from 'react'; import { AxiosError } from 'axios'; -import userEvent from '@testing-library/user-event'; import { render, waitFor, @@ -12,9 +11,9 @@ import { cleanup, initializeMocks, } from '@src/testUtils'; -import * as apiHooksModule from '../data/apiHooks'; +import * as apiHooksModule from '@src/taxonomy/data/apiHooks'; +import * as treeTableModule from '@src/taxonomy/tree-table'; import * as hooksModule from './hooks'; -import * as treeTableModule from '../tree-table'; import TagListTable from './TagListTable'; let axiosMock; @@ -117,7 +116,8 @@ const subTagsUrl = 'http://localhost:18010/api/content_tagging/v1/taxonomies/1/tags/?full_depth_threshold=10000&parent_tag=root+tag+1'; const createTagUrl = 'http://localhost:18010/api/content_tagging/v1/taxonomies/1/tags/'; const deleteTagUrl = createTagUrl; -const deleteConfirmMessage = 'Warning: are you sure you want to delete this tag and all its subtags and descendants? Any tags applied to course content will be deleted.'; +const deleteConfirmMessage = + 'Warning: are you sure you want to delete this tag and all its subtags and descendants? Any tags applied to course content will be deleted.'; const renderTagListTable = (maxDepth = 3) => render(); diff --git a/src/taxonomy/tag-list/hooks.ts b/src/taxonomy/tag-list/hooks.ts index d0bbb942f1..f66dc82bc3 100644 --- a/src/taxonomy/tag-list/hooks.ts +++ b/src/taxonomy/tag-list/hooks.ts @@ -18,7 +18,8 @@ import messages from './messages'; import { getTagListRowData, getTagWithDescendantsCount } from './utils'; import { Row } from '@tanstack/react-table'; -const DELETE_CONFIRM_MESSAGE = 'Warning: are you sure you want to delete this tag and all its subtags and descendants? Any tags applied to course content will be deleted.'; +const DELETE_CONFIRM_MESSAGE = + 'Warning: are you sure you want to delete this tag and all its subtags and descendants? Any tags applied to course content will be deleted.'; /** Interface for table mode actions for React's `useReducer` hook. * From 496e69fe96bd2f2f34cdf45cef0a834a82e43b7d Mon Sep 17 00:00:00 2001 From: Mary Gwozdz Date: Tue, 28 Apr 2026 11:43:18 -0700 Subject: [PATCH 3/9] fix: cleanup from comments on PR #3025 --- src/taxonomy/tag-list/TagListTable.test.jsx | 5 ----- src/taxonomy/tag-list/TagListTable.tsx | 4 ++-- src/taxonomy/tag-list/hooks.ts | 6 +++--- src/taxonomy/tree-table/SaveErrorAlert.tsx | 8 ++++---- src/taxonomy/tree-table/TableView.test.tsx | 6 +++--- src/taxonomy/tree-table/TableView.tsx | 6 +++--- 6 files changed, 15 insertions(+), 20 deletions(-) diff --git a/src/taxonomy/tag-list/TagListTable.test.jsx b/src/taxonomy/tag-list/TagListTable.test.jsx index c74b325755..c61fefd347 100644 --- a/src/taxonomy/tag-list/TagListTable.test.jsx +++ b/src/taxonomy/tag-list/TagListTable.test.jsx @@ -982,11 +982,6 @@ describe('', () => { }); it('should disable delete action if tag includes `can_delete: false`', async () => { - axiosMock.reset(); - axiosMock - .onGet(rootTagsListUrl) - .reply(200, mockTagResponseDisallowingEdits); - axiosMock.onGet(subTagsUrl).reply(200, subTagsResponse); cleanup(); ({ axiosMock } = initializeMocks({ user: adminUser })); axiosMock diff --git a/src/taxonomy/tag-list/TagListTable.tsx b/src/taxonomy/tag-list/TagListTable.tsx index eb5317e96c..ebe0679c7a 100644 --- a/src/taxonomy/tag-list/TagListTable.tsx +++ b/src/taxonomy/tag-list/TagListTable.tsx @@ -46,7 +46,7 @@ const TagListTable = ({ taxonomyId, maxDepth }: TagListTableProps) => { const [toast, setToast] = useState({ show: false, message: '', variant: 'success' }); const [tagTree, setTagTree] = useState(null); const [isCreatingTopTag, setIsCreatingTopTag] = useState(false); - const [, setActiveActionMenuRowId] = useState(null); + const [_, setActiveActionMenuRowId] = useState(null); const [draftError, setDraftError] = useState(''); const treeData = (tagTree?.getAllAsDeepCopy() || []) as unknown as TreeRowData[]; const hasOpenDraft = isCreatingTopTag || creatingParentId !== null || editingRowId !== null; @@ -154,7 +154,7 @@ const TagListTable = ({ taxonomyId, maxDepth }: TagListTableProps) => { return ( - + ); }; diff --git a/src/taxonomy/tag-list/hooks.ts b/src/taxonomy/tag-list/hooks.ts index f66dc82bc3..6d27b92f09 100644 --- a/src/taxonomy/tag-list/hooks.ts +++ b/src/taxonomy/tag-list/hooks.ts @@ -291,9 +291,9 @@ const useEditActions = ({ const handleDeleteTag = async (row: Row) => { const rowData = getTagListRowData(row); const count = getTagWithDescendantsCount(rowData); - // If the tag in the frontend state does not have subtags, - // don't allow the backend to delete subtags. - // That prevents problems in case of stale frontend state. + // Only request recursive deletion when the frontend has loaded descendants. + // If this state is stale and the backend finds subtags while with_subtags is false, + // the backend rejects the request instead of deleting the parent alone. const shouldDeleteSubtags = count > 1; try { // In view mode, the table reloads on change, reflecting the deletion diff --git a/src/taxonomy/tree-table/SaveErrorAlert.tsx b/src/taxonomy/tree-table/SaveErrorAlert.tsx index 2b94d9286f..a3246ab8cf 100644 --- a/src/taxonomy/tree-table/SaveErrorAlert.tsx +++ b/src/taxonomy/tree-table/SaveErrorAlert.tsx @@ -13,21 +13,21 @@ interface SaveErrorAlertProps { draftError: string | undefined; isError: boolean | undefined; isUpdateError: boolean | undefined; - isAdditionalError?: boolean; + isDeleteError?: boolean; } const SaveErrorAlert = ({ draftError, isError, isUpdateError, - isAdditionalError = false, + isDeleteError = false, }: SaveErrorAlertProps) => { const intl = useIntl(); - const hasError: boolean = Boolean((isError || isUpdateError || isAdditionalError) && !!draftError); + const hasError: boolean = Boolean((isError || isUpdateError || isDeleteError) && !!draftError); const [alertOpen, setAlertOpen] = React.useState(hasError); useEffect(() => { setAlertOpen(hasError); - }, [hasError, isError, isUpdateError, isAdditionalError, draftError]); + }, [hasError, isError, isUpdateError, isDeleteError, draftError]); if (!alertOpen) { return null; } diff --git a/src/taxonomy/tree-table/TableView.test.tsx b/src/taxonomy/tree-table/TableView.test.tsx index 83a4751775..06dbcb762f 100644 --- a/src/taxonomy/tree-table/TableView.test.tsx +++ b/src/taxonomy/tree-table/TableView.test.tsx @@ -113,7 +113,7 @@ describe('TableView', () => { const contextValue = baseContextValue(); contextValue.draftError = 'Delete request failed'; - renderTableView(contextValue, { hasAdditionalError: true }); + renderTableView(contextValue, { hasDeleteError: true }); expect(screen.getByText('Error saving changes')).toBeInTheDocument(); expect(screen.getByText('Delete request failed. Please try again.')).toBeInTheDocument(); @@ -123,7 +123,7 @@ describe('TableView', () => { const contextValue = baseContextValue(); contextValue.draftError = 'First delete failure'; - const { rerender } = renderTableView(contextValue, { hasAdditionalError: true }); + const { rerender } = renderTableView(contextValue, { hasDeleteError: true }); fireEvent.click(screen.getByRole('button', { name: /dismiss/i })); expect(screen.queryByText('Error saving changes')).not.toBeInTheDocument(); @@ -133,7 +133,7 @@ describe('TableView', () => { rerender( - + , ); diff --git a/src/taxonomy/tree-table/TableView.tsx b/src/taxonomy/tree-table/TableView.tsx index 9f2d13f637..c1a979ba3a 100644 --- a/src/taxonomy/tree-table/TableView.tsx +++ b/src/taxonomy/tree-table/TableView.tsx @@ -27,12 +27,12 @@ import { TreeTable } from './types'; interface TableViewProps { enablePagination?: boolean; - hasAdditionalError?: boolean; + hasDeleteError?: boolean; } const TableView = ({ enablePagination = false, - hasAdditionalError = false, + hasDeleteError = false, }: TableViewProps) => { const intl = useIntl(); @@ -84,7 +84,7 @@ const TableView = ({ draftError={draftError} isError={isError} isUpdateError={isUpdateError} - isAdditionalError={hasAdditionalError} + isDeleteError={hasDeleteError} /> From 957755d949857e18280964e6da834b9ce085b2f4 Mon Sep 17 00:00:00 2001 From: Mary Gwozdz Date: Tue, 28 Apr 2026 12:17:59 -0700 Subject: [PATCH 4/9] fix: implement disableTagActions logic --- src/taxonomy/tag-list/Actions.tsx | 4 +- src/taxonomy/tag-list/TagListTable.tsx | 2 + src/taxonomy/tag-list/hooks.test.tsx | 69 +++++++++++++++++++++++ src/taxonomy/tag-list/hooks.ts | 2 +- src/taxonomy/tag-list/tagColumns.test.tsx | 50 ++++++++++++++++ src/taxonomy/tag-list/tagColumns.tsx | 9 ++- 6 files changed, 131 insertions(+), 5 deletions(-) create mode 100644 src/taxonomy/tag-list/tagColumns.test.tsx diff --git a/src/taxonomy/tag-list/Actions.tsx b/src/taxonomy/tag-list/Actions.tsx index 0740ed39ef..dbb0930def 100644 --- a/src/taxonomy/tag-list/Actions.tsx +++ b/src/taxonomy/tag-list/Actions.tsx @@ -25,6 +25,7 @@ interface ActionsHeaderProps { setEditingRowId: (id: RowId | null) => void; setActiveActionMenuRowId: (id: RowId | null) => void; hasOpenDraft: boolean; + disableTagActions: boolean; draftInProgressHintId: string; canAddTag: boolean; } @@ -36,6 +37,7 @@ const ActionsHeader = ({ setEditingRowId, setActiveActionMenuRowId, hasOpenDraft, + disableTagActions, canAddTag, draftInProgressHintId, }: ActionsHeaderProps) => { @@ -55,7 +57,7 @@ const ActionsHeader = ({ setEditingRowId(null); setActiveActionMenuRowId(null); }} - disabled={hasOpenDraft || !canAddTag} + disabled={disableTagActions || !canAddTag} aria-describedby={hasOpenDraft ? draftInProgressHintId : undefined} />
diff --git a/src/taxonomy/tag-list/TagListTable.tsx b/src/taxonomy/tag-list/TagListTable.tsx index ebe0679c7a..c7e1e5479f 100644 --- a/src/taxonomy/tag-list/TagListTable.tsx +++ b/src/taxonomy/tag-list/TagListTable.tsx @@ -83,6 +83,7 @@ const TagListTable = ({ taxonomyId, maxDepth }: TagListTableProps) => { const createTagMutation = useCreateTag(taxonomyId); const updateTagMutation = useUpdateTag(taxonomyId); const deleteTagMutation = useDeleteTag(taxonomyId); + const disableTagActions = hasOpenDraft || deleteTagMutation.isPending; const pageCount = tagList?.numPages ?? -1; const canAddTag = tagList?.canAddTag !== false; @@ -143,6 +144,7 @@ const TagListTable = ({ taxonomyId, maxDepth }: TagListTableProps) => { onStartDraft: enterDraftMode, setActiveActionMenuRowId, hasOpenDraft, + disableTagActions, canAddTag, maxDepth, }; diff --git a/src/taxonomy/tag-list/hooks.test.tsx b/src/taxonomy/tag-list/hooks.test.tsx index f9e8ec4f0e..99ae1973d1 100644 --- a/src/taxonomy/tag-list/hooks.test.tsx +++ b/src/taxonomy/tag-list/hooks.test.tsx @@ -42,6 +42,10 @@ describe('useTableModes', () => { }); describe('useEditActions', () => { + afterEach(() => { + jest.restoreAllMocks(); + }); + const buildActions = (overrides = {}) => { const createTagMutation = { mutateAsync: jest.fn() }; const updateTagMutation = { mutateAsync: jest.fn() }; @@ -177,4 +181,69 @@ describe('useEditActions', () => { message: 'Error creating tag: server failed', }); }); + + it('enters view mode only after a delete request succeeds', async () => { + const { + actions, + deleteTagMutation, + enterViewMode, + setToast, + } = buildActions(); + deleteTagMutation.mutateAsync.mockResolvedValue(undefined); + jest.spyOn(window, 'confirm').mockReturnValue(true); + + actions.startDeleteRow({ + original: { + id: 1, + value: 'tag to delete', + depth: 0, + childCount: 0, + }, + } as any); + + await waitFor(() => { + expect(enterViewMode).toHaveBeenCalled(); + }); + expect(deleteTagMutation.mutateAsync).toHaveBeenCalledWith({ + value: 'tag to delete', + withSubtags: false, + }); + expect(deleteTagMutation.mutateAsync.mock.invocationCallOrder[0]).toBeLessThan( + enterViewMode.mock.invocationCallOrder[0], + ); + expect(setToast).toHaveBeenCalledWith({ + show: true, + message: '1 tag(s) deleted. This change will be applied across all tagged content.', + }); + }); + + it('does not enter view mode when a delete request fails', async () => { + const { + actions, + deleteTagMutation, + enterViewMode, + setDraftError, + setToast, + } = buildActions(); + deleteTagMutation.mutateAsync.mockRejectedValue(new Error('server failed')); + jest.spyOn(window, 'confirm').mockReturnValue(true); + + actions.startDeleteRow({ + original: { + id: 1, + value: 'tag to delete', + depth: 0, + childCount: 0, + }, + } as any); + + await waitFor(() => { + expect(setDraftError).toHaveBeenCalledWith('server failed'); + }); + expect(enterViewMode).not.toHaveBeenCalled(); + expect(setToast).toHaveBeenCalledWith({ + show: true, + message: 'Error deleting tag: server failed', + }); + }); }); diff --git a/src/taxonomy/tag-list/hooks.ts b/src/taxonomy/tag-list/hooks.ts index 6d27b92f09..e4a7c95bf5 100644 --- a/src/taxonomy/tag-list/hooks.ts +++ b/src/taxonomy/tag-list/hooks.ts @@ -296,10 +296,10 @@ const useEditActions = ({ // the backend rejects the request instead of deleting the parent alone. const shouldDeleteSubtags = count > 1; try { + await deleteTagMutation.mutateAsync({ value: rowData.value, withSubtags: shouldDeleteSubtags }); // In view mode, the table reloads on change, reflecting the deletion // without needing to manually update the table state enterViewMode(); - await deleteTagMutation.mutateAsync({ value: rowData.value, withSubtags: shouldDeleteSubtags }); setToast({ show: true, message: intl.formatMessage(messages.tagsDeleteSuccessMessage, { count }), diff --git a/src/taxonomy/tag-list/tagColumns.test.tsx b/src/taxonomy/tag-list/tagColumns.test.tsx new file mode 100644 index 0000000000..c3d4f92f7a --- /dev/null +++ b/src/taxonomy/tag-list/tagColumns.test.tsx @@ -0,0 +1,50 @@ +import React from 'react'; + +import { getColumns } from './tagColumns'; + +const baseArgs = (overrides = {}) => ({ + setIsCreatingTopRow: jest.fn(), + setEditingRowId: jest.fn(), + onStartDraft: jest.fn(), + setActiveActionMenuRowId: jest.fn(), + hasOpenDraft: false, + disableTagActions: false, + canAddTag: true, + setDraftError: jest.fn(), + maxDepth: 3, + startSubtagDraft: jest.fn(), + startEditRow: jest.fn(), + startDeleteRow: jest.fn(), + ...overrides, +}); + +const getActionsColumn = (overrides = {}) => { + const columns = getColumns(baseArgs(overrides)); + return columns.find(column => column.id === 'actions'); +}; + +describe('tagColumns', () => { + it('disables create, edit, and delete actions when tag actions are disabled', () => { + const actionsColumn = getActionsColumn({ disableTagActions: true }); + const headerElement = (actionsColumn?.header as any)(); + const cellElement = (actionsColumn?.cell as any)({ + row: { + depth: 0, + original: { + id: 1, + value: 'root', + depth: 0, + childCount: 0, + canChangeTag: true, + canDeleteTag: true, + }, + }, + }); + const menuElement = React.Children.only(cellElement.props.children) as React.ReactElement; + + expect(headerElement.props.disableTagActions).toBe(true); + expect(menuElement.props.disableAddSubtag).toBe(true); + expect(menuElement.props.disableEditRow).toBe(true); + expect(menuElement.props.disableDeleteRow).toBe(true); + }); +}); diff --git a/src/taxonomy/tag-list/tagColumns.tsx b/src/taxonomy/tag-list/tagColumns.tsx index 800355a761..c1ee3e0c0a 100644 --- a/src/taxonomy/tag-list/tagColumns.tsx +++ b/src/taxonomy/tag-list/tagColumns.tsx @@ -20,6 +20,7 @@ interface GetColumnsArgs { onStartDraft: () => void; setActiveActionMenuRowId: (id: RowId | null) => void; hasOpenDraft: boolean; + disableTagActions: boolean; canAddTag: boolean; setDraftError: (error: string) => void; maxDepth: number; @@ -37,6 +38,7 @@ function getColumns({ startDeleteRow, setActiveActionMenuRowId, hasOpenDraft, + disableTagActions, canAddTag, setDraftError, maxDepth, @@ -76,6 +78,7 @@ function getColumns({ setEditingRowId={setEditingRowId} setActiveActionMenuRowId={setActiveActionMenuRowId} hasOpenDraft={hasOpenDraft} + disableTagActions={disableTagActions} draftInProgressHintId={draftInProgressHintId} canAddTag={canAddTag} /> @@ -87,9 +90,9 @@ function getColumns({ return
; } - const disableAddSubtag = hasOpenDraft || !canAddTag; - const disableEditRow = hasOpenDraft || rowData.canChangeTag === false; - const disableDeleteRow = hasOpenDraft || rowData.canDeleteTag === false; + const disableAddSubtag = disableTagActions || !canAddTag; + const disableEditRow = disableTagActions || rowData.canChangeTag === false; + const disableDeleteRow = disableTagActions || rowData.canDeleteTag === false; return (
From 594a3b456ccefa95dc501aa1ecd5177fdef57906 Mon Sep 17 00:00:00 2001 From: Mary Gwozdz Date: Tue, 28 Apr 2026 12:26:21 -0700 Subject: [PATCH 5/9] fix: remove unused variable --- src/taxonomy/tag-list/TagListTable.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/taxonomy/tag-list/TagListTable.tsx b/src/taxonomy/tag-list/TagListTable.tsx index c7e1e5479f..cbd3e357ec 100644 --- a/src/taxonomy/tag-list/TagListTable.tsx +++ b/src/taxonomy/tag-list/TagListTable.tsx @@ -46,7 +46,7 @@ const TagListTable = ({ taxonomyId, maxDepth }: TagListTableProps) => { const [toast, setToast] = useState({ show: false, message: '', variant: 'success' }); const [tagTree, setTagTree] = useState(null); const [isCreatingTopTag, setIsCreatingTopTag] = useState(false); - const [_, setActiveActionMenuRowId] = useState(null); + const [, setActiveActionMenuRowId] = useState(null); const [draftError, setDraftError] = useState(''); const treeData = (tagTree?.getAllAsDeepCopy() || []) as unknown as TreeRowData[]; const hasOpenDraft = isCreatingTopTag || creatingParentId !== null || editingRowId !== null; From 2ebe17abc7d42631ed9296dc5983a9a05766d504 Mon Sep 17 00:00:00 2001 From: Mary Gwozdz Date: Tue, 28 Apr 2026 13:14:31 -0700 Subject: [PATCH 6/9] fix: fix unsafe optional chaining lint --- src/taxonomy/tag-list/tagColumns.test.tsx | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/taxonomy/tag-list/tagColumns.test.tsx b/src/taxonomy/tag-list/tagColumns.test.tsx index c3d4f92f7a..dbe06a69d5 100644 --- a/src/taxonomy/tag-list/tagColumns.test.tsx +++ b/src/taxonomy/tag-list/tagColumns.test.tsx @@ -26,8 +26,13 @@ const getActionsColumn = (overrides = {}) => { describe('tagColumns', () => { it('disables create, edit, and delete actions when tag actions are disabled', () => { const actionsColumn = getActionsColumn({ disableTagActions: true }); - const headerElement = (actionsColumn?.header as any)(); - const cellElement = (actionsColumn?.cell as any)({ + + if (!actionsColumn) { + throw new Error('Actions column not found'); + } + + const headerElement = (actionsColumn.header as any)(); + const cellElement = (actionsColumn.cell as any)({ row: { depth: 0, original: { From fc2fccc6f441dcbec9a99358dc4f88d3a718407e Mon Sep 17 00:00:00 2001 From: Mary Gwozdz Date: Wed, 29 Apr 2026 14:39:04 -0700 Subject: [PATCH 7/9] feat: Add delete modal to delete functionality --- src/taxonomy/tag-list/DeleteModal.test.tsx | 12 +++--- src/taxonomy/tag-list/DeleteModal.tsx | 4 +- src/taxonomy/tag-list/TagListTable.test.jsx | 48 +++++++++++---------- src/taxonomy/tag-list/TagListTable.tsx | 19 +++++++- src/taxonomy/tag-list/hooks.test.tsx | 6 +-- src/taxonomy/tag-list/hooks.ts | 13 +----- src/taxonomy/tag-list/messages.ts | 1 + 7 files changed, 58 insertions(+), 45 deletions(-) diff --git a/src/taxonomy/tag-list/DeleteModal.test.tsx b/src/taxonomy/tag-list/DeleteModal.test.tsx index 3d41bb172a..8153150774 100644 --- a/src/taxonomy/tag-list/DeleteModal.test.tsx +++ b/src/taxonomy/tag-list/DeleteModal.test.tsx @@ -1,7 +1,7 @@ import React from 'react'; import userEvent from '@testing-library/user-event'; import { IntlProvider } from '@edx/frontend-platform/i18n'; -import { render, screen, within } from '@testing-library/react'; +import { render, screen, waitFor, within } from '@testing-library/react'; import DeleteModal from './DeleteModal'; @@ -94,7 +94,7 @@ describe('DeleteModal', () => { it('calls handleDeleteRow with the dialog row context and then closes and clears the dialog state on confirm', async () => { const user = userEvent.setup(); const row = createRow(leafRowData); - const handleDeleteRow = jest.fn(); + const handleDeleteRow = jest.fn().mockResolvedValue(undefined); const setIsOpen = jest.fn(); const setRow = jest.fn(); @@ -108,9 +108,11 @@ describe('DeleteModal', () => { await user.type(screen.getByRole('textbox'), 'DELETE'); await user.click(screen.getByRole('button', { name: 'Delete Tag' })); - expect(handleDeleteRow).toHaveBeenCalledWith(row); - expect(setIsOpen).toHaveBeenCalledWith(false); - expect(setRow).toHaveBeenCalledWith(null); + await waitFor(() => { + expect(handleDeleteRow).toHaveBeenCalledWith(row); + expect(setIsOpen).toHaveBeenCalledWith(false); + expect(setRow).toHaveBeenCalledWith(null); + }); }); it('closes and clears the dialog context on cancel without invoking deletion', async () => { diff --git a/src/taxonomy/tag-list/DeleteModal.tsx b/src/taxonomy/tag-list/DeleteModal.tsx index a31966186a..e1d0e21952 100644 --- a/src/taxonomy/tag-list/DeleteModal.tsx +++ b/src/taxonomy/tag-list/DeleteModal.tsx @@ -25,8 +25,8 @@ const DeleteModal = ({ }: DeleteModalProps) => { const intl = useIntl(); - const handleConfirm = (row: Row) => { - handleDeleteRow(row); + const handleConfirm = async (row: Row) => { + await handleDeleteRow(row); setIsOpen(false); setRow(null); }; diff --git a/src/taxonomy/tag-list/TagListTable.test.jsx b/src/taxonomy/tag-list/TagListTable.test.jsx index c61fefd347..0c1c7e1ace 100644 --- a/src/taxonomy/tag-list/TagListTable.test.jsx +++ b/src/taxonomy/tag-list/TagListTable.test.jsx @@ -116,8 +116,6 @@ const subTagsUrl = 'http://localhost:18010/api/content_tagging/v1/taxonomies/1/tags/?full_depth_threshold=10000&parent_tag=root+tag+1'; const createTagUrl = 'http://localhost:18010/api/content_tagging/v1/taxonomies/1/tags/'; const deleteTagUrl = createTagUrl; -const deleteConfirmMessage = - 'Warning: are you sure you want to delete this tag and all its subtags and descendants? Any tags applied to course content will be deleted.'; const renderTagListTable = (maxDepth = 3) => render(); @@ -956,16 +954,6 @@ describe('', () => { }); describe('Delete Tags', () => { - let confirmSpy; - - beforeEach(() => { - confirmSpy = jest.spyOn(window, 'confirm').mockReturnValue(true); - }); - - afterEach(() => { - confirmSpy.mockRestore(); - }); - const tagDepthScenarios = [ { description: 'Delete a top-level tag', @@ -999,7 +987,7 @@ describe('', () => { }); }); - it('asks for browser confirmation before deleting a leaf tag and deletes after acceptance', async () => { + it('opens the delete modal for a leaf tag and deletes only after typed confirmation', async () => { axiosMock.reset(); axiosMock.onDelete(deleteTagUrl).reply(204); axiosMock @@ -1018,7 +1006,14 @@ describe('', () => { openActionsMenuForTag('root tag 2'); fireEvent.click(screen.getByRole('button', { name: /^Delete$/i })); - expect(confirmSpy).toHaveBeenCalledWith(deleteConfirmMessage); + const dialog = screen.getByRole('dialog'); + expect(dialog).toHaveTextContent('Delete "root tag 2"'); + expect(dialog).toHaveTextContent('Type DELETE to confirm'); + expect(axiosMock.history.delete.length).toBe(0); + + fireEvent.change(screen.getByRole('textbox'), { target: { value: 'DELETE' } }); + fireEvent.click(screen.getByRole('button', { name: 'Delete Tag' })); + await expectDeleteRequest({ tagName: 'root tag 2', withSubtags: false }); expect( await screen.findByText( @@ -1030,7 +1025,7 @@ describe('', () => { }); }); - it('deletes a parent tag together with all rendered descendants after browser confirmation', async () => { + it('deletes a parent tag together with all rendered descendants after typed modal confirmation', async () => { fireEvent.click(screen.getAllByText('Expand All')[0]); await screen.findByText('the child tag'); await screen.findByText('the grandchild tag'); @@ -1054,7 +1049,13 @@ describe('', () => { openActionsMenuForTag('root tag 1'); fireEvent.click(screen.getByRole('button', { name: /^Delete$/i })); - expect(confirmSpy).toHaveBeenCalledWith(deleteConfirmMessage); + const dialog = screen.getByRole('dialog'); + expect(dialog).toHaveTextContent('Delete "root tag 1"'); + expect(dialog).toHaveTextContent('Type DELETE ALL 3 TAGS to confirm'); + + fireEvent.change(screen.getByRole('textbox'), { target: { value: 'DELETE ALL 3 TAGS' } }); + fireEvent.click(screen.getByRole('button', { name: 'Delete Tags' })); + await expectDeleteRequest({ tagName: 'root tag 1', withSubtags: true }); expect( await screen.findByText( @@ -1071,14 +1072,15 @@ describe('', () => { }); }); - it('does not issue a delete request when browser confirmation is canceled and leaves the table unchanged', async () => { - confirmSpy.mockReturnValue(false); - + it('does not issue a delete request when the delete modal is canceled and leaves the table unchanged', async () => { openActionsMenuForTag('root tag 1'); fireEvent.click(screen.getByRole('button', { name: /^Delete$/i })); - expect(confirmSpy).toHaveBeenCalledWith(deleteConfirmMessage); + expect(screen.getByRole('dialog')).toHaveTextContent('Delete "root tag 1"'); + fireEvent.click(screen.getByRole('button', { name: 'Cancel' })); + expect(axiosMock.history.delete.length).toBe(0); + expect(screen.queryByRole('dialog')).not.toBeInTheDocument(); expect(screen.getByText('root tag 1')).toBeInTheDocument(); }); @@ -1097,7 +1099,9 @@ describe('', () => { openActionsMenuForTag('root tag 1'); fireEvent.click(screen.getByRole('button', { name: /^Delete$/i })); - expect(confirmSpy).toHaveBeenCalledWith(deleteConfirmMessage); + fireEvent.change(screen.getByRole('textbox'), { target: { value: 'DELETE ALL 3 TAGS' } }); + fireEvent.click(screen.getByRole('button', { name: 'Delete Tags' })); + await expectDeleteRequest({ tagName: 'root tag 1', withSubtags: true }); expect( await screen.findByText('Error saving changes'), @@ -1325,9 +1329,9 @@ describe(' pagination transition behavior', () => { jest.spyOn(hooksModule, 'useEditActions').mockReturnValue({ handleCreateRow: jest.fn(), handleUpdateRow: jest.fn(), + handleDeleteRow: jest.fn(), startSubtagDraft: jest.fn(), startEditRow: jest.fn(), - startDeleteRow: jest.fn(), validate: jest.fn(() => true), }); jest.spyOn(treeTableModule, 'TableView').mockImplementation(() => { diff --git a/src/taxonomy/tag-list/TagListTable.tsx b/src/taxonomy/tag-list/TagListTable.tsx index cbd3e357ec..87a9121e12 100644 --- a/src/taxonomy/tag-list/TagListTable.tsx +++ b/src/taxonomy/tag-list/TagListTable.tsx @@ -3,7 +3,7 @@ import React, { useMemo, useEffect, } from 'react'; -import type { PaginationState } from '@tanstack/react-table'; +import type { PaginationState, Row } from '@tanstack/react-table'; import { useTagListData, useCreateTag, useUpdateTag, useDeleteTag } from '@src/taxonomy/data/apiHooks'; import { TableView, TreeTableContext } from '@src/taxonomy/tree-table'; import type { @@ -12,6 +12,7 @@ import type { } from '../tree-table/types'; import { TagTree } from './tagTree'; import { getColumns } from './tagColumns'; +import DeleteModal from './DeleteModal'; import { TABLE_MODES, } from './constants'; @@ -47,6 +48,8 @@ const TagListTable = ({ taxonomyId, maxDepth }: TagListTableProps) => { const [tagTree, setTagTree] = useState(null); const [isCreatingTopTag, setIsCreatingTopTag] = useState(false); const [, setActiveActionMenuRowId] = useState(null); + const [deleteRow, setDeleteRow] = useState | null>(null); + const [isDeleteModalOpen, setIsDeleteModalOpen] = useState(false); const [draftError, setDraftError] = useState(''); const treeData = (tagTree?.getAllAsDeepCopy() || []) as unknown as TreeRowData[]; const hasOpenDraft = isCreatingTopTag || creatingParentId !== null || editingRowId !== null; @@ -108,6 +111,12 @@ const TagListTable = ({ taxonomyId, maxDepth }: TagListTableProps) => { }, ); + const startDeleteRow = (row: Row) => { + setDeleteRow(row); + setIsDeleteModalOpen(true); + setActiveActionMenuRowId(null); + }; + // RELOAD DATA IN VIEW MODE useEffect(() => { // Get row data in VIEW mode. Otherwise keep current data to avoid disrupting @@ -123,6 +132,7 @@ const TagListTable = ({ taxonomyId, maxDepth }: TagListTableProps) => { // TreeTable context const contextValueArgs = { ...editActions, + startDeleteRow, treeData, pageCount, pagination, @@ -157,6 +167,13 @@ const TagListTable = ({ taxonomyId, maxDepth }: TagListTableProps) => { return ( + ); }; diff --git a/src/taxonomy/tag-list/hooks.test.tsx b/src/taxonomy/tag-list/hooks.test.tsx index 99ae1973d1..2dda0dfae3 100644 --- a/src/taxonomy/tag-list/hooks.test.tsx +++ b/src/taxonomy/tag-list/hooks.test.tsx @@ -190,9 +190,8 @@ describe('useEditActions', () => { setToast, } = buildActions(); deleteTagMutation.mutateAsync.mockResolvedValue(undefined); - jest.spyOn(window, 'confirm').mockReturnValue(true); - actions.startDeleteRow({ + actions.handleDeleteRow({ original: { id: 1, value: 'tag to delete', @@ -226,9 +225,8 @@ describe('useEditActions', () => { setToast, } = buildActions(); deleteTagMutation.mutateAsync.mockRejectedValue(new Error('server failed')); - jest.spyOn(window, 'confirm').mockReturnValue(true); - actions.startDeleteRow({ + actions.handleDeleteRow({ original: { id: 1, value: 'tag to delete', diff --git a/src/taxonomy/tag-list/hooks.ts b/src/taxonomy/tag-list/hooks.ts index e4a7c95bf5..0d65e47f00 100644 --- a/src/taxonomy/tag-list/hooks.ts +++ b/src/taxonomy/tag-list/hooks.ts @@ -1,6 +1,7 @@ import { useReducer } from 'react'; import { AxiosError } from 'axios'; import { useIntl } from '@edx/frontend-platform/i18n'; +import { Row } from '@tanstack/react-table'; import globalMessages from '@src/messages'; import { useCreateTag, useDeleteTag, useUpdateTag } from '@src/taxonomy/data/apiHooks'; @@ -16,10 +17,6 @@ import { import messages from './messages'; import { getTagListRowData, getTagWithDescendantsCount } from './utils'; -import { Row } from '@tanstack/react-table'; - -const DELETE_CONFIRM_MESSAGE = - 'Warning: are you sure you want to delete this tag and all its subtags and descendants? Any tags applied to course content will be deleted.'; /** Interface for table mode actions for React's `useReducer` hook. * @@ -282,12 +279,6 @@ const useEditActions = ({ setEditingRowId(`${rowData.id}:${rowData.value}`); }; - const startDeleteTag = (row: Row) => { - if (window.confirm(DELETE_CONFIRM_MESSAGE)) { - void handleDeleteTag(row); - } - }; - const handleDeleteTag = async (row: Row) => { const rowData = getTagListRowData(row); const count = getTagWithDescendantsCount(rowData); @@ -315,9 +306,9 @@ const useEditActions = ({ updateTableWithoutDataReload, handleCreateRow: handleCreateTag, handleUpdateRow: handleUpdateTag, + handleDeleteRow: handleDeleteTag, startSubtagDraft, startEditRow: startEditTag, - startDeleteRow: startDeleteTag, validate, }; }; diff --git a/src/taxonomy/tag-list/messages.ts b/src/taxonomy/tag-list/messages.ts index cc338f4607..b448e30519 100644 --- a/src/taxonomy/tag-list/messages.ts +++ b/src/taxonomy/tag-list/messages.ts @@ -100,6 +100,7 @@ const messages = defineMessages({ deleteTagConfirmationEmphasizedPart: { id: 'course-authoring.tag-list.delete-tag-confirmation-bold-part', defaultMessage: 'Any tags applied to course content will be removed across all assigned organizations.', + }, deleteTag: { id: 'course-authoring.tag-list.delete-tag', defaultMessage: 'Delete', From b5b1d319247840f874db7b60890729f863f96e3e Mon Sep 17 00:00:00 2001 From: Mary Gwozdz Date: Wed, 29 Apr 2026 15:16:22 -0700 Subject: [PATCH 8/9] fix: code coverage --- .../tree-table/SaveErrorAlert.test.tsx | 52 +++++ src/taxonomy/tree-table/TableBody.test.tsx | 200 ++++++++++++++++++ .../tree-table/TreeTableContext.test.tsx | 60 ++++++ 3 files changed, 312 insertions(+) create mode 100644 src/taxonomy/tree-table/SaveErrorAlert.test.tsx create mode 100644 src/taxonomy/tree-table/TableBody.test.tsx create mode 100644 src/taxonomy/tree-table/TreeTableContext.test.tsx diff --git a/src/taxonomy/tree-table/SaveErrorAlert.test.tsx b/src/taxonomy/tree-table/SaveErrorAlert.test.tsx new file mode 100644 index 0000000000..891e92d993 --- /dev/null +++ b/src/taxonomy/tree-table/SaveErrorAlert.test.tsx @@ -0,0 +1,52 @@ +import React from 'react'; +import { IntlProvider } from '@edx/frontend-platform/i18n'; +import { fireEvent, render, screen } from '@testing-library/react'; + +import SaveErrorAlert from './SaveErrorAlert'; + +const wrapper = ({ children }: { children: React.ReactNode; }) => ( + {children} +); + +describe('SaveErrorAlert', () => { + it('stays closed when only a draft error is present and delete-error state is omitted', () => { + render( + , + { wrapper }, + ); + + expect(screen.queryByText('Error saving changes')).not.toBeInTheDocument(); + }); + + it('opens for delete errors and reopens when a new delete failure arrives', () => { + const { rerender } = render( + , + { wrapper }, + ); + + expect(screen.getByText('Error saving changes')).toBeInTheDocument(); + fireEvent.click(screen.getByRole('button', { name: /dismiss/i })); + expect(screen.queryByText('Error saving changes')).not.toBeInTheDocument(); + + rerender( + , + ); + + expect(screen.getByText('Error saving changes')).toBeInTheDocument(); + expect(screen.getByText('Second delete failure. Please try again.')).toBeInTheDocument(); + }); +}); diff --git a/src/taxonomy/tree-table/TableBody.test.tsx b/src/taxonomy/tree-table/TableBody.test.tsx new file mode 100644 index 0000000000..c71bfcf317 --- /dev/null +++ b/src/taxonomy/tree-table/TableBody.test.tsx @@ -0,0 +1,200 @@ +import React from 'react'; +import { IntlProvider } from '@edx/frontend-platform/i18n'; +import { fireEvent, render, screen } from '@testing-library/react'; + +import TableBody from './TableBody'; +import { TreeTableContext } from './TreeTableContext'; + +jest.mock('./CreateRow', () => () => ( + + Create row + +)); + +jest.mock('./EditRow', () => ({ + initialValue, + handleUpdateRow, + cancelEditRow, +}: { + initialValue: string; + handleUpdateRow: (value: string) => void; + cancelEditRow: () => void; +}) => ( + + {initialValue} + + + + + + + +)); + +jest.mock('./NestedRows', () => ({ + parentRowValue, + isCreating, + onSaveNewChildRow, + onCancelCreation, +}: { + parentRowValue: string; + isCreating: boolean; + onSaveNewChildRow: (value: string, parentRowValue?: string) => void; + onCancelCreation: () => void; +}) => ( + + {parentRowValue} + {String(isCreating)} + + + + + + + +)); + +const wrapper = ({ children }: { children: React.ReactNode; }) => ( + {children} +); + +const baseContextValue = (): any => ({ + treeData: [], + columns: [{ accessorKey: 'value', header: 'Tag name', cell: () => 'unused' }], + pageCount: -1, + pagination: { pageIndex: 0, pageSize: 10 }, + handlePaginationChange: jest.fn(), + isLoading: false, + isCreatingTopRow: false, + draftError: '', + createRowMutation: {}, + updateRowMutation: {}, + toast: { show: false, message: '', variant: 'success' as const }, + setToast: jest.fn(), + setIsCreatingTopRow: jest.fn(), + exitDraftWithoutSave: jest.fn(), + handleCreateRow: jest.fn(), + creatingParentId: null, + setCreatingParentId: jest.fn(), + setDraftError: jest.fn(), + validate: jest.fn(() => true), + handleUpdateRow: jest.fn(), + editingRowId: null, + setEditingRowId: jest.fn(), + table: null, +}); + +const makeCell = (id: string, content: string) => ({ + id, + column: { columnDef: { cell: () => content } }, + getContext: () => ({}), +}); + +const makeRow = ({ + id, + value, + depth = 0, + subRows = [], +}: { + id: number; + value: string; + depth?: number; + subRows?: any[]; +}) => ({ + id: String(id), + depth, + original: { id, value }, + subRows, + getVisibleCells: () => [makeCell(`${id}-cell`, `${value} cell`)], +}); + +const renderTableBody = (contextValue = baseContextValue()) => render( + + + +
+ , + { wrapper }, +); + +describe('TableBody', () => { + it('returns null when no table instance is available in context', () => { + const { container } = render( + + +
, + { wrapper }, + ); + + expect(container.querySelector('tbody')).toBeNull(); + }); + + it('shows an empty-state row when the table has no rows', () => { + const contextValue = baseContextValue(); + contextValue.table = { + getRowModel: () => ({ rows: [] }), + }; + + renderTableBody(contextValue); + + expect(screen.getByText('No results found')).toBeInTheDocument(); + }); + + it('shows a loading row when table data is still loading', () => { + const contextValue = baseContextValue(); + contextValue.isLoading = true; + contextValue.table = { + getRowModel: () => ({ rows: [] }), + }; + + renderTableBody(contextValue); + + expect(screen.getByRole('status')).toBeInTheDocument(); + }); + + it('renders top-level creation and nested-row callbacks from context', () => { + const contextValue = baseContextValue(); + const rootRow = makeRow({ id: 1, value: 'root tag' }); + + contextValue.isCreatingTopRow = true; + contextValue.creatingParentId = 1; + contextValue.table = { + getRowModel: () => ({ rows: [rootRow] }), + }; + + renderTableBody(contextValue); + + expect(screen.getByTestId('create-row')).toBeInTheDocument(); + expect(screen.getByText('root tag cell')).toBeInTheDocument(); + expect(screen.getByTestId('nested-rows-root tag')).toHaveTextContent('true'); + + fireEvent.click(screen.getByRole('button', { name: 'save child' })); + expect(contextValue.handleCreateRow).toHaveBeenCalledWith('new child', 'root tag'); + + fireEvent.click(screen.getByRole('button', { name: 'cancel child' })); + expect(contextValue.setDraftError).toHaveBeenCalledWith(''); + expect(contextValue.setCreatingParentId).toHaveBeenCalledWith(null); + expect(contextValue.exitDraftWithoutSave).toHaveBeenCalled(); + }); + + it('renders edit mode for the matching row and wires save and cancel through context', () => { + const contextValue = baseContextValue(); + const rootRow = makeRow({ id: 1, value: 'root tag' }); + + contextValue.editingRowId = '1:root tag'; + contextValue.table = { + getRowModel: () => ({ rows: [rootRow] }), + }; + + renderTableBody(contextValue); + + expect(screen.getByTestId('edit-row')).toBeInTheDocument(); + + fireEvent.click(screen.getByRole('button', { name: 'save edit' })); + expect(contextValue.handleUpdateRow).toHaveBeenCalledWith('updated root', 'root tag'); + + fireEvent.click(screen.getByRole('button', { name: 'cancel edit' })); + expect(contextValue.setEditingRowId).toHaveBeenCalledWith(null); + expect(contextValue.exitDraftWithoutSave).toHaveBeenCalled(); + }); +}); diff --git a/src/taxonomy/tree-table/TreeTableContext.test.tsx b/src/taxonomy/tree-table/TreeTableContext.test.tsx new file mode 100644 index 0000000000..6f66dd01f1 --- /dev/null +++ b/src/taxonomy/tree-table/TreeTableContext.test.tsx @@ -0,0 +1,60 @@ +import React, { useContext, useEffect } from 'react'; +import { render, screen } from '@testing-library/react'; + +import { TreeTableContext } from './TreeTableContext'; + +const ContextProbe = () => { + const context = useContext(TreeTableContext); + + useEffect(() => { + context.handlePaginationChange({ pageIndex: 1, pageSize: 25 }); + context.setToast((prevToast) => ({ ...prevToast, show: true })); + context.setIsCreatingTopRow(true); + context.exitDraftWithoutSave(); + context.handleCreateRow('new tag', 'parent tag'); + context.setCreatingParentId(123); + context.setDraftError('Draft error'); + context.handleUpdateRow('updated tag', 'original tag'); + context.setEditingRowId(456); + }, [context]); + + return ( + <> + {String(context.pageCount)} + + {`${context.pagination.pageIndex}:${context.pagination.pageSize}`} + + {String(context.isLoading)} + {String(context.isCreatingTopRow)} + {context.draftError} + {JSON.stringify(context.createRowMutation)} + {JSON.stringify(context.updateRowMutation)} + {JSON.stringify(context.toast)} + {String(context.creatingParentId)} + {String(context.editingRowId)} + {String(context.table === null)} + {String(context.validate('tag value', 'hard'))} + + ); +}; + +describe('TreeTableContext', () => { + it('provides safe default values and no-op handlers when no provider is present', () => { + render(); + + expect(screen.getByTestId('page-count')).toHaveTextContent('-1'); + expect(screen.getByTestId('pagination')).toHaveTextContent('0:0'); + expect(screen.getByTestId('is-loading')).toHaveTextContent('false'); + expect(screen.getByTestId('is-creating-top-row')).toHaveTextContent('false'); + expect(screen.getByTestId('draft-error')).toBeEmptyDOMElement(); + expect(screen.getByTestId('create-row-mutation')).toHaveTextContent('{}'); + expect(screen.getByTestId('update-row-mutation')).toHaveTextContent('{}'); + expect(screen.getByTestId('toast')).toHaveTextContent( + '{"show":false,"message":"","variant":"success"}', + ); + expect(screen.getByTestId('creating-parent-id')).toHaveTextContent('null'); + expect(screen.getByTestId('editing-row-id')).toHaveTextContent('null'); + expect(screen.getByTestId('table-is-null')).toHaveTextContent('true'); + expect(screen.getByTestId('validate-result')).toHaveTextContent('true'); + }); +}); From adc529925b5f9a4ad99b5f756796f88ca91f6183 Mon Sep 17 00:00:00 2001 From: Mary Gwozdz Date: Wed, 29 Apr 2026 15:19:50 -0700 Subject: [PATCH 9/9] fix: lint formatting --- src/taxonomy/tree-table/TableBody.test.tsx | 23 ++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/src/taxonomy/tree-table/TableBody.test.tsx b/src/taxonomy/tree-table/TableBody.test.tsx index c71bfcf317..f31f13a21a 100644 --- a/src/taxonomy/tree-table/TableBody.test.tsx +++ b/src/taxonomy/tree-table/TableBody.test.tsx @@ -11,7 +11,8 @@ jest.mock('./CreateRow', () => () => ( )); -jest.mock('./EditRow', () => ({ +jest.mock('./EditRow', () => +({ initialValue, handleUpdateRow, cancelEditRow, @@ -31,7 +32,8 @@ jest.mock('./EditRow', () => ({ )); -jest.mock('./NestedRows', () => ({ +jest.mock('./NestedRows', () => +({ parentRowValue, isCreating, onSaveNewChildRow, @@ -108,14 +110,15 @@ const makeRow = ({ getVisibleCells: () => [makeCell(`${id}-cell`, `${value} cell`)], }); -const renderTableBody = (contextValue = baseContextValue()) => render( - - - -
- , - { wrapper }, -); +const renderTableBody = (contextValue = baseContextValue()) => + render( + + + +
+ , + { wrapper }, + ); describe('TableBody', () => { it('returns null when no table instance is available in context', () => {