Enhance delete_course management command

cms/djangoapps/contentstore/management/commands/delete_course.py

@@ -11,7 +11,7 @@ from django.core.management.base import BaseCommand, CommandError
 from opaque_keys import InvalidKeyError
 from opaque_keys.edx.keys import CourseKey
 
-from contentstore.utils import delete_course_and_groups
+from contentstore.utils import delete_course
 from xmodule.modulestore import ModuleStoreEnum
 from xmodule.modulestore.django import modulestore
 
@@ -21,12 +21,36 @@ from .prompt import query_yes_no
 class Command(BaseCommand):
     """
     Delete a MongoDB backed course
+    Example usage:
+        $ ./manage.py cms delete_course 'course-v1:edX+DemoX+Demo_Course' --settings=devstack
+        $ ./manage.py cms delete_course 'course-v1:edX+DemoX+Demo_Course' --keep-instructors --settings=devstack
+
+    Note:
+        keep-instructors option is added in effort to delete duplicate courses safely.
+        There happens to be courses with difference of casing in ids, for example
+        course-v1:DartmouthX+DART.ENGL.01.X+2016_T1 is a duplicate of course-v1:DartmouthX+DART.ENGL.01.x+2016_T1
+        (Note the differene in 'x' of course number). These two are independent courses in MongoDB.
+        Current MYSQL setup is case-insensitive which essentially means there are not
+        seperate entries (in all course related mysql tables, but here we are concerned about accesses)
+        for duplicate courses.
+        This option will make us able to delete course (duplicate one) from
+        mongo while perserving course's related access data in mysql.
     """
     help = '''Delete a MongoDB backed course'''
 
     def add_arguments(self, parser):
+        """
+        Add arguments to the command parser.
+        """
         parser.add_argument('course_key', help="ID of the course to delete.")
 
+        parser.add_argument(
+            '--keep-instructors',
+            action='store_true',
+            default=False,
+            help='Do not remove permissions of users and groups for course',
+        )
+
     def handle(self, *args, **options):
         try:
             course_key = CourseKey.from_string(options['course_key'])
@@ -39,5 +63,5 @@ class Command(BaseCommand):
         print 'Going to delete the %s course from DB....' % options['course_key']
         if query_yes_no("Deleting course {0}. Confirm?".format(course_key), default="no"):
             if query_yes_no("Are you sure. This action cannot be undone!", default="no"):
-                delete_course_and_groups(course_key, ModuleStoreEnum.UserID.mgmt_command)
+                delete_course(course_key, ModuleStoreEnum.UserID.mgmt_command, options['keep_instructors'])
                 print "Deleted course {}".format(course_key)

cms/djangoapps/contentstore/management/commands/tests/test_delete_course.py

@@ -6,9 +6,11 @@ import mock
 
 from opaque_keys.edx.locations import SlashSeparatedCourseKey
 from django.core.management import call_command, CommandError
+from django.contrib.auth.models import User
 from contentstore.tests.utils import CourseTestCase
 from xmodule.modulestore.tests.factories import CourseFactory
 from xmodule.modulestore.django import modulestore
+from student.roles import CourseInstructorRole
 
 
 class DeleteCourseTest(CourseTestCase):
@@ -60,3 +62,27 @@ class DeleteCourseTest(CourseTestCase):
             patched_yes_no.return_value = True
             call_command('delete_course', 'TestX/TS01/2015_Q1')
             self.assertIsNone(modulestore().get_course(SlashSeparatedCourseKey("TestX", "TS01", "2015_Q1")))
+
+    def test_course_deletion_with_keep_instructors(self):
+        """
+        Tests that deleting course with keep-instructors option do not remove instructors from course.
+        """
+        instructor_user = User.objects.create(
+            username='test_instructor',
+            email='test_email@example.com'
+        )
+        self.assertIsNotNone(instructor_user)
+
+        # Add and verify instructor role for the course
+        instructor_role = CourseInstructorRole(self.course.id)
+        instructor_role.add_users(instructor_user)
+        self.assertTrue(instructor_role.has_user(instructor_user))
+
+        # Verify the course we are about to delete exists in the modulestore
+        self.assertIsNotNone(modulestore().get_course(self.course.id))
+
+        with mock.patch(self.YESNO_PATCH_LOCATION, return_value=True):
+            call_command('delete_course', 'TestX/TS01/2015_Q1', '--keep-instructors')
+
+        self.assertIsNone(modulestore().get_course(self.course.id))
+        self.assertTrue(instructor_role.has_user(instructor_user))

cms/djangoapps/contentstore/tests/test_contentstore.py

@@ -26,7 +26,7 @@ from path import Path as path
 
 from common.test.utils import XssTestMixin
 from contentstore.tests.utils import AjaxEnabledTestClient, CourseTestCase, get_url, parse_json
-from contentstore.utils import delete_course_and_groups, reverse_course_url, reverse_url
+from contentstore.utils import delete_course, reverse_course_url, reverse_url
 from contentstore.views.component import ADVANCED_COMPONENT_TYPES
 from course_action_state.managers import CourseActionStateItemNotFoundError
 from course_action_state.models import CourseRerunState, CourseRerunUIStateManager
@@ -1223,7 +1223,7 @@ class ContentStoreTest(ContentStoreTestCase, XssTestMixin):
         test_course_data = self.assert_created_course(number_suffix=uuid4().hex)
         course_id = _get_course_id(self.store, test_course_data)
         self.assertTrue(are_permissions_roles_seeded(course_id))
-        delete_course_and_groups(course_id, self.user.id)
+        delete_course(course_id, self.user.id)
         # should raise an exception for checking permissions on deleted course
         with self.assertRaises(ItemNotFoundError):
             are_permissions_roles_seeded(course_id)
@@ -1235,7 +1235,7 @@ class ContentStoreTest(ContentStoreTestCase, XssTestMixin):
 
         # unseed the forums for the first course
         course_id = _get_course_id(self.store, test_course_data)
-        delete_course_and_groups(course_id, self.user.id)
+        delete_course(course_id, self.user.id)
         # should raise an exception for checking permissions on deleted course
         with self.assertRaises(ItemNotFoundError):
             are_permissions_roles_seeded(course_id)
@@ -1255,7 +1255,7 @@ class ContentStoreTest(ContentStoreTestCase, XssTestMixin):
         self.assertTrue(CourseEnrollment.is_enrolled(self.user, course_id))
         self.assertTrue(self.user.roles.filter(name="Student", course_id=course_id))
 
-        delete_course_and_groups(course_id, self.user.id)
+        delete_course(course_id, self.user.id)
         # check that user's enrollment for this course is not deleted
         self.assertTrue(CourseEnrollment.is_enrolled(self.user, course_id))
         # check that user has form role "Student" for this course even after deleting it
@@ -1277,7 +1277,7 @@ class ContentStoreTest(ContentStoreTestCase, XssTestMixin):
         self.assertGreater(len(instructor_role.users_with_role()), 0)
 
         # Now delete course and check that user not in instructor groups of this course
-        delete_course_and_groups(course_id, self.user.id)
+        delete_course(course_id, self.user.id)
 
         # Update our cached user since its roles have changed
         self.user = User.objects.get_by_natural_key(self.user.natural_key()[0])
@@ -1285,6 +1285,26 @@ class ContentStoreTest(ContentStoreTestCase, XssTestMixin):
         self.assertFalse(instructor_role.has_user(self.user))
         self.assertEqual(len(instructor_role.users_with_role()), 0)
 
+    def test_delete_course_with_keep_instructors(self):
+        """
+        Tests that when you delete a course with 'keep_instructors',
+        it does not remove any permissions of users/groups from the course
+        """
+        test_course_data = self.assert_created_course(number_suffix=uuid4().hex)
+        course_id = _get_course_id(self.store, test_course_data)
+
+        # Add and verify instructor role for the course
+        instructor_role = CourseInstructorRole(course_id)
+        instructor_role.add_users(self.user)
+        self.assertTrue(instructor_role.has_user(self.user))
+
+        delete_course(course_id, self.user.id, keep_instructors=True)
+
+        # Update our cached user so if any change in roles can be captured
+        self.user = User.objects.get_by_natural_key(self.user.natural_key()[0])
+
+        self.assertTrue(instructor_role.has_user(self.user))
+
     def test_create_course_after_delete(self):
         """
         Test that course creation works after deleting a course with the same URL
@@ -1292,7 +1312,7 @@ class ContentStoreTest(ContentStoreTestCase, XssTestMixin):
         test_course_data = self.assert_created_course()
         course_id = _get_course_id(self.store, test_course_data)
 
-        delete_course_and_groups(course_id, self.user.id)
+        delete_course(course_id, self.user.id)
 
         self.assert_created_course()
 

cms/djangoapps/contentstore/tests/test_course_listing.py

@@ -15,7 +15,7 @@ from opaque_keys.edx.locations import CourseLocator
 
 from common.test.utils import XssTestMixin
 from contentstore.tests.utils import AjaxEnabledTestClient
-from contentstore.utils import delete_course_and_groups
+from contentstore.utils import delete_course
 from contentstore.views.course import (
     AccessListFallback,
     _accessible_courses_iter,
@@ -298,7 +298,7 @@ class TestCourseListing(ModuleStoreTestCase, XssTestMixin):
         self.assertEqual(courses_list, courses_list_by_groups)
 
         # now delete this course and re-add user to instructor group of this course
-        delete_course_and_groups(course_key, self.user.id)
+        delete_course(course_key, self.user.id)
 
         CourseInstructorRole(course_key).add_users(self.user)
 

cms/djangoapps/contentstore/tests/test_users_default_role.py

@@ -3,7 +3,7 @@ Unit tests for checking default forum role "Student" of a user when he creates a
 after deleting it creates same course again
 """
 from contentstore.tests.utils import AjaxEnabledTestClient
-from contentstore.utils import delete_course_and_groups, reverse_url
+from contentstore.utils import delete_course, reverse_url
 from courseware.tests.factories import UserFactory
 from student.models import CourseEnrollment
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
@@ -60,7 +60,7 @@ class TestUsersDefaultRole(ModuleStoreTestCase):
         # check that user has his default "Student" forum role for this course
         self.assertTrue(self.user.roles.filter(name="Student", course_id=self.course_key))
 
-        delete_course_and_groups(self.course_key, self.user.id)
+        delete_course(self.course_key, self.user.id)
 
         # check that user's enrollment for this course is not deleted
         self.assertTrue(CourseEnrollment.is_enrolled(self.user, self.course_key))
@@ -78,7 +78,7 @@ class TestUsersDefaultRole(ModuleStoreTestCase):
         self.assertTrue(self.user.roles.filter(name="Student", course_id=self.course_key))
 
         # delete this course and recreate this course with same user
-        delete_course_and_groups(self.course_key, self.user.id)
+        delete_course(self.course_key, self.user.id)
         resp = self._create_course_with_given_location(self.course_key)
         self.assertEqual(resp.status_code, 200)
 
@@ -96,7 +96,7 @@ class TestUsersDefaultRole(ModuleStoreTestCase):
         # check that user has enrollment and his default "Student" forum role for this course
         self.assertTrue(CourseEnrollment.is_enrolled(self.user, self.course_key))
         # delete this course and recreate this course with same user
-        delete_course_and_groups(self.course_key, self.user.id)
+        delete_course(self.course_key, self.user.id)
 
         # now create same course with different name case ('uppercase')
         new_course_key = self.course_key.replace(course=self.course_key.course.upper())

cms/djangoapps/contentstore/utils.py

@@ -61,22 +61,38 @@ def remove_all_instructors(course_key):
     instructor_role.remove_users(*instructor_role.users_with_role())
 
 
-def delete_course_and_groups(course_key, user_id):
+def delete_course(course_key, user_id, keep_instructors=False):
     """
-    This deletes the courseware associated with a course_key as well as cleaning update_item
-    the various user table stuff (groups, permissions, etc.)
+    Delete course from module store and if specified remove user and
+    groups permissions from course.
+    """
+    _delete_course_from_modulestore(course_key, user_id)
+
+    if not keep_instructors:
+        _remove_instructors(course_key)
+
+
+def _delete_course_from_modulestore(course_key, user_id):
+    """
+    Delete course from MongoDB. Deleting course will fire a signal which will result into
+    deletion of the courseware associated with a course_key.
     """
     module_store = modulestore()
 
     with module_store.bulk_operations(course_key):
         module_store.delete_course(course_key, user_id)
 
-        print 'removing User permissions from course....'
-        # in the django layer, we need to remove all the user permissions groups associated with this course
-        try:
-            remove_all_instructors(course_key)
-        except Exception as err:
-            log.error("Error in deleting course groups for {0}: {1}".format(course_key, err))
+
+def _remove_instructors(course_key):
+    """
+    In the django layer, remove all the user/groups permissions associated with this course
+    """
+    print 'removing User permissions from course....'
+
+    try:
+        remove_all_instructors(course_key)
+    except Exception as err:
+        log.error("Error in deleting course groups for {0}: {1}".format(course_key, err))
 
 
 def get_lms_link_for_item(location, preview=False):