Refactor bulk unenroll mgmt cmd

common/djangoapps/student/management/commands/bulk_unenroll.py

@@ -1,5 +1,5 @@
 """
-Un-enroll Bulk users course wide as well as provided in csv
+Un-enroll Bulk users course wide as well as specified in csv
 """
 import logging
 
@@ -20,8 +20,11 @@ class Command(BaseCommand):
     help = """"
     Un-enroll bulk users from the courses.
     It expect that the data will be provided in a csv file format with
-    first row being the header and columns will be as follows:
-    user_id, username, email, course_id, is_verified, verification_date
+    first row being the header and columns will be either one of the
+    following:
+    username,course-id
+    OR
+    course-id
     """
 
     def add_arguments(self, parser):
@@ -30,20 +33,9 @@ def add_arguments(self, parser):
                             dest='csv_path',
                             required=False,
                             help='Path to CSV file.')
-        parser.add_argument('-c', '--course-id',
-                            metavar='course_id',
-                            dest='course-id',
-                            required=False,
-                            help='unenroll all users from provided course-id')
 
     def handle(self, *args, **options):
-
         csv_path = options['csv_path']
-        course_id = options['course-id']
-
-        if course_id:
-            self.unenroll_all_users(course_id=course_id)
-            return
 
         if csv_path:
             with open(csv_path, 'rb') as csv_file:
@@ -57,52 +49,25 @@ def unenroll_users_from_csv(self, csv_file):
         Un-enroll the given set of users provided in csv
         """
         reader = list(unicodecsv.DictReader(csv_file))
-        users_unenrolled = {}
         for row in reader:
-            username = row['username']
-            course_key = row['course_id']
-
-            try:
-                course_id = CourseKey.from_string(row['course_id'])
-            except InvalidKeyError:
-                msg = 'Invalid course id {course_id}, skipping un-enrollement for {username}, {email}'.format(**row)
-                logger.warning(msg)
-                continue
-
-            try:
-                enrollment = CourseEnrollment.objects.get(user__username=username, course_id=course_id)
-                enrollment.update_enrollment(is_active=False, skip_refund=True)
-                if username in users_unenrolled:
-                    users_unenrolled[username].append(course_key.encode())
-                else:
-                    users_unenrolled[username] = [course_key.encode()]
+            self.unenroll_learners(row.get('course_id'), username=row.get('username', None))
 
-            except ObjectDoesNotExist:
-                msg = 'Enrollment for the user {} in course {} does not exist!'.format(username, course_key)
-                logger.info(msg)
-
-            except Exception as err:  # pylint: disable=W0703
-                msg = 'Error un-enrolling User {} from course {}: with error: {}'.format(username, course_key, err)
-                logger.error(msg, exc_info=True)
-
-        logger.info("Following users have been unenrolled successfully from the following courses: {users_unenrolled}"
-                    .format(users_unenrolled=["{}:{}".format(k, v) for k, v in users_unenrolled.items()]))
-
-    def unenroll_all_users(self, course_id):
+    def unenroll_learners(self, course_id, username=None):
         """
-        Un-enroll all users from a given course
+        Un-enroll learners from course_id(s)
         """
         try:
-            course_id = CourseKey.from_string(course_id)
+            course_key = CourseKey.from_string(course_id)
         except InvalidKeyError:
             msg = 'Invalid course id {}, skipping un-enrollement.'.format(course_id)
             logger.warning(msg)
             return
 
-        try:
-            updated_count = CourseEnrollment.objects.filter(course_id=course_id, is_active=True).update(is_active=False)
-            logger.info("{} users have been unenrolled successfully from the provided course: {}"
-                        .format(updated_count, course_id))
-        except Exception as err:  # pylint: disable=W0703
-            msg = 'Error un-enrolling Users from course {}: with error: {}'.format(course_id, err)
-            logger.error(msg, exc_info=True)
+        enrollments = CourseEnrollment.objects.filter(course_id=course_key, is_active=True)
+        if username:
+            enrollments = enrollments.filter(user__username=username)
+
+        for enrollment in enrollments:
+            enrollment.update_enrollment(is_active=False, skip_refund=True)
+            logger.info("User [{}] have been successfully unenrolled from the course: {}"
+                        .format(enrollment.user.username, course_key))

common/djangoapps/student/management/tests/test_bulk_unenroll.py

@@ -1,14 +1,15 @@
+"""Tests for Bulk Un-enroll Management command"""
 
+from tempfile import NamedTemporaryFile
 
 import six
-
-from tempfile import NamedTemporaryFile
-from course_modes.tests.factories import CourseModeFactory
 from django.core.files.uploadedfile import SimpleUploadedFile
 from django.core.management import call_command
-from student.models import CourseEnrollment, BulkUnenrollConfiguration
-from student.tests.factories import UserFactory
 from testfixtures import LogCapture
+
+from course_modes.tests.factories import CourseModeFactory
+from student.models import BulkUnenrollConfiguration, CourseEnrollment
+from student.tests.factories import UserFactory
 from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase
 from xmodule.modulestore.tests.factories import CourseFactory
 
@@ -35,13 +36,17 @@ def setUp(self):
         self.users = []
 
         for username, email, password in self.user_info:
-            user = UserFactory.create(username=username, email=email, password=password)
+            user = UserFactory.create(
+                username=username, email=email, password=password
+            )
             self.users.append(user)
-            self.enrollments.append(CourseEnrollment.enroll(user, self.course.id, mode='audit'))
+            self.enrollments.append(
+                CourseEnrollment.enroll(user, self.course.id, mode='audit')
+            )
 
     def _write_test_csv(self, csv, lines):
         """Write a test csv file with the lines provided"""
-        csv.write(b"user_id,username,email,course_id\n")
+        csv.write(b"username,course_id\n")
         for line in lines:
             csv.write(six.b(line))
         csv.seek(0)
@@ -50,36 +55,22 @@ def _write_test_csv(self, csv, lines):
     def test_invalid_course_key(self):
         """Verify in case of invalid course key warning is logged."""
         with NamedTemporaryFile() as csv:
-            csv = self._write_test_csv(csv, lines=["111,amy,[email protected],test_course\n"])
+            csv = self._write_test_csv(csv, lines=["amy,test_course\n"])
 
             with LogCapture(LOGGER_NAME) as log:
                 call_command("bulk_unenroll", "--csv_path={}".format(csv.name))
-                expected_message = 'Invalid course id {}, skipping un-enrollement for {}, {}'.\
-                    format('test_course', 'amy', '[email protected]')
+                expected_message = 'Invalid course id {}, skipping un-enrollement.'.\
+                    format('test_course')
 
                 log.check_present(
                     (LOGGER_NAME, 'WARNING', expected_message)
                 )
 
-    def test_user_not_enrolled(self):
-        """Verify in case of user not enrolled in course warning is logged."""
-        with NamedTemporaryFile() as csv:
-            csv = self._write_test_csv(csv, lines=["111,amy,[email protected],course-v1:edX+DemoX+Demo_Course\n"])
-
-            with LogCapture(LOGGER_NAME) as log:
-                call_command("bulk_unenroll", "--csv_path={}".format(csv.name))
-                expected_message = 'Enrollment for the user {} in course {} does not exist!'.\
-                    format('amy', 'course-v1:edX+DemoX+Demo_Course')
-
-                log.check_present(
-                    (LOGGER_NAME, 'INFO', expected_message)
-                )
-
     def test_bulk_un_enroll(self):
         """Verify users are unenrolled using the command."""
         lines = [
-            str(enrollment.user.id) + "," + enrollment.user.username + "," +
-            enrollment.user.email + "," + str(enrollment.course.id) + "\n"
+            enrollment.user.username + "," +
+            str(enrollment.course.id) + "\n"
             for enrollment in self.enrollments
         ]
         with NamedTemporaryFile() as csv:
@@ -105,18 +96,8 @@ def test_bulk_unenroll_from_config_model(self):
 
     def test_users_unenroll_successfully_logged(self):
         """Verify users unenrolled are logged """
-        lines = "user_id,username,email,course_id\n"
-        users_unenrolled = {}
-        for enrollment in self.enrollments:
-            username = enrollment.user.username
-            if username in users_unenrolled:
-                users_unenrolled[username].append(str(enrollment.course.id).encode('utf-8'))
-            else:
-                users_unenrolled[username] = [str(enrollment.course.id).encode('utf-8')]
-
-            lines += str(enrollment.user.id) + "," + username + "," + \
-                enrollment.user.email + "," + str(enrollment.course.id) + "\n"
-
+        lines = "username,course_id\n"
+        lines += self.enrollments[0].username + "," + str(self.enrollments[0].course.id) + "\n"
         csv_file = SimpleUploadedFile(name='test.csv', content=lines.encode('utf-8'), content_type='text/csv')
         BulkUnenrollConfiguration.objects.create(enabled=True, csv_file=csv_file)
 
@@ -126,8 +107,8 @@ def test_users_unenroll_successfully_logged(self):
                 (
                     LOGGER_NAME,
                     'INFO',
-                    'Following users have been unenrolled successfully from the following courses:'
-                    ' {users_unenrolled}'.format(users_unenrolled=["{}:{}".format(k, v) for k, v in
-                                                                   users_unenrolled.items()])
+                    'User [{}] have been successfully unenrolled from the course: {}'.format(
+                        self.enrollments[0].username, self.enrollments[0].course.id
+                    )
                 )
             )