From ce6ffe6d613486b7d2207d90e532b1d486f706cd Mon Sep 17 00:00:00 2001 From: cahrens Date: Thu, 14 Apr 2016 13:17:24 -0400 Subject: [PATCH] Allow multiple random cohorts. --- .../verified_track_content/models.py | 38 +++++++--------- .../tests/test_forms.py | 2 - .../tests/test_models.py | 45 ++++++++++--------- 3 files changed, 40 insertions(+), 45 deletions(-) diff --git a/lms/djangoapps/verified_track_content/models.py b/lms/djangoapps/verified_track_content/models.py index fbb5ef947d..44cc0827d8 100644 --- a/lms/djangoapps/verified_track_content/models.py +++ b/lms/djangoapps/verified_track_content/models.py @@ -39,30 +39,24 @@ def move_to_verified_cohort(sender, instance, **kwargs): # pylint: disable=unus course = get_course_by_id(course_key) existing_manual_cohorts = get_course_cohorts(course, CourseCohort.MANUAL) if any(cohort.name == verified_cohort_name for cohort in existing_manual_cohorts): - # Verify that a single random cohort exists in the course. Note that calling this method will create - # a "Default Group" random cohort if no random cohorts exist yet. + # Get a random cohort to use as the default cohort (for audit learners). + # Note that calling this method will create a "Default Group" random cohort if no random + # cohort yet exist. random_cohort = get_random_cohort(course_key) - if not is_default_cohort(random_cohort): - log.error( - "Automatic verified cohorting enabled for course '%s', " - "but course does not have exactly one default cohort for audit learners.", - course_key - ) - else: - args = { - 'course_id': unicode(course_key), - 'user_id': instance.user.id, - 'verified_cohort_name': verified_cohort_name, - 'default_cohort_name': random_cohort.name - } - # Do the update with a 3-second delay in hopes that the CourseEnrollment transaction has been - # completed before the celery task runs. We want a reasonably short delay in case the learner - # immediately goes to the courseware. - sync_cohort_with_mode.apply_async(kwargs=args, countdown=3) + args = { + 'course_id': unicode(course_key), + 'user_id': instance.user.id, + 'verified_cohort_name': verified_cohort_name, + 'default_cohort_name': random_cohort.name + } + # Do the update with a 3-second delay in hopes that the CourseEnrollment transaction has been + # completed before the celery task runs. We want a reasonably short delay in case the learner + # immediately goes to the courseware. + sync_cohort_with_mode.apply_async(kwargs=args, countdown=3) - # In case the transaction actually was not committed before the celery task runs, - # run it again after 5 minutes. If the first completed successfully, this task will be a no-op. - sync_cohort_with_mode.apply_async(kwargs=args, countdown=300) + # In case the transaction actually was not committed before the celery task runs, + # run it again after 5 minutes. If the first completed successfully, this task will be a no-op. + sync_cohort_with_mode.apply_async(kwargs=args, countdown=300) else: log.error( "Automatic verified cohorting enabled for course '%s', " diff --git a/lms/djangoapps/verified_track_content/tests/test_forms.py b/lms/djangoapps/verified_track_content/tests/test_forms.py index 6526f57fd3..b400c292c0 100644 --- a/lms/djangoapps/verified_track_content/tests/test_forms.py +++ b/lms/djangoapps/verified_track_content/tests/test_forms.py @@ -1,8 +1,6 @@ """ Test for forms helpers. """ -from opaque_keys.edx.keys import CourseKey - from xmodule.modulestore.tests.factories import CourseFactory from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase diff --git a/lms/djangoapps/verified_track_content/tests/test_models.py b/lms/djangoapps/verified_track_content/tests/test_models.py index cc9a585589..9a1162e7f9 100644 --- a/lms/djangoapps/verified_track_content/tests/test_models.py +++ b/lms/djangoapps/verified_track_content/tests/test_models.py @@ -165,27 +165,6 @@ class TestMoveToVerified(SharedModuleStoreTestCase): error_message = "cohort named '%s' does not exist" self.assertIn(error_message, error_logger.call_args[0][0]) - @mock.patch('verified_track_content.models.log.error') - def test_cohorting_enabled_too_many_random_cohorts(self, error_logger): - """ - If the VerifiedTrackCohortedCourse feature is enabled for a course and the course is cohorted, - but the course has > 1 random cohorts, an error is logged and enrollment mode changes do not - move learners into a cohort. - """ - # Enable cohorting, and create the verified cohort. - self._enable_cohorting() - self._create_verified_cohort() - # Create two random cohorts. - self._create_named_random_cohort("Random 1") - self._create_named_random_cohort("Random 2") - # Enable verified track cohorting feature - self._enable_verified_track_cohorting() - self.assertTrue(VerifiedTrackCohortedCourse.is_verified_track_cohort_enabled(self.course.id)) - self._verify_no_automatic_cohorting() - self.assertTrue(error_logger.called) - error_message = "course does not have exactly one default cohort" - self.assertIn(error_message, error_logger.call_args[0][0]) - def test_automatic_cohorting_enabled(self): """ If the VerifiedTrackCohortedCourse feature is enabled for a course (with course cohorting enabled @@ -207,6 +186,30 @@ class TestMoveToVerified(SharedModuleStoreTestCase): self.assertEqual(4, self.mocked_celery_task.call_count) self.assertEqual(DEFAULT_VERIFIED_COHORT_NAME, get_cohort(self.user, self.course.id, assign=False).name) + def test_cohorting_enabled_multiple_random_cohorts(self): + """ + If the VerifiedTrackCohortedCourse feature is enabled for a course, and the course is cohorted + with > 1 random cohorts, the learner is randomly assigned to one of the random + cohorts when in the audit track. + """ + # Enable cohorting, and create the verified cohort. + self._enable_cohorting() + self._create_verified_cohort() + # Create two random cohorts. + self._create_named_random_cohort("Random 1") + self._create_named_random_cohort("Random 2") + # Enable verified track cohorting feature + self._enable_verified_track_cohorting() + + self._enroll_in_course() + self.assertIn(get_cohort(self.user, self.course.id, assign=False).name, ["Random 1", "Random 2"]) + self._upgrade_to_verified() + self.assertEqual(DEFAULT_VERIFIED_COHORT_NAME, get_cohort(self.user, self.course.id, assign=False).name) + + self._unenroll() + self._reenroll() + self.assertIn(get_cohort(self.user, self.course.id, assign=False).name, ["Random 1", "Random 2"]) + def test_unenrolled(self): """ Test that un-enrolling and re-enrolling works correctly. This is important because usually