diff --git a/src/content-tags-drawer/ContentTagsDrawer.test.jsx b/src/content-tags-drawer/ContentTagsDrawer.test.jsx index 426f0d5323..0ff99eaf27 100644 --- a/src/content-tags-drawer/ContentTagsDrawer.test.jsx +++ b/src/content-tags-drawer/ContentTagsDrawer.test.jsx @@ -18,7 +18,7 @@ import { } from './data/api.mocks'; import { getContentTaxonomyTagsApiUrl } from './data/api'; -const path = '/content/:contentId/*'; +const path = '/content/:contentId?/*'; const mockOnClose = jest.fn(); const mockSetBlockingSheet = jest.fn(); const mockNavigate = jest.fn(); diff --git a/src/hooks.ts b/src/hooks.ts index d9cde4a80f..d121ddda8f 100644 --- a/src/hooks.ts +++ b/src/hooks.ts @@ -1,6 +1,13 @@ -import { useEffect, useState } from 'react'; +import { + type Dispatch, + type SetStateAction, + useCallback, + useEffect, + useRef, + useState, +} from 'react'; import { history } from '@edx/frontend-platform'; -import { useLocation } from 'react-router-dom'; +import { useLocation, useSearchParams } from 'react-router-dom'; export const useScrollToHashElement = ({ isLoading }: { isLoading: boolean }) => { const [elementWithHash, setElementWithHash] = useState(null); @@ -77,3 +84,109 @@ export const useLoadOnScroll = ( return () => { }; }, [hasNextPage, isFetchingNextPage, fetchNextPage]); }; + +/** + * Types used by the useStateWithUrlSearchParam hook. + */ +export type FromStringFn = (value: string | null) => Type | undefined; +export type ToStringFn = (value: Type | undefined) => string | undefined; + +/** + * Hook that stores/retrieves state variables using the URL search parameters. + * This function is overloaded to accept simple Types or Array values. + * + * @param defaultValue: Type | Type[] + * Returned when no valid value is found in the url search parameter. + * If an Array Type is used, then an Array Type of state values will be maintained. + * @param paramName: name of the url search parameter to store this value in. + * @param fromString: returns the Type equivalent of the given string value, + * or undefined if the value is invalid. + * @param toString: returns the string equivalent of the given Type value. + * Return defaultValue to clear the url search paramName. + */ +export function useStateWithUrlSearchParam( + defaultValue: Type[], + paramName: string, + fromString: FromStringFn, + toString: ToStringFn, +): [value: Type[], setter: Dispatch>]; +export function useStateWithUrlSearchParam( + defaultValue: Type, + paramName: string, + fromString: FromStringFn, + toString: ToStringFn, +): [value: Type, setter: Dispatch>]; +export function useStateWithUrlSearchParam( + defaultValue: Type | Type[], + paramName: string, + fromString: FromStringFn, + toString: ToStringFn, +): [value: Type | Type[], setter: Dispatch>] { + // STATE WORKAROUND: + // If we use this hook to control multiple state parameters on the same + // page, we can run into state update issues. Because our state variables + // are actually stored in setSearchParams, and not in separate variables like + // useState would do, the searchParams "previous" state may not be updated + // for sequential calls to returnSetter in the same render loop (as we do in + // SearchManager's clearFilters). + // + // One workaround could be to use window.location.search as the "previous" + // value when returnSetter constructs the new URLSearchParams. This works + // fine with BrowserRouter, however our test suite uses MemoryRouter, and + // that router doesn't store URL search params, cf + // https://github.com/remix-run/react-router/issues/9757 + // + // So instead, we maintain a reference to the current useLocation() + // object, and use its search params as the "previous" value when + // initializing URLSearchParams. + const location = useLocation(); + const locationRef = useRef(location); + const [searchParams, setSearchParams] = useSearchParams(); + const paramValues = searchParams.getAll(paramName); + + const returnValue: Type | Type[] = ( + defaultValue instanceof Array + ? (paramValues.map(fromString).filter((v) => v !== undefined)) as Type[] + : fromString(paramValues[0]) + ) ?? defaultValue; + + // Update the url search parameter using: + type ReturnSetterParams = ( + // a Type value + value?: Type | Type[] + // or a function that returns a Type from the previous returnValue + | ((value: Type | Type[]) => Type | Type[]) + ) => void; + const returnSetter: Dispatch> = useCallback((value) => { + setSearchParams((/* previous -- see STATE WORKAROUND above */) => { + const useValue = value instanceof Function ? value(returnValue) : value; + const paramValue: string | string[] | undefined = ( + useValue instanceof Array + ? useValue.map(toString).filter((v) => v !== undefined) as string[] + : toString(useValue) + ); + + const newSearchParams = new URLSearchParams(locationRef.current.search); + if (paramValue === undefined || paramValue === defaultValue) { + // If the provided value was invalid (toString returned undefined) or + // the same as the defaultValue, remove it from the search params. + newSearchParams.delete(paramName); + } else if (paramValue instanceof Array) { + // Replace paramName with the new list of values. + newSearchParams.delete(paramName); + paramValue.forEach((v) => v && newSearchParams.append(paramName, v)); + } else { + // Otherwise, just set the new (single) value. + newSearchParams.set(paramName, paramValue); + } + + // Update locationRef + locationRef.current.search = newSearchParams.toString(); + + return newSearchParams; + }, { replace: true }); + }, [returnValue, setSearchParams]); + + // Return the computed value and wrapped set state function + return [returnValue, returnSetter]; +} diff --git a/src/library-authoring/LibraryAuthoringPage.test.tsx b/src/library-authoring/LibraryAuthoringPage.test.tsx index 0c725fbf2d..2b55375fd2 100644 --- a/src/library-authoring/LibraryAuthoringPage.test.tsx +++ b/src/library-authoring/LibraryAuthoringPage.test.tsx @@ -137,7 +137,7 @@ describe('', () => { expect((await screen.findAllByText(libraryTitle))[0]).toBeInTheDocument(); fireEvent.click(screen.getByRole('tab', { name: 'Collections' })); - expect(screen.getByText('You have not added any collections to this library yet.')).toBeInTheDocument(); + expect(await screen.findByText('You have not added any collections to this library yet.')).toBeInTheDocument(); // Open Create collection modal const addCollectionButton = screen.getByRole('button', { name: /add collection/i }); @@ -151,7 +151,7 @@ describe('', () => { expect(collectionModalHeading).not.toBeInTheDocument(); fireEvent.click(screen.getByRole('tab', { name: 'All Content' })); - expect(screen.getByText('You have not added any content to this library yet.')).toBeInTheDocument(); + expect(await screen.findByText('You have not added any content to this library yet.')).toBeInTheDocument(); const addComponentButton = screen.getByRole('button', { name: /add component/i }); fireEvent.click(addComponentButton); @@ -196,15 +196,15 @@ describe('', () => { // should not be impacted by the search await waitFor(() => { expect(fetchMock).toHaveFetchedTimes(2, searchEndpoint, 'post'); }); - expect(screen.getByText('No matching components found in this library.')).toBeInTheDocument(); + expect(await screen.findByText('No matching components found in this library.')).toBeInTheDocument(); // Navigate to the components tab fireEvent.click(screen.getByRole('tab', { name: 'Components' })); - expect(screen.getByText('No matching components found in this library.')).toBeInTheDocument(); + expect(await screen.findByText('No matching components found in this library.')).toBeInTheDocument(); // Navigate to the collections tab fireEvent.click(screen.getByRole('tab', { name: 'Collections' })); - expect(screen.getByText('No matching collections found in this library.')).toBeInTheDocument(); + expect(await screen.findByText('No matching collections found in this library.')).toBeInTheDocument(); // Go back to Home tab // This step is necessary to avoid the url change leak to other tests @@ -431,27 +431,23 @@ describe('', () => { // Click on the first collection fireEvent.click((await screen.findByText('Collection 1'))); - const sidebar = screen.getByTestId('library-sidebar'); - - const { getByRole } = within(sidebar); - // Click on the Details tab - fireEvent.click(getByRole('tab', { name: 'Details' })); + fireEvent.click(screen.getByRole('tab', { name: 'Details' })); // Change to a component fireEvent.click((await screen.findAllByText('Introduction to Testing'))[0]); // Check that the Details tab is still selected - expect(getByRole('tab', { name: 'Details' })).toHaveAttribute('aria-selected', 'true'); + expect(screen.getByRole('tab', { name: 'Details' })).toHaveAttribute('aria-selected', 'true'); // Click on the Previews tab - fireEvent.click(getByRole('tab', { name: 'Preview' })); + fireEvent.click(screen.getByRole('tab', { name: 'Preview' })); // Switch back to the collection fireEvent.click((await screen.findByText('Collection 1'))); // The Manage (default) tab should be selected because the collection does not have a Preview tab - expect(getByRole('tab', { name: 'Manage' })).toHaveAttribute('aria-selected', 'true'); + expect(screen.getByRole('tab', { name: 'Manage' })).toHaveAttribute('aria-selected', 'true'); }); it('can filter by capa problem type', async () => { @@ -505,6 +501,15 @@ describe('', () => { // eslint-disable-next-line no-await-in-loop await validateSubmenu(key); } + }); + + it('can filter by block type', async () => { + await renderLibraryPage(); + + // Ensure the search endpoint is called + await waitFor(() => { expect(fetchMock).toHaveFetchedTimes(1, searchEndpoint, 'post'); }); + const filterButton = screen.getByRole('button', { name: /type/i }); + fireEvent.click(filterButton); // Validate click on Problem type const problemMenu = screen.getAllByText('Problem')[0]; @@ -528,15 +533,13 @@ describe('', () => { }); // Validate clear filters - const submenu = screen.getByText('Checkboxes'); - expect(submenu).toBeInTheDocument(); - fireEvent.click(submenu); + fireEvent.click(problemMenu); const clearFitlersButton = screen.getByRole('button', { name: /clear filters/i }); fireEvent.click(clearFitlersButton); await waitFor(() => { expect(fetchMock).toHaveBeenLastCalledWith(searchEndpoint, { - body: expect.not.stringContaining(`content.problem_types = ${problemTypes.Checkboxes}`), + body: expect.not.stringContaining('block_type = problem'), method: 'POST', headers: expect.anything(), }); @@ -706,6 +709,34 @@ describe('', () => { }); }); + it('Disables Type filter on Collections tab', async () => { + await renderLibraryPage(); + + expect(await screen.findByText('Content library')).toBeInTheDocument(); + expect((await screen.findAllByText(libraryTitle))[0]).toBeInTheDocument(); + expect((await screen.findAllByText('Introduction to Testing'))[0]).toBeInTheDocument(); + expect((await screen.findAllByText('Collection 1'))[0]).toBeInTheDocument(); + + // Filter by Text block type + fireEvent.click(screen.getByRole('button', { name: /type/i })); + fireEvent.click(screen.getByRole('checkbox', { name: /text/i })); + // Escape to close the Types filter drop-down and re-enable the tabs + fireEvent.keyDown(screen.getByRole('button', { name: /type/i }), { key: 'Escape' }); + + // Navigate to the collections tab + fireEvent.click(await screen.findByRole('tab', { name: 'Collections' })); + expect((await screen.findAllByText('Collection 1'))[0]).toBeInTheDocument(); + // No Types filter shown + expect(screen.queryByRole('button', { name: /type/i })).not.toBeInTheDocument(); + + // Navigate to the components tab + fireEvent.click(screen.getByRole('tab', { name: 'Components' })); + // Text components should be shown + expect((await screen.findAllByText('Introduction to Testing'))[0]).toBeInTheDocument(); + // Types filter is shown + expect(screen.getByRole('button', { name: /type/i })).toBeInTheDocument(); + }); + it('Shows an error if libraries V2 is disabled', async () => { const { axiosMock } = initializeMocks(); axiosMock.onGet(getStudioHomeApiUrl()).reply(200, { diff --git a/src/library-authoring/LibraryAuthoringPage.tsx b/src/library-authoring/LibraryAuthoringPage.tsx index 88b92eb1af..b952310f5d 100644 --- a/src/library-authoring/LibraryAuthoringPage.tsx +++ b/src/library-authoring/LibraryAuthoringPage.tsx @@ -1,4 +1,4 @@ -import { useEffect, useState } from 'react'; +import { useCallback, useEffect, useState } from 'react'; import { Helmet } from 'react-helmet'; import classNames from 'classnames'; import { StudioFooter } from '@edx/frontend-component-footer'; @@ -16,12 +16,7 @@ import { Tabs, } from '@openedx/paragon'; import { Add, ArrowBack, InfoOutline } from '@openedx/paragon/icons'; -import { - Link, - useLocation, - useNavigate, - useSearchParams, -} from 'react-router-dom'; +import { Link } from 'react-router-dom'; import Loading from '../generic/Loading'; import SubHeader from '../generic/sub-header/SubHeader'; @@ -35,12 +30,14 @@ import { SearchContextProvider, SearchKeywordsField, SearchSortWidget, + TypesFilterData, } from '../search-manager'; -import LibraryContent, { ContentType } from './LibraryContent'; +import LibraryContent from './LibraryContent'; import { LibrarySidebar } from './library-sidebar'; import { useComponentPickerContext } from './common/context/ComponentPickerContext'; import { useLibraryContext } from './common/context/LibraryContext'; import { SidebarBodyComponentId, useSidebarContext } from './common/context/SidebarContext'; +import { ContentType, useLibraryRoutes } from './routes'; import messages from './messages'; @@ -51,7 +48,7 @@ const HeaderActions = () => { const { openAddContentSidebar, - openInfoSidebar, + openLibrarySidebar, closeLibrarySidebar, sidebarComponentInfo, } = useSidebarContext(); @@ -60,13 +57,19 @@ const HeaderActions = () => { const infoSidebarIsOpen = sidebarComponentInfo?.type === SidebarBodyComponentId.Info; - const handleOnClickInfoSidebar = () => { + const { navigateTo } = useLibraryRoutes(); + const handleOnClickInfoSidebar = useCallback(() => { if (infoSidebarIsOpen) { closeLibrarySidebar(); } else { - openInfoSidebar(); + openLibrarySidebar(); } - }; + + if (!componentPickerMode) { + // Reset URL to library home + navigateTo({ componentId: '', collectionId: '' }); + } + }, [navigateTo, sidebarComponentInfo, closeLibrarySidebar, openLibrarySidebar]); return (
@@ -124,8 +127,6 @@ interface LibraryAuthoringPageProps { const LibraryAuthoringPage = ({ returnToLibrarySelection }: LibraryAuthoringPageProps) => { const intl = useIntl(); - const location = useLocation(); - const navigate = useNavigate(); const { isLoadingPage: isLoadingStudioHome, @@ -139,29 +140,34 @@ const LibraryAuthoringPage = ({ returnToLibrarySelection }: LibraryAuthoringPage libraryData, isLoadingLibraryData, showOnlyPublished, + componentId, + collectionId, } = useLibraryContext(); const { openInfoSidebar, sidebarComponentInfo } = useSidebarContext(); - const [activeKey, setActiveKey] = useState(ContentType.home); - - useEffect(() => { - const currentPath = location.pathname.split('/').pop(); + const { insideCollections, insideComponents, navigateTo } = useLibraryRoutes(); - if (componentPickerMode || currentPath === libraryId || currentPath === '') { - setActiveKey(ContentType.home); - } else if (currentPath && currentPath in ContentType) { - setActiveKey(ContentType[currentPath] || ContentType.home); + // The activeKey determines the currently selected tab. + const getActiveKey = () => { + if (componentPickerMode) { + return ContentType.home; } - }, []); + if (insideCollections) { + return ContentType.collections; + } + if (insideComponents) { + return ContentType.components; + } + return ContentType.home; + }; + const [activeKey, setActiveKey] = useState(getActiveKey); useEffect(() => { if (!componentPickerMode) { - openInfoSidebar(); + openInfoSidebar(componentId, collectionId); } }, []); - const [searchParams] = useSearchParams(); - if (isLoadingLibraryData) { return ; } @@ -181,10 +187,7 @@ const LibraryAuthoringPage = ({ returnToLibrarySelection }: LibraryAuthoringPage const handleTabChange = (key: ContentType) => { setActiveKey(key); if (!componentPickerMode) { - navigate({ - pathname: key, - search: searchParams.toString(), - }); + navigateTo({ contentType: key }); } }; @@ -218,6 +221,9 @@ const LibraryAuthoringPage = ({ returnToLibrarySelection }: LibraryAuthoringPage extraFilter.push(activeTypeFilters[activeKey]); } + // Disable filtering by block/problem type when viewing the Collections tab. + const overrideTypesFilter = insideCollections ? new TypesFilterData() : undefined; + return (
@@ -237,6 +243,7 @@ const LibraryAuthoringPage = ({ returnToLibrarySelection }: LibraryAuthoringPage } @@ -258,7 +265,7 @@ const LibraryAuthoringPage = ({ returnToLibrarySelection }: LibraryAuthoringPage - + {!insideCollections && } diff --git a/src/library-authoring/LibraryContent.tsx b/src/library-authoring/LibraryContent.tsx index 5eb0505201..1913994b28 100644 --- a/src/library-authoring/LibraryContent.tsx +++ b/src/library-authoring/LibraryContent.tsx @@ -6,15 +6,10 @@ import { useLibraryContext } from './common/context/LibraryContext'; import { useSidebarContext } from './common/context/SidebarContext'; import CollectionCard from './components/CollectionCard'; import ComponentCard from './components/ComponentCard'; +import { ContentType } from './routes'; import { useLoadOnScroll } from '../hooks'; import messages from './collections/messages'; -export enum ContentType { - home = '', - components = 'components', - collections = 'collections', -} - /** * Library Content to show content grid * diff --git a/src/library-authoring/LibraryLayout.tsx b/src/library-authoring/LibraryLayout.tsx index 610a28eacb..c093af7ad3 100644 --- a/src/library-authoring/LibraryLayout.tsx +++ b/src/library-authoring/LibraryLayout.tsx @@ -1,10 +1,12 @@ +import { useCallback } from 'react'; import { Route, Routes, useParams, - useMatch, + useLocation, } from 'react-router-dom'; +import { ROUTES } from './routes'; import LibraryAuthoringPage from './LibraryAuthoringPage'; import { LibraryProvider } from './common/context/LibraryContext'; import { SidebarProvider } from './common/context/SidebarContext'; @@ -16,22 +18,18 @@ import { ComponentEditorModal } from './components/ComponentEditorModal'; const LibraryLayout = () => { const { libraryId } = useParams(); - const match = useMatch('/library/:libraryId/collection/:collectionId'); - - const collectionId = match?.params.collectionId; - if (libraryId === undefined) { // istanbul ignore next - This shouldn't be possible; it's just here to satisfy the type checker. throw new Error('Error: route is missing libraryId.'); } - return ( + const location = useLocation(); + const context = useCallback((childPage) => ( LibraryAuthoringPage/LibraryCollectionPage > @@ -39,20 +37,38 @@ const LibraryLayout = () => { componentPicker={ComponentPicker} > - - } - /> - } - /> - - - + <> + {childPage} + + + + ), [location.pathname]); + + return ( + + )} + /> + )} + /> + )} + /> + )} + /> + )} + /> + ); }; diff --git a/src/library-authoring/add-content/AddContentContainer.test.tsx b/src/library-authoring/add-content/AddContentContainer.test.tsx index 2f233629cd..229948c39e 100644 --- a/src/library-authoring/add-content/AddContentContainer.test.tsx +++ b/src/library-authoring/add-content/AddContentContainer.test.tsx @@ -25,17 +25,13 @@ jest.mock('frontend-components-tinymce-advanced-plugins', () => ({ a11ycheckerCs const { libraryId } = mockContentLibrary; const render = (collectionId?: string) => { - const params: { libraryId: string, collectionId?: string } = { libraryId }; - if (collectionId) { - params.collectionId = collectionId; - } + const params: { libraryId: string, collectionId?: string } = { libraryId, collectionId }; return baseRender(, { - path: '/library/:libraryId/*', + path: '/library/:libraryId/:collectionId?', params, extraWrapper: ({ children }) => ( { children } diff --git a/src/library-authoring/add-content/PickLibraryContentModal.test.tsx b/src/library-authoring/add-content/PickLibraryContentModal.test.tsx index f5d3606c5d..80efc8cb3c 100644 --- a/src/library-authoring/add-content/PickLibraryContentModal.test.tsx +++ b/src/library-authoring/add-content/PickLibraryContentModal.test.tsx @@ -34,7 +34,6 @@ const render = () => baseRender( ( {children} diff --git a/src/library-authoring/collections/CollectionInfo.tsx b/src/library-authoring/collections/CollectionInfo.tsx index 4c370f26e2..277a6fc1c6 100644 --- a/src/library-authoring/collections/CollectionInfo.tsx +++ b/src/library-authoring/collections/CollectionInfo.tsx @@ -6,7 +6,6 @@ import { Tabs, } from '@openedx/paragon'; import { useCallback } from 'react'; -import { useNavigate, useMatch } from 'react-router-dom'; import { useComponentPickerContext } from '../common/context/ComponentPickerContext'; import { useLibraryContext } from '../common/context/LibraryContext'; @@ -17,6 +16,7 @@ import { isCollectionInfoTab, useSidebarContext, } from '../common/context/SidebarContext'; +import { useLibraryRoutes } from '../routes'; import { ContentTagsDrawer } from '../../content-tags-drawer'; import { buildCollectionUsageKey } from '../../generic/key-utils'; import CollectionDetails from './CollectionDetails'; @@ -24,36 +24,33 @@ import messages from './messages'; const CollectionInfo = () => { const intl = useIntl(); - const navigate = useNavigate(); const { componentPickerMode } = useComponentPickerContext(); - const { libraryId, collectionId, setCollectionId } = useLibraryContext(); - const { sidebarComponentInfo, setSidebarCurrentTab } = useSidebarContext(); + const { libraryId, setCollectionId } = useLibraryContext(); + const { sidebarComponentInfo, sidebarTab, setSidebarTab } = useSidebarContext(); const tab: CollectionInfoTab = ( - sidebarComponentInfo?.currentTab && isCollectionInfoTab(sidebarComponentInfo.currentTab) - ) ? sidebarComponentInfo?.currentTab : COLLECTION_INFO_TABS.Manage; + sidebarTab && isCollectionInfoTab(sidebarTab) + ) ? sidebarTab : COLLECTION_INFO_TABS.Manage; - const sidebarCollectionId = sidebarComponentInfo?.id; + const collectionId = sidebarComponentInfo?.id; // istanbul ignore if: this should never happen - if (!sidebarCollectionId) { - throw new Error('sidebarCollectionId is required'); + if (!collectionId) { + throw new Error('collectionId is required'); } - const url = `/library/${libraryId}/collection/${sidebarCollectionId}`; - const urlMatch = useMatch(url); + const collectionUsageKey = buildCollectionUsageKey(libraryId, collectionId); - const showOpenCollectionButton = !urlMatch && collectionId !== sidebarCollectionId; - - const collectionUsageKey = buildCollectionUsageKey(libraryId, sidebarCollectionId); + const { insideCollection, navigateTo } = useLibraryRoutes(); + const showOpenCollectionButton = !insideCollection || componentPickerMode; const handleOpenCollection = useCallback(() => { - if (!componentPickerMode) { - navigate(url); + if (componentPickerMode) { + setCollectionId(collectionId); } else { - setCollectionId(sidebarCollectionId); + navigateTo({ collectionId }); } - }, [componentPickerMode, url]); + }, [componentPickerMode, navigateTo]); return ( @@ -73,7 +70,7 @@ const CollectionInfo = () => { className="my-3 d-flex justify-content-around" defaultActiveKey={COMPONENT_INFO_TABS.Manage} activeKey={tab} - onSelect={setSidebarCurrentTab} + onSelect={setSidebarTab} > { const { totalHits: componentCount, isFiltered } = useSearchContext(); diff --git a/src/library-authoring/collections/LibraryCollectionPage.tsx b/src/library-authoring/collections/LibraryCollectionPage.tsx index 1d9eca8212..1305183b68 100644 --- a/src/library-authoring/collections/LibraryCollectionPage.tsx +++ b/src/library-authoring/collections/LibraryCollectionPage.tsx @@ -13,6 +13,7 @@ import classNames from 'classnames'; import { Helmet } from 'react-helmet'; import { Link } from 'react-router-dom'; +import { useLibraryRoutes } from '../routes'; import Loading from '../../generic/Loading'; import ErrorAlert from '../../generic/alert-error'; import SubHeader from '../../generic/sub-header/SubHeader'; @@ -46,6 +47,7 @@ const HeaderActions = () => { openCollectionInfoSidebar, sidebarComponentInfo, } = useSidebarContext(); + const { navigateTo } = useLibraryRoutes(); // istanbul ignore if: this should never happen if (!collectionId) { @@ -61,6 +63,10 @@ const HeaderActions = () => { } else { openCollectionInfoSidebar(collectionId); } + + if (!componentPickerMode) { + navigateTo({ collectionId }); + } }; return ( @@ -102,8 +108,8 @@ const LibraryCollectionPage = () => { } const { componentPickerMode } = useComponentPickerContext(); - const { showOnlyPublished, setCollectionId } = useLibraryContext(); - const { sidebarComponentInfo, openCollectionInfoSidebar } = useSidebarContext(); + const { showOnlyPublished, setCollectionId, componentId } = useLibraryContext(); + const { sidebarComponentInfo, openInfoSidebar } = useSidebarContext(); const { data: collectionData, @@ -113,8 +119,8 @@ const LibraryCollectionPage = () => { } = useCollection(libraryId, collectionId); useEffect(() => { - openCollectionInfoSidebar(collectionId); - }, [collectionData]); + openInfoSidebar(componentId, collectionId); + }, []); const { data: libraryData, isLoading: isLibLoading } = useContentLibrary(libraryId); diff --git a/src/library-authoring/common/context/LibraryContext.tsx b/src/library-authoring/common/context/LibraryContext.tsx index 9612a92855..d1abb40783 100644 --- a/src/library-authoring/common/context/LibraryContext.tsx +++ b/src/library-authoring/common/context/LibraryContext.tsx @@ -6,6 +6,7 @@ import { useMemo, useState, } from 'react'; +import { useParams } from 'react-router-dom'; import type { ComponentPicker } from '../../component-picker'; import type { ContentLibrary } from '../../data/api'; @@ -25,6 +26,8 @@ export type LibraryContextData = { isLoadingLibraryData: boolean; collectionId: string | undefined; setCollectionId: (collectionId?: string) => void; + componentId: string | undefined; + setComponentId: (componentId?: string) => void; // Only show published components showOnlyPublished: boolean; // "Create New Collection" modal @@ -53,9 +56,10 @@ const LibraryContext = createContext(undefined); type LibraryProviderProps = { children?: React.ReactNode; libraryId: string; - /** The initial collection ID to show */ - collectionId?: string; showOnlyPublished?: boolean; + // If set, will initialize the current collection and/or component from the current URL + skipUrlUpdate?: boolean; + /** The component picker modal to use. We need to pass it as a reference instead of * directly importing it to avoid the import cycle: * ComponentPicker > LibraryAuthoringPage/LibraryCollectionPage > @@ -69,11 +73,10 @@ type LibraryProviderProps = { export const LibraryProvider = ({ children, libraryId, - collectionId: collectionIdProp, showOnlyPublished = false, + skipUrlUpdate = false, componentPicker, }: LibraryProviderProps) => { - const [collectionId, setCollectionId] = useState(collectionIdProp); const [isCreateCollectionModalOpen, openCreateCollectionModal, closeCreateCollectionModal] = useToggle(false); const [componentBeingEdited, setComponentBeingEdited] = useState(); const closeComponentEditor = useCallback(() => { @@ -94,12 +97,23 @@ export const LibraryProvider = ({ const readOnly = !!componentPickerMode || !libraryData?.canEditLibrary; + // Parse the initial collectionId and/or componentId from the current URL params + const params = useParams(); + const [componentId, setComponentId] = useState( + skipUrlUpdate ? undefined : params.componentId, + ); + const [collectionId, setCollectionId] = useState( + skipUrlUpdate ? undefined : params.collectionId, + ); + const context = useMemo(() => { const contextValue = { libraryId, libraryData, collectionId, setCollectionId, + componentId, + setComponentId, readOnly, isLoadingLibraryData, showOnlyPublished, @@ -115,9 +129,11 @@ export const LibraryProvider = ({ return contextValue; }, [ libraryId, + libraryData, collectionId, setCollectionId, - libraryData, + componentId, + setComponentId, readOnly, isLoadingLibraryData, showOnlyPublished, diff --git a/src/library-authoring/common/context/SidebarContext.tsx b/src/library-authoring/common/context/SidebarContext.tsx index d9ba68d69b..7dd7e9c7db 100644 --- a/src/library-authoring/common/context/SidebarContext.tsx +++ b/src/library-authoring/common/context/SidebarContext.tsx @@ -5,6 +5,7 @@ import { useMemo, useState, } from 'react'; +import { useStateWithUrlSearchParam } from '../../../hooks'; export enum SidebarBodyComponentId { AddContent = 'add-content', @@ -32,28 +33,36 @@ export const isComponentInfoTab = (tab: string): tab is ComponentInfoTab => ( Object.values(COMPONENT_INFO_TABS).includes(tab) ); +type SidebarInfoTab = ComponentInfoTab | CollectionInfoTab; +const toSidebarInfoTab = (tab: string): SidebarInfoTab | undefined => ( + isComponentInfoTab(tab) || isCollectionInfoTab(tab) + ? tab : undefined +); + export interface SidebarComponentInfo { type: SidebarBodyComponentId; id: string; - /** Additional action on Sidebar display */ - additionalAction?: SidebarAdditionalActions; - /** Current tab in the sidebar */ - currentTab?: CollectionInfoTab | ComponentInfoTab; } -export enum SidebarAdditionalActions { +export enum SidebarActions { JumpToAddCollections = 'jump-to-add-collections', + ManageTeam = 'manage-team', + None = '', } export type SidebarContextData = { closeLibrarySidebar: () => void; openAddContentSidebar: () => void; - openInfoSidebar: () => void; - openCollectionInfoSidebar: (collectionId: string, additionalAction?: SidebarAdditionalActions) => void; - openComponentInfoSidebar: (usageKey: string, additionalAction?: SidebarAdditionalActions) => void; + openInfoSidebar: (componentId?: string, collectionId?: string) => void; + openLibrarySidebar: () => void; + openCollectionInfoSidebar: (collectionId: string) => void; + openComponentInfoSidebar: (usageKey: string) => void; sidebarComponentInfo?: SidebarComponentInfo; - resetSidebarAdditionalActions: () => void; - setSidebarCurrentTab: (tab: CollectionInfoTab | ComponentInfoTab) => void; + sidebarAction: SidebarActions; + setSidebarAction: (action: SidebarActions) => void; + resetSidebarAction: () => void; + sidebarTab: SidebarInfoTab; + setSidebarTab: (tab: SidebarInfoTab) => void; }; /** @@ -71,7 +80,7 @@ type SidebarProviderProps = { }; /** - * React component to provide `LibraryContext` + * React component to provide `SidebarContext` */ export const SidebarProvider = ({ children, @@ -81,12 +90,22 @@ export const SidebarProvider = ({ initialSidebarComponentInfo, ); - /** Helper function to consume addtional action once performed. - Required to redo the action. - */ - const resetSidebarAdditionalActions = useCallback(() => { - setSidebarComponentInfo((prev) => (prev && { ...prev, additionalAction: undefined })); - }, []); + const [sidebarTab, setSidebarTab] = useStateWithUrlSearchParam( + COMPONENT_INFO_TABS.Preview, + 'st', + (value: string) => toSidebarInfoTab(value), + (value: SidebarInfoTab) => value.toString(), + ); + + const [sidebarAction, setSidebarAction] = useStateWithUrlSearchParam( + SidebarActions.None, + 'sa', + (value: string) => Object.values(SidebarActions).find((enumValue) => value === enumValue), + (value: SidebarActions) => value.toString(), + ); + const resetSidebarAction = useCallback(() => { + setSidebarAction(SidebarActions.None); + }, [setSidebarAction]); const closeLibrarySidebar = useCallback(() => { setSidebarComponentInfo(undefined); @@ -94,33 +113,32 @@ export const SidebarProvider = ({ const openAddContentSidebar = useCallback(() => { setSidebarComponentInfo({ id: '', type: SidebarBodyComponentId.AddContent }); }, []); - const openInfoSidebar = useCallback(() => { + const openLibrarySidebar = useCallback(() => { setSidebarComponentInfo({ id: '', type: SidebarBodyComponentId.Info }); }, []); - const openComponentInfoSidebar = useCallback((usageKey: string, additionalAction?: SidebarAdditionalActions) => { - setSidebarComponentInfo((prev) => ({ - ...prev, + const openComponentInfoSidebar = useCallback((usageKey: string) => { + setSidebarComponentInfo({ id: usageKey, type: SidebarBodyComponentId.ComponentInfo, - additionalAction, - })); + }); }, []); - const openCollectionInfoSidebar = useCallback(( - newCollectionId: string, - additionalAction?: SidebarAdditionalActions, - ) => { - setSidebarComponentInfo((prev) => ({ - ...prev, + const openCollectionInfoSidebar = useCallback((newCollectionId: string) => { + setSidebarComponentInfo({ id: newCollectionId, type: SidebarBodyComponentId.CollectionInfo, - additionalAction, - })); + }); }, []); - const setSidebarCurrentTab = useCallback((tab: CollectionInfoTab | ComponentInfoTab) => { - setSidebarComponentInfo((prev) => (prev && { ...prev, currentTab: tab })); + const openInfoSidebar = useCallback((componentId?: string, collectionId?: string) => { + if (componentId) { + openComponentInfoSidebar(componentId); + } else if (collectionId) { + openCollectionInfoSidebar(collectionId); + } else { + openLibrarySidebar(); + } }, []); const context = useMemo(() => { @@ -128,11 +146,15 @@ export const SidebarProvider = ({ closeLibrarySidebar, openAddContentSidebar, openInfoSidebar, + openLibrarySidebar, openComponentInfoSidebar, sidebarComponentInfo, openCollectionInfoSidebar, - resetSidebarAdditionalActions, - setSidebarCurrentTab, + sidebarAction, + setSidebarAction, + resetSidebarAction, + sidebarTab, + setSidebarTab, }; return contextValue; @@ -140,11 +162,15 @@ export const SidebarProvider = ({ closeLibrarySidebar, openAddContentSidebar, openInfoSidebar, + openLibrarySidebar, openComponentInfoSidebar, sidebarComponentInfo, openCollectionInfoSidebar, - resetSidebarAdditionalActions, - setSidebarCurrentTab, + sidebarAction, + setSidebarAction, + resetSidebarAction, + sidebarTab, + setSidebarTab, ]); return ( @@ -162,10 +188,14 @@ export function useSidebarContext(): SidebarContextData { closeLibrarySidebar: () => {}, openAddContentSidebar: () => {}, openInfoSidebar: () => {}, + openLibrarySidebar: () => {}, openComponentInfoSidebar: () => {}, openCollectionInfoSidebar: () => {}, - resetSidebarAdditionalActions: () => {}, - setSidebarCurrentTab: () => {}, + sidebarAction: SidebarActions.None, + setSidebarAction: () => {}, + resetSidebarAction: () => {}, + sidebarTab: COMPONENT_INFO_TABS.Preview, + setSidebarTab: () => {}, sidebarComponentInfo: undefined, }; } diff --git a/src/library-authoring/component-info/ComponentInfo.tsx b/src/library-authoring/component-info/ComponentInfo.tsx index c9db685e92..085e6d120f 100644 --- a/src/library-authoring/component-info/ComponentInfo.tsx +++ b/src/library-authoring/component-info/ComponentInfo.tsx @@ -16,7 +16,7 @@ import { useLibraryContext } from '../common/context/LibraryContext'; import { type ComponentInfoTab, COMPONENT_INFO_TABS, - SidebarAdditionalActions, + SidebarActions, isComponentInfoTab, useSidebarContext, } from '../common/context/SidebarContext'; @@ -101,27 +101,27 @@ const ComponentInfo = () => { const intl = useIntl(); const { readOnly, openComponentEditor } = useLibraryContext(); - const { setSidebarCurrentTab, sidebarComponentInfo, resetSidebarAdditionalActions } = useSidebarContext(); + const { + sidebarTab, + setSidebarTab, + sidebarComponentInfo, + sidebarAction, + } = useSidebarContext(); - const jumpToCollections = sidebarComponentInfo?.additionalAction === SidebarAdditionalActions.JumpToAddCollections; + const jumpToCollections = sidebarAction === SidebarActions.JumpToAddCollections; const tab: ComponentInfoTab = ( - sidebarComponentInfo?.currentTab && isComponentInfoTab(sidebarComponentInfo.currentTab) - ) ? sidebarComponentInfo?.currentTab : COMPONENT_INFO_TABS.Preview; + isComponentInfoTab(sidebarTab) + ? sidebarTab + : COMPONENT_INFO_TABS.Preview + ); useEffect(() => { // Show Manage tab if JumpToAddCollections action is set in sidebarComponentInfo if (jumpToCollections) { - setSidebarCurrentTab(COMPONENT_INFO_TABS.Manage); - } - }, [jumpToCollections]); - - useEffect(() => { - // This is required to redo actions. - if (tab !== COMPONENT_INFO_TABS.Manage) { - resetSidebarAdditionalActions(); + setSidebarTab(COMPONENT_INFO_TABS.Manage); } - }, [tab]); + }, [jumpToCollections, setSidebarTab]); const usageKey = sidebarComponentInfo?.id; // istanbul ignore if: this should never happen @@ -169,7 +169,7 @@ const ComponentInfo = () => { className="my-3 d-flex justify-content-around" defaultActiveKey={COMPONENT_INFO_TABS.Preview} activeKey={tab} - onSelect={setSidebarCurrentTab} + onSelect={setSidebarTab} > diff --git a/src/library-authoring/component-info/ComponentManagement.tsx b/src/library-authoring/component-info/ComponentManagement.tsx index bbdfc51efb..67a224dd00 100644 --- a/src/library-authoring/component-info/ComponentManagement.tsx +++ b/src/library-authoring/component-info/ComponentManagement.tsx @@ -7,7 +7,7 @@ import { } from '@openedx/paragon/icons'; import { useLibraryContext } from '../common/context/LibraryContext'; -import { SidebarAdditionalActions, useSidebarContext } from '../common/context/SidebarContext'; +import { SidebarActions, useSidebarContext } from '../common/context/SidebarContext'; import { useLibraryBlockMetadata } from '../data/apiHooks'; import StatusWidget from '../generic/status-widget'; import messages from './messages'; @@ -18,8 +18,8 @@ import ManageCollections from './ManageCollections'; const ComponentManagement = () => { const intl = useIntl(); const { readOnly, isLoadingLibraryData } = useLibraryContext(); - const { sidebarComponentInfo, resetSidebarAdditionalActions } = useSidebarContext(); - const jumpToCollections = sidebarComponentInfo?.additionalAction === SidebarAdditionalActions.JumpToAddCollections; + const { sidebarComponentInfo, sidebarAction, resetSidebarAction } = useSidebarContext(); + const jumpToCollections = sidebarAction === SidebarActions.JumpToAddCollections; const [tagsCollapseIsOpen, setTagsCollapseOpen] = React.useState(!jumpToCollections); const [collectionsCollapseIsOpen, setCollectionsCollapseOpen] = React.useState(true); @@ -33,7 +33,7 @@ const ComponentManagement = () => { useEffect(() => { // This is required to redo actions. if (tagsCollapseIsOpen || !collectionsCollapseIsOpen) { - resetSidebarAdditionalActions(); + resetSidebarAction(); } }, [tagsCollapseIsOpen, collectionsCollapseIsOpen]); diff --git a/src/library-authoring/component-info/ManageCollections.test.tsx b/src/library-authoring/component-info/ManageCollections.test.tsx index 9c3d696546..790fb0a993 100644 --- a/src/library-authoring/component-info/ManageCollections.test.tsx +++ b/src/library-authoring/component-info/ManageCollections.test.tsx @@ -13,6 +13,7 @@ import { mockContentSearchConfig } from '../../search-manager/data/api.mock'; import { mockContentLibrary, mockLibraryBlockMetadata } from '../data/api.mocks'; import ManageCollections from './ManageCollections'; import { LibraryProvider } from '../common/context/LibraryContext'; +import { SidebarProvider } from '../common/context/SidebarContext'; import { getLibraryBlockCollectionsUrl } from '../data/api'; let axiosMock: MockAdapter; @@ -25,7 +26,9 @@ mockContentSearchConfig.applyMock(); const render = (ui: React.ReactElement) => baseRender(ui, { extraWrapper: ({ children }) => ( - {children} + + {children} + ), }); diff --git a/src/library-authoring/component-info/ManageCollections.tsx b/src/library-authoring/component-info/ManageCollections.tsx index b7cbc98323..1ab32c8b93 100644 --- a/src/library-authoring/component-info/ManageCollections.tsx +++ b/src/library-authoring/component-info/ManageCollections.tsx @@ -1,4 +1,4 @@ -import { useContext, useEffect, useState } from 'react'; +import { useContext, useState } from 'react'; import { FormattedMessage, useIntl } from '@edx/frontend-platform/i18n'; import { Button, Icon, Scrollable, SelectableBox, Stack, StatefulButton, useCheckboxSetValues, @@ -16,7 +16,7 @@ import { useUpdateComponentCollections } from '../data/apiHooks'; import { ToastContext } from '../../generic/toast-context'; import { CollectionMetadata } from '../data/api'; import { useLibraryContext } from '../common/context/LibraryContext'; -import { SidebarAdditionalActions, useSidebarContext } from '../common/context/SidebarContext'; +import { SidebarActions, useSidebarContext } from '../common/context/SidebarContext'; interface ManageCollectionsProps { usageKey: string; @@ -191,38 +191,23 @@ const ComponentCollections = ({ collections, onManageClick }: { }; const ManageCollections = ({ usageKey, collections }: ManageCollectionsProps) => { - const { sidebarComponentInfo, resetSidebarAdditionalActions } = useSidebarContext(); - const jumpToCollections = sidebarComponentInfo?.additionalAction === SidebarAdditionalActions.JumpToAddCollections; - const [editing, setEditing] = useState(jumpToCollections); + const { sidebarAction, resetSidebarAction, setSidebarAction } = useSidebarContext(); const collectionNames = collections.map((collection) => collection.title); - useEffect(() => { - if (jumpToCollections) { - setEditing(true); - } - }, [sidebarComponentInfo]); - - useEffect(() => { - // This is required to redo actions. - if (!editing) { - resetSidebarAdditionalActions(); - } - }, [editing]); - - if (editing) { - return ( - setEditing(false)} - /> - ); - } return ( - setEditing(true)} - /> + sidebarAction === SidebarActions.JumpToAddCollections + ? ( + resetSidebarAction()} + /> + ) : ( + setSidebarAction(SidebarActions.JumpToAddCollections)} + /> + ) ); }; diff --git a/src/library-authoring/component-picker/ComponentPicker.tsx b/src/library-authoring/component-picker/ComponentPicker.tsx index 115c081fec..75bf178ac3 100644 --- a/src/library-authoring/component-picker/ComponentPicker.tsx +++ b/src/library-authoring/component-picker/ComponentPicker.tsx @@ -105,6 +105,7 @@ export const ComponentPicker: React.FC = ({ { calcShowOnlyPublished diff --git a/src/library-authoring/components/BaseComponentCard.tsx b/src/library-authoring/components/BaseComponentCard.tsx index 3b5aa748c9..00ccc00165 100644 --- a/src/library-authoring/components/BaseComponentCard.tsx +++ b/src/library-authoring/components/BaseComponentCard.tsx @@ -16,7 +16,7 @@ type BaseComponentCardProps = { numChildren?: number, tags: ContentHitTags, actions: React.ReactNode, - openInfoSidebar: () => void + onSelect: () => void }; const BaseComponentCard = ({ @@ -26,7 +26,7 @@ const BaseComponentCard = ({ numChildren, tags, actions, - openInfoSidebar, + onSelect, } : BaseComponentCardProps) => { const tagCount = useMemo(() => { if (!tags) { @@ -42,10 +42,10 @@ const BaseComponentCard = ({ { if (['Enter', ' '].includes(e.key)) { - openInfoSidebar(); + onSelect(); } }} > diff --git a/src/library-authoring/components/CollectionCard.tsx b/src/library-authoring/components/CollectionCard.tsx index 6935bc12c8..ee165e04a0 100644 --- a/src/library-authoring/components/CollectionCard.tsx +++ b/src/library-authoring/components/CollectionCard.tsx @@ -14,6 +14,7 @@ import { type CollectionHit } from '../../search-manager'; import { useComponentPickerContext } from '../common/context/ComponentPickerContext'; import { useLibraryContext } from '../common/context/LibraryContext'; import { useSidebarContext } from '../common/context/SidebarContext'; +import { useLibraryRoutes } from '../routes'; import BaseComponentCard from './BaseComponentCard'; import { ToastContext } from '../../generic/toast-context'; import { useDeleteCollection, useRestoreCollection } from '../data/apiHooks'; @@ -112,6 +113,7 @@ const CollectionCard = ({ collectionHit } : CollectionCardProps) => { const { type: componentType, + blockId: collectionId, formatted, tags, numChildren, @@ -124,6 +126,15 @@ const CollectionCard = ({ collectionHit } : CollectionCardProps) => { const { displayName = '', description = '' } = formatted; + const { navigateTo } = useLibraryRoutes(); + const openCollection = useCallback(() => { + openCollectionInfoSidebar(collectionId); + + if (!componentPickerMode) { + navigateTo({ collectionId }); + } + }, [collectionId, navigateTo, openCollectionInfoSidebar]); + return ( { )} - openInfoSidebar={() => openCollectionInfoSidebar(collectionHit.blockId)} + onSelect={openCollection} /> ); }; diff --git a/src/library-authoring/components/ComponentCard.tsx b/src/library-authoring/components/ComponentCard.tsx index 813255b97a..196a060f97 100644 --- a/src/library-authoring/components/ComponentCard.tsx +++ b/src/library-authoring/components/ComponentCard.tsx @@ -1,4 +1,4 @@ -import { useContext, useState } from 'react'; +import { useCallback, useContext, useState } from 'react'; import { FormattedMessage, useIntl } from '@edx/frontend-platform/i18n'; import { ActionRow, @@ -21,8 +21,10 @@ import { ToastContext } from '../../generic/toast-context'; import { type ContentHit } from '../../search-manager'; import { useComponentPickerContext } from '../common/context/ComponentPickerContext'; import { useLibraryContext } from '../common/context/LibraryContext'; -import { SidebarAdditionalActions, useSidebarContext } from '../common/context/SidebarContext'; +import { SidebarActions, useSidebarContext } from '../common/context/SidebarContext'; import { useRemoveComponentsFromCollection } from '../data/apiHooks'; +import { useLibraryRoutes } from '../routes'; + import BaseComponentCard from './BaseComponentCard'; import { canEditComponent } from './ComponentEditorModal'; import messages from './messages'; @@ -44,6 +46,7 @@ export const ComponentMenu = ({ usageKey }: { usageKey: string }) => { sidebarComponentInfo, openComponentInfoSidebar, closeLibrarySidebar, + setSidebarAction, } = useSidebarContext(); const canEdit = usageKey && canEditComponent(usageKey); @@ -73,9 +76,10 @@ export const ComponentMenu = ({ usageKey }: { usageKey: string }) => { }); }; - const showManageCollections = () => { - openComponentInfoSidebar(usageKey, SidebarAdditionalActions.JumpToAddCollections); - }; + const showManageCollections = useCallback(() => { + setSidebarAction(SidebarActions.JumpToAddCollections); + openComponentInfoSidebar(usageKey); + }, [setSidebarAction, openComponentInfoSidebar, usageKey]); return ( @@ -200,6 +204,15 @@ const ComponentCard = ({ contentHit }: ComponentCardProps) => { showOnlyPublished ? formatted.published?.displayName : formatted.displayName ) ?? ''; + const { navigateTo } = useLibraryRoutes(); + const openComponent = useCallback(() => { + openComponentInfoSidebar(usageKey); + + if (!componentPickerMode) { + navigateTo({ componentId: usageKey }); + } + }, [usageKey, navigateTo, openComponentInfoSidebar]); + return ( { )} )} - openInfoSidebar={() => openComponentInfoSidebar(usageKey)} + onSelect={openComponent} /> ); }; diff --git a/src/library-authoring/library-info/LibraryInfo.tsx b/src/library-authoring/library-info/LibraryInfo.tsx index 755e2ec765..5269b798af 100644 --- a/src/library-authoring/library-info/LibraryInfo.tsx +++ b/src/library-authoring/library-info/LibraryInfo.tsx @@ -1,15 +1,24 @@ -import { Button, Stack, useToggle } from '@openedx/paragon'; +import { useCallback } from 'react'; +import { Button, Stack } from '@openedx/paragon'; import { FormattedDate, useIntl } from '@edx/frontend-platform/i18n'; import messages from './messages'; import LibraryPublishStatus from './LibraryPublishStatus'; import { LibraryTeamModal } from '../library-team'; import { useLibraryContext } from '../common/context/LibraryContext'; +import { SidebarActions, useSidebarContext } from '../common/context/SidebarContext'; const LibraryInfo = () => { const intl = useIntl(); const { libraryData, readOnly } = useLibraryContext(); - const [isLibraryTeamModalOpen, openLibraryTeamModal, closeLibraryTeamModal] = useToggle(); + const { sidebarAction, setSidebarAction, resetSidebarAction } = useSidebarContext(); + const isLibraryTeamModalOpen = (sidebarAction === SidebarActions.ManageTeam); + const openLibraryTeamModal = useCallback(() => { + setSidebarAction(SidebarActions.ManageTeam); + }, [setSidebarAction]); + const closeLibraryTeamModal = useCallback(() => { + resetSidebarAction(); + }, [resetSidebarAction]); return ( diff --git a/src/library-authoring/library-info/LibraryInfoHeader.tsx b/src/library-authoring/library-info/LibraryInfoHeader.tsx index 95d2f5fd20..cafeccf268 100644 --- a/src/library-authoring/library-info/LibraryInfoHeader.tsx +++ b/src/library-authoring/library-info/LibraryInfoHeader.tsx @@ -43,7 +43,7 @@ const LibraryInfoHeader = () => { setIsActive(true); }; - const hanldeOnKeyDown = (event) => { + const handleOnKeyDown = (event) => { if (event.key === 'Enter') { handleSaveTitle(event); } else if (event.key === 'Escape') { @@ -63,7 +63,7 @@ const LibraryInfoHeader = () => { aria-label="Title input" defaultValue={library.title} onBlur={handleSaveTitle} - onKeyDown={hanldeOnKeyDown} + onKeyDown={handleOnKeyDown} /> ) : ( diff --git a/src/library-authoring/routes.test.tsx b/src/library-authoring/routes.test.tsx new file mode 100644 index 0000000000..488b7889de --- /dev/null +++ b/src/library-authoring/routes.test.tsx @@ -0,0 +1,289 @@ +import { useEffect } from 'react'; +import fetchMock from 'fetch-mock-jest'; +import { + mockContentLibrary, +} from './data/api.mocks'; +import { LibraryLayout } from '.'; +import { ContentType, useLibraryRoutes } from './routes'; +import mockResult from './__mocks__/library-search.json'; +import { initializeMocks, render } from '../testUtils'; +import { studioHomeMock } from '../studio-home/__mocks__'; +import { getStudioHomeApiUrl } from '../studio-home/data/api'; + +const mockNavigate = jest.fn(); +jest.mock('react-router-dom', () => ({ + ...jest.requireActual('react-router-dom'), + useNavigate: () => mockNavigate, +})); + +mockContentLibrary.applyMock(); + +const searchEndpoint = 'http://mock.meilisearch.local/multi-search'; +describe('Library Authoring routes', () => { + beforeEach(async () => { + const { axiosMock } = initializeMocks(); + axiosMock.onGet(getStudioHomeApiUrl()).reply(200, studioHomeMock); + + // The Meilisearch client-side API uses fetch, not Axios. + fetchMock.mockReset(); + fetchMock.post(searchEndpoint, (_url, req) => { + const requestData = JSON.parse(req.body?.toString() ?? ''); + const query = requestData?.queries[0]?.q ?? ''; + // We have to replace the query (search keywords) in the mock results with the actual query, + // because otherwise Instantsearch will update the UI and change the query, + // leading to unexpected results in the test cases. + const newMockResult = { ...mockResult }; + newMockResult.results[0].query = query; + // And fake the required '_formatted' fields; it contains the highlighting ... around matched words + // eslint-disable-next-line no-underscore-dangle, no-param-reassign + newMockResult.results[0]?.hits.forEach((hit) => { hit._formatted = { ...hit }; }); + return newMockResult; + }); + }); + + test.each([ + // "All Content" tab + { + label: 'navigate from All Content tab to Components tab', + origin: { + path: '', + params: {}, + }, + destination: { + path: '/components', + params: { + contentType: ContentType.components, + }, + }, + }, + { + label: 'navigate from All Content tab to Collections tab', + origin: { + path: '', + params: {}, + }, + destination: { + params: { + contentType: ContentType.collections, + }, + path: '/collections', + }, + }, + { + label: 'from All Content tab, select a Component', + origin: { + path: '', + params: {}, + }, + destination: { + params: { + componentId: 'cmptId', + }, + path: '/component/cmptId', + }, + }, + { + label: 'from All Content tab > selected Component, select a different Component', + origin: { + path: '', + params: {}, + }, + destination: { + params: { + componentId: 'cmptId2', + }, + path: '/component/cmptId2', + }, + }, + { + label: 'from All Content tab, select a Collection', + origin: { + path: '', + params: {}, + }, + destination: { + params: { + collectionId: 'clctnId', + }, + path: '/clctnId', + }, + }, + { + label: 'navigate from All Content > selected Collection to the Collection page', + origin: { + params: { + collectionId: 'clctnId', + }, + path: '/clctnId', + }, + destination: { + params: { + collectionId: 'clctnId', + }, + /* + * Note: the MemoryRouter used by testUtils breaks this, but should be: + * path: '/collection/clctnId', + */ + path: '/clctnId', + }, + }, + { + label: 'from All Content > Collection, select a different Collection', + origin: { + params: { + collectionId: 'clctnId', + }, + path: '/clctnId', + }, + destination: { + params: { + collectionId: 'clctnId2', + }, + path: '/clctnId2', + }, + }, + // "Components" tab + { + label: 'navigate from Components tab to All Content tab', + origin: { + path: '/components', + params: {}, + }, + destination: { + path: '', + params: { + contentType: ContentType.home, + }, + }, + }, + { + label: 'navigate from Components tab to Collections tab', + origin: { + label: 'Components tab', + path: '/components', + params: {}, + }, + destination: { + label: 'Collections tab', + params: { + contentType: ContentType.collections, + }, + path: '/collections', + }, + }, + { + label: 'from Components tab, select a Component', + origin: { + path: '/components', + params: {}, + }, + destination: { + params: { + componentId: 'cmptId', + }, + path: '/components/cmptId', + }, + }, + // "Collections" tab + { + label: 'navigate from Collections tab to All Content tab', + origin: { + path: '/collections', + params: {}, + }, + destination: { + path: '', + params: { + contentType: ContentType.home, + }, + }, + }, + { + label: 'navigate from Collections tab to Components tab', + origin: { + path: '/collections', + params: {}, + }, + destination: { + path: '/components', + params: { + contentType: ContentType.components, + }, + }, + }, + { + label: 'from Collections tab, select a Collection', + origin: { + path: '/collections', + params: {}, + }, + destination: { + params: { + collectionId: 'clctnId', + }, + path: '/collections/clctnId', + }, + }, + { + label: 'from Collections tab > selected Collection, navigate to the Collection page', + origin: { + params: { + collectionId: 'clctnId', + }, + path: '/collections/clctnId', + }, + destination: { + params: { + collectionId: 'clctnId', + }, + /* + * Note: the MemoryRouter used by testUtils breaks this, but should be: + * path: '/collection/clctnId', + */ + path: '/collections/clctnId', + }, + }, + { + label: 'from Collections > selected Collection, select a different Collection', + origin: { + params: { + collectionId: 'clctnId', + }, + path: '/collections/clctnId', + }, + destination: { + params: { + collectionId: 'clctnId2', + }, + path: '/collections/clctnId2', + }, + }, + ])( + '$label', + async ({ origin, destination }) => { + const LibraryRouterTest = () => { + /* + * Note: we'd also like to test the insideComponent etc. flags returned here, + * but the MemoryRouter used by testUtils makes this impossible. + */ + const { navigateTo } = useLibraryRoutes(); + useEffect(() => navigateTo(destination.params), [destination.params]); + return ; + }; + + render(, { + path: `/library/:libraryId${origin.path}/*`, + params: { + libraryId: mockContentLibrary.libraryId, + collectionId: '', + ...origin.params, + }, + }); + + expect(mockNavigate).toBeCalledWith({ + pathname: `/library/${mockContentLibrary.libraryId}${destination.path}`, + search: '', + }); + }, + ); +}); diff --git a/src/library-authoring/routes.ts b/src/library-authoring/routes.ts new file mode 100644 index 0000000000..fa96c7be01 --- /dev/null +++ b/src/library-authoring/routes.ts @@ -0,0 +1,128 @@ +/** + * Constants and utility hook for the Library Authoring routes. + */ +import { useCallback } from 'react'; +import { + generatePath, + matchPath, + useParams, + useLocation, + useNavigate, + useSearchParams, + type PathMatch, +} from 'react-router-dom'; + +export const BASE_ROUTE = '/library/:libraryId'; + +export const ROUTES = { + // LibraryAuthoringPage routes: + // * Components tab, with an optionally selected componentId in the sidebar. + COMPONENTS: '/components/:componentId?', + // * Collections tab, with an optionally selected collectionId in the sidebar. + COLLECTIONS: '/collections/:collectionId?', + // * All Content tab, with an optionally selected componentId in the sidebar. + COMPONENT: '/component/:componentId', + // * All Content tab, with an optionally selected collectionId in the sidebar. + HOME: '/:collectionId?', + // LibraryCollectionPage route: + // * with a selected collectionId and/or an optionally selected componentId. + COLLECTION: '/collection/:collectionId/:componentId?', +}; + +export enum ContentType { + home = '', + components = 'components', + collections = 'collections', +} + +export type NavigateToData = { + componentId?: string, + collectionId?: string, + contentType?: ContentType, +}; + +export type LibraryRoutesData = { + insideCollection: PathMatch | null; + insideCollections: PathMatch | null; + insideComponents: PathMatch | null; + + // Navigate using the best route from the current location for the given parameters. + navigateTo: (dict?: NavigateToData) => void; +}; + +export const useLibraryRoutes = (): LibraryRoutesData => { + const { pathname } = useLocation(); + const params = useParams(); + const [searchParams] = useSearchParams(); + const navigate = useNavigate(); + + const insideCollection = matchPath(BASE_ROUTE + ROUTES.COLLECTION, pathname); + const insideCollections = matchPath(BASE_ROUTE + ROUTES.COLLECTIONS, pathname); + const insideComponents = matchPath(BASE_ROUTE + ROUTES.COMPONENTS, pathname); + + const navigateTo = useCallback(({ + componentId, + collectionId, + contentType, + }: NavigateToData = {}) => { + const routeParams = { + ...params, + // Overwrite the current componentId/collectionId params if provided + ...((componentId !== undefined) && { componentId }), + ...((collectionId !== undefined) && { collectionId }), + }; + let route; + + // Providing contentType overrides the current route so we can change tabs. + if (contentType === ContentType.components) { + route = ROUTES.COMPONENTS; + } else if (contentType === ContentType.collections) { + route = ROUTES.COLLECTIONS; + } else if (contentType === ContentType.home) { + route = ROUTES.HOME; + } else if (insideCollections) { + // We're inside the Collections tab, + route = ( + (collectionId && collectionId === params.collectionId) + // now open the previously-selected collection, + ? ROUTES.COLLECTION + // or stay there to list all collections, or a selected collection. + : ROUTES.COLLECTIONS + ); + } else if (insideCollection) { + // We're viewing a Collection, so stay there, + // and optionally select a component in that collection. + route = ROUTES.COLLECTION; + } else if (insideComponents) { + // We're inside the Components tab, so stay there, + // optionally selecting a component. + route = ROUTES.COMPONENTS; + } else if (componentId) { + // We're inside the All Content tab, so stay there, + // and select a component. + route = ROUTES.COMPONENT; + } else { + // We're inside the All Content tab, + route = ( + (collectionId && collectionId === params.collectionId) + // now open the previously-selected collection + ? ROUTES.COLLECTION + // or stay there to list all content, or optionally select a collection. + : ROUTES.HOME + ); + } + + const newPath = generatePath(BASE_ROUTE + route, routeParams); + navigate({ + pathname: newPath, + search: searchParams.toString(), + }); + }, [navigate, params, searchParams, pathname]); + + return { + navigateTo, + insideCollection, + insideCollections, + insideComponents, + }; +}; diff --git a/src/search-manager/FilterByBlockType.tsx b/src/search-manager/FilterByBlockType.tsx index 95c8ee756d..0b5160d37d 100644 --- a/src/search-manager/FilterByBlockType.tsx +++ b/src/search-manager/FilterByBlockType.tsx @@ -18,7 +18,7 @@ import { useSearchContext } from './SearchManager'; interface ProblemFilterItemProps { count: number, - handleCheckboxChange: Function, + handleCheckboxChange: (e: any) => void; } interface FilterItemProps { blockType: string, @@ -29,11 +29,9 @@ const ProblemFilterItem = ({ count, handleCheckboxChange } : ProblemFilterItemPr const blockType = 'problem'; const { - setBlockTypesFilter, problemTypes, - problemTypesFilter, - blockTypesFilter, - setProblemTypesFilter, + typesFilter, + setTypesFilter, } = useSearchContext(); const intl = useIntl(); @@ -45,65 +43,41 @@ const ProblemFilterItem = ({ count, handleCheckboxChange } : ProblemFilterItemPr useEffect(() => { /* istanbul ignore next */ - if (problemTypesFilter.length !== 0 - && !blockTypesFilter.includes(blockType)) { + const selectedProblemTypes = typesFilter.problems.size; + if (!selectedProblemTypes || selectedProblemTypes === problemTypesLength) { + setIsProblemIndeterminate(false); + } else if (selectedProblemTypes) { setIsProblemIndeterminate(true); } - }, []); - - const handleCheckBoxChangeOnProblem = React.useCallback((e) => { - handleCheckboxChange(e); - setIsProblemIndeterminate(false); - if (e.target.checked) { - setProblemTypesFilter(Object.keys(problemTypes)); - } else { - setProblemTypesFilter([]); - } - }, [handleCheckboxChange, setProblemTypesFilter]); + }, [typesFilter, problemTypesLength, setIsProblemIndeterminate]); const handleProblemCheckboxChange = React.useCallback((e) => { - setProblemTypesFilter(currentFiltersProblem => { - let result; - if (currentFiltersProblem.includes(e.target.value)) { - result = currentFiltersProblem.filter(x => x !== e.target.value); + setTypesFilter((types) => { + if (e.target.checked) { + types.problems.add(e.target.value); } else { - result = [...currentFiltersProblem, e.target.value]; + types.problems.delete(e.target.value); } - if (e.target.checked) { - /* istanbul ignore next */ - if (result.length === problemTypesLength) { - // Add 'problem' to type filter if all problem types are selected. - setIsProblemIndeterminate(false); - setBlockTypesFilter(currentFilters => [...currentFilters, 'problem']); - } else { - setIsProblemIndeterminate(true); - } - } /* istanbul ignore next */ else { - // Delete 'problem' filter if a problem is deselected. - setBlockTypesFilter(currentFilters => { - /* istanbul ignore next */ - if (currentFilters.includes('problem')) { - return currentFilters.filter(x => x !== 'problem'); - } - return [...currentFilters]; - }); - setIsProblemIndeterminate(result.length !== 0); + + if (types.problems.size === problemTypesLength) { + // Add 'problem' to block type filter if all problem types are selected. + types.blocks.add(blockType); + } else { + // Delete 'problem' filter if its selected. + types.blocks.delete(blockType); } - return result; + + return types; }); - }, [ - setProblemTypesFilter, - problemTypesFilter, - setBlockTypesFilter, - problemTypesLength, - ]); + }, [setTypesFilter, problemTypesLength]); return (
@@ -135,7 +109,7 @@ const ProblemFilterItem = ({ count, handleCheckboxChange } : ProblemFilterItemPr { Object.entries(problemTypes).map(([problemType, problemTypeCount]) => ( @@ -170,17 +144,28 @@ const ProblemFilterItem = ({ count, handleCheckboxChange } : ProblemFilterItemPr const FilterItem = ({ blockType, count } : FilterItemProps) => { const { - setBlockTypesFilter, + problemTypes, + setTypesFilter, } = useSearchContext(); const handleCheckboxChange = React.useCallback((e) => { - setBlockTypesFilter(currentFilters => { - if (currentFilters.includes(e.target.value)) { - return currentFilters.filter(x => x !== e.target.value); + setTypesFilter((types) => { + if (e.target.checked) { + types.blocks.add(e.target.value); + } else { + types.blocks.delete(e.target.value); + } + // The "problem" block type also selects/clears all the problem types + if (blockType === 'problem') { + if (e.target.checked) { + types.union({ problems: Object.keys(problemTypes) }); + } else { + types.problems.clear(); + } } - return [...currentFilters, e.target.value]; + return types; }); - }, [setBlockTypesFilter]); + }, [setTypesFilter]); if (blockType === 'problem') { // Build Capa Problem types filter submenu @@ -206,44 +191,21 @@ const FilterItem = ({ blockType, count } : FilterItemProps) => { ); }; -interface FilterByBlockTypeProps { - disabled?: boolean, -} /** * A button with a dropdown that allows filtering the current search by component type (XBlock type) * e.g. Limit results to "Text" (html) and "Problem" (problem) components. * The button displays the first type selected, and a count of how many other types are selected, if more than one. - * @param disabled - If true, the filter is disabled and hidden. */ -const FilterByBlockType: React.FC = ({ disabled = false }) => { +const FilterByBlockType: React.FC> = () => { const { blockTypes, - blockTypesFilter, - problemTypesFilter, - setBlockTypesFilter, - setProblemTypesFilter, + typesFilter, + setTypesFilter, } = useSearchContext(); const clearFilters = useCallback(/* istanbul ignore next */ () => { - setBlockTypesFilter([]); - setProblemTypesFilter([]); - }, []); - - useEffect(() => { - if (disabled) { - // Clear filters when disabled - const selectedBlockTypesFilter = blockTypesFilter; - const selectedProblemTypesFilter = problemTypesFilter; - clearFilters(); - - return () => { - // Restore filters when re-enabled - setBlockTypesFilter(selectedBlockTypesFilter); - setProblemTypesFilter(selectedProblemTypesFilter); - }; - } - return () => {}; - }, [disabled]); + setTypesFilter((types) => types.clear()); + }, [setTypesFilter]); // Sort blocktypes in order of hierarchy followed by alphabetically for components const sortedBlockTypeKeys = Object.keys(blockTypes).sort((a, b) => { @@ -278,14 +240,10 @@ const FilterByBlockType: React.FC = ({ disabled = false sortedBlockTypes[key] = blockTypes[key]; }); - const appliedFilters = [...blockTypesFilter, ...problemTypesFilter].map( + const appliedFilters = [...typesFilter.blocks, ...typesFilter.problems].map( blockType => ({ label: }), ); - if (disabled) { - return null; - } - return ( = ({ disabled = false { diff --git a/src/search-manager/SearchManager.ts b/src/search-manager/SearchManager.ts index 314c90020a..334897cb9f 100644 --- a/src/search-manager/SearchManager.ts +++ b/src/search-manager/SearchManager.ts @@ -5,24 +5,23 @@ * https://github.com/algolia/instantsearch/issues/1658 */ import React from 'react'; -import { useSearchParams } from 'react-router-dom'; import { MeiliSearch, type Filter } from 'meilisearch'; import { union } from 'lodash'; import { CollectionHit, ContentHit, SearchSortOption, forceArray, } from './data/api'; +import { TypesFilterData, useStateOrUrlSearchParam } from './hooks'; import { useContentSearchConnection, useContentSearchResults } from './data/apiHooks'; +import { getBlockType } from '../generic/key-utils'; export interface SearchContextData { client?: MeiliSearch; indexName?: string; searchKeywords: string; setSearchKeywords: React.Dispatch>; - blockTypesFilter: string[]; - setBlockTypesFilter: React.Dispatch>; - problemTypesFilter: string[]; - setProblemTypesFilter: React.Dispatch>; + typesFilter: TypesFilterData; + setTypesFilter: React.Dispatch>; tagsFilter: string[]; setTagsFilter: React.Dispatch>; blockTypes: Record; @@ -47,64 +46,76 @@ export interface SearchContextData { const SearchContext = React.createContext(undefined); -/** - * Hook which lets you store state variables in the URL search parameters. - * - * It wraps useState with functions that get/set a query string - * search parameter when returning/setting the state variable. - * - */ -function useStateWithUrlSearchParam( - defaultValue: Type, - paramName: string, - // Returns the Type equivalent of the given string value, or - // undefined if value is invalid. - fromString: (value: string | null) => Type | undefined, - // Returns the string equivalent of the given Type value. - // Returning empty string/undefined will clear the url search paramName. - toString: (value: Type) => string | undefined, -): [value: Type, setter: React.Dispatch>] { - const [searchParams, setSearchParams] = useSearchParams(); - // The converted search parameter value takes precedence over the state value. - const returnValue: Type = fromString(searchParams.get(paramName)) ?? defaultValue; - // Function to update the url search parameter - const returnSetter: React.Dispatch> = React.useCallback((value: Type) => { - setSearchParams((prevParams) => { - const paramValue: string = toString(value) ?? ''; - const newSearchParams = new URLSearchParams(prevParams); - // If using the default paramValue, remove it from the search params. - if (paramValue === defaultValue) { - newSearchParams.delete(paramName); - } else { - newSearchParams.set(paramName, paramValue); - } - return newSearchParams; - }, { replace: true }); - }, [setSearchParams]); - - // Return the computed value and wrapped set state function - return [returnValue, returnSetter]; -} - export const SearchContextProvider: React.FC<{ - extraFilter?: Filter; + extraFilter?: Filter, + overrideTypesFilter?: TypesFilterData, overrideSearchSortOrder?: SearchSortOption children: React.ReactNode, closeSearchModal?: () => void, skipBlockTypeFetch?: boolean, skipUrlUpdate?: boolean, }> = ({ - overrideSearchSortOrder, skipBlockTypeFetch, skipUrlUpdate, ...props + overrideTypesFilter, + overrideSearchSortOrder, + skipBlockTypeFetch, + skipUrlUpdate, + ...props }) => { - const [searchKeywords, setSearchKeywords] = React.useState(''); - const [blockTypesFilter, setBlockTypesFilter] = React.useState([]); - const [problemTypesFilter, setProblemTypesFilter] = React.useState([]); - const [tagsFilter, setTagsFilter] = React.useState([]); - const [usageKey, setUsageKey] = useStateWithUrlSearchParam( + // Search parameters can be set via the query string + // E.g. ?q=draft+text + // TODO -- how to sanitize search terms? + const [searchKeywords, setSearchKeywords] = useStateOrUrlSearchParam( + '', + 'q', + (value: string) => value || '', + (value: string) => value || '', + skipUrlUpdate, + ); + + // Block + problem types use alphanumeric plus a few other characters. + // E.g ?type=html&type=video&type=p.multiplechoiceresponse + const [internalTypesFilter, setTypesFilter] = useStateOrUrlSearchParam( + new TypesFilterData(), + 'type', + (value: string | null) => new TypesFilterData(value), + (value: TypesFilterData | undefined) => (value ? value.toString() : undefined), + skipUrlUpdate, + ); + // Callers can override the types filter when searching, but we still preserve the user's selected state. + const typesFilter = overrideTypesFilter ?? internalTypesFilter; + + // Tags can be almost any string value (see openedx-learning's RESERVED_TAG_CHARS) + // and multiple tags may be selected together. + // E.g ?tag=Skills+>+Abilities&tag=Skills+>+Knowledge + const sanitizeTag = (value: string | null | undefined): string | undefined => ( + (value && /^[^\t;]+$/.test(value)) ? value : undefined + ); + const [tagsFilter, setTagsFilter] = useStateOrUrlSearchParam( + [], + 'tag', + sanitizeTag, + sanitizeTag, + skipUrlUpdate, + ); + + // E.g ?usageKey=lb:OpenCraft:libA:problem:5714eb65-7c36-4eee-8ab9-a54ed5a95849 + const sanitizeUsageKey = (value: string): string | undefined => { + try { + if (getBlockType(value)) { + return value; + } + } catch (error) { + // Error thrown if value cannot be parsed into a library usage key. + // Pass through to return below. + } + return undefined; + }; + const [usageKey, setUsageKey] = useStateOrUrlSearchParam( '', 'usageKey', - (value: string) => value, - (value: string) => value, + sanitizeUsageKey, + sanitizeUsageKey, + skipUrlUpdate, ); let extraFilter: string[] = forceArray(props.extraFilter); @@ -112,21 +123,15 @@ export const SearchContextProvider: React.FC<{ extraFilter = union(extraFilter, [`usage_key = "${usageKey}"`]); } - // The search sort order can be set via the query string - // E.g. ?sort=display_name:desc maps to SearchSortOption.TITLE_ZA. // Default sort by Most Relevant if there's search keyword(s), else by Recently Modified. const defaultSearchSortOrder = searchKeywords ? SearchSortOption.RELEVANCE : SearchSortOption.RECENTLY_MODIFIED; - let sortStateManager = React.useState(defaultSearchSortOrder); - const sortUrlStateManager = useStateWithUrlSearchParam( + const [searchSortOrder, setSearchSortOrder] = useStateOrUrlSearchParam( defaultSearchSortOrder, 'sort', (value: string) => Object.values(SearchSortOption).find((enumValue) => value === enumValue), (value: SearchSortOption) => value.toString(), + skipUrlUpdate, ); - if (!skipUrlUpdate) { - sortStateManager = sortUrlStateManager; - } - const [searchSortOrder, setSearchSortOrder] = sortStateManager; // SearchSortOption.RELEVANCE is special, it means "no custom sorting", so we // send it to useContentSearchResults as an empty array. const searchSortOrderToUse = overrideSearchSortOrder ?? searchSortOrder; @@ -137,16 +142,14 @@ export const SearchContextProvider: React.FC<{ } const canClearFilters = ( - blockTypesFilter.length > 0 - || problemTypesFilter.length > 0 + !typesFilter.isEmpty() || tagsFilter.length > 0 || !!usageKey ); const isFiltered = canClearFilters || (searchKeywords !== ''); const clearFilters = React.useCallback(() => { - setBlockTypesFilter([]); + setTypesFilter((types) => types.clear()); setTagsFilter([]); - setProblemTypesFilter([]); if (usageKey !== '') { setUsageKey(''); } @@ -161,8 +164,8 @@ export const SearchContextProvider: React.FC<{ indexName, extraFilter, searchKeywords, - blockTypesFilter, - problemTypesFilter, + blockTypesFilter: [...typesFilter.blocks], + problemTypesFilter: [...typesFilter.problems], tagsFilter, sort, skipBlockTypeFetch, @@ -174,10 +177,8 @@ export const SearchContextProvider: React.FC<{ indexName, searchKeywords, setSearchKeywords, - blockTypesFilter, - setBlockTypesFilter, - problemTypesFilter, - setProblemTypesFilter, + typesFilter, + setTypesFilter, tagsFilter, setTagsFilter, extraFilter, diff --git a/src/search-manager/hooks.ts b/src/search-manager/hooks.ts new file mode 100644 index 0000000000..4741c3469a --- /dev/null +++ b/src/search-manager/hooks.ts @@ -0,0 +1,133 @@ +import React from 'react'; +import { + type FromStringFn, + type ToStringFn, + useStateWithUrlSearchParam, +} from '../hooks'; + +/** + * Typed hook that returns useState if skipUrlUpdate, + * or useStateWithUrlSearchParam otherwise. + * + * Function is overloaded to accept simple Type or Type[] values. + * + * Provided here to reduce some code overhead in SearchManager. + */ +export function useStateOrUrlSearchParam( + defaultValue: Type[], + paramName: string, + fromString: FromStringFn, + toString: ToStringFn, + skipUrlUpdate?: boolean, +): [value: Type[], setter: React.Dispatch>]; +export function useStateOrUrlSearchParam( + defaultValue: Type, + paramName: string, + fromString: FromStringFn, + toString: ToStringFn, + skipUrlUpdate?: boolean, +): [value: Type, setter: React.Dispatch>]; +export function useStateOrUrlSearchParam( + defaultValue: Type | Type[], + paramName: string, + fromString: FromStringFn, + toString: ToStringFn, + skipUrlUpdate?: boolean, +): [value: Type | Type[], setter: React.Dispatch>] { + const useStateManager = React.useState(defaultValue); + const urlStateManager = useStateWithUrlSearchParam( + defaultValue, + paramName, + fromString, + toString, + ); + + return skipUrlUpdate ? useStateManager : urlStateManager; +} + +/** + * Helper class for managing Block + Problem type states. + * + * We use a class to store both Block and Problem types together because + * their behaviour is tightly intertwined: e.g if Block type "problem" is + * selected, that means all available Problem types are also selected. + * + */ +export class TypesFilterData { + #blocks = new Set(); + + #problems = new Set(); + + static #sanitizeType = (value: string | null | undefined): string | undefined => ( + (value && /^[a-z0-9._-]+$/.test(value)) + ? value + : undefined + ); + + static #sep1 = ','; // separates the individual types + + static #sep2 = '|'; // separates the block types from the problem types + + // Constructs a TypesFilterData from a string as generated from toString(). + constructor(value?: string | null) { + const [blocks, problems] = (value || '').split(TypesFilterData.#sep2); + this.union({ blocks, problems }); + } + + // Serialize the TypesFilterData to a string, or undefined if isEmpty(). + toString(): string | undefined { + if (this.isEmpty()) { + return undefined; + } + return [ + [...this.#blocks].join(TypesFilterData.#sep1), + [...this.#problems].join(TypesFilterData.#sep1), + ].join(TypesFilterData.#sep2); + } + + // Returns true if there are no block or problem types. + isEmpty(): boolean { + return !(this.#blocks.size || this.#problems.size); + } + + get blocks() : Set { + return this.#blocks; + } + + get problems(): Set { + return this.#problems; + } + + clear(): TypesFilterData { + this.#blocks.clear(); + this.#problems.clear(); + return this; + } + + union({ blocks, problems }: { + blocks?: string[] | Set | string | undefined, + problems?: string[] | Set | string | undefined, + }): void { + let newBlocks: string[]; + if (!blocks) { + newBlocks = []; + } else if (typeof blocks === 'string') { + newBlocks = blocks.split(TypesFilterData.#sep1) || []; + } else { + newBlocks = [...blocks]; + } + newBlocks = newBlocks.filter(TypesFilterData.#sanitizeType); + this.#blocks = new Set([...this.#blocks, ...newBlocks]); + + let newProblems: string[]; + if (!problems) { + newProblems = []; + } else if (typeof problems === 'string') { + newProblems = problems.split(TypesFilterData.#sep1) || []; + } else { + newProblems = [...problems]; + } + newProblems = newProblems.filter(TypesFilterData.#sanitizeType); + this.#problems = new Set([...this.#problems, ...newProblems]); + } +} diff --git a/src/search-manager/index.ts b/src/search-manager/index.ts index e2d4188be1..278a537158 100644 --- a/src/search-manager/index.ts +++ b/src/search-manager/index.ts @@ -9,5 +9,6 @@ export { default as SearchSortWidget } from './SearchSortWidget'; export { default as Stats } from './Stats'; export { HIGHLIGHT_PRE_TAG, HIGHLIGHT_POST_TAG } from './data/api'; export { useGetBlockTypes } from './data/apiHooks'; +export { TypesFilterData } from './hooks'; export type { CollectionHit, ContentHit, ContentHitTags } from './data/api'; diff --git a/src/search-modal/SearchUI.test.tsx b/src/search-modal/SearchUI.test.tsx index 2938e9fbd9..fb82694135 100644 --- a/src/search-modal/SearchUI.test.tsx +++ b/src/search-modal/SearchUI.test.tsx @@ -165,7 +165,7 @@ describe('', () => { // Enter a keyword - search for 'giraffe': fireEvent.change(getByRole('searchbox'), { target: { value: 'giraffe' } }); // Wait for the new search request to load all the results: - await waitFor(() => { expect(fetchMock).toHaveFetchedTimes(2, searchEndpoint, 'post'); }); + await waitFor(() => { expect(fetchMock).toHaveFetchedTimes(1, searchEndpoint, 'post'); }); // And make sure the request was limited to this course: expect(fetchMock).toHaveLastFetched((_url, req) => { const requestData = JSON.parse(req.body?.toString() ?? ''); @@ -353,7 +353,7 @@ describe('', () => { // Enter a keyword - search for 'giraffe': fireEvent.change(getByRole('searchbox'), { target: { value: 'giraffe' } }); // Wait for the new search request to load all the results and the filter options, based on the search so far: - await waitFor(() => { expect(fetchMock).toHaveFetchedTimes(2, searchEndpoint, 'post'); }); + await waitFor(() => { expect(fetchMock).toHaveFetchedTimes(1, searchEndpoint, 'post'); }); // And make sure the request was limited to this course: expect(fetchMock).toHaveLastFetched((_url, req) => { const requestData = JSON.parse(req.body?.toString() ?? ''); @@ -367,6 +367,16 @@ describe('', () => { expect(getByText(mockResultDisplayName)).toBeInTheDocument(); }); + afterEach(async () => { + // Clear any search filters applied by the previous test. + // We need to do this because search filters are stored in the URL, and so they can leak between tests. + const { queryByRole } = rendered; + const clearFilters = await queryByRole('button', { name: /clear filters/i }); + if (clearFilters) { + fireEvent.click(clearFilters); + } + }); + it('can filter results by component/XBlock type', async () => { const { getByRole, getByText } = rendered; // Now open the filters menu: @@ -379,7 +389,7 @@ describe('', () => { expect(rendered.getByRole('button', { name: /type: problem/i, hidden: true })).toBeInTheDocument(); }); // Now wait for the filter to be applied and the new results to be fetched. - await waitFor(() => { expect(fetchMock).toHaveFetchedTimes(3, searchEndpoint, 'post'); }); + await waitFor(() => { expect(fetchMock).toHaveFetchedTimes(2, searchEndpoint, 'post'); }); // Because we're mocking the results, there's no actual changes to the mock results, // but we can verify that the filter was sent in the request expect(fetchMock).toHaveLastFetched((_url, req) => { @@ -409,10 +419,10 @@ describe('', () => { await waitFor(() => { expect(getByLabelText(checkboxLabel)).toBeInTheDocument(); }); // In addition to the checkbox, there is another button to show the child tags: expect(getByLabelText(/Expand to show child tags of "ESDC Skills and Competencies"/i)).toBeInTheDocument(); - const competentciesCheckbox = getByLabelText(checkboxLabel); - fireEvent.click(competentciesCheckbox, {}); + const competenciesCheckbox = getByLabelText(checkboxLabel); + fireEvent.click(competenciesCheckbox, {}); // Now wait for the filter to be applied and the new results to be fetched. - await waitFor(() => { expect(fetchMock).toHaveFetchedTimes(3, searchEndpoint, 'post'); }); + await waitFor(() => { expect(fetchMock).toHaveFetchedTimes(2, searchEndpoint, 'post'); }); // Because we're mocking the results, there's no actual changes to the mock results, // but we can verify that the filter was sent in the request expect(fetchMock).toBeDone((_url, req) => { @@ -447,7 +457,7 @@ describe('', () => { const abilitiesTagFilterCheckbox = getByLabelText(childTagLabel); fireEvent.click(abilitiesTagFilterCheckbox); // Now wait for the filter to be applied and the new results to be fetched. - await waitFor(() => { expect(fetchMock).toHaveFetchedTimes(3, searchEndpoint, 'post'); }); + await waitFor(() => { expect(fetchMock).toHaveFetchedTimes(2, searchEndpoint, 'post'); }); // Because we're mocking the results, there's no actual changes to the mock results, // but we can verify that the filter was sent in the request expect(fetchMock).toBeDone((_url, req) => { diff --git a/src/testUtils.tsx b/src/testUtils.tsx index 6cfda2c0f8..b9fdf8e611 100644 --- a/src/testUtils.tsx +++ b/src/testUtils.tsx @@ -16,6 +16,7 @@ import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; import { render, type RenderResult } from '@testing-library/react'; import MockAdapter from 'axios-mock-adapter'; import { + generatePath, MemoryRouter, MemoryRouterProps, Route, @@ -94,10 +95,7 @@ const RouterAndRoute: React.FC = ({ const newRouterProps = { ...routerProps }; if (!routerProps.initialEntries) { // Substitute the params into the URL so '/library/:libraryId' becomes '/library/lib:org:123' - let pathWithParams = path; - for (const [key, value] of Object.entries(params)) { - pathWithParams = pathWithParams.replaceAll(`:${key}`, value); - } + let pathWithParams = generatePath(path, params); if (pathWithParams.endsWith('/*')) { // Some routes (that contain child routes) need to end with /* in the but not in the router pathWithParams = pathWithParams.substring(0, pathWithParams.length - 1);