From 9d04a9270b0ce899dcddc716ea038395d502bb73 Mon Sep 17 00:00:00 2001 From: zubiar-arbi Date: Wed, 19 Mar 2014 17:09:21 +0500 Subject: [PATCH] remove course location from loc_mapper on delete --- .../contentstore/tests/test_course_listing.py | 85 +++++++++++++++---- cms/djangoapps/contentstore/views/course.py | 4 + .../xmodule/modulestore/loc_mapper_store.py | 31 +++++++ .../xmodule/modulestore/store_utilities.py | 4 + .../modulestore/tests/test_location_mapper.py | 39 +++++++++ 5 files changed, 145 insertions(+), 18 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_course_listing.py b/cms/djangoapps/contentstore/tests/test_course_listing.py index 58691a45bc..bdf95a3496 100644 --- a/cms/djangoapps/contentstore/tests/test_course_listing.py +++ b/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) diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index b50cc7ee55..ce577cb903 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/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 diff --git a/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py b/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py index b67454865e..eedb933336 100644 --- a/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py +++ b/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) diff --git a/common/lib/xmodule/xmodule/modulestore/store_utilities.py b/common/lib/xmodule/xmodule/modulestore/store_utilities.py index 9d11994506..92fd23f58e 100644 --- a/common/lib/xmodule/xmodule/modulestore/store_utilities.py +++ b/common/lib/xmodule/xmodule/modulestore/store_utilities.py @@ -1,6 +1,7 @@ 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 diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_location_mapper.py b/common/lib/xmodule/xmodule/modulestore/tests/test_location_mapper.py index 7a878cc9f1..395271f91b 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_location_mapper.py +++ b/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]