diff --git a/common/djangoapps/student/models/course_enrollment.py b/common/djangoapps/student/models/course_enrollment.py index f879762679..227f60081e 100644 --- a/common/djangoapps/student/models/course_enrollment.py +++ b/common/djangoapps/student/models/course_enrollment.py @@ -590,7 +590,7 @@ class CourseEnrollment(models.Model): segment_traits['email'] = self.user.email if event_name == EVENT_NAME_ENROLLMENT_ACTIVATED: - if should_send_enrollment_email(): + if should_send_enrollment_email() and self.course_id: course_pacing_type = 'self-paced' if self.course_overview.self_paced else 'instructor-paced' send_course_enrollment_email.apply_async((self.user.id, str(self.course_id), self.course_overview.display_name, diff --git a/common/djangoapps/student/tasks.py b/common/djangoapps/student/tasks.py index 154199a0a0..c7e0842664 100644 --- a/common/djangoapps/student/tasks.py +++ b/common/djangoapps/student/tasks.py @@ -79,7 +79,7 @@ def send_course_enrollment_email( if not course_ended: course_date_blocks = get_course_dates_for_email(user, course_key, request=None) except Exception as err: # pylint: disable=broad-except - log.exception(err) + log.exception(f'[Enrollment email] Failed to get course dates with error: {err}') is_course_date_missing = True canvas_entry_properties.update( @@ -89,17 +89,16 @@ def send_course_enrollment_email( } ) - if course_id is None: - raise ValueError('missing course_id') - try: course_uuid = get_course_uuid_for_course(course_id) if course_uuid is None: - raise ValueError('missing course_uuid') - owners = get_owners_for_course(course_uuid=course_uuid) or [{}] + raise ValueError('Missing course_uuid') + + owners = get_owners_for_course(course_uuid=course_uuid) course_run = get_course_run_details(course_id, course_run_fields) - if course_run is None: - raise ValueError('missing course_run') + if not course_run: + raise ValueError('Missing course_run') + marketing_root_url = settings.MKTG_URLS.get("ROOT") instructors = get_instructors(course_run, marketing_root_url) enrollment_count = int(course_run.get("enrollment_count")) if course_run.get("enrollment_count") else 0 @@ -115,12 +114,12 @@ def send_course_enrollment_email( "course_title": course_run.get("title"), "short_description": course_run.get("short_description"), "pacing_type": course_run.get("pacing_type"), - "partner_image_url": owners[0].get("logo_image_url") or "", + "partner_image_url": owners[0].get("logo_image_url") if owners else "", } ) except Exception as err: # pylint: disable=broad-except is_course_run_missing = True - log.warning(f"[Course Enrollment] Course run call failed for user: {user_id} course: {course_id} err: {err}") + log.warning(f"[Course Enrollment] Course run call failed for user: {user_id} course: {course_id} error: {err}") if is_course_run_missing or is_course_date_missing: segment_properties = { diff --git a/common/djangoapps/student/tests/test_tasks.py b/common/djangoapps/student/tests/test_tasks.py index b0526d8476..99d24fed37 100644 --- a/common/djangoapps/student/tests/test_tasks.py +++ b/common/djangoapps/student/tests/test_tasks.py @@ -15,6 +15,7 @@ from common.djangoapps.student.tests.factories import UserFactory from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory +TASK_LOGGER = 'common.djangoapps.student.tasks.log' BRAZE_COURSE_ENROLLMENT_CANVAS_ID = "braze-canvas-id" @@ -247,7 +248,7 @@ class TestCourseEnrollmentEmailTask(ModuleStoreTestCase): "common.djangoapps.student.tasks.get_course_run_details", Mock(side_effect=Exception), ) - def test_canvas_properties_without_discovery_call( + def test_canvas_properties_on_get_course_run_details_failure( self, mock_get_braze_client, mock_get_course_dates_for_email, @@ -255,8 +256,8 @@ class TestCourseEnrollmentEmailTask(ModuleStoreTestCase): mock_get_course_uuid_for_course, ): """ - Test to verify the "canvas entry properties" for enrollment email when - course run call is failed. + Test to verify the "canvas entry properties" in the enrollment email when + get_course_run_details fails. """ mock_get_course_uuid_for_course.return_value = self.course_uuid mock_get_owners_for_course.return_value = self._get_course_owners() @@ -277,6 +278,84 @@ class TestCourseEnrollmentEmailTask(ModuleStoreTestCase): ), ) + @patch("common.djangoapps.student.tasks.get_course_uuid_for_course") + @patch("common.djangoapps.student.tasks.get_course_dates_for_email") + @patch("common.djangoapps.student.tasks.get_braze_client") + @patch(TASK_LOGGER) + def test_email_task_when_course_uuid_is_missing( + self, + mocked_logger, + mock_get_braze_client, + mock_get_course_dates_for_email, + mock_get_course_uuid_for_course, + ): + """ + Test that exception is logged when course_uuid returned by + get_course_uuid_for_course is None and that email is sent with + appropriate canvas properties. + """ + mock_get_course_uuid_for_course.return_value = None + mock_get_course_dates_for_email.return_value = self._get_course_dates() + + send_course_enrollment_email.apply_async( + kwargs=self.send_course_enrollment_email_kwargs + ) + mocked_logger.warning.assert_called_once_with( + f"[Course Enrollment] Course run call failed for " + f"user: {self.user.id} course: {self.course.id} error: Missing course_uuid" + ) + mock_get_braze_client.return_value.send_canvas_message.assert_called_with( + canvas_id=BRAZE_COURSE_ENROLLMENT_CANVAS_ID, + recipients=[ + { + "external_user_id": self.user.id, + } + ], + canvas_entry_properties=self._get_canvas_properties(add_course_run_details=False), + ) + + @patch("common.djangoapps.student.tasks.get_course_uuid_for_course") + @patch("common.djangoapps.student.tasks.get_owners_for_course") + @patch("common.djangoapps.student.tasks.get_course_run_details") + @patch("common.djangoapps.student.tasks.get_course_dates_for_email") + @patch("common.djangoapps.student.tasks.get_braze_client") + @patch(TASK_LOGGER) + def test_email_task_when_course_run_is_missing( + self, + mocked_logger, + mock_get_braze_client, + mock_get_course_dates_for_email, + mock_get_course_run_details, + mock_get_owners_for_course, + mock_get_course_uuid_for_course, + ): + """ + Test that exception is logged when course_run returned by + get_course_run_details is an empty dictionary and that email is sent with + appropriate canvas properties. + """ + mock_get_course_dates_for_email.return_value = self._get_course_dates() + mock_get_course_uuid_for_course.return_value = self.course_uuid + mock_get_owners_for_course.return_value = [] + mock_get_course_run_details.return_value = {} + + send_course_enrollment_email.apply_async( + kwargs=self.send_course_enrollment_email_kwargs + ) + mocked_logger.warning.assert_called_once_with( + f"[Course Enrollment] Course run call failed for " + f"user: {self.user.id} course: {self.course.id} error: Missing course_run" + ) + mock_get_braze_client.return_value.send_canvas_message.assert_called_with( + canvas_id=BRAZE_COURSE_ENROLLMENT_CANVAS_ID, + recipients=[ + { + "external_user_id": self.user.id, + } + ], + canvas_entry_properties=self._get_canvas_properties(add_course_run_details=False), + ) + @patch("common.djangoapps.student.tasks.get_course_uuid_for_course") @patch("common.djangoapps.student.tasks.get_owners_for_course") @patch("common.djangoapps.student.tasks.get_course_run_details") diff --git a/openedx/core/djangoapps/catalog/utils.py b/openedx/core/djangoapps/catalog/utils.py index c49c5accfe..f9b46ee546 100644 --- a/openedx/core/djangoapps/catalog/utils.py +++ b/openedx/core/djangoapps/catalog/utils.py @@ -469,9 +469,16 @@ def get_course_runs_for_course(course_uuid): # lint-amnesty, pylint: disable=mi return [] -def get_owners_for_course(course_uuid): # lint-amnesty, pylint: disable=missing-function-docstring +def get_owners_for_course(course_uuid): + """ + Retrieves the course owner given a course uuid. + + Arguments + course_uuid (string): Course UUID + """ if course_uuid is None: - raise ValueError("missing course_uuid") + return [] + user, catalog_integration = check_catalog_integration_and_get_user(error_message_field='Owners') if user: cache_key = f"{catalog_integration.CACHE_KEY}.course.{course_uuid}.course_runs" @@ -483,6 +490,7 @@ def get_owners_for_course(course_uuid): # lint-amnesty, pylint: disable=missing api_client=get_catalog_api_client(user), base_api_url=get_catalog_api_base_url(), cache_key=cache_key if catalog_integration.is_cache_enabled else None, + traverse_pagination=False, long_term_cache=True, many=False ) @@ -503,7 +511,8 @@ def get_course_uuid_for_course(course_run_key): UUID: Course UUID and None if it was not retrieved. """ if course_run_key is None: - raise ValueError("missing course_run_key") + return None + user, catalog_integration = check_catalog_integration_and_get_user(error_message_field='Course UUID') if user: api_client = get_catalog_api_client(user) @@ -520,6 +529,7 @@ def get_course_uuid_for_course(course_run_key): cache_key=run_cache_key if catalog_integration.is_cache_enabled else None, long_term_cache=True, many=False, + traverse_pagination=False, ) course_key_str = course_run_data.get('course', None) @@ -536,6 +546,7 @@ def get_course_uuid_for_course(course_run_key): cache_key=run_cache_key if catalog_integration.is_cache_enabled else None, long_term_cache=True, many=False, + traverse_pagination=False, ) uuid_str = data.get('uuid', None) if uuid_str: @@ -617,9 +628,10 @@ def get_course_run_details(course_run_key, fields): Returns: dict with language, start date, end date, and max_effort details about specified course run """ - if course_run_key is None: - raise ValueError("missing course_run_key") course_run_details = {} + if course_run_key is None: + return course_run_details + user, catalog_integration = check_catalog_integration_and_get_user( error_message_field=f'Data for course_run {course_run_key}' )