fix: Taxonomy table QA fixes (#3000)

* fix: taxonomy alignment and error message

* fix: lint and cleanup

* fix: UsageCountDisplay

* fix: tests

* fix: test

* fix: lint

* fix: test accuracy

* fix: typecheck for AxiosError

* fix: PR comments

* fix: Apply suggestions from code review

* fix: lint

* fix: types

Author: jesperhodge

src/messages.ts

@@ -10,6 +10,10 @@ const messages = defineMessages({
     id: 'authoring.alert.support.text',
     defaultMessage: 'Support Page',
   },
+  unknownError: {
+    id: 'authoring.alert.error.unknown',
+    defaultMessage: 'Unknown error',
+  },
 });
 
 export default messages;

src/taxonomy/data/apiHooks.test.jsx

@@ -110,11 +110,19 @@ describe('import taxonomy api calls', () => {
     expect(axiosMock.history.put[0].url).toEqual(apiUrls.tagsPlanImport(1));
   });
 
-  it('should surface duplicate tag error returned as an array', async () => {
-    const duplicateError = 'Tag with value \'ab\' already exists for taxonomy.';
-    axiosMock.onPost(apiUrls.createTag(1)).reply(400, [duplicateError]);
+  it('should surface tag errors', async () => {
+    const duplicateMessage = 'Tag with value \'ab\' already exists for taxonomy.';
+    axiosMock.onPost(apiUrls.createTag(1)).reply(400, [duplicateMessage]);
     const { result } = renderHook(() => useCreateTag(1), { wrapper });
 
-    await expect(result.current.mutateAsync({ value: 'ab' })).rejects.toEqual(Error(duplicateError));
+    try {
+      await result.current.mutateAsync({ value: 'ab' });
+      // expect: if code reaches this line, the test should fail because an error should have been thrown
+      expect('This line should not be reached').toBe(false);
+    } catch (error) {
+      // we check the response data, not the error message, because of how react-query surfaces errors from axios
+      // @ts-ignore
+      expect(error.response.data).toEqual([duplicateMessage]);
+    }
   });
 });

src/taxonomy/data/apiHooks.ts

@@ -13,7 +13,6 @@
 import { useQuery, useMutation, useQueryClient } from '@tanstack/react-query';
 import { camelCaseObject } from '@edx/frontend-platform';
 import { getAuthenticatedHttpClient } from '@edx/frontend-platform/auth';
-import { useIntl } from '@edx/frontend-platform/i18n';
 import { apiUrls, ALL_TAXONOMIES, getApiErrorMessage } from './api';
 import * as api from './api';
 import type { QueryOptions, TagListData } from './types';
@@ -229,18 +228,13 @@ export const useSubTags = (taxonomyId: number, parentTagValue: string) =>
 
 export const useCreateTag = (taxonomyId: number) => {
   const queryClient = useQueryClient();
-  const intl = useIntl();
 
   return useMutation({
     mutationFn: async ({ value, parentTagValue }: { value: string; parentTagValue?: string; }) => {
-      try {
-        await getAuthenticatedHttpClient().post(
-          apiUrls.createTag(taxonomyId),
-          { tag: value, parent_tag_value: parentTagValue },
-        );
-      } catch (err) {
-        throw new Error(getApiErrorMessage(err, intl));
-      }
+      await getAuthenticatedHttpClient().post(
+        apiUrls.createTag(taxonomyId),
+        { tag: value, parent_tag_value: parentTagValue },
+      );
     },
     onSuccess: () => {
       queryClient.invalidateQueries({
@@ -261,14 +255,10 @@ export const useUpdateTag = (taxonomyId: number) => {
 
   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));
-      }
+      await getAuthenticatedHttpClient().patch(
+        apiUrls.updateTag(taxonomyId),
+        { tag: originalValue, updated_tag_value: value },
+      );
     },
     onSuccess: () => {
       queryClient.invalidateQueries({

src/taxonomy/tag-list/OptionalExpandLink.tsx

@@ -4,7 +4,7 @@ import { ExpandLess, ExpandMore } from '@openedx/paragon/icons';
 import { Row } from '@tanstack/react-table';
 import { useIntl } from '@edx/frontend-platform/i18n';
 
-import type { TreeRowData } from '../tree-table/types';
+import type { TreeRowData } from '@src/taxonomy/tree-table/types';
 import messages from './messages';
 
 interface OptionalExpandLinkProps {

src/taxonomy/tag-list/TagListTable.test.jsx

@@ -1,4 +1,5 @@
 import React from 'react';
+import { AxiosError } from 'axios';
 import {
   render,
   waitFor,
@@ -598,8 +599,16 @@ describe('<TagListTable />', () => {
         expect(screen.getByText(/invalid character/i)).toBeInTheDocument();
       });
 
-      it('should show an inline duplicate-name error when the entered root tag already exists', async () => {
-        axiosMock.onPost(createTagUrl).reply(400, ['Tag with this name already exists']);
+      it('should show failure feedback when creating a duplicate root tag name', async () => {
+        axiosMock.onPost(createTagUrl).reply(() => {
+          const error = new AxiosError('Request failed with status code 400');
+          error.response = {
+            data: {
+              tag: ['Tag with this name already exists'],
+            },
+          };
+          return Promise.reject(error);
+        });
 
         fireEvent.click(await screen.findByLabelText('Create Tag'));
         const draftRow = await screen.findAllByRole('row');
@@ -609,12 +618,18 @@ describe('<TagListTable />', () => {
         fireEvent.change(input, { target: { value: 'root tag 1' } });
         fireEvent.click(saveButton);
 
-        expect(await screen.findByText('Tag with this name already exists')).toBeInTheDocument();
+        expect(await screen.findByText('Error creating tag: Tag with this name already exists')).toBeInTheDocument();
       });
 
       it('should keep the inline row and show a failure toast when save request fails', async () => {
-        axiosMock.onPost(createTagUrl).reply(500, {
-          error: 'Internal server error',
+        axiosMock.onPost(createTagUrl).reply(() => {
+          const error = new AxiosError('Request failed with status code 500');
+          error.response = {
+            data: {
+              tag: ['Internal server error'],
+            },
+          };
+          return Promise.reject(error);
         });
 
         fireEvent.click(await screen.findByLabelText('Create Tag'));

src/taxonomy/tag-list/TagListTable.tsx

@@ -4,9 +4,9 @@ import React, {
   useEffect,
 } from 'react';
 import type { PaginationState } from '@tanstack/react-table';
-import { useTagListData, useCreateTag, useUpdateTag } from '../data/apiHooks';
+import { TableView } from '@src/taxonomy/tree-table';
+import { useTagListData, useCreateTag, useUpdateTag } from '@src/taxonomy/data/apiHooks';
 import { TagTree } from './tagTree';
-import { TableView } from '../tree-table';
 import type {
   RowId,
   TreeColumnDef,

src/taxonomy/tag-list/UsageCountDisplay.tsx

@@ -0,0 +1,24 @@
+import {
+  Bubble,
+} from '@openedx/paragon';
+import type { Row } from '@tanstack/react-table';
+import type {
+  TreeRowData,
+} from '@src/taxonomy/tree-table/types';
+import { getTagListRowData } from './utils';
+
+const UsageCountDisplay = ({ row }: { row: Row<TreeRowData>; }) => {
+  const count = getTagListRowData(row).usageCount ?? 0;
+
+  if (count <= 0) {
+    return null;
+  }
+
+  return (
+    <Bubble expandable>
+      {count}
+    </Bubble>
+  );
+};
+
+export default UsageCountDisplay;

src/taxonomy/tag-list/hooks.ts

@@ -1,10 +1,12 @@
 import { useReducer } from 'react';
+import { AxiosError } from 'axios';
 import { useIntl } from '@edx/frontend-platform/i18n';
 
-import { useCreateTag, useUpdateTag } from '../data/apiHooks';
+import globalMessages from '@src/messages';
+import { useCreateTag, useUpdateTag } from '@src/taxonomy/data/apiHooks';
+import type { RowId } from '@src/taxonomy/tree-table/types';
 import { TagTree } from './tagTree';
 import { TagListTableError } from './errors';
-import type { RowId } from '../tree-table/types';
 import {
   TABLE_MODES,
   TRANSITION_TABLE,
@@ -167,6 +169,38 @@ const useEditActions = ({
     return true;
   };
 
+  const formatErrorMessage = (errorMessage: string): string => {
+    // Remove trailing period for better message formatting
+    return errorMessage.replace(/\.$/, '');
+  };
+
+  const getAxiosErrorMessage = (axiosError: AxiosError) => {
+    const responseData = axiosError.response?.data;
+    const tagError = responseData ?
+      Object.entries(responseData)?.find((errItem: [string, unknown]) => (
+        ['tag', 'value', 'updated_tag_value'].includes(errItem[0].toLowerCase())
+      )) :
+      null;
+
+    const errorMessages = tagError ? tagError[1] : (
+      axiosError.message || intl.formatMessage(globalMessages.unknownError)
+    );
+    const errorMessage = Array.isArray(errorMessages) ? errorMessages.join('; ') : String(errorMessages);
+    return formatErrorMessage(errorMessage);
+  };
+
+  const getErrorMessage = (error: unknown): string => {
+    if (error instanceof AxiosError) {
+      return getAxiosErrorMessage(error);
+    }
+
+    if (error instanceof Error && error.message) {
+      return formatErrorMessage(error.message);
+    }
+
+    return intl.formatMessage(globalMessages.unknownError);
+  };
+
   const handleCreateTag = async (value: string, parentTagValue?: string) => {
     const trimmed = value.trim();
 
@@ -186,11 +220,9 @@ const useEditActions = ({
       setIsCreatingTopTag(false);
       setCreatingParentId(null);
     } catch (error) {
-      const message = intl.formatMessage(messages.tagCreationErrorMessage, { errorMessage: (error as Error)?.message });
-      setDraftError(
-        (error as Error)?.message || intl.formatMessage(messages.tagCreationErrorMessage, { errorMessage: '' }),
-      );
-      setToast({ show: true, message });
+      const errorMessage = getErrorMessage(error);
+      setDraftError(errorMessage);
+      setToast({ show: true, message: intl.formatMessage(messages.tagCreationErrorMessage, { errorMessage }) });
     }
   };
 
@@ -217,11 +249,9 @@ const useEditActions = ({
         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 });
+      const errorMessage = getErrorMessage(error);
+      setDraftError(errorMessage);
+      setToast({ show: true, message: intl.formatMessage(messages.tagUpdateErrorMessage, { errorMessage }) });
     }
   };
 

src/taxonomy/tag-list/tagColumns.tsx

@@ -1,5 +1,4 @@
 import {
-  Bubble,
   Icon,
   IconButton,
   IconButtonWithTooltip,
@@ -12,28 +11,19 @@ import {
 import { FormattedMessage, useIntl } from '@edx/frontend-platform/i18n';
 import type { Row } from '@tanstack/react-table';
 
-import messages from './messages';
 import type {
   RowId,
   TreeColumnDef,
   TreeRowData,
-} from '../tree-table/types';
+} 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';
 
 const EDITABLE_COLUMNS = ['value'];
 
-interface TagListRowData extends TreeRowData {
-  depth: number;
-  childCount: number;
-  usageCount?: number;
-  isNew?: boolean;
-  isEditing?: boolean;
-}
-
-const asTagListRowData = (row: Row<TreeRowData>): TagListRowData => (
-  row.original as unknown as TagListRowData
-);
-
 interface GetColumnsArgs {
   setIsCreatingTopTag: (isCreating: boolean) => void;
   setCreatingParentId: (id: RowId | null) => void;
@@ -49,17 +39,6 @@ interface GetColumnsArgs {
   maxDepth: number;
 }
 
-const UsageCountDisplay = ({ row }: { row: Row<TreeRowData>; }) => {
-  const count = asTagListRowData(row).usageCount ?? 0;
-  return (
-    count > 0 && (
-      <Bubble expandable>
-        {count}
-      </Bubble>
-    )
-  );
-};
-
 interface ActionsHeaderProps {
   onStartDraft: () => void;
   setDraftError: (error: string) => void;
@@ -175,7 +154,7 @@ function getColumns({
       cell: ({ row }) => {
         const {
           value,
-        } = asTagListRowData(row);
+        } = getTagListRowData(row);
 
         return (
           <span className="d-flex align-items-center gap-2">
@@ -205,14 +184,14 @@ function getColumns({
         />
       ),
       cell: ({ row }) => {
-        const rowData = asTagListRowData(row);
+        const rowData = getTagListRowData(row);
 
         if (rowData.isNew || rowData.isEditing) {
           return <div className="d-flex gap-2" />;
         }
 
         const disableAddSubtag = hasOpenDraft || !canAddTag;
-        const disableEditTag = hasOpenDraft || row.original.canChangeTag === false;
+        const disableEditTag = hasOpenDraft || rowData.canChangeTag === false;
 
         const startSubtagDraft = () => {
           onStartDraft();

src/taxonomy/tag-list/tagTree.ts

@@ -1,5 +1,5 @@
+import type { TagData } from '@src/taxonomy/data/types';
 import { TagTreeError } from './errors';
-import type { TagData } from '../data/types';
 
 export interface TagTreeNode extends TagData {
   subRows?: TagTreeNode[];