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.

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

feat: add watchers to the courses #4013

Merged
merged 1 commit into from
Jul 18, 2023
Merged

feat: add watchers to the courses #4013

merged 1 commit into from
Jul 18, 2023

Conversation

AfaqShuaib09
Copy link
Contributor

@AfaqShuaib09 AfaqShuaib09 commented Jul 10, 2023

PROD-3412

This PR adds the watchers field to Course model and adds the watchers field to the CourseSerializer to add the watchers field to the API response.
This PR also enables email notification for the watchers of the course when the Course Run of the course is published or published.

Testing:

  • Run course-discovery, frontend-app-publisher, studio and ecommerce locally.
  • Activate discovery shell make shell-discovery
  • Run make requirements to install the requirements for django-multi-email-field
  • Run ./manage.py migrate course_metadata to migrate the changes.
  • Add watchers of the course in the Course model through the Django admin.
  • Check if wachers field is present in the API response of the course.
  • To check the email notification, you can clone this frontend PR (feat: add watchers field to course edit form frontend-app-publisher#889) (Currently WIP only need to replace text field with multi email field).
  • Fill watchers field with the email comma separated email ids.
  • Publish & Schedule the course run of the course through the publisher. Add the pdb.set_trace() in the send_email_to_watchers to check if the email method is called. Because on local it will produce error due to missing email credentials.

Screenshots:

Email Template:
(Write HTML file on local as it will give credential error in sending email function)
image

@@ -2519,6 +2527,8 @@ def handle_status_change(self, send_emails):

if send_emails and email_method:
email_method(self)
if (self.course.watchers and (self.status in [CourseRunStatus.Reviewed, CourseRunStatus.Published])):
emails.send_email_for_course_url(self.course, self.go_live_date, self.status)
Copy link
Contributor

Choose a reason for hiding this comment

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

Once you consolidate the email methods, update it here accordingly.

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've combined both email methods, so no need to update it.

Copy link
Contributor

@DawoudSheraz DawoudSheraz left a comment

Choose a reason for hiding this comment

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

I have left some nits & comments to address.

Copy link
Contributor

@DawoudSheraz DawoudSheraz left a comment

Choose a reason for hiding this comment

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

I have left some more followup comments.

Copy link
Contributor

@DawoudSheraz DawoudSheraz left a comment

Choose a reason for hiding this comment

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

  • Some small nits to address
  • I have not tested it out on the local. I will give it a shot on stage. Changes look good.

@AfaqShuaib09 AfaqShuaib09 merged commit 50dcc9a into master Jul 18, 2023
19 checks passed
@AfaqShuaib09 AfaqShuaib09 deleted the afaq/prod-3412 branch July 18, 2023 12:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants