From d723425a83edb4c2a199e722bfe77421a97f5816 Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Thu, 2 Oct 2014 10:29:02 -0400 Subject: [PATCH] Fix cached definition change detection LMS-11485 --- .../tests/test_course_settings.py | 22 +++++++++++++++++-- .../models/settings/course_grading.py | 1 - common/lib/xmodule/xmodule/course_module.py | 1 + .../split_mongo/definition_lazy_loader.py | 7 +++++- .../xmodule/modulestore/split_mongo/split.py | 3 +-- .../xmodule/modulestore/tests/factories.py | 13 ++++++++--- 6 files changed, 38 insertions(+), 9 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_course_settings.py b/cms/djangoapps/contentstore/tests/test_course_settings.py index 08587ee5ed..63926053e3 100644 --- a/cms/djangoapps/contentstore/tests/test_course_settings.py +++ b/cms/djangoapps/contentstore/tests/test_course_settings.py @@ -20,11 +20,14 @@ from xmodule.fields import Date from .utils import CourseTestCase from xmodule.modulestore.django import modulestore from contentstore.views.component import ADVANCED_COMPONENT_POLICY_KEY +import ddt +from xmodule.modulestore import ModuleStoreEnum def get_url(course_id, handler_name='settings_handler'): return reverse_course_url(handler_name, course_id) + class CourseDetailsTestCase(CourseTestCase): """ Tests the first course settings page (course dates, overview, etc.). @@ -139,7 +142,6 @@ class CourseDetailsTestCase(CourseTestCase): response = self.client.get_html(settings_details_url) self.assertNotContains(response, "Course Short Description") - def test_regular_site_fetch(self): settings_details_url = get_url(self.course.id) @@ -240,6 +242,7 @@ class CourseDetailsViewTest(CourseTestCase): self.fail(field + " included in encoding but missing from details at " + context) +@ddt.ddt class CourseGradingTest(CourseTestCase): """ Tests for the course settings grading page. @@ -258,7 +261,10 @@ class CourseGradingTest(CourseTestCase): subgrader = CourseGradingModel.fetch_grader(self.course.id, i) self.assertDictEqual(grader, subgrader, str(i) + "th graders not equal") - def test_update_from_json(self): + @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split) + def test_update_from_json(self, store): + self.course = CourseFactory.create(default_store=store) + test_grader = CourseGradingModel.fetch(self.course.id) altered_grader = CourseGradingModel.update_from_json(self.course.id, test_grader.__dict__, self.user) self.assertDictEqual(test_grader.__dict__, altered_grader.__dict__, "Noop update") @@ -267,6 +273,18 @@ class CourseGradingTest(CourseTestCase): altered_grader = CourseGradingModel.update_from_json(self.course.id, test_grader.__dict__, self.user) self.assertDictEqual(test_grader.__dict__, altered_grader.__dict__, "Weight[0] * 2") + # test for bug LMS-11485 + with modulestore().bulk_operations(self.course.id): + new_grader = test_grader.graders[0].copy() + new_grader['type'] += '_foo' + new_grader['short_label'] += '_foo' + new_grader['id'] = len(test_grader.graders) + test_grader.graders.append(new_grader) + # don't use altered cached def, get a fresh one + CourseGradingModel.update_from_json(self.course.id, test_grader.__dict__, self.user) + altered_grader = CourseGradingModel.fetch(self.course.id) + self.assertDictEqual(test_grader.__dict__, altered_grader.__dict__) + test_grader.grade_cutoffs['D'] = 0.3 altered_grader = CourseGradingModel.update_from_json(self.course.id, test_grader.__dict__, self.user) self.assertDictEqual(test_grader.__dict__, altered_grader.__dict__, "cutoff add D") diff --git a/cms/djangoapps/models/settings/course_grading.py b/cms/djangoapps/models/settings/course_grading.py index eb50047c58..35a279e28a 100644 --- a/cms/djangoapps/models/settings/course_grading.py +++ b/cms/djangoapps/models/settings/course_grading.py @@ -1,6 +1,5 @@ from datetime import timedelta from xmodule.modulestore.django import modulestore -from xblock.fields import Scope class CourseGradingModel(object): diff --git a/common/lib/xmodule/xmodule/course_module.py b/common/lib/xmodule/xmodule/course_module.py index 445639d5f3..08402c38bf 100644 --- a/common/lib/xmodule/xmodule/course_module.py +++ b/common/lib/xmodule/xmodule/course_module.py @@ -24,6 +24,7 @@ _ = lambda text: text DEFAULT_START_DATE = datetime(2030, 1, 1, tzinfo=UTC()) + class StringOrDate(Date): def from_json(self, value): """ diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/definition_lazy_loader.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/definition_lazy_loader.py index 226d36c606..8bbc9e0ab5 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/definition_lazy_loader.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/definition_lazy_loader.py @@ -1,4 +1,5 @@ from opaque_keys.edx.locator import DefinitionLocator +import copy class DefinitionLazyLoader(object): @@ -24,4 +25,8 @@ class DefinitionLazyLoader(object): Fetch the definition. Note, the caller should replace this lazy loader pointer with the result so as not to fetch more than once """ - return self.modulestore.get_definition(self.course_key, self.definition_locator.definition_id) + # get_definition may return a cached value perhaps from another course or code path + # so, we copy the result here so that updates don't cross-pollinate nor change the cached + # value in such a way that we can't tell that the definition's been updated. + definition = self.modulestore.get_definition(self.course_key, self.definition_locator.definition_id) + return copy.deepcopy(definition) diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index eefe89fe37..03ed1d5d37 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py @@ -361,7 +361,6 @@ class SplitBulkWriteMixin(BulkOperationsMixin): definitions.extend(self.db_connection.get_definitions(list(ids))) return definitions - def update_definition(self, course_key, definition): """ Update a definition, respecting the current bulk operation status @@ -1892,7 +1891,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): # find the parents and put root in the right sequence parent = self._get_parent_from_structure(BlockKey.from_usage_key(subtree_root), source_structure) if parent is not None: # may be a detached category xblock - if not parent in destination_blocks: + if parent not in destination_blocks: raise ItemNotFoundError(parent) orphans.update( self._sync_children( diff --git a/common/lib/xmodule/xmodule/modulestore/tests/factories.py b/common/lib/xmodule/xmodule/modulestore/tests/factories.py index 0e0366b567..b3dde849d3 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/factories.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/factories.py @@ -63,10 +63,17 @@ class CourseFactory(XModuleFactory): run = kwargs.get('run', name) user_id = kwargs.pop('user_id', ModuleStoreEnum.UserID.test) + # Pass the metadata just as field=value pairs + kwargs.update(kwargs.pop('metadata', {})) + default_store_override = kwargs.pop('default_store', None) + with store.branch_setting(ModuleStoreEnum.Branch.draft_preferred): - # Write the data to the mongo datastore - kwargs.update(kwargs.get('metadata', {})) - new_course = store.create_course(org, number, run, user_id, fields=kwargs) + if default_store_override is not None: + with store.default_store(default_store_override): + new_course = store.create_course(org, number, run, user_id, fields=kwargs) + else: + new_course = store.create_course(org, number, run, user_id, fields=kwargs) + last_course.loc = new_course.location return new_course