Merge pull request #16796 from edx/thallada/schedules-hold-back-fix

Schedules: Fix hold back error

openedx/core/djangoapps/schedules/signals.py

@@ -27,59 +27,31 @@ log = logging.getLogger(__name__)
 
 
 @receiver(post_save, sender=CourseEnrollment, dispatch_uid='create_schedule_for_enrollment')
-def create_schedule(sender, **kwargs):
-    if not kwargs['created']:
-        # only create schedules when enrollment records are created
-        return
-
-    current_site = get_current_site()
-    if current_site is None:
-        log.debug('Schedules: No current site')
-        return
-
-    enrollment = kwargs['instance']
-    schedule_config = ScheduleConfig.current(current_site)
-    if (
-        not schedule_config.create_schedules
-        and not CREATE_SCHEDULE_WAFFLE_FLAG.is_enabled(enrollment.course_id)
-    ):
-        log.debug('Schedules: Creation not enabled for this course or for this site')
-        return
-
-    if not enrollment.course_overview.self_paced:
-        log.debug('Schedules: Creation only enabled for self-paced courses')
-        return
-
-    # This represents the first date at which the learner can access the content. This will be the latter of
-    # either the enrollment date or the course's start date.
-    content_availability_date = max(enrollment.created, enrollment.course_overview.start)
-
-    upgrade_deadline = _calculate_upgrade_deadline(enrollment.course_id, content_availability_date)
-
-    if course_has_highlights(enrollment.course_id):
-        experience_type = ScheduleExperience.EXPERIENCES.course_updates
-    else:
-        experience_type = ScheduleExperience.EXPERIENCES.default
-
-    if _should_randomly_suppress_schedule_creation(
-        schedule_config,
-        enrollment,
-        upgrade_deadline,
-        experience_type,
-        content_availability_date,
-    ):
-        return
-
-    schedule = Schedule.objects.create(
-        enrollment=enrollment,
-        start=content_availability_date,
-        upgrade_deadline=upgrade_deadline
-    )
-
-    ScheduleExperience(schedule=schedule, experience_type=experience_type).save()
-
-    log.debug('Schedules: created a new schedule starting at %s with an upgrade deadline of %s and experience type: %s',
-              content_availability_date, upgrade_deadline, ScheduleExperience.EXPERIENCES[experience_type])
+def create_schedule(sender, **kwargs):  # pylint: disable=unused-argument
+    """
+    When a CourseEnrollment is created, create a Schedule if configured.
+    """
+    try:
+        enrollment = kwargs.get('instance')
+        enrollment_created = kwargs.get('created')
+
+        schedule_details = _create_schedule(enrollment, enrollment_created)
+
+        if schedule_details:
+            log.debug(
+                'Schedules: created a new schedule starting at ' +
+                '%s with an upgrade deadline of %s and experience type: %s',
+                schedule_details['content_availability_date'],
+                schedule_details['upgrade_deadline'],
+                ScheduleExperience.EXPERIENCES[schedule_details['experience_type']]
+            )
+    except Exception:  # pylint: disable=broad-except
+        # We do not want to block the creation of a CourseEnrollment because of an error in creating a Schedule.
+        # No Schedule is acceptable, but no CourseEnrollment is not.
+        log.exception('Encountered error in creating a Schedule for CourseEnrollment for user {} in course {}'.format(
+            enrollment.user.id if (enrollment and enrollment.user) else None,
+            enrollment.course_id if enrollment else None
+        ))
 
 
 @receiver(COURSE_START_DATE_CHANGED, dispatch_uid="update_schedules_on_course_start_changed")
@@ -167,9 +139,9 @@ def _should_randomly_suppress_schedule_creation(
         if upgrade_deadline:
             upgrade_deadline_str = upgrade_deadline.isoformat()
         analytics.track(
-            'edx.bi.schedule.suppressed',
-            {
-                'user_id': enrollment.user.id,
+            user_id=enrollment.user.id,
+            event='edx.bi.schedule.suppressed',
+            properties={
                 'course_id': unicode(enrollment.course_id),
                 'experience_type': experience_type,
                 'upgrade_deadline': upgrade_deadline_str,
@@ -179,3 +151,70 @@ def _should_randomly_suppress_schedule_creation(
         return True
 
     return False
+
+
+def _create_schedule(enrollment, enrollment_created):
+    """
+    Checks configuration and creates Schedule with ScheduleExperience for this enrollment.
+    """
+    if not enrollment_created:
+        # only create schedules when enrollment records are created
+        return
+
+    current_site = get_current_site()
+    if current_site is None:
+        log.debug('Schedules: No current site')
+        return
+
+    schedule_config = ScheduleConfig.current(current_site)
+    if (
+        not schedule_config.create_schedules and
+        not CREATE_SCHEDULE_WAFFLE_FLAG.is_enabled(enrollment.course_id)
+    ):
+        log.debug('Schedules: Creation not enabled for this course or for this site')
+        return
+
+    if not enrollment.course_overview.self_paced:
+        log.debug('Schedules: Creation only enabled for self-paced courses')
+        return
+
+    # This represents the first date at which the learner can access the content. This will be the latter of
+    # either the enrollment date or the course's start date.
+    content_availability_date = max(enrollment.created, enrollment.course_overview.start)
+    upgrade_deadline = _calculate_upgrade_deadline(enrollment.course_id, content_availability_date)
+    experience_type = _get_experience_type(enrollment)
+
+    if _should_randomly_suppress_schedule_creation(
+            schedule_config,
+            enrollment,
+            upgrade_deadline,
+            experience_type,
+            content_availability_date,
+    ):
+        return
+
+    schedule = Schedule.objects.create(
+        enrollment=enrollment,
+        start=content_availability_date,
+        upgrade_deadline=upgrade_deadline
+    )
+
+    ScheduleExperience(schedule=schedule, experience_type=experience_type).save()
+
+    return {
+        'content_availability_date': content_availability_date,
+        'upgrade_deadline': upgrade_deadline,
+        'experience_type': experience_type,
+    }
+
+
+def _get_experience_type(enrollment):
+    """
+    Returns the ScheduleExperience type for the given enrollment.
+
+    Schedules will receive the Course Updates experience if the course has any section highlights defined.
+    """
+    if course_has_highlights(enrollment.course_id):
+        return ScheduleExperience.EXPERIENCES.course_updates
+    else:
+        return ScheduleExperience.EXPERIENCES.default

openedx/core/djangoapps/schedules/tests/test_signals.py

 import datetime
 import ddt
+import pytest
 from mock import patch
 from pytz import utc
 
@@ -31,9 +32,9 @@ class CreateScheduleTests(SharedModuleStoreTestCase):
             course_id=course.id,
             mode=CourseMode.AUDIT,
         )
-        self.assertIsNotNone(enrollment.schedule)
-        self.assertIsNone(enrollment.schedule.upgrade_deadline)
-        self.assertEquals(enrollment.schedule.experience.experience_type, experience_type)
+        assert enrollment.schedule is not None
+        assert enrollment.schedule.upgrade_deadline is None
+        assert enrollment.schedule.experience.experience_type == experience_type
 
     def assert_schedule_not_created(self):
         course = _create_course_run(self_paced=True)
@@ -41,7 +42,7 @@ class CreateScheduleTests(SharedModuleStoreTestCase):
             course_id=course.id,
             mode=CourseMode.AUDIT,
         )
-        with self.assertRaises(Schedule.DoesNotExist):
+        with pytest.raises(Schedule.DoesNotExist, message="Expecting Schedule to not exist"):
             enrollment.schedule
 
     @override_waffle_flag(CREATE_SCHEDULE_WAFFLE_FLAG, True)
@@ -84,7 +85,7 @@ class CreateScheduleTests(SharedModuleStoreTestCase):
         ScheduleConfigFactory.create(site=site, enabled=True, create_schedules=True)
         course = _create_course_run(self_paced=False)
         enrollment = CourseEnrollmentFactory(course_id=course.id, mode=CourseMode.AUDIT)
-        with self.assertRaises(Schedule.DoesNotExist):
+        with pytest.raises(Schedule.DoesNotExist, message="Expecting Schedule to not exist"):
             enrollment.schedule
 
     @override_waffle_flag(CREATE_SCHEDULE_WAFFLE_FLAG, True)
@@ -117,10 +118,22 @@ class CreateScheduleTests(SharedModuleStoreTestCase):
         mock_get_current_site.return_value = schedule_config.site
         if expect_schedule_created:
             self.assert_schedule_created()
-            self.assertFalse(mock_track.called)
+            assert not mock_track.called
         else:
             self.assert_schedule_not_created()
             mock_track.assert_called_once()
+            assert mock_track.call_args[1].get('event') == 'edx.bi.schedule.suppressed'
+
+    @patch('openedx.core.djangoapps.schedules.signals.log.exception')
+    @patch('openedx.core.djangoapps.schedules.signals.Schedule.objects.create')
+    def test_create_schedule_error(self, mock_create_schedule, mock_log, mock_get_current_site):
+        site = SiteFactory.create()
+        mock_get_current_site.return_value = site
+        ScheduleConfigFactory.create(site=site)
+        mock_create_schedule.side_effect = ValueError('Fake error')
+        self.assert_schedule_not_created()
+        mock_log.assert_called_once()
+        assert 'Encountered error in creating a Schedule for CourseEnrollment' in mock_log.call_args[0][0]
 
 
 @ddt.ddt
@@ -137,9 +150,9 @@ class UpdateScheduleTests(SharedModuleStoreTestCase):
         DynamicUpgradeDeadlineConfiguration.objects.create(enabled=True, deadline_days=self.VERIFICATION_DEADLINE_DAYS)
 
     def assert_schedule_dates(self, schedule, expected_start):
-        self.assertEquals(_strip_secs(schedule.start), _strip_secs(expected_start))
-        self.assertEquals(
-            _strip_secs(schedule.upgrade_deadline),
+        assert _strip_secs(schedule.start) == _strip_secs(expected_start)
+        assert (
+            _strip_secs(schedule.upgrade_deadline) ==
             _strip_secs(expected_start) + datetime.timedelta(days=self.VERIFICATION_DEADLINE_DAYS),
         )