diff --git a/cms/djangoapps/contentstore/management/commands/delete_course.py b/cms/djangoapps/contentstore/management/commands/delete_course.py index 989aa6c5b9..1870bce5bd 100644 --- a/cms/djangoapps/contentstore/management/commands/delete_course.py +++ b/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) diff --git a/cms/djangoapps/contentstore/management/commands/tests/test_delete_course.py b/cms/djangoapps/contentstore/management/commands/tests/test_delete_course.py index 5ea41377eb..28a26b41ed 100644 --- a/cms/djangoapps/contentstore/management/commands/tests/test_delete_course.py +++ b/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)) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index f7f52f42f1..5dd616ebfc 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/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() diff --git a/cms/djangoapps/contentstore/tests/test_course_listing.py b/cms/djangoapps/contentstore/tests/test_course_listing.py index 1e7cdf4a87..296b53bb61 100644 --- a/cms/djangoapps/contentstore/tests/test_course_listing.py +++ b/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) diff --git a/cms/djangoapps/contentstore/tests/test_users_default_role.py b/cms/djangoapps/contentstore/tests/test_users_default_role.py index 0817195525..32648657ca 100644 --- a/cms/djangoapps/contentstore/tests/test_users_default_role.py +++ b/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()) diff --git a/cms/djangoapps/contentstore/utils.py b/cms/djangoapps/contentstore/utils.py index d41c313e7d..a32686736b 100644 --- a/cms/djangoapps/contentstore/utils.py +++ b/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):