refactor: code refactoring. (#166)

* refactor: discussion topic code refactoring * test: update topic item test cases after code refactor
2021-07-27 19:48:08 +05:00
parent 0c545bcc68
commit d1a677aff7
8 changed files with 56 additions and 100 deletions
--- 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",
--- 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",
--- 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) => {
--- 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]}
                />
              ))}
--- 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);
--- 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(
      <AppProvider store={store}>
        <IntlProvider locale="en">
-          <LegacyConfigFormProvider value={contextValue}>
-            <Formik initialValues={appConfig}>
-              <TopicItem {...props} />
-            </Formik>
-          </LegacyConfigFormProvider>
+          <Formik initialValues={appConfig}>
+            <TopicItem {...props} />
+          </Formik>
        </IntlProvider>
      </AppProvider>,
    );
--- 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;
--- 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,