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.

Sign up for GitHub

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: update filter naming on courseware search modal #1255

Merged
merged 1 commit into from
Dec 18, 2023
Merged

feat: update filter naming on courseware search modal #1255

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
54 changes: 28 additions & 26 deletions src/course-home/courseware-search/CoursewareResultsFilter.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React from 'react';
import React, { useMemo } from 'react';
import { injectIntl, intlShape } from '@edx/frontend-platform/i18n';
import { Tabs, Tab } from '@edx/paragon';

Expand All @@ -8,12 +8,15 @@ import messages from './messages';
import { useCoursewareSearchParams } from './hooks';
import { useModel } from '../../generic/model-store';

const noFilterKey = 'none';
const noFilterLabel = 'All content';

export const filteredResultsBySelection = ({ key = noFilterKey, results = [] }) => (
key === noFilterKey ? results : results.filter(({ type }) => type === key)
);
const allFilterKey = 'all';
const otherFilterKey = 'other';
const allowedFilterKeys = {
[allFilterKey]: true,
text: true,
video: true,
sequence: true,
[otherFilterKey]: true,
};

export const CoursewareSearchResultsFilter = ({ intl }) => {
const { courseId } = useParams();
Expand All @@ -22,24 +25,25 @@ export const CoursewareSearchResultsFilter = ({ intl }) => {

if (!lastSearch || !lastSearch?.results?.length) { return null; }

const { total, results } = lastSearch;
const { results: data = [] } = lastSearch;

const results = useMemo(() => data.reduce((acc, { type, ...rest }) => {
acc[allFilterKey] = [...(acc[allFilterKey] || []), { type: allFilterKey, ...rest }];
if (type === allFilterKey) { return acc; }

const filters = [
{
key: noFilterKey,
label: noFilterLabel,
count: total,
},
...lastSearch.filters,
];
let targetKey = otherFilterKey;
if (allowedFilterKeys[type]) { targetKey = type; }
acc[targetKey] = [...(acc[targetKey] || []), { type: targetKey, ...rest }];
return acc;
}, {}), [data]);

const activeKey = filters.find(({ key }) => key === filterKeyword)?.key || noFilterKey;
const filters = useMemo(() => Object.keys(allowedFilterKeys).map((key) => ({
key,
label: intl.formatMessage(messages[`filter:${key}`]),
count: results[key].length,
})), [results]);

const getFilterTitle = (key, fallback) => {
const msg = messages[`filter:${key}`];
if (!msg) { return fallback; }
return intl.formatMessage(msg);
};
const activeKey = allowedFilterKeys[filterKeyword] ? filterKeyword : allFilterKey;

return (
<Tabs
Expand All @@ -51,10 +55,8 @@ export const CoursewareSearchResultsFilter = ({ intl }) => {
onSelect={setFilter}
>
{filters.map(({ key, label }) => (
<Tab key={key} eventKey={key} title={getFilterTitle(key, label)}>
<CoursewareSearchResults
results={filteredResultsBySelection({ key, results })}
/>
<Tab key={key} eventKey={key} title={label} data-testid={`courseware-search-results-tabs-${key}`}>
<CoursewareSearchResults results={results[key]} />
</Tab>
))}
</Tabs>
Expand Down
62 changes: 6 additions & 56 deletions src/course-home/courseware-search/CoursewareResultsFilter.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import {
screen,
waitFor,
} from '../../setupTest';
import { CoursewareSearchResultsFilter, filteredResultsBySelection } from './CoursewareResultsFilter';
import { CoursewareSearchResultsFilter } from './CoursewareResultsFilter';
import { useCoursewareSearchParams } from './hooks';
import initializeStore from '../../store';
import { useModel } from '../../generic/model-store';
Expand All @@ -19,27 +19,6 @@ jest.mock('../../generic/model-store', () => ({
useModel: jest.fn(),
}));

const mockResults = [
{
id: 'video-1', type: 'video', title: 'video_title', score: 3, contentHits: 1, url: '/video-1', location: ['path1', 'path2'],
},
{
id: 'video-2', type: 'video', title: 'video_title2', score: 2, contentHits: 1, url: '/video-2', location: ['path1', 'path2'],
},
{
id: 'document-1', type: 'document', title: 'document_title', score: 3, contentHits: 1, url: '/document-1', location: ['path1', 'path2'],
},
{
id: 'text-1', type: 'text', title: 'text_title1', score: 3, contentHits: 1, url: '/text-1', location: ['path1', 'path2'],
},
{
id: 'text-2', type: 'text', title: 'text_title2', score: 2, contentHits: 1, url: '/text-2', location: ['path1', 'path2'],
},
{
id: 'text-3', type: 'text', title: 'text_title3', score: 1, contentHits: 1, url: '/text-3', location: ['path1', 'path2'],
},
];

const decodedCourseId = 'course-v1:edX+DemoX+Demo_Course';
const decodedSequenceId = 'block-v1:edX+DemoX+Demo_Course+type@sequential+block@edx_introduction';
const decodedUnitId = 'block-v1:edX+DemoX+Demo_Course+type@vertical+block@vertical_0270f6de40fc';
Expand Down Expand Up @@ -73,38 +52,6 @@ function renderComponent(props = {}) {
describe('CoursewareSearchResultsFilter', () => {
beforeAll(initializeMockApp);

describe('filteredResultsBySelection', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why were all these unit tests removed? Wouldn't this lower the code coverage?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nop, because the filters are now autogenerated based on the results. @rijuma added a mocked factory that returns all different kind of results, and bellow I am checking that we have all the allowed filters:

  1. All Content
  2. Text
  3. Video
  4. Section
  5. Other

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here: https://github.com/openedx/frontend-app-learning/pull/1255/files#diff-196671061483745d24904f5c4b39b9505ee34fd28ed336cdf52499eec362b4ceR67 I am checking to have all the filters based on this mock data https://github.com/openedx/frontend-app-learning/blob/master/src/course-home/courseware-search/test-data/mocked-response.json

it('returns a no values array when no results are provided', () => {
const results = filteredResultsBySelection({});

expect(results.length).toEqual(0);
});

it('returns all values when no key value is provided', () => {
const results = filteredResultsBySelection({ results: mockResults });

expect(results.length).toEqual(mockResults.length);
});

it('returns only "video"-typed elements when the key value "video" is given', () => {
const results = filteredResultsBySelection({ key: 'video', results: mockResults });

expect(results.length).toEqual(2);
});

it('returns only "course_outline"-typed elements when the key value "document" is given', () => {
const results = filteredResultsBySelection({ key: 'document', results: mockResults });

expect(results.length).toEqual(1);
});

it('returns only "text"-typed elements when the key value "text" is given', () => {
const results = filteredResultsBySelection({ key: 'text', results: mockResults });

expect(results.length).toEqual(3);
});
});

describe('</CoursewareSearchResultsFilter />', () => {
beforeEach(() => {
jest.clearAllMocks();
Expand All @@ -117,8 +64,11 @@ describe('CoursewareSearchResultsFilter', () => {
await renderComponent();

await waitFor(() => {
expect(screen.queryByTestId('courseware-search-results-tabs')).toBeInTheDocument();
expect(screen.queryByText(/All content/)).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();
});
});
});
Expand Down
3 changes: 1 addition & 2 deletions src/course-home/courseware-search/CoursewareSearchResults.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,14 +70,13 @@ const CoursewareSearchResults = ({ results }) => {
};

CoursewareSearchResults.propTypes = {
results: PropTypes.arrayOf(PropTypes.objectOf({
results: PropTypes.arrayOf(PropTypes.shape({
id: PropTypes.string,
title: PropTypes.string,
type: PropTypes.string,
location: PropTypes.arrayOf(PropTypes.string),
url: PropTypes.string,
contentHits: PropTypes.number,
score: PropTypes.number,
})),
};

Expand Down
14 changes: 12 additions & 2 deletions src/course-home/courseware-search/messages.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,8 @@ const messages = defineMessages({
},

// These are translations for labeling the filters
'filter:none': {
id: 'learn.coursewareSerch.filter:none',
'filter:all': {
id: 'learn.coursewareSerch.filter:all',
defaultMessage: 'All content',
description: 'Label for the search results filter that shows all content (no filter).',
},
Expand All @@ -68,6 +68,16 @@ const messages = defineMessages({
defaultMessage: 'Video',
description: 'Label for the search results filter that shows results with video content.',
},
'filter:sequence': {
id: 'learn.coursewareSerch.filter:sequence',
defaultMessage: 'Section',
description: 'Label for the search results filter that shows results with section content.',
},
'filter:other': {
id: 'learn.coursewareSerch.filter:other',
defaultMessage: 'Other',
description: 'Label for the search results filter that shows results with other content.',
},
});

export default messages;