From 643e82ace3cedb59e49860eb2c443cf6a136a542 Mon Sep 17 00:00:00 2001 From: zubair-arbi Date: Tue, 29 Apr 2014 18:31:27 +0500 Subject: [PATCH] Revert "remove location of item from loc_mapper and cache" This reverts commit d2b8b4f0dd43cfec379b2e12cc9f723ee0fab7d6. --- cms/djangoapps/contentstore/views/item.py | 3 - .../contentstore/views/tests/test_item.py | 45 +-------- .../xmodule/modulestore/loc_mapper_store.py | 93 +++++++------------ 3 files changed, 32 insertions(+), 109 deletions(-) diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py index 82c54af153..8e0e5fc14a 100644 --- a/cms/djangoapps/contentstore/views/item.py +++ b/cms/djangoapps/contentstore/views/item.py @@ -125,9 +125,6 @@ def xblock_handler(request, tag=None, package_id=None, branch=None, version_guid delete_children = str_to_bool(request.REQUEST.get('recurse', 'False')) delete_all_versions = str_to_bool(request.REQUEST.get('all_versions', 'False')) - # delete item from loc_mapper and cache - loc_mapper().delete_item_mapping(locator, old_location) - return _delete_item_at_location(old_location, delete_children, delete_all_versions, request.user) else: # Since we have a package_id, we are updating an existing xblock. if block == 'handouts' and old_location is None: diff --git a/cms/djangoapps/contentstore/views/tests/test_item.py b/cms/djangoapps/contentstore/views/tests/test_item.py index bcbad62802..5f4f3a46df 100644 --- a/cms/djangoapps/contentstore/views/tests/test_item.py +++ b/cms/djangoapps/contentstore/views/tests/test_item.py @@ -3,7 +3,6 @@ import json from datetime import datetime import ddt -from uuid import uuid4 from mock import patch from pytz import UTC @@ -57,10 +56,7 @@ class ItemTest(CourseTestCase): parsed = json.loads(response.content) return parsed['locator'] - def create_xblock(self, parent_locator=None, display_name=None, category=None, boilerplate=None, type=None): - """ - Create xblock - """ + def create_xblock(self, parent_locator=None, display_name=None, category=None, boilerplate=None): data = { 'parent_locator': self.unicode_locator if parent_locator is None else parent_locator, 'category': category @@ -69,8 +65,6 @@ class ItemTest(CourseTestCase): data['display_name'] = display_name if boilerplate is not None: data['boilerplate'] = boilerplate - if type is not None: - data['type'] = type return self.client.ajax_post('/xblock', json.dumps(data)) @@ -679,43 +673,6 @@ class TestEditItem(ItemTest): draft = self.get_item_from_modulestore(self.problem_locator, True) self.assertNotEqual(draft.data, published.data) - def test_create_delete_discussion_components(self): - """ - Create discussion components and test that they are deleted from loc_mapper on delete - """ - # Test that creating two discussion components with first 3 digit same of uuid will have - # different block_ids (locators) - with patch("uuid.UUID.hex", '987' + uuid4().hex[3:]): - resp = self.create_xblock(parent_locator=self.seq_locator, category='discussion', type='discussion') - self.assertEqual(resp.status_code, 200) - discussion_1_locator = self.response_locator(resp) - discussion_1_draft = self.get_item_from_modulestore(discussion_1_locator, True) - self.assertIsNotNone(discussion_1_draft) - - with patch("uuid.UUID.hex", '987' + uuid4().hex[3:]): - resp = self.create_xblock(parent_locator=self.seq_locator, category='discussion', type='discussion') - self.assertEqual(resp.status_code, 200) - discussion_2_locator = self.response_locator(resp) - discussion_2_draft = self.get_item_from_modulestore(discussion_2_locator, True) - self.assertIsNotNone(discussion_2_draft) - - self.assertNotEqual(discussion_1_locator, discussion_2_locator) - # now delete first discussion component - resp = self.client.delete('/xblock/' + discussion_1_locator) - self.assertEqual(resp.status_code, 204) - - # create a new discussion component and check that it has same locator as first discussion component - # but different id's - with patch("uuid.UUID.hex", '987' + uuid4().hex[3:]): - resp = self.create_xblock(parent_locator=self.seq_locator, category='discussion', type='discussion') - self.assertEqual(resp.status_code, 200) - discussion_3_locator = self.response_locator(resp) - discussion_3_draft = self.get_item_from_modulestore(discussion_3_locator, True) - self.assertIsNotNone(discussion_3_draft) - - self.assertEqual(discussion_1_locator, discussion_3_locator) - self.assertNotEqual(discussion_1_draft.id, discussion_3_draft.id) - def test_publish_states_of_nested_xblocks(self): """ Test publishing of a unit page containing a nested xblock """ diff --git a/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py b/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py index d60abedb9a..eedb933336 100644 --- a/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py +++ b/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py @@ -152,9 +152,25 @@ class LocMapperStore(object): if cached_value: return cached_value - entry = self._get_map_entry_form_location(location_id, location, add_entry_if_missing) - if entry is None: - raise ItemNotFoundError(location) + maps = self.location_map.find(location_id) + maps = list(maps) + if len(maps) == 0: + if add_entry_if_missing: + # create a new map + course_location = location.replace(category='course', name=location_id['_id']['name']) + self.create_map_entry(course_location) + entry = self.location_map.find_one(location_id) + else: + raise ItemNotFoundError(location) + elif len(maps) == 1: + entry = maps[0] + else: + # find entry w/o name, if any; otherwise, pick arbitrary + entry = maps[0] + for item in maps: + if 'name' not in item['_id']: + entry = item + break block_id = entry['block_map'].get(self.encode_key_for_mongo(location.name)) if block_id is None: @@ -193,34 +209,6 @@ class LocMapperStore(object): self._cache_location_map_entry(old_style_course_id, location, published_usage, draft_usage) return result - def _get_map_entry_form_location(self, location_id, location=None, add_entry_if_missing=False): - """ - Returns loc_mapper entry related to provided location_id - - :param location: a Location pointing to a module - :param location_id: location id in form of SON dict for querying the mapping table if location is provided - :param add_entry_if_missing: a boolean as to whether to return none or to create an entry - """ - maps = self.location_map.find(location_id) - maps = list(maps) - entry = None - if len(maps) == 0: - if location is not None and add_entry_if_missing: - # create a new map - course_location = location.replace(category='course', name=location_id['_id']['name']) - self.create_map_entry(course_location) - entry = self.location_map.find_one(location_id) - elif len(maps) == 1: - entry = maps[0] - else: - # find entry w/o name, if any; otherwise, pick arbitrary - entry = maps[0] - for item in maps: - if 'name' not in item['_id']: - entry = item - break - return entry - def translate_locator_to_location(self, locator, get_course=False, lower_only=False): """ Returns an old style Location for the given Locator if there's an appropriate entry in the @@ -311,10 +299,19 @@ class LocMapperStore(object): location_id = self._interpret_location_course_id(old_style_course_id, location, lower_only) - entry = self._get_map_entry_form_location(location_id) - if entry is None: + maps = self.location_map.find(location_id) + maps = list(maps) + if len(maps) == 0: raise ItemNotFoundError(location) - + elif len(maps) == 1: + entry = maps[0] + else: + # find entry w/o name, if any; otherwise, pick arbitrary + entry = maps[0] + for item in maps: + if 'name' not in item['_id']: + entry = item + break published_course_locator = CourseLocator(package_id=entry['course_id'], branch=entry['prod_branch']) draft_course_locator = CourseLocator(package_id=entry['course_id'], branch=entry['draft_branch']) self._cache_course_locator(old_style_course_id, published_course_locator, draft_course_locator) @@ -341,20 +338,6 @@ class LocMapperStore(object): self.location_map.update(location_id, {'$set': {'block_map': block_map}}) return block_id - def _remove_from_block_map(self, location, location_id, block_map): - """ - Remove the given location from the block_map and persist it - """ - encoded_location_name = self.encode_key_for_mongo(location.name) - if encoded_location_name in block_map.keys(): - map_entry = block_map[encoded_location_name] - if location.category in map_entry: - if len(map_entry) == 1: - del block_map[encoded_location_name] - else: - del map_entry[location.category] - self.location_map.update(location_id, {'$set': {'block_map': block_map}}) - def _interpret_location_course_id(self, course_id, location, lower_only=False): """ Take the old style course id (org/course/run) and return a dict w/ a SON for querying the mapping table. @@ -548,17 +531,3 @@ class LocMapperStore(object): delete_keys.append(u'{}+{}'.format(old_course_id, location.url())) delete_keys.append(old_course_id) self.cache.delete_many(delete_keys) - - def delete_item_mapping(self, locator, location): - """ - Delete item from loc_mapper and cache - - :param locator: a BlockUsageLocator - :param location: a Location pointing to a module - """ - course_location = self.translate_locator_to_location(locator, get_course=True) - location_id = self._interpret_location_course_id(course_location.course_id, location) - - entry = self._get_map_entry_form_location(location_id) - if entry is not None: - self._remove_from_block_map(location, location_id, entry['block_map'])