fix: discussion topics validation and UI changes (#142)

* fix: discussion topics validation and UI changes
This commit is contained in:
Awais Ansari
2021-06-24 13:20:36 +05:00
committed by GitHub
parent 9004270cb0
commit f0699762ae
6 changed files with 237 additions and 257 deletions

View File

@@ -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 (
<Formik
initialValues={appConfig}
validateOnChange={false}
validationSchema={legacyFormValidationSchema}
onSubmit={(values) => (onSubmit(values))}
>
@@ -60,35 +62,42 @@ function LegacyConfigForm({
handleBlur,
values,
errors,
touched,
},
) => (
<Card className="mb-5 px-4 px-sm-5 pb-5" data-testid="legacyConfigForm">
<Form ref={formRef} onSubmit={handleSubmit}>
<h3 className="text-primary-500 my-3">{title}</h3>
<AppConfigFormDivider thick />
<AnonymousPostingFields
onBlur={handleBlur}
onChange={handleChange}
values={values}
/>
<AppConfigFormDivider thick />
<DiscussionTopics />
<AppConfigFormDivider thick />
<DivisionByGroupFields
onBlur={handleBlur}
onChange={handleChange}
appConfig={values}
/>
<AppConfigFormDivider thick />
<BlackoutDatesField
errors={errors}
onBlur={handleBlur}
onChange={handleChange}
values={values}
/>
</Form>
</Card>
)}
) => {
const { discussionTopics } = values;
const discussionTopicErrors = discussionTopics.map((value, index) => Boolean(
touched.discussionTopics
&& touched.discussionTopics[index]?.name
&& errors.discussionTopics
&& errors?.discussionTopics[index]?.name,
));
return (
<Card className="mb-5 px-4 px-sm-5 pb-5" data-testid="legacyConfigForm">
<Form ref={formRef} onSubmit={handleSubmit}>
<h3 className="text-primary-500 my-3">{title}</h3>
<AppConfigFormDivider thick />
<AnonymousPostingFields
onBlur={handleBlur}
onChange={handleChange}
values={values}
/>
<AppConfigFormDivider thick />
<DiscussionTopics discussionTopicErrors={discussionTopicErrors} />
<AppConfigFormDivider thick />
<DivisionByGroupFields discussionTopicErrors={discussionTopicErrors} />
<AppConfigFormDivider thick />
<BlackoutDatesField
errors={errors}
onBlur={handleBlur}
onChange={handleChange}
values={values}
/>
</Form>
</Card>
);
}}
</Formik>
);
}

View File

@@ -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)}
</h5>
<FormSwitchGroup
onChange={onChange}
onChange={handleChange}
className="mt-2"
onBlur={onBlur}
onBlur={handleBlur}
id="divideByCohorts"
checked={divideByCohorts}
label={intl.formatMessage(messages.divideByCohortsLabel)}
@@ -96,7 +84,7 @@ function DivisionByGroupFields({
<AppConfigFormDivider />
<FormSwitchGroup
onChange={(event) => handleDivideCourseTopicsByCohortsToggle(event)}
onBlur={onBlur}
onBlur={handleBlur}
className="ml-4 mt-3"
id="divideCourseTopicsByCohorts"
checked={divideCourseTopicsByCohorts}
@@ -113,11 +101,11 @@ function DivisionByGroupFields({
<Form.CheckboxSet
name="dividedTopics"
onChange={(event) => handleCheckBoxToggle(event, push, remove)}
onBlur={onBlur}
onBlur={handleBlur}
defaultValue={divideDiscussionIds}
>
{discussionTopics.map((topic) => (
topic.name ? (
{discussionTopics.map((topic, index) => (
topic.name && !discussionTopicErrors[index] ? (
<Form.Checkbox
key={`checkbox-${topic.id}`}
id={`checkbox-${topic.id}`}
@@ -143,21 +131,11 @@ function DivisionByGroupFields({
</TransitionReplace>
</>
);
}
};
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);

View File

@@ -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 }) => (
<div>
{
topics.map((topic, index) => (
<TopicItem
{...topic}
key={`topic-${topic.id}`}
index={index}
onDelete={() => handleTopicDelete(index, topic.id, remove)}
/>
))
}
{discussionTopics.map((topic, index) => (
<TopicItem
{...topic}
key={`topic-${topic.id}`}
index={index}
onDelete={() => handleTopicDelete(index, topic.id, remove)}
hasError={discussionTopicErrors[index]}
/>
))}
<div className="mb-4">
<Button
onClick={() => addNewTopic(push)}
@@ -86,6 +85,7 @@ const DiscussionTopics = ({ intl }) => {
DiscussionTopics.propTypes = {
intl: intlShape.isRequired,
discussionTopicErrors: PropTypes.arrayOf(PropTypes.bool).isRequired,
};
export default injectIntl(DiscussionTopics);

View File

@@ -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(
<AppProvider store={store}>
<IntlProvider locale="en">
<Formik
initialValues={data}
>
<DiscussionTopics />
<Formik initialValues={data}>
<DiscussionTopics discussionTopicErrors={discussionTopicErrors} />
</Formik>
</IntlProvider>
</AppProvider>,

View File

@@ -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 = (
<div className="h4 py-2 mr-auto">
{intl.formatMessage(messages.renameGeneralTopic)}
<div className="small text-muted mt-2">{intl.formatMessage(messages.generalTopicHelp)}</div>
<div className="small text-muted mt-2">
{intl.formatMessage(messages.generalTopicHelp)}
</div>
</div>
);
} else if (isOpen) {
heading = <span className="h4 py-2 mr-auto">{intl.formatMessage(messages.configureAdditionalTopic)}</span>;
heading = (
<span className="h4 py-2 mr-auto">
{intl.formatMessage(messages.configureAdditionalTopic)}
</span>
);
} else {
heading = <span className="py-2">{title}</span>;
heading = <span className="py-2">{name}</span>;
}
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') => (
<Form.Control.Feedback type={messageType} hasIcon={false}>
<div className="small">{message}</div>
</Form.Control.Feedback>
);
const handleFocusOut = (event) => {
handleBlur(event);
setInFocus(false);
};
const deleteTopicPopup = (
<Card className="rounded mb-3 p-1">
<Card.Body>
<div className="text-primary-500 mb-2 h4">
@@ -89,10 +79,7 @@ const TopicItem = ({
{intl.formatMessage(messages.discussionTopicDeletionHelp)}
</Card.Text>
<div className="d-flex justify-content-end">
<Button
variant="tertiary"
onClick={() => setShowDeletePopup(false)}
>
<Button variant="tertiary" onClick={() => setShowDeletePopup(false)}>
{intl.formatMessage(messages.cancelButton)}
</Button>
<Button
@@ -109,91 +96,92 @@ const TopicItem = ({
return (
<>
{
showDeletePopup ? (
deletetopic
) : (
<Collapsible.Advanced
className="collapsible-card rounded mb-3 px-3 py-2"
onToggle={handleToggle}
defaultOpen={!title}
open={collapseIsOpen}
id={id}
{showDeletePopup ? (
deleteTopicPopup
) : (
<Collapsible.Advanced
className="collapsible-card rounded mb-3 px-3 py-2"
onToggle={handleToggle}
defaultOpen={!name || hasError}
open={collapseIsOpen}
id={id}
>
<Collapsible.Trigger
className="collapsible-trigger d-flex border-0"
style={{ justifyContent: 'unset' }}
>
<Collapsible.Trigger
className="collapsible-trigger d-flex border-0"
style={{ justifyContent: 'unset' }}
>
<Collapsible.Visible whenClosed>
{getHeading(false)}
<div className="ml-auto">
<IconButton
alt={intl.formatMessage(messages.expandAltText)}
src={ExpandMore}
iconAs={Icon}
onClick={() => {}}
variant="dark"
/>
</div>
</Collapsible.Visible>
<Collapsible.Visible whenOpen>
{getHeading(true)}
{
!isGeneralTopic && (
<div className="pr-4 border-right">
<IconButton
onClick={deleteDiscussionTopic}
alt={intl.formatMessage(messages.deleteAltText)}
src={Delete}
iconAs={Icon}
variant="dark"
/>
</div>
)
}
<div className="pl-4">
<IconButton
alt={intl.formatMessage(messages.collapseAltText)}
src={ExpandLess}
iconAs={Icon}
onClick={() => {}}
variant="dark"
/>
</div>
</Collapsible.Visible>
</Collapsible.Trigger>
<Collapsible.Body className="collapsible-body rounded px-0">
<Form.Group
controlId={`discussionTopics.${index}.name`}
isInvalid={isInvalidTopicNameKey}
className="m-2"
>
<Form.Control
floatingLabel="Topic name"
onChange={handleChange}
onBlur={handleBlur}
value={name}
controlClassName="bg-white"
<Collapsible.Visible whenClosed>
{getHeading(false)}
<div className="ml-auto">
<IconButton
alt={intl.formatMessage(messages.expandAltText)}
src={ExpandMore}
iconAs={Icon}
onClick={() => {}}
variant="dark"
/>
{isInvalidTopicNameKey && (
<Form.Control.Feedback type="invalid" hasIcon={false}>
<div className="small">
{intl.formatMessage(messages.discussionTopicRequired)}
</div>
</Form.Control.Feedback>
</div>
</Collapsible.Visible>
<Collapsible.Visible whenOpen>
{getHeading(true)}
{!isGeneralTopic && (
<div className="pr-4 border-right">
<IconButton
onClick={deleteDiscussionTopic}
alt={intl.formatMessage(messages.deleteAltText)}
src={Delete}
iconAs={Icon}
variant="dark"
/>
</div>
)}
<div className="pl-4">
<IconButton
alt={intl.formatMessage(messages.collapseAltText)}
src={ExpandLess}
iconAs={Icon}
onClick={() => {}}
variant="dark"
/>
</div>
</Collapsible.Visible>
</Collapsible.Trigger>
<Collapsible.Body className="collapsible-body rounded px-0">
<Form.Group
controlId={`discussionTopics.${index}.name`}
isInvalid={hasError && !inFocus}
className="m-2"
>
<Form.Control
floatingLabel="Topic name"
onChange={handleChange}
onBlur={(event) => handleFocusOut(event)}
value={name}
controlClassName="bg-white"
onFocus={() => setInFocus(true)}
/>
<TransitionReplace key={id} className="mt-1">
{inFocus ? (
<React.Fragment key="open">
{renderFormFeedback(intl.formatMessage(messages.addTopicHelpText))}
</React.Fragment>
) : (
<React.Fragment key="closed" />
)}
{isExistingName && (
<Form.Control.Feedback type="invalid" hasIcon={false}>
<div className="small">
{intl.formatMessage(messages.discussionTopicNameAlreadyExist)}
</div>
</Form.Control.Feedback>
</TransitionReplace>
<TransitionReplace key={`${name}-${id}`}>
{hasError && !inFocus ? (
<React.Fragment key="open">
{renderFormFeedback(errors?.discussionTopics[index].name, 'invalid')}
</React.Fragment>
) : (
<React.Fragment key="closed" />
)}
</Form.Group>
</Collapsible.Body>
</Collapsible.Advanced>
)
}
</TransitionReplace>
</Form.Group>
</Collapsible.Body>
</Collapsible.Advanced>
)}
</>
);
};
@@ -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);

View File

@@ -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',