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.

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

Jump to bottom

feat: added edx-ace template and message type for email notifications #34315

Merged
merged 8 commits into from
Mar 19, 2024
Merged

feat: added edx-ace template and message type for email notifications #34315

merged 8 commits into from
Mar 19, 2024

Conversation

muhammadadeeltajamul
Copy link
Contributor

@muhammadadeeltajamul muhammadadeeltajamul commented Mar 1, 2024 •
edited
Loading

Description

Added edx-ace template and message type for email notifications.
Changes includes

  • Daily Digest Template
  • Weekly Digest Template

Note: Some of the code will update when it will be integrated with content code

Ticket: INF-1253

How to test

email_content = [
  {
      "title": "Updates", "help_text": "", "help_text_url": "",
      "content": [
          {"course_name": "course 1", "title": "Title 01", "time_ago": "1d"},
          {"course_name": "course 2", "title": "Title 02", "time_ago": "1w"},
      ]
  }
]

start_date = datetime.datetime.now()
recipient = Recipient(user_id, user_email)
course_language = _get_course_language(course_key)
message_context = create_email_digest_content(start_date=start_date, digest_frequency="Daily", notifications_count=25, updates_count=2, email_content=email_content)
message = EmailNotificationMessageType(app_label="notifications", name="email_digest").personalize(recipient, course_language, message_context)
send(message)

Screenshot

Screenshot 2024-03-15

saadyousafarbi
saadyousafarbi requested changes Mar 7, 2024
View reviewed changes
Copy link
Contributor

@saadyousafarbi saadyousafarbi left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I have added some questions/suggestions!

lms/envs/common.py Outdated
NOTIFICATIONS_EXPIRY = 60
EXPIRED_NOTIFICATIONS_DELETE_BATCH_SIZE = 10000
NOTIFICATION_CREATION_BATCH_SIZE = 99
NOTIFICATIONS_DEFAULT_FROM_EMAIL = "[email protected]"
NOTIFICATION_TYPE_ICONS = {}
DEFAULT_NOTIFICATION_ICON_URL = "https://edx-notifications-static.edx.org/icons/newspaper.png"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we have a NOTIFICATION_STATIC_ASSETS_ROOT_URL, and extend that for this URL!

Something like NOTIFICATION_STATIC_ASSETS_ROOT_URL/icons/newspaper.png

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. Will do that

openedx/core/djangoapps/notifications/email/utils.py
return datetime_instance.strftime('%A, %b %d')


def get_icon_url_for_notification_type(notification_type):
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need this method when we already have the get icon method in NotificationTypeIcons class

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 can remove it. I thought of making all methods available in utils.py.

openedx/core/djangoapps/notifications/email/utils.py Outdated
Comment on lines 34 to 40
return {
"platform_name": settings.PLATFORM_NAME,
"logo_url": get_logo_url_for_email(),
"social_media": social_media_info,
"notification_settings_url": f"{settings.ACCOUNT_MICROFRONTEND_URL}/notifications",
}
Copy link
Contributor

Choose a reason for hiding this comment

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

is this all that is required? Do we have some requirements from legal to include stuff to the footer, or email body in general?

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 have added these from the figma templates (values from bulk_email)

openedx/core/djangoapps/notifications/email/utils.py
Comment on lines +50 to +52
start_date_str = create_datetime_string(start_date)
end_date_str = create_datetime_string(end_date if end_date else start_date)
Copy link
Contributor

Choose a reason for hiding this comment

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

are these going to be UTC? What time will the user receive these digest emails?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The task will run at 00:00 UTC but timezone is not needed for creating string. Format for string is Monday, Jun 12

openedx/core/djangoapps/notifications/email/utils.py
Comment on lines +44 to +48
"""
Creates email context based on content
start_date: datetime instance
end_date: datetime instance
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please update DocString with some more details, including the optional params

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 will be updated on integration.

openedx/core/djangoapps/notifications/email/utils.py
"start_date": start_date_str,
"end_date": end_date_str,
"digest_frequency": digest_frequency,
"updates": [
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we rename this updates. We already have a notification app with this name, might cause confusion!
Something like email_digest_updates might be clearer!

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 will be updated when it will be integrated to send email. There are some ambiguities where it is being used.

openedx/core/djangoapps/notifications/templates/notifications/digest_footer.html Outdated
Comment on lines 1 to 48
<table cellpadding="0" cellspacing="0" style="padding:1.875rem; background-color:#F2F0EF; color:black; font-weight:400; font-size:0.75rem;line-height:20px" width="100%">
<tbody>
<tr>
<td>
<table width="100%">
<tbody>
<tr>
<td width="20%" align="left">
<img src="{{ logo_url }}" style="width: auto;" height="24" alt="Logo"/></a>
</td>
<td width="80%" align="right">
<table>
<tbody>
<tr>
{% for social_name, social_data in social_media.items %}
<td>
<a href="{{social_data.url}}">
<img src="{{social_data.icon}}" style="max-height: 24px; max-width: 24px" alt="{{social_name}}" />
</a>
</td>
{% endfor %}
</tr>
</tbody>
</table>
</td>
</tr>
</tbody>
</table>
</td>
</tr>
<tr>
<td>
<p>
You are receiving this email because you have subscribed to email digest
</p>
<p>
<a href="{{notification_settings_url}}" rel="noopener noreferrer" target="_blank" style="color: black">
Notification Settings
</a>
</p>
<p>
&copy; 2023 edX LLC. All Rights Reserved <br/>
7900 Harkins Road, Lanham MD 20706
</p>
</td>
</tr>
</tbody>
</table>
Copy link
Contributor

Choose a reason for hiding this comment

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

how does this footer compare to other ace footers in platform? Is it similar, or designed completely from scratch?

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 footer is designed from scratch.

...core/djangoapps/notifications/templates/notifications/edx_ace/email_digest/email/subject.txt
@@ -0,0 +1 @@
{{ digest_frequency }} Notifications Digest for {% if digest_frequency == "Weekly" %}the Week of {% endif %}{{ start_date }}
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens here for Daily Digest?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For daily digest: Daily Notifications Digest for Jun 12
For weekly digest: Weekly Notifications Digest for the week of Jun 12

saadyousafarbi
saadyousafarbi approved these changes Mar 12, 2024
View reviewed changes
Copy link
Contributor

@saadyousafarbi saadyousafarbi left a comment

Choose a reason for hiding this comment

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

the changes look good to me for an initial setup of the ace emails!
Please make a note of all pending items that would need to be updated on integration, so we don't miss anything later!
Thanks!

@muhammadadeeltajamul muhammadadeeltajamul merged commit 692965a into master Mar 19, 2024
67 of 68 checks passed
@muhammadadeeltajamul muhammadadeeltajamul deleted the inf-1253 branch March 19, 2024 08:55
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

1 similar comment
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@awais-ansari awais-ansari awais-ansari approved these changes

@saadyousafarbi saadyousafarbi saadyousafarbi approved these changes

@AhtishamShahid AhtishamShahid AhtishamShahid approved these changes

@asadazam93 asadazam93 Awaiting requested review from asadazam93

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@muhammadadeeltajamul @edx-pipeline-bot @AhtishamShahid @saadyousafarbi @awais-ansari