feat: same titles for sent email groups#259
Conversation
openedx-webhooks
added
open-source-contribution
|
Thanks for the pull request, @ihor-romaniuk! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo 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:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere 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:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #259 +/- ##
==========================================
+ Coverage 84.14% 84.29% +0.15%
==========================================
Files 47 50 +3
Lines 700 707 +7
Branches 135 136 +1
==========================================
+ Hits 589 596 +7
Misses 111 111 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
brian-smith-tcril
left a comment
brian-smith-tcril
left a comment
There was a problem hiding this comment.
I have not tested this but the code LGTM.
I left one comment with a question about a CSS class being added.
| </div> | ||
| <div className="d-flex flex-row"> | ||
| <p>{intl.formatMessage(messages.modalMessageSentTo)}</p> | ||
| <p className="flex-shrink-0">{intl.formatMessage(messages.modalMessageSentTo)}</p> |
There was a problem hiding this comment.
I don't see anything in the PR description about this change. Could you explain why this change is needed?
There was a problem hiding this comment.
I have re-checked it and we can revert this class. Thanks
ihor-romaniuk
added
the
create-sandbox
|
Sandbox deployment successful 🚀 |
ihor-romaniuk
force-pushed
the
romaniuk/different-titles-for-email-groups
branch
from
9ad4092 to
323af89
Compare
|
Sandbox deployment successful 🚀 |
|
Sandbox deployment successful 🚀 |
|
Sandbox deployment successful 🚀 |
peterkulko
left a comment
peterkulko
left a comment
There was a problem hiding this comment.
@ihor-romaniuk thanks for the PR! I left a minor comment and one question about the package-lock.json changes.
| * @returns {string} - The formatted display name for the recipient, | ||
| * or the original recipient key if no display name is found. | ||
| */ | ||
| // eslint-disable-next-line import/prefer-default-export |
There was a problem hiding this comment.
[optional]: We could export RECIPIENTS_DISPLAY_NAMES from this file to avoid disabling the import/prefer-default-export eslint rule.
Alternatively, we could move RECIPIENTS_DISPLAY_NAMES to a constants file since it’s a constant, and keep the rule disabled.
There was a problem hiding this comment.
Done - I exported RECIPIENTS_DISPLAY_NAMES from this file and removed the import/prefer-default-export disable since it’s no longer needed. Thank you.
There was a problem hiding this comment.
[important]: Why do we have these changes?
There was a problem hiding this comment.
Yep - those changes were required. I only updated package-lock.json because it was out of date, and the PR checks were failing because of it. No functional changes intended there.
There was a problem hiding this comment.
I have reverted changes refarding the package-lock.json file because we have already merged commit in master branch 4a34632
|
Sandbox deployment failed 💥 |
|
Sandbox deployment failed 💥 |
|
Sandbox deployment successful 🚀 |
kdmccormick
removed
the
create-sandbox
|
I've deleted this PR's sandbox but feel free to create it by adding the |
arbrandes
left a comment
arbrandes
left a comment
There was a problem hiding this comment.
Ok, some stuff Claude caught that I agree warrants addressing. Only the first one is an actual bug, though:
ViewEmailModal for scheduled emails still shows raw identifiers
In BulkEmailScheduledEmailsTable.jsx, the handleViewEmail function passes row.original.targets to the ViewEmailModal:
sent_to: row.original.targets,targets is the raw joined string (e.g., "learners" or "staff"), not the display-friendly targetsText (e.g., "All learners" or "Staff and instructors"). The table column was correctly switched to use targetsText at line 160, but the View modal was missed.
This means: clicking "View" on a scheduled email will display "learners" in the "Sent to" field while the table row next to it says "All learners". This partially defeats the PR's purpose of making display names consistent across all views.
The fix is straightforward - use targetsText instead of targets on that line.
Changed i18n message IDs for "Myself" label
The "Myself" label text didn't change, but its message ID was renamed from bulk.email.form.recipients.myself to bulk.email.recipients.myself.label. This means any existing translations for that string will be lost and fall back to the English default until re-translated.
For "Staff/Administrators" and "All Learners", new IDs make sense since the English text actually changed. For "Myself", keeping the old message ID (or extracting it to the shared file under the same ID) would preserve existing translations.
This may or may not matter depending on how actively translations are maintained for this MFE, but it's worth being aware of.
Nit: unclear variable name
In BulkEmailScheduledEmailsTable.jsx line 33:
targetsText: email.courseEmail.targets.map((mess) => getDisplayTextFromRecipient(intl, mess)).join(', '),The parameter mess reads like it might mean "message", but these items are recipient identifiers. Something like recipient or target would be clearer.
@arbrandes Thanks for the review! All three items are addressed in the latest commit:
Please re-check when you get a chance, and go forward if everything looks good. |
|
@ihor-romaniuk, almost there! All issues address, thank you - but package-lock.json is out of sync with package.json, so |
Co-authored-by: Braden MacDonald <mail@bradenm.com>
- Use targetsText in ViewEmailModal so scheduled emails show display-friendly names instead of raw identifiers - Restore original i18n message ID for "Myself" to preserve existing translations - Rename unclear `mess` parameter to `recipient`
ihor-romaniuk
force-pushed
the
romaniuk/different-titles-for-email-groups
branch
from
7b859d9 to
40adfae
Compare
ihor-romaniuk
force-pushed
the
romaniuk/different-titles-for-email-groups
branch
from
40adfae to
637b9cf
Compare
|
@arbrandes it looks like the package-lock file was not updated to use node 24. I've rebuild the package-lock.json file and now all CI checkers pass. |
8 participants
Reopened Pull Request based on #205
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)
Steps to reproduce
click on "Staff/Administrators" checkbox -> complete the form (subject, body) -> options:
(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
Screenshots before
Example with selected "Staff/Administrators"
Example with selected "All Learners"
Screenshots after