Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: shareable URLs for library components and searches [FC-0076] #1575

Merged
merged 19 commits into from
Jan 10, 2025
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
aa25827
fix: use generatePath in testUtils
pomegranited Dec 18, 2024
a4728e1
fix: test uses contentId=undefined, so made this param optional in th…
pomegranited Dec 19, 2024
a6465cb
Merge remote-tracking branch 'origin/master' into jill/fal-3984-shara…
pomegranited Dec 19, 2024
ce4c27e
feat: adds sharable URLs for library components/collections
pomegranited Dec 12, 2024
fc2f467
feat: store search keywords in query string
pomegranited Dec 19, 2024
f6b46c4
feat: store sidebar tab and additional action in query string
pomegranited Dec 19, 2024
37370d2
feat: store selected tags in the URL search string
pomegranited Dec 20, 2024
1736942
refactor: use one typesFilter state in SearchManager
pomegranited Dec 20, 2024
a3ee610
docs: adds more comments to the library authoring routes
pomegranited Dec 20, 2024
9d3e049
Merge remote-tracking branch 'origin/master' into jill/fal-3984-shara…
pomegranited Dec 30, 2024
5054020
fix: clicking "Library Info" navigates to the Library URL
pomegranited Dec 30, 2024
88dfb0e
fix: use sidebarAction instead of managing separate state
pomegranited Dec 30, 2024
9ae67f5
refactor: use getActiveKey to initialize the tab state
pomegranited Dec 30, 2024
2f28e3b
refactor: allow Type or Type[] values in useStateUrlSearchParams
pomegranited Dec 30, 2024
b4c70ca
fix: disable filter by type on collections tab
pomegranited Jan 1, 2025
ced360e
test: fixes broken tests
pomegranited Jan 1, 2025
d060113
test: clear search filters in between tests
pomegranited Jan 1, 2025
4c1855d
test: split long-running "can filter by capa problem type" test into two
pomegranited Jan 1, 2025
39fd745
test: adds tests for library authoring routes.ts
pomegranited Jan 10, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/content-tags-drawer/ContentTagsDrawer.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
117 changes: 115 additions & 2 deletions src/hooks.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,13 @@
import { useEffect, useState } from 'react';
import {
type Dispatch,
type SetStateAction,
useCallback,
useEffect,
useRef,
useState,
} from 'react';
import { history } from '@edx/frontend-platform';
import { useLocation } from 'react-router-dom';
import { useLocation, useSearchParams } from 'react-router-dom';

export const useScrollToHashElement = ({ isLoading }: { isLoading: boolean }) => {
const [elementWithHash, setElementWithHash] = useState<string | null>(null);
Expand Down Expand Up @@ -77,3 +84,109 @@ export const useLoadOnScroll = (
return () => { };
}, [hasNextPage, isFetchingNextPage, fetchNextPage]);
};

/**
* Types used by the useStateWithUrlSearchParam hook.
*/
export type FromStringFn<Type> = (value: string | null) => Type | undefined;
export type ToStringFn<Type> = (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<Type> values.
*
* @param defaultValue: Type | Type[]
* Returned when no valid value is found in the url search parameter.
* If an Array Type is used, then an Array Type of state values will be maintained.
* @param paramName: name of the url search parameter to store this value in.
* @param fromString: returns the Type equivalent of the given string value,
* or undefined if the value is invalid.
* @param toString: returns the string equivalent of the given Type value.
* Return defaultValue to clear the url search paramName.
*/
export function useStateWithUrlSearchParam<Type>(
defaultValue: Type[],
paramName: string,
fromString: FromStringFn<Type>,
toString: ToStringFn<Type>,
): [value: Type[], setter: Dispatch<SetStateAction<Type[]>>];
export function useStateWithUrlSearchParam<Type>(
defaultValue: Type,
paramName: string,
fromString: FromStringFn<Type>,
toString: ToStringFn<Type>,
): [value: Type, setter: Dispatch<SetStateAction<Type>>];
export function useStateWithUrlSearchParam<Type>(
defaultValue: Type | Type[],
paramName: string,
fromString: FromStringFn<Type>,
toString: ToStringFn<Type>,
): [value: Type | Type[], setter: Dispatch<SetStateAction<Type | Type[]>>] {
// STATE WORKAROUND:
// If we use this hook to control multiple state parameters on the same
// page, we can run into state update issues. Because our state variables
// are actually stored in setSearchParams, and not in separate variables like
// useState would do, the searchParams "previous" state may not be updated
// for sequential calls to returnSetter in the same render loop (as we do in
// SearchManager's clearFilters).
//
// One workaround could be to use window.location.search as the "previous"
// value when returnSetter constructs the new URLSearchParams. This works
// fine with BrowserRouter, however our test suite uses MemoryRouter, and
// that router doesn't store URL search params, cf
// https://github.com/remix-run/react-router/issues/9757
//
// So instead, we maintain a reference to the current useLocation()
// object, and use its search params as the "previous" value when
// initializing URLSearchParams.
const location = useLocation();
const locationRef = useRef(location);
const [searchParams, setSearchParams] = useSearchParams();
const paramValues = searchParams.getAll(paramName);

const returnValue: Type | Type[] = (
defaultValue instanceof Array
? (paramValues.map(fromString).filter((v) => v !== undefined)) as Type[]
: fromString(paramValues[0])
) ?? defaultValue;

// Update the url search parameter using:
type ReturnSetterParams = (
// a Type value
value?: Type | Type[]
// or a function that returns a Type from the previous returnValue
| ((value: Type | Type[]) => Type | Type[])
) => void;
const returnSetter: Dispatch<SetStateAction<Type | Type[]>> = useCallback<ReturnSetterParams>((value) => {
setSearchParams((/* previous -- see STATE WORKAROUND above */) => {
const useValue = value instanceof Function ? value(returnValue) : value;
const paramValue: string | string[] | undefined = (
useValue instanceof Array
? useValue.map(toString).filter((v) => v !== undefined) as string[]
: toString(useValue)
);

const newSearchParams = new URLSearchParams(locationRef.current.search);
if (paramValue === undefined || paramValue === defaultValue) {
// If the provided value was invalid (toString returned undefined) or
// the same as the defaultValue, remove it from the search params.
newSearchParams.delete(paramName);
} else if (paramValue instanceof Array) {
// Replace paramName with the new list of values.
newSearchParams.delete(paramName);
paramValue.forEach((v) => v && newSearchParams.append(paramName, v));
} else {
// Otherwise, just set the new (single) value.
newSearchParams.set(paramName, paramValue);
}

// Update locationRef
locationRef.current.search = newSearchParams.toString();

return newSearchParams;
}, { replace: true });
}, [returnValue, setSearchParams]);

// Return the computed value and wrapped set state function
return [returnValue, returnSetter];
}
65 changes: 48 additions & 17 deletions src/library-authoring/LibraryAuthoringPage.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ describe('<LibraryAuthoringPage />', () => {
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 });
Expand All @@ -151,7 +151,7 @@ describe('<LibraryAuthoringPage />', () => {
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);
Expand Down Expand Up @@ -196,15 +196,15 @@ describe('<LibraryAuthoringPage />', () => {
// 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
Expand Down Expand Up @@ -431,27 +431,23 @@ describe('<LibraryAuthoringPage />', () => {
// 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 () => {
Expand Down Expand Up @@ -505,6 +501,15 @@ describe('<LibraryAuthoringPage />', () => {
// 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];
Expand All @@ -528,15 +533,13 @@ describe('<LibraryAuthoringPage />', () => {
});

// 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(),
});
Expand Down Expand Up @@ -706,6 +709,34 @@ describe('<LibraryAuthoringPage />', () => {
});
});

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, {
Expand Down
Loading
Loading