From 3f9431e8cff9ba0ad0cf2308e3290334cd8624fa Mon Sep 17 00:00:00 2001 From: Diana Huang <dkh@edx.org> Date: Wed, 3 Jul 2013 14:38:22 -0400 Subject: [PATCH] Provide implicit saves for XBlocks and XModules. Update existing tests and provide new ones to test new paradigm. --- CHANGELOG.rst | 2 + .../contentstore/tests/test_checklists.py | 2 + .../contentstore/tests/test_contentstore.py | 13 ++++ .../tests/test_course_settings.py | 65 ++++++++++++++++ .../contentstore/tests/test_textbooks.py | 6 ++ cms/djangoapps/contentstore/views/course.py | 17 ++++- cms/djangoapps/contentstore/views/item.py | 3 + cms/djangoapps/contentstore/views/preview.py | 9 ++- cms/djangoapps/contentstore/views/tabs.py | 3 + .../models/settings/course_details.py | 4 + .../models/settings/course_grading.py | 45 +++++++---- .../models/settings/course_metadata.py | 7 ++ common/djangoapps/xmodule_modifiers.py | 15 ++++ .../xmodule/xmodule/modulestore/mongo/base.py | 13 +++- .../xmodule/modulestore/tests/django_utils.py | 21 +++-- .../xmodule/modulestore/tests/factories.py | 4 - common/lib/xmodule/xmodule/modulestore/xml.py | 4 + .../xmodule/tests/test_combined_open_ended.py | 6 +- .../xmodule/xmodule/tests/test_conditional.py | 7 +- lms/djangoapps/courseware/model_data.py | 37 +++------ lms/djangoapps/courseware/module_render.py | 76 ++++++++++++------- .../courseware/tests/test_model_data.py | 30 ++++++-- .../courseware/tests/test_module_render.py | 61 ++++++++++++++- lms/djangoapps/courseware/views.py | 2 + requirements/edx/github.txt | 2 +- 25 files changed, 349 insertions(+), 105 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 04c8a5baaee..13015d6ce6f 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -9,6 +9,8 @@ Common: Added *experimental* support for jsinput type. Common: Added setting to specify Celery Broker vhost +Common: Utilize new XBlock bulk save API in LMS and CMS. + Studio: Add table for tracking course creator permissions (not yet used). Update rake django-admin[syncdb] and rake django-admin[migrate] so they run for both LMS and CMS. diff --git a/cms/djangoapps/contentstore/tests/test_checklists.py b/cms/djangoapps/contentstore/tests/test_checklists.py index 99ffb8678db..02999f65671 100644 --- a/cms/djangoapps/contentstore/tests/test_checklists.py +++ b/cms/djangoapps/contentstore/tests/test_checklists.py @@ -46,6 +46,8 @@ class ChecklistTestCase(CourseTestCase): # Now delete the checklists from the course and verify they get repopulated (for courses # created before checklists were introduced). self.course.checklists = None + # Save the changed `checklists` to the underlying KeyValueStore before updating the modulestore + self.course.save() modulestore = get_modulestore(self.course.location) modulestore.update_metadata(self.course.location, own_metadata(self.course)) self.assertEqual(self.get_persisted_checklists(), None) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 500db414f4d..839175f04d9 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -87,6 +87,8 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): self.user.is_active = True # Staff has access to view all courses self.user.is_staff = True + + # Save the data that we've just changed to the db. self.user.save() self.client = Client() @@ -117,6 +119,10 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): course.advanced_modules = component_types + # Save the data that we've just changed to the underlying + # MongoKeyValueStore before we update the mongo datastore. + course.save() + store.update_metadata(course.location, own_metadata(course)) # just pick one vertical @@ -239,6 +245,9 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): self.assertNotIn('graceperiod', own_metadata(html_module)) html_module.lms.graceperiod = new_graceperiod + # Save the data that we've just changed to the underlying + # MongoKeyValueStore before we update the mongo datastore. + html_module.save() self.assertIn('graceperiod', own_metadata(html_module)) self.assertEqual(html_module.lms.graceperiod, new_graceperiod) @@ -883,6 +892,9 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): # add a bool piece of unknown metadata so we can verify we don't throw an exception metadata['new_metadata'] = True + # Save the data that we've just changed to the underlying + # MongoKeyValueStore before we update the mongo datastore. + course.save() module_store.update_metadata(location, metadata) print 'Exporting to tempdir = {0}'.format(root_dir) @@ -1299,6 +1311,7 @@ class ContentStoreTest(ModuleStoreTestCase): # now let's define an override at the leaf node level # new_module.lms.graceperiod = timedelta(1) + new_module.save() module_store.update_metadata(new_module.location, own_metadata(new_module)) # flush the cache and refetch diff --git a/cms/djangoapps/contentstore/tests/test_course_settings.py b/cms/djangoapps/contentstore/tests/test_course_settings.py index 44eb16436d6..0862eb462d9 100644 --- a/cms/djangoapps/contentstore/tests/test_course_settings.py +++ b/cms/djangoapps/contentstore/tests/test_course_settings.py @@ -290,6 +290,71 @@ class CourseGradingTest(CourseTestCase): altered_grader = CourseGradingModel.update_grader_from_json(test_grader.course_location, test_grader.graders[1]) self.assertDictEqual(test_grader.graders[1], altered_grader, "drop_count[1] + 2") + def test_update_cutoffs_from_json(self): + test_grader = CourseGradingModel.fetch(self.course.location) + CourseGradingModel.update_cutoffs_from_json(test_grader.course_location, test_grader.grade_cutoffs) + # Unlike other tests, need to actually perform a db fetch for this test since update_cutoffs_from_json + # simply returns the cutoffs you send into it, rather than returning the db contents. + altered_grader = CourseGradingModel.fetch(self.course.location) + self.assertDictEqual(test_grader.grade_cutoffs, altered_grader.grade_cutoffs, "Noop update") + + test_grader.grade_cutoffs['D'] = 0.3 + CourseGradingModel.update_cutoffs_from_json(test_grader.course_location, test_grader.grade_cutoffs) + altered_grader = CourseGradingModel.fetch(self.course.location) + self.assertDictEqual(test_grader.grade_cutoffs, altered_grader.grade_cutoffs, "cutoff add D") + + test_grader.grade_cutoffs['Pass'] = 0.75 + CourseGradingModel.update_cutoffs_from_json(test_grader.course_location, test_grader.grade_cutoffs) + altered_grader = CourseGradingModel.fetch(self.course.location) + self.assertDictEqual(test_grader.grade_cutoffs, altered_grader.grade_cutoffs, "cutoff change 'Pass'") + + def test_delete_grace_period(self): + test_grader = CourseGradingModel.fetch(self.course.location) + CourseGradingModel.update_grace_period_from_json(test_grader.course_location, test_grader.grace_period) + # update_grace_period_from_json doesn't return anything, so query the db for its contents. + altered_grader = CourseGradingModel.fetch(self.course.location) + self.assertEqual(test_grader.grace_period, altered_grader.grace_period, "Noop update") + + test_grader.grace_period = {'hours': 15, 'minutes': 5, 'seconds': 30} + CourseGradingModel.update_grace_period_from_json(test_grader.course_location, test_grader.grace_period) + altered_grader = CourseGradingModel.fetch(self.course.location) + self.assertDictEqual(test_grader.grace_period, altered_grader.grace_period, "Adding in a grace period") + + test_grader.grace_period = {'hours': 1, 'minutes': 10, 'seconds': 0} + # Now delete the grace period + CourseGradingModel.delete_grace_period(test_grader.course_location) + # update_grace_period_from_json doesn't return anything, so query the db for its contents. + altered_grader = CourseGradingModel.fetch(self.course.location) + # Once deleted, the grace period should simply be None + self.assertEqual(None, altered_grader.grace_period, "Delete grace period") + + def test_update_section_grader_type(self): + # Get the descriptor and the section_grader_type and assert they are the default values + descriptor = get_modulestore(self.course.location).get_item(self.course.location) + section_grader_type = CourseGradingModel.get_section_grader_type(self.course.location) + + self.assertEqual('Not Graded', section_grader_type['graderType']) + self.assertEqual(None, descriptor.lms.format) + self.assertEqual(False, descriptor.lms.graded) + + # Change the default grader type to Homework, which should also mark the section as graded + CourseGradingModel.update_section_grader_type(self.course.location, {'graderType': 'Homework'}) + descriptor = get_modulestore(self.course.location).get_item(self.course.location) + section_grader_type = CourseGradingModel.get_section_grader_type(self.course.location) + + self.assertEqual('Homework', section_grader_type['graderType']) + self.assertEqual('Homework', descriptor.lms.format) + self.assertEqual(True, descriptor.lms.graded) + + # Change the grader type back to Not Graded, which should also unmark the section as graded + CourseGradingModel.update_section_grader_type(self.course.location, {'graderType': 'Not Graded'}) + descriptor = get_modulestore(self.course.location).get_item(self.course.location) + section_grader_type = CourseGradingModel.get_section_grader_type(self.course.location) + + self.assertEqual('Not Graded', section_grader_type['graderType']) + self.assertEqual(None, descriptor.lms.format) + self.assertEqual(False, descriptor.lms.graded) + class CourseMetadataEditingTest(CourseTestCase): """ diff --git a/cms/djangoapps/contentstore/tests/test_textbooks.py b/cms/djangoapps/contentstore/tests/test_textbooks.py index 02c64e94137..a21a1b10231 100644 --- a/cms/djangoapps/contentstore/tests/test_textbooks.py +++ b/cms/djangoapps/contentstore/tests/test_textbooks.py @@ -62,6 +62,9 @@ class TextbookIndexTestCase(CourseTestCase): } ] self.course.pdf_textbooks = content + # Save the data that we've just changed to the underlying + # MongoKeyValueStore before we update the mongo datastore. + self.course.save() store = get_modulestore(self.course.location) store.update_metadata(self.course.location, own_metadata(self.course)) @@ -220,6 +223,9 @@ class TextbookByIdTestCase(CourseTestCase): 'tid': 2, }) self.course.pdf_textbooks = [self.textbook1, self.textbook2] + # Save the data that we've just changed to the underlying + # MongoKeyValueStore before we update the mongo datastore. + self.course.save() self.store = get_modulestore(self.course.location) self.store.update_metadata(self.course.location, own_metadata(self.course)) self.url_nonexist = reverse('textbook_by_id', kwargs={ diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 0e16624c428..3791e6779af 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -1,10 +1,9 @@ """ Views related to operations on course objects """ -#pylint: disable=W0402 import json import random -import string +import string # pylint: disable=W0402 from django.contrib.auth.decorators import login_required from django_future.csrf import ensure_csrf_cookie @@ -496,6 +495,9 @@ def textbook_index(request, org, course, name): if not any(tab['type'] == 'pdf_textbooks' for tab in course_module.tabs): course_module.tabs.append({"type": "pdf_textbooks"}) course_module.pdf_textbooks = textbooks + # Save the data that we've just changed to the underlying + # MongoKeyValueStore before we update the mongo datastore. + course_module.save() store.update_metadata(course_module.location, own_metadata(course_module)) return JsonResponse(course_module.pdf_textbooks) else: @@ -542,6 +544,9 @@ def create_textbook(request, org, course, name): tabs = course_module.tabs tabs.append({"type": "pdf_textbooks"}) course_module.tabs = tabs + # Save the data that we've just changed to the underlying + # MongoKeyValueStore before we update the mongo datastore. + course_module.save() store.update_metadata(course_module.location, own_metadata(course_module)) resp = JsonResponse(textbook, status=201) resp["Location"] = reverse("textbook_by_id", kwargs={ @@ -585,10 +590,13 @@ def textbook_by_id(request, org, course, name, tid): i = course_module.pdf_textbooks.index(textbook) new_textbooks = course_module.pdf_textbooks[0:i] new_textbooks.append(new_textbook) - new_textbooks.extend(course_module.pdf_textbooks[i+1:]) + new_textbooks.extend(course_module.pdf_textbooks[i + 1:]) course_module.pdf_textbooks = new_textbooks else: course_module.pdf_textbooks.append(new_textbook) + # Save the data that we've just changed to the underlying + # MongoKeyValueStore before we update the mongo datastore. + course_module.save() store.update_metadata(course_module.location, own_metadata(course_module)) return JsonResponse(new_textbook, status=201) elif request.method == 'DELETE': @@ -596,7 +604,8 @@ def textbook_by_id(request, org, course, name, tid): return JsonResponse(status=404) i = course_module.pdf_textbooks.index(textbook) new_textbooks = course_module.pdf_textbooks[0:i] - new_textbooks.extend(course_module.pdf_textbooks[i+1:]) + new_textbooks.extend(course_module.pdf_textbooks[i + 1:]) course_module.pdf_textbooks = new_textbooks + course_module.save() store.update_metadata(course_module.location, own_metadata(course_module)) return JsonResponse() diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py index 90dae10c238..9a5e40e9675 100644 --- a/cms/djangoapps/contentstore/views/item.py +++ b/cms/djangoapps/contentstore/views/item.py @@ -70,6 +70,9 @@ def save_item(request): delattr(existing_item, metadata_key) else: setattr(existing_item, metadata_key, value) + # Save the data that we've just changed to the underlying + # MongoKeyValueStore before we update the mongo datastore. + existing_item.save() # commit to datastore store.update_metadata(item_location, own_metadata(existing_item)) diff --git a/cms/djangoapps/contentstore/views/preview.py b/cms/djangoapps/contentstore/views/preview.py index ba393e72f4d..35af3e9ac34 100644 --- a/cms/djangoapps/contentstore/views/preview.py +++ b/cms/djangoapps/contentstore/views/preview.py @@ -7,7 +7,7 @@ from django.core.urlresolvers import reverse from django.contrib.auth.decorators import login_required from mitxmako.shortcuts import render_to_response -from xmodule_modifiers import replace_static_urls, wrap_xmodule +from xmodule_modifiers import replace_static_urls, wrap_xmodule, save_module # pylint: disable=F0401 from xmodule.error_module import ErrorDescriptor from xmodule.errortracker import exc_info_to_str from xmodule.exceptions import NotFoundError, ProcessingError @@ -47,6 +47,8 @@ def preview_dispatch(request, preview_id, location, dispatch=None): # Let the module handle the AJAX try: ajax_return = instance.handle_ajax(dispatch, request.POST) + # Save any module data that has changed to the underlying KeyValueStore + instance.save() except NotFoundError: log.exception("Module indicating to user that request doesn't exist") @@ -166,6 +168,11 @@ def load_preview_module(request, preview_id, descriptor): course_namespace=Location([module.location.tag, module.location.org, module.location.course, None, None]) ) + module.get_html = save_module( + module.get_html, + module + ) + return module diff --git a/cms/djangoapps/contentstore/views/tabs.py b/cms/djangoapps/contentstore/views/tabs.py index 154f9fb55d4..d55932e33dc 100644 --- a/cms/djangoapps/contentstore/views/tabs.py +++ b/cms/djangoapps/contentstore/views/tabs.py @@ -76,6 +76,9 @@ def reorder_static_tabs(request): # OK, re-assemble the static tabs in the new order course.tabs = reordered_tabs + # Save the data that we've just changed to the underlying + # MongoKeyValueStore before we update the mongo datastore. + course.save() modulestore('direct').update_metadata(course.location, own_metadata(course)) return HttpResponse() diff --git a/cms/djangoapps/models/settings/course_details.py b/cms/djangoapps/models/settings/course_details.py index 8ce8c2db34b..7c3b8832839 100644 --- a/cms/djangoapps/models/settings/course_details.py +++ b/cms/djangoapps/models/settings/course_details.py @@ -122,6 +122,10 @@ class CourseDetails(object): descriptor.enrollment_end = converted if dirty: + # Save the data that we've just changed to the underlying + # MongoKeyValueStore before we update the mongo datastore. + descriptor.save() + get_modulestore(course_location).update_metadata(course_location, own_metadata(descriptor)) # NOTE: below auto writes to the db w/o verifying that any of the fields actually changed diff --git a/cms/djangoapps/models/settings/course_grading.py b/cms/djangoapps/models/settings/course_grading.py index e529a284c6b..0746fc7a902 100644 --- a/cms/djangoapps/models/settings/course_grading.py +++ b/cms/djangoapps/models/settings/course_grading.py @@ -7,6 +7,9 @@ class CourseGradingModel(object): """ Basically a DAO and Model combo for CRUD operations pertaining to grading policy. """ + # Within this class, allow access to protected members of client classes. + # This comes up when accessing kvs data and caches during kvs saves and modulestore writes. + # pylint: disable=W0212 def __init__(self, course_descriptor): self.course_location = course_descriptor.location self.graders = [CourseGradingModel.jsonize_grader(i, grader) for i, grader in enumerate(course_descriptor.raw_grader)] # weights transformed to ints [0..100] @@ -83,13 +86,16 @@ class CourseGradingModel(object): """ course_location = Location(jsondict['course_location']) descriptor = get_modulestore(course_location).get_item(course_location) - graders_parsed = [CourseGradingModel.parse_grader(jsonele) for jsonele in jsondict['graders']] descriptor.raw_grader = graders_parsed descriptor.grade_cutoffs = jsondict['grade_cutoffs'] + # Save the data that we've just changed to the underlying + # MongoKeyValueStore before we update the mongo datastore. + descriptor.save() get_modulestore(course_location).update_item(course_location, descriptor.xblock_kvs._data) + CourseGradingModel.update_grace_period_from_json(course_location, jsondict['grace_period']) return CourseGradingModel.fetch(course_location) @@ -116,6 +122,9 @@ class CourseGradingModel(object): else: descriptor.raw_grader.append(grader) + # Save the data that we've just changed to the underlying + # MongoKeyValueStore before we update the mongo datastore. + descriptor.save() get_modulestore(course_location).update_item(course_location, descriptor._model_data._kvs._data) return CourseGradingModel.jsonize_grader(index, descriptor.raw_grader[index]) @@ -131,6 +140,10 @@ class CourseGradingModel(object): descriptor = get_modulestore(course_location).get_item(course_location) descriptor.grade_cutoffs = cutoffs + + # Save the data that we've just changed to the underlying + # MongoKeyValueStore before we update the mongo datastore. + descriptor.save() get_modulestore(course_location).update_item(course_location, descriptor._model_data._kvs._data) return cutoffs @@ -156,6 +169,10 @@ class CourseGradingModel(object): descriptor = get_modulestore(course_location).get_item(course_location) descriptor.lms.graceperiod = grace_timedelta + + # Save the data that we've just changed to the underlying + # MongoKeyValueStore before we update the mongo datastore. + descriptor.save() get_modulestore(course_location).update_metadata(course_location, descriptor._model_data._kvs._metadata) @staticmethod @@ -172,22 +189,11 @@ class CourseGradingModel(object): del descriptor.raw_grader[index] # force propagation to definition descriptor.raw_grader = descriptor.raw_grader - get_modulestore(course_location).update_item(course_location, descriptor._model_data._kvs._data) - # NOTE cannot delete cutoffs. May be useful to reset - @staticmethod - def delete_cutoffs(course_location, cutoffs): - """ - Resets the cutoffs to the defaults - """ - if not isinstance(course_location, Location): - course_location = Location(course_location) - - descriptor = get_modulestore(course_location).get_item(course_location) - descriptor.grade_cutoffs = descriptor.defaut_grading_policy['GRADE_CUTOFFS'] - get_modulestore(course_location).update_item(course_location, descriptor._model_data._kvs._data) - - return descriptor.grade_cutoffs + # Save the data that we've just changed to the underlying + # MongoKeyValueStore before we update the mongo datastore. + descriptor.save() + get_modulestore(course_location).update_item(course_location, descriptor._model_data._kvs._data) @staticmethod def delete_grace_period(course_location): @@ -199,6 +205,10 @@ class CourseGradingModel(object): descriptor = get_modulestore(course_location).get_item(course_location) del descriptor.lms.graceperiod + + # Save the data that we've just changed to the underlying + # MongoKeyValueStore before we update the mongo datastore. + descriptor.save() get_modulestore(course_location).update_metadata(course_location, descriptor._model_data._kvs._metadata) @staticmethod @@ -225,6 +235,9 @@ class CourseGradingModel(object): del descriptor.lms.format del descriptor.lms.graded + # Save the data that we've just changed to the underlying + # MongoKeyValueStore before we update the mongo datastore. + descriptor.save() get_modulestore(location).update_metadata(location, descriptor._model_data._kvs._metadata) @staticmethod diff --git a/cms/djangoapps/models/settings/course_metadata.py b/cms/djangoapps/models/settings/course_metadata.py index 5fb07fe8064..8d9a2928676 100644 --- a/cms/djangoapps/models/settings/course_metadata.py +++ b/cms/djangoapps/models/settings/course_metadata.py @@ -76,6 +76,9 @@ class CourseMetadata(object): setattr(descriptor.lms, key, value) if dirty: + # Save the data that we've just changed to the underlying + # MongoKeyValueStore before we update the mongo datastore. + descriptor.save() get_modulestore(course_location).update_metadata(course_location, own_metadata(descriptor)) @@ -97,6 +100,10 @@ class CourseMetadata(object): elif hasattr(descriptor.lms, key): delattr(descriptor.lms, key) + # Save the data that we've just changed to the underlying + # MongoKeyValueStore before we update the mongo datastore. + descriptor.save() + get_modulestore(course_location).update_metadata(course_location, own_metadata(descriptor)) diff --git a/common/djangoapps/xmodule_modifiers.py b/common/djangoapps/xmodule_modifiers.py index 3914892bbf0..3efc04789e3 100644 --- a/common/djangoapps/xmodule_modifiers.py +++ b/common/djangoapps/xmodule_modifiers.py @@ -89,6 +89,21 @@ def grade_histogram(module_id): return grades +def save_module(get_html, module): + """ + Updates the given get_html function for the given module to save the fields + after rendering. + """ + @wraps(get_html) + def _get_html(): + """Cache the rendered output, save, then return the output.""" + rendered_html = get_html() + module.save() + return rendered_html + + return _get_html + + def add_histogram(get_html, module, user): """ Updates the supplied module with a new get_html function that wraps diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/base.py b/common/lib/xmodule/xmodule/modulestore/mongo/base.py index 7fc903623a1..ae879ba3e84 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/base.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/base.py @@ -108,8 +108,9 @@ class MongoKeyValueStore(KeyValueStore): def set_many(self, update_dict): """set_many method. Implementations should accept an `update_dict` of key-value pairs, and set all the `keys` to the given `value`s.""" - # It appears that `set` simply updates an in-memory db, rather than calling down - # to a real db; need to figure out if this is the case. + # `set` simply updates an in-memory db, rather than calling down to a real db, + # as mongo bulk save is handled elsewhere. A future improvement would be to pull + # the mongo-specific bulk save logic into this method. for key, value in update_dict.iteritems(): self.set(key, value) @@ -647,6 +648,8 @@ class MongoModuleStore(ModuleStoreBase): :param xmodule: """ + # Save any changes to the xmodule to the MongoKeyValueStore + xmodule.save() # split mongo's persist_dag is more general and useful. self.collection.save({ '_id': xmodule.location.dict(), @@ -691,6 +694,8 @@ class MongoModuleStore(ModuleStoreBase): 'url_slug': new_object.location.name }) course.tabs = existing_tabs + # Save any changes to the course to the MongoKeyValueStore + course.save() self.update_metadata(course.location, course.xblock_kvs._metadata) def fire_updated_modulestore_signal(self, course_id, location): @@ -797,6 +802,8 @@ class MongoModuleStore(ModuleStoreBase): tab['name'] = metadata.get('display_name') break course.tabs = existing_tabs + # Save the updates to the course to the MongoKeyValueStore + course.save() self.update_metadata(course.location, own_metadata(course)) self._update_single_item(location, {'metadata': metadata}) @@ -819,6 +826,8 @@ class MongoModuleStore(ModuleStoreBase): course = self.get_course_for_item(item.location) existing_tabs = course.tabs or [] course.tabs = [tab for tab in existing_tabs if tab.get('url_slug') != location.name] + # Save the updates to the course to the MongoKeyValueStore + course.save() self.update_metadata(course.location, own_metadata(course)) # Must include this to avoid the django debug toolbar (which defines the deprecated "safe=False") diff --git a/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py b/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py index 564aac141da..4f998d57fb3 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py @@ -165,34 +165,31 @@ class ModuleStoreTestCase(TestCase): # Call superclass implementation super(ModuleStoreTestCase, self)._post_teardown() + def assert2XX(self, status_code, msg=None): """ Assert that the given value is a success status (between 200 and 299) """ - if not 200 <= status_code < 300: - msg = self._formatMessage(msg, "%s is not a success status" % safe_repr(status_code)) - raise self.failureExecption(msg) + msg = self._formatMessage(msg, "%s is not a success status" % safe_repr(status_code)) + self.assertTrue(status_code >= 200 and status_code < 300, msg=msg) def assert3XX(self, status_code, msg=None): """ Assert that the given value is a redirection status (between 300 and 399) """ - if not 300 <= status_code < 400: - msg = self._formatMessage(msg, "%s is not a redirection status" % safe_repr(status_code)) - raise self.failureExecption(msg) + msg = self._formatMessage(msg, "%s is not a redirection status" % safe_repr(status_code)) + self.assertTrue(status_code >= 300 and status_code < 400, msg=msg) def assert4XX(self, status_code, msg=None): """ Assert that the given value is a client error status (between 400 and 499) """ - if not 400 <= status_code < 500: - msg = self._formatMessage(msg, "%s is not a client error status" % safe_repr(status_code)) - raise self.failureExecption(msg) + msg = self._formatMessage(msg, "%s is not a client error status" % safe_repr(status_code)) + self.assertTrue(status_code >= 400 and status_code < 500, msg=msg) def assert5XX(self, status_code, msg=None): """ Assert that the given value is a server error status (between 500 and 599) """ - if not 500 <= status_code < 600: - msg = self._formatMessage(msg, "%s is not a server error status" % safe_repr(status_code)) - raise self.failureExecption(msg) + msg = self._formatMessage(msg, "%s is not a server error status" % safe_repr(status_code)) + self.assertTrue(status_code >= 500 and status_code < 600, msg=msg) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/factories.py b/common/lib/xmodule/xmodule/modulestore/tests/factories.py index e442944805c..8c3b5d59dd0 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/factories.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/factories.py @@ -53,9 +53,6 @@ class XModuleCourseFactory(Factory): for k, v in kwargs.iteritems(): setattr(new_course, k, v) - # Save the data we've just created before we update mongo datastore - new_course.save() - # Update the data in the mongo datastore store.save_xmodule(new_course) return new_course @@ -138,7 +135,6 @@ class XModuleItemFactory(Factory): # replace the display name with an optional parameter passed in from the caller if display_name is not None: metadata['display_name'] = display_name - # note that location comes from above lazy_attribute store.create_and_save_xmodule(location, metadata=metadata, definition_data=data) if location.category not in DETACHED_CATEGORIES: diff --git a/common/lib/xmodule/xmodule/modulestore/xml.py b/common/lib/xmodule/xmodule/modulestore/xml.py index 012740ff9a2..8bc3142c777 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml.py +++ b/common/lib/xmodule/xmodule/modulestore/xml.py @@ -194,6 +194,10 @@ class ImportSystem(XMLParsingSystem, MakoDescriptorSystem): if hasattr(descriptor, 'children'): for child in descriptor.get_children(): parent_tracker.add_parent(child.location, descriptor.location) + + # After setting up the descriptor, save any changes that we have + # made to attributes on the descriptor to the underlying KeyValueStore. + descriptor.save() return descriptor render_template = lambda: '' diff --git a/common/lib/xmodule/xmodule/tests/test_combined_open_ended.py b/common/lib/xmodule/xmodule/tests/test_combined_open_ended.py index e1f8d135dea..bed9cd86ba9 100644 --- a/common/lib/xmodule/xmodule/tests/test_combined_open_ended.py +++ b/common/lib/xmodule/xmodule/tests/test_combined_open_ended.py @@ -504,11 +504,13 @@ class OpenEndedModuleXmlTest(unittest.TestCase, DummyModulestore): See if we can load the module and save an answer @return: """ - #Load the module + # Load the module module = self.get_module_from_location(self.problem_location, COURSE) - #Try saving an answer + # Try saving an answer module.handle_ajax("save_answer", {"student_answer": self.answer}) + # Save our modifications to the underlying KeyValueStore so they can be persisted + module.save() task_one_json = json.loads(module.task_states[0]) self.assertEqual(task_one_json['child_history'][0]['answer'], self.answer) diff --git a/common/lib/xmodule/xmodule/tests/test_conditional.py b/common/lib/xmodule/xmodule/tests/test_conditional.py index abb8b4941a5..b28d2363693 100644 --- a/common/lib/xmodule/xmodule/tests/test_conditional.py +++ b/common/lib/xmodule/xmodule/tests/test_conditional.py @@ -217,8 +217,11 @@ class ConditionalModuleXmlTest(unittest.TestCase): html = ajax['html'] self.assertFalse(any(['This is a secret' in item for item in html])) - # now change state of the capa problem to make it completed - inner_get_module(Location('i4x://HarvardX/ER22x/problem/choiceprob')).attempts = 1 + # Now change state of the capa problem to make it completed + inner_module = inner_get_module(Location('i4x://HarvardX/ER22x/problem/choiceprob')) + inner_module.attempts = 1 + # Save our modifications to the underlying KeyValueStore so they can be persisted + inner_module.save() ajax = json.loads(module.handle_ajax('', '')) print "post-attempt ajax: ", ajax diff --git a/lms/djangoapps/courseware/model_data.py b/lms/djangoapps/courseware/model_data.py index dec4805ced8..44be16e4418 100644 --- a/lms/djangoapps/courseware/model_data.py +++ b/lms/djangoapps/courseware/model_data.py @@ -247,9 +247,10 @@ class ModelDataCache(object): course_id=self.course_id, student=self.user, module_state_key=key.block_scope_id.url(), - defaults={'state': json.dumps({}), - 'module_type': key.block_scope_id.category, - }, + defaults={ + 'state': json.dumps({}), + 'module_type': key.block_scope_id.category, + }, ) elif key.scope == Scope.content: field_object, _ = XModuleContentField.objects.get_or_create( @@ -333,22 +334,10 @@ class LmsKeyValueStore(KeyValueStore): return json.loads(field_object.value) def set(self, key, value): - if key.field_name in self._descriptor_model_data: - raise InvalidWriteError("Not allowed to overwrite descriptor model data", key.field_name) - - field_object = self._model_data_cache.find_or_create(key) - - if key.scope not in self._allowed_scopes: - raise InvalidScopeError(key.scope) - - if key.scope == Scope.user_state: - state = json.loads(field_object.state) - state[key.field_name] = value - field_object.state = json.dumps(state) - else: - field_object.value = json.dumps(value) - - field_object.save() + """ + Set a single value in the KeyValueStore + """ + self.set_many({key: value}) def set_many(self, kv_dict): """ @@ -362,23 +351,21 @@ class LmsKeyValueStore(KeyValueStore): # field_objects maps a field_object to a list of associated fields field_objects = dict() for field in kv_dict: - # check field for validity + # Check field for validity if field.field_name in self._descriptor_model_data: raise InvalidWriteError("Not allowed to overwrite descriptor model data", field.field_name) if field.scope not in self._allowed_scopes: raise InvalidScopeError(field.scope) - # if the field is valid + # If the field is valid and isn't already in the dictionary, add it. field_object = self._model_data_cache.find_or_create(field) - # if this field_object isn't already in the dictionary - # add it if field_object not in field_objects.keys(): field_objects[field_object] = [] - # update the list of associated fields + # Update the list of associated fields field_objects[field_object].append(field) - # special case when scope is for the user state + # Special case when scope is for the user state, because this scope saves fields in a single row if field.scope == Scope.user_state: state = json.loads(field_object.state) state[field.field_name] = kv_dict[field] diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index c343701a94d..de709f76525 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/lms/djangoapps/courseware/module_render.py @@ -27,7 +27,7 @@ from xmodule.modulestore import Location from xmodule.modulestore.django import modulestore from xmodule.modulestore.exceptions import ItemNotFoundError from xmodule.x_module import ModuleSystem -from xmodule_modifiers import replace_course_urls, replace_static_urls, add_histogram, wrap_xmodule +from xmodule_modifiers import replace_course_urls, replace_static_urls, add_histogram, wrap_xmodule, save_module # pylint: disable=F0401 import static_replace from psychometrics.psychoanalyze import make_psychometrics_data_update_handler @@ -36,6 +36,8 @@ from student.models import unique_id_for_user from courseware.access import has_access from courseware.masquerade import setup_masquerade from courseware.model_data import LmsKeyValueStore, LmsUsage, ModelDataCache +from xblock.runtime import KeyValueStore +from xblock.core import Scope from courseware.models import StudentModule from util.sandboxing import can_execute_unsafe_code from util.json_request import JsonResponse @@ -226,7 +228,7 @@ def get_module_for_descriptor_internal(user, descriptor, model_data_cache, cours userid=str(user.id), mod_id=descriptor.location.url(), dispatch=dispatch), - ) + ) return xqueue_callback_url_prefix + relative_xqueue_callback_url # Default queuename is course-specific and is derived from the course that @@ -234,11 +236,12 @@ def get_module_for_descriptor_internal(user, descriptor, model_data_cache, cours # TODO: Queuename should be derived from 'course_settings.json' of each course xqueue_default_queuename = descriptor.location.org + '-' + descriptor.location.course - xqueue = {'interface': xqueue_interface, - 'construct_callback': make_xqueue_callback, - 'default_queuename': xqueue_default_queuename.replace(' ', '_'), - 'waittime': settings.XQUEUE_WAITTIME_BETWEEN_REQUESTS - } + xqueue = { + 'interface': xqueue_interface, + 'construct_callback': make_xqueue_callback, + 'default_queuename': xqueue_default_queuename.replace(' ', '_'), + 'waittime': settings.XQUEUE_WAITTIME_BETWEEN_REQUESTS + } # This is a hacky way to pass settings to the combined open ended xmodule # It needs an S3 interface to upload images to S3 @@ -286,18 +289,24 @@ def get_module_for_descriptor_internal(user, descriptor, model_data_cache, cours ) def publish(event): + """A function that allows XModules to publish events. This only supports grade changes right now.""" if event.get('event_name') != 'grade': return - student_module, created = StudentModule.objects.get_or_create( - course_id=course_id, - student=user, - module_type=descriptor.location.category, - module_state_key=descriptor.location.url(), - defaults={'state': '{}'}, + usage = LmsUsage(descriptor.location, descriptor.location) + # Construct the key for the module + key = KeyValueStore.Key( + scope=Scope.user_state, + student_id=user.id, + block_scope_id=usage.id, + field_name='grade' ) + + student_module = model_data_cache.find_or_create(key) + # Update the grades student_module.grade = event.get('value') student_module.max_grade = event.get('max_value') + # Save all changes to the underlying KeyValueStore student_module.save() # Bin score into range and increment stats @@ -388,9 +397,31 @@ def get_module_for_descriptor_internal(user, descriptor, model_data_cache, cours if has_access(user, module, 'staff', course_id): module.get_html = add_histogram(module.get_html, module, user) + # force the module to save after rendering + module.get_html = save_module(module.get_html, module) return module +def find_target_student_module(request, user_id, course_id, mod_id): + """ + Retrieve target StudentModule + """ + user = User.objects.get(id=user_id) + model_data_cache = ModelDataCache.cache_for_descriptor_descendents( + course_id, + user, + modulestore().get_instance(course_id, mod_id), + depth=0, + select_for_update=True + ) + instance = get_module(user, request, mod_id, model_data_cache, course_id, grade_bucket_type='xqueue') + if instance is None: + msg = "No module {0} for user {1}--access denied?".format(mod_id, user) + log.debug(msg) + raise Http404 + return instance + + @csrf_exempt def xqueue_callback(request, course_id, userid, mod_id, dispatch): ''' @@ -409,20 +440,7 @@ def xqueue_callback(request, course_id, userid, mod_id, dispatch): if not isinstance(header, dict) or 'lms_key' not in header: raise Http404 - # Retrieve target StudentModule - user = User.objects.get(id=userid) - model_data_cache = ModelDataCache.cache_for_descriptor_descendents( - course_id, - user, - modulestore().get_instance(course_id, mod_id), - depth=0, - select_for_update=True - ) - instance = get_module(user, request, mod_id, model_data_cache, course_id, grade_bucket_type='xqueue') - if instance is None: - msg = "No module {0} for user {1}--access denied?".format(mod_id, user) - log.debug(msg) - raise Http404 + instance = find_target_student_module(request, userid, course_id, mod_id) # Transfer 'queuekey' from xqueue response header to the data. # This is required to use the interface defined by 'handle_ajax' @@ -433,6 +451,8 @@ def xqueue_callback(request, course_id, userid, mod_id, dispatch): try: # Can ignore the return value--not used for xqueue_callback instance.handle_ajax(dispatch, data) + # Save any state that has changed to the underlying KeyValueStore + instance.save() except: log.exception("error processing ajax call") raise @@ -504,6 +524,8 @@ def modx_dispatch(request, dispatch, location, course_id): # Let the module handle the AJAX try: ajax_return = instance.handle_ajax(dispatch, data) + # Save any fields that have changed to the underlying KeyValueStore + instance.save() # If we can't find the module, respond with a 404 except NotFoundError: diff --git a/lms/djangoapps/courseware/tests/test_model_data.py b/lms/djangoapps/courseware/tests/test_model_data.py index 07aaa664738..0368bb040be 100644 --- a/lms/djangoapps/courseware/tests/test_model_data.py +++ b/lms/djangoapps/courseware/tests/test_model_data.py @@ -1,3 +1,6 @@ +""" +Test for lms courseware app, module data (runtime data storage for XBlocks) +""" import json from mock import Mock, patch from functools import partial @@ -68,12 +71,17 @@ class TestDescriptorFallback(TestCase): self.assertRaises(InvalidWriteError, self.kvs.set, settings_key('field_b'), 'foo') self.assertEquals('settings', self.desc_md['field_b']) + self.assertRaises(InvalidWriteError, self.kvs.set_many, {content_key('field_a'): 'foo'}) + self.assertEquals('content', self.desc_md['field_a']) + self.assertRaises(InvalidWriteError, self.kvs.delete, content_key('field_a')) self.assertEquals('content', self.desc_md['field_a']) self.assertRaises(InvalidWriteError, self.kvs.delete, settings_key('field_b')) self.assertEquals('settings', self.desc_md['field_b']) + + class TestInvalidScopes(TestCase): def setUp(self): self.desc_md = {} @@ -85,10 +93,13 @@ class TestInvalidScopes(TestCase): for scope in (Scope(user=True, block=BlockScope.DEFINITION), Scope(user=False, block=BlockScope.TYPE), Scope(user=False, block=BlockScope.ALL)): - self.assertRaises(InvalidScopeError, self.kvs.get, LmsKeyValueStore.Key(scope, None, None, 'field')) - self.assertRaises(InvalidScopeError, self.kvs.set, LmsKeyValueStore.Key(scope, None, None, 'field'), 'value') - self.assertRaises(InvalidScopeError, self.kvs.delete, LmsKeyValueStore.Key(scope, None, None, 'field')) - self.assertRaises(InvalidScopeError, self.kvs.has, LmsKeyValueStore.Key(scope, None, None, 'field')) + key = LmsKeyValueStore.Key(scope, None, None, 'field') + + self.assertRaises(InvalidScopeError, self.kvs.get, key) + self.assertRaises(InvalidScopeError, self.kvs.set, key, 'value') + self.assertRaises(InvalidScopeError, self.kvs.delete, key) + self.assertRaises(InvalidScopeError, self.kvs.has, key) + self.assertRaises(InvalidScopeError, self.kvs.set_many, {key: 'value'}) class TestStudentModuleStorage(TestCase): @@ -141,7 +152,7 @@ class TestStudentModuleStorage(TestCase): self.assertFalse(self.kvs.has(user_state_key('not_a_field'))) def construct_kv_dict(self): - """ construct a kv_dict that can be passed to set_many """ + """Construct a kv_dict that can be passed to set_many""" key1 = user_state_key('field_a') key2 = user_state_key('field_b') new_value = 'new value' @@ -149,7 +160,7 @@ class TestStudentModuleStorage(TestCase): return {key1: new_value, key2: newer_value} def test_set_many(self): - """Test setting many fields that are scoped to Scope.user_state """ + "Test setting many fields that are scoped to Scope.user_state" kv_dict = self.construct_kv_dict() self.kvs.set_many(kv_dict) @@ -157,7 +168,7 @@ class TestStudentModuleStorage(TestCase): self.assertEquals(self.kvs.get(key), kv_dict[key]) def test_set_many_failure(self): - """Test failures when setting many fields that are scoped to Scope.user_state """ + "Test failures when setting many fields that are scoped to Scope.user_state" kv_dict = self.construct_kv_dict() # because we're patching the underlying save, we need to ensure the # fields are in the cache @@ -211,6 +222,10 @@ class StorageTestBase(object): A base class for that gets subclassed when testing each of the scopes. """ + # Disable pylint warnings that arise because of the way the child classes call + # this base class -- pylint's static analysis can't keep up with it. + # pylint: disable=E1101, E1102 + factory = None scope = None key_factory = None @@ -273,6 +288,7 @@ class StorageTestBase(object): self.assertFalse(self.kvs.has(self.key_factory('missing_field'))) def construct_kv_dict(self): + """Construct a kv_dict that can be passed to set_many""" key1 = self.key_factory('existing_field') key2 = self.key_factory('other_existing_field') new_value = 'new value' diff --git a/lms/djangoapps/courseware/tests/test_module_render.py b/lms/djangoapps/courseware/tests/test_module_render.py index a9060b57e97..732758be2c9 100644 --- a/lms/djangoapps/courseware/tests/test_module_render.py +++ b/lms/djangoapps/courseware/tests/test_module_render.py @@ -1,4 +1,7 @@ -from mock import MagicMock +""" +Test for lms courseware app, module render unit +""" +from mock import MagicMock, patch import json from django.http import Http404, HttpResponse @@ -28,6 +31,20 @@ class ModuleRenderTestCase(LoginEnrollmentTestCase): self.location = ['i4x', 'edX', 'toy', 'chapter', 'Overview'] self.course_id = 'edX/toy/2012_Fall' self.toy_course = modulestore().get_course(self.course_id) + self.mock_user = UserFactory() + self.mock_user.id = 1 + self.request_factory = RequestFactory() + + # Construct a mock module for the modulestore to return + self.mock_module = MagicMock() + self.mock_module.id = 1 + self.dispatch = 'score_update' + + # Construct a 'standard' xqueue_callback url + self.callback_url = reverse('xqueue_callback', kwargs=dict(course_id=self.course_id, + userid=str(self.mock_user.id), + mod_id=self.mock_module.id, + dispatch=self.dispatch)) def test_get_module(self): self.assertIsNone(render.get_module('dummyuser', None, @@ -56,7 +73,7 @@ class ModuleRenderTestCase(LoginEnrollmentTestCase): mock_request_3 = MagicMock() mock_request_3.POST.copy.return_value = {'position': 1} mock_request_3.FILES = False - mock_request_3.user = UserFactory() + mock_request_3.user = self.mock_user inputfile_2 = Stub() inputfile_2.size = 1 inputfile_2.name = 'name' @@ -87,6 +104,46 @@ class ModuleRenderTestCase(LoginEnrollmentTestCase): self.course_id ) + def test_xqueue_callback_success(self): + """ + Test for happy-path xqueue_callback + """ + fake_key = 'fake key' + xqueue_header = json.dumps({'lms_key': fake_key}) + data = { + 'xqueue_header': xqueue_header, + 'xqueue_body': 'hello world', + } + + # Patch getmodule to return our mock module + with patch('courseware.module_render.find_target_student_module') as get_fake_module: + get_fake_module.return_value = self.mock_module + # call xqueue_callback with our mocked information + request = self.request_factory.post(self.callback_url, data) + render.xqueue_callback(request, self.course_id, self.mock_user.id, self.mock_module.id, self.dispatch) + + # Verify that handle ajax is called with the correct data + request.POST['queuekey'] = fake_key + self.mock_module.handle_ajax.assert_called_once_with(self.dispatch, request.POST) + + def test_xqueue_callback_missing_header_info(self): + data = { + 'xqueue_header': '{}', + 'xqueue_body': 'hello world', + } + + with patch('courseware.module_render.find_target_student_module') as get_fake_module: + get_fake_module.return_value = self.mock_module + # Test with missing xqueue data + with self.assertRaises(Http404): + request = self.request_factory.post(self.callback_url, {}) + render.xqueue_callback(request, self.course_id, self.mock_user.id, self.mock_module.id, self.dispatch) + + # Test with missing xqueue_header + with self.assertRaises(Http404): + request = self.request_factory.post(self.callback_url, data) + render.xqueue_callback(request, self.course_id, self.mock_user.id, self.mock_module.id, self.dispatch) + def test_get_score_bucket(self): self.assertEquals(render.get_score_bucket(0, 10), 'incorrect') self.assertEquals(render.get_score_bucket(1, 10), 'partial') diff --git a/lms/djangoapps/courseware/views.py b/lms/djangoapps/courseware/views.py index 9c5a6657543..e6e062c4947 100644 --- a/lms/djangoapps/courseware/views.py +++ b/lms/djangoapps/courseware/views.py @@ -167,6 +167,8 @@ def save_child_position(seq_module, child_name): # Only save if position changed if position != seq_module.position: seq_module.position = position + # Save this new position to the underlying KeyValueStore + seq_module.save() def check_for_active_timelimit_module(request, course_id, course): diff --git a/requirements/edx/github.txt b/requirements/edx/github.txt index 62a44f13074..5d1a505ace7 100644 --- a/requirements/edx/github.txt +++ b/requirements/edx/github.txt @@ -8,6 +8,6 @@ -e git://github.com/eventbrite/zendesk.git@d53fe0e81b623f084e91776bcf6369f8b7b63879#egg=zendesk # Our libraries: --e git+https://github.com/edx/XBlock.git@0b71db6ee6f9b216d0dd85cd76b55f2354b93dff#egg=XBlock +-e git+https://github.com/edx/XBlock.git@3974e999fe853a37dfa6fadf0611289434349409#egg=XBlock -e git+https://github.com/edx/codejail.git@0a1b468#egg=codejail -e git+https://github.com/edx/diff-cover.git@v0.1.3#egg=diff_cover -- GitLab