From d802e5331f96517d1d4c351217d9ac9023fe11db Mon Sep 17 00:00:00 2001 From: Kshitij Sobti Date: Mon, 14 Mar 2022 11:06:28 +0530 Subject: [PATCH] fix: filtering by post type wasn't working [BD-38] [TNL-9609] (#82) Fixes an issue where filtering by post type wasn't working, since the filter wasn't being passed to the API. It also simplifies the code a bit and adds tests. --- src/data/constants.js | 23 +- src/discussions/data/selectors.js | 4 +- src/discussions/posts/PostsView.test.jsx | 253 ++++++++++++++++++ .../data/__factories__/threads.factory.js | 14 +- src/discussions/posts/data/api.js | 3 + src/discussions/posts/data/redux.test.js | 2 +- src/discussions/posts/data/slices.js | 17 +- src/discussions/posts/data/thunks.js | 9 +- .../posts/post-filter-bar/PostFilterBar.jsx | 46 ++-- .../posts/post-filter-bar/messages.js | 8 +- 10 files changed, 310 insertions(+), 69 deletions(-) create mode 100644 src/discussions/posts/PostsView.test.jsx diff --git a/src/data/constants.js b/src/data/constants.js index f75de87f..ce779366 100644 --- a/src/data/constants.js +++ b/src/data/constants.js @@ -8,6 +8,7 @@ export const API_BASE_URL = getConfig().LMS_BASE_URL; * @enum {string} */ export const ThreadType = { + ALL: 'all', QUESTION: 'question', DISCUSSION: 'discussion', }; @@ -88,17 +89,6 @@ export const ThreadViewStatus = { UNANSWERED: 'unanswered', }; -/** - * Enum for filtering user posts. - * @readonly - * @enum {string} - */ -export const MyPostsFilter = { - MY_POSTS: 'myPosts', - MY_DISCUSSIONS: 'myDiscussions', - MY_QUESTIONS: 'myQuestions', -}; - /** * Enum for filtering posts by status. * @readonly @@ -112,17 +102,6 @@ export const PostsStatusFilter = { UNANSWERED: 'statusUnanswered', }; -/** - * Enum for filtering all posts. - * @readonly - * @enum {string} - */ -export const AllPostsFilter = { - ALL_POSTS: 'allPosts', - ALL_DISCUSSIONS: 'allDiscussions', - ALL_QUESTIONS: 'allQuestions', -}; - /** * Enum for filtering topics. * @readonly diff --git a/src/discussions/data/selectors.js b/src/discussions/data/selectors.js index 6edda371..31c57570 100644 --- a/src/discussions/data/selectors.js +++ b/src/discussions/data/selectors.js @@ -1,5 +1,5 @@ /* eslint-disable import/prefer-default-export */ -import { AllPostsFilter, PostsStatusFilter } from '../../data/constants'; +import { PostsStatusFilter, ThreadType } from '../../data/constants'; export const selectAnonymousPostingConfig = state => ({ allowAnonymous: state.config.allowAnonymous, @@ -21,7 +21,7 @@ export function selectAreThreadsFiltered(state) { return !( filters.status === PostsStatusFilter.ALL - && filters.allPosts === AllPostsFilter.ALL_POSTS + && filters.postType === ThreadType.ALL ); } diff --git a/src/discussions/posts/PostsView.test.jsx b/src/discussions/posts/PostsView.test.jsx new file mode 100644 index 00000000..5e73ad94 --- /dev/null +++ b/src/discussions/posts/PostsView.test.jsx @@ -0,0 +1,253 @@ +import React from 'react'; + +import { fireEvent, render, screen } from '@testing-library/react'; +import MockAdapter from 'axios-mock-adapter'; +import { act } from 'react-dom/test-utils'; +import { IntlProvider } from 'react-intl'; +import { + generatePath, MemoryRouter, Route, Switch, +} from 'react-router'; +import { Factory } from 'rosie'; + +import { initializeMockApp } from '@edx/frontend-platform'; +import { getAuthenticatedHttpClient } from '@edx/frontend-platform/auth'; +import { AppProvider } from '@edx/frontend-platform/react'; + +import { Routes, ThreadType } from '../../data/constants'; +import { initializeStore } from '../../store'; +import { DiscussionContext } from '../common/context'; +import { threadsApiUrl } from './data/api'; +import { PostsView } from './index'; + +import './data/__factories__'; + +const courseId = 'course-v1:edX+TestX+Test_Course'; +let store; +let axiosMock; + +async function renderComponent({ + postId, topicId, category, myPosts, +} = { myPosts: false }) { + let path = generatePath(Routes.POSTS.ALL_POSTS, { courseId }); + let showOwnPosts = false; + if (postId) { + path = generatePath(Routes.POSTS.ALL_POSTS, { courseId, postId }); + } else if (topicId) { + path = generatePath(Routes.POSTS.PATH, { courseId, topicId }); + } else if (category) { + path = generatePath(Routes.TOPICS.CATEGORY, { courseId, category }); + } else if (myPosts) { + path = generatePath(Routes.POSTS.MY_POSTS, { courseId }); + showOwnPosts = myPosts; + } + await render( + + + + + + + + + + + + + + , + ); +} + +describe('PostsView', () => { + const threadCount = 6; + beforeEach(async () => { + initializeMockApp({ + authenticatedUser: { + userId: 3, + username: 'abc123', + administrator: true, + roles: [], + }, + }); + + store = initializeStore({ + blocks: { blocks: { 'test-usage-key': { topics: ['some-topic-2', 'some-topic-0'] } } }, + }); + Factory.resetAll(); + axiosMock = new MockAdapter(getAuthenticatedHttpClient()); + axiosMock.onGet(threadsApiUrl) + .reply((args) => { + const threadAttrs = {}; + if (args.params.author) { + threadAttrs.author = args.params.author; + } + return [200, Factory.build('threadsResult', {}, { + topicId: undefined, + count: threadCount, + threadAttrs, + pageSize: 6, + })]; + }); + }); + + describe('Basic', () => { + test('displays a list of all posts', async () => { + await act(async () => { + await renderComponent(); + }); + expect(screen.getAllByText(/this is thread-\d+/i)).toHaveLength(threadCount); + }); + test('displays a list of user posts', async () => { + await act(async () => { + await renderComponent({ myPosts: true }); + }); + expect(screen.getAllByText('abc123')).toHaveLength(threadCount); + }); + test('displays a list of posts in a topic', async () => { + await act(async () => { + await renderComponent({ topicId: 'some-topic-1' }); + }); + expect(screen.getAllByText(/this is thread-\d+ in topic some-topic-1/i)).toHaveLength(Math.ceil(threadCount / 3)); + }); + test('displays a list of posts in a category', async () => { + await act(async () => { + await renderComponent({ category: 'test-usage-key' }); + }); + expect(screen.queryAllByText(/this is thread-\d+ in topic some-topic-1}/i)).toHaveLength(0); + expect(screen.queryAllByText(/this is thread-\d+ in topic some-topic-2/i)).toHaveLength(Math.ceil(threadCount / 3)); + expect(screen.queryAllByText(/this is thread-\d+ in topic some-topic-0/i)).toHaveLength(Math.ceil(threadCount / 3)); + }); + }); + + describe('Filtering', () => { + let dropDownButton; + + function lastQueryParams() { + const { params } = axiosMock.history.get[axiosMock.history.get.length - 1]; + return params; + } + + beforeEach(async () => { + await act(async () => { + await renderComponent(); + }); + dropDownButton = screen.getByRole('button', { + name: /all posts by recent activity/i, + }); + await act(async () => { + fireEvent.click(dropDownButton); + }); + }); + test('test that the filter bar works', async () => { + // 3 type filters: all, discussion, question + // 5 status filters: any, unread, following, reported, unanswered + // 3 sort: activity, comments, votes + expect(screen.queryAllByRole('radio')).toHaveLength(11); + }); + + describe.each([ + { + label: 'Discussions', + queryParam: { thread_type: ThreadType.DISCUSSION }, + }, + { + label: 'Questions', + queryParam: { thread_type: ThreadType.QUESTION }, + }, + { + label: 'Unread', + queryParam: { view: 'unread' }, + }, + { + label: 'Unanswered', + queryParam: { view: 'unanswered' }, + }, + { + label: 'Following', + queryParam: { following: true }, + }, + { + label: 'Reported', + queryParam: { flagged: true }, + }, + { + label: 'Most activity', + queryParam: { order_by: 'comment_count' }, + }, + { + label: 'Most votes', + queryParam: { order_by: 'vote_count' }, + }, + ])( + 'one at a time', + ({ + label, + queryParam, + }) => { + test(`select "${label}"`, async () => { + await act(async () => { + fireEvent.click(screen.getByLabelText(label)); + }); + // Assert that changing the filters results in the correct query + expect(lastQueryParams()).toMatchObject(queryParam); + }); + }, + ); + + describe.each([ + { + firstClick: 'Discussions', + secondClick: 'Unanswered', + selected: ['Questions', 'Unanswered'], + }, + { + firstClick: 'Unanswered', + secondClick: 'Discussions', + selected: ['Discussions', 'Any'], + }, + { + firstClick: 'Questions', + secondClick: 'Unread', + selected: ['Discussions', 'Unread'], + }, + { + firstClick: 'Unread', + secondClick: 'Questions', + selected: ['Questions', 'Any'], + }, + ])( + 'incompatible combinations', + ({ + firstClick, + secondClick, + selected, + }) => { + test(`select "${firstClick}" then "${secondClick}"`, async () => { + await act(async () => { + fireEvent.click(screen.getByLabelText(firstClick)); + }); + await act(async () => { + fireEvent.click(dropDownButton); + }); + await act(async () => { + fireEvent.click(screen.getByLabelText(secondClick)); + }); + await act(async () => { + fireEvent.click(dropDownButton); + }); + expect(screen.getAllByTestId('selected')[0]).toHaveTextContent(selected[0]); + expect(screen.getAllByTestId('selected')[1]).toHaveTextContent(selected[1]); + }); + }, + ); + }); +}); diff --git a/src/discussions/posts/data/__factories__/threads.factory.js b/src/discussions/posts/data/__factories__/threads.factory.js index dca8cd92..0ddcb9ac 100644 --- a/src/discussions/posts/data/__factories__/threads.factory.js +++ b/src/discussions/posts/data/__factories__/threads.factory.js @@ -2,10 +2,12 @@ import { Factory } from 'rosie'; Factory.define('thread') .sequence('id', (idx) => `thread-${idx}`) - .sequence('title', (idx) => `This is Thread-${idx}`) + .sequence('title', ['topic_id'], (idx, topicId) => `This is Thread-${idx} in topic ${topicId}`) .sequence('raw_body', (idx) => `Some contents for **thread number ${idx}**.`) .sequence('rendered_body', (idx) => `Some contents for thread number ${idx}.`) .sequence('type', (idx) => (idx % 2 === 1 ? 'discussion' : 'question')) + .sequence('pinned', idx => (idx < 3)) + .sequence('topic_id', idx => `some-topic-${(idx % 3)}`) .attr('comment_list_url', ['id'], (threadId) => `http://test.site/api/discussion/v1/comments/?thread_id=${threadId}`) .attrs({ created_at: () => (new Date()).toISOString(), @@ -28,11 +30,9 @@ Factory.define('thread') voted: false, vote_count: 1, course_id: 'course-v1:Test+TestX+Test_Course', - topic_id: 'some-topic', group_id: null, group_name: null, abuse_flagged_count: 0, - pinned: false, closed: false, following: false, comment_count: 8, @@ -49,6 +49,8 @@ Factory.define('threadsResult') .option('pageSize', null, 5) .option('courseId', null, 'course-v1:Test+TestX+Test_Course') .option('topicId', null, 'test-topic') + .option('threadAttrs', null, {}) + .option('threadOptions', null, {}) .attr('pagination', ['courseId', 'count', 'page', 'pageSize'], (courseId, count, page, pageSize) => { const numPages = Math.ceil(count / pageSize); const next = (page < numPages) ? `http://test.site/api/discussion/v1/threads/?course_id=${courseId}&page=${page + 1}` : null; @@ -60,7 +62,9 @@ Factory.define('threadsResult') num_pages: numPages, }; }) - .attr('results', ['count', 'pageSize', 'page', 'courseId', 'topicId'], (count, pageSize, page, courseId, topicId) => { + .attr('results', ['count', 'pageSize', 'page', 'courseId', 'topicId', 'threadAttrs', 'threadOptions'], (count, pageSize, page, courseId, topicId, threadAttrs, threadOptions) => { + const attrs = { course_id: courseId, topic_id: topicId, ...threadAttrs }; + Object.keys(attrs).forEach(key => (attrs[key] === undefined ? delete attrs[key] : {})); const len = (pageSize * page <= count) ? pageSize : count % pageSize; - return Factory.buildList('thread', len, { course_id: courseId, topic_id: topicId }); + return Factory.buildList('thread', len, attrs, threadOptions); }); diff --git a/src/discussions/posts/data/api.js b/src/discussions/posts/data/api.js index 6df5a3d5..7ac6e381 100644 --- a/src/discussions/posts/data/api.js +++ b/src/discussions/posts/data/api.js @@ -24,6 +24,7 @@ export const coursesApiUrl = `${apiBaseUrl}/api/discussion/v1/courses/`; * @param {ThreadOrdering} orderBy The results wil be sorted on this basis. * @param {boolean} following If true, only threads followed by the current user will be returned. * @param {boolean} flagged If true, only threads that have been reported will be returned. + * @param {string} threadType Can be 'discussion' or 'question'. * @param {ThreadViewStatus} view Set to "unread" on "unanswered" to filter to only those statuses. * @returns {Promise<{}>} */ @@ -38,6 +39,7 @@ export async function getThreads( view, author, flagged, + threadType, } = {}, ) { const params = snakeCaseObject({ @@ -46,6 +48,7 @@ export async function getThreads( pageSize, topicId: topicIds && topicIds.join(','), textSearch, + threadType, orderBy: snakeCase(orderBy), following, view, diff --git a/src/discussions/posts/data/redux.test.js b/src/discussions/posts/data/redux.test.js index fa774925..b5303826 100644 --- a/src/discussions/posts/data/redux.test.js +++ b/src/discussions/posts/data/redux.test.js @@ -101,7 +101,7 @@ describe('Threads/Posts data layer tests', () => { expect(store.getState().threads.threadsById['thread-1']) .toHaveProperty('topicId'); expect(store.getState().threads.threadsById['thread-1'].topicId) - .toEqual('some-topic'); + .toEqual('some-topic-1'); }); test('successfully handles thread creation', async () => { diff --git a/src/discussions/posts/data/slices.js b/src/discussions/posts/data/slices.js index b3b5ef66..27155f3e 100644 --- a/src/discussions/posts/data/slices.js +++ b/src/discussions/posts/data/slices.js @@ -2,11 +2,10 @@ import { createSlice } from '@reduxjs/toolkit'; import { - AllPostsFilter, - MyPostsFilter, PostsStatusFilter, RequestStatus, ThreadOrdering, + ThreadType, } from '../../../data/constants'; const threadsSlice = createSlice({ @@ -31,8 +30,7 @@ const threadsSlice = createSlice({ postStatus: RequestStatus.SUCCESSFUL, filters: { status: PostsStatusFilter.ALL, - allPosts: AllPostsFilter.ALL_POSTS, - myPosts: MyPostsFilter.MY_POSTS, + postType: ThreadType.ALL, search: '', }, postEditorVisible: false, @@ -141,12 +139,8 @@ const threadsSlice = createSlice({ state.filters.status = payload; state.pages = []; }, - setAllPostsTypeFilter: (state, { payload }) => { - state.filters.allPosts = payload; - state.pages = []; - }, - setMyPostsTypeFilter: (state, { payload }) => { - state.filters.myPosts = payload; + setPostsTypeFilter: (state, { payload }) => { + state.filters.postType = payload; state.pages = []; }, setSearchQuery: (state, { payload }) => { @@ -191,8 +185,7 @@ export const { updateThreadFailed, updateThreadRequest, updateThreadSuccess, - setAllPostsTypeFilter, - setMyPostsTypeFilter, + setPostsTypeFilter, setSortedBy, setStatusFilter, setSearchQuery, diff --git a/src/discussions/posts/data/thunks.js b/src/discussions/posts/data/thunks.js index 5075c50c..ea57624e 100644 --- a/src/discussions/posts/data/thunks.js +++ b/src/discussions/posts/data/thunks.js @@ -2,7 +2,9 @@ import { camelCaseObject } from '@edx/frontend-platform'; import { logError } from '@edx/frontend-platform/logging'; -import { PostsStatusFilter } from '../../../data/constants'; +import { + PostsStatusFilter, ThreadType, +} from '../../../data/constants'; import { getHttpErrorStatus } from '../../utils'; import { deleteThread, getThread, getThreads, postThread, updateThread, @@ -34,7 +36,7 @@ import { * Filters to apply to a thread/posts query. * @typedef {Object} ThreadFilter * @property {PostsStatusFilter} status - * @property {AllPostsFilter} allPosts + * @property {ThreadType} postType */ /** @@ -110,6 +112,9 @@ export function fetchThreads(courseId, { if (filters.status === PostsStatusFilter.REPORTED) { options.flagged = true; } + if (filters.postType !== ThreadType.ALL) { + options.threadType = filters.postType; + } if (filters.search) { options.textSearch = filters.search; } diff --git a/src/discussions/posts/post-filter-bar/PostFilterBar.jsx b/src/discussions/posts/post-filter-bar/PostFilterBar.jsx index b04ad1ef..d18ef863 100644 --- a/src/discussions/posts/post-filter-bar/PostFilterBar.jsx +++ b/src/discussions/posts/post-filter-bar/PostFilterBar.jsx @@ -1,7 +1,7 @@ import React, { useContext, useState } from 'react'; import PropTypes from 'prop-types'; -import * as classNames from 'classnames'; +import classNames from 'classnames'; import { useDispatch, useSelector } from 'react-redux'; import { injectIntl, intlShape } from '@edx/frontend-platform/i18n'; @@ -9,8 +9,10 @@ import { AppContext } from '@edx/frontend-platform/react'; import { Collapsible, Form, Icon } from '@edx/paragon'; import { Check, Sort } from '@edx/paragon/icons'; -import { AllPostsFilter, PostsStatusFilter, ThreadOrdering } from '../../../data/constants'; -import { setAllPostsTypeFilter, setSortedBy, setStatusFilter } from '../data'; +import { + PostsStatusFilter, ThreadOrdering, ThreadType, +} from '../../../data/constants'; +import { setPostsTypeFilter, setSortedBy, setStatusFilter } from '../data'; import { selectThreadFilters, selectThreadSorting } from '../data/selectors'; import messages from './messages'; @@ -20,12 +22,14 @@ const ActionItem = ({ value, selected, }) => ( -