-
Notifications
You must be signed in to change notification settings - Fork 39
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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; | ||||||
return ( | ||||||
<Form.Group> | ||||||
<Form.Label> | ||||||
|
@@ -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 | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
&& 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 | ||||||
|
@@ -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" | ||||||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a doc for the module, not for import There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 = () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
---|---|---|
|
@@ -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'); | ||
|
||
|
@@ -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" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 💯 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I decided to not change |
||
cohorts={cohorts} | ||
courseModes={courseMode} | ||
/> | ||
</BulkEmailProvider> | ||
); | ||
} | ||
|
||
function renderBulkEmailFormContext(value) { | ||
return ( | ||
<BulkEmailContext.Provider value={[value, dispatchMock]}> | ||
<BulkEmailForm courseId="test" /> | ||
<BulkEmailForm courseId="test" courseMode={courseMode} /> | ||
</BulkEmailContext.Provider> | ||
); | ||
} | ||
|
@@ -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(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.