From 9bb4ca5525e1d2965191e34cab33254290d9b79b Mon Sep 17 00:00:00 2001 From: Douglas Hall Date: Wed, 12 Apr 2017 05:30:46 -0400 Subject: [PATCH] Fix bulk_change_enrollment command bug When using the `org` option with the bulk_change_enrollment command, the command should not exit if it encounters a course which does not have the given `to_mode`. WL-1033 --- .../commands/bulk_change_enrollment.py | 10 +- .../tests/test_bulk_change_enrollment.py | 100 ++++++++++++------ 2 files changed, 73 insertions(+), 37 deletions(-) diff --git a/common/djangoapps/student/management/commands/bulk_change_enrollment.py b/common/djangoapps/student/management/commands/bulk_change_enrollment.py index 94bcfc4119..569dc78488 100644 --- a/common/djangoapps/student/management/commands/bulk_change_enrollment.py +++ b/common/djangoapps/student/management/commands/bulk_change_enrollment.py @@ -111,15 +111,15 @@ class Command(BaseCommand): commit (bool): required to make the change to the database. Otherwise just a count will be displayed. """ + unicode_course_key = unicode(course_key) if CourseMode.mode_for_course(course_key, to_mode) is None: - raise CommandError('The given mode to move users into ({}) does not exist.'.format(to_mode)) - - course_key_str = unicode(course_key) + logger.info('Mode ({}) does not exist for course ({}).'.format(to_mode, unicode_course_key)) + return course_enrollments = CourseEnrollment.objects.filter(course_id=course_key, mode=from_mode) logger.info( 'Moving %d users from %s to %s in course %s.', - course_enrollments.count(), from_mode, to_mode, course_key_str + course_enrollments.count(), from_mode, to_mode, unicode_course_key ) if commit: # call `change_mode` which will change the mode and also emit tracking event @@ -127,4 +127,4 @@ class Command(BaseCommand): with transaction.atomic(): enrollment.change_mode(mode=to_mode) - logger.info('Finished moving users from %s to %s in course %s.', from_mode, to_mode, course_key_str) + logger.info('Finished moving users from %s to %s in course %s.', from_mode, to_mode, unicode_course_key) diff --git a/common/djangoapps/student/management/tests/test_bulk_change_enrollment.py b/common/djangoapps/student/management/tests/test_bulk_change_enrollment.py index dec5c637af..56d43241da 100644 --- a/common/djangoapps/student/management/tests/test_bulk_change_enrollment.py +++ b/common/djangoapps/student/management/tests/test_bulk_change_enrollment.py @@ -28,7 +28,7 @@ class BulkChangeEnrollmentTests(SharedModuleStoreTestCase): @ddt.unpack def test_bulk_convert(self, from_mode, to_mode, mock_tracker): """Verify that enrollments are changed correctly.""" - self._enroll_users(from_mode) + self._enroll_users(self.course, self.users, from_mode) CourseModeFactory(course_id=self.course.id, mode_slug=to_mode) # Verify that no users are in the `from` mode yet. @@ -46,27 +46,25 @@ class BulkChangeEnrollmentTests(SharedModuleStoreTestCase): # raise CourseEnrollment.DoesNotExist for user in self.users: CourseEnrollment.objects.get(mode=to_mode, course_id=self.course.id, user=user) - - # Confirm the analytics event was emitted. - mock_tracker.emit.assert_has_calls( # pylint: disable=maybe-no-member - [ - call( - EVENT_NAME_ENROLLMENT_MODE_CHANGED, - {'course_id': unicode(self.course.id), 'user_id': user.id, 'mode': to_mode} - ), - ] - ) + self._assert_mode_changed(mock_tracker, self.course, user, to_mode) @patch('student.models.tracker') @ddt.data(('audit', 'no-id-professional'), ('no-id-professional', 'audit')) @ddt.unpack def test_bulk_convert_with_org(self, from_mode, to_mode, mock_tracker): """Verify that enrollments are changed correctly when org was given.""" - self._enroll_users(from_mode) + self._enroll_users(self.course, self.users, from_mode) CourseModeFactory(course_id=self.course.id, mode_slug=to_mode) - # Verify that no users are in the `from` mode yet. + # Create a second course under the same org + course_2 = CourseFactory.create(org=self.org) + CourseModeFactory(course_id=course_2.id, mode_slug=to_mode) + CourseOverview.load_from_module_store(course_2.id) + self._enroll_users(course_2, self.users, from_mode) + + # Verify that no users are in the `to` mode yet. self.assertEqual(len(CourseEnrollment.objects.filter(mode=to_mode, course_id=self.course.id)), 0) + self.assertEqual(len(CourseEnrollment.objects.filter(mode=to_mode, course_id=course_2.id)), 0) call_command( 'bulk_change_enrollment', @@ -79,21 +77,13 @@ class BulkChangeEnrollmentTests(SharedModuleStoreTestCase): # Verify that all users have been moved -- if not, this will # raise CourseEnrollment.DoesNotExist for user in self.users: - CourseEnrollment.objects.get(mode=to_mode, course_id=self.course.id, user=user) - - # Confirm the analytics event was emitted. - mock_tracker.emit.assert_has_calls( # pylint: disable=maybe-no-member - [ - call( - EVENT_NAME_ENROLLMENT_MODE_CHANGED, - {'course_id': unicode(self.course.id), 'user_id': user.id, 'mode': to_mode} - ), - ] - ) + for course in [self.course, course_2]: + CourseEnrollment.objects.get(mode=to_mode, course_id=course.id, user=user) + self._assert_mode_changed(mock_tracker, course, user, to_mode) def test_with_org_and_course_key(self): """Verify that command raises CommandError when `org` and `course_key` both are given.""" - self._enroll_users('audit') + self._enroll_users(self.course, self.users, 'audit') CourseModeFactory(course_id=self.course.id, mode_slug='no-id-professional') with self.assertRaises(CommandError): @@ -106,9 +96,45 @@ class BulkChangeEnrollmentTests(SharedModuleStoreTestCase): commit=True, ) + @patch('student.models.tracker') + def test_with_org_and_invalid_to_mode(self, mock_tracker): + """Verify that enrollments are changed correctly when org was given.""" + from_mode = 'audit' + to_mode = 'no-id-professional' + self._enroll_users(self.course, self.users, from_mode) + + # Create a second course under the same org + course_2 = CourseFactory.create(org=self.org) + CourseModeFactory(course_id=course_2.id, mode_slug=to_mode) + CourseOverview.load_from_module_store(course_2.id) + self._enroll_users(course_2, self.users, from_mode) + + # Verify that no users are in the `to` mode yet. + self.assertEqual(len(CourseEnrollment.objects.filter(mode=to_mode, course_id=self.course.id)), 0) + self.assertEqual(len(CourseEnrollment.objects.filter(mode=to_mode, course_id=course_2.id)), 0) + + call_command( + 'bulk_change_enrollment', + org=self.org, + from_mode=from_mode, + to_mode=to_mode, + commit=True, + ) + + # Verify that users were not moved for the invalid course/mode combination + for user in self.users: + with self.assertRaises(CourseEnrollment.DoesNotExist): + CourseEnrollment.objects.get(mode=to_mode, course_id=self.course.id, user=user) + + # Verify that all users have been moved -- if not, this will + # raise CourseEnrollment.DoesNotExist + for user in self.users: + CourseEnrollment.objects.get(mode=to_mode, course_id=course_2.id, user=user) + self._assert_mode_changed(mock_tracker, course_2, user, to_mode) + def test_with_invalid_org(self): """Verify that command raises CommandError when invalid `org` is given.""" - self._enroll_users('audit') + self._enroll_users(self.course, self.users, 'audit') CourseModeFactory(course_id=self.course.id, mode_slug='no-id-professional') with self.assertRaises(CommandError): @@ -122,7 +148,7 @@ class BulkChangeEnrollmentTests(SharedModuleStoreTestCase): def test_without_commit(self): """Verify that nothing happens when the `commit` flag is not given.""" - self._enroll_users('audit') + self._enroll_users(self.course, self.users, 'audit') CourseModeFactory(course_id=self.course.id, mode_slug='honor') call_command( @@ -137,7 +163,7 @@ class BulkChangeEnrollmentTests(SharedModuleStoreTestCase): def test_without_to_mode(self): """Verify that the command fails when the `to_mode` argument does not exist.""" - self._enroll_users('audit') + self._enroll_users(self.course, self.users, 'audit') CourseModeFactory(course_id=self.course.id, mode_slug='audit') with self.assertRaises(CommandError): @@ -145,7 +171,6 @@ class BulkChangeEnrollmentTests(SharedModuleStoreTestCase): 'bulk_change_enrollment', course=unicode(self.course.id), from_mode='audit', - to_mode='honor', ) @ddt.data('from_mode', 'to_mode', 'course') @@ -177,7 +202,18 @@ class BulkChangeEnrollmentTests(SharedModuleStoreTestCase): commit=True ) - def _enroll_users(self, mode): + def _assert_mode_changed(self, mock_tracker, course, user, to_mode): + """Confirm the analytics event was emitted.""" + mock_tracker.emit.assert_has_calls( # pylint: disable=maybe-no-member + [ + call( + EVENT_NAME_ENROLLMENT_MODE_CHANGED, + {'course_id': unicode(course.id), 'user_id': user.id, 'mode': to_mode} + ), + ] + ) + + def _enroll_users(self, course, users, mode): """Enroll users in the given mode.""" - for user in self.users: - CourseEnrollmentFactory(mode=mode, course_id=self.course.id, user=user) + for user in users: + CourseEnrollmentFactory(mode=mode, course_id=course.id, user=user)