diff --git a/src/pages-and-resources/discussions/app-config-form/apps/legacy/LegacyConfigForm.jsx b/src/pages-and-resources/discussions/app-config-form/apps/legacy/LegacyConfigForm.jsx index 1e019ff68..983c82a91 100644 --- a/src/pages-and-resources/discussions/app-config-form/apps/legacy/LegacyConfigForm.jsx +++ b/src/pages-and-resources/discussions/app-config-form/apps/legacy/LegacyConfigForm.jsx @@ -13,6 +13,26 @@ import BlackoutDatesField, { blackoutDatesRegex } from '../shared/BlackoutDatesF import messages from '../shared/messages'; import AppConfigFormDivider from '../shared/AppConfigFormDivider'; +// eslint-disable-next-line func-names +Yup.addMethod(Yup.object, 'uniqueProperty', function (propertyName, message) { + // eslint-disable-next-line func-names + return this.test('unique', message, function (discussionTopic) { + if (!discussionTopic || !discussionTopic[propertyName]) { + return true; + } + const isDuplicate = this.parent.filter(topic => topic !== discussionTopic) + .some(topic => topic[propertyName]?.toLowerCase() === discussionTopic[propertyName].toLowerCase()); + + if (isDuplicate) { + throw this.createError({ + path: `${this.path}.${propertyName}`, + error: message, + }); + } + return true; + }); +}); + function LegacyConfigForm({ appConfig, onSubmit, formRef, intl, title, }) { @@ -21,35 +41,17 @@ function LegacyConfigForm({ blackoutDatesRegex, intl.formatMessage(messages.blackoutDatesFormattingError), ), - discussionTopics: Yup.array() - .of( - Yup.object().shape({ - name: Yup.string().required(intl.formatMessage(messages.discussionTopicRequired)), - }), - ).test('unique', ( - discussionTopics, - testContext, - message = intl.formatMessage(messages.discussionTopicNameAlreadyExist), - ) => { - const uniqueDiscussionTopics = [...new Set(discussionTopics.map(topic => topic.name))]; - const isUnique = discussionTopics.length === uniqueDiscussionTopics.length; - if (isUnique) { - return true; - } - - const duplicateNameIndex = discussionTopics.findIndex( - (topic, index) => topic.name !== uniqueDiscussionTopics[index], - ); - return testContext.createError({ - path: `[${duplicateNameIndex}].name`, - message, - }); - }), + discussionTopics: Yup.array( + Yup.object({ + name: Yup.string().required(intl.formatMessage(messages.discussionTopicRequired)), + }).uniqueProperty('name', intl.formatMessage(messages.discussionTopicNameAlreadyExist)), + ), }); return ( (onSubmit(values))} > @@ -60,35 +62,42 @@ function LegacyConfigForm({ handleBlur, values, errors, + touched, }, - ) => ( - - - {title} - - - - - - - - - - - )} + ) => { + const { discussionTopics } = values; + const discussionTopicErrors = discussionTopics.map((value, index) => Boolean( + touched.discussionTopics + && touched.discussionTopics[index]?.name + && errors.discussionTopics + && errors?.discussionTopics[index]?.name, + )); + + return ( + + + {title} + + + + + + + + + + + ); + }} ); } 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 69c023c65..a213740c1 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 @@ -1,5 +1,4 @@ import React, { useEffect } from 'react'; -import { useDispatch } from 'react-redux'; import PropTypes from 'prop-types'; import { injectIntl, intlShape } from '@edx/frontend-platform/i18n'; import { Form, TransitionReplace } from '@edx/paragon'; @@ -7,14 +6,14 @@ import { FieldArray, useFormikContext } from 'formik'; import FormSwitchGroup from '../../../../../generic/FormSwitchGroup'; import messages from './messages'; import AppConfigFormDivider from './AppConfigFormDivider'; -import { updateDividedDiscussionsIds } from '../../../data/slice'; -import { updateModel } from '../../../../../generic/model-store'; -function DivisionByGroupFields({ - onBlur, onChange, intl, appConfig, -}) { - const dispatch = useDispatch(); - const { setFieldValue } = useFormikContext(); +const DivisionByGroupFields = ({ intl, discussionTopicErrors }) => { + const { + handleChange, + handleBlur, + values: appConfig, + setFieldValue, + } = useFormikContext(); const { divideDiscussionIds, discussionTopics, @@ -22,10 +21,6 @@ function DivisionByGroupFields({ divideCourseTopicsByCohorts, } = appConfig; - useEffect(() => { - dispatch(updateDividedDiscussionsIds({ divideDiscussionIds })); - }, [divideDiscussionIds]); - useEffect(() => { const discussionTopicIds = discussionTopics.map( (topic) => topic.id, @@ -46,13 +41,6 @@ function DivisionByGroupFields({ setFieldValue('divideDiscussionIds', []); setFieldValue('divideCourseTopicsByCohorts', false); } - - const { - divideDiscussionIds: courseDividedDiscussionsIds, - discussionTopics: courseDiscussionTopics, - ...payload - } = appConfig; - dispatch(updateModel({ modelType: 'appConfigs', model: payload })); }, [ divideByCohorts, divideCourseTopicsByCohorts, @@ -73,7 +61,7 @@ function DivisionByGroupFields({ if (!checked) { setFieldValue('divideDiscussionIds', []); } - onChange(event); + handleChange(event); }; return ( @@ -82,9 +70,9 @@ function DivisionByGroupFields({ {intl.formatMessage(messages.divisionByGroup)} handleDivideCourseTopicsByCohortsToggle(event)} - onBlur={onBlur} + onBlur={handleBlur} className="ml-4 mt-3" id="divideCourseTopicsByCohorts" checked={divideCourseTopicsByCohorts} @@ -113,11 +101,11 @@ function DivisionByGroupFields({ handleCheckBoxToggle(event, push, remove)} - onBlur={onBlur} + onBlur={handleBlur} defaultValue={divideDiscussionIds} > - {discussionTopics.map((topic) => ( - topic.name ? ( + {discussionTopics.map((topic, index) => ( + topic.name && !discussionTopicErrors[index] ? ( > ); -} +}; DivisionByGroupFields.propTypes = { - onBlur: PropTypes.func.isRequired, - onChange: PropTypes.func.isRequired, intl: intlShape.isRequired, - appConfig: PropTypes.shape({ - divideByCohorts: PropTypes.bool, - divideCourseTopicsByCohorts: PropTypes.bool, - discussionTopics: PropTypes.arrayOf(PropTypes.shape({ - name: PropTypes.string, - id: PropTypes.string, - })), - divideDiscussionIds: PropTypes.arrayOf(PropTypes.string), - }).isRequired, + discussionTopicErrors: PropTypes.arrayOf(PropTypes.bool).isRequired, }; export default injectIntl(DivisionByGroupFields); 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 0c6210280..973fd34e0 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,33 +1,33 @@ -import React, { useState, useEffect } from 'react'; +import React, { useEffect } 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 PropTypes from 'prop-types'; import messages from '../messages'; import TopicItem from './TopicItem'; -import { removeModel, updateModels } from '../../../../../../generic/model-store'; -import { updateDiscussionTopicIds } from '../../../../data/slice'; +import { updateValidationStatus } from '../../../../data/slice'; -const DiscussionTopics = ({ intl }) => { - const dispatch = useDispatch(); - const { values: appConfig, setFieldValue } = useFormikContext(); +const DiscussionTopics = ({ intl, discussionTopicErrors }) => { + const { + values: appConfig, + setFieldValue, + validateForm, + } = useFormikContext(); const { discussionTopics, divideDiscussionIds } = appConfig; - const [topics, setTopics] = useState(discussionTopics); + const dispatch = useDispatch(); + const isFormInvalid = discussionTopicErrors.some((error) => error === true); useEffect(() => { - const updatedDiscussionTopicIds = discussionTopics.map(topic => topic.id); + dispatch(updateValidationStatus({ hasError: isFormInvalid })); + }, [isFormInvalid]); - setTopics(discussionTopics); - dispatch(updateDiscussionTopicIds({ updatedDiscussionTopicIds })); - dispatch(updateModels({ modelType: 'discussionTopics', models: discussionTopics })); - }, [discussionTopics]); - - const handleTopicDelete = (topicIndex, topicId, remove) => { - remove(topicIndex); - dispatch(removeModel({ modelType: 'discussionTopics', id: topicId })); + const handleTopicDelete = async (topicIndex, topicId, remove) => { + await remove(topicIndex); + validateForm(); const updatedDividedDiscussionsIds = divideDiscussionIds.filter( (id) => id !== topicId, ); @@ -56,16 +56,15 @@ const DiscussionTopics = ({ intl }) => { name="discussionTopics" render={({ push, remove }) => ( - { - topics.map((topic, index) => ( - handleTopicDelete(index, topic.id, remove)} - /> - )) - } + {discussionTopics.map((topic, index) => ( + handleTopicDelete(index, topic.id, remove)} + hasError={discussionTopicErrors[index]} + /> + ))} addNewTopic(push)} @@ -86,6 +85,7 @@ const DiscussionTopics = ({ intl }) => { DiscussionTopics.propTypes = { intl: intlShape.isRequired, + discussionTopicErrors: PropTypes.arrayOf(PropTypes.bool).isRequired, }; export default injectIntl(DiscussionTopics); diff --git a/src/pages-and-resources/discussions/app-config-form/apps/shared/discussion-topics/DiscussionTopics.test.jsx b/src/pages-and-resources/discussions/app-config-form/apps/shared/discussion-topics/DiscussionTopics.test.jsx index 726926859..752d223cd 100644 --- a/src/pages-and-resources/discussions/app-config-form/apps/shared/discussion-topics/DiscussionTopics.test.jsx +++ b/src/pages-and-resources/discussions/app-config-form/apps/shared/discussion-topics/DiscussionTopics.test.jsx @@ -36,6 +36,7 @@ const appConfig = { blackoutDates: '[]', }; +const discussionTopicErrors = [false, false]; const courseId = 'course-v1:edX+TestX+Test_Course'; describe('DiscussionTopics', () => { @@ -65,10 +66,8 @@ describe('DiscussionTopics', () => { const wrapper = render( - - + + , 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 4e9c7b6bb..8269a3d64 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,77 +1,56 @@ -import React, { useState, useEffect } from 'react'; -import { useDispatch } from 'react-redux'; -import PropTypes from 'prop-types'; -import { - Collapsible, Form, Card, Button, IconButton, Icon, -} from '@edx/paragon'; +import React, { useState } from 'react'; import { injectIntl, intlShape } from '@edx/frontend-platform/i18n'; -import { useFormikContext } from 'formik'; -import { ExpandLess, ExpandMore, Delete } from '@edx/paragon/icons'; -import messages from '../messages'; import { - updateValidationStatus, -} from '../../../../data/slice'; + Button, + Card, + Collapsible, + Form, + Icon, + IconButton, + TransitionReplace, +} from '@edx/paragon'; +import { Delete, ExpandLess, ExpandMore } from '@edx/paragon/icons'; +import { useFormikContext } from 'formik'; +import PropTypes from 'prop-types'; +import messages from '../messages'; const TopicItem = ({ - intl, index, name, onDelete, id, + intl, index, name, onDelete, id, hasError, }) => { - const [title, setTitle] = useState(name); + const { handleChange, handleBlur, errors } = useFormikContext(); + const [inFocus, setInFocus] = useState(false); const [showDeletePopup, setShowDeletePopup] = useState(false); - const [collapseIsOpen, setCollapseIsOpen] = useState(!name.length); - - const { - handleChange, - handleBlur, - touched, - errors, - } = useFormikContext(); - const dispatch = useDispatch(); + const [collapseIsOpen, setCollapseOpen] = useState(); const isGeneralTopic = id === 'course'; - useEffect(() => { - setTitle(name); - }, [name]); - - useEffect(() => { - if (Object.keys(touched).length) { - dispatch(updateValidationStatus({ hasError: Object.keys(errors).length > 0 })); - } - }, [errors, touched]); - - const isInvalidTopicNameKey = Boolean( - (touched.discussionTopics && touched.discussionTopics[index]?.name) - && (errors.discussionTopics && errors?.discussionTopics[index]?.name), - ); - - const isExistingName = Boolean( - (touched.discussionTopics && touched.discussionTopics[index]?.name) - && (errors && errors[index]?.name), - ); - const getHeading = (isOpen = false) => { let heading; if (isGeneralTopic && isOpen) { heading = ( {intl.formatMessage(messages.renameGeneralTopic)} - {intl.formatMessage(messages.generalTopicHelp)} + + {intl.formatMessage(messages.generalTopicHelp)} + ); } else if (isOpen) { - heading = {intl.formatMessage(messages.configureAdditionalTopic)}; + heading = ( + + {intl.formatMessage(messages.configureAdditionalTopic)} + + ); } else { - heading = {title}; + heading = {name}; } return heading; }; const handleToggle = (isOpen) => { - if (!isOpen) { - const inputHasError = !name.length || isExistingName || isInvalidTopicNameKey; - setCollapseIsOpen(inputHasError); - } else { - setCollapseIsOpen(isOpen); + if (!isOpen && (!name.length || hasError)) { + return setCollapseOpen(true); } + return setCollapseOpen(isOpen); }; const deleteDiscussionTopic = (event) => { @@ -79,7 +58,18 @@ const TopicItem = ({ setShowDeletePopup(true); }; - const deletetopic = ( + const renderFormFeedback = (message, messageType = 'default') => ( + + {message} + + ); + + const handleFocusOut = (event) => { + handleBlur(event); + setInFocus(false); + }; + + const deleteTopicPopup = ( @@ -89,10 +79,7 @@ const TopicItem = ({ {intl.formatMessage(messages.discussionTopicDeletionHelp)} - setShowDeletePopup(false)} - > + setShowDeletePopup(false)}> {intl.formatMessage(messages.cancelButton)} - { - showDeletePopup ? ( - deletetopic - ) : ( - + - - - {getHeading(false)} - - {}} - variant="dark" - /> - - - - {getHeading(true)} - { - !isGeneralTopic && ( - - - - ) - } - - {}} - variant="dark" - /> - - - - - - + {getHeading(false)} + + {}} + variant="dark" /> - {isInvalidTopicNameKey && ( - - - {intl.formatMessage(messages.discussionTopicRequired)} - - + + + + {getHeading(true)} + {!isGeneralTopic && ( + + + + )} + + {}} + variant="dark" + /> + + + + + + handleFocusOut(event)} + value={name} + controlClassName="bg-white" + onFocus={() => setInFocus(true)} + /> + + {inFocus ? ( + + {renderFormFeedback(intl.formatMessage(messages.addTopicHelpText))} + + ) : ( + )} - {isExistingName && ( - - - {intl.formatMessage(messages.discussionTopicNameAlreadyExist)} - - + + + {hasError && !inFocus ? ( + + {renderFormFeedback(errors?.discussionTopics[index].name, 'invalid')} + + ) : ( + )} - - - - ) - } + + + + + )} > ); }; @@ -204,6 +192,7 @@ TopicItem.propTypes = { index: PropTypes.number.isRequired, onDelete: PropTypes.func.isRequired, intl: intlShape.isRequired, + hasError: PropTypes.bool.isRequired, }; export default injectIntl(TopicItem); diff --git a/src/pages-and-resources/discussions/app-config-form/apps/shared/messages.js b/src/pages-and-resources/discussions/app-config-form/apps/shared/messages.js index 7bd565104..898a8e416 100644 --- a/src/pages-and-resources/discussions/app-config-form/apps/shared/messages.js +++ b/src/pages-and-resources/discussions/app-config-form/apps/shared/messages.js @@ -124,7 +124,7 @@ const messages = defineMessages({ }, addTopicButton: { id: 'authoring.discussions.addTopicButton', - defaultMessage: 'Add Topic', + defaultMessage: 'Add topic', description: 'Button label when Add a new discussion topic.', }, deleteButton: { @@ -162,6 +162,11 @@ const messages = defineMessages({ defaultMessage: 'Configure topic', description: 'Label for Additional topic allowing user to configure additional topic name', }, + addTopicHelpText: { + id: 'authoring.discussions.addTopicHelpText', + defaultMessage: 'Choose a unique name for your topic', + description: 'Help text for input field in adding a discussion topic', + }, // Blackout dates blackoutDates: { id: 'authoring.discussions.blackoutDates',