Skip to content

feat: same titles for sent email groups #205

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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

feat: same titles for sent email groups #205

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a3da16d
feat: same titles for sent email groups
Jul 3, 2024
bb21584
feat: aligned the checkbox labels with the BE
Jul 4, 2024
62debac
fix: refactor the code
ihor-romaniuk Sep 9, 2024
a2f8f52
fix: add fall back to the recipient key
ihor-romaniuk Dec 3, 2024
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
5 changes: 3 additions & 2 deletions src/components/bulk-email-tool/bulk-email-form/BulkEmailForm.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import {
} from './data/actions';
import { editScheduledEmailThunk, postBulkEmailThunk } from './data/thunks';
import { getScheduledBulkEmailThunk } from '../bulk-email-task-manager/bulk-email-scheduled-emails-table/data/thunks';
import { getDisplayTextFromRecipient } from '../utils';

import './bulkEmailForm.scss';

Expand Down Expand Up @@ -219,7 +220,7 @@ function BulkEmailForm(props) {
<p>{intl.formatMessage(messages.bulkEmailTaskAlertRecipients, { subject: editor.emailSubject })}</p>
<ul className="list-unstyled">
{editor.emailRecipients.map((group) => (
<li key={group}>{group}</li>
<li key={group}>{getDisplayTextFromRecipient(group)}</li>
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not going to be internationalized, if I'm understanding this correctly.

You need to either:

  1. Make getDisplayTextFromRecipient(group) return a <FormattedMessage /> component, or
  2. Make getDisplayTextFromRecipient(group) return a MessageDescriptor and then pass that to intl.formatMessage(...) as seen three lines above.

Copy link
Contributor

Choose a reason for hiding this comment

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

I completely agree with you. Correct wrapper for localization has been added.

))}
</ul>
{!isScheduled && (
Expand All @@ -246,7 +247,7 @@ function BulkEmailForm(props) {
<p>{intl.formatMessage(messages.bulkEmailTaskAlertEditingTo)}</p>
<ul className="list-unstyled">
{editor.emailRecipients.map((group) => (
<li key={group}>{group}</li>
<li key={group}>{getDisplayTextFromRecipient(group)}</li>
))}
</ul>
<p>{intl.formatMessage(messages.bulkEmailTaskAlertEditingWarning)}</p>
Expand Down
7 changes: 4 additions & 3 deletions src/components/bulk-email-tool/bulk-email-form/bulk-email-recipient/BulkEmailRecipient.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import React from 'react';
import PropTypes from 'prop-types';
import { Form } from '@openedx/paragon';
import { FormattedMessage } from '@edx/frontend-platform/i18n';
import { RECIPIENTS_DISPLAY_NAMES } from '../../utils';

import './bulkEmailRecepient.scss';

Expand Down Expand Up @@ -41,7 +42,7 @@ export default function BulkEmailRecipient(props) {
<Form.Checkbox key="myself" value="myself" className="mt-2.5 col col-lg-4 col-sm-6 col-12">
<FormattedMessage
id="bulk.email.form.recipients.myself"
defaultMessage="Myself"
defaultMessage={RECIPIENTS_DISPLAY_NAMES.myself}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure but I think setting defaultMessage to a constant like this is going to break the extraction of strings for localization. In addition, it looks confusing because it looks like it's maybe a dynamic defaultMessage. Instead, all parts of this message descriptor (the id, the defaultMessage, and the description) should be put into a messages file and then you can just reference it here as <FormattedMessage {...messages.recipientMyself} />

Copy link
Contributor

Choose a reason for hiding this comment

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

Refactored according to new solution

description="A selectable choice from a list of potential email recipients"
/>
</Form.Checkbox>
Expand All @@ -52,7 +53,7 @@ export default function BulkEmailRecipient(props) {
>
<FormattedMessage
id="bulk.email.form.recipients.staff"
defaultMessage="Staff/Administrators"
defaultMessage={RECIPIENTS_DISPLAY_NAMES.staff}
description="A selectable choice from a list of potential email recipients"
/>
</Form.Checkbox>
Expand Down Expand Up @@ -99,7 +100,7 @@ export default function BulkEmailRecipient(props) {
>
<FormattedMessage
id="bulk.email.form.recipients.learners"
defaultMessage="All Learners"
defaultMessage={RECIPIENTS_DISPLAY_NAMES.learners}
description="A selectable choice from a list of potential email recipients"
/>
</Form.Checkbox>
Expand Down
13 changes: 7 additions & 6 deletions src/components/bulk-email-tool/bulk-email-form/test/BulkEmailForm.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ 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';
import { RECIPIENTS_DISPLAY_NAMES } from '../../utils';

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

Expand Down Expand Up @@ -75,8 +76,8 @@ describe('bulk-email-form', () => {
success: true,
});
render(renderBulkEmailForm());
fireEvent.click(screen.getByRole('checkbox', { name: 'Myself' }));
expect(screen.getByRole('checkbox', { name: 'Myself' })).toBeChecked();
fireEvent.click(screen.getByRole('checkbox', { name: RECIPIENTS_DISPLAY_NAMES.myself }));
expect(screen.getByRole('checkbox', { name: RECIPIENTS_DISPLAY_NAMES.myself })).toBeChecked();
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: It's fine to hard-code strings in test files; it makes it easier to see when you've accidentally changed something, like accidentally setting the value of RECIPIENTS_DISPLAY_NAMES.myself to "Learners".

Copy link
Contributor

Choose a reason for hiding this comment

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

Make sense, thank you for notice that. I'll revert these changes for tests.

fireEvent.change(screen.getByRole('textbox', { name: 'Subject' }), { target: { value: 'test subject' } });
fireEvent.change(screen.getByTestId('textEditor'), { target: { value: 'test body' } });
fireEvent.click(screen.getByText('Send email'));
Expand All @@ -90,7 +91,7 @@ describe('bulk-email-form', () => {
axiosMock.onPost(`${getConfig().LMS_BASE_URL}/courses/test/instructor/api/send_email`).reply(500);
render(renderBulkEmailForm());
const subjectLine = screen.getByRole('textbox', { name: 'Subject' });
const recipient = screen.getByRole('checkbox', { name: 'Myself' });
const recipient = screen.getByRole('checkbox', { name: RECIPIENTS_DISPLAY_NAMES.myself });
fireEvent.click(recipient);
fireEvent.change(subjectLine, { target: { value: 'test subject' } });
fireEvent.change(screen.getByTestId('textEditor'), { target: { value: 'test body' } });
Expand All @@ -99,9 +100,9 @@ describe('bulk-email-form', () => {
fireEvent.click(await screen.findByRole('button', { name: /continue/i }));
expect(await screen.findByText('An error occured while attempting to send the email.')).toBeInTheDocument();
});
test('Checking "All Learners" disables each learner group', async () => {
test('Checking "All Students" disables each learner group', async () => {
render(renderBulkEmailForm());
fireEvent.click(screen.getByRole('checkbox', { name: 'All Learners' }));
fireEvent.click(screen.getByRole('checkbox', { name: RECIPIENTS_DISPLAY_NAMES.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 { cohorts } = cohortFactory.build();
Expand Down Expand Up @@ -130,7 +131,7 @@ describe('bulk-email-form', () => {
test('Adds scheduling data to POST requests when schedule is selected', async () => {
const postBulkEmailInstructorTask = jest.spyOn(bulkEmailFormApi, 'postBulkEmailInstructorTask');
render(renderBulkEmailForm());
fireEvent.click(screen.getByRole('checkbox', { name: 'Myself' }));
fireEvent.click(screen.getByRole('checkbox', { name: RECIPIENTS_DISPLAY_NAMES.myself }));
fireEvent.change(screen.getByRole('textbox', { name: 'Subject' }), { target: { value: 'test subject' } });
fireEvent.change(screen.getByTestId('textEditor'), { target: { value: 'test body' } });
const scheduleCheckbox = screen.getByText('Schedule this email for a future date');
Expand Down
2 changes: 1 addition & 1 deletion src/components/bulk-email-tool/bulk-email-task-manager/ViewEmailModal.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ function ViewEmailModal({
<p className="pl-2">{messageContent.created}</p>
</div>
<div className="d-flex flex-row">
<p>{intl.formatMessage(messages.modalMessageSentTo)}</p>
<p className="flex-shrink-0">{intl.formatMessage(messages.modalMessageSentTo)}</p>
<p className="pl-2">{messageContent.sent_to}</p>
</div>
<hr className="py-2" />
Expand Down
11 changes: 6 additions & 5 deletions ...lk-email-task-manager/bulk-email-scheduled-emails-table/BulkEmailScheduledEmailsTable.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import ViewEmailModal from '../ViewEmailModal';
import { copyToEditor } from '../../bulk-email-form/data/actions';
import TaskAlertModal from '../../task-alert-modal';
import { formatDate, formatTime } from '../../../../utils/formatDateAndTime';
import { getDisplayTextFromRecipient, getRecipientFromDisplayText } from '../../utils';

function flattenScheduledEmailsArray(emails) {
return emails.map((email) => ({
Expand All @@ -28,11 +29,11 @@ function flattenScheduledEmailsArray(emails) {
taskDue: new Date(email.taskDue).toLocaleString(),
taskDueUTC: email.taskDue,
...email.courseEmail,
targets: email.courseEmail.targets.join(', '),
targets: email.courseEmail.targets.map(getDisplayTextFromRecipient).join(', '),
}));
}

function BulkEmailScheduledEmailsTable({ intl }) {
function BulkEmailScheduledEmailsTable({ intl, courseModes }) {
const { courseId } = useParams();
const [{ scheduledEmailsTable }, dispatch] = useContext(BulkEmailContext);
const [tableData, setTableData] = useState([]);
Expand All @@ -44,8 +45,8 @@ function BulkEmailScheduledEmailsTable({ intl }) {
const [currentTask, setCurrentTask] = useState({});

useEffect(() => {
setTableData(flattenScheduledEmailsArray(scheduledEmailsTable.results));
}, [scheduledEmailsTable.results]);
setTableData(flattenScheduledEmailsArray(scheduledEmailsTable.results, courseModes));
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand the addition of courseModes to this table. It's not mentioned in the PR description nor seems to be used anywhere ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I also didn't find usage of courseModes and remove it here.

}, [scheduledEmailsTable.results, courseModes]);

const fetchTableData = useCallback((args) => {
dispatch(getScheduledBulkEmailThunk(courseId, args.pageIndex + 1));
Expand Down Expand Up @@ -96,7 +97,7 @@ function BulkEmailScheduledEmailsTable({ intl }) {
},
} = row;
const dateTime = new Date(taskDueUTC);
const emailRecipients = targets.replaceAll('-', ':').split(', ');
const emailRecipients = targets.replaceAll('-', ':').split(', ').map(getRecipientFromDisplayText);
Copy link
Contributor

Choose a reason for hiding this comment

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

Reversing the "display text" like this is not a very robust solution. Can you just modify flattenScheduledEmailsArray so that it stores both the email.courseEmail.targets IDs and the display text on row.original ? Then you won't have to do this reversing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Simplified by adding a targetsText values to the flattenScheduledEmailsArray data

const scheduleDate = formatDate(dateTime);
const scheduleTime = formatTime(dateTime);
dispatch(
Expand Down
3 changes: 2 additions & 1 deletion ...ask-manager/bulk-email-scheduled-emails-table/test/BulkEmailScheduledEmailsTable.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import { BulkEmailProvider } from '../../../bulk-email-context';
import BulkEmailScheduledEmailsTable from '..';
import scheduledEmailsFactory from './__factories__/scheduledEmails.factory';
import * as actions from '../../../bulk-email-form/data/actions';
import { RECIPIENTS_DISPLAY_NAMES } from '../../../utils';

jest.mock('react-router-dom', () => ({
...jest.requireActual('react-router-dom'),
Expand Down Expand Up @@ -42,7 +43,7 @@ describe('BulkEmailScheduledEmailsTable', () => {
.onGet(`${getConfig().LMS_BASE_URL}/api/instructor_task/v1/schedules/test-id/bulk_email/?page=1`)
.reply(200, scheduledEmailsFactory.build(1));
render(renderBulkEmailScheduledEmailsTable());
expect(await screen.findByText('learners')).toBeTruthy();
expect(await screen.findByText(RECIPIENTS_DISPLAY_NAMES.learners)).toBeTruthy();
expect(await screen.findByText('subject')).toBeTruthy();
expect(await screen.findByText('edx')).toBeTruthy();
expect(await screen.findByLabelText('View')).toBeTruthy();
Expand Down
14 changes: 14 additions & 0 deletions src/components/bulk-email-tool/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
export const RECIPIENTS_DISPLAY_NAMES = {
myself: 'Myself',
staff: 'Staff and instructors',
learners: 'All students',
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we generally prefer the term "Learner" to "Student" so this seems like a step backwards ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure. It should be Learner instead of Student

};

// Output: { 'Myself': 'myself', 'Staff and instructors': 'staff', 'All students': 'learners' }
export const REVERSE_RECIPIENTS_DISPLAY_NAMES = Object.fromEntries(
Object.entries(RECIPIENTS_DISPLAY_NAMES).map(([key, value]) => [value, key]),
);

export const getDisplayTextFromRecipient = (recipient) => RECIPIENTS_DISPLAY_NAMES[recipient] || recipient;

export const getRecipientFromDisplayText = (recipient) => REVERSE_RECIPIENTS_DISPLAY_NAMES[recipient] || recipient;