diff --git a/src/taxonomy/data/api.ts b/src/taxonomy/data/api.ts index 063971ee08..f9ade6ccbe 100644 --- a/src/taxonomy/data/api.ts +++ b/src/taxonomy/data/api.ts @@ -90,6 +90,7 @@ export const apiUrls = { /** URL to plan (preview what would happen) a taxonomy import */ tagsPlanImport: (taxonomyId: number) => makeUrl(`${taxonomyId}/tags/import/plan/`), createTag: (taxonomyId: number) => makeUrl(`${taxonomyId}/tags/`), + updateTag: (taxonomyId: number) => makeUrl(`${taxonomyId}/tags/`), } satisfies Record string>; /** diff --git a/src/taxonomy/data/apiHooks.ts b/src/taxonomy/data/apiHooks.ts index 753503cf3f..dd0b5cf2de 100644 --- a/src/taxonomy/data/apiHooks.ts +++ b/src/taxonomy/data/apiHooks.ts @@ -234,3 +234,27 @@ export const useCreateTag = (taxonomyId: number) => { }, }); }; + +export const useUpdateTag = (taxonomyId: number) => { + const queryClient = useQueryClient(); + + return useMutation({ + mutationFn: async ({ value, originalValue }: { value: string, originalValue: string }) => { + try { + await getAuthenticatedHttpClient().patch( + apiUrls.updateTag(taxonomyId), + { tag: originalValue, updated_tag_value: value }, + ); + } catch (err) { + throw new Error(getApiErrorMessage(err)); + } + }, + 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/data/types.ts b/src/taxonomy/data/types.ts index d8ca63d1e7..4fd3c41167 100644 --- a/src/taxonomy/data/types.ts +++ b/src/taxonomy/data/types.ts @@ -59,6 +59,7 @@ export interface TagListData { next: string; numPages: number; previous: string; + canAddTag?: boolean; results: TagData[]; start: number; } diff --git a/src/taxonomy/tag-list/TagListTable.test.jsx b/src/taxonomy/tag-list/TagListTable.test.jsx index a7616a2ab7..53018661e2 100644 --- a/src/taxonomy/tag-list/TagListTable.test.jsx +++ b/src/taxonomy/tag-list/TagListTable.test.jsx @@ -98,6 +98,15 @@ const mockTagsResponse = { }, ], }; + +const mockTagResponseDisallowingEdits = { + ...mockTagsResponse, + results: mockTagsResponse.results.map(tag => ({ + ...tag, + can_change_tag: false, + can_delete_tag: false, + })), +}; const mockTagsPaginationResponse = { next: null, previous: null, @@ -161,6 +170,15 @@ const openTopLevelDraftRow = async () => { }; const openActionsMenuForTag = (tagName, actionButtonName = /actions/i) => { + if (!screen.queryAllByText(tagName)?.length) { + // expand all + const expandButton = screen.queryAllByText('Expand All')?.[0]; + act(() => { + if (expandButton) { + fireEvent.click(expandButton); + } + }); + } const row = screen.getByText(tagName).closest('tr'); const actionsButton = within(row).getByRole('button', { name: actionButtonName }); act(() => { @@ -183,6 +201,22 @@ const openSubtagDraftRow = async ({ return { rows, draftRow, input }; }; +const openRenameDraftRow = async (tagName = 'root tag 1') => { + openActionsMenuForTag(tagName); + fireEvent.click(screen.getByText('Rename')); + const input = screen.getByRole('textbox'); + const row = input.closest('tr'); + expect(row).toBeInTheDocument(); + const saveButton = within(row).getByText('Save'); + const cancelButton = within(row).getByText('Cancel'); + return { + row, + input, + saveButton, + cancelButton, + }; +}; + describe('', () => { beforeAll(async () => { initializeMockApp({ @@ -191,8 +225,13 @@ describe('', () => { axiosMock = new MockAdapter(getAuthenticatedHttpClient()); }); beforeEach(async () => { + initializeMockApp({ + authenticatedUser: adminUser, + }); store = initializeStore(); + queryClient.clear(); axiosMock.reset(); + axiosMock = new MockAdapter(getAuthenticatedHttpClient()); axiosMock.onGet(rootTagsListUrl).reply(200, mockTagsResponse); axiosMock.onGet(subTagsUrl).reply(200, subTagsResponse); renderTagListTable(); @@ -255,7 +294,26 @@ describe('', () => { }); 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, { + ...mockTagsResponse, + can_add_tag: false, + }); + cleanup(); + queryClient.clear(); + renderTagListTable(); + await waitForRootTag(); + + openActionsMenuForTag('root tag 1'); + expect(screen.getByText('Add Subtag')).toBeDisabled(); + expect(screen.getByLabelText('Create Tag')).toBeDisabled(); + }); + describe('with editable user and loaded taxonomy', () => { + beforeEach(() => { + axiosMock.onGet(rootTagsListUrl).reply(200, mockTagsResponse); + }); + it('should add draft row when top-level "Add tag" button is clicked', async () => { const { creatingRow } = await openTopLevelDraftRow(); @@ -429,6 +487,8 @@ describe('', () => { // 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 + axiosMock.reset(); axiosMock.onPost(createTagUrl).reply(config => { const requestData = JSON.parse(config.data); return [201, { @@ -470,7 +530,7 @@ describe('', () => { expect(tagBRowIndex).toBeLessThan(tagARowIndex); // no additional get requests should have been made, that is, the table should not have been refreshed - expect(axiosMock.history.get.length).toBe(1); + expect(axiosMock.history.get.length).toBe(0); }); it('should disable the Save button when the input is empty', async () => { @@ -596,108 +656,106 @@ describe('', () => { }); describe('Create a new subtag', () => { - describe('with editable user and loaded taxonomy', () => { - it('should show an Add sub-tag option in the parent tag actions', async () => { - expect(screen.queryAllByText('Add Subtag').length).toBe(0); - // user clicks on row actions for root tag 1 - openActionsMenuForTag('root tag 1'); - expect(screen.getByText('Add Subtag')).toBeInTheDocument(); - }); + it('should show an Add sub-tag option in the parent tag actions', async () => { + expect(screen.queryAllByText('Add Subtag').length).toBe(0); + // user clicks on row actions for root tag 1 + openActionsMenuForTag('root tag 1'); + expect(screen.getByText('Add Subtag')).toBeInTheDocument(); + }); - it('should render an inline add-subtag row with input, placeholder, and action buttons', async () => { - const { rows } = await openSubtagDraftRow({ tagName: 'root tag 1' }); - const draftRows = rows.filter(tableRow => tableRow.querySelector('input')); - expect(draftRows[0].querySelector('input')).toBeInTheDocument(); - // expect the draft row to be directly beneath the parent tag row - const parentRowIndex = rows.findIndex(tableRow => within(tableRow).queryByText('root tag 1')); - const draftRowIndex = rows.findIndex(tableRow => tableRow.querySelector('input')); - expect(draftRowIndex).toBe(parentRowIndex + 1); - expect(draftRows[0].querySelector('input')).toBeInTheDocument(); - expect(within(draftRows[0]).getByText('Cancel')).toBeInTheDocument(); - expect(within(draftRows[0]).getByText('Save')).toBeInTheDocument(); - }); + it('should render an inline add-subtag row with input, placeholder, and action buttons', async () => { + const { rows } = await openSubtagDraftRow({ tagName: 'root tag 1' }); + const draftRows = rows.filter(tableRow => tableRow.querySelector('input')); + expect(draftRows[0].querySelector('input')).toBeInTheDocument(); + // expect the draft row to be directly beneath the parent tag row + const parentRowIndex = rows.findIndex(tableRow => within(tableRow).queryByText('root tag 1')); + const draftRowIndex = rows.findIndex(tableRow => tableRow.querySelector('input')); + expect(draftRowIndex).toBe(parentRowIndex + 1); + expect(draftRows[0].querySelector('input')).toBeInTheDocument(); + expect(within(draftRows[0]).getByText('Cancel')).toBeInTheDocument(); + expect(within(draftRows[0]).getByText('Save')).toBeInTheDocument(); + }); - it('should remove add-subtag row and avoid create request when cancelled', async () => { - const { draftRow, input } = await openSubtagDraftRow({ tagName: 'root tag 1' }); - fireEvent.change(input, { target: { value: 'new subtag' } }); - fireEvent.click(within(draftRow).getByText('Cancel')); + it('should remove add-subtag row and avoid create request when cancelled', async () => { + const { draftRow, input } = await openSubtagDraftRow({ tagName: 'root tag 1' }); + fireEvent.change(input, { target: { value: 'new subtag' } }); + fireEvent.click(within(draftRow).getByText('Cancel')); - await waitFor(() => { - expect(axiosMock.history.post.length).toBe(0); - expectNoDraftRows(); - }); + await waitFor(() => { + expect(axiosMock.history.post.length).toBe(0); + expectNoDraftRows(); }); + }); - it('should remove add-subtag row and avoid create request on escape key', async () => { - const { input } = await openSubtagDraftRow({ tagName: 'root tag 1' }); + it('should remove add-subtag row and avoid create request on escape key', async () => { + const { input } = await openSubtagDraftRow({ tagName: 'root tag 1' }); - fireEvent.change(input, { target: { value: 'new subtag' } }); - fireEvent.keyDown(input, { key: 'Escape', code: 'Escape' }); + fireEvent.change(input, { target: { value: 'new subtag' } }); + fireEvent.keyDown(input, { key: 'Escape', code: 'Escape' }); - await waitFor(() => { - expect(axiosMock.history.post.length).toBe(0); - expectNoDraftRows(); - }); + await waitFor(() => { + expect(axiosMock.history.post.length).toBe(0); + expectNoDraftRows(); }); + }); - it('should disable Save and show required-name inline error for empty sub-tag input', async () => { - openActionsMenuForTag('root tag 1'); - fireEvent.click(screen.getByText('Add Subtag')); - const rows = await screen.findAllByRole('row'); - const draftRow = rows.find(tableRow => tableRow.querySelector('input')); - const saveButton = within(draftRow).getByText('Save'); - const input = draftRow.querySelector('input'); - act(() => { - fireEvent.change(input, { target: { value: ' ' } }); - }); - - expect(saveButton).toBeDisabled(); - expect(within(draftRow).getByText(/Name is required/i)).toBeInTheDocument(); + it('should disable Save and show required-name inline error for empty sub-tag input', async () => { + openActionsMenuForTag('root tag 1'); + fireEvent.click(screen.getByText('Add Subtag')); + const rows = await screen.findAllByRole('row'); + const draftRow = rows.find(tableRow => tableRow.querySelector('input')); + const saveButton = within(draftRow).getByText('Save'); + const input = draftRow.querySelector('input'); + act(() => { + fireEvent.change(input, { target: { value: ' ' } }); }); - it('should keep Save disabled for whitespace-only sub-tag input', async () => { - openActionsMenuForTag('root tag 1'); - fireEvent.click(screen.getByText('Add Subtag')); - const rows = await screen.findAllByRole('row'); - const draftRow = rows.find(tableRow => tableRow.querySelector('input')); - const input = draftRow.querySelector('input'); - const saveButton = within(draftRow).getByText('Save'); + expect(saveButton).toBeDisabled(); + expect(within(draftRow).getByText(/Name is required/i)).toBeInTheDocument(); + }); - fireEvent.change(input, { target: { value: ' ' } }); - expect(saveButton).toBeDisabled(); - }); + it('should keep Save disabled for whitespace-only sub-tag input', async () => { + openActionsMenuForTag('root tag 1'); + fireEvent.click(screen.getByText('Add Subtag')); + const rows = await screen.findAllByRole('row'); + const draftRow = rows.find(tableRow => tableRow.querySelector('input')); + const input = draftRow.querySelector('input'); + const saveButton = within(draftRow).getByText('Save'); + + fireEvent.change(input, { target: { value: ' ' } }); + expect(saveButton).toBeDisabled(); + }); - it('should disable Save and show invalid-character error for sub-tag input', async () => { - openActionsMenuForTag('root tag 1'); - fireEvent.click(screen.getByText('Add Subtag')); - const rows = await screen.findAllByRole('row'); - const draftRow = rows.find(row => row.querySelector('input')); - const input = draftRow.querySelector('input'); - const saveButton = within(draftRow).getByText('Save'); + it('should disable Save and show invalid-character error for sub-tag input', async () => { + openActionsMenuForTag('root tag 1'); + fireEvent.click(screen.getByText('Add Subtag')); + const rows = await screen.findAllByRole('row'); + const draftRow = rows.find(row => row.querySelector('input')); + const input = draftRow.querySelector('input'); + const saveButton = within(draftRow).getByText('Save'); - fireEvent.change(input, { target: { value: 'invalid;name' } }); - expect(saveButton).toBeDisabled(); - expect(within(draftRow).getByText(/invalid character/i)).toBeInTheDocument(); - }); + fireEvent.change(input, { target: { value: 'invalid;name' } }); + expect(saveButton).toBeDisabled(); + expect(within(draftRow).getByText(/invalid character/i)).toBeInTheDocument(); + }); - it('should keep inline row and show failure feedback when sub-tag save fails', async () => { - axiosMock.onPost(createTagUrl).reply(500, { - error: 'Internal server error', - }); + it('should keep inline row and show failure feedback when sub-tag save fails', async () => { + axiosMock.onPost(createTagUrl).reply(500, { + error: 'Internal server error', + }); - openActionsMenuForTag('root tag 1'); - fireEvent.click(screen.getByText('Add Subtag')); - const rows = await screen.findAllByRole('row'); - const draftRow = rows.find(row => row.querySelector('input')); - const input = draftRow.querySelector('input'); - fireEvent.change(input, { target: { value: 'subtag fail' } }); - fireEvent.click(within(draftRow).getByText('Save')); + openActionsMenuForTag('root tag 1'); + fireEvent.click(screen.getByText('Add Subtag')); + const rows = await screen.findAllByRole('row'); + const draftRow = rows.find(row => row.querySelector('input')); + const input = draftRow.querySelector('input'); + fireEvent.change(input, { target: { value: 'subtag fail' } }); + fireEvent.click(within(draftRow).getByText('Save')); - await waitFor(() => { - expect(getDraftRows().length).toBe(1); - }); - expect(await screen.findByText(/Error creating tag:/i)).toBeInTheDocument(); + await waitFor(() => { + expect(getDraftRows().length).toBe(1); }); + expect(await screen.findByText(/Error creating tag:/i)).toBeInTheDocument(); }); it('should hide or disable Add sub-tag actions when user lacks edit permissions', async () => { @@ -713,6 +771,192 @@ describe('', () => { }); }); + describe('Tag Rename Errors', () => { + it('should keep the inline row and show a failure toast when save request fails', async () => { + axiosMock.onPatch().reply(500, { + error: 'Internal server error', + }); + const { input } = await openRenameDraftRow('root tag 1'); + + fireEvent.change(input, { target: { value: 'will fail' } }); + act(() => { + fireEvent.keyDown(input, { key: 'Enter', code: 'Enter' }); + }); + + let draftRow; + await waitFor(() => { + const rows = screen.getAllByRole('row'); + const draftRows = rows.filter(row => row.querySelector('input')); + expect(draftRows.length).toBe(1); + draftRow = draftRows[0]; // eslint-disable-line prefer-destructuring + }); + + // Banner error message should be shown at the top of the table + expect(await screen.findByText('Error saving changes')).toBeInTheDocument(); + + // Toast message to indicate that the save failed + expect(await screen.findByText('Error saving changes')).toBeInTheDocument(); + expect(await screen.findByText('Internal server error')).toBeInTheDocument(); + + // expect the input to retain the value that was entered before + expect(draftRow.querySelector('input').value).toEqual('will fail'); + // expect the new tag to not be in the document outside the input field + expect(screen.queryByText('will fail')).not.toBeInTheDocument(); + }); + }); + + const tagDepthScenarios = [ + { + description: 'Rename a top-level tag', + tagName: 'root tag 1', + }, + { description: 'Rename a sub-tag', tagName: 'the child tag' }, + { description: 'Rename a grandchild tag', tagName: 'the grandchild tag' }, + ]; + + tagDepthScenarios.forEach(({ description, tagName }) => { + describe(description, () => { + beforeEach(async () => { + axiosMock.resetHistory(); + }); + it('should disable tag edit buttons if the taxonomy includes `can_add_tag: false`', async () => { + axiosMock.onGet(rootTagsListUrl).reply(200, { + ...mockTagsResponse, + can_add_tag: false, + }); + cleanup(); + queryClient.clear(); + renderTagListTable(); + await waitForRootTag(); + + openActionsMenuForTag(tagName); + expect(screen.getByText('Rename')).toBeInTheDocument(); + expect(screen.getByText('Rename')).toBeDisabled(); + }); + it('should disable tag edit buttons if tag includes `can_edit: false`', async () => { + axiosMock.reset(); + axiosMock.onGet(rootTagsListUrl).reply(200, mockTagResponseDisallowingEdits); + cleanup(); + queryClient.clear(); + renderTagListTable(); + await waitForRootTag(); + + openActionsMenuForTag(tagName); + expect(screen.getByText('Rename')).toBeInTheDocument(); + expect(screen.getByText('Rename')).toBeDisabled(); + }); + + it('should show tag actions menu', async () => { + openActionsMenuForTag(tagName); + expect(screen.getByText('Add Subtag')).toBeInTheDocument(); + expect(screen.getByText('Rename')).toBeInTheDocument(); + }); + it('should show editable input and action buttons when Rename is selected from actions menu', async () => { + const { row } = await openRenameDraftRow(tagName); + expect(within(row).getByRole('textbox')).toBeInTheDocument(); + // expect the input to be pre-filled with the current tag name + expect(within(row).getByRole('textbox').value).toEqual(tagName); + expect(within(row).getByText('Save')).toBeInTheDocument(); + expect(within(row).getByText('Cancel')).toBeInTheDocument(); + }); + it('should disable Save button until the tag name is changed', async () => { + const { input, saveButton } = await openRenameDraftRow(tagName); + + expect(saveButton).toBeDisabled(); + fireEvent.change(input, { target: { value: `${tagName} updated` } }); + expect(saveButton).not.toBeDisabled(); + }); + it('should save changes and show success toast when Enter is pressed', async () => { + axiosMock.onPatch(/.*/).reply(200, {}); + const { input } = await openRenameDraftRow(tagName); + + fireEvent.change(input, { target: { value: `${tagName} updated` } }); + act(() => { + fireEvent.keyDown(input, { key: 'Enter', code: 'Enter' }); + }); + + await waitFor(() => { + expect(axiosMock.history.patch.length).toBe(1); + expect(axiosMock.history.patch[0].data).toEqual(JSON.stringify({ + tag: tagName, + updated_tag_value: `${tagName} updated`, + })); + }); + expect(await screen.findByText(`Tag "${tagName} updated" updated successfully`)).toBeInTheDocument(); + }); + it('should save changes and show success toast when Save is clicked', async () => { + axiosMock.onPatch(/.*/).reply(200, {}); + const { input, saveButton } = await openRenameDraftRow(tagName); + + fireEvent.change(input, { target: { value: `${tagName} updated` } }); + fireEvent.click(saveButton); + + await waitFor(() => { + expect(axiosMock.history.patch.length).toBe(1); + expect(axiosMock.history.patch[0].data).toEqual(JSON.stringify({ + tag: tagName, + updated_tag_value: `${tagName} updated`, + })); + }); + expect(await screen.findByText(`Tag "${tagName} updated" updated successfully`)).toBeInTheDocument(); + }); + it('should cancel editing and revert to original name when Esc is pressed', async () => { + const { input } = await openRenameDraftRow(tagName); + + fireEvent.change(input, { target: { value: `${tagName} updated` } }); + fireEvent.keyDown(input, { key: 'Escape', code: 'Escape' }); + + expect(screen.queryByRole('textbox')).not.toBeInTheDocument(); + expect(screen.getByText(tagName)).toBeInTheDocument(); + expect(axiosMock.history.patch.length).toBe(0); + }); + it('should cancel editing and revert to original name when Cancel is clicked', async () => { + const { input, cancelButton } = await openRenameDraftRow(tagName); + + fireEvent.change(input, { target: { value: `${tagName} updated` } }); + fireEvent.click(cancelButton); + + expect(screen.queryByRole('textbox')).not.toBeInTheDocument(); + expect(screen.getByText(tagName)).toBeInTheDocument(); + expect(axiosMock.history.patch.length).toBe(0); + }); + }); + }); + + describe('Nested behavior', () => { + beforeEach(async () => { + axiosMock.resetHistory(); + }); + + it('should keep the parent-child relationships in the updated tree data when renaming a parent tag', async () => { + // this only tests that the frontend is updated correctly before reloading data; + // the rest is covered by the backend tests for the rename endpoint + + axiosMock.onPatch(/.*/).reply(200, {}); + const { input, saveButton } = await openRenameDraftRow('root tag 1'); + + fireEvent.change(input, { target: { value: 'root tag 1 updated' } }); + fireEvent.click(saveButton); + + await waitFor(() => { + expect(axiosMock.history.patch.length).toBe(1); + expect(axiosMock.history.patch[0].data).toEqual(JSON.stringify({ + tag: 'root tag 1', + updated_tag_value: 'root tag 1 updated', + })); + }); + // make sure rows are not already expanded by checking that the child tag is not visible before expanding + expect(screen.queryAllByText('the child tag')?.length).toBeFalsy(); + fireEvent.click(await screen.findByLabelText('Show Subtags')); + // expect the child tag to still be present under the renamed parent tag + expect(await screen.findByText('the child tag')).toBeInTheDocument(); + // expect the grandchild tag to still be present under the child tag + openActionsMenuForTag('the child tag'); + fireEvent.click(await screen.findByLabelText('Show Subtags')); + expect(await screen.findByText('the grandchild tag')).toBeInTheDocument(); + }); + }); + describe('At smaller max depth', () => { beforeEach(async () => { const maxDepth = 2; @@ -743,7 +987,10 @@ describe('', () => { // depth 2 is innermost when maxDepth=2, so no add-subtag action should be shown const grandchildTagRow = screen.getByText('the grandchild tag').closest('tr'); - expect(within(grandchildTagRow).queryByRole('button', { name: /actions/i })).not.toBeInTheDocument(); + expect(within(grandchildTagRow).queryByRole('button', { name: /actions/i })).toBeInTheDocument(); + openActionsMenuForTag('the grandchild tag'); + expect(within(grandchildTagRow).getByText('Rename')).toBeInTheDocument(); + expect(within(grandchildTagRow).getByText('Add Subtag')).toBeDisabled(); }); }); }); diff --git a/src/taxonomy/tag-list/TagListTable.tsx b/src/taxonomy/tag-list/TagListTable.tsx index 01795b8917..e8b7ebac0b 100644 --- a/src/taxonomy/tag-list/TagListTable.tsx +++ b/src/taxonomy/tag-list/TagListTable.tsx @@ -5,7 +5,7 @@ import React, { } from 'react'; import { useIntl } from '@edx/frontend-platform/i18n'; import type { PaginationState } from '@tanstack/react-table'; -import { useTagListData, useCreateTag } from '../data/apiHooks'; +import { useTagListData, useCreateTag, useUpdateTag } from '../data/apiHooks'; import { TagTree } from './tagTree'; import { TableView } from '../tree-table'; import type { @@ -80,6 +80,7 @@ const TagListTable = ({ taxonomyId, maxDepth }: TagListTableProps) => { enabled: tableMode === TABLE_MODES.VIEW, }); const createTagMutation = useCreateTag(taxonomyId); + const updateTagMutation = useUpdateTag(taxonomyId); const pageCount = tagList?.numPages ?? -1; // TODO: to make this more readable, introduce a React context for the TagListTable instead of passing props. @@ -90,6 +91,7 @@ const TagListTable = ({ taxonomyId, maxDepth }: TagListTableProps) => { setTagTree, setDraftError, createTagMutation, + updateTagMutation, enterPreviewMode, setToast, setIsCreatingTopTag, @@ -107,19 +109,19 @@ const TagListTable = ({ taxonomyId, maxDepth }: TagListTableProps) => { onStartDraft: enterDraftMode, setActiveActionMenuRowId, hasOpenDraft, + canAddTag: tagList?.canAddTag !== false, draftError, setDraftError, isSavingDraft: createTagMutation.isPending, maxDepth, - creatingParentId, }), [ isCreatingTopTag, - editingRowId, tableMode, activeActionMenuRowId, hasOpenDraft, creatingParentId, + tagList?.canAddTag, draftError, createTagMutation.isPending, maxDepth, @@ -157,7 +159,9 @@ const TagListTable = ({ taxonomyId, maxDepth }: TagListTableProps) => { isCreatingTopRow: isCreatingTopTag, draftError, createRowMutation: createTagMutation, + updateRowMutation: updateTagMutation, handleCreateRow: handleCreateTag, + handleUpdateRow: handleUpdateTag, toast, setToast, setIsCreatingTopRow: setIsCreatingTopTag, @@ -166,6 +170,8 @@ const TagListTable = ({ taxonomyId, maxDepth }: TagListTableProps) => { setCreatingParentId, setDraftError, validate, + editingRowId, + setEditingRowId, }} /> ); diff --git a/src/taxonomy/tag-list/hooks.test.tsx b/src/taxonomy/tag-list/hooks.test.tsx index 7d799afceb..fc3dbdaebb 100644 --- a/src/taxonomy/tag-list/hooks.test.tsx +++ b/src/taxonomy/tag-list/hooks.test.tsx @@ -1,6 +1,6 @@ import React from 'react'; import { IntlProvider } from '@edx/frontend-platform/i18n'; -import { act, renderHook } from '@testing-library/react'; +import { act, renderHook, waitFor } from '@testing-library/react'; import { TagTree } from './tagTree'; import { useEditActions, useTableModes } from './hooks'; @@ -44,6 +44,8 @@ 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 setTagTree = jest.fn(); const setDraftError = jest.fn(); const enterPreviewMode = jest.fn(); @@ -63,6 +65,7 @@ describe('useEditActions', () => { setCreatingParentId, exitDraftWithoutSave, setEditingRowId, + updateTagMutation: updateTagMutation as any, ...(overrides as any), }; @@ -139,7 +142,9 @@ describe('useEditActions', () => { await actions.handleUpdateTag('updated', 'original'); }); - expect(enterPreviewMode).toHaveBeenCalled(); + await waitFor(() => { + expect(enterPreviewMode).toHaveBeenCalled(); + }); expect(setToast).toHaveBeenCalledWith({ show: true, message: 'Tag "updated" updated successfully', diff --git a/src/taxonomy/tag-list/hooks.ts b/src/taxonomy/tag-list/hooks.ts index a350e3eabe..f92531bae4 100644 --- a/src/taxonomy/tag-list/hooks.ts +++ b/src/taxonomy/tag-list/hooks.ts @@ -1,7 +1,7 @@ import { useReducer } from 'react'; import { useIntl } from '@edx/frontend-platform/i18n'; -import { useCreateTag } from '../data/apiHooks'; +import { useCreateTag, useUpdateTag } from '../data/apiHooks'; import { TagTree } from './tagTree'; import { TagListTableError } from './errors'; import type { RowId } from '../tree-table/types'; @@ -45,6 +45,7 @@ interface UseEditActionsParams { setCreatingParentId: React.Dispatch>; exitDraftWithoutSave: () => void; setEditingRowId: React.Dispatch>; + updateTagMutation: ReturnType; } const getInlineValidationMessage = (value: string, intl: ReturnType): string => { @@ -111,9 +112,20 @@ const useEditActions = ({ setToast, setIsCreatingTopTag, setCreatingParentId, + exitDraftWithoutSave, setEditingRowId, + updateTagMutation, }: UseEditActionsParams) => { const intl = useIntl(); + + const updateTableAfterRename = (oldValue: string, newValue: string) => { + setTagTree((currentTagTree) => { + const nextTree = currentTagTree || new TagTree([]); + nextTree.editTagValue(oldValue, newValue); + return nextTree; + }); + }; + const updateTableWithoutDataReload = (value: string, parentTagValue: string | null = null) => { setTagTree((currentTagTree) => { const nextTree = currentTagTree || new TagTree([]); @@ -179,14 +191,31 @@ const useEditActions = ({ const handleUpdateTag = async (value: string, originalValue: string) => { const trimmed = value.trim(); - if (trimmed && trimmed !== originalValue) { + if (!validate(trimmed, 'soft')) { + return; + } + + if (trimmed === originalValue) { + setEditingRowId(null); + exitDraftWithoutSave(); + return; + } + + try { + setDraftError(''); + await updateTagMutation.mutateAsync({ value: trimmed, originalValue }); + updateTableAfterRename(originalValue, trimmed); enterPreviewMode(); + setEditingRowId(null); setToast({ show: true, message: intl.formatMessage(messages.tagUpdateSuccessMessage, { name: trimmed }), }); + } catch (error) { + const message = intl.formatMessage(messages.tagUpdateErrorMessage, { errorMessage: (error as Error)?.message }); + setDraftError((error as Error)?.message || intl.formatMessage(messages.tagUpdateErrorMessage, { errorMessage: '' })); + setToast({ show: true, message }); } - setEditingRowId(null); }; return { diff --git a/src/taxonomy/tag-list/messages.ts b/src/taxonomy/tag-list/messages.ts index 2e9b7ecadb..f40f5f4d05 100644 --- a/src/taxonomy/tag-list/messages.ts +++ b/src/taxonomy/tag-list/messages.ts @@ -53,6 +53,14 @@ const messages = defineMessages({ id: 'course-authoring.tag-list.hide-subtags.button-label', defaultMessage: 'Hide Subtags', }, + tagUpdateErrorMessage: { + id: 'course-authoring.tag-list.update-error', + defaultMessage: 'Error updating tag: {errorMessage}', + }, + renameTag: { + id: 'course-authoring.tag-list.rename-tag', + defaultMessage: 'Rename', + }, }); export default messages; diff --git a/src/taxonomy/tag-list/tagColumns.tsx b/src/taxonomy/tag-list/tagColumns.tsx index c70b415a69..4710d92bc7 100644 --- a/src/taxonomy/tag-list/tagColumns.tsx +++ b/src/taxonomy/tag-list/tagColumns.tsx @@ -20,6 +20,8 @@ import type { } from '../tree-table/types'; import OptionalExpandLink from './OptionalExpandLink'; +const EDITABLE_COLUMNS = ['value']; + interface TagListRowData extends TreeRowData { depth: number; childCount: number; @@ -40,11 +42,11 @@ interface GetColumnsArgs { onStartDraft: () => void; setActiveActionMenuRowId: (id: RowId | null) => void; hasOpenDraft: boolean; + canAddTag: boolean; draftError: string; setDraftError: (error: string) => void; isSavingDraft: boolean; maxDepth: number; - creatingParentId: RowId | null; } interface ActionsHeaderProps { @@ -55,6 +57,7 @@ interface ActionsHeaderProps { setActiveActionMenuRowId: (id: RowId | null) => void; hasOpenDraft: boolean; draftInProgressHintId: string; + canAddTag: boolean; } const ActionsHeader = ({ @@ -64,6 +67,7 @@ const ActionsHeader = ({ setEditingRowId, setActiveActionMenuRowId, hasOpenDraft, + canAddTag, draftInProgressHintId, }: ActionsHeaderProps) => { const intl = useIntl(); @@ -82,7 +86,7 @@ const ActionsHeader = ({ setEditingRowId(null); setActiveActionMenuRowId(null); }} - disabled={hasOpenDraft} + disabled={hasOpenDraft || !canAddTag} aria-describedby={hasOpenDraft ? draftInProgressHintId : undefined} /> @@ -93,9 +97,21 @@ interface ActionsMenuProps { rowData: TagListRowData; startSubtagDraft: () => void; disableAddSubtag: boolean; + editTag: () => void; + disableEditTag: boolean; + reachedMaxDepth: (row: Row) => boolean; + row: Row; } -const ActionsMenu = ({ rowData, startSubtagDraft, disableAddSubtag }: ActionsMenuProps) => { +const ActionsMenu = ({ + rowData, + row, + startSubtagDraft, + disableAddSubtag, + editTag, + disableEditTag, + reachedMaxDepth, +}: ActionsMenuProps) => { const intl = useIntl(); return ( @@ -113,14 +129,21 @@ const ActionsMenu = ({ rowData, startSubtagDraft, disableAddSubtag }: ActionsMen {intl.formatMessage(messages.addSubtag)} + + {intl.formatMessage(messages.renameTag)} + ); -} +}; function getColumns({ setIsCreatingTopTag, @@ -129,11 +152,11 @@ function getColumns({ onStartDraft, setActiveActionMenuRowId, hasOpenDraft, + canAddTag, setDraftError, maxDepth, - creatingParentId, }: GetColumnsArgs): TreeColumnDef[] { - const canAddSubtag = (row: Row) => row.depth < maxDepth; + const reachedMaxDepth = (row: Row) => row.depth >= maxDepth; const draftInProgressHintId = 'tag-list-draft-in-progress-hint'; return [ @@ -164,16 +187,19 @@ function getColumns({ setActiveActionMenuRowId={setActiveActionMenuRowId} hasOpenDraft={hasOpenDraft} draftInProgressHintId={draftInProgressHintId} + canAddTag={canAddTag} /> ), cell: ({ row }) => { const rowData = asTagListRowData(row); - if (rowData.isNew || rowData.isEditing || !canAddSubtag(row)) { + if (rowData.isNew || rowData.isEditing) { return
; } - const disableAddSubtag = hasOpenDraft && creatingParentId !== rowData.id; + const disableAddSubtag = hasOpenDraft || !canAddTag; + const disableEditTag = hasOpenDraft || !canAddTag || row.original.canChangeTag === false; + const startSubtagDraft = () => { onStartDraft(); setDraftError(''); @@ -184,9 +210,26 @@ function getColumns({ row.toggleExpanded(true); }; + const editTag = () => { + onStartDraft(); + setDraftError(''); + setEditingRowId(`${rowData.id}:${rowData.value}`); + setCreatingParentId(null); + setIsCreatingTopTag(false); + setActiveActionMenuRowId(null); + }; + return (
- +
); }, @@ -194,4 +237,4 @@ function getColumns({ ]; } -export { getColumns }; +export { getColumns, EDITABLE_COLUMNS }; diff --git a/src/taxonomy/tree-table/CreateRow.tsx b/src/taxonomy/tree-table/CreateRow.tsx index 57ea7dcfd2..4091db0a56 100644 --- a/src/taxonomy/tree-table/CreateRow.tsx +++ b/src/taxonomy/tree-table/CreateRow.tsx @@ -6,6 +6,20 @@ import { EditableCell } from './EditableCell'; import type { CreateRowMutationState, TreeColumnDef } from './types'; import messages from './messages'; +interface DraftRowProps { + draftError: string; + initialValue?: string; + onSave: (value: string) => void; + onCancel: () => void; + mutationState: CreateRowMutationState; + columns: TreeColumnDef[]; + indent?: number; + validate: (value: string, mode?: 'soft' | 'hard') => boolean; + requireValueChangeToEnableSave?: boolean; + rowTestId?: string; + rowId?: string; +} + interface CreateRowProps { draftError: string; setDraftError: (error: string) => void; @@ -18,57 +32,73 @@ interface CreateRowProps { validate: (value: string, mode?: 'soft' | 'hard') => boolean; } -const CreateRow: React.FC = ({ +interface EditRowProps { + draftError: string; + setDraftError: (error: string) => void; + initialValue: string; + handleUpdateRow: (value: string) => void; + cancelEditRow: () => void; + updateRowMutation: CreateRowMutationState; + columns: TreeColumnDef[]; + indent?: number; + validate: (value: string, mode?: 'soft' | 'hard') => boolean; +} + +const DraftRow: React.FC = ({ draftError, - setDraftError, - handleCreateRow, - setIsCreatingTopRow, - exitDraftWithoutSave, - createRowMutation, + initialValue = '', + onSave, + onCancel, + mutationState, columns, indent = 0, validate, + requireValueChangeToEnableSave = false, + rowTestId, + rowId, }) => { - const [newRowValue, setNewRowValue] = useState(''); - const intl = useIntl(); + const [rowValue, setRowValue] = useState(initialValue); const [saveDisabled, setSaveDisabled] = useState(true); + const intl = useIntl(); - const handleValueChange = (e: React.ChangeEvent) => { - const { value } = e.target; - setNewRowValue(value); + const updateSaveDisabled = (value: string) => { + const trimmedValue = value.trim(); const isValid = validate(value, 'soft'); - setSaveDisabled(!isValid || createRowMutation.isPending || false); + const isUnchanged = requireValueChangeToEnableSave && trimmedValue === initialValue.trim(); + setSaveDisabled(!isValid || !trimmedValue || isUnchanged || mutationState.isPending || false); }; - const handleCancel = () => { - setDraftError(''); - setNewRowValue(''); - setIsCreatingTopRow(false); - exitDraftWithoutSave(); + const handleValueChange = (e: React.ChangeEvent) => { + const { value } = e.target; + setRowValue(value); + updateSaveDisabled(value); }; const handleSave = () => { - handleCreateRow(newRowValue.trim()); + onSave(rowValue.trim()); }; const handleValueCellKeyPress = (e: React.KeyboardEvent) => { - if (e.key === 'Enter' && newRowValue.trim() && !createRowMutation.isPending && !draftError) { + if (e.key === 'Enter' && !saveDisabled && !draftError) { e.preventDefault(); handleSave(); - } else if (e.key === 'Escape') { + return; + } + + if (e.key === 'Escape') { e.preventDefault(); - handleCancel(); + onCancel(); } }; return ( - - +
= ({ @@ -94,7 +124,7 @@ const CreateRow: React.FC = ({ {intl.formatMessage(messages.saveButtonLabel)} - {createRowMutation.isPending && ( + {mutationState.isPending && ( = ({ ); }; -export { CreateRow }; +const CreateRow: React.FC = ({ + draftError, + setDraftError, + handleCreateRow, + setIsCreatingTopRow, + exitDraftWithoutSave, + createRowMutation, + columns, + indent = 0, + validate, +}) => { + const handleCancel = () => { + setDraftError(''); + setIsCreatingTopRow(false); + exitDraftWithoutSave(); + }; + + return ( + + ); +}; + +const EditRow: React.FC = ({ + draftError, + setDraftError, + initialValue, + handleUpdateRow, + cancelEditRow, + updateRowMutation, + columns, + indent = 0, + validate, +}) => { + const handleCancel = () => { + setDraftError(''); + cancelEditRow(); + }; + + return ( + + ); +}; + +export { CreateRow, EditRow }; diff --git a/src/taxonomy/tree-table/NestedRows.test.tsx b/src/taxonomy/tree-table/NestedRows.test.tsx index 1eda32fe41..0909ad5a86 100644 --- a/src/taxonomy/tree-table/NestedRows.test.tsx +++ b/src/taxonomy/tree-table/NestedRows.test.tsx @@ -8,6 +8,17 @@ const wrapper = ({ children }: { children: React.ReactNode }) => ( {children} ); +const defaultRequiredProps = { + setIsCreatingTopRow: jest.fn(), + createRowMutation: {}, + updateRowMutation: {}, + handleUpdateRow: jest.fn(), + editingRowId: null, + setEditingRowId: jest.fn(), + exitDraftWithoutSave: jest.fn(), + validate: () => true, +}; + const makeCell = (id: string, content: string) => ({ id, column: { columnDef: { cell: () => content } }, @@ -41,9 +52,7 @@ describe('NestedRows', () => { true} + {...defaultRequiredProps} /> , @@ -74,9 +83,8 @@ describe('NestedRows', () => { creatingParentId={2} setCreatingParentId={setCreatingParentId} onCancelCreation={onCancelCreation} - setIsCreatingTopRow={jest.fn()} + {...defaultRequiredProps} createRowMutation={{ isPending: false }} - validate={() => true} /> , diff --git a/src/taxonomy/tree-table/NestedRows.tsx b/src/taxonomy/tree-table/NestedRows.tsx index 8facb825b7..216bcfbf18 100644 --- a/src/taxonomy/tree-table/NestedRows.tsx +++ b/src/taxonomy/tree-table/NestedRows.tsx @@ -4,9 +4,10 @@ import { flexRender } from '@tanstack/react-table'; import type { RowId, TreeRow, + TreeColumnDef, CreateRowMutationState, } from './types'; -import { CreateRow } from './CreateRow'; +import { CreateRow, EditRow } from './CreateRow'; interface NestedRowsProps { /** The parent row object from TanStack React Table */ @@ -35,6 +36,18 @@ interface NestedRowsProps { 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; @@ -67,6 +80,12 @@ const NestedRows = ({ setCreatingParentId = () => {}, setIsCreatingTopRow, createRowMutation, + updateRowMutation, + handleUpdateRow, + editingRowId, + setEditingRowId, + exitDraftWithoutSave, + columns = [], validate, }: NestedRowsProps) => { if (!parentRow.getIsExpanded()) { @@ -93,26 +112,43 @@ const NestedRows = ({ const rowData = row.original || row; return ( - - {row.getVisibleCells() - .map((cell, index) => { - const content = flexRender(cell.column.columnDef.cell, cell.getContext()); - const isFirstColumn = index === 0; + {editingRowId === `${row.original.id}:${String(row.original.value)}` ? ( + handleUpdateRow(value, String(row.original.value))} + cancelEditRow={() => { + setEditingRowId(null); + exitDraftWithoutSave(); + }} + updateRowMutation={updateRowMutation} + columns={columns} + indent={indent} + validate={validate} + /> + ) : ( + + {row.getVisibleCells() + .map((cell, index) => { + const content = flexRender(cell.column.columnDef.cell, cell.getContext()); + const isFirstColumn = index === 0; - return ( - - {isFirstColumn ? ( -
{content}
- ) : ( - content - )} - - ); - })} - + return ( + + {isFirstColumn ? ( +
{content}
+ ) : ( + content + )} + + ); + })} + + )}
diff --git a/src/taxonomy/tree-table/TableBody.tsx b/src/taxonomy/tree-table/TableBody.tsx index 77a9b7af21..6e1819a289 100644 --- a/src/taxonomy/tree-table/TableBody.tsx +++ b/src/taxonomy/tree-table/TableBody.tsx @@ -13,7 +13,7 @@ import type { TreeColumnDef, TreeTable, } from './types'; -import { CreateRow } from './CreateRow'; +import { CreateRow, EditRow } from './CreateRow'; interface TableBodyProps { columns: TreeColumnDef[]; @@ -26,9 +26,13 @@ interface TableBodyProps { 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 = ({ @@ -42,9 +46,13 @@ const TableBody = ({ setCreatingParentId, setDraftError, createRowMutation, + updateRowMutation, table, isLoading, validate, + handleUpdateRow, + editingRowId, + setEditingRowId, }: TableBodyProps) => { const intl = useIntl(); @@ -85,14 +93,30 @@ const TableBody = ({ {table.getRowModel().rows.filter(row => row.depth === 0).map(row => ( - - {row.getVisibleCells() - .map((cell, index) => ( - - {flexRender(cell.column.columnDef.cell, cell.getContext())} - - ))} - + {editingRowId === `${row.original.id}:${String(row.original.value)}` ? ( + handleUpdateRow(value, String(row.original.value))} + cancelEditRow={() => { + setEditingRowId(null); + exitDraftWithoutSave(); + }} + updateRowMutation={updateRowMutation} + columns={columns} + validate={validate} + /> + ) : ( + + {row.getVisibleCells() + .map((cell, index) => ( + + {flexRender(cell.column.columnDef.cell, cell.getContext())} + + ))} + + )} ))} diff --git a/src/taxonomy/tree-table/TableView.test.tsx b/src/taxonomy/tree-table/TableView.test.tsx index 47a4720b7f..f1382eeea5 100644 --- a/src/taxonomy/tree-table/TableView.test.tsx +++ b/src/taxonomy/tree-table/TableView.test.tsx @@ -29,6 +29,7 @@ const baseProps = () => ({ isCreatingTopRow: false, draftError: '', createRowMutation: { isPending: false, isError: false }, + updateRowMutation: { isPending: false, isError: false }, toast: { show: false, message: '', variant: 'success' }, setToast: jest.fn(), setIsCreatingTopRow: jest.fn(), @@ -38,6 +39,9 @@ const baseProps = () => ({ setCreatingParentId: jest.fn(), setDraftError: jest.fn(), validate: jest.fn(() => true), + handleUpdateRow: jest.fn(), + editingRowId: null, + setEditingRowId: jest.fn(), }); describe('TableView', () => { diff --git a/src/taxonomy/tree-table/TableView.tsx b/src/taxonomy/tree-table/TableView.tsx index 19f347b06d..79aa625259 100644 --- a/src/taxonomy/tree-table/TableView.tsx +++ b/src/taxonomy/tree-table/TableView.tsx @@ -42,6 +42,7 @@ interface TableViewProps { isCreatingTopRow: boolean; draftError: string; createRowMutation: CreateRowMutationState; + updateRowMutation: CreateRowMutationState; toast: ToastState; setToast: React.Dispatch>; setIsCreatingTopRow: (isCreating: boolean) => void; @@ -51,6 +52,9 @@ interface TableViewProps { 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; } const TableView = ({ @@ -64,6 +68,7 @@ const TableView = ({ isCreatingTopRow, draftError, createRowMutation, + updateRowMutation, handleCreateRow, toast, setToast, @@ -73,6 +78,9 @@ const TableView = ({ setCreatingParentId, setDraftError, validate, + handleUpdateRow, + editingRowId, + setEditingRowId, }: TableViewProps) => { const intl = useIntl(); @@ -93,11 +101,12 @@ const TableView = ({ const currentPageIndex = table.getState().pagination.pageIndex + 1; const { isError } = createRowMutation; + const { isError: isUpdateError } = updateRowMutation; const [showError, setShowError] = React.useState(true); return ( <> - {isError && showError && ( + {(isError || isUpdateError) && showError && ( setShowError(false)}> {intl.formatMessage(messages.errorSavingTitle)} @@ -157,9 +166,13 @@ const TableView = ({ setCreatingParentId={setCreatingParentId} setDraftError={setDraftError} createRowMutation={createRowMutation} + updateRowMutation={updateRowMutation} table={table} isLoading={isLoading} validate={validate} + handleUpdateRow={handleUpdateRow} + editingRowId={editingRowId} + setEditingRowId={setEditingRowId} />