From 6e791c58eacc11ed0023b151cf1758d9abd3e700 Mon Sep 17 00:00:00 2001 From: Tyler Hallada Date: Tue, 5 Dec 2017 15:53:19 -0500 Subject: [PATCH] Fix hold back error Also add try/except that prevents enroll failure caused by Schedule creation exception in the future. Address PR comments Split one-line docstrings on their own line Add period to the end of docstring summaries Use kwargs.get to be more defensive Disable pylint unused-argument about sender param --- openedx/core/djangoapps/schedules/signals.py | 147 +++++++++++------- .../schedules/tests/test_signals.py | 12 ++ 2 files changed, 105 insertions(+), 54 deletions(-) diff --git a/openedx/core/djangoapps/schedules/signals.py b/openedx/core/djangoapps/schedules/signals.py index 8a8e28249c..53603acf45 100644 --- a/openedx/core/djangoapps/schedules/signals.py +++ b/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 +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') - current_site = get_current_site() - if current_site is None: - log.debug('Schedules: No current site') - return + schedule_details = _create_schedule(enrollment, enrollment_created) - 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]) + 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 diff --git a/openedx/core/djangoapps/schedules/tests/test_signals.py b/openedx/core/djangoapps/schedules/tests/test_signals.py index 167744e4c5..6479b0f169 100644 --- a/openedx/core/djangoapps/schedules/tests/test_signals.py +++ b/openedx/core/djangoapps/schedules/tests/test_signals.py @@ -121,6 +121,18 @@ class CreateScheduleTests(SharedModuleStoreTestCase): else: self.assert_schedule_not_created() mock_track.assert_called_once() + self.assertEquals(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