Merge pull request #2985 from edx/zub/story/deletecoursefromlocmapper

remove course location from loc_mapper on delete

cms/djangoapps/contentstore/tests/test_course_listing.py

@@ -10,6 +10,7 @@ from django.contrib.auth.models import Group
 from django.test import RequestFactory
 
 from contentstore.views.course import _accessible_courses_list, _accessible_courses_list_from_groups
+from contentstore.utils import delete_course_and_groups
 from contentstore.tests.utils import AjaxEnabledTestClient
 from student.tests.factories import UserFactory
 from student.roles import CourseInstructorRole, CourseStaffRole
@@ -19,7 +20,7 @@ from xmodule.modulestore.exceptions import ItemNotFoundError
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 from xmodule.modulestore.tests.factories import CourseFactory
 
-TOTAL_COURSES_COUNT = 1000
+TOTAL_COURSES_COUNT = 500
 USER_COURSES_COUNT = 50
 
 
@@ -56,18 +57,18 @@ class TestCourseListing(ModuleStoreTestCase):
             groupnames = role(course_locator)._group_names
             if group_name_format == 'group_name_with_course_name_only':
                 # Create role (instructor/staff) groups with course_name only: 'instructor_run'
-                group, _ = Group.objects.get_or_create(name=groupnames[2])
+                group, __ = Group.objects.get_or_create(name=groupnames[2])
             elif group_name_format == 'group_name_with_slashes':
                 # Create role (instructor/staff) groups with format: 'instructor_edX/Course/Run'
                 # Since "Group.objects.get_or_create(name=groupnames[1])" would have made group with lowercase name
                 # so manually create group name of old type
                 if role == CourseInstructorRole:
-                    group, _ = Group.objects.get_or_create(name=u'{}_{}'.format('instructor', course_location.course_id))
+                    group, __ = Group.objects.get_or_create(name=u'{}_{}'.format('instructor', course_location.course_id))
                 else:
-                    group, _ = Group.objects.get_or_create(name=u'{}_{}'.format('staff', course_location.course_id))
+                    group, __ = Group.objects.get_or_create(name=u'{}_{}'.format('staff', course_location.course_id))
             else:
                 # Create role (instructor/staff) groups with format: 'instructor_edx.course.run'
-                group, _ = Group.objects.get_or_create(name=groupnames[0])
+                group, __ = Group.objects.get_or_create(name=groupnames[0])
 
             if user is not None:
                 user.groups.add(group)
@@ -145,18 +146,6 @@ class TestCourseListing(ModuleStoreTestCase):
         with self.assertRaises(ItemNotFoundError):
             courses_list_by_groups = _accessible_courses_list_from_groups(request)
 
-
-    # Temporarily disabling this test because it caused the following failure intermittently in Jenkins.
-    # Perhaps due to a test ordering or cleanup issue?
-    # 
-    # 1) FAIL: test_course_listing_performance (contentstore.tests.test_course_listing.TestCourseListing)
-    # 
-    #    Traceback (most recent call last):
-    #     cms/djangoapps/contentstore/tests/test_course_listing.py line 176 in test_course_listing_performance
-    #       self.assertEqual(len(courses_list), USER_COURSES_COUNT)
-    #    AssertionError: 49 != 50
-    #
-    @skip
     def test_course_listing_performance(self):
         """
         Create large number of courses and give access of some of these courses to the user and
@@ -173,7 +162,7 @@ class TestCourseListing(ModuleStoreTestCase):
         user_course_ids = random.sample(range(TOTAL_COURSES_COUNT), USER_COURSES_COUNT)
 
         # create courses and assign those to the user which have their number in user_course_ids
-        for number in range(1, TOTAL_COURSES_COUNT):
+        for number in range(TOTAL_COURSES_COUNT):
             org = 'Org{0}'.format(number)
             course = 'Course{0}'.format(number)
             run = 'Run{0}'.format(number)
@@ -207,3 +196,63 @@ class TestCourseListing(ModuleStoreTestCase):
         # taken by traversing through all courses (if accessible courses are relatively small)
         self.assertGreaterEqual(iteration_over_courses_time_1.elapsed, iteration_over_groups_time_1.elapsed)
         self.assertGreaterEqual(iteration_over_courses_time_2.elapsed, iteration_over_groups_time_2.elapsed)
+
+    def test_get_course_list_with_same_course_id(self):
+        """
+        Test getting courses with same id but with different name case. Then try to delete one of them and
+        check that it is properly deleted and other one is accessible
+        """
+        request = self.factory.get('/course')
+        request.user = self.user
+
+        course_location_caps = Location(['i4x', 'Org', 'COURSE', 'course', 'Run'])
+        self._create_course_with_access_groups(course_location_caps, 'group_name_with_dots', self.user)
+
+        # get courses through iterating all courses
+        courses_list = _accessible_courses_list(request)
+        self.assertEqual(len(courses_list), 1)
+
+        # get courses by reversing group name formats
+        courses_list_by_groups = _accessible_courses_list_from_groups(request)
+        self.assertEqual(len(courses_list_by_groups), 1)
+        # check both course lists have same courses
+        self.assertEqual(courses_list, courses_list_by_groups)
+
+        # now create another course with same course_id but different name case
+        course_location_camel = Location(['i4x', 'Org', 'Course', 'course', 'Run'])
+        self._create_course_with_access_groups(course_location_camel, 'group_name_with_dots', self.user)
+
+        # test that get courses through iterating all courses returns both courses
+        courses_list = _accessible_courses_list(request)
+        self.assertEqual(len(courses_list), 2)
+
+        # test that get courses by reversing group name formats returns only one course
+        courses_list_by_groups = _accessible_courses_list_from_groups(request)
+        self.assertEqual(len(courses_list_by_groups), 1)
+
+        course_locator = loc_mapper().translate_location(course_location_caps.course_id, course_location_caps)
+        outline_url = course_locator.url_reverse('course/')
+        # now delete first course (course_location_caps) and check that it is no longer accessible
+        delete_course_and_groups(course_location_caps.course_id, commit=True)
+        # add user to this course instructor group since he was removed from that group on course delete
+        instructor_group_name = CourseInstructorRole(course_locator)._group_names[0]  # pylint: disable=protected-access
+        group, __ = Group.objects.get_or_create(name=instructor_group_name)
+        self.user.groups.add(group)
+
+        # test that get courses through iterating all courses now returns one course
+        courses_list = _accessible_courses_list(request)
+        self.assertEqual(len(courses_list), 1)
+
+        # test that get courses by reversing group name formats also returns one course
+        courses_list_by_groups = _accessible_courses_list_from_groups(request)
+        self.assertEqual(len(courses_list_by_groups), 1)
+
+        # now check that deleted course in not accessible
+        response = self.client.get(outline_url, HTTP_ACCEPT='application/json')
+        self.assertEqual(response.status_code, 403)
+
+        # now check that other course in accessible
+        course_locator = loc_mapper().translate_location(course_location_camel.course_id, course_location_camel)
+        outline_url = course_locator.url_reverse('course/')
+        response = self.client.get(outline_url, HTTP_ACCEPT='application/json')
+        self.assertEqual(response.status_code, 200)

cms/djangoapps/contentstore/views/course.py

@@ -73,7 +73,11 @@ def _get_locator_and_course(package_id, branch, version_guid, block_id, user, de
     locator = BlockUsageLocator(package_id=package_id, branch=branch, version_guid=version_guid, block_id=block_id)
     if not has_course_access(user, locator):
         raise PermissionDenied()
+
     course_location = loc_mapper().translate_locator_to_location(locator)
+    if course_location is None:
+        raise PermissionDenied()
+
     course_module = modulestore().get_item(course_location, depth=depth)
     return locator, course_module
 

common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py

@@ -500,3 +500,34 @@ class LocMapperStore(object):
         setmany[u'{}+{}'.format(old_course_id, location.url())] = (published_usage, draft_usage)
         setmany[old_course_id] = (published_usage, draft_usage)
         self.cache.set_many(setmany)
+
+    def delete_course_mapping(self, course_location):
+        """
+        Remove provided course location from loc_mapper and cache.
+
+        :param course_location: a Location whose category is 'course'.
+        """
+        course_locator = self.translate_location(course_location.course_id, course_location)
+        course_locator_draft = self.translate_location(
+            course_location.course_id, course_location, published=False
+        )
+
+        self.location_map.remove({'course_id': course_locator.package_id})
+        self._delete_cache_location_map_entry(
+            course_location.course_id, course_location, course_locator, course_locator_draft
+        )
+
+    def _delete_cache_location_map_entry(self, old_course_id, location, published_usage, draft_usage):
+        """
+        Remove the location of course (draft and published) from cache
+        """
+        delete_keys = []
+        if location.category == 'course':
+            delete_keys.append(u'courseId+{}'.format(published_usage.package_id))
+            delete_keys.append(u'courseIdLower+{}'.format(published_usage.package_id.lower()))
+
+        delete_keys.append(unicode(published_usage))
+        delete_keys.append(unicode(draft_usage))
+        delete_keys.append(u'{}+{}'.format(old_course_id, location.url()))
+        delete_keys.append(old_course_id)
+        self.cache.delete_many(delete_keys)

common/lib/xmodule/xmodule/modulestore/store_utilities.py

 import re
 from xmodule.contentstore.content import StaticContent
 from xmodule.modulestore import Location
+from xmodule.modulestore.django import loc_mapper
 
 import logging
 
@@ -266,4 +267,7 @@ def delete_course(modulestore, contentstore, source_location, commit=False):
     if commit:
         modulestore.delete_item(source_location)
 
+        # remove location of this course from loc_mapper and cache
+        loc_mapper().delete_course_mapping(source_location)
+
     return True

common/lib/xmodule/xmodule/modulestore/tests/test_location_mapper.py

@@ -80,6 +80,38 @@ class TestLocationMapper(LocMapperSetupSansDjango):
         self.assertEqual(entry['prod_branch'], 'live')
         self.assertEqual(entry['block_map'], block_map)
 
+    def test_delete_course_map(self):
+        """
+        Test that course location is properly remove from loc_mapper and cache when course is deleted
+        """
+        org = u'foo_org'
+        course = u'bar_course'
+        run = u'baz_run'
+        course_location = Location('i4x', org, course, 'course', run)
+        course_locator = loc_mapper().translate_location(course_location.course_id, course_location)
+        loc_mapper().create_map_entry(course_location)
+        # pylint: disable=protected-access
+        entry = loc_mapper().location_map.find_one({
+            '_id': loc_mapper()._construct_location_son(org, course, run)
+        })
+        self.assertIsNotNone(entry, 'Entry not found in loc_mapper')
+        self.assertEqual(entry['course_id'], u'{0}.{1}.{2}'.format(org, course, run))
+
+        # now delete course location from loc_mapper and cache and test that course location no longer
+        # exists in loca_mapper and cache
+        loc_mapper().delete_course_mapping(course_location)
+        # pylint: disable=protected-access
+        entry = loc_mapper().location_map.find_one({
+            '_id': loc_mapper()._construct_location_son(org, course, run)
+        })
+        self.assertIsNone(entry, 'Entry found in loc_mapper')
+        # pylint: disable=protected-access
+        cached_value = loc_mapper()._get_location_from_cache(course_locator)
+        self.assertIsNone(cached_value, 'course_locator found in cache')
+        # pylint: disable=protected-access
+        cached_value = loc_mapper()._get_course_location_from_cache(course_locator.package_id)
+        self.assertIsNone(cached_value, 'Entry found in cache')
+
     def translate_n_check(self, location, old_style_course_id, new_style_package_id, block_id, branch, add_entry=False):
         """
         Request translation, check package_id, block_id, and branch
@@ -427,3 +459,10 @@ class TrivialCache(object):
         mock set
         """
         self.cache[key] = entry
+
+    def delete_many(self, entries):
+        """
+        mock delete_many
+        """
+        for entry in entries:
+            del self.cache[entry]