Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Bulk email recipient options #171

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/components/bulk-email-tool/BulkEmailTool.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,11 @@ export default function BulkEmailTool() {
</h1>
</div>
<div className="row">
<BulkEmailForm courseId={courseId} cohorts={courseMetadata.cohorts} />
<BulkEmailForm
courseId={courseId}
cohorts={courseMetadata.cohorts}
courseModes={courseMetadata.courseModes}
/>
</div>
<div className="row py-5">
<BulkEmailTaskManager courseId={courseId} />
Expand Down
14 changes: 13 additions & 1 deletion src/components/bulk-email-tool/bulk-email-form/BulkEmailForm.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,12 @@ const FORM_ACTIONS = {
};

function BulkEmailForm(props) {
const { courseId, cohorts, intl } = props;
const {
courseId,
cohorts,
courseModes,
intl,
} = props;
const [{ editor }, dispatch] = useContext(BulkEmailContext);
const [emailFormStatus, setEmailFormStatus] = useState(FORM_SUBMIT_STATES.DEFAULT);
const [emailFormValidation, setEmailFormValidation] = useState({
Expand Down Expand Up @@ -272,6 +277,7 @@ function BulkEmailForm(props) {
handleCheckboxes={onRecipientChange}
additionalCohorts={cohorts}
isValid={emailFormValidation.recipients}
courseModes={courseModes}
/>
<Form.Group controlId="emailSubject">
<Form.Label className="h3 text-primary-500">{intl.formatMessage(messages.bulkEmailSubjectLabel)}</Form.Label>
Expand Down Expand Up @@ -384,6 +390,12 @@ BulkEmailForm.propTypes = {
courseId: PropTypes.string.isRequired,
cohorts: PropTypes.arrayOf(PropTypes.string),
intl: intlShape.isRequired,
courseModes: PropTypes.arrayOf(
PropTypes.shape({
slug: PropTypes.string.isRequired,
name: PropTypes.string.isRequired,
}),
).isRequired,
};

export default injectIntl(BulkEmailForm);
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,13 @@ const DEFAULT_GROUPS = {
};

export default function BulkEmailRecipient(props) {
const { handleCheckboxes, selectedGroups, additionalCohorts } = props;
const {
handleCheckboxes,
selectedGroups,
additionalCohorts,
courseModes,
} = props;
const isCourseModes = courseModes && courseModes.length > 1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const isCourseModes = courseModes && courseModes.length > 1;
const hasCourseModes = courseModes && courseModes.length > 1;

return (
<Form.Group>
<Form.Label>
Expand Down Expand Up @@ -50,18 +56,24 @@ export default function BulkEmailRecipient(props) {
description="A selectable choice from a list of potential email recipients"
/>
</Form.Checkbox>
<Form.Checkbox
key="track:verified"
value="track:verified"
disabled={selectedGroups.find((group) => group === DEFAULT_GROUPS.ALL_LEARNERS)}
className="col col-lg-4 col-sm-6 col-12"
>
<FormattedMessage
id="bulk.email.form.recipients.verified"
defaultMessage="Learners in the verified certificate track"
description="A selectable choice from a list of potential email recipients"
/>
</Form.Checkbox>
{
// additional modes
isCourseModes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
isCourseModes
hasCourseModes

&& courseModes.map((courseMode) => (
<Form.Checkbox
key={`track:${courseMode.slug}`}
value={`track:${courseMode.slug}`}
disabled={selectedGroups.find((group) => group === DEFAULT_GROUPS.ALL_LEARNERS)}
className="col col-lg-4 col-sm-6 col-12"
>
<FormattedMessage
id="bulk.email.form.mode.label"
defaultMessage="Learners in the {courseModeName} Track"
values={{ courseModeName: courseMode.name }}
/>
</Form.Checkbox>
))
}
{
// additional cohorts
additionalCohorts
Expand All @@ -80,18 +92,6 @@ export default function BulkEmailRecipient(props) {
</Form.Checkbox>
))
}
<Form.Checkbox
key="track:audit"
value="track:audit"
disabled={selectedGroups.find((group) => group === DEFAULT_GROUPS.ALL_LEARNERS)}
className="col col-lg-4 col-sm-6 col-12"
>
<FormattedMessage
id="bulk.email.form.recipients.audit"
defaultMessage="Learners in the audit track"
description="A selectable choice from a list of potential email recipients"
/>
</Form.Checkbox>
<Form.Checkbox
key="learners"
value="learners"
Expand Down Expand Up @@ -127,4 +127,10 @@ BulkEmailRecipient.propTypes = {
handleCheckboxes: PropTypes.func.isRequired,
isValid: PropTypes.bool,
additionalCohorts: PropTypes.arrayOf(PropTypes.string),
courseModes: PropTypes.arrayOf(
PropTypes.shape({
slug: PropTypes.string.isRequired,
name: PropTypes.string.isRequired,
}),
).isRequired,
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import { Factory } from 'rosie'; // eslint-disable-line import/no-extraneous-dependencies

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to me that there is no need to write jsDocs for import.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a doc for the module, not for import

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would remove it, but I leave the decision up to you 👍


/**
* Generates an array of course mode objects using Rosie Factory.
* @returns {Array<Object>} An array of course mode objects with attributes 'slug' and 'name'.
*/
const courseModeFactory = () => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be a good idea to add some jsDocs documentation for this code. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. It has already been added. Check please and add any suggestions if needed.

const AuditModeFactory = Factory.define('AuditModeFactory')
.attr('slug', 'audit')
.attr('name', 'Audit');

const VerifiedModeFactory = Factory.define('VerifiedModeFactory')
.attr('slug', 'verified')
.attr('name', 'Verified Certificate');

return [
AuditModeFactory.build(),
VerifiedModeFactory.build(),
];
};

export default courseModeFactory;
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import * as bulkEmailFormApi from '../data/api';
import { BulkEmailContext, BulkEmailProvider } from '../../bulk-email-context';
import { formatDate } from '../../../../utils/formatDateAndTime';
import cohortFactory from '../data/__factories__/bulkEmailFormCohort.factory';
import courseModeFactory from '../data/__factories__/bulkEmailFormCourseMode.factory';

jest.mock('../../text-editor/TextEditor');

Expand All @@ -20,20 +21,25 @@ const dispatchMock = jest.fn();

const tomorrow = new Date();
tomorrow.setDate(new Date().getDate() + 1);
const courseMode = courseModeFactory();

function renderBulkEmailForm() {
const { cohorts } = cohortFactory.build();
return (
<BulkEmailProvider>
<BulkEmailForm courseId="test" cohorts={cohorts} />
<BulkEmailForm
courseId="test"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a suggestion: let's come up with a more meaningful name for this identifier 💯

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have any examples? I used the same name as in the backend.

Copy link

@PKulkoRaccoonGang PKulkoRaccoonGang Nov 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mockCourseId or exampleCourseId
Please, If it's possible

Copy link
Contributor Author

@Inferato Inferato Nov 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, if it is necessary. This ID was here before I made the changes to the tests so I didn't check this part for correctness

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided to not change courseId value to save the common style of test values.

cohorts={cohorts}
courseModes={courseMode}
/>
</BulkEmailProvider>
);
}

function renderBulkEmailFormContext(value) {
return (
<BulkEmailContext.Provider value={[value, dispatchMock]}>
<BulkEmailForm courseId="test" />
<BulkEmailForm courseId="test" courseMode={courseMode} />
</BulkEmailContext.Provider>
);
}
Expand Down Expand Up @@ -96,8 +102,8 @@ describe('bulk-email-form', () => {
test('Checking "All Learners" disables each learner group', async () => {
render(renderBulkEmailForm());
fireEvent.click(screen.getByRole('checkbox', { name: 'All Learners' }));
const verifiedLearners = screen.getByRole('checkbox', { name: 'Learners in the verified certificate track' });
const auditLearners = screen.getByRole('checkbox', { name: 'Learners in the audit track' });
const verifiedLearners = screen.getByRole('checkbox', { name: 'Learners in the Verified Certificate Track' });
const auditLearners = screen.getByRole('checkbox', { name: 'Learners in the Audit Track' });
const { cohorts } = cohortFactory.build();
cohorts.forEach(cohort => expect(screen.getByRole('checkbox', { name: `Cohort: ${cohort}` })).toBeDisabled());
expect(verifiedLearners).toBeDisabled();
Expand Down
3 changes: 2 additions & 1 deletion src/components/page-container/PageContainer.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ export default function PageContainer(props) {
}

const {
org, number, title, tabs, originalUserIsStaff,
org, number, title, tabs, originalUserIsStaff, courseModes,
} = metadataResponse;
const { cohorts } = cohortsResponse;

Expand All @@ -48,6 +48,7 @@ export default function PageContainer(props) {
number,
title,
originalUserIsStaff,
courseModes,
tabs: [...tabs],
cohorts: cohorts.map(({ name }) => name),
});
Expand Down