CoursewareSearch - Show filters if needed only (#1287)

* fix: CoursewareSearch - Show filters if needed only * fix: Refactored CoursewareSearch filtering logic * chore: Fixed unit tests for CoursewareResultsFilter * chore: Added CoursewareSearch hooks coverage
openedx · Feb 9, 2024 · efd57c3 · efd57c3
1 parent a18ed8e
commit efd57c3
Show file tree

Hide file tree

Showing 9 changed files with 157 additions and 74 deletions.
diff --git a/src/course-home/courseware-search/CoursewareResultsFilter.jsx b/src/course-home/courseware-search/CoursewareResultsFilter.jsx
@@ -8,15 +8,10 @@ import messages from './messages';
 import { useCoursewareSearchParams } from './hooks';
 import { useModel } from '../../generic/model-store';
 
-const allFilterKey = 'all';
-const otherFilterKey = 'other';
-const allowedFilterKeys = {
-  [allFilterKey]: true,
-  text: true,
-  video: true,
-  sequence: true,
-  [otherFilterKey]: true,
-};
+const filterAll = 'all';
+const filterTypes = ['text', 'video', 'sequence'];
+const filterOther = 'other';
+const validFilters = [filterAll, ...filterTypes, filterOther];
 
 export const CoursewareSearchResultsFilter = ({ intl }) => {
   const { courseId } = useParams();
@@ -27,23 +22,38 @@ export const CoursewareSearchResultsFilter = ({ intl }) => {
 
   const { results: data = [] } = lastSearch;
 
-  const results = useMemo(() => data.reduce((acc, { type, ...rest }) => {
-    acc[allFilterKey] = [...(acc[allFilterKey] || []), { type: allFilterKey, ...rest }];
-    if (type === allFilterKey) { return acc; }
+  // If there's no data, we show an empty result.
+  if (!data.length) { return <CoursewareSearchResults />; }
+
+  const results = useMemo(() => {
+    // This reducer distributes the data into different groups to make it easy to
+    // use on the filters.
+    // All results are added to the "all" key and then to its proper group key as well.
+    const grouped = data.reduce((acc, { type, ...rest }) => {
+      const resultType = filterTypes.includes(type) ? type : filterOther;
+      acc[filterAll].push({ type: resultType, ...rest });
+      acc[resultType] = [...(acc[resultType] || []), { type: resultType, ...rest }];
+      return acc;
+    }, { [filterAll]: [] });
+
+    // This is just to format the output object with the expected tab order.
+    const output = {};
+    validFilters.forEach(key => { if (grouped[key]) { output[key] = grouped[key]; } });
+
+    return output;
+  }, [lastSearch]);
 
-    let targetKey = otherFilterKey;
-    if (allowedFilterKeys[type]) { targetKey = type; }
-    acc[targetKey] = [...(acc[targetKey] || []), { type: targetKey, ...rest }];
-    return acc;
-  }, {}), [data]);
+  const tabKeys = Object.keys(results);
+  // Filter has no use if it has only 2 tabs (The "all" tab and another one with the same items).
+  if (tabKeys.length < 3) { return <CoursewareSearchResults results={results[filterAll]} />; }
 
-  const filters = useMemo(() => Object.keys(allowedFilterKeys).map((key) => ({
+  const filters = useMemo(() => tabKeys.map((key) => ({
     key,
     label: intl.formatMessage(messages[`filter:${key}`]),
-    count: results[key]?.length || 0,
+    count: results[key].length,
   })), [results]);
 
-  const activeKey = allowedFilterKeys[filterKeyword] ? filterKeyword : allFilterKey;
+  const activeKey = validFilters.includes(filterKeyword) ? filterKeyword : filterAll;
 
   return (
     <Tabs
@@ -54,7 +64,7 @@ export const CoursewareSearchResultsFilter = ({ intl }) => {
       activeKey={activeKey}
       onSelect={setFilter}
     >
-      {filters.map(({ key, label }) => (
+      {filters.filter(({ count }) => (count > 0)).map(({ key, label }) => (
         <Tab key={key} eventKey={key} title={label} data-testid={`courseware-search-results-tabs-${key}`}>
           <CoursewareSearchResults results={results[key]} />
         </Tab>

diff --git a/src/course-home/courseware-search/CoursewareResultsFilter.test.jsx b/src/course-home/courseware-search/CoursewareResultsFilter.test.jsx
@@ -52,49 +52,75 @@ function renderComponent(props = {}) {
 describe('CoursewareSearchResultsFilter', () => {
   beforeAll(initializeMockApp);
 
-  describe('</CoursewareSearchResultsFilter />', () => {
-    beforeEach(() => {
-      useCoursewareSearchParams.mockReturnValue(coursewareSearch);
-    });
+  beforeEach(() => {
+    useCoursewareSearchParams.mockReturnValue(coursewareSearch);
+  });
+
+  afterEach(() => {
+    jest.clearAllMocks();
+  });
 
-    afterEach(() => {
-      jest.clearAllMocks();
+  describe('when returning full results', () => {
+    beforeEach(async () => {
+      useModel.mockReturnValue(searchResultsFactory());
+      await renderComponent();
     });
 
     it('should render without errors', async () => {
-      useModel.mockReturnValue(searchResultsFactory());
+      await waitFor(() => {
+        expect(useCoursewareSearchParams).toBeCalled();
+      });
 
+      expect(screen.queryByTestId('courseware-search-results-tabs')).toBeInTheDocument();
+      expect(screen.queryByTestId('courseware-search-results-tabs-all')).toBeInTheDocument();
+      expect(screen.queryByTestId('courseware-search-results-tabs-text')).toBeInTheDocument();
+      expect(screen.queryByTestId('courseware-search-results-tabs-video')).toBeInTheDocument();
+      expect(screen.queryByTestId('courseware-search-results-tabs-sequence')).toBeInTheDocument();
+      expect(screen.queryByTestId('courseware-search-results-tabs-other')).toBeInTheDocument();
+    });
+  });
+
+  describe('when returning only one result type', () => {
+    beforeEach(async () => {
+      // Get results for only videos
+      const data = searchResultsFactory();
+      const onlyVideos = data.results.filter(({ type }) => type === 'video');
+      const filteredResults = {
+        ...data,
+        results: onlyVideos,
+      };
+
+      useModel.mockReturnValue(filteredResults);
       await renderComponent();
+    });
 
+    it('should not render', async () => {
       await waitFor(() => {
-        expect(screen.queryByTestId('courseware-search-results-tabs-all')).toBeInTheDocument();
-        expect(screen.queryByTestId('courseware-search-results-tabs-text')).toBeInTheDocument();
-        expect(screen.queryByTestId('courseware-search-results-tabs-video')).toBeInTheDocument();
-        expect(screen.queryByTestId('courseware-search-results-tabs-sequence')).toBeInTheDocument();
-        expect(screen.queryByTestId('courseware-search-results-tabs-other')).toBeInTheDocument();
+        expect(useCoursewareSearchParams).toBeCalled();
       });
+
+      expect(screen.queryByTestId('courseware-search-results-tabs')).not.toBeInTheDocument();
+    });
+  });
+
+  describe('when there are not results', () => {
+    beforeEach(async () => {
+      useModel.mockReturnValue(searchResultsFactory('blah', {
+        results: [],
+        filters: [],
+        total: 0,
+        maxScore: null,
+        ms: 5,
+      }));
+      await renderComponent();
     });
 
-    describe('when there are not results', () => {
-      it('should render without errors', async () => {
-        useModel.mockReturnValue(searchResultsFactory('blah', {
-          results: [],
-          filters: [],
-          total: 0,
-          maxScore: null,
-          ms: 5,
-        }));
-
-        await renderComponent();
-
-        await waitFor(() => {
-          expect(screen.queryByTestId('courseware-search-results-tabs-all')).toBeInTheDocument();
-          expect(screen.queryByTestId('courseware-search-results-tabs-text')).toBeInTheDocument();
-          expect(screen.queryByTestId('courseware-search-results-tabs-video')).toBeInTheDocument();
-          expect(screen.queryByTestId('courseware-search-results-tabs-sequence')).toBeInTheDocument();
-          expect(screen.queryByTestId('courseware-search-results-tabs-other')).toBeInTheDocument();
-        });
+    it('should not render', async () => {
+      await waitFor(() => {
+        expect(useCoursewareSearchParams).toBeCalled();
       });
+
+      expect(screen.queryByTestId('courseware-search-results-tabs')).not.toBeInTheDocument();
     });
   });
 });
diff --git a/src/course-home/courseware-search/CoursewareSearch.jsx b/src/course-home/courseware-search/CoursewareSearch.jsx
@@ -122,16 +122,16 @@ const CoursewareSearch = ({ intl, ...sectionProps }) => {
           )}
           {status === 'results' ? (
             <>
-              <div
-                className="courseware-search__results-summary"
-                aria-live="polite"
-                aria-relevant="all"
-                aria-atomic="true"
-                data-testid="courseware-search-summary"
-              >{total > 0
-                ? intl.formatMessage(messages.searchResultsLabel, { total, keyword: lastSearchKeyword })
-                : intl.formatMessage(messages.searchResultsNone)}
-              </div>
+              {total > 0 ? (
+                <div
+                  className="courseware-search__results-summary"
+                  aria-live="polite"
+                  aria-relevant="all"
+                  aria-atomic="true"
+                  data-testid="courseware-search-summary"
+                >{intl.formatMessage(messages.searchResultsLabel, { total, keyword: lastSearchKeyword })}
+                </div>
+              ) : null}
               <CoursewareSearchResultsFilterContainer />
             </>
           ) : null}

diff --git a/src/course-home/courseware-search/CoursewareSearch.test.jsx b/src/course-home/courseware-search/CoursewareSearch.test.jsx
@@ -236,14 +236,14 @@ describe('CoursewareSearch', () => {
       expect(screen.queryByTestId('courseware-search-error')).toBeInTheDocument();
     });
 
-    it('should show "No results found." if results is empty', () => {
+    it('should not show a summary if there are no results', () => {
       mockModels({
         searchKeyword: 'test',
         total: 0,
       });
       renderComponent();
 
-      expect(screen.queryByTestId('courseware-search-summary').textContent).toBe('No results found.');
+      expect(screen.queryByTestId('courseware-search-summary')).not.toBeInTheDocument();
     });
 
     it('should show a summary for the results', () => {

diff --git a/src/course-home/courseware-search/CoursewareSearchResults.jsx b/src/course-home/courseware-search/CoursewareSearchResults.jsx
@@ -8,9 +8,10 @@ import PropTypes from 'prop-types';
 import CoursewareSearchEmpty from './CoursewareSearchEmpty';
 
 const iconTypeMapping = {
-  document: Folder,
   text: TextFields,
   video: VideoCamera,
+  sequence: Folder,
+  other: Article,
 };
 
 const defaultIcon = Article;
@@ -34,9 +35,7 @@ const CoursewareSearchResults = ({ results = [] }) => {
       }) => {
         const key = type.toLowerCase();
         const icon = iconTypeMapping[key] || defaultIcon;
-
         const isExternal = !url.startsWith('/');
-
         const linkProps = isExternal ? {
           href: url,
           target: '_blank',

diff --git a/src/course-home/courseware-search/__snapshots__/CoursewareSearchResults.test.jsx.snap b/src/course-home/courseware-search/__snapshots__/CoursewareSearchResults.test.jsx.snap
@@ -26,7 +26,7 @@ exports[`CoursewareSearchResults when list of results is provided should match t
           xmlns="http://www.w3.org/2000/svg"
         >
           <path
-            d="M3 3v18h18V3H3Zm11 14H7v-2h7v2Zm3-4H7v-2h10v2Zm0-4H7V7h10v2Z"
+            d="M10 4H2v16h20V6H12l-2-2z"
             fill="currentColor"
           />
         </svg>
@@ -140,7 +140,7 @@ exports[`CoursewareSearchResults when list of results is provided should match t
           xmlns="http://www.w3.org/2000/svg"
         >
           <path
-            d="M3 3v18h18V3H3Zm11 14H7v-2h7v2Zm3-4H7v-2h10v2Zm0-4H7V7h10v2Z"
+            d="M10 4H2v16h20V6H12l-2-2z"
             fill="currentColor"
           />
         </svg>

diff --git a/src/course-home/courseware-search/courseware-search.scss b/src/course-home/courseware-search/courseware-search.scss
@@ -51,6 +51,8 @@
 
   &__empty {
     color: $gray-500;
+    padding: 6rem 0;
+    text-align: center;
   }
 
   &__item {

diff --git a/src/course-home/courseware-search/hooks.js b/src/course-home/courseware-search/hooks.js
@@ -70,12 +70,13 @@ export function useLockScroll() {
   }, []);
 }
 
+const initSearchParams = { q: '', f: '' };
 export function useCoursewareSearchParams() {
-  const [searchParams, setSearchParams] = useSearchParams();
-  const clearSearchParams = () => setSearchParams({ q: '', f: '' });
+  const [searchParams, setSearchParams] = useSearchParams(initSearchParams);
+  const clearSearchParams = () => setSearchParams(initSearchParams);
 
   const query = searchParams.get('q');
-  const filter = searchParams.get('f');
+  const filter = searchParams.get('f')?.toLowerCase();
 
   const setQuery = (q) => setSearchParams((params) => ({ q, f: params.get('f') }));
   const setFilter = (f) => setSearchParams((params) => ({ q: params.get('q'), f }));

diff --git a/src/course-home/courseware-search/hooks.test.jsx b/src/course-home/courseware-search/hooks.test.jsx
@@ -1,9 +1,13 @@
 import { renderHook, act } from '@testing-library/react-hooks';
-import { useParams } from 'react-router-dom';
+import { useParams, useSearchParams } from 'react-router-dom';
 import { useSelector } from 'react-redux';
 import { fetchCoursewareSearchSettings } from '../data/thunks';
 import {
-  useCoursewareSearchFeatureFlag, useCoursewareSearchState, useElementBoundingBox, useLockScroll,
+  useCoursewareSearchFeatureFlag,
+  useCoursewareSearchParams,
+  useCoursewareSearchState,
+  useElementBoundingBox,
+  useLockScroll,
 } from './hooks';
 
 jest.mock('react-redux');
@@ -184,4 +188,45 @@ describe('CoursewareSearch Hooks', () => {
       expect(removeBodyClassSpy).toHaveBeenCalledWith('_search-no-scroll');
     });
   });
+
+  describe('useSearchParams', () => {
+    const initSearch = { q: '', f: '' };
+    const q = { value: '' };
+    const f = { value: '' };
+    const mockedQuery = { q, f };
+    const searchParams = { get: (prop) => mockedQuery[prop].value };
+    const setSearchParams = jest.fn();
+
+    beforeEach(() => {
+      useSearchParams.mockImplementation(() => [searchParams, setSearchParams]);
+    });
+
+    it('should init the search params properly', () => {
+      const {
+        query, filter, setQuery, setFilter, clearSearchParams,
+      } = useCoursewareSearchParams();
+
+      expect(useSearchParams).toBeCalledWith(initSearch);
+      expect(query).toBe('');
+      expect(filter).toBe('');
+
+      setQuery('setQuery');
+      expect(setSearchParams).toBeCalledWith(expect.any(Function));
+
+      setFilter('setFilter');
+      expect(setSearchParams).toBeCalledWith(expect.any(Function));
+
+      clearSearchParams();
+      expect(setSearchParams).toBeCalledWith(initSearch);
+    });
+
+    it('should return the query and lowercase filter if any', () => {
+      q.value = '42';
+      f.value = 'LOWERCASE';
+      const { query, filter } = useCoursewareSearchParams();
+
+      expect(query).toBe('42');
+      expect(filter).toBe('lowercase');
+    });
+  });
 });