From aa2582701f2d4ebcbda6fdfe8bc30e9687458ebc Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Wed, 18 Dec 2024 16:59:47 +1030 Subject: [PATCH 01/17] fix: use generatePath in testUtils to support optional parameters in test paths --- src/testUtils.tsx | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) 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); From a4728e1eddb88c57820871b931c874914bed4951 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Thu, 19 Dec 2024 11:37:19 +1030 Subject: [PATCH 02/17] fix: test uses contentId=undefined, so made this param optional in the test route. --- src/content-tags-drawer/ContentTagsDrawer.test.jsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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(); From ce4c27ea6a7290109de68633bae58a2d4816beb8 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Thu, 12 Dec 2024 12:15:28 +1030 Subject: [PATCH 03/17] feat: adds sharable URLs for library components/collections * Restructures LibraryLayout so that LibraryContext can useParams() to initialize its componentId/collectionId instead of having to parse complicated route strings. Initialization-from-URL can be disabled for the content pickers by passing skipUrlUpdate to the LibraryContext -- which is needed by the component picker. * Clicking/selecting a ComponentCard/CollectionCard navigates to an appropriate component/collection route given the current page. * Adds useLibraryRoutes() hook so components can easily navigate to the best available route without having to know the route strings or maintain search params. * Moves ContentType declaration to the new routes.ts to avoid circular imports. * Renames openInfoSidebar to openLibrarySidebar, so that openInfoSidebar can be used to open the best sidebar for a given library/component/collection. --- .../LibraryAuthoringPage.tsx | 59 +++++---- src/library-authoring/LibraryContent.tsx | 7 +- src/library-authoring/LibraryLayout.tsx | 60 ++++++---- .../add-content/AddContentContainer.test.tsx | 8 +- .../PickLibraryContentModal.test.tsx | 1 - .../collections/CollectionInfo.tsx | 27 ++--- .../LibraryCollectionComponents.tsx | 3 +- .../collections/LibraryCollectionPage.tsx | 14 ++- .../common/context/LibraryContext.tsx | 26 +++- .../common/context/SidebarContext.tsx | 22 +++- .../component-picker/ComponentPicker.tsx | 1 + .../components/BaseComponentCard.tsx | 8 +- .../components/CollectionCard.tsx | 13 +- .../components/ComponentCard.tsx | 15 ++- .../library-info/LibraryInfoHeader.tsx | 4 +- src/library-authoring/routes.ts | 113 ++++++++++++++++++ 16 files changed, 283 insertions(+), 98 deletions(-) create mode 100644 src/library-authoring/routes.ts diff --git a/src/library-authoring/LibraryAuthoringPage.tsx b/src/library-authoring/LibraryAuthoringPage.tsx index 46dd1cd49d..d1412a1262 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'; @@ -36,11 +31,12 @@ import { SearchKeywordsField, SearchSortWidget, } 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 +47,7 @@ const HeaderActions = () => { const { openAddContentSidebar, - openInfoSidebar, + openLibrarySidebar, closeLibrarySidebar, sidebarComponentInfo, } = useSidebarContext(); @@ -60,13 +56,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(); + } + }, [navigateTo, sidebarComponentInfo, closeLibrarySidebar, openLibrarySidebar]); return (
@@ -124,8 +126,6 @@ interface LibraryAuthoringPageProps { const LibraryAuthoringPage = ({ returnToLibrarySelection }: LibraryAuthoringPageProps) => { const intl = useIntl(); - const location = useLocation(); - const navigate = useNavigate(); const { isLoadingPage: isLoadingStudioHome, @@ -139,29 +139,41 @@ const LibraryAuthoringPage = ({ returnToLibrarySelection }: LibraryAuthoringPage libraryData, isLoadingLibraryData, showOnlyPublished, + componentId, + collectionId, } = useLibraryContext(); const { openInfoSidebar, sidebarComponentInfo } = useSidebarContext(); + const { insideCollections, insideComponents, navigateTo } = useLibraryRoutes(); + + // The activeKey determines the currently selected tab. const [activeKey, setActiveKey] = useState(ContentType.home); + const getActiveKey = () => { + if (insideCollections) { + return ContentType.collections; + } + if (insideComponents) { + return ContentType.components; + } + return ContentType.home; + }; useEffect(() => { - const currentPath = location.pathname.split('/').pop(); + const contentType = getActiveKey(); - if (componentPickerMode || currentPath === libraryId || currentPath === '') { + if (componentPickerMode) { setActiveKey(ContentType.home); - } else if (currentPath && currentPath in ContentType) { - setActiveKey(ContentType[currentPath] || ContentType.home); + } else { + setActiveKey(contentType); } }, []); useEffect(() => { if (!componentPickerMode) { - openInfoSidebar(); + openInfoSidebar(componentId, collectionId); } }, []); - const [searchParams] = useSearchParams(); - if (isLoadingLibraryData) { return ; } @@ -181,10 +193,7 @@ const LibraryAuthoringPage = ({ returnToLibrarySelection }: LibraryAuthoringPage const handleTabChange = (key: ContentType) => { setActiveKey(key); if (!componentPickerMode) { - navigate({ - pathname: key, - search: searchParams.toString(), - }); + navigateTo({ contentType: key }); } }; 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..10b2541353 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 { libraryId, setCollectionId } = useLibraryContext(); const { sidebarComponentInfo, setSidebarCurrentTab } = useSidebarContext(); const tab: CollectionInfoTab = ( sidebarComponentInfo?.currentTab && isCollectionInfoTab(sidebarComponentInfo.currentTab) ) ? sidebarComponentInfo?.currentTab : 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 ( diff --git a/src/library-authoring/collections/LibraryCollectionComponents.tsx b/src/library-authoring/collections/LibraryCollectionComponents.tsx index e0338dd11e..6fcd79aa5c 100644 --- a/src/library-authoring/collections/LibraryCollectionComponents.tsx +++ b/src/library-authoring/collections/LibraryCollectionComponents.tsx @@ -3,7 +3,8 @@ import { NoComponents, NoSearchResults } from '../EmptyStates'; import { useSearchContext } from '../../search-manager'; import messages from './messages'; import { useSidebarContext } from '../common/context/SidebarContext'; -import LibraryContent, { ContentType } from '../LibraryContent'; +import LibraryContent from '../LibraryContent'; +import { ContentType } from '../routes'; const LibraryCollectionComponents = () => { 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..3f5408830b 100644 --- a/src/library-authoring/common/context/SidebarContext.tsx +++ b/src/library-authoring/common/context/SidebarContext.tsx @@ -48,7 +48,8 @@ export enum SidebarAdditionalActions { export type SidebarContextData = { closeLibrarySidebar: () => void; openAddContentSidebar: () => void; - openInfoSidebar: () => void; + openInfoSidebar: (componentId?: string, collectionId?: string) => void; + openLibrarySidebar: () => void; openCollectionInfoSidebar: (collectionId: string, additionalAction?: SidebarAdditionalActions) => void; openComponentInfoSidebar: (usageKey: string, additionalAction?: SidebarAdditionalActions) => void; sidebarComponentInfo?: SidebarComponentInfo; @@ -71,7 +72,7 @@ type SidebarProviderProps = { }; /** - * React component to provide `LibraryContext` + * React component to provide `SidebarContext` */ export const SidebarProvider = ({ children, @@ -81,7 +82,7 @@ export const SidebarProvider = ({ initialSidebarComponentInfo, ); - /** Helper function to consume addtional action once performed. + /** Helper function to consume additional action once performed. Required to redo the action. */ const resetSidebarAdditionalActions = useCallback(() => { @@ -94,7 +95,7 @@ export const SidebarProvider = ({ const openAddContentSidebar = useCallback(() => { setSidebarComponentInfo({ id: '', type: SidebarBodyComponentId.AddContent }); }, []); - const openInfoSidebar = useCallback(() => { + const openLibrarySidebar = useCallback(() => { setSidebarComponentInfo({ id: '', type: SidebarBodyComponentId.Info }); }, []); @@ -119,6 +120,16 @@ export const SidebarProvider = ({ })); }, []); + const openInfoSidebar = useCallback((componentId?: string, collectionId?: string) => { + if (componentId) { + openComponentInfoSidebar(componentId); + } else if (collectionId) { + openCollectionInfoSidebar(collectionId); + } else { + openLibrarySidebar(); + } + }, []); + const setSidebarCurrentTab = useCallback((tab: CollectionInfoTab | ComponentInfoTab) => { setSidebarComponentInfo((prev) => (prev && { ...prev, currentTab: tab })); }, []); @@ -128,6 +139,7 @@ export const SidebarProvider = ({ closeLibrarySidebar, openAddContentSidebar, openInfoSidebar, + openLibrarySidebar, openComponentInfoSidebar, sidebarComponentInfo, openCollectionInfoSidebar, @@ -140,6 +152,7 @@ export const SidebarProvider = ({ closeLibrarySidebar, openAddContentSidebar, openInfoSidebar, + openLibrarySidebar, openComponentInfoSidebar, sidebarComponentInfo, openCollectionInfoSidebar, @@ -162,6 +175,7 @@ export function useSidebarContext(): SidebarContextData { closeLibrarySidebar: () => {}, openAddContentSidebar: () => {}, openInfoSidebar: () => {}, + openLibrarySidebar: () => {}, openComponentInfoSidebar: () => {}, openCollectionInfoSidebar: () => {}, resetSidebarAdditionalActions: () => {}, 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..6f5fdb8c59 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, @@ -23,6 +23,8 @@ import { useComponentPickerContext } from '../common/context/ComponentPickerCont import { useLibraryContext } from '../common/context/LibraryContext'; import { SidebarAdditionalActions, 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'; @@ -200,6 +202,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/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.ts b/src/library-authoring/routes.ts new file mode 100644 index 0000000000..59d0811790 --- /dev/null +++ b/src/library-authoring/routes.ts @@ -0,0 +1,113 @@ +/** + * 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 = { + COMPONENTS: '/components/:componentId?', + COLLECTIONS: '/collections/:collectionId?', + COMPONENT: '/component/:componentId', + COLLECTION: '/collection/:collectionId/:componentId?', + HOME: '/:collectionId?', +}; + +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, + componentId, + // Overwrite the current collectionId param only if one is specified + ...(collectionId && { collectionId }), + }; + let route; + + // contentType overrides the current route + 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) { + route = ( + (collectionId && collectionId === params.collectionId) + // Open the previously-selected collection + ? ROUTES.COLLECTION + // Otherwise just preview the collection, if specified + : ROUTES.COLLECTIONS + ); + } else if (insideCollection) { + route = ROUTES.COLLECTION; + } else if (insideComponents) { + route = ROUTES.COMPONENTS; + } else if (componentId) { + route = ROUTES.COMPONENT; + } else { + route = ( + (collectionId && collectionId === params.collectionId) + // Open the previously-selected collection + ? ROUTES.COLLECTION + // Otherwise just preview the collection, if specified + : ROUTES.HOME + ); + } + + const newPath = generatePath(BASE_ROUTE + route, routeParams); + navigate({ + pathname: newPath, + search: searchParams.toString(), + }); + }, [navigate, params, searchParams, pathname]); + + return { + navigateTo, + insideCollection, + insideCollections, + insideComponents, + }; +}; From fc2f467c8d3ce268ef395c859e5a59cd159497b9 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Thu, 19 Dec 2024 11:38:17 +1030 Subject: [PATCH 04/17] feat: store search keywords in query string This triggers a navigate() call when the searchbox changes, which resets the query counts in testing. So had to reduce the expected counts by 1. --- src/search-manager/SearchManager.ts | 17 ++++++++++++++++- src/search-modal/SearchUI.test.tsx | 10 +++++----- 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/src/search-manager/SearchManager.ts b/src/search-manager/SearchManager.ts index 314c90020a..434ba204ba 100644 --- a/src/search-manager/SearchManager.ts +++ b/src/search-manager/SearchManager.ts @@ -96,7 +96,22 @@ export const SearchContextProvider: React.FC<{ }> = ({ overrideSearchSortOrder, skipBlockTypeFetch, skipUrlUpdate, ...props }) => { - const [searchKeywords, setSearchKeywords] = React.useState(''); + // Search parameters can be set via the query string + // E.g. q=draft+text + // TODO -- how to scrub search terms? + const keywordStateManager = React.useState(''); + const keywordUrlStateManager = useStateWithUrlSearchParam( + '', + 'q', + (value: string) => value || '', + (value: string) => value || '', + ); + const [searchKeywords, setSearchKeywords] = ( + skipUrlUpdate + ? keywordStateManager + : keywordUrlStateManager + ); + const [blockTypesFilter, setBlockTypesFilter] = React.useState([]); const [problemTypesFilter, setProblemTypesFilter] = React.useState([]); const [tagsFilter, setTagsFilter] = React.useState([]); diff --git a/src/search-modal/SearchUI.test.tsx b/src/search-modal/SearchUI.test.tsx index 2938e9fbd9..5e100ededa 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() ?? ''); @@ -379,7 +379,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) => { @@ -412,7 +412,7 @@ describe('', () => { const competentciesCheckbox = getByLabelText(checkboxLabel); fireEvent.click(competentciesCheckbox, {}); // 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 +447,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) => { From f6b46c483de15677e35a4cc0c484427156117304 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Thu, 19 Dec 2024 12:01:25 +1030 Subject: [PATCH 05/17] feat: store sidebar tab and additional action in query string Related changes: * adds "manage-team" sidebar action to trigger LibraryInfo's "Manage Team" modal * moves useStateWithUrlSearchParam up to hooks.ts so it can be used by search-manager and library-authoring * splits sidebarCurrentTab and additionalAction out of SidebarComponentInfo -- they're now managed independently as url parameters. This also simplifies setSidebarComponentInfo: we can simply set the new id + key, and so there's no need to merge the ...prev state and new state. * shortens some sidebar property names: sidebarCurrentTab => sidebarTab sidebarAdditionalAction => sidebarAction * test: Tab changes now trigger a navigate() call, which invalidates the within(sidebar) element in the tests. So using screen instead. --- src/hooks.ts | 51 +++++++++- .../LibraryAuthoringPage.test.tsx | 12 +-- .../collections/CollectionInfo.tsx | 8 +- .../common/context/SidebarContext.tsx | 92 +++++++++++-------- .../component-info/ComponentInfo.tsx | 30 +++--- .../component-info/ComponentManagement.tsx | 8 +- .../component-info/ManageCollections.tsx | 10 +- .../components/ComponentCard.tsx | 10 +- .../library-info/LibraryInfo.tsx | 13 ++- src/search-manager/SearchManager.ts | 41 +-------- 10 files changed, 153 insertions(+), 122 deletions(-) diff --git a/src/hooks.ts b/src/hooks.ts index d9cde4a80f..facaf65ef8 100644 --- a/src/hooks.ts +++ b/src/hooks.ts @@ -1,6 +1,12 @@ -import { useEffect, useState } from 'react'; +import { + type Dispatch, + type SetStateAction, + useCallback, + useEffect, + 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 +83,44 @@ export const useLoadOnScroll = ( return () => { }; }, [hasNextPage, isFetchingNextPage, fetchNextPage]); }; + +/** + * Hook which stores 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. + * + * @param defaultValue: Type + * Returned when no valid value is found in the url search parameter. + * @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: (value: string | null) => Type | undefined, + toString: (value: Type) => string | undefined, +): [value: Type, setter: Dispatch>] { + const [searchParams, setSearchParams] = useSearchParams(); + const returnValue: Type = fromString(searchParams.get(paramName)) ?? defaultValue; + // Function to update the url search parameter + const returnSetter: Dispatch> = 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]; +} diff --git a/src/library-authoring/LibraryAuthoringPage.test.tsx b/src/library-authoring/LibraryAuthoringPage.test.tsx index 0c725fbf2d..66d72800c3 100644 --- a/src/library-authoring/LibraryAuthoringPage.test.tsx +++ b/src/library-authoring/LibraryAuthoringPage.test.tsx @@ -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 () => { diff --git a/src/library-authoring/collections/CollectionInfo.tsx b/src/library-authoring/collections/CollectionInfo.tsx index 10b2541353..277a6fc1c6 100644 --- a/src/library-authoring/collections/CollectionInfo.tsx +++ b/src/library-authoring/collections/CollectionInfo.tsx @@ -27,11 +27,11 @@ const CollectionInfo = () => { const { componentPickerMode } = useComponentPickerContext(); const { libraryId, setCollectionId } = useLibraryContext(); - const { sidebarComponentInfo, setSidebarCurrentTab } = useSidebarContext(); + 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 collectionId = sidebarComponentInfo?.id; // istanbul ignore if: this should never happen @@ -70,7 +70,7 @@ const CollectionInfo = () => { className="my-3 d-flex justify-content-around" defaultActiveKey={COMPONENT_INFO_TABS.Manage} activeKey={tab} - onSelect={setSidebarCurrentTab} + onSelect={setSidebarTab} > ( 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 = { @@ -50,11 +55,14 @@ export type SidebarContextData = { openAddContentSidebar: () => void; openInfoSidebar: (componentId?: string, collectionId?: string) => void; openLibrarySidebar: () => void; - openCollectionInfoSidebar: (collectionId: string, additionalAction?: SidebarAdditionalActions) => void; - openComponentInfoSidebar: (usageKey: string, additionalAction?: SidebarAdditionalActions) => 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; }; /** @@ -82,12 +90,22 @@ export const SidebarProvider = ({ initialSidebarComponentInfo, ); - /** Helper function to consume additional 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); @@ -99,25 +117,18 @@ export const SidebarProvider = ({ 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 openInfoSidebar = useCallback((componentId?: string, collectionId?: string) => { @@ -130,10 +141,6 @@ export const SidebarProvider = ({ } }, []); - const setSidebarCurrentTab = useCallback((tab: CollectionInfoTab | ComponentInfoTab) => { - setSidebarComponentInfo((prev) => (prev && { ...prev, currentTab: tab })); - }, []); - const context = useMemo(() => { const contextValue = { closeLibrarySidebar, @@ -143,8 +150,11 @@ export const SidebarProvider = ({ openComponentInfoSidebar, sidebarComponentInfo, openCollectionInfoSidebar, - resetSidebarAdditionalActions, - setSidebarCurrentTab, + sidebarAction, + setSidebarAction, + resetSidebarAction, + sidebarTab, + setSidebarTab, }; return contextValue; @@ -156,8 +166,11 @@ export const SidebarProvider = ({ openComponentInfoSidebar, sidebarComponentInfo, openCollectionInfoSidebar, - resetSidebarAdditionalActions, - setSidebarCurrentTab, + sidebarAction, + setSidebarAction, + resetSidebarAction, + sidebarTab, + setSidebarTab, ]); return ( @@ -178,8 +191,11 @@ export function useSidebarContext(): SidebarContextData { 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.tsx b/src/library-authoring/component-info/ManageCollections.tsx index b7cbc98323..581e427ccb 100644 --- a/src/library-authoring/component-info/ManageCollections.tsx +++ b/src/library-authoring/component-info/ManageCollections.tsx @@ -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,8 +191,8 @@ const ComponentCollections = ({ collections, onManageClick }: { }; const ManageCollections = ({ usageKey, collections }: ManageCollectionsProps) => { - const { sidebarComponentInfo, resetSidebarAdditionalActions } = useSidebarContext(); - const jumpToCollections = sidebarComponentInfo?.additionalAction === SidebarAdditionalActions.JumpToAddCollections; + const { sidebarAction, resetSidebarAction } = useSidebarContext(); + const jumpToCollections = sidebarAction === SidebarActions.JumpToAddCollections; const [editing, setEditing] = useState(jumpToCollections); const collectionNames = collections.map((collection) => collection.title); @@ -200,12 +200,12 @@ const ManageCollections = ({ usageKey, collections }: ManageCollectionsProps) => if (jumpToCollections) { setEditing(true); } - }, [sidebarComponentInfo]); + }, [jumpToCollections]); useEffect(() => { // This is required to redo actions. if (!editing) { - resetSidebarAdditionalActions(); + resetSidebarAction(); } }, [editing]); diff --git a/src/library-authoring/components/ComponentCard.tsx b/src/library-authoring/components/ComponentCard.tsx index 6f5fdb8c59..196a060f97 100644 --- a/src/library-authoring/components/ComponentCard.tsx +++ b/src/library-authoring/components/ComponentCard.tsx @@ -21,7 +21,7 @@ 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'; @@ -46,6 +46,7 @@ export const ComponentMenu = ({ usageKey }: { usageKey: string }) => { sidebarComponentInfo, openComponentInfoSidebar, closeLibrarySidebar, + setSidebarAction, } = useSidebarContext(); const canEdit = usageKey && canEditComponent(usageKey); @@ -75,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 ( 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/search-manager/SearchManager.ts b/src/search-manager/SearchManager.ts index 434ba204ba..acf5a0519b 100644 --- a/src/search-manager/SearchManager.ts +++ b/src/search-manager/SearchManager.ts @@ -5,7 +5,6 @@ * 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'; @@ -13,6 +12,7 @@ import { CollectionHit, ContentHit, SearchSortOption, forceArray, } from './data/api'; import { useContentSearchConnection, useContentSearchResults } from './data/apiHooks'; +import { useStateWithUrlSearchParam } from '../hooks'; export interface SearchContextData { client?: MeiliSearch; @@ -47,45 +47,6 @@ 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; overrideSearchSortOrder?: SearchSortOption From 37370d2d9f2d06c35b047d8d8f4baf5b9843075d Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Fri, 20 Dec 2024 23:35:18 +1030 Subject: [PATCH 06/17] feat: store selected tags in the URL search string Related features: 1. We can filter on more than one tag, so the search field needs to store an Array of values which still need to be sanitised. This change adds useListHelpers to assist with the parsing and validating of an Array of Typed values. 2. SearchManager takes a skipUrlUpdate property which should disable using search params for state variables. Our "sort" parameters respected this, but none of the other search parameters did. So this change also adds a wrapper hook called useStateOrUrlSearchParam that handles this switch cleanly. This feature also revealed two bugs fixed in useStateWithUrlSearchParam: 1. When the returnSetter is called with a function instead of a simple value, we need to pass in previous returnValue to the function so it can generate the new value. 2. When the returnSetter is called multiple times by a single callback (like with clearFilters), the latest changes to the UrlSearchParams weren't showing up. To fix this, we had to use the location.search string as the "latest" previous url search, not the prevParams passed into setSearchParams, because these params may not have the latest updates. --- src/hooks.ts | 109 ++++++++++++++++++++++++---- src/search-manager/SearchManager.ts | 80 +++++++++++++++----- 2 files changed, 155 insertions(+), 34 deletions(-) diff --git a/src/hooks.ts b/src/hooks.ts index facaf65ef8..7a3341d862 100644 --- a/src/hooks.ts +++ b/src/hooks.ts @@ -3,6 +3,7 @@ import { type SetStateAction, useCallback, useEffect, + useRef, useState, } from 'react'; import { history } from '@edx/frontend-platform'; @@ -85,10 +86,13 @@ export const useLoadOnScroll = ( }; /** - * Hook which stores 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. + * Types used by the useListHelpers and useStateWithUrlSearchParam hooks. + */ +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. * * @param defaultValue: Type * Returned when no valid value is found in the url search parameter. @@ -101,26 +105,103 @@ export const useLoadOnScroll = ( export function useStateWithUrlSearchParam( defaultValue: Type, paramName: string, - fromString: (value: string | null) => Type | undefined, - toString: (value: Type) => string | undefined, + fromString: FromStringFn, + toString: ToStringFn, ): [value: 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 (like 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, but 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 returnValue: Type = fromString(searchParams.get(paramName)) ?? defaultValue; - // Function to update the url search parameter - const returnSetter: Dispatch> = 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) { + // Update the url search parameter using: + type ReturnSetterParams = ( + // a Type value + value?: Type + // or a function that returns a Type from the previous returnValue + | ((value: Type) => Type) + ) => void; + const returnSetter: Dispatch> = useCallback((value) => { + setSearchParams((/* prev */) => { + const useValue = value instanceof Function ? value(returnValue) : value; + const paramValue = toString(useValue); + const newSearchParams = new URLSearchParams(locationRef.current.search); + // If the provided value was invalid (toString returned undefined) + // or the same as the defaultValue, remove it from the search params. + if (paramValue === undefined || paramValue === defaultValue) { newSearchParams.delete(paramName); } else { newSearchParams.set(paramName, paramValue); } + + // Update locationRef + locationRef.current.search = newSearchParams.toString(); + return newSearchParams; }, { replace: true }); - }, [setSearchParams]); + }, [returnValue, setSearchParams]); // Return the computed value and wrapped set state function return [returnValue, returnSetter]; } + +/** + * Helper hook for useStateWithUrlSearchParam. + * + * useListHelpers provides toString and fromString handlers that can: + * - split/join a list of values using a separator string, and + * - validate each value using the provided functions, omitting any invalid values. + * + * @param fromString + * Serialize a string to a Type, or undefined if not valid. + * @param toString + * Deserialize a Type to a string. + * @param separator : string to use when splitting/joining the types. + * Defaults value is ','. + */ +export function useListHelpers({ + fromString, + toString, + separator = ',', +}: { + fromString: FromStringFn, + toString: ToStringFn, + separator?: string; +}): [ FromStringFn, ToStringFn ] { + const isType = (item: Type | undefined): item is Type => item !== undefined; + + // Split the given string with separator, + // and convert the parts to a list of Types, omiting any invalid Types. + const fromStringToList : FromStringFn = (value: string) => ( + value + ? value.split(separator).map(fromString).filter(isType) + : [] + ); + // Convert an array of Types to strings and join with separator. + // Returns undefined if the given list contains no valid Types. + const fromListToString : ToStringFn = (value: Type[]) => { + const stringValues = value.map(toString).filter((val) => val !== undefined); + return ( + stringValues && stringValues.length + ? stringValues.join(separator) + : undefined + ); + }; + return [fromStringToList, fromListToString]; +} diff --git a/src/search-manager/SearchManager.ts b/src/search-manager/SearchManager.ts index acf5a0519b..5a44db4ea9 100644 --- a/src/search-manager/SearchManager.ts +++ b/src/search-manager/SearchManager.ts @@ -12,7 +12,35 @@ import { CollectionHit, ContentHit, SearchSortOption, forceArray, } from './data/api'; import { useContentSearchConnection, useContentSearchResults } from './data/apiHooks'; -import { useStateWithUrlSearchParam } from '../hooks'; +import { + type FromStringFn, + type ToStringFn, + useListHelpers, + useStateWithUrlSearchParam, +} from '../hooks'; + +/** + * Typed hook that returns useState if skipUrlUpdate, + * or useStateWithUrlSearchParam if it's not. + * + * Provided here to reduce some code overhead in SearchManager. + */ +function useStateOrUrlSearchParam( + defaultValue: Type, + paramName: string, + fromString: FromStringFn, + toString: ToStringFn, + skipUrlUpdate?: boolean, +): [value: Type, setter: React.Dispatch>] { + const useStateManager = React.useState(defaultValue); + const urlStateManager = useStateWithUrlSearchParam( + defaultValue, + paramName, + fromString, + toString, + ); + return skipUrlUpdate ? useStateManager : urlStateManager; +} export interface SearchContextData { client?: MeiliSearch; @@ -58,29 +86,47 @@ export const SearchContextProvider: React.FC<{ overrideSearchSortOrder, skipBlockTypeFetch, skipUrlUpdate, ...props }) => { // Search parameters can be set via the query string - // E.g. q=draft+text - // TODO -- how to scrub search terms? - const keywordStateManager = React.useState(''); - const keywordUrlStateManager = useStateWithUrlSearchParam( + // E.g. ?q=draft+text + // TODO -- how to sanitize search terms? + const [searchKeywords, setSearchKeywords] = useStateOrUrlSearchParam( '', 'q', (value: string) => value || '', (value: string) => value || '', - ); - const [searchKeywords, setSearchKeywords] = ( - skipUrlUpdate - ? keywordStateManager - : keywordUrlStateManager + skipUrlUpdate, ); const [blockTypesFilter, setBlockTypesFilter] = React.useState([]); const [problemTypesFilter, setProblemTypesFilter] = React.useState([]); - const [tagsFilter, setTagsFilter] = React.useState([]); - const [usageKey, setUsageKey] = useStateWithUrlSearchParam( + // Tags can be almost any string value, except our separator (|) + // TODO how to sanitize tags? + // E.g ?tags=Skills+>+Abilities|Skills+>+Knowledge + const sanitizeTag = (value: string | null | undefined): string | undefined => ( + (value && /^[^|]+$/.test(value)) + ? value + : undefined + ); + const [tagToList, listToTag] = useListHelpers({ + toString: sanitizeTag, + fromString: sanitizeTag, + separator: '|', + }); + const [tagsFilter, setTagsFilter] = useStateOrUrlSearchParam( + [], + 'tags', + tagToList, + listToTag, + skipUrlUpdate, + ); + + // E.g ?usageKey=lb:OpenCraft:libA:problem:5714eb65-7c36-4eee-8ab9-a54ed5a95849 + const [usageKey, setUsageKey] = useStateOrUrlSearchParam( '', 'usageKey', + // TODO should sanitize usageKeys too. (value: string) => value, (value: string) => value, + skipUrlUpdate, ); let extraFilter: string[] = forceArray(props.extraFilter); @@ -88,21 +134,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; From 1736942dae4abfb932a9482565a643f857feb833 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Fri, 20 Dec 2024 23:47:05 +1030 Subject: [PATCH 07/17] refactor: use one typesFilter state in SearchManager Combine blockTypesFilter + problemTypesFilter into a single state variable called typesFilter, and store selected block + problem types in the url search parameters. These two states are heavily interdependent, and so the FilterByBlockType code was quite complicated. Then storing these states as url search params caused re-renders that disrupted the delicate dependencies between these two states. This change stores both these type lists using a single TypesFilterData class to simplify the code and avoid bugs when updating their states. --- src/search-manager/FilterByBlockType.tsx | 114 +++++++++----------- src/search-manager/SearchManager.ts | 130 ++++++++++++++++++++--- 2 files changed, 162 insertions(+), 82 deletions(-) diff --git a/src/search-manager/FilterByBlockType.tsx b/src/search-manager/FilterByBlockType.tsx index 98410be18b..fd83646781 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 @@ -214,15 +199,12 @@ const FilterItem = ({ blockType, count } : FilterItemProps) => { const FilterByBlockType: React.FC> = () => { const { blockTypes, - blockTypesFilter, - problemTypesFilter, - setBlockTypesFilter, - setProblemTypesFilter, + typesFilter, + setTypesFilter, } = useSearchContext(); const clearFilters = useCallback(/* istanbul ignore next */ () => { - setBlockTypesFilter([]); - setProblemTypesFilter([]); + setTypesFilter((types) => types.clear()); }, []); // Sort blocktypes in order of hierarchy followed by alphabetically for components @@ -258,11 +240,11 @@ const FilterByBlockType: React.FC> = () => { sortedBlockTypes[key] = blockTypes[key]; }); - const appliedFilters = [...blockTypesFilter, ...problemTypesFilter].map( + const appliedFilters = [...typesFilter.blocks, ...typesFilter.problems].map( blockType => ({ label: }), ); - const hiddenBlockTypes = blockTypesFilter.filter(blockType => !Object.keys(blockTypes).includes(blockType)); + const hiddenBlockTypes = [...typesFilter.blocks].filter(blockType => !Object.keys(blockTypes).includes(blockType)); return ( > = () => { { diff --git a/src/search-manager/SearchManager.ts b/src/search-manager/SearchManager.ts index 5a44db4ea9..a03907d88a 100644 --- a/src/search-manager/SearchManager.ts +++ b/src/search-manager/SearchManager.ts @@ -42,15 +42,92 @@ function useStateOrUrlSearchParam( return skipUrlUpdate ? useStateManager : urlStateManager; } +// Block / Problem type helper class +export class TypesFilterData { + #blocks = new Set(); + + #problems = new Set(); + + #typeToList: FromStringFn; + + #listToType: ToStringFn; + + // Constructs a TypesFilterData from a string as generated from toString(). + constructor( + value: string | null, + typeToList: FromStringFn, + listToType: ToStringFn, + ) { + this.#typeToList = typeToList; + this.#listToType = listToType; + + const [blocks, problems] = (value || '').split('|'); + this.union({ blocks, problems }); + } + + // Serialize the TypesFilterData to a string, or undefined if isEmpty(). + toString(): string | undefined { + if (this.isEmpty()) { + return undefined; + } + return [ + this.#listToType([...this.blocks]), + this.#listToType([...this.problems]), + ].join('|'); + } + + // 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 = this.#typeToList(blocks) || []; + } else { + newBlocks = [...blocks]; + } + this.#blocks = new Set([...this.#blocks, ...newBlocks]); + + let newProblems: string[]; + if (!problems) { + newProblems = []; + } else if (typeof problems === 'string') { + newProblems = this.#typeToList(problems) || []; + } else { + newProblems = [...problems]; + } + this.#problems = new Set([...this.#problems, ...newProblems]); + } +} + 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; @@ -96,8 +173,33 @@ export const SearchContextProvider: React.FC<{ skipUrlUpdate, ); - const [blockTypesFilter, setBlockTypesFilter] = React.useState([]); - const [problemTypesFilter, setProblemTypesFilter] = React.useState([]); + // Block + problem types use alphanumeric plus a few other characters. + // E.g ?type=html,video|multiplechoiceresponse,choiceresponse + const sanitizeType = (value: string | null | undefined): string | undefined => ( + (value && /^[a-z0-9._-]+$/.test(value)) + ? value + : undefined + ); + const [typeToList, listToType] = useListHelpers({ + toString: sanitizeType, + fromString: sanitizeType, + separator: ',', + }); + const stringToTypesFilter = (value: string | null): TypesFilterData | undefined => ( + new TypesFilterData(value, typeToList, listToType) + ); + const typesFilterToString = (value: TypesFilterData | undefined): string | undefined => ( + value ? value.toString() : undefined + ); + const [typesFilter, setTypesFilter] = useStateOrUrlSearchParam( + new TypesFilterData('', typeToList, listToType), + 'types', + stringToTypesFilter, + typesFilterToString, + // Returns e.g 'problem,text|multiplechoice,checkbox' + skipUrlUpdate, + ); + // Tags can be almost any string value, except our separator (|) // TODO how to sanitize tags? // E.g ?tags=Skills+>+Abilities|Skills+>+Knowledge @@ -153,16 +255,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(''); } @@ -177,8 +277,8 @@ export const SearchContextProvider: React.FC<{ indexName, extraFilter, searchKeywords, - blockTypesFilter, - problemTypesFilter, + blockTypesFilter: [...typesFilter.blocks], + problemTypesFilter: [...typesFilter.problems], tagsFilter, sort, skipBlockTypeFetch, @@ -190,10 +290,8 @@ export const SearchContextProvider: React.FC<{ indexName, searchKeywords, setSearchKeywords, - blockTypesFilter, - setBlockTypesFilter, - problemTypesFilter, - setProblemTypesFilter, + typesFilter, + setTypesFilter, tagsFilter, setTagsFilter, extraFilter, From a3ee610b5f761ac28c0e9c721a738ee0193fd678 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Sat, 21 Dec 2024 00:27:36 +1030 Subject: [PATCH 08/17] docs: adds more comments to the library authoring routes --- src/library-authoring/routes.ts | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/src/library-authoring/routes.ts b/src/library-authoring/routes.ts index 59d0811790..383a1d6c78 100644 --- a/src/library-authoring/routes.ts +++ b/src/library-authoring/routes.ts @@ -15,11 +15,18 @@ import { 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', - COLLECTION: '/collection/:collectionId/: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 { @@ -66,7 +73,7 @@ export const useLibraryRoutes = (): LibraryRoutesData => { }; let route; - // contentType overrides the current route + // Providing contentType overrides the current route so we can change tabs. if (contentType === ContentType.components) { route = ROUTES.COMPONENTS; } else if (contentType === ContentType.collections) { @@ -74,25 +81,33 @@ export const useLibraryRoutes = (): LibraryRoutesData => { } else if (contentType === ContentType.home) { route = ROUTES.HOME; } else if (insideCollections) { + // We're inside the Collections tab, route = ( (collectionId && collectionId === params.collectionId) - // Open the previously-selected collection + // now open the previously-selected collection, ? ROUTES.COLLECTION - // Otherwise just preview the collection, if specified + // 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) - // Open the previously-selected collection + // now open the previously-selected collection ? ROUTES.COLLECTION - // Otherwise just preview the collection, if specified + // or stay there to list all content, or optionally select a collection. : ROUTES.HOME ); } From 505402074622814f21dc0ce788ab50e9d8324439 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Mon, 30 Dec 2024 11:48:45 +1030 Subject: [PATCH 09/17] fix: clicking "Library Info" navigates to the Library URL clearing out the selected component/collection. --- src/library-authoring/LibraryAuthoringPage.tsx | 2 +- src/library-authoring/routes.ts | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/library-authoring/LibraryAuthoringPage.tsx b/src/library-authoring/LibraryAuthoringPage.tsx index 4e291c5f46..23552c8063 100644 --- a/src/library-authoring/LibraryAuthoringPage.tsx +++ b/src/library-authoring/LibraryAuthoringPage.tsx @@ -66,7 +66,7 @@ const HeaderActions = () => { if (!componentPickerMode) { // Reset URL to library home - navigateTo(); + navigateTo({ componentId: '', collectionId: '' }); } }, [navigateTo, sidebarComponentInfo, closeLibrarySidebar, openLibrarySidebar]); diff --git a/src/library-authoring/routes.ts b/src/library-authoring/routes.ts index 383a1d6c78..fa96c7be01 100644 --- a/src/library-authoring/routes.ts +++ b/src/library-authoring/routes.ts @@ -67,9 +67,9 @@ export const useLibraryRoutes = (): LibraryRoutesData => { }: NavigateToData = {}) => { const routeParams = { ...params, - componentId, - // Overwrite the current collectionId param only if one is specified - ...(collectionId && { collectionId }), + // Overwrite the current componentId/collectionId params if provided + ...((componentId !== undefined) && { componentId }), + ...((collectionId !== undefined) && { collectionId }), }; let route; From 88dfb0e676b38cbd915a9773ce6a3145bcf026fb Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Mon, 30 Dec 2024 12:03:41 +1030 Subject: [PATCH 10/17] fix: use sidebarAction instead of managing separate state when editing component collections. --- .../component-info/ManageCollections.tsx | 45 +++++++------------ 1 file changed, 15 insertions(+), 30 deletions(-) diff --git a/src/library-authoring/component-info/ManageCollections.tsx b/src/library-authoring/component-info/ManageCollections.tsx index 581e427ccb..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, @@ -191,38 +191,23 @@ const ComponentCollections = ({ collections, onManageClick }: { }; const ManageCollections = ({ usageKey, collections }: ManageCollectionsProps) => { - const { sidebarAction, resetSidebarAction } = useSidebarContext(); - const jumpToCollections = sidebarAction === SidebarActions.JumpToAddCollections; - const [editing, setEditing] = useState(jumpToCollections); + const { sidebarAction, resetSidebarAction, setSidebarAction } = useSidebarContext(); const collectionNames = collections.map((collection) => collection.title); - useEffect(() => { - if (jumpToCollections) { - setEditing(true); - } - }, [jumpToCollections]); - - useEffect(() => { - // This is required to redo actions. - if (!editing) { - resetSidebarAction(); - } - }, [editing]); - - if (editing) { - return ( - setEditing(false)} - /> - ); - } return ( - setEditing(true)} - /> + sidebarAction === SidebarActions.JumpToAddCollections + ? ( + resetSidebarAction()} + /> + ) : ( + setSidebarAction(SidebarActions.JumpToAddCollections)} + /> + ) ); }; From 9ae67f51ee73563852485f8a18a44c5d6dc42e75 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Mon, 30 Dec 2024 12:09:21 +1030 Subject: [PATCH 11/17] refactor: use getActiveKey to initialize the tab state on library authoring page. --- .../LibraryAuthoringPage.test.tsx | 10 +++++----- src/library-authoring/LibraryAuthoringPage.tsx | 15 ++++----------- 2 files changed, 9 insertions(+), 16 deletions(-) diff --git a/src/library-authoring/LibraryAuthoringPage.test.tsx b/src/library-authoring/LibraryAuthoringPage.test.tsx index 66d72800c3..9eaf0167d8 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 diff --git a/src/library-authoring/LibraryAuthoringPage.tsx b/src/library-authoring/LibraryAuthoringPage.tsx index 23552c8063..13687987ed 100644 --- a/src/library-authoring/LibraryAuthoringPage.tsx +++ b/src/library-authoring/LibraryAuthoringPage.tsx @@ -147,8 +147,10 @@ const LibraryAuthoringPage = ({ returnToLibrarySelection }: LibraryAuthoringPage const { insideCollections, insideComponents, navigateTo } = useLibraryRoutes(); // The activeKey determines the currently selected tab. - const [activeKey, setActiveKey] = useState(ContentType.home); const getActiveKey = () => { + if (componentPickerMode) { + return ContentType.home; + } if (insideCollections) { return ContentType.collections; } @@ -157,16 +159,7 @@ const LibraryAuthoringPage = ({ returnToLibrarySelection }: LibraryAuthoringPage } return ContentType.home; }; - - useEffect(() => { - const contentType = getActiveKey(); - - if (componentPickerMode) { - setActiveKey(ContentType.home); - } else { - setActiveKey(contentType); - } - }, []); + const [activeKey, setActiveKey] = useState(getActiveKey); useEffect(() => { if (!componentPickerMode) { From 2f28e3bd59fd00779575e0894d8ef93fbeef05d8 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Mon, 30 Dec 2024 20:02:24 +1030 Subject: [PATCH 12/17] refactor: allow Type or Type[] values in useStateUrlSearchParams using typescript function overloading. Multiple values in the search params are provided by UrlSearchParams.getAll(). Also: * removes the useListHelpers hook added in previous commits -- it is no longer needed. * moves the useStateOrUrlSearchParam hook and TypesFilterData class to a separate search-manager/hooks.ts * sanitizes tag search parameters using oel RESERVED_TAG_CHARS * uses getBlockType to sanitize usage key search parameters --- src/hooks.ts | 103 +++++++--------- src/search-manager/SearchManager.ts | 176 +++++----------------------- src/search-manager/hooks.ts | 133 +++++++++++++++++++++ 3 files changed, 205 insertions(+), 207 deletions(-) create mode 100644 src/search-manager/hooks.ts diff --git a/src/hooks.ts b/src/hooks.ts index 7a3341d862..d121ddda8f 100644 --- a/src/hooks.ts +++ b/src/hooks.ts @@ -86,40 +86,54 @@ export const useLoadOnScroll = ( }; /** - * Types used by the useListHelpers and useStateWithUrlSearchParam hooks. + * 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 + * @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>] { +): [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 (like in + // 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, but our test suite uses MemoryRouter, and that - // router doesn't store URL search params, cf + // 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() @@ -128,25 +142,41 @@ export function useStateWithUrlSearchParam( 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; - const returnValue: Type = fromString(searchParams.get(paramName)) ?? defaultValue; // Update the url search parameter using: type ReturnSetterParams = ( // a Type value - value?: Type + value?: Type | Type[] // or a function that returns a Type from the previous returnValue - | ((value: Type) => Type) + | ((value: Type | Type[]) => Type | Type[]) ) => void; - const returnSetter: Dispatch> = useCallback((value) => { - setSearchParams((/* prev */) => { + const returnSetter: Dispatch> = useCallback((value) => { + setSearchParams((/* previous -- see STATE WORKAROUND above */) => { const useValue = value instanceof Function ? value(returnValue) : value; - const paramValue = toString(useValue); + 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 the provided value was invalid (toString returned undefined) - // or the same as the defaultValue, remove it from the search params. 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); } @@ -160,48 +190,3 @@ export function useStateWithUrlSearchParam( // Return the computed value and wrapped set state function return [returnValue, returnSetter]; } - -/** - * Helper hook for useStateWithUrlSearchParam. - * - * useListHelpers provides toString and fromString handlers that can: - * - split/join a list of values using a separator string, and - * - validate each value using the provided functions, omitting any invalid values. - * - * @param fromString - * Serialize a string to a Type, or undefined if not valid. - * @param toString - * Deserialize a Type to a string. - * @param separator : string to use when splitting/joining the types. - * Defaults value is ','. - */ -export function useListHelpers({ - fromString, - toString, - separator = ',', -}: { - fromString: FromStringFn, - toString: ToStringFn, - separator?: string; -}): [ FromStringFn, ToStringFn ] { - const isType = (item: Type | undefined): item is Type => item !== undefined; - - // Split the given string with separator, - // and convert the parts to a list of Types, omiting any invalid Types. - const fromStringToList : FromStringFn = (value: string) => ( - value - ? value.split(separator).map(fromString).filter(isType) - : [] - ); - // Convert an array of Types to strings and join with separator. - // Returns undefined if the given list contains no valid Types. - const fromListToString : ToStringFn = (value: Type[]) => { - const stringValues = value.map(toString).filter((val) => val !== undefined); - return ( - stringValues && stringValues.length - ? stringValues.join(separator) - : undefined - ); - }; - return [fromStringToList, fromListToString]; -} diff --git a/src/search-manager/SearchManager.ts b/src/search-manager/SearchManager.ts index a03907d88a..ac9d196675 100644 --- a/src/search-manager/SearchManager.ts +++ b/src/search-manager/SearchManager.ts @@ -11,115 +11,9 @@ import { union } from 'lodash'; import { CollectionHit, ContentHit, SearchSortOption, forceArray, } from './data/api'; +import { TypesFilterData, useStateOrUrlSearchParam } from './hooks'; import { useContentSearchConnection, useContentSearchResults } from './data/apiHooks'; -import { - type FromStringFn, - type ToStringFn, - useListHelpers, - useStateWithUrlSearchParam, -} from '../hooks'; - -/** - * Typed hook that returns useState if skipUrlUpdate, - * or useStateWithUrlSearchParam if it's not. - * - * Provided here to reduce some code overhead in SearchManager. - */ -function useStateOrUrlSearchParam( - defaultValue: Type, - paramName: string, - fromString: FromStringFn, - toString: ToStringFn, - skipUrlUpdate?: boolean, -): [value: Type, setter: React.Dispatch>] { - const useStateManager = React.useState(defaultValue); - const urlStateManager = useStateWithUrlSearchParam( - defaultValue, - paramName, - fromString, - toString, - ); - return skipUrlUpdate ? useStateManager : urlStateManager; -} - -// Block / Problem type helper class -export class TypesFilterData { - #blocks = new Set(); - - #problems = new Set(); - - #typeToList: FromStringFn; - - #listToType: ToStringFn; - - // Constructs a TypesFilterData from a string as generated from toString(). - constructor( - value: string | null, - typeToList: FromStringFn, - listToType: ToStringFn, - ) { - this.#typeToList = typeToList; - this.#listToType = listToType; - - const [blocks, problems] = (value || '').split('|'); - this.union({ blocks, problems }); - } - - // Serialize the TypesFilterData to a string, or undefined if isEmpty(). - toString(): string | undefined { - if (this.isEmpty()) { - return undefined; - } - return [ - this.#listToType([...this.blocks]), - this.#listToType([...this.problems]), - ].join('|'); - } - - // 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 = this.#typeToList(blocks) || []; - } else { - newBlocks = [...blocks]; - } - this.#blocks = new Set([...this.#blocks, ...newBlocks]); - - let newProblems: string[]; - if (!problems) { - newProblems = []; - } else if (typeof problems === 'string') { - newProblems = this.#typeToList(problems) || []; - } else { - newProblems = [...problems]; - } - this.#problems = new Set([...this.#problems, ...newProblems]); - } -} +import { getBlockType } from '../generic/key-utils'; export interface SearchContextData { client?: MeiliSearch; @@ -174,60 +68,46 @@ export const SearchContextProvider: React.FC<{ ); // Block + problem types use alphanumeric plus a few other characters. - // E.g ?type=html,video|multiplechoiceresponse,choiceresponse - const sanitizeType = (value: string | null | undefined): string | undefined => ( - (value && /^[a-z0-9._-]+$/.test(value)) - ? value - : undefined - ); - const [typeToList, listToType] = useListHelpers({ - toString: sanitizeType, - fromString: sanitizeType, - separator: ',', - }); - const stringToTypesFilter = (value: string | null): TypesFilterData | undefined => ( - new TypesFilterData(value, typeToList, listToType) - ); - const typesFilterToString = (value: TypesFilterData | undefined): string | undefined => ( - value ? value.toString() : undefined - ); + // E.g ?type=html&type=video&type=p.multiplechoiceresponse const [typesFilter, setTypesFilter] = useStateOrUrlSearchParam( - new TypesFilterData('', typeToList, listToType), - 'types', - stringToTypesFilter, - typesFilterToString, - // Returns e.g 'problem,text|multiplechoice,checkbox' + new TypesFilterData(), + 'type', + (value: string | null) => new TypesFilterData(value), + (value: TypesFilterData | undefined) => (value ? value.toString() : undefined), skipUrlUpdate, ); - // Tags can be almost any string value, except our separator (|) - // TODO how to sanitize tags? - // E.g ?tags=Skills+>+Abilities|Skills+>+Knowledge + // 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 && /^[^|]+$/.test(value)) - ? value - : undefined + (value && /^[^\t;]+$/.test(value)) ? value : undefined ); - const [tagToList, listToTag] = useListHelpers({ - toString: sanitizeTag, - fromString: sanitizeTag, - separator: '|', - }); - const [tagsFilter, setTagsFilter] = useStateOrUrlSearchParam( + const [tagsFilter, setTagsFilter] = useStateOrUrlSearchParam( [], - 'tags', - tagToList, - listToTag, + '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', - // TODO should sanitize usageKeys too. - (value: string) => value, - (value: string) => value, + sanitizeUsageKey, + sanitizeUsageKey, skipUrlUpdate, ); 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]); + } +} From b4c70ca14fa95b2823507958e41fa28ed5ccd57a Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Wed, 1 Jan 2025 15:31:51 +1030 Subject: [PATCH 13/17] fix: disable filter by type on collections tab Re-implements the functionality added by https://github.com/openedx/frontend-app-authoring/pull/1576 which was broken when storing types in search params. Had to use a different approach to avoid "insecure operation" errors from React. Also adds tests. --- .../LibraryAuthoringPage.test.tsx | 28 +++++++++++++++++++ .../LibraryAuthoringPage.tsx | 7 ++++- src/search-manager/FilterByBlockType.tsx | 10 +------ src/search-manager/SearchManager.ts | 13 +++++++-- src/search-manager/index.ts | 1 + 5 files changed, 46 insertions(+), 13 deletions(-) diff --git a/src/library-authoring/LibraryAuthoringPage.test.tsx b/src/library-authoring/LibraryAuthoringPage.test.tsx index 9eaf0167d8..d6e7f1c300 100644 --- a/src/library-authoring/LibraryAuthoringPage.test.tsx +++ b/src/library-authoring/LibraryAuthoringPage.test.tsx @@ -702,6 +702,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 13687987ed..b952310f5d 100644 --- a/src/library-authoring/LibraryAuthoringPage.tsx +++ b/src/library-authoring/LibraryAuthoringPage.tsx @@ -30,6 +30,7 @@ import { SearchContextProvider, SearchKeywordsField, SearchSortWidget, + TypesFilterData, } from '../search-manager'; import LibraryContent from './LibraryContent'; import { LibrarySidebar } from './library-sidebar'; @@ -220,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 (
@@ -239,6 +243,7 @@ const LibraryAuthoringPage = ({ returnToLibrarySelection }: LibraryAuthoringPage } @@ -260,7 +265,7 @@ const LibraryAuthoringPage = ({ returnToLibrarySelection }: LibraryAuthoringPage - + {!insideCollections && } diff --git a/src/search-manager/FilterByBlockType.tsx b/src/search-manager/FilterByBlockType.tsx index 16c0b1de1d..0b5160d37d 100644 --- a/src/search-manager/FilterByBlockType.tsx +++ b/src/search-manager/FilterByBlockType.tsx @@ -191,16 +191,12 @@ 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, typesFilter, @@ -248,10 +244,6 @@ const FilterByBlockType: React.FC = ({ disabled = false blockType => ({ label: }), ); - if (disabled) { - return null; - } - return ( (undefined); 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 }) => { // Search parameters can be set via the query string // E.g. ?q=draft+text @@ -69,13 +74,15 @@ export const SearchContextProvider: React.FC<{ // Block + problem types use alphanumeric plus a few other characters. // E.g ?type=html&type=video&type=p.multiplechoiceresponse - const [typesFilter, setTypesFilter] = useStateOrUrlSearchParam( + 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. 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'; From ced360e4f2f162b745fc9c59ca1e0794ffc61e5c Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Wed, 1 Jan 2025 15:35:45 +1030 Subject: [PATCH 14/17] test: fixes broken tests need to wrap the ManageCollections element in SidebarProvider. --- .../component-info/ManageCollections.test.tsx | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) 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} + ), }); From d060113eba9fd35d8f550732fc3493a61a29d96f Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Wed, 1 Jan 2025 16:01:27 +1030 Subject: [PATCH 15/17] test: clear search filters in between tests Since search filters are now stored in the URL, they can leak between tests. --- src/search-modal/SearchUI.test.tsx | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/search-modal/SearchUI.test.tsx b/src/search-modal/SearchUI.test.tsx index 5e100ededa..fb82694135 100644 --- a/src/search-modal/SearchUI.test.tsx +++ b/src/search-modal/SearchUI.test.tsx @@ -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: @@ -409,8 +419,8 @@ 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(2, searchEndpoint, 'post'); }); // Because we're mocking the results, there's no actual changes to the mock results, From 4c1855d2ada2f5873f062f9284423f721cfbfdf9 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Wed, 1 Jan 2025 18:03:46 +1030 Subject: [PATCH 16/17] test: split long-running "can filter by capa problem type" test into two --- .../LibraryAuthoringPage.test.tsx | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/src/library-authoring/LibraryAuthoringPage.test.tsx b/src/library-authoring/LibraryAuthoringPage.test.tsx index d6e7f1c300..2b55375fd2 100644 --- a/src/library-authoring/LibraryAuthoringPage.test.tsx +++ b/src/library-authoring/LibraryAuthoringPage.test.tsx @@ -501,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]; @@ -524,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(), }); From 39fd7450fd9c227e09dc1a3e6aea23c22a7572b1 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Sat, 11 Jan 2025 00:12:18 +1030 Subject: [PATCH 17/17] test: adds tests for library authoring routes.ts --- src/library-authoring/routes.test.tsx | 289 ++++++++++++++++++++++++++ 1 file changed, 289 insertions(+) create mode 100644 src/library-authoring/routes.test.tsx 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: '', + }); + }, + ); +});