UI fixes for Sort Library Component [FC-0059] (openedx#1222)

fix: use "Recently Modified" as the default sort option

When search keyword(s) are entered, use "Most Relevant" as default.

Also

* Hides "Most Relevant" option if no keyword is entered.
* Re-orders the sort menu options
* Ensures the default sort option is not stored in the query string
* Shows the selected sort option on the drop-down toggle button
* Shows "Sort By" as a header inside the drop-down menu

Author: pomegranited

src/library-authoring/LibraryAuthoringPage.test.tsx

@@ -189,33 +189,33 @@ describe('<LibraryAuthoringPage />', () => {
     axiosMock.onGet(getContentLibraryApiUrl(libraryData.id)).reply(200, libraryData);
 
     const {
-      getByRole, getByText, queryByText, findByText, findAllByText,
+      getByRole, getAllByText, getByText, queryByText, findByText, findAllByText,
     } = render(<RootWrapper />);
 
-    // Ensure the search endpoint is called:
-    // Call 1: To fetch searchable/filterable/sortable library data
-    // Call 2: To fetch the recently modified components only
-    await waitFor(() => { expect(fetchMock).toHaveFetchedTimes(2, searchEndpoint, 'post'); });
+    await waitFor(() => { expect(fetchMock).toHaveFetchedTimes(1, searchEndpoint, 'post'); });
 
     expect(await findByText('Content library')).toBeInTheDocument();
     expect((await findAllByText(libraryData.title))[0]).toBeInTheDocument();
 
     expect(queryByText('You have not added any content to this library yet.')).not.toBeInTheDocument();
 
-    expect(getByText('Recently Modified')).toBeInTheDocument();
+    // "Recently Modified" header + sort shown
+    expect(getAllByText('Recently Modified').length).toEqual(2);
     expect(getByText('Collections (0)')).toBeInTheDocument();
     expect(getByText('Components (6)')).toBeInTheDocument();
     expect((await findAllByText('Test HTML Block'))[0]).toBeInTheDocument();
 
     // Navigate to the components tab
     fireEvent.click(getByRole('tab', { name: 'Components' }));
-    expect(queryByText('Recently Modified')).not.toBeInTheDocument();
+    // "Recently Modified" default sort shown
+    expect(getAllByText('Recently Modified').length).toEqual(1);
     expect(queryByText('Collections (0)')).not.toBeInTheDocument();
     expect(queryByText('Components (6)')).not.toBeInTheDocument();
 
     // Navigate to the collections tab
     fireEvent.click(getByRole('tab', { name: 'Collections' }));
-    expect(queryByText('Recently Modified')).not.toBeInTheDocument();
+    // "Recently Modified" default sort shown
+    expect(getAllByText('Recently Modified').length).toEqual(1);
     expect(queryByText('Collections (0)')).not.toBeInTheDocument();
     expect(queryByText('Components (6)')).not.toBeInTheDocument();
     expect(queryByText('There are 6 components in this library')).not.toBeInTheDocument();
@@ -224,7 +224,8 @@ describe('<LibraryAuthoringPage />', () => {
     // Go back to Home tab
     // This step is necessary to avoid the url change leak to other tests
     fireEvent.click(getByRole('tab', { name: 'Home' }));
-    expect(getByText('Recently Modified')).toBeInTheDocument();
+    // "Recently Modified" header + sort shown
+    expect(getAllByText('Recently Modified').length).toEqual(2);
     expect(getByText('Collections (0)')).toBeInTheDocument();
     expect(getByText('Components (6)')).toBeInTheDocument();
   });
@@ -239,10 +240,7 @@ describe('<LibraryAuthoringPage />', () => {
     expect(await findByText('Content library')).toBeInTheDocument();
     expect((await findAllByText(libraryData.title))[0]).toBeInTheDocument();
 
-    // Ensure the search endpoint is called:
-    // Call 1: To fetch searchable/filterable/sortable library data
-    // Call 2: To fetch the recently modified components only
-    await waitFor(() => { expect(fetchMock).toHaveFetchedTimes(2, searchEndpoint, 'post'); });
+    await waitFor(() => { expect(fetchMock).toHaveFetchedTimes(1, searchEndpoint, 'post'); });
 
     expect(getByText('You have not added any content to this library yet.')).toBeInTheDocument();
   });
@@ -304,16 +302,13 @@ describe('<LibraryAuthoringPage />', () => {
     expect(await findByText('Content library')).toBeInTheDocument();
     expect((await findAllByText(libraryData.title))[0]).toBeInTheDocument();
 
-    // Ensure the search endpoint is called:
-    // Call 1: To fetch searchable/filterable/sortable library data
-    // Call 2: To fetch the recently modified components only
-    await waitFor(() => { expect(fetchMock).toHaveFetchedTimes(2, searchEndpoint, 'post'); });
+    await waitFor(() => { expect(fetchMock).toHaveFetchedTimes(1, searchEndpoint, 'post'); });
 
     fireEvent.change(getByRole('searchbox'), { target: { value: 'noresults' } });
 
     // Ensure the search endpoint is called again, only once more since the recently modified call
     // should not be impacted by the search
-    await waitFor(() => { expect(fetchMock).toHaveFetchedTimes(3, searchEndpoint, 'post'); });
+    await waitFor(() => { expect(fetchMock).toHaveFetchedTimes(2, searchEndpoint, 'post'); });
 
     expect(getByText('No matching components found in this library.')).toBeInTheDocument();
 
@@ -396,15 +391,13 @@ describe('<LibraryAuthoringPage />', () => {
       getByRole, getByText, queryByText, getAllByText, findAllByText,
     } = render(<RootWrapper />);
 
-    // Ensure the search endpoint is called:
-    // Call 1: To fetch searchable/filterable/sortable library data
-    // Call 2: To fetch the recently modified components only
-    await waitFor(() => { expect(fetchMock).toHaveFetchedTimes(2, searchEndpoint, 'post'); });
+    await waitFor(() => { expect(fetchMock).toHaveFetchedTimes(1, searchEndpoint, 'post'); });
 
     expect(getByText('Content library')).toBeInTheDocument();
     expect((await findAllByText(libraryData.title))[0]).toBeInTheDocument();
 
-    await waitFor(() => { expect(getByText('Recently Modified')).toBeInTheDocument(); });
+    // "Recently Modified" header + sort shown
+    await waitFor(() => { expect(getAllByText('Recently Modified').length).toEqual(2); });
     expect(getByText('Collections (0)')).toBeInTheDocument();
     expect(getByText('Components (6)')).toBeInTheDocument();
     expect(getAllByText('Test HTML Block')[0]).toBeInTheDocument();
@@ -416,15 +409,17 @@ describe('<LibraryAuthoringPage />', () => {
 
     // Clicking on "View All" button should navigate to the Components tab
     fireEvent.click(getByText('View All'));
-    expect(queryByText('Recently Modified')).not.toBeInTheDocument();
+    // "Recently Modified" default sort shown
+    expect(getAllByText('Recently Modified').length).toEqual(1);
     expect(queryByText('Collections (0)')).not.toBeInTheDocument();
     expect(queryByText('Components (6)')).not.toBeInTheDocument();
     expect(getAllByText('Test HTML Block')[0]).toBeInTheDocument();
 
     // Go back to Home tab
     // This step is necessary to avoid the url change leak to other tests
     fireEvent.click(getByRole('tab', { name: 'Home' }));
-    expect(getByText('Recently Modified')).toBeInTheDocument();
+    // "Recently Modified" header + sort shown
+    expect(getAllByText('Recently Modified').length).toEqual(2);
     expect(getByText('Collections (0)')).toBeInTheDocument();
     expect(getByText('Components (6)')).toBeInTheDocument();
   });
@@ -438,15 +433,13 @@ describe('<LibraryAuthoringPage />', () => {
       getByText, queryByText, getAllByText, findAllByText,
     } = render(<RootWrapper />);
 
-    // Ensure the search endpoint is called:
-    // Call 1: To fetch searchable/filterable/sortable library data
-    // Call 2: To fetch the recently modified components only
-    await waitFor(() => { expect(fetchMock).toHaveFetchedTimes(2, searchEndpoint, 'post'); });
+    await waitFor(() => { expect(fetchMock).toHaveFetchedTimes(1, searchEndpoint, 'post'); });
 
     expect(getByText('Content library')).toBeInTheDocument();
     expect((await findAllByText(libraryData.title))[0]).toBeInTheDocument();
 
-    await waitFor(() => { expect(getByText('Recently Modified')).toBeInTheDocument(); });
+    // "Recently Modified" header + sort shown
+    await waitFor(() => { expect(getAllByText('Recently Modified').length).toEqual(2); });
     expect(getByText('Collections (0)')).toBeInTheDocument();
     expect(getByText('Components (2)')).toBeInTheDocument();
     expect(getAllByText('Test HTML Block')[0]).toBeInTheDocument();
@@ -463,35 +456,49 @@ describe('<LibraryAuthoringPage />', () => {
     fetchMock.post(searchEndpoint, returnEmptyResult, { overwriteRoutes: true });
 
     const {
-      findByTitle, getAllByText, getByText, getByTitle,
+      findByTitle, getAllByText, getByRole, getByTitle,
     } = render(<RootWrapper />);
 
     expect(await findByTitle('Sort search results')).toBeInTheDocument();
 
-    const testSortOption = (async (optionText, sortBy) => {
-      if (optionText) {
-        fireEvent.click(getByTitle('Sort search results'));
-        fireEvent.click(getByText(optionText));
-      }
+    const testSortOption = (async (optionText, sortBy, isDefault) => {
+      // Open the drop-down menu
+      fireEvent.click(getByTitle('Sort search results'));
+
+      // Click the option with the given text
+      // Since the sort drop-down also shows the selected sort
+      // option in its toggle button, we need to make sure we're
+      // clicking on the last one found.
+      const options = getAllByText(optionText);
+      expect(options.length).toBeGreaterThan(0);
+      fireEvent.click(options[options.length - 1]);
+
+      // Did the search happen with the expected sort option?
       const bodyText = sortBy ? `"sort":["${sortBy}"]` : '"sort":[]';
-      const searchText = sortBy ? `?sort=${encodeURIComponent(sortBy)}` : '';
       await waitFor(() => {
         expect(fetchMock).toHaveBeenLastCalledWith(searchEndpoint, {
           body: expect.stringContaining(bodyText),
           method: 'POST',
           headers: expect.anything(),
         });
       });
+
+      // Is the sort option stored in the query string?
+      const searchText = isDefault ? '' : `?sort=${encodeURIComponent(sortBy)}`;
       expect(window.location.search).toEqual(searchText);
+
+      // Is the selected sort option shown in the toggle button (if not default)
+      // as well as in the drop-down menu?
+      expect(getAllByText(optionText).length).toEqual(isDefault ? 1 : 2);
     });
 
-    await testSortOption('Title, A-Z', 'display_name:asc');
-    await testSortOption('Title, Z-A', 'display_name:desc');
-    await testSortOption('Newest', 'created:desc');
-    await testSortOption('Oldest', 'created:asc');
+    await testSortOption('Title, A-Z', 'display_name:asc', false);
+    await testSortOption('Title, Z-A', 'display_name:desc', false);
+    await testSortOption('Newest', 'created:desc', false);
+    await testSortOption('Oldest', 'created:asc', false);
 
     // Sorting by Recently Published also excludes unpublished components
-    await testSortOption('Recently Published', 'last_published:desc');
+    await testSortOption('Recently Published', 'last_published:desc', false);
     await waitFor(() => {
       expect(fetchMock).toHaveBeenLastCalledWith(searchEndpoint, {
         body: expect.stringContaining('last_published IS NOT NULL'),
@@ -500,8 +507,22 @@ describe('<LibraryAuthoringPage />', () => {
       });
     });
 
-    // Clearing filters clears the url search param and uses default sort
-    fireEvent.click(getAllByText('Clear Filters')[0]);
-    await testSortOption('', '');
+    // Re-selecting the previous sort option resets sort to default "Recently Modified"
+    await testSortOption('Recently Published', 'modified:desc', true);
+    expect(getAllByText('Recently Modified').length).toEqual(2);
+
+    // Enter a keyword into the search box
+    const searchBox = getByRole('searchbox');
+    fireEvent.change(searchBox, { target: { value: 'words to find' } });
+
+    // Default sort option changes to "Most Relevant"
+    expect(getAllByText('Most Relevant').length).toEqual(2);
+    await waitFor(() => {
+      expect(fetchMock).toHaveBeenLastCalledWith(searchEndpoint, {
+        body: expect.stringContaining('"sort":[]'),
+        method: 'POST',
+        headers: expect.anything(),
+      });
+    });
   });
 });

src/search-manager/SearchManager.ts

@@ -28,6 +28,7 @@ export interface SearchContextData {
   isFiltered: boolean;
   searchSortOrder: SearchSortOption;
   setSearchSortOrder: React.Dispatch<React.SetStateAction<SearchSortOption>>;
+  defaultSearchSortOrder: SearchSortOption;
   hits: ContentHit[];
   totalHits: number;
   isFetching: boolean;
@@ -65,14 +66,11 @@ function useStateWithUrlSearchParam<Type>(
     setSearchParams((prevParams) => {
       const paramValue: string = toString(value) ?? '';
       const newSearchParams = new URLSearchParams(prevParams);
-      if (paramValue) {
-        newSearchParams.set(paramName, paramValue);
-      } else {
-        // If no paramValue, remove it from the search params, so
-        // we don't get dangling parameter values like ?paramName=
-        // Another way to decide this would be to check value === defaultValue,
-        // and ensure that default values are never stored in the search string.
+      // 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 });
@@ -95,17 +93,18 @@ export const SearchContextProvider: React.FC<{
 
   // The search sort order can be set via the query string
   // E.g. ?sort=display_name:desc maps to SearchSortOption.TITLE_ZA.
-  const defaultSortOption = SearchSortOption.RELEVANCE;
+  // Default sort by Most Relevant if there's search keyword(s), else by Recently Modified.
+  const defaultSearchSortOrder = searchKeywords ? SearchSortOption.RELEVANCE : SearchSortOption.RECENTLY_MODIFIED;
   const [searchSortOrder, setSearchSortOrder] = useStateWithUrlSearchParam<SearchSortOption>(
-    defaultSortOption,
+    defaultSearchSortOrder,
     'sort',
     (value: string) => Object.values(SearchSortOption).find((enumValue) => value === enumValue),
     (value: SearchSortOption) => value.toString(),
   );
   // SearchSortOption.RELEVANCE is special, it means "no custom sorting", so we
   // send it to useContentSearchResults as an empty array.
   const searchSortOrderToUse = overrideSearchSortOrder ?? searchSortOrder;
-  const sort: SearchSortOption[] = (searchSortOrderToUse === defaultSortOption ? [] : [searchSortOrderToUse]);
+  const sort: SearchSortOption[] = (searchSortOrderToUse === SearchSortOption.RELEVANCE ? [] : [searchSortOrderToUse]);
   // Selecting SearchSortOption.RECENTLY_PUBLISHED also excludes unpublished components.
   if (searchSortOrderToUse === SearchSortOption.RECENTLY_PUBLISHED) {
     extraFilter.push('last_published IS NOT NULL');
@@ -114,13 +113,11 @@ export const SearchContextProvider: React.FC<{
   const canClearFilters = (
     blockTypesFilter.length > 0
     || tagsFilter.length > 0
-    || searchSortOrderToUse !== defaultSortOption
   );
   const isFiltered = canClearFilters || (searchKeywords !== '');
   const clearFilters = React.useCallback(() => {
     setBlockTypesFilter([]);
     setTagsFilter([]);
-    setSearchSortOrder(defaultSortOption);
   }, []);
 
   // Initialize a connection to Meilisearch:
@@ -160,6 +157,7 @@ export const SearchContextProvider: React.FC<{
       clearFilters,
       searchSortOrder,
       setSearchSortOrder,
+      defaultSearchSortOrder,
       closeSearchModal: props.closeSearchModal ?? (() => {}),
       hasError: hasConnectionError || result.isError,
       ...result,