From d1a677aff79b559396072f1c8ea219ea25140b7a Mon Sep 17 00:00:00 2001 From: Awais Jibran Date: Tue, 27 Jul 2021 19:48:08 +0500 Subject: [PATCH] refactor: code refactoring. (#166) * refactor: discussion topic code refactoring * test: update topic item test cases after code refactor --- package-lock.json | 33 ++------------- package.json | 1 + .../apps/shared/DivisionByGroupFields.jsx | 17 ++------ .../discussion-topics/DiscussionTopics.jsx | 26 +++++++++--- .../shared/discussion-topics/TopicItem.jsx | 42 ++++++------------- .../discussion-topics/TopicItem.test.jsx | 19 +++------ .../discussions/app-config-form/utils.js | 7 ++-- .../discussions/data/api.js | 11 +++-- 8 files changed, 56 insertions(+), 100 deletions(-) diff --git a/package-lock.json b/package-lock.json index 6fbe0793a..027fe8a36 100644 --- a/package-lock.json +++ b/package-lock.json @@ -12384,14 +12384,6 @@ "lodash": "^4.17.20", "pretty-error": "^2.1.1", "tapable": "^2.0.0" - }, - "dependencies": { - "lodash": { - "version": "4.17.21", - "resolved": "https://registry.npmjs.org/lodash/-/lodash-4.17.21.tgz", - "integrity": "sha512-v2kDEe57lecTulaDIuNTPy3Ry4gLGJ6Z1O3vE1krgXZNrsQ+LFTGHVxVjcXPs17LhbZVGedAJv8XZ1tvj5FvSg==", - "dev": true - } } }, "htmlparser2": { @@ -18078,9 +18070,9 @@ } }, "lodash": { - "version": "4.17.19", - "resolved": "https://registry.npmjs.org/lodash/-/lodash-4.17.19.tgz", - "integrity": "sha512-JNvd8XER9GQX0v2qJgsaN/mzFCNA5BRe/j8JN9d+tWyGLSodKQHKFicdwNYzWwI3wjRnaKPsGj1XkBjx/F96DQ==" + "version": "4.17.21", + "resolved": "https://registry.npmjs.org/lodash/-/lodash-4.17.21.tgz", + "integrity": "sha512-v2kDEe57lecTulaDIuNTPy3Ry4gLGJ6Z1O3vE1krgXZNrsQ+LFTGHVxVjcXPs17LhbZVGedAJv8XZ1tvj5FvSg==" }, "lodash-es": { "version": "4.17.20", @@ -20574,14 +20566,6 @@ "requires": { "lodash": "^4.17.20", "renderkid": "^2.0.4" - }, - "dependencies": { - "lodash": { - "version": "4.17.21", - "resolved": "https://registry.npmjs.org/lodash/-/lodash-4.17.21.tgz", - "integrity": "sha512-v2kDEe57lecTulaDIuNTPy3Ry4gLGJ6Z1O3vE1krgXZNrsQ+LFTGHVxVjcXPs17LhbZVGedAJv8XZ1tvj5FvSg==", - "dev": true - } } }, "pretty-format": { @@ -21641,12 +21625,6 @@ "domhandler": "^4.2.0" } }, - "lodash": { - "version": "4.17.21", - "resolved": "https://registry.npmjs.org/lodash/-/lodash-4.17.21.tgz", - "integrity": "sha512-v2kDEe57lecTulaDIuNTPy3Ry4gLGJ6Z1O3vE1krgXZNrsQ+LFTGHVxVjcXPs17LhbZVGedAJv8XZ1tvj5FvSg==", - "dev": true - }, "nth-check": { "version": "2.0.0", "resolved": "https://registry.npmjs.org/nth-check/-/nth-check-2.0.0.tgz", @@ -25472,11 +25450,6 @@ "toposort": "^2.0.2" }, "dependencies": { - "lodash": { - "version": "4.17.20", - "resolved": "https://registry.npmjs.org/lodash/-/lodash-4.17.20.tgz", - "integrity": "sha512-PlhdFcillOINfeV7Ni6oF1TAEayyZBoZ8bcshTHqOYJYlrqzRK5hagpagky5o4HfCzzd1TRkXPMFq6cKk9rGmA==" - }, "toposort": { "version": "2.0.2", "resolved": "https://registry.npmjs.org/toposort/-/toposort-2.0.2.tgz", diff --git a/package.json b/package.json index 26fd105eb..5ae6250b9 100644 --- a/package.json +++ b/package.json @@ -49,6 +49,7 @@ "core-js": "3.8.1", "email-validator": "2.0.4", "formik": "2.2.6", + "lodash": "4.17.21", "moment": "2.27.0", "prop-types": "15.7.2", "react": "16.14.0", diff --git a/src/pages-and-resources/discussions/app-config-form/apps/shared/DivisionByGroupFields.jsx b/src/pages-and-resources/discussions/app-config-form/apps/shared/DivisionByGroupFields.jsx index 21aac43f0..93567f1c9 100644 --- a/src/pages-and-resources/discussions/app-config-form/apps/shared/DivisionByGroupFields.jsx +++ b/src/pages-and-resources/discussions/app-config-form/apps/shared/DivisionByGroupFields.jsx @@ -2,6 +2,7 @@ import React, { useEffect, useContext } from 'react'; import { injectIntl, intlShape } from '@edx/frontend-platform/i18n'; import { Form, TransitionReplace } from '@edx/paragon'; import { FieldArray, useFormikContext } from 'formik'; +import _ from 'lodash'; import FormSwitchGroup from '../../../../../generic/FormSwitchGroup'; import messages from './messages'; import AppConfigFormDivider from './AppConfigFormDivider'; @@ -23,20 +24,9 @@ const DivisionByGroupFields = ({ intl }) => { } = appConfig; useEffect(() => { - const discussionTopicIds = discussionTopics.map( - (topic) => topic.id, - ); - const divideCourseTopicsByCohortsOff = ( - discussionTopicIds.length === divideDiscussionIds.length - && discussionTopicIds.every((topicId) => divideDiscussionIds.includes(topicId)) - ) || !divideDiscussionIds.length; - if (divideByCohorts) { - if (divideCourseTopicsByCohortsOff && !divideCourseTopicsByCohorts) { - setFieldValue('divideCourseTopicsByCohorts', false); - setFieldValue('divideDiscussionIds', discussionTopicIds); - } else { - setFieldValue('divideCourseTopicsByCohorts', true); + if (!divideCourseTopicsByCohorts && _.size(discussionTopics) !== _.size(divideDiscussionIds)) { + setFieldValue('divideDiscussionIds', discussionTopics.map(topic => topic.id)); } } else { setFieldValue('divideDiscussionIds', []); @@ -45,7 +35,6 @@ const DivisionByGroupFields = ({ intl }) => { }, [ divideByCohorts, divideCourseTopicsByCohorts, - discussionTopics, ]); const handleCheckBoxToggle = (event, push, remove) => { diff --git a/src/pages-and-resources/discussions/app-config-form/apps/shared/discussion-topics/DiscussionTopics.jsx b/src/pages-and-resources/discussions/app-config-form/apps/shared/discussion-topics/DiscussionTopics.jsx index ef9cc6a15..06dfb08ed 100644 --- a/src/pages-and-resources/discussions/app-config-form/apps/shared/discussion-topics/DiscussionTopics.jsx +++ b/src/pages-and-resources/discussions/app-config-form/apps/shared/discussion-topics/DiscussionTopics.jsx @@ -1,22 +1,24 @@ -import React, { useEffect, useContext } from 'react'; +import React, { useEffect, useContext, useCallback } from 'react'; import { useDispatch } from 'react-redux'; import { Add } from '@edx/paragon/icons'; import { Button } from '@edx/paragon'; import { injectIntl, intlShape } from '@edx/frontend-platform/i18n'; import { FieldArray, useFormikContext } from 'formik'; import { v4 as uuid } from 'uuid'; - +import _ from 'lodash'; import messages from '../messages'; import TopicItem from './TopicItem'; import { updateValidationStatus } from '../../../../data/slice'; import { LegacyConfigFormContext } from '../../legacy/LegacyConfigFormProvider'; +import filterItemFromObject from '../../../utils'; const DiscussionTopics = ({ intl }) => { const { values: appConfig, validateForm, + setFieldValue, } = useFormikContext(); - const { discussionTopics } = appConfig; + const { discussionTopics, divideDiscussionIds } = appConfig; const dispatch = useDispatch(); const { discussionTopicErrors, @@ -32,10 +34,23 @@ const DiscussionTopics = ({ intl }) => { const handleTopicDelete = async (topicIndex, topicId, remove) => { await remove(topicIndex); validateForm(); - const validTopics = validDiscussionTopics.filter(topic => topic.id !== topicId); - setValidDiscussionTopics(validTopics); + setValidDiscussionTopics(filterItemFromObject(validDiscussionTopics, 'id', topicId)); }; + const handleOnFocus = useCallback((id, hasError) => { + if (hasError) { + setValidDiscussionTopics(currentValidTopics => filterItemFromObject(currentValidTopics, 'id', id)); + setFieldValue('divideDiscussionIds', filterItemFromObject(divideDiscussionIds, 'id', id)); + } else { + setValidDiscussionTopics(currentValidTopics => { + const allDiscussionTopics = [...currentValidTopics, ...discussionTopics.filter(topic => topic.id === id)]; + const allValidTopics = _.remove(allDiscussionTopics, topic => topic.name !== ''); + return _.uniqBy(allValidTopics, 'id'); + }); + setFieldValue('divideDiscussionIds', _.uniq([...divideDiscussionIds, id])); + } + }, [divideDiscussionIds, discussionTopics]); + const addNewTopic = (push) => { const payload = { name: '', id: uuid() }; push(payload); @@ -63,6 +78,7 @@ const DiscussionTopics = ({ intl }) => { key={`topic-${topic.id}`} index={index} onDelete={() => handleTopicDelete(index, topic.id, remove)} + onFocus={(hasError) => handleOnFocus(topic.id, hasError)} hasError={discussionTopicErrors[index]} /> ))} diff --git a/src/pages-and-resources/discussions/app-config-form/apps/shared/discussion-topics/TopicItem.jsx b/src/pages-and-resources/discussions/app-config-form/apps/shared/discussion-topics/TopicItem.jsx index 1051cb29a..ed8fa321c 100644 --- a/src/pages-and-resources/discussions/app-config-form/apps/shared/discussion-topics/TopicItem.jsx +++ b/src/pages-and-resources/discussions/app-config-form/apps/shared/discussion-topics/TopicItem.jsx @@ -1,4 +1,4 @@ -import React, { useState, useContext, useEffect } from 'react'; +import React, { useState, useEffect } from 'react'; import { injectIntl, intlShape } from '@edx/frontend-platform/i18n'; import { Button, @@ -13,46 +13,27 @@ import { Delete, ExpandLess, ExpandMore } from '@edx/paragon/icons'; import { useFormikContext } from 'formik'; import PropTypes from 'prop-types'; import messages from '../messages'; -import { LegacyConfigFormContext } from '../../legacy/LegacyConfigFormProvider'; -import uniqueItems from '../../../utils'; const TopicItem = ({ - intl, index, name, onDelete, id, hasError, + intl, + index, + id, + name, + onDelete, + hasError, + onFocus, }) => { const { - handleChange, handleBlur, errors, values: appConfig, setFieldValue, + handleChange, handleBlur, errors, } = useFormikContext(); const [inFocus, setInFocus] = useState(false); const [showDeletePopup, setShowDeletePopup] = useState(false); const [collapseIsOpen, setCollapseOpen] = useState(); const isGeneralTopic = id === 'course'; - const { - validDiscussionTopics, - setValidDiscussionTopics, - } = useContext(LegacyConfigFormContext); - const { discussionTopics, divideDiscussionIds } = appConfig; - /** - * Update valid discussion topics & divided discussion topics. - * Removes a specific topic from valid discussion topics & divided discussion topics - * if it is invalid. - * Adds a specific topic to valid discussion topics & divided discussion topics - * if it is invalid. - */ useEffect(() => { - if (hasError) { - const validTopicsIds = validDiscussionTopics.filter(topic => topic.id !== id); - setValidDiscussionTopics(validTopicsIds); - setFieldValue('divideDiscussionIds', divideDiscussionIds.filter(topic => topic.id !== id)); - } else { - const validTopicsIds = uniqueItems(validDiscussionTopics.map(topic => topic.id), [id]); - const validTopics = discussionTopics.filter( - topic => validTopicsIds.includes(topic.id), - ); - setValidDiscussionTopics(validTopics); - setFieldValue('divideDiscussionIds', uniqueItems(divideDiscussionIds, [id])); - } - }, [hasError, inFocus]); + onFocus(hasError); + }, [inFocus, hasError]); const getHeading = (isOpen = false) => { let heading; @@ -224,6 +205,7 @@ TopicItem.propTypes = { onDelete: PropTypes.func.isRequired, intl: intlShape.isRequired, hasError: PropTypes.bool.isRequired, + onFocus: PropTypes.func.isRequired, }; export default injectIntl(TopicItem); diff --git a/src/pages-and-resources/discussions/app-config-form/apps/shared/discussion-topics/TopicItem.test.jsx b/src/pages-and-resources/discussions/app-config-form/apps/shared/discussion-topics/TopicItem.test.jsx index 46fe36e55..ba943c8c6 100644 --- a/src/pages-and-resources/discussions/app-config-form/apps/shared/discussion-topics/TopicItem.test.jsx +++ b/src/pages-and-resources/discussions/app-config-form/apps/shared/discussion-topics/TopicItem.test.jsx @@ -21,7 +21,6 @@ import { getAppsUrl } from '../../../../data/api'; import { fetchApps } from '../../../../data/thunks'; import { executeThunk } from '../../../../../../utils'; import { legacyApiResponse } from '../../../../factories/mockApiResponses'; -import LegacyConfigFormProvider from '../../legacy/LegacyConfigFormProvider'; import messages from '../messages'; const appConfig = { @@ -38,6 +37,7 @@ const generalTopic = { onDelete: jest.fn(), id: 'course', hasError: false, + onFocus: jest.fn(), }; const additionalTopic = { @@ -46,14 +46,7 @@ const additionalTopic = { onDelete: jest.fn(), id: '13f106c6-6735-4e84-b097-0456cff55960', hasError: false, -}; - -const contextValue = { - validDiscussionTopics: [ - { name: 'General', id: 'course' }, - { name: 'Edx', id: '13f106c6-6735-4e84-b097-0456cff55960' }, - ], - setValidDiscussionTopics: jest.fn(), + onFocus: jest.fn(), }; const courseId = 'course-v1:edX+TestX+Test_Course'; @@ -85,11 +78,9 @@ describe('TopicItem', () => { const wrapper = render( - - - - - + + + , ); diff --git a/src/pages-and-resources/discussions/app-config-form/utils.js b/src/pages-and-resources/discussions/app-config-form/utils.js index ceeee2f63..232c6e38f 100644 --- a/src/pages-and-resources/discussions/app-config-form/utils.js +++ b/src/pages-and-resources/discussions/app-config-form/utils.js @@ -1,6 +1,5 @@ -/** Append items and makes sure the items are unique. */ -const uniqueItems = (sourceArray, items = []) => ( - [...new Set([...sourceArray, ...items])] +const filterItemFromObject = (array, key, value) => ( + array.filter(item => item[key] !== value) ); -export default uniqueItems; +export default filterItemFromObject; diff --git a/src/pages-and-resources/discussions/data/api.js b/src/pages-and-resources/discussions/data/api.js index e0f6c1d59..66517ed09 100644 --- a/src/pages-and-resources/discussions/data/api.js +++ b/src/pages-and-resources/discussions/data/api.js @@ -1,5 +1,6 @@ import { getConfig } from '@edx/frontend-platform'; import { getAuthenticatedHttpClient } from '@edx/frontend-platform/auth'; +import _ from 'lodash'; function normalizeLtiConfig(data) { if (!data || Object.keys(data).length < 1) { @@ -33,13 +34,18 @@ function normalizePluginConfig(data) { if (!data || Object.keys(data).length < 1) { return {}; } + const discussionDividedTopicsCount = _.size(data.divided_course_wide_discussions); + const discussionTopicsCount = _.size(data.discussion_topics); + const enableDivideCourseTopicsByCohorts = ( + discussionDividedTopicsCount && discussionDividedTopicsCount !== discussionTopicsCount + ); return { allowAnonymousPosts: data.allow_anonymous, allowAnonymousPostsPeers: data.allow_anonymous_to_peers, blackoutDates: JSON.stringify(data.discussion_blackouts), allowDivisionByUnit: false, - divideByCohorts: data.divided_course_wide_discussions.length > 0, - divideCourseTopicsByCohorts: false, + divideByCohorts: discussionDividedTopicsCount > 0, + divideCourseTopicsByCohorts: enableDivideCourseTopicsByCohorts, }; } @@ -48,7 +54,6 @@ function normalizeApps(data) { id: key, messages: app.messages, featureIds: app.features, - // TODO: Fix this and get it from the backend! externalLinks: { learnMore: app.external_links.learn_more, configuration: app.external_links.configuration,