feat: same titles for sent email groups #205

khudym · 2024-07-03T21:34:37Z

Description

this PR proposes making the display of mail recipients the same for all places (checkbox labels, Send email alert modal, Scheduled emails table with View Modal, Email Task History)

Related Pull Requests
PR to the open-release/redwood.master branch: #207
PR to the open-release/quince.master branch: #206

Steps to reproduce

click on "Staff/Administrators" checkbox -> complete the form (subject, body) -> options:

to see recipients titles in "Caution" modal, click on "Send email" button
same if you schedule day and time and click on "Schedule email" button
(in this modal you can see the difference between recipients titles and selected checkbox label)

-> click "Continue" button -> wait for the "Scheduled emails" table to load and also compare titles in "Send to" column

BEFORE

Example with selected "Staff/Administrators"

Example with selected "All Learners"

AFTER

openedx-webhooks · 2024-07-03T21:34:42Z

Thanks for the pull request, @khudym!

What's next?

Please work through the following steps to get your changes ready for engineering review:

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
- This process (including the steps you'll need to take) is documented here.
If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

Dependencies

This PR must be merged before / after / at the same time as ...
Blockers

This PR is waiting for OEP-1234 to be accepted.
Timeline information

This PR must be merged by XX date because ...
Partner information

This is for a course on edx.org.
Supporting documentation
Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

🔘 Let us know that your PR is ready for review:

Who will review my changes?

This repository is currently maintained by @openedx/committers-frontend. Tag them in a comment and let them know that your changes are ready for review.

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

The size and impact of the changes that it introduces
The need for product review
Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

codecov · 2024-07-03T22:17:18Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.40%. Comparing base (10f906b) to head (62debac).
Report is 5 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #205      +/-   ##
==========================================
+ Coverage   83.26%   83.40%   +0.14%     
==========================================
  Files          47       50       +3     
  Lines         693      699       +6     
  Branches      135      134       -1     
==========================================
+ Hits          577      583       +6     
  Misses        116      116

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bradenmacdonald

I'm not very familiar with this code, but I took a pass over this for you. In the future, I'd recommend waiting until the master PR has been reviewed and merged before you open the backport PRs.

bradenmacdonald · 2024-07-25T17:08:22Z

src/components/bulk-email-tool/bulk-email-form/BulkEmailForm.jsx

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


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

You need to either:

Make getDisplayTextFromRecipient(group) return a <FormattedMessage /> component, or

Make getDisplayTextFromRecipient(group) return a MessageDescriptor and then pass that to intl.formatMessage(...) as seen three lines above.

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

bradenmacdonald · 2024-07-25T17:11:04Z

src/components/bulk-email-tool/bulk-email-form/bulk-email-recipient/BulkEmailRecipient.jsx

@@ -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}


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} />

Refactored according to new solution

bradenmacdonald · 2024-07-25T17:11:51Z

src/components/bulk-email-tool/utils.js

+export const RECIPIENTS_DISPLAY_NAMES = {
+  myself: 'Myself',
+  staff: 'Staff and instructors',
+  learners: 'All students',


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

Sure. It should be Learner instead of Student

bradenmacdonald · 2024-07-25T17:12:44Z

src/components/bulk-email-tool/bulk-email-form/test/BulkEmailForm.test.jsx

-    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();


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".

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

bradenmacdonald · 2024-07-25T17:17:02Z

.../bulk-email-task-manager/bulk-email-scheduled-emails-table/BulkEmailScheduledEmailsTable.jsx

@@ -44,8 +45,8 @@ function BulkEmailScheduledEmailsTable({ intl }) {
  const [currentTask, setCurrentTask] = useState({});

  useEffect(() => {
-    setTableData(flattenScheduledEmailsArray(scheduledEmailsTable.results));
-  }, [scheduledEmailsTable.results]);
+    setTableData(flattenScheduledEmailsArray(scheduledEmailsTable.results, courseModes));


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 ?

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

bradenmacdonald · 2024-07-25T17:19:58Z

.../bulk-email-task-manager/bulk-email-scheduled-emails-table/BulkEmailScheduledEmailsTable.jsx

@@ -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);


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.

Simplified by adding a targetsText values to the flattenScheduledEmailsArray data

mphilbrick211 · 2024-08-26T20:05:51Z

@khudym hi there! Just checking in to see if this is still in-progress?

ihor-romaniuk · 2024-09-09T10:35:29Z

@mphilbrick211 The code has been refactored according to the comments after the review. Thank you for your friendly ping.

bradenmacdonald

Thanks for those changes. This looks fine to me (besides one last suggestion), but I'm not familiar with this app and don't know how to test it. @mphilbrick211 Is someone else able to finish this review? I was just trying to help with some suggestions, not take on the review myself.

bradenmacdonald · 2024-11-19T21:44:30Z

src/components/bulk-email-tool/utils.js

+ */
+// eslint-disable-next-line import/prefer-default-export
+export const getDisplayTextFromRecipient = (intl, recipient) => (
+  intl.formatMessage(RECIPIENTS_DISPLAY_NAMES[recipient]) || recipient


Did you test this? I'm thinking that intl.formatMessage is going to throw an error if passed undefined, so I would write this as

Suggested change

intl.formatMessage(RECIPIENTS_DISPLAY_NAMES[recipient]) || recipient

const msg = RECIPIENTS_DISPLAY_NAMES[recipient];

return msg ? intl.formatMessage(msg) : recipient; // Fall back to the recipient key if no display name is found.

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Jul 3, 2024

feat: same titles for sent email groups

a3da16d

khudym force-pushed the hudym/different-titles-for-email-groups branch from e6c995f to a3da16d Compare July 3, 2024 22:12

feat: aligned the checkbox labels with the BE

bb21584

khudym marked this pull request as ready for review July 4, 2024 16:42

This was referenced Jul 5, 2024

feat: same titles for sent email groups #206

Open

feat: same titles for sent email groups #207

Open

mphilbrick211 requested a review from a team July 16, 2024 20:22

bradenmacdonald requested changes Jul 25, 2024

View reviewed changes

mphilbrick211 added the changes requested label Aug 26, 2024

fix: refactor the code

62debac

bradenmacdonald reviewed Nov 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: same titles for sent email groups #205

feat: same titles for sent email groups #205

khudym commented Jul 3, 2024 •

edited

Loading

openedx-webhooks commented Jul 3, 2024 •

edited

Loading

codecov bot commented Jul 3, 2024 •

edited

Loading

bradenmacdonald left a comment

bradenmacdonald Jul 25, 2024

ihor-romaniuk Sep 9, 2024

bradenmacdonald Jul 25, 2024

ihor-romaniuk Sep 9, 2024

bradenmacdonald Jul 25, 2024

ihor-romaniuk Sep 9, 2024

bradenmacdonald Jul 25, 2024

ihor-romaniuk Sep 9, 2024

bradenmacdonald Jul 25, 2024

ihor-romaniuk Sep 9, 2024

bradenmacdonald Jul 25, 2024

ihor-romaniuk Sep 9, 2024

mphilbrick211 commented Aug 26, 2024

ihor-romaniuk commented Sep 9, 2024

bradenmacdonald left a comment

bradenmacdonald Nov 19, 2024

	intl.formatMessage(RECIPIENTS_DISPLAY_NAMES[recipient]) \|\| recipient
	const msg = RECIPIENTS_DISPLAY_NAMES[recipient];
	return msg ? intl.formatMessage(msg) : recipient; // Fall back to the recipient key if no display name is found.

feat: same titles for sent email groups #205

Are you sure you want to change the base?

feat: same titles for sent email groups #205

Conversation

khudym commented Jul 3, 2024 • edited Loading

Description

Steps to reproduce

BEFORE

AFTER

openedx-webhooks commented Jul 3, 2024 • edited Loading

What's next?

🔘 Get product approval

🔘 Provide context

🔘 Get a green build

🔘 Let us know that your PR is ready for review:

Who will review my changes?

Where can I find more information?

When can I expect my changes to be merged?

codecov bot commented Jul 3, 2024 • edited Loading

Codecov Report

bradenmacdonald left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mphilbrick211 commented Aug 26, 2024

ihor-romaniuk commented Sep 9, 2024

bradenmacdonald left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khudym commented Jul 3, 2024 •

edited

Loading

openedx-webhooks commented Jul 3, 2024 •

edited

Loading

codecov bot commented Jul 3, 2024 •

edited

Loading