diff --git a/cms/djangoapps/contentstore/module_info_model.py b/cms/djangoapps/contentstore/module_info_model.py index c0e1ff7207..b43a32f635 100644 --- a/cms/djangoapps/contentstore/module_info_model.py +++ b/cms/djangoapps/contentstore/module_info_model.py @@ -64,11 +64,11 @@ def set_module_info(store, location, post_data): if posted_metadata[metadata_key] is None: # remove both from passed in collection as well as the collection read in from the modulestore - if metadata_key in module._model_data: - del module._model_data[metadata_key] + if module._field_data.has(module, metadata_key): + module._field_data.delete(module, metadata_key) del posted_metadata[metadata_key] else: - module._model_data[metadata_key] = value + module._field_data.set(module, metadata_key, value) # commit to datastore # TODO (cpennington): This really shouldn't have to do this much reaching in to get the metadata diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 696b60fbe5..f76b563d6a 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -219,7 +219,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): 'course', '2012_Fall', None]), depth=None) html_module = draft_store.get_item(['i4x', 'edX', 'simple', 'html', 'test_html', None]) - self.assertEqual(html_module.lms.graceperiod, course.lms.graceperiod) + self.assertEqual(html_module.graceperiod, course.graceperiod) self.assertNotIn('graceperiod', own_metadata(html_module)) draft_store.convert_to_draft(html_module.location) @@ -227,7 +227,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): # refetch to check metadata html_module = draft_store.get_item(['i4x', 'edX', 'simple', 'html', 'test_html', None]) - self.assertEqual(html_module.lms.graceperiod, course.lms.graceperiod) + self.assertEqual(html_module.graceperiod, course.graceperiod) self.assertNotIn('graceperiod', own_metadata(html_module)) # publish module @@ -236,7 +236,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): # refetch to check metadata html_module = draft_store.get_item(['i4x', 'edX', 'simple', 'html', 'test_html', None]) - self.assertEqual(html_module.lms.graceperiod, course.lms.graceperiod) + self.assertEqual(html_module.graceperiod, course.graceperiod) self.assertNotIn('graceperiod', own_metadata(html_module)) # put back in draft and change metadata and see if it's now marked as 'own_metadata' @@ -246,12 +246,12 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): new_graceperiod = timedelta(hours=1) self.assertNotIn('graceperiod', own_metadata(html_module)) - html_module.lms.graceperiod = new_graceperiod + html_module.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) + self.assertEqual(html_module.graceperiod, new_graceperiod) draft_store.update_metadata(html_module.location, own_metadata(html_module)) @@ -259,7 +259,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): html_module = draft_store.get_item(['i4x', 'edX', 'simple', 'html', 'test_html', None]) self.assertIn('graceperiod', own_metadata(html_module)) - self.assertEqual(html_module.lms.graceperiod, new_graceperiod) + self.assertEqual(html_module.graceperiod, new_graceperiod) # republish draft_store.publish(html_module.location, 0) @@ -269,7 +269,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): html_module = draft_store.get_item(['i4x', 'edX', 'simple', 'html', 'test_html', None]) self.assertIn('graceperiod', own_metadata(html_module)) - self.assertEqual(html_module.lms.graceperiod, new_graceperiod) + self.assertEqual(html_module.graceperiod, new_graceperiod) def test_get_depth_with_drafts(self): import_from_xml(modulestore('direct'), 'common/test/data/', ['simple']) @@ -696,7 +696,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): # we want to assert equality between the objects, but we know the locations # differ, so just make them equal for testing purposes - source_item.location = new_loc + source_item.scope_ids = source_item.scope_ids._replace(def_id=new_loc, usage_id=new_loc) if hasattr(source_item, 'data') and hasattr(lookup_item, 'data'): self.assertEqual(source_item.data, lookup_item.data) @@ -877,7 +877,9 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): depth=1 ) # We had a bug where orphaned draft nodes caused export to fail. This is here to cover that case. - vertical.location = mongo.draft.as_draft(vertical.location.replace(name='no_references')) + draft_loc = mongo.draft.as_draft(vertical.location.replace(name='no_references')) + vertical.scope_ids = vertical.scope_ids._replace(def_id=draft_loc, usage_id=draft_loc) + draft_store.save_xmodule(vertical) orphan_vertical = draft_store.get_item(vertical.location) self.assertEqual(orphan_vertical.location.name, 'no_references') @@ -894,7 +896,8 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): root_dir = path(mkdtemp_clean()) # now create a new/different private (draft only) vertical - vertical.location = mongo.draft.as_draft(Location(['i4x', 'edX', 'toy', 'vertical', 'a_private_vertical', None])) + draft_loc = mongo.draft.as_draft(Location(['i4x', 'edX', 'toy', 'vertical', 'a_private_vertical', None])) + vertical.scope_ids = vertical.scope_ids._replace(def_id=draft_loc, usage_id=draft_loc) draft_store.save_xmodule(vertical) private_vertical = draft_store.get_item(vertical.location) vertical = None # blank out b/c i destructively manipulated its location 2 lines above @@ -965,7 +968,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): self.assertTrue(getattr(vertical, 'is_draft', False)) self.assertNotIn('index_in_children_list', child.xml_attributes) self.assertNotIn('parent_sequential_url', vertical.xml_attributes) - + for child in vertical.get_children(): self.assertTrue(getattr(child, 'is_draft', False)) self.assertNotIn('index_in_children_list', child.xml_attributes) @@ -1628,8 +1631,8 @@ class ContentStoreTest(ModuleStoreTestCase): # let's assert on the metadata_inheritance on an existing vertical for vertical in verticals: - self.assertEqual(course.lms.xqa_key, vertical.lms.xqa_key) - self.assertEqual(course.start, vertical.lms.start) + self.assertEqual(course.xqa_key, vertical.xqa_key) + self.assertEqual(course.start, vertical.start) self.assertGreater(len(verticals), 0) @@ -1645,16 +1648,16 @@ class ContentStoreTest(ModuleStoreTestCase): new_module = module_store.get_item(new_component_location) # check for grace period definition which should be defined at the course level - self.assertEqual(parent.lms.graceperiod, new_module.lms.graceperiod) - self.assertEqual(parent.lms.start, new_module.lms.start) - self.assertEqual(course.start, new_module.lms.start) + self.assertEqual(parent.graceperiod, new_module.graceperiod) + self.assertEqual(parent.start, new_module.start) + self.assertEqual(course.start, new_module.start) - self.assertEqual(course.lms.xqa_key, new_module.lms.xqa_key) + self.assertEqual(course.xqa_key, new_module.xqa_key) # # now let's define an override at the leaf node level # - new_module.lms.graceperiod = timedelta(1) + new_module.graceperiod = timedelta(1) new_module.save() module_store.update_metadata(new_module.location, own_metadata(new_module)) @@ -1662,7 +1665,7 @@ class ContentStoreTest(ModuleStoreTestCase): module_store.refresh_cached_metadata_inheritance_tree(new_component_location) new_module = module_store.get_item(new_component_location) - self.assertEqual(timedelta(1), new_module.lms.graceperiod) + self.assertEqual(timedelta(1), new_module.graceperiod) def test_default_metadata_inheritance(self): course = CourseFactory.create() @@ -1670,7 +1673,7 @@ class ContentStoreTest(ModuleStoreTestCase): course.children.append(vertical) # in memory self.assertIsNotNone(course.start) - self.assertEqual(course.start, vertical.lms.start) + self.assertEqual(course.start, vertical.start) self.assertEqual(course.textbooks, []) self.assertIn('GRADER', course.grading_policy) self.assertIn('GRADE_CUTOFFS', course.grading_policy) @@ -1682,7 +1685,7 @@ class ContentStoreTest(ModuleStoreTestCase): fetched_item = module_store.get_item(vertical.location) self.assertIsNotNone(fetched_course.start) self.assertEqual(course.start, fetched_course.start) - self.assertEqual(fetched_course.start, fetched_item.lms.start) + self.assertEqual(fetched_course.start, fetched_item.start) self.assertEqual(course.textbooks, fetched_course.textbooks) # is this test too strict? i.e., it requires the dicts to be == self.assertEqual(course.checklists, fetched_course.checklists) @@ -1755,12 +1758,10 @@ class MetadataSaveTestCase(ModuleStoreTestCase): 'track' } - fields = self.video_descriptor.fields location = self.video_descriptor.location - for field in fields: - if field.name in attrs_to_strip: - field.delete_from(self.video_descriptor) + for field_name in attrs_to_strip: + delattr(self.video_descriptor, field_name) self.assertNotIn('html5_sources', own_metadata(self.video_descriptor)) get_modulestore(location).update_metadata( diff --git a/cms/djangoapps/contentstore/tests/test_course_settings.py b/cms/djangoapps/contentstore/tests/test_course_settings.py index 286477228b..b27d1494e9 100644 --- a/cms/djangoapps/contentstore/tests/test_course_settings.py +++ b/cms/djangoapps/contentstore/tests/test_course_settings.py @@ -343,8 +343,8 @@ class CourseGradingTest(CourseTestCase): 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) + self.assertEqual(None, descriptor.format) + self.assertEqual(False, descriptor.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'}) @@ -352,8 +352,8 @@ class CourseGradingTest(CourseTestCase): 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) + self.assertEqual('Homework', descriptor.format) + self.assertEqual(True, descriptor.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'}) @@ -361,8 +361,8 @@ class CourseGradingTest(CourseTestCase): 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) + self.assertEqual(None, descriptor.format) + self.assertEqual(False, descriptor.graded) class CourseMetadataEditingTest(CourseTestCase): diff --git a/cms/djangoapps/contentstore/tests/test_crud.py b/cms/djangoapps/contentstore/tests/test_crud.py index e543b7b517..a16251fa0f 100644 --- a/cms/djangoapps/contentstore/tests/test_crud.py +++ b/cms/djangoapps/contentstore/tests/test_crud.py @@ -218,20 +218,16 @@ class TemplateTests(unittest.TestCase): ) usage_id = json_data.get('_id', None) if not '_inherited_settings' in json_data and parent_xblock is not None: - json_data['_inherited_settings'] = parent_xblock.xblock_kvs.get_inherited_settings().copy() + json_data['_inherited_settings'] = parent_xblock.xblock_kvs.inherited_settings.copy() json_fields = json_data.get('fields', {}) - for field in inheritance.INHERITABLE_METADATA: - if field in json_fields: - json_data['_inherited_settings'][field] = json_fields[field] + for field_name in inheritance.InheritanceMixin.fields: + if field_name in json_fields: + json_data['_inherited_settings'][field_name] = json_fields[field_name] new_block = system.xblock_from_json(class_, usage_id, json_data) if parent_xblock is not None: - children = parent_xblock.children - children.append(new_block) - # trigger setter method by using top level field access - parent_xblock.children = children - # decache pending children field settings (Note, truly persisting at this point would break b/c - # persistence assumes children is a list of ids not actual xblocks) + parent_xblock.children.append(new_block.scope_ids.usage_id) + # decache pending children field settings parent_xblock.save() return new_block diff --git a/cms/djangoapps/contentstore/tests/test_import_nostatic.py b/cms/djangoapps/contentstore/tests/test_import_nostatic.py index f0f65c9b07..275f9b6b1f 100644 --- a/cms/djangoapps/contentstore/tests/test_import_nostatic.py +++ b/cms/djangoapps/contentstore/tests/test_import_nostatic.py @@ -97,9 +97,9 @@ class ContentStoreImportNoStaticTest(ModuleStoreTestCase): self.assertIsNotNone(content) - # make sure course.lms.static_asset_path is correct - print "static_asset_path = {0}".format(course.lms.static_asset_path) - self.assertEqual(course.lms.static_asset_path, 'test_import_course') + # make sure course.static_asset_path is correct + print "static_asset_path = {0}".format(course.static_asset_path) + self.assertEqual(course.static_asset_path, 'test_import_course') def test_asset_import_nostatic(self): ''' diff --git a/cms/djangoapps/contentstore/tests/test_item.py b/cms/djangoapps/contentstore/tests/test_item.py index e5ff992cb8..dcd94838c8 100644 --- a/cms/djangoapps/contentstore/tests/test_item.py +++ b/cms/djangoapps/contentstore/tests/test_item.py @@ -1,4 +1,4 @@ -from contentstore.tests.test_course_settings import CourseTestCase +from contentstore.tests.utils import CourseTestCase from xmodule.modulestore.tests.factories import CourseFactory from django.core.urlresolvers import reverse from xmodule.capa_module import CapaDescriptor @@ -69,7 +69,7 @@ class TestCreateItem(CourseTestCase): # get the new item and check its category and display_name chap_location = self.response_id(resp) new_obj = modulestore().get_item(chap_location) - self.assertEqual(new_obj.category, 'chapter') + self.assertEqual(new_obj.scope_ids.block_type, 'chapter') self.assertEqual(new_obj.display_name, display_name) self.assertEqual(new_obj.location.org, self.course.location.org) self.assertEqual(new_obj.location.course, self.course.location.course) @@ -226,7 +226,7 @@ class TestEditItem(CourseTestCase): Test setting due & start dates on sequential """ sequential = modulestore().get_item(self.seq_location) - self.assertIsNone(sequential.lms.due) + self.assertIsNone(sequential.due) self.client.post( reverse('save_item'), json.dumps({ @@ -236,7 +236,7 @@ class TestEditItem(CourseTestCase): content_type="application/json" ) sequential = modulestore().get_item(self.seq_location) - self.assertEqual(sequential.lms.due, datetime.datetime(2010, 11, 22, 4, 0, tzinfo=UTC)) + self.assertEqual(sequential.due, datetime.datetime(2010, 11, 22, 4, 0, tzinfo=UTC)) self.client.post( reverse('save_item'), json.dumps({ @@ -246,5 +246,5 @@ class TestEditItem(CourseTestCase): content_type="application/json" ) sequential = modulestore().get_item(self.seq_location) - self.assertEqual(sequential.lms.due, datetime.datetime(2010, 11, 22, 4, 0, tzinfo=UTC)) - self.assertEqual(sequential.lms.start, datetime.datetime(2010, 9, 12, 14, 0, tzinfo=UTC)) + self.assertEqual(sequential.due, datetime.datetime(2010, 11, 22, 4, 0, tzinfo=UTC)) + self.assertEqual(sequential.start, datetime.datetime(2010, 9, 12, 14, 0, tzinfo=UTC)) diff --git a/cms/djangoapps/contentstore/views/component.py b/cms/djangoapps/contentstore/views/component.py index a5fec7c033..724dc439d9 100644 --- a/cms/djangoapps/contentstore/views/component.py +++ b/cms/djangoapps/contentstore/views/component.py @@ -2,27 +2,27 @@ import json import logging from collections import defaultdict -from django.http import ( HttpResponse, HttpResponseBadRequest, - HttpResponseForbidden ) +from django.http import (HttpResponse, HttpResponseBadRequest, + HttpResponseForbidden) from django.contrib.auth.decorators import login_required from django.views.decorators.http import require_http_methods from django.core.exceptions import PermissionDenied from django_future.csrf import ensure_csrf_cookie from django.conf import settings -from xmodule.modulestore.exceptions import ( ItemNotFoundError, - InvalidLocationError ) +from xmodule.modulestore.exceptions import (ItemNotFoundError, + InvalidLocationError) from mitxmako.shortcuts import render_to_response from xmodule.modulestore import Location from xmodule.modulestore.django import modulestore from xmodule.util.date_utils import get_default_time_display -from xblock.core import Scope +from xblock.fields import Scope from util.json_request import expect_json, JsonResponse from contentstore.module_info_model import get_module_info, set_module_info -from contentstore.utils import ( get_modulestore, get_lms_link_for_item, - compute_unit_state, UnitState, get_course_for_item ) +from contentstore.utils import (get_modulestore, get_lms_link_for_item, + compute_unit_state, UnitState, get_course_for_item) from models.settings.course_grading import CourseGradingModel @@ -30,6 +30,7 @@ from .requests import _xmodule_recurse from .access import has_access from xmodule.x_module import XModuleDescriptor from xblock.plugin import PluginMissingError +from xblock.runtime import Mixologist __all__ = ['OPEN_ENDED_COMPONENT_TYPES', 'ADVANCED_COMPONENT_POLICY_KEY', @@ -91,7 +92,7 @@ def edit_subsection(request, location): # we're for now assuming a single parent if len(parent_locs) != 1: logging.error( - 'Multiple (or none) parents have been found for %', + 'Multiple (or none) parents have been found for %s', location ) @@ -99,12 +100,14 @@ def edit_subsection(request, location): parent = modulestore().get_item(parent_locs[0]) # remove all metadata from the generic dictionary that is presented in a - # more normalized UI + # more normalized UI. We only want to display the XBlocks fields, not + # the fields from any mixins that have been added + fields = getattr(item, 'unmixed_class', item.__class__).fields policy_metadata = dict( (field.name, field.read_from(item)) for field - in item.fields + in fields.values() if field.name not in ['display_name', 'start', 'due', 'format'] and field.scope == Scope.settings ) @@ -135,6 +138,15 @@ def edit_subsection(request, location): ) +def load_mixed_class(category): + """ + Load an XBlock by category name, and apply all defined mixins + """ + component_class = XModuleDescriptor.load_class(category) + mixologist = Mixologist(settings.XBLOCK_MIXINS) + return mixologist.mix(component_class) + + @login_required def edit_unit(request, location): """ @@ -163,22 +175,29 @@ def edit_unit(request, location): component_templates = defaultdict(list) for category in COMPONENT_TYPES: - component_class = XModuleDescriptor.load_class(category) + component_class = load_mixed_class(category) # add the default template + # TODO: Once mixins are defined per-application, rather than per-runtime, + # this should use a cms mixed-in class. (cpennington) + if hasattr(component_class, 'display_name'): + display_name = component_class.display_name.default or 'Blank' + else: + display_name = 'Blank' component_templates[category].append(( - component_class.display_name.default or 'Blank', + display_name, category, False, # No defaults have markdown (hardcoded current default) None # no boilerplate for overrides )) # add boilerplates - for template in component_class.templates(): - component_templates[category].append(( - template['metadata'].get('display_name'), - category, - template['metadata'].get('markdown') is not None, - template.get('template_id') - )) + if hasattr(component_class, 'templates'): + for template in component_class.templates(): + component_templates[category].append(( + template['metadata'].get('display_name'), + category, + template['metadata'].get('markdown') is not None, + template.get('template_id') + )) # Check if there are any advanced modules specified in the course policy. # These modules should be specified as a list of strings, where the strings @@ -194,7 +213,7 @@ def edit_unit(request, location): # class? i.e., can an advanced have more than one entry in the # menu? one for default and others for prefilled boilerplates? try: - component_class = XModuleDescriptor.load_class(category) + component_class = load_mixed_class(category) component_templates['advanced'].append(( component_class.display_name.default or category, @@ -272,13 +291,17 @@ def edit_unit(request, location): 'draft_preview_link': preview_lms_link, 'published_preview_link': lms_link, 'subsection': containing_subsection, - 'release_date': get_default_time_display(containing_subsection.lms.start) - if containing_subsection.lms.start is not None else None, + 'release_date': ( + get_default_time_display(containing_subsection.start) + if containing_subsection.start is not None else None + ), 'section': containing_section, 'new_unit_category': 'vertical', 'unit_state': unit_state, - 'published_date': get_default_time_display(item.cms.published_date) - if item.cms.published_date is not None else None + 'published_date': ( + get_default_time_display(item.published_date) + if item.published_date is not None else None + ), }) diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py index bbd95dba84..84a199d71d 100644 --- a/cms/djangoapps/contentstore/views/item.py +++ b/cms/djangoapps/contentstore/views/item.py @@ -58,13 +58,13 @@ def save_item(request): # 'apply' the submitted metadata, so we don't end up deleting system metadata existing_item = modulestore().get_item(item_location) for metadata_key in request.POST.get('nullout', []): - _get_xblock_field(existing_item, metadata_key).write_to(existing_item, None) + setattr(existing_item, metadata_key, None) # update existing metadata with submitted metadata (which can be partial) # IMPORTANT NOTE: if the client passed 'null' (None) for a piece of metadata that means 'remove it'. If # the intent is to make it None, use the nullout field for metadata_key, value in request.POST.get('metadata', {}).items(): - field = _get_xblock_field(existing_item, metadata_key) + field = existing_item.fields[metadata_key] if value is None: field.delete_from(existing_item) @@ -80,16 +80,6 @@ def save_item(request): return JsonResponse() -def _get_xblock_field(xblock, field_name): - """ - A temporary function to get the xblock field either from the xblock or one of its namespaces by name. - :param xblock: - :param field_name: - """ - for field in xblock.iterfields(): - if field.name == field_name: - return field - @login_required @expect_json def create_item(request): diff --git a/cms/djangoapps/contentstore/views/preview.py b/cms/djangoapps/contentstore/views/preview.py index 7a3a224d86..ccbb7fb5bb 100644 --- a/cms/djangoapps/contentstore/views/preview.py +++ b/cms/djangoapps/contentstore/views/preview.py @@ -2,6 +2,7 @@ import logging import sys from functools import partial +from django.conf import settings from django.http import HttpResponse, Http404, HttpResponseBadRequest, HttpResponseForbidden from django.core.urlresolvers import reverse from django.contrib.auth.decorators import login_required @@ -11,12 +12,12 @@ from xmodule_modifiers import replace_static_urls, wrap_xmodule, save_module # from xmodule.error_module import ErrorDescriptor from xmodule.errortracker import exc_info_to_str from xmodule.exceptions import NotFoundError, ProcessingError -from xmodule.modulestore import Location from xmodule.modulestore.django import modulestore -from xmodule.modulestore.mongo import MongoUsage from xmodule.x_module import ModuleSystem from xblock.runtime import DbModel +from lms.xblock.field_data import lms_field_data + from util.sandboxing import can_execute_unsafe_code import static_replace @@ -97,14 +98,10 @@ def preview_module_system(request, preview_id, descriptor): descriptor: An XModuleDescriptor """ - def preview_model_data(descriptor): + def preview_field_data(descriptor): "Helper method to create a DbModel from a descriptor" - return DbModel( - SessionKeyValueStore(request, descriptor._model_data), - descriptor.module_class, - preview_id, - MongoUsage(preview_id, descriptor.location.url()), - ) + student_data = DbModel(SessionKeyValueStore(request)) + return lms_field_data(descriptor._field_data, student_data) course_id = get_course_for_item(descriptor.location).location.course_id @@ -118,8 +115,9 @@ def preview_module_system(request, preview_id, descriptor): debug=True, replace_urls=partial(static_replace.replace_static_urls, data_directory=None, course_id=course_id), user=request.user, - xblock_model_data=preview_model_data, + xblock_field_data=preview_field_data, can_execute_unsafe_code=(lambda: can_execute_unsafe_code(course_id)), + mixins=settings.XBLOCK_MIXINS, ) diff --git a/cms/djangoapps/contentstore/views/session_kv_store.py b/cms/djangoapps/contentstore/views/session_kv_store.py index 87a92a9e2e..ddee2c1dbc 100644 --- a/cms/djangoapps/contentstore/views/session_kv_store.py +++ b/cms/djangoapps/contentstore/views/session_kv_store.py @@ -1,28 +1,21 @@ -from xblock.runtime import KeyValueStore, InvalidScopeError +""" +An :class:`~xblock.runtime.KeyValueStore` that stores data in the django session +""" +from xblock.runtime import KeyValueStore class SessionKeyValueStore(KeyValueStore): - def __init__(self, request, descriptor_model_data): - self._descriptor_model_data = descriptor_model_data + def __init__(self, request): self._session = request.session def get(self, key): - try: - return self._descriptor_model_data[key.field_name] - except (KeyError, InvalidScopeError): - return self._session[tuple(key)] + return self._session[tuple(key)] def set(self, key, value): - try: - self._descriptor_model_data[key.field_name] = value - except (KeyError, InvalidScopeError): - self._session[tuple(key)] = value + self._session[tuple(key)] = value def delete(self, key): - try: - del self._descriptor_model_data[key.field_name] - except (KeyError, InvalidScopeError): - del self._session[tuple(key)] + del self._session[tuple(key)] def has(self, key): - return key.field_name in self._descriptor_model_data or tuple(key) in self._session + return tuple(key) in self._session diff --git a/cms/djangoapps/models/settings/course_grading.py b/cms/djangoapps/models/settings/course_grading.py index 0746fc7a90..578961fad6 100644 --- a/cms/djangoapps/models/settings/course_grading.py +++ b/cms/djangoapps/models/settings/course_grading.py @@ -125,7 +125,7 @@ class CourseGradingModel(object): # 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) + get_modulestore(course_location).update_item(course_location, descriptor._field_data._kvs._data) return CourseGradingModel.jsonize_grader(index, descriptor.raw_grader[index]) @@ -144,7 +144,7 @@ class CourseGradingModel(object): # 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) + get_modulestore(course_location).update_item(course_location, descriptor._field_data._kvs._data) return cutoffs @@ -168,12 +168,12 @@ class CourseGradingModel(object): grace_timedelta = timedelta(**graceperiodjson) descriptor = get_modulestore(course_location).get_item(course_location) - descriptor.lms.graceperiod = grace_timedelta + descriptor.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) + get_modulestore(course_location).update_metadata(course_location, descriptor._field_data._kvs._metadata) @staticmethod def delete_grader(course_location, index): @@ -193,7 +193,7 @@ class CourseGradingModel(object): # 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) + get_modulestore(course_location).update_item(course_location, descriptor._field_data._kvs._data) @staticmethod def delete_grace_period(course_location): @@ -204,12 +204,12 @@ class CourseGradingModel(object): course_location = Location(course_location) descriptor = get_modulestore(course_location).get_item(course_location) - del descriptor.lms.graceperiod + del descriptor.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) + get_modulestore(course_location).update_metadata(course_location, descriptor._field_data._kvs._metadata) @staticmethod def get_section_grader_type(location): @@ -217,7 +217,7 @@ class CourseGradingModel(object): location = Location(location) descriptor = get_modulestore(location).get_item(location) - return {"graderType": descriptor.lms.format if descriptor.lms.format is not None else 'Not Graded', + return {"graderType": descriptor.format if descriptor.format is not None else 'Not Graded', "location": location, "id": 99 # just an arbitrary value to } @@ -229,21 +229,21 @@ class CourseGradingModel(object): descriptor = get_modulestore(location).get_item(location) if 'graderType' in jsondict and jsondict['graderType'] != u"Not Graded": - descriptor.lms.format = jsondict.get('graderType') - descriptor.lms.graded = True + descriptor.format = jsondict.get('graderType') + descriptor.graded = True else: - del descriptor.lms.format - del descriptor.lms.graded + del descriptor.format + del descriptor.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) + get_modulestore(location).update_metadata(location, descriptor._field_data._kvs._metadata) @staticmethod def convert_set_grace_period(descriptor): # 5 hours 59 minutes 59 seconds => converted to iso format - rawgrace = descriptor.lms.graceperiod + rawgrace = descriptor.graceperiod if rawgrace: hours_from_days = rawgrace.days * 24 seconds = rawgrace.seconds diff --git a/cms/djangoapps/models/settings/course_metadata.py b/cms/djangoapps/models/settings/course_metadata.py index adbbf06c64..f186fbf116 100644 --- a/cms/djangoapps/models/settings/course_metadata.py +++ b/cms/djangoapps/models/settings/course_metadata.py @@ -1,8 +1,9 @@ from xmodule.modulestore import Location from contentstore.utils import get_modulestore from xmodule.modulestore.inheritance import own_metadata -from xblock.core import Scope +from xblock.fields import Scope from xmodule.course_module import CourseDescriptor +from cms.xmodule_namespace import CmsBlockMixin class CourseMetadata(object): @@ -34,12 +35,17 @@ class CourseMetadata(object): descriptor = get_modulestore(course_location).get_item(course_location) - for field in descriptor.fields + descriptor.lms.fields: + for field in descriptor.fields.values(): + if field.name in CmsBlockMixin.fields: + continue + if field.scope != Scope.settings: continue - if field.name not in cls.FILTERED_LIST: - course[field.name] = field.read_json(descriptor) + if field.name in cls.FILTERED_LIST: + continue + + course[field.name] = field.read_json(descriptor) return course @@ -67,12 +73,8 @@ class CourseMetadata(object): if hasattr(descriptor, key) and getattr(descriptor, key) != val: dirty = True - value = getattr(CourseDescriptor, key).from_json(val) + value = descriptor.fields[key].from_json(val) setattr(descriptor, key, value) - elif hasattr(descriptor.lms, key) and getattr(descriptor.lms, key) != key: - dirty = True - value = getattr(CourseDescriptor.lms, key).from_json(val) - setattr(descriptor.lms, key, value) if dirty: # Save the data that we've just changed to the underlying @@ -96,8 +98,6 @@ class CourseMetadata(object): for key in payload['deleteKeys']: if hasattr(descriptor, key): delattr(descriptor, key) - 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. diff --git a/cms/envs/common.py b/cms/envs/common.py index d0650e9ed2..ecb85a05e0 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -28,6 +28,10 @@ import lms.envs.common from lms.envs.common import USE_TZ, TECH_SUPPORT_EMAIL, PLATFORM_NAME, BUGS_EMAIL from path import path +from lms.xblock.mixin import LmsBlockMixin +from cms.xmodule_namespace import CmsBlockMixin +from xmodule.modulestore.inheritance import InheritanceMixin + ############################ FEATURE CONFIGURATION ############################# MITX_FEATURES = { @@ -160,6 +164,13 @@ MIDDLEWARE_CLASSES = ( 'ratelimitbackend.middleware.RateLimitMiddleware', ) +############# XBlock Configuration ########## + +# This should be moved into an XBlock Runtime/Application object +# once the responsibility of XBlock creation is moved out of modulestore - cpennington +XBLOCK_MIXINS = (LmsBlockMixin, CmsBlockMixin, InheritanceMixin) + + ############################ SIGNAL HANDLERS ################################ # This is imported to register the exception signal handling that logs exceptions import monitoring.exceptions # noqa diff --git a/cms/startup.py b/cms/startup.py index eb1098a707..111cb9f96a 100644 --- a/cms/startup.py +++ b/cms/startup.py @@ -1,6 +1,7 @@ """ Module with code executed during Studio startup """ +import logging from django.conf import settings # Force settings to run so that the python path is modified @@ -8,6 +9,8 @@ settings.INSTALLED_APPS # pylint: disable=W0104 from django_startup import autostartup +log = logging.getLogger(__name__) + # TODO: Remove this code once Studio/CMS runs via wsgi in all environments INITIALIZED = False @@ -22,4 +25,3 @@ def run(): INITIALIZED = True autostartup() - diff --git a/cms/static/coffee/spec/views/metadata_edit_spec.coffee b/cms/static/coffee/spec/views/metadata_edit_spec.coffee index b3e4567d82..3d2e8c244b 100644 --- a/cms/static/coffee/spec/views/metadata_edit_spec.coffee +++ b/cms/static/coffee/spec/views/metadata_edit_spec.coffee @@ -26,7 +26,6 @@ describe "Test Metadata Editor", -> explicitly_set: true, field_name: "display_name", help: "Specifies the name for this component.", - inheritable: false, options: [], type: CMS.Models.Metadata.GENERIC_TYPE, value: "Word cloud" @@ -38,7 +37,6 @@ describe "Test Metadata Editor", -> explicitly_set: false, field_name: "show_answer", help: "When should you show the answer", - inheritable: true, options: [ {"display_name": "Always", "value": "always"}, {"display_name": "Answered", "value": "answered"}, @@ -54,7 +52,6 @@ describe "Test Metadata Editor", -> explicitly_set: false, field_name: "num_inputs", help: "Number of text boxes for student to input words/sentences.", - inheritable: false, options: {min: 1}, type: CMS.Models.Metadata.INTEGER_TYPE, value: 5 @@ -66,7 +63,6 @@ describe "Test Metadata Editor", -> explicitly_set: true, field_name: "weight", help: "Weight for this problem", - inheritable: true, options: {min: 1.3, max:100.2, step:0.1}, type: CMS.Models.Metadata.FLOAT_TYPE, value: 10.2 @@ -78,7 +74,6 @@ describe "Test Metadata Editor", -> explicitly_set: false, field_name: "list", help: "A list of things.", - inheritable: false, options: [], type: CMS.Models.Metadata.LIST_TYPE, value: ["the first display value", "the second"] @@ -99,7 +94,6 @@ describe "Test Metadata Editor", -> explicitly_set: true, field_name: "unknown_type", help: "Mystery property.", - inheritable: false, options: [ {"display_name": "Always", "value": "always"}, {"display_name": "Answered", "value": "answered"}, @@ -145,7 +139,6 @@ describe "Test Metadata Editor", -> explicitly_set: false, field_name: "display_name", help: "", - inheritable: false, options: [], type: CMS.Models.Metadata.GENERIC_TYPE, value: null diff --git a/cms/templates/edit_subsection.html b/cms/templates/edit_subsection.html index 5b03643f3b..b1be834709 100644 --- a/cms/templates/edit_subsection.html +++ b/cms/templates/edit_subsection.html @@ -37,21 +37,21 @@
- % if subsection.lms.start and not almost_same_datetime(subsection.lms.start, parent_item.lms.start): - % if parent_item.lms.start is None: + % if subsection.start and not almost_same_datetime(subsection.start, parent_item.start): + % if parent_item.start is None:

${_("The date above differs from the release date of {name}, which is unset.").format(name=parent_item.display_name_with_default)} % else: -

${_("The date above differs from the release date of {name} - {start_time}").format(name=parent_item.display_name_with_default, start_time=get_default_time_display(parent_item.lms.start))}. +

${_("The date above differs from the release date of {name} - {start_time}").format(name=parent_item.display_name_with_default, start_time=get_default_time_display(parent_item.start))}. % endif ${_("Sync to {name}.").format(name=parent_item.display_name_with_default)}

% endif @@ -60,7 +60,7 @@
-
+
@@ -69,13 +69,13 @@
${_("Remove due date")} diff --git a/cms/templates/overview.html b/cms/templates/overview.html index 2c42df187a..a8cdfff554 100644 --- a/cms/templates/overview.html +++ b/cms/templates/overview.html @@ -157,19 +157,19 @@

-
+
diff --git a/cms/templates/widgets/sequence-edit.html b/cms/templates/widgets/sequence-edit.html index 7956fbfbaf..c729f28950 100644 --- a/cms/templates/widgets/sequence-edit.html +++ b/cms/templates/widgets/sequence-edit.html @@ -35,7 +35,7 @@
    1. % for child in module.get_children(): -
    2. +
    3. - + ${unit.display_name_with_default} % if actions: diff --git a/cms/xmodule_namespace.py b/cms/xmodule_namespace.py index eef4b41f37..c028ab3710 100644 --- a/cms/xmodule_namespace.py +++ b/cms/xmodule_namespace.py @@ -4,12 +4,12 @@ Namespace defining common fields used by Studio for all blocks import datetime -from xblock.core import Namespace, Scope, ModelType, String +from xblock.fields import Scope, Field, Integer, XBlockMixin -class DateTuple(ModelType): +class DateTuple(Field): """ - ModelType that stores datetime objects as time tuples + Field that stores datetime objects as time tuples """ def from_json(self, value): return datetime.datetime(*value[0:6]) @@ -21,9 +21,9 @@ class DateTuple(ModelType): return list(value.timetuple()) -class CmsNamespace(Namespace): +class CmsBlockMixin(XBlockMixin): """ - Namespace with fields common to all blocks in Studio + Mixin with fields common to all blocks in Studio """ published_date = DateTuple(help="Date when the module was published", scope=Scope.settings) - published_by = String(help="Id of the user who published this module", scope=Scope.settings) + published_by = Integer(help="Id of the user who published this module", scope=Scope.settings) diff --git a/common/djangoapps/external_auth/views.py b/common/djangoapps/external_auth/views.py index 6d264293f1..e707a95518 100644 --- a/common/djangoapps/external_auth/views.py +++ b/common/djangoapps/external_auth/views.py @@ -47,7 +47,7 @@ from ratelimitbackend.exceptions import RateLimitException import student.views as student_views # Required for Pearson from courseware.views import get_module_for_descriptor, jump_to -from courseware.model_data import ModelDataCache +from courseware.model_data import FieldDataCache from xmodule.modulestore.django import modulestore from xmodule.course_module import CourseDescriptor from xmodule.modulestore import Location @@ -944,7 +944,7 @@ def test_center_login(request): log.error("cand {} on exam {} for course {}: descriptor not found for location {}".format(client_candidate_id, exam_series_code, course_id, location)) return HttpResponseRedirect(makeErrorURL(error_url, "missingClientProgram")) - timelimit_module_cache = ModelDataCache.cache_for_descriptor_descendents(course_id, testcenteruser.user, + timelimit_module_cache = FieldDataCache.cache_for_descriptor_descendents(course_id, testcenteruser.user, timelimit_descriptor, depth=None) timelimit_module = get_module_for_descriptor(request.user, request, timelimit_descriptor, timelimit_module_cache, course_id, position=None) diff --git a/common/djangoapps/tests.py b/common/djangoapps/tests.py index 4a61a106d4..17e9c07f6a 100644 --- a/common/djangoapps/tests.py +++ b/common/djangoapps/tests.py @@ -32,7 +32,7 @@ class TestXmoduleModfiers(ModuleStoreTestCase): late_problem = ItemFactory.create( parent_location=section.location, display_name='problem hist 2', category='problem') - late_problem.lms.start = datetime.datetime.now(UTC) + datetime.timedelta(days=32) + late_problem.start = datetime.datetime.now(UTC) + datetime.timedelta(days=32) late_problem.has_score = False diff --git a/common/djangoapps/xmodule_modifiers.py b/common/djangoapps/xmodule_modifiers.py index 16fe12371b..14f80c0a70 100644 --- a/common/djangoapps/xmodule_modifiers.py +++ b/common/djangoapps/xmodule_modifiers.py @@ -29,12 +29,16 @@ def wrap_xmodule(get_html, module, template, context=None): if context is None: context = {} + # If XBlock generated this class, then use the first baseclass + # as the name (since that's the original, unmixed class) + class_name = getattr(module, 'unmixed_class', module.__class__).__name__ + @wraps(get_html) def _get_html(): context.update({ 'content': get_html(), 'display_name': module.display_name, - 'class_': module.__class__.__name__, + 'class_': class_name, 'module_name': module.js_module_name }) @@ -157,7 +161,7 @@ def add_histogram(get_html, module, user): # doesn't like symlinks) filepath = filename data_dir = osfs.root_path.rsplit('/')[-1] - giturl = module.lms.giturl or 'https://github.com/MITx' + giturl = module.giturl or 'https://github.com/MITx' edit_link = "%s/%s/tree/master/%s" % (giturl, data_dir, filepath) else: edit_link = False @@ -165,22 +169,21 @@ def add_histogram(get_html, module, user): giturl = "" data_dir = "" - source_file = module.lms.source_file # source used to generate the problem XML, eg latex or word + source_file = module.source_file # source used to generate the problem XML, eg latex or word # useful to indicate to staff if problem has been released or not # TODO (ichuang): use _has_access_descriptor.can_load in lms.courseware.access, instead of now>mstart comparison here now = datetime.datetime.now(UTC()) is_released = "unknown" - mstart = module.descriptor.lms.start + mstart = module.descriptor.start if mstart is not None: is_released = "Yes!" if (now > mstart) else "Not yet" - staff_context = {'fields': [(field.name, getattr(module, field.name)) for field in module.fields], - 'lms_fields': [(field.name, getattr(module.lms, field.name)) for field in module.lms.fields], - 'xml_attributes' : getattr(module.descriptor, 'xml_attributes', {}), + staff_context = {'fields': [(name, field.read_from(module)) for name, field in module.fields.items()], + 'xml_attributes': getattr(module.descriptor, 'xml_attributes', {}), 'location': module.location, - 'xqa_key': module.lms.xqa_key, + 'xqa_key': module.xqa_key, 'source_file': source_file, 'source_url': '%s/%s/tree/master/%s' % (giturl, data_dir, source_file), 'category': str(module.__class__.__name__), diff --git a/common/lib/xmodule/xmodule/abtest_module.py b/common/lib/xmodule/xmodule/abtest_module.py index 53f080eb3a..c0a53d048f 100644 --- a/common/lib/xmodule/xmodule/abtest_module.py +++ b/common/lib/xmodule/xmodule/abtest_module.py @@ -6,7 +6,7 @@ from xmodule.x_module import XModule from xmodule.raw_module import RawDescriptor from xmodule.xml_module import XmlDescriptor from xmodule.exceptions import InvalidDefinitionError -from xblock.core import String, Scope, Dict +from xblock.fields import String, Scope, Dict DEFAULT = "_DEFAULT_GROUP" diff --git a/common/lib/xmodule/xmodule/annotatable_module.py b/common/lib/xmodule/xmodule/annotatable_module.py index f80e3e488e..ca85065577 100644 --- a/common/lib/xmodule/xmodule/annotatable_module.py +++ b/common/lib/xmodule/xmodule/annotatable_module.py @@ -5,7 +5,7 @@ from pkg_resources import resource_string from xmodule.x_module import XModule from xmodule.raw_module import RawDescriptor -from xblock.core import Scope, String +from xblock.fields import Scope, String import textwrap log = logging.getLogger(__name__) diff --git a/common/lib/xmodule/xmodule/capa_module.py b/common/lib/xmodule/xmodule/capa_module.py index dbd535a471..8de38022b7 100644 --- a/common/lib/xmodule/xmodule/capa_module.py +++ b/common/lib/xmodule/xmodule/capa_module.py @@ -18,7 +18,7 @@ from .progress import Progress from xmodule.x_module import XModule from xmodule.raw_module import RawDescriptor from xmodule.exceptions import NotFoundError, ProcessingError -from xblock.core import Scope, String, Boolean, Dict, Integer, Float +from xblock.fields import Scope, String, Boolean, Dict, Integer, Float from .fields import Timedelta, Date from django.utils.timezone import UTC diff --git a/common/lib/xmodule/xmodule/combined_open_ended_module.py b/common/lib/xmodule/xmodule/combined_open_ended_module.py index 5354915b86..c6dc882bb6 100644 --- a/common/lib/xmodule/xmodule/combined_open_ended_module.py +++ b/common/lib/xmodule/xmodule/combined_open_ended_module.py @@ -5,7 +5,7 @@ from pkg_resources import resource_string from xmodule.raw_module import RawDescriptor from .x_module import XModule -from xblock.core import Integer, Scope, String, List, Float, Boolean +from xblock.fields import Integer, Scope, String, List, Float, Boolean from xmodule.open_ended_grading_classes.combined_open_ended_modulev1 import CombinedOpenEndedV1Module, CombinedOpenEndedV1Descriptor from collections import namedtuple from .fields import Date, Timedelta diff --git a/common/lib/xmodule/xmodule/conditional_module.py b/common/lib/xmodule/xmodule/conditional_module.py index 5bdc8e7797..82416d48d1 100644 --- a/common/lib/xmodule/xmodule/conditional_module.py +++ b/common/lib/xmodule/xmodule/conditional_module.py @@ -10,7 +10,7 @@ from pkg_resources import resource_string from xmodule.x_module import XModule from xmodule.modulestore import Location from xmodule.seq_module import SequenceDescriptor -from xblock.core import Scope, List +from xblock.fields import Scope, List from xmodule.modulestore.exceptions import ItemNotFoundError diff --git a/common/lib/xmodule/xmodule/course_module.py b/common/lib/xmodule/xmodule/course_module.py index c583aec3d1..aca804d5e2 100644 --- a/common/lib/xmodule/xmodule/course_module.py +++ b/common/lib/xmodule/xmodule/course_module.py @@ -13,7 +13,7 @@ from xmodule.util.decorators import lazyproperty from xmodule.graders import grader_from_conf import json -from xblock.core import Scope, List, String, Dict, Boolean +from xblock.fields import Scope, List, String, Dict, Boolean from .fields import Date from xmodule.modulestore.locator import CourseLocator from django.utils.timezone import UTC @@ -118,6 +118,13 @@ class Textbook(object): return table_of_contents + def __eq__(self, other): + return (self.title == other.title and + self.book_url == other.book_url) + + def __ne__(self, other): + return not self == other + class TextbookList(List): def from_json(self, values): @@ -737,7 +744,7 @@ class CourseDescriptor(CourseFields, SequenceDescriptor): all_descriptors - This contains a list of all xmodules that can effect grading a student. This is used to efficiently fetch - all the xmodule state for a ModelDataCache without walking + all the xmodule state for a FieldDataCache without walking the descriptor tree again. @@ -754,14 +761,14 @@ class CourseDescriptor(CourseFields, SequenceDescriptor): for c in self.get_children(): for s in c.get_children(): - if s.lms.graded: + if s.graded: xmoduledescriptors = list(yield_descriptor_descendents(s)) xmoduledescriptors.append(s) # The xmoduledescriptors included here are only the ones that have scores. section_description = {'section_descriptor': s, 'xmoduledescriptors': filter(lambda child: child.has_score, xmoduledescriptors)} - section_format = s.lms.format if s.lms.format is not None else '' + section_format = s.format if s.format is not None else '' graded_sections[section_format] = graded_sections.get(section_format, []) + [section_description] all_descriptors.extend(xmoduledescriptors) diff --git a/common/lib/xmodule/xmodule/crowdsource_hinter.py b/common/lib/xmodule/xmodule/crowdsource_hinter.py index 7e538efa24..fcc74d5cb9 100644 --- a/common/lib/xmodule/xmodule/crowdsource_hinter.py +++ b/common/lib/xmodule/xmodule/crowdsource_hinter.py @@ -15,7 +15,7 @@ from lxml import etree from xmodule.x_module import XModule from xmodule.raw_module import RawDescriptor -from xblock.core import Scope, String, Integer, Boolean, Dict, List +from xblock.fields import Scope, String, Integer, Boolean, Dict, List from capa.responsetypes import FormulaResponse diff --git a/common/lib/xmodule/xmodule/discussion_module.py b/common/lib/xmodule/xmodule/discussion_module.py index 2c6cc9f4dd..8051cb23a3 100644 --- a/common/lib/xmodule/xmodule/discussion_module.py +++ b/common/lib/xmodule/xmodule/discussion_module.py @@ -3,7 +3,7 @@ from pkg_resources import resource_string from xmodule.x_module import XModule from xmodule.raw_module import RawDescriptor from xmodule.editing_module import MetadataOnlyEditingDescriptor -from xblock.core import String, Scope +from xblock.fields import String, Scope from uuid import uuid4 diff --git a/common/lib/xmodule/xmodule/editing_module.py b/common/lib/xmodule/xmodule/editing_module.py index eed8fb310f..35e3e4c6d1 100644 --- a/common/lib/xmodule/xmodule/editing_module.py +++ b/common/lib/xmodule/xmodule/editing_module.py @@ -2,7 +2,7 @@ from pkg_resources import resource_string from xmodule.mako_module import MakoModuleDescriptor -from xblock.core import Scope, String +from xblock.fields import Scope, String import logging log = logging.getLogger(__name__) diff --git a/common/lib/xmodule/xmodule/error_module.py b/common/lib/xmodule/xmodule/error_module.py index 027067f4c0..2013ee2003 100644 --- a/common/lib/xmodule/xmodule/error_module.py +++ b/common/lib/xmodule/xmodule/error_module.py @@ -12,7 +12,8 @@ from lxml import etree from xmodule.x_module import XModule, XModuleDescriptor from xmodule.errortracker import exc_info_to_str from xmodule.modulestore import Location -from xblock.core import String, Scope +from xblock.fields import String, Scope, ScopeIds +from xblock.field_data import DictFieldData log = logging.getLogger(__name__) @@ -95,16 +96,19 @@ class ErrorDescriptor(ErrorFields, XModuleDescriptor): ) # real metadata stays in the content, but add a display name - model_data = { + field_data = DictFieldData({ 'error_msg': str(error_msg), 'contents': contents, 'display_name': 'Error: ' + location.url(), 'location': location, 'category': 'error' - } - return cls( - system, - model_data, + }) + return system.construct_xblock_from_class( + cls, + field_data, + # The error module doesn't use scoped data, and thus doesn't need + # real scope keys + ScopeIds('error', None, location, location) ) def get_context(self): diff --git a/common/lib/xmodule/xmodule/fields.py b/common/lib/xmodule/xmodule/fields.py index b7094203c4..a0b53859ce 100644 --- a/common/lib/xmodule/xmodule/fields.py +++ b/common/lib/xmodule/xmodule/fields.py @@ -2,7 +2,7 @@ import time import logging import re -from xblock.core import ModelType +from xblock.fields import Field import datetime import dateutil.parser @@ -11,7 +11,7 @@ from pytz import UTC log = logging.getLogger(__name__) -class Date(ModelType): +class Date(Field): ''' Date fields know how to parse and produce json (iso) compatible formats. Converts to tz aware datetimes. ''' @@ -20,6 +20,8 @@ class Date(ModelType): PREVENT_DEFAULT_DAY_MON_SEED1 = datetime.datetime(CURRENT_YEAR, 1, 1, tzinfo=UTC) PREVENT_DEFAULT_DAY_MON_SEED2 = datetime.datetime(CURRENT_YEAR, 2, 2, tzinfo=UTC) + MUTABLE = False + def _parse_date_wo_default_month_day(self, field): """ Parse the field as an iso string but prevent dateutils from defaulting the day or month while @@ -76,12 +78,12 @@ class Date(ModelType): else: return value.isoformat() else: - raise TypeError("Cannot convert {} to json".format(value)) + raise TypeError("Cannot convert {!r} to json".format(value)) TIMEDELTA_REGEX = re.compile(r'^((?P\d+?) day(?:s?))?(\s)?((?P\d+?) hour(?:s?))?(\s)?((?P\d+?) minute(?:s)?)?(\s)?((?P\d+?) second(?:s)?)?$') -class Timedelta(ModelType): +class Timedelta(Field): # Timedeltas are immutable, see http://docs.python.org/2/library/datetime.html#available-types MUTABLE = False diff --git a/common/lib/xmodule/xmodule/foldit_module.py b/common/lib/xmodule/xmodule/foldit_module.py index cadf6cef0b..e1714ff96b 100644 --- a/common/lib/xmodule/xmodule/foldit_module.py +++ b/common/lib/xmodule/xmodule/foldit_module.py @@ -6,7 +6,7 @@ from pkg_resources import resource_string from xmodule.editing_module import EditingDescriptor from xmodule.x_module import XModule from xmodule.xml_module import XmlDescriptor -from xblock.core import Scope, Integer, String +from xblock.fields import Scope, Integer, String from .fields import Date diff --git a/common/lib/xmodule/xmodule/gst_module.py b/common/lib/xmodule/xmodule/gst_module.py index d6516d92ef..ef69130f84 100644 --- a/common/lib/xmodule/xmodule/gst_module.py +++ b/common/lib/xmodule/xmodule/gst_module.py @@ -14,7 +14,7 @@ from xmodule.xml_module import XmlDescriptor from xmodule.x_module import XModule from xmodule.stringify import stringify_children from pkg_resources import resource_string -from xblock.core import String, Scope +from xblock.fields import String, Scope log = logging.getLogger(__name__) diff --git a/common/lib/xmodule/xmodule/html_module.py b/common/lib/xmodule/xmodule/html_module.py index 7a68c42ac9..5b3ca5661a 100644 --- a/common/lib/xmodule/xmodule/html_module.py +++ b/common/lib/xmodule/xmodule/html_module.py @@ -7,7 +7,7 @@ from lxml import etree from path import path from pkg_resources import resource_string -from xblock.core import Scope, String +from xblock.fields import Scope, String from xmodule.editing_module import EditingDescriptor from xmodule.html_checker import check_html from xmodule.stringify import stringify_children diff --git a/common/lib/xmodule/xmodule/mako_module.py b/common/lib/xmodule/xmodule/mako_module.py index 526fc1a0eb..3d37bd615e 100644 --- a/common/lib/xmodule/xmodule/mako_module.py +++ b/common/lib/xmodule/xmodule/mako_module.py @@ -2,10 +2,8 @@ from .x_module import XModuleDescriptor, DescriptorSystem class MakoDescriptorSystem(DescriptorSystem): - def __init__(self, load_item, resources_fs, error_tracker, - render_template, **kwargs): - super(MakoDescriptorSystem, self).__init__( - load_item, resources_fs, error_tracker, **kwargs) + def __init__(self, render_template, **kwargs): + super(MakoDescriptorSystem, self).__init__(**kwargs) self.render_template = render_template diff --git a/common/lib/xmodule/xmodule/modulestore/__init__.py b/common/lib/xmodule/xmodule/modulestore/__init__.py index f2b70ad365..e7bcc171f7 100644 --- a/common/lib/xmodule/xmodule/modulestore/__init__.py +++ b/common/lib/xmodule/xmodule/modulestore/__init__.py @@ -398,7 +398,7 @@ class ModuleStoreBase(ModuleStore): ''' Implement interface functionality that can be shared. ''' - def __init__(self, metadata_inheritance_cache_subsystem=None, request_cache=None, modulestore_update_signal=None): + def __init__(self, metadata_inheritance_cache_subsystem=None, request_cache=None, modulestore_update_signal=None, xblock_mixins=()): ''' Set up the error-tracking logic. ''' @@ -406,6 +406,7 @@ class ModuleStoreBase(ModuleStore): self.metadata_inheritance_cache_subsystem = metadata_inheritance_cache_subsystem self.modulestore_update_signal = modulestore_update_signal self.request_cache = request_cache + self.xblock_mixins = xblock_mixins def _get_errorlog(self, location): """ diff --git a/common/lib/xmodule/xmodule/modulestore/django.py b/common/lib/xmodule/xmodule/modulestore/django.py index 9ff82e1137..93416cae17 100644 --- a/common/lib/xmodule/xmodule/modulestore/django.py +++ b/common/lib/xmodule/xmodule/modulestore/django.py @@ -62,6 +62,7 @@ def create_modulestore_instance(engine, options): metadata_inheritance_cache_subsystem=metadata_inheritance_cache, request_cache=request_cache, modulestore_update_signal=Signal(providing_args=['modulestore', 'course_id', 'location']), + xblock_mixins=getattr(settings, 'XBLOCK_MIXINS', ()), **_options ) diff --git a/common/lib/xmodule/xmodule/modulestore/inheritance.py b/common/lib/xmodule/xmodule/modulestore/inheritance.py index aeec53cc29..78d584580f 100644 --- a/common/lib/xmodule/xmodule/modulestore/inheritance.py +++ b/common/lib/xmodule/xmodule/modulestore/inheritance.py @@ -1,17 +1,48 @@ -from xblock.core import Scope +from datetime import datetime +from pytz import UTC -# A list of metadata that this module can inherit from its parent module -INHERITABLE_METADATA = ( - 'graded', 'start', 'due', 'graceperiod', 'showanswer', 'rerandomize', - # TODO (ichuang): used for Fall 2012 xqa server access - 'xqa_key', - # How many days early to show a course element to beta testers (float) - # intended to be set per-course, but can be overridden in for specific - # elements. Can be a float. - 'days_early_for_beta', - 'giturl', # for git edit link - 'static_asset_path', # for static assets placed outside xcontent contentstore -) +from xblock.fields import Scope, Boolean, String, Float, XBlockMixin +from xmodule.fields import Date, Timedelta +from xblock.runtime import KeyValueStore + + +class InheritanceMixin(XBlockMixin): + """Field definitions for inheritable fields""" + + graded = Boolean( + help="Whether this module contributes to the final course grade", + default=False, + scope=Scope.settings + ) + + start = Date( + help="Start time when this module is visible", + default=datetime.fromtimestamp(0, UTC), + scope=Scope.settings + ) + due = Date(help="Date that this problem is due by", scope=Scope.settings) + giturl = String(help="url root for course data git repository", scope=Scope.settings) + xqa_key = String(help="DO NOT USE", scope=Scope.settings) + graceperiod = Timedelta( + help="Amount of time after the due date that submissions will be accepted", + scope=Scope.settings + ) + showanswer = String( + help="When to show the problem answer to the student", + scope=Scope.settings, + default="finished" + ) + rerandomize = String( + help="When to rerandomize the problem", + default="never", + scope=Scope.settings + ) + days_early_for_beta = Float( + help="Number of days early to show content to beta users", + default=None, + scope=Scope.settings + ) + static_asset_path = String(help="Path to use for static assets - overrides Studio c4x://", scope=Scope.settings, default='') def compute_inherited_metadata(descriptor): @@ -21,59 +52,69 @@ def compute_inherited_metadata(descriptor): NOTE: This means that there is no such thing as lazy loading at the moment--this accesses all the children.""" - for child in descriptor.get_children(): - inherit_metadata(child, descriptor._model_data) - compute_inherited_metadata(child) + if descriptor.has_children: + parent_metadata = descriptor.xblock_kvs.inherited_settings.copy() + # add any of descriptor's explicitly set fields to the inheriting list + for field in InheritanceMixin.fields.values(): + # pylint: disable = W0212 + if descriptor._field_data.has(descriptor, field.name): + # inherited_settings values are json repr + parent_metadata[field.name] = field.read_json(descriptor) + + for child in descriptor.get_children(): + inherit_metadata(child, parent_metadata) + compute_inherited_metadata(child) -def inherit_metadata(descriptor, model_data): +def inherit_metadata(descriptor, inherited_data): """ Updates this module with metadata inherited from a containing module. Only metadata specified in self.inheritable_metadata will be inherited + + `inherited_data`: A dictionary mapping field names to the values that + they should inherit """ - # The inherited values that are actually being used. - if not hasattr(descriptor, '_inherited_metadata'): - setattr(descriptor, '_inherited_metadata', {}) - - # All inheritable metadata values (for which a value exists in model_data). - if not hasattr(descriptor, '_inheritable_metadata'): - setattr(descriptor, '_inheritable_metadata', {}) - - # Set all inheritable metadata from kwargs that are - # in self.inheritable_metadata and aren't already set in metadata - for attr in INHERITABLE_METADATA: - if attr in model_data: - descriptor._inheritable_metadata[attr] = model_data[attr] - if attr not in descriptor._model_data: - descriptor._inherited_metadata[attr] = model_data[attr] - descriptor._model_data[attr] = model_data[attr] + try: + descriptor.xblock_kvs.inherited_settings = inherited_data + except AttributeError: # the kvs doesn't have inherited_settings probably b/c it's an error module + pass def own_metadata(module): - # IN SPLIT MONGO this is just ['metadata'] as it keeps ['_inherited_metadata'] separate! - # FIXME move into kvs? will that work for xml mongo? """ Return a dictionary that contains only non-inherited field keys, - mapped to their values + mapped to their serialized values """ - inherited_metadata = getattr(module, '_inherited_metadata', {}) - metadata = {} - for field in module.fields + module.lms.fields: - # Only save metadata that wasn't inherited - if field.scope != Scope.settings: - continue + return module.get_explicitly_set_fields_by_scope(Scope.settings) - if field.name in inherited_metadata and module._model_data.get(field.name) == inherited_metadata.get(field.name): - continue +class InheritanceKeyValueStore(KeyValueStore): + """ + Common superclass for kvs's which know about inheritance of settings. Offers simple + dict-based storage of fields and lookup of inherited values. - if field.name not in module._model_data: - continue + Note: inherited_settings is a dict of key to json values (internal xblock field repr) + """ + def __init__(self, initial_values=None, inherited_settings=None): + super(InheritanceKeyValueStore, self).__init__() + self.inherited_settings = inherited_settings or {} + self._fields = initial_values or {} - try: - metadata[field.name] = module._model_data[field.name] - except KeyError: - # Ignore any missing keys in _model_data - pass + def get(self, key): + return self._fields[key.field_name] - return metadata + def set(self, key, value): + # xml backed courses are read-only, but they do have some computed fields + self._fields[key.field_name] = value + + def delete(self, key): + del self._fields[key.field_name] + + def has(self, key): + return key.field_name in self._fields + + def default(self, key): + """ + Check to see if the default should be from inheritance rather than from the field's global default + """ + return self.inherited_settings[key.field_name] diff --git a/common/lib/xmodule/xmodule/modulestore/locator.py b/common/lib/xmodule/xmodule/modulestore/locator.py index bdc9e61fce..916a970ad0 100644 --- a/common/lib/xmodule/xmodule/modulestore/locator.py +++ b/common/lib/xmodule/xmodule/modulestore/locator.py @@ -6,7 +6,6 @@ from __future__ import absolute_import import logging import inspect from abc import ABCMeta, abstractmethod -from urllib import quote from bson.objectid import ObjectId from bson.errors import InvalidId @@ -19,6 +18,15 @@ from .parsers import BRANCH_PREFIX, BLOCK_PREFIX, URL_VERSION_PREFIX log = logging.getLogger(__name__) +class LocalId(object): + """ + Class for local ids for non-persisted xblocks + + Should be hashable and distinguishable, but nothing else + """ + pass + + class Locator(object): """ A locator is like a URL, it refers to a course resource. @@ -386,9 +394,12 @@ class BlockUsageLocator(CourseLocator): self.set_property('usage_id', new) def init_block_ref(self, block_ref): - parse = parse_block_ref(block_ref) - assert parse, 'Could not parse "%s" as a block_ref' % block_ref - self.set_usage_id(parse['block']) + if isinstance(block_ref, LocalId): + self.set_usage_id(block_ref) + else: + parse = parse_block_ref(block_ref) + assert parse, 'Could not parse "%s" as a block_ref' % block_ref + self.set_usage_id(parse['block']) def init_block_ref_from_url(self, url): if isinstance(url, Locator): @@ -409,12 +420,8 @@ class BlockUsageLocator(CourseLocator): """ Return a string representing this location. """ - rep = CourseLocator.__unicode__(self) - if self.usage_id is None: - # usage_id has not been initialized - return rep + BLOCK_PREFIX + 'NONE' - else: - return rep + BLOCK_PREFIX + self.usage_id + rep = super(BlockUsageLocator, self).__unicode__() + return rep + BLOCK_PREFIX + unicode(self.usage_id) class DescriptionLocator(Locator): diff --git a/common/lib/xmodule/xmodule/modulestore/mixed.py b/common/lib/xmodule/xmodule/modulestore/mixed.py index 8827f33360..6a1c3b37c9 100644 --- a/common/lib/xmodule/xmodule/modulestore/mixed.py +++ b/common/lib/xmodule/xmodule/modulestore/mixed.py @@ -29,9 +29,9 @@ class MixedModuleStore(ModuleStoreBase): if 'default' not in stores: raise Exception('Missing a default modulestore in the MixedModuleStore __init__ method.') - for key in stores: - self.modulestores[key] = create_modulestore_instance(stores[key]['ENGINE'], - stores[key]['OPTIONS']) + for key, store in stores.items(): + self.modulestores[key] = create_modulestore_instance(store['ENGINE'], + store['OPTIONS']) def _get_modulestore_for_courseid(self, course_id): """ diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/__init__.py b/common/lib/xmodule/xmodule/modulestore/mongo/__init__.py index bdeda76d32..c20ca43e84 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/__init__.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/__init__.py @@ -2,7 +2,7 @@ Provide names as exported by older mongo.py module """ -from xmodule.modulestore.mongo.base import MongoModuleStore, MongoKeyValueStore, MongoUsage +from xmodule.modulestore.mongo.base import MongoModuleStore, MongoKeyValueStore # Backwards compatibility for prod systems that refererence # xmodule.modulestore.mongo.DraftMongoModuleStore diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/base.py b/common/lib/xmodule/xmodule/modulestore/mongo/base.py index b1fecec120..11fd392134 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/base.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/base.py @@ -17,24 +17,23 @@ import sys import logging import copy -from collections import namedtuple from fs.osfs import OSFS from itertools import repeat from path import path from operator import attrgetter -from uuid import uuid4 from importlib import import_module from xmodule.errortracker import null_error_tracker, exc_info_to_str from xmodule.mako_module import MakoDescriptorSystem from xmodule.x_module import XModuleDescriptor from xmodule.error_module import ErrorDescriptor -from xblock.runtime import DbModel, KeyValueStore, InvalidScopeError -from xblock.core import Scope +from xblock.runtime import DbModel +from xblock.exceptions import InvalidScopeError +from xblock.fields import Scope, ScopeIds from xmodule.modulestore import ModuleStoreBase, Location, namedtuple_to_son, MONGO_MODULESTORE_TYPE from xmodule.modulestore.exceptions import ItemNotFoundError -from xmodule.modulestore.inheritance import own_metadata, INHERITABLE_METADATA, inherit_metadata +from xmodule.modulestore.inheritance import own_metadata, InheritanceMixin, inherit_metadata, InheritanceKeyValueStore log = logging.getLogger(__name__) @@ -57,17 +56,16 @@ class InvalidWriteError(Exception): """ -class MongoKeyValueStore(KeyValueStore): +class MongoKeyValueStore(InheritanceKeyValueStore): """ A KeyValueStore that maps keyed data access to one of the 3 data areas known to the MongoModuleStore (data, children, and metadata) """ - def __init__(self, data, children, metadata, location, category): + def __init__(self, data, children, metadata): + super(MongoKeyValueStore, self).__init__() self._data = data self._children = children self._metadata = metadata - self._location = location - self._category = category def get(self, key): if key.scope == Scope.children: @@ -77,11 +75,7 @@ class MongoKeyValueStore(KeyValueStore): elif key.scope == Scope.settings: return self._metadata[key.field_name] elif key.scope == Scope.content: - if key.field_name == 'location': - return self._location - elif key.field_name == 'category': - return self._category - elif key.field_name == 'data' and not isinstance(self._data, dict): + if key.field_name == 'data' and not isinstance(self._data, dict): return self._data else: return self._data[key.field_name] @@ -94,11 +88,7 @@ class MongoKeyValueStore(KeyValueStore): elif key.scope == Scope.settings: self._metadata[key.field_name] = value elif key.scope == Scope.content: - if key.field_name == 'location': - self._location = value - elif key.field_name == 'category': - self._category = value - elif key.field_name == 'data' and not isinstance(self._data, dict): + if key.field_name == 'data' and not isinstance(self._data, dict): self._data = value else: self._data[key.field_name] = value @@ -112,11 +102,7 @@ class MongoKeyValueStore(KeyValueStore): if key.field_name in self._metadata: del self._metadata[key.field_name] elif key.scope == Scope.content: - if key.field_name == 'location': - self._location = Location(None) - elif key.field_name == 'category': - self._category = None - elif key.field_name == 'data' and not isinstance(self._data, dict): + if key.field_name == 'data' and not isinstance(self._data, dict): self._data = None else: del self._data[key.field_name] @@ -129,12 +115,7 @@ class MongoKeyValueStore(KeyValueStore): elif key.scope == Scope.settings: return key.field_name in self._metadata elif key.scope == Scope.content: - if key.field_name == 'location': - # WHY TRUE? if it's been deleted should it be False? - return True - elif key.field_name == 'category': - return self._category is not None - elif key.field_name == 'data' and not isinstance(self._data, dict): + if key.field_name == 'data' and not isinstance(self._data, dict): return True else: return key.field_name in self._data @@ -142,9 +123,6 @@ class MongoKeyValueStore(KeyValueStore): return False -MongoUsage = namedtuple('MongoUsage', 'id, def_id') - - class CachingDescriptorSystem(MakoDescriptorSystem): """ A system that has a cache of module json that it will use to load modules @@ -152,8 +130,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem): TODO (cdodge) when the 'split module store' work has been completed we can remove all references to metadata_inheritance_tree """ - def __init__(self, modulestore, module_data, default_class, resources_fs, - error_tracker, render_template, cached_metadata=None): + def __init__(self, modulestore, module_data, default_class, cached_metadata, **kwargs): """ modulestore: the module store that can be used to retrieve additional modules @@ -170,8 +147,8 @@ class CachingDescriptorSystem(MakoDescriptorSystem): render_template: a function for rendering templates, as per MakoDescriptorSystem """ - super(CachingDescriptorSystem, self).__init__(self.load_item, resources_fs, - error_tracker, render_template) + super(CachingDescriptorSystem, self).__init__(load_item=self.load_item, **kwargs) + self.modulestore = modulestore self.module_data = module_data self.default_class = default_class @@ -190,7 +167,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem): module = self.modulestore.get_item(location) if module is not None: # update our own cache after going to the DB to get cache miss - self.module_data.update(module.system.module_data) + self.module_data.update(module.runtime.module_data) return module else: # load the module and apply the inherited metadata @@ -202,7 +179,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem): ) definition = json_data.get('definition', {}) metadata = json_data.get('metadata', {}) - for old_name, new_name in class_.metadata_translations.items(): + for old_name, new_name in getattr(class_, 'metadata_translations', {}).items(): if old_name in metadata: metadata[new_name] = metadata[old_name] del metadata[old_name] @@ -211,18 +188,18 @@ class CachingDescriptorSystem(MakoDescriptorSystem): definition.get('data', {}), definition.get('children', []), metadata, - location, - category ) - model_data = DbModel(kvs, class_, None, MongoUsage(self.course_id, location)) - model_data['category'] = category - model_data['location'] = location - module = class_(self, model_data) + field_data = DbModel(kvs) + scope_ids = ScopeIds(None, category, location, location) + module = self.construct_xblock_from_class(class_, field_data, scope_ids) if self.cached_metadata is not None: # parent container pointers don't differentiate between draft and non-draft # so when we do the lookup, we should do so with a non-draft location non_draft_loc = location.replace(revision=None) + + # Convert the serialized fields values in self.cached_metadata + # to python values metadata_to_inherit = self.cached_metadata.get(non_draft_loc.url(), {}) inherit_metadata(module, metadata_to_inherit) # decache any computed pending field settings @@ -323,8 +300,8 @@ class MongoModuleStore(ModuleStoreBase): # just get the inheritable metadata since that is all we need for the computation # this minimizes both data pushed over the wire - for attr in INHERITABLE_METADATA: - record_filter['metadata.{0}'.format(attr)] = 1 + for field_name in InheritanceMixin.fields: + record_filter['metadata.{0}'.format(field_name)] = 1 # call out to the DB resultset = self.collection.find(query, record_filter) @@ -496,13 +473,14 @@ class MongoModuleStore(ModuleStoreBase): # TODO (cdodge): When the 'split module store' work has been completed, we should remove # the 'metadata_inheritance_tree' parameter system = CachingDescriptorSystem( - self, - data_cache, - self.default_class, - resource_fs, - self.error_tracker, - self.render_template, - cached_metadata, + modulestore=self, + module_data=data_cache, + default_class=self.default_class, + resources_fs=resource_fs, + error_tracker=self.error_tracker, + render_template=self.render_template, + cached_metadata=cached_metadata, + mixins=self.xblock_mixins, ) return system.load_item(item['location']) @@ -606,7 +584,7 @@ class MongoModuleStore(ModuleStoreBase): :param location: a Location--must have a category :param definition_data: can be empty. The initial definition_data for the kvs :param metadata: can be empty, the initial metadata for the kvs - :param system: if you already have an xmodule from the course, the xmodule.system value + :param system: if you already have an xblock from the course, the xblock.runtime value """ if not isinstance(location, Location): location = Location(location) @@ -616,13 +594,14 @@ class MongoModuleStore(ModuleStoreBase): metadata = {} if system is None: system = CachingDescriptorSystem( - self, - {}, - self.default_class, - None, - self.error_tracker, - self.render_template, - {} + modulestore=self, + module_data={}, + default_class=self.default_class, + resources_fs=None, + error_tracker=self.error_tracker, + render_template=self.render_template, + cached_metadata={}, + mixins=self.xblock_mixins, ) xblock_class = XModuleDescriptor.load_class(location.category, self.default_class) if definition_data is None: @@ -630,8 +609,16 @@ class MongoModuleStore(ModuleStoreBase): definition_data = getattr(xblock_class, 'data').default else: definition_data = {} - dbmodel = self._create_new_model_data(location.category, location, definition_data, metadata) - xmodule = xblock_class(system, dbmodel) + dbmodel = self._create_new_field_data(location.category, location, definition_data, metadata) + xmodule = system.construct_xblock_from_class( + xblock_class, + dbmodel, + + # We're loading a descriptor, so student_id is meaningless + # We also don't have separate notions of definition and usage ids yet, + # so we use the location for both. + ScopeIds(None, location.category, location, location) + ) # decache any pending field settings from init xmodule.save() return xmodule @@ -668,7 +655,7 @@ class MongoModuleStore(ModuleStoreBase): :param location: a Location--must have a category :param definition_data: can be empty. The initial definition_data for the kvs :param metadata: can be empty, the initial metadata for the kvs - :param system: if you already have an xmodule from the course, the xmodule.system value + :param system: if you already have an xblock from the course, the xblock.runtime value """ # differs from split mongo in that I believe most of this logic should be above the persistence # layer but added it here to enable quick conversion. I'll need to reconcile these. @@ -848,7 +835,7 @@ class MongoModuleStore(ModuleStoreBase): """ return MONGO_MODULESTORE_TYPE - def _create_new_model_data(self, category, location, definition_data, metadata): + def _create_new_field_data(self, category, location, definition_data, metadata): """ To instantiate a new xmodule which will be saved latter, set up the dbModel and kvs """ @@ -856,15 +843,7 @@ class MongoModuleStore(ModuleStoreBase): definition_data, [], metadata, - location, - category ) - class_ = XModuleDescriptor.load_class( - category, - self.default_class - ) - model_data = DbModel(kvs, class_, None, MongoUsage(None, location)) - model_data['category'] = category - model_data['location'] = location - return model_data + field_data = DbModel(kvs) + return field_data diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py b/common/lib/xmodule/xmodule/modulestore/mongo/draft.py index d289e03739..610f7b1b8a 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/draft.py @@ -42,7 +42,7 @@ def wrap_draft(item): non-draft location in either case """ setattr(item, 'is_draft', item.location.revision == DRAFT) - item.location = item.location.replace(revision=None) + item.scope_ids = item.scope_ids._replace(usage_id=item.location.replace(revision=None)) return item @@ -235,10 +235,10 @@ class DraftModuleStore(MongoModuleStore): """ draft = self.get_item(location) - draft.cms.published_date = datetime.now(UTC) - draft.cms.published_by = published_by_id - super(DraftModuleStore, self).update_item(location, draft._model_data._kvs._data) - super(DraftModuleStore, self).update_children(location, draft._model_data._kvs._children) + draft.published_date = datetime.now(UTC) + draft.published_by = published_by_id + super(DraftModuleStore, self).update_item(location, draft._field_data._kvs._data) + super(DraftModuleStore, self).update_children(location, draft._field_data._kvs._children) super(DraftModuleStore, self).update_metadata(location, own_metadata(draft)) self.delete_item(location) diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py index 73dcabfa69..020ebdbbfe 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py @@ -2,15 +2,17 @@ import sys import logging from xmodule.mako_module import MakoDescriptorSystem from xmodule.x_module import XModuleDescriptor -from xmodule.modulestore.locator import BlockUsageLocator +from xmodule.modulestore.locator import BlockUsageLocator, LocalId from xmodule.error_module import ErrorDescriptor from xmodule.errortracker import exc_info_to_str from xblock.runtime import DbModel from ..exceptions import ItemNotFoundError -from .split_mongo_kvs import SplitMongoKVS, SplitMongoKVSid +from .split_mongo_kvs import SplitMongoKVS +from xblock.fields import ScopeIds log = logging.getLogger(__name__) + class CachingDescriptorSystem(MakoDescriptorSystem): """ A system that has a cache of a course version's json that it will use to load modules @@ -18,8 +20,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem): Computes the settings (nee 'metadata') inheritance upon creation. """ - def __init__(self, modulestore, course_entry, module_data, lazy, - default_class, error_tracker, render_template): + def __init__(self, modulestore, course_entry, default_class, module_data, lazy, **kwargs): """ Computes the settings inheritance and sets up the cache. @@ -28,34 +29,31 @@ class CachingDescriptorSystem(MakoDescriptorSystem): module_data: a dict mapping Location -> json that was cached from the underlying modulestore - - default_class: The default_class to use when loading an - XModuleDescriptor from the module_data - - resources_fs: a filesystem, as per MakoDescriptorSystem - - error_tracker: a function that logs errors for later display to users - - render_template: a function for rendering templates, as per - MakoDescriptorSystem """ # TODO find all references to resources_fs and make handle None - super(CachingDescriptorSystem, self).__init__( - self._load_item, None, error_tracker, render_template) + super(CachingDescriptorSystem, self).__init__(load_item=self._load_item, **kwargs) self.modulestore = modulestore self.course_entry = course_entry self.lazy = lazy self.module_data = module_data - self.default_class = default_class # TODO see if self.course_id is needed: is already in course_entry but could be > 1 value # Compute inheritance modulestore.inherit_settings( course_entry.get('blocks', {}), course_entry.get('blocks', {}).get(course_entry.get('root')) ) + self.default_class = default_class + self.local_modules = {} def _load_item(self, usage_id, course_entry_override=None): # TODO ensure all callers of system.load_item pass just the id + + if isinstance(usage_id, BlockUsageLocator) and isinstance(usage_id.usage_id, LocalId): + try: + return self.local_modules[usage_id] + except KeyError: + raise ItemNotFoundError + json_data = self.module_data.get(usage_id) if json_data is None: # deeper than initial descendant fetch or doesn't exist @@ -75,6 +73,11 @@ class CachingDescriptorSystem(MakoDescriptorSystem): course_entry_override = self.course_entry # most likely a lazy loader or the id directly definition = json_data.get('definition', {}) + definition_id = self.modulestore.definition_locator(definition) + + # If no usage id is provided, generate an in-memory id + if usage_id is None: + usage_id = LocalId() block_locator = BlockUsageLocator( version_guid=course_entry_override['_id'], @@ -87,25 +90,24 @@ class CachingDescriptorSystem(MakoDescriptorSystem): definition, json_data.get('fields', {}), json_data.get('_inherited_settings'), - block_locator, - json_data.get('category')) - model_data = DbModel(kvs, class_, None, - SplitMongoKVSid( - # DbModel req's that these support .url() - block_locator, - self.modulestore.definition_locator(definition))) + ) + field_data = DbModel(kvs) try: - module = class_(self, model_data) + module = self.construct_xblock_from_class( + class_, + field_data, + ScopeIds(None, json_data.get('category'), definition_id, block_locator) + ) except Exception: log.warning("Failed to load descriptor", exc_info=True) - if usage_id is None: - usage_id = "MISSING" return ErrorDescriptor.from_json( json_data, self, - BlockUsageLocator(version_guid=course_entry_override['_id'], - usage_id=usage_id), + BlockUsageLocator( + version_guid=course_entry_override['_id'], + usage_id=usage_id + ), error_msg=exc_info_to_str(sys.exc_info()) ) @@ -117,4 +119,9 @@ class CachingDescriptorSystem(MakoDescriptorSystem): module.definition_locator = self.modulestore.definition_locator(definition) # decache any pending field settings module.save() + + # If this is an in-memory block, store it in this system + if isinstance(block_locator.usage_id, LocalId): + self.local_modules[block_locator] = module + return module diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index c5f4c2404c..bb9aa59016 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py @@ -8,14 +8,15 @@ from path import path from xmodule.errortracker import null_error_tracker from xmodule.x_module import XModuleDescriptor -from xmodule.modulestore.locator import BlockUsageLocator, DescriptionLocator, CourseLocator, VersionTree +from xmodule.modulestore.locator import BlockUsageLocator, DescriptionLocator, CourseLocator, VersionTree, LocalId from xmodule.modulestore.exceptions import InsufficientSpecificationError, VersionConflictError from xmodule.modulestore import inheritance, ModuleStoreBase from ..exceptions import ItemNotFoundError from .definition_lazy_loader import DefinitionLazyLoader from .caching_descriptor_system import CachingDescriptorSystem -from xblock.core import Scope +from xblock.fields import Scope +from xblock.runtime import Mixologist from pytz import UTC import collections @@ -41,6 +42,8 @@ log = logging.getLogger(__name__) #============================================================================== + + class SplitMongoModuleStore(ModuleStoreBase): """ A Mongodb backed ModuleStore supporting versions, inheritance, @@ -53,7 +56,7 @@ class SplitMongoModuleStore(ModuleStoreBase): mongo_options=None, **kwargs): - ModuleStoreBase.__init__(self) + super(SplitMongoModuleStore, self).__init__(**kwargs) if mongo_options is None: mongo_options = {} @@ -93,6 +96,11 @@ class SplitMongoModuleStore(ModuleStoreBase): self.error_tracker = error_tracker self.render_template = render_template + # TODO: Don't have a runtime just to generate the appropriate mixin classes (cpennington) + # This is only used by _partition_fields_by_scope, which is only needed because + # the split mongo store is used for item creation as well as item persistence + self.mixologist = Mixologist(self.xblock_mixins) + def cache_items(self, system, base_usage_ids, depth=0, lazy=True): ''' Handles caching of items once inheritance and any other one time @@ -144,13 +152,15 @@ class SplitMongoModuleStore(ModuleStoreBase): system = self._get_cache(course_entry['_id']) if system is None: system = CachingDescriptorSystem( - self, - course_entry, - {}, - lazy, - self.default_class, - self.error_tracker, - self.render_template + modulestore=self, + course_entry=course_entry, + module_data={}, + lazy=lazy, + default_class=self.default_class, + error_tracker=self.error_tracker, + render_template=self.render_template, + resources_fs=None, + mixins=self.xblock_mixins ) self._add_cache(course_entry['_id'], system) self.cache_items(system, usage_ids, depth, lazy) @@ -943,12 +953,12 @@ class SplitMongoModuleStore(ModuleStoreBase): xblock.definition_locator, is_updated = self.update_definition_from_data( xblock.definition_locator, new_def_data, user_id) - if xblock.location.usage_id is None: + if isinstance(xblock.scope_ids.usage_id.usage_id, LocalId): # generate an id is_new = True is_updated = True usage_id = self._generate_usage_id(structure_blocks, xblock.category) - xblock.location.usage_id = usage_id + xblock.scope_ids.usage_id.usage_id = usage_id else: is_new = False usage_id = xblock.location.usage_id @@ -960,9 +970,10 @@ class SplitMongoModuleStore(ModuleStoreBase): updated_blocks = [] if xblock.has_children: for child in xblock.children: - if isinstance(child, XModuleDescriptor): - updated_blocks += self._persist_subdag(child, user_id, structure_blocks) - children.append(child.location.usage_id) + if isinstance(child.usage_id, LocalId): + child_block = xblock.system.get_block(child) + updated_blocks += self._persist_subdag(child_block, user_id, structure_blocks) + children.append(child_block.location.usage_id) else: children.append(child) @@ -1118,11 +1129,11 @@ class SplitMongoModuleStore(ModuleStoreBase): """ return {} - def inherit_settings(self, block_map, block, inheriting_settings=None): + def inherit_settings(self, block_map, block_json, inheriting_settings=None): """ - Updates block with any inheritable setting set by an ancestor and recurses to children. + Updates block_json with any inheritable setting set by an ancestor and recurses to children. """ - if block is None: + if block_json is None: return if inheriting_settings is None: @@ -1132,14 +1143,14 @@ class SplitMongoModuleStore(ModuleStoreBase): # NOTE: this should show the values which all fields would have if inherited: i.e., # not set to the locally defined value but to value set by nearest ancestor who sets it # ALSO NOTE: no xblock should ever define a _inherited_settings field as it will collide w/ this logic. - block.setdefault('_inherited_settings', {}).update(inheriting_settings) + block_json.setdefault('_inherited_settings', {}).update(inheriting_settings) # update the inheriting w/ what should pass to children - inheriting_settings = block['_inherited_settings'].copy() - block_fields = block['fields'] - for field in inheritance.INHERITABLE_METADATA: - if field in block_fields: - inheriting_settings[field] = block_fields[field] + inheriting_settings = block_json['_inherited_settings'].copy() + block_fields = block_json['fields'] + for field_name in inheritance.InheritanceMixin.fields: + if field_name in block_fields: + inheriting_settings[field_name] = block_fields[field_name] for child in block_fields.get('children', []): self.inherit_settings(block_map, block_map[child], inheriting_settings) @@ -1308,7 +1319,7 @@ class SplitMongoModuleStore(ModuleStoreBase): """ if fields is None: return {} - cls = XModuleDescriptor.load_class(category) + cls = self.mixologist.mix(XModuleDescriptor.load_class(category)) result = collections.defaultdict(dict) for field_name, value in fields.iteritems(): field = getattr(cls, field_name) diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split_mongo_kvs.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split_mongo_kvs.py index e4a4b92027..e2d8fd30a9 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split_mongo_kvs.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split_mongo_kvs.py @@ -1,41 +1,34 @@ import copy -from xblock.core import Scope +from xblock.fields import Scope from collections import namedtuple -from xblock.runtime import KeyValueStore, InvalidScopeError +from xblock.runtime import KeyValueStore +from xblock.exceptions import InvalidScopeError from .definition_lazy_loader import DefinitionLazyLoader +from xmodule.modulestore.inheritance import InheritanceKeyValueStore # id is a BlockUsageLocator, def_id is the definition's guid SplitMongoKVSid = namedtuple('SplitMongoKVSid', 'id, def_id') -PROVENANCE_LOCAL = 'local' -PROVENANCE_DEFAULT = 'default' -PROVENANCE_INHERITED = 'inherited' - -class SplitMongoKVS(KeyValueStore): +class SplitMongoKVS(InheritanceKeyValueStore): """ A KeyValueStore that maps keyed data access to one of the 3 data areas known to the MongoModuleStore (data, children, and metadata) """ - def __init__(self, definition, fields, _inherited_settings, location, category): + def __init__(self, definition, fields, inherited_settings): """ :param definition: either a lazyloader or definition id for the definition :param fields: a dictionary of the locally set fields - :param _inherited_settings: the value of each inheritable field from above this. + :param inherited_settings: the json value of each inheritable field from above this. Note, local fields may override and disagree w/ this b/c this says what the value should be if the field is undefined. """ - # ensure kvs's don't share objects w/ others so that changes can't appear in separate ones - # the particular use case was that changes to kvs's were polluting caches. My thinking was - # that kvs's should be independent thus responsible for the isolation. + super(SplitMongoKVS, self).__init__(copy.copy(fields), inherited_settings) self._definition = definition # either a DefinitionLazyLoader or the db id of the definition. # if the db id, then the definition is presumed to be loaded into _fields - self._fields = copy.copy(fields) - self._inherited_settings = _inherited_settings - self._location = location - self._category = category + def get(self, key): # simplest case, field is directly set @@ -50,18 +43,10 @@ class SplitMongoKVS(KeyValueStore): # didn't find children in _fields; so, see if there's a default raise KeyError() elif key.scope == Scope.settings: - # didn't find in _fields; so, get from inheritance since not locally set - if key.field_name in self._inherited_settings: - return self._inherited_settings[key.field_name] - else: - # or get default - raise KeyError() + # get default which may be the inherited value + raise KeyError() elif key.scope == Scope.content: - if key.field_name == 'location': - return self._location - elif key.field_name == 'category': - return self._category - elif isinstance(self._definition, DefinitionLazyLoader): + if isinstance(self._definition, DefinitionLazyLoader): self._load_definition() if key.field_name in self._fields: return self._fields[key.field_name] @@ -75,14 +60,7 @@ class SplitMongoKVS(KeyValueStore): if key.scope not in [Scope.children, Scope.settings, Scope.content]: raise InvalidScopeError(key.scope) if key.scope == Scope.content: - if key.field_name == 'location': - self._location = value # is changing this legal? - return - elif key.field_name == 'category': - # TODO should this raise an exception? category is not changeable. - return - else: - self._load_definition() + self._load_definition() # set the field self._fields[key.field_name] = value @@ -99,13 +77,7 @@ class SplitMongoKVS(KeyValueStore): if key.scope not in [Scope.children, Scope.settings, Scope.content]: raise InvalidScopeError(key.scope) if key.scope == Scope.content: - if key.field_name == 'location': - return # noop - elif key.field_name == 'category': - # TODO should this raise an exception? category is not deleteable. - return # noop - else: - self._load_definition() + self._load_definition() # delete the field value if key.field_name in self._fields: @@ -123,53 +95,14 @@ class SplitMongoKVS(KeyValueStore): """ # handle any special cases if key.scope == Scope.content: - if key.field_name == 'location': - return True - elif key.field_name == 'category': - return self._category is not None - else: - self._load_definition() + self._load_definition() elif key.scope == Scope.parent: return True # it's not clear whether inherited values should return True. Right now they don't - # if someone changes it so that they do, then change any tests of field.name in xx._model_data + # if someone changes it so that they do, then change any tests of field.name in xx._field_data return key.field_name in self._fields - # would like to just take a key, but there's a bunch of magic in DbModel for constructing the key via - # a private method - def field_value_provenance(self, key_scope, key_name): - """ - Where the field value comes from: one of [PROVENANCE_LOCAL, PROVENANCE_DEFAULT, PROVENANCE_INHERITED]. - """ - # handle any special cases - if key_scope == Scope.content: - if key_name == 'location': - return PROVENANCE_LOCAL - elif key_name == 'category': - return PROVENANCE_LOCAL - else: - self._load_definition() - if key_name in self._fields: - return PROVENANCE_LOCAL - else: - return PROVENANCE_DEFAULT - elif key_scope == Scope.parent: - return PROVENANCE_DEFAULT - # catch the locally set state - elif key_name in self._fields: - return PROVENANCE_LOCAL - elif key_scope == Scope.settings and key_name in self._inherited_settings: - return PROVENANCE_INHERITED - else: - return PROVENANCE_DEFAULT - - def get_inherited_settings(self): - """ - Get the settings set by the ancestors (which locally set fields may override or not) - """ - return self._inherited_settings - def _load_definition(self): """ Update fields w/ the lazily loaded definitions diff --git a/common/lib/xmodule/xmodule/modulestore/store_utilities.py b/common/lib/xmodule/xmodule/modulestore/store_utilities.py index 725d4aebb7..a025eb7c82 100644 --- a/common/lib/xmodule/xmodule/modulestore/store_utilities.py +++ b/common/lib/xmodule/xmodule/modulestore/store_utilities.py @@ -110,12 +110,27 @@ def _clone_modules(modulestore, modules, source_location, dest_location): original_loc = Location(module.location) if original_loc.category != 'course': - module.location = module.location._replace( - tag=dest_location.tag, org=dest_location.org, course=dest_location.course) + new_location = module.location._replace( + tag=dest_location.tag, + org=dest_location.org, + course=dest_location.course + ) + module.scope_ids = module.scope_ids._replace( + def_id=new_location, + usage_id=new_location + ) else: # on the course module we also have to update the module name - module.location = module.location._replace( - tag=dest_location.tag, org=dest_location.org, course=dest_location.course, name=dest_location.name) + new_location = module.location._replace( + tag=dest_location.tag, + org=dest_location.org, + course=dest_location.course, + name=dest_location.name + ) + module.scope_ids = module.scope_ids._replace( + def_id=new_location, + usage_id=new_location + ) print "Cloning module {0} to {1}....".format(original_loc, module.location) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/factories.py b/common/lib/xmodule/xmodule/modulestore/tests/factories.py index 7913434086..4ad801aef8 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/factories.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/factories.py @@ -6,8 +6,6 @@ from pytz import UTC from xmodule.modulestore import Location from xmodule.modulestore.django import editable_modulestore -from xmodule.course_module import CourseDescriptor -from xblock.core import Scope from xmodule.x_module import XModuleDescriptor @@ -35,7 +33,7 @@ class XModuleCourseFactory(Factory): if display_name is not None: new_course.display_name = display_name - new_course.lms.start = datetime.datetime.now(UTC).replace(microsecond=0) + new_course.start = datetime.datetime.now(UTC).replace(microsecond=0) # The rest of kwargs become attributes on the course: for k, v in kwargs.iteritems(): diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py index 25ad258668..6f3276bf44 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py @@ -6,8 +6,9 @@ from nose.tools import assert_equals, assert_raises, \ import pymongo from uuid import uuid4 -from xblock.core import Scope -from xblock.runtime import KeyValueStore, InvalidScopeError +from xblock.fields import Scope +from xblock.runtime import KeyValueStore +from xblock.exceptions import InvalidScopeError from xmodule.tests import DATA_DIR from xmodule.modulestore import Location @@ -181,7 +182,7 @@ class TestMongoKeyValueStore(object): self.location = Location('i4x://org/course/category/name@version') self.children = ['i4x://org/course/child/a', 'i4x://org/course/child/b'] self.metadata = {'meta': 'meta_val'} - self.kvs = MongoKeyValueStore(self.data, self.children, self.metadata, self.location, 'category') + self.kvs = MongoKeyValueStore(self.data, self.children, self.metadata) def _check_read(self, key, expected_value): """ @@ -192,7 +193,6 @@ class TestMongoKeyValueStore(object): def test_read(self): assert_equals(self.data['foo'], self.kvs.get(KeyValueStore.Key(Scope.content, None, None, 'foo'))) - assert_equals(self.location, self.kvs.get(KeyValueStore.Key(Scope.content, None, None, 'location'))) assert_equals(self.children, self.kvs.get(KeyValueStore.Key(Scope.children, None, None, 'children'))) assert_equals(self.metadata['meta'], self.kvs.get(KeyValueStore.Key(Scope.settings, None, None, 'meta'))) assert_equals(None, self.kvs.get(KeyValueStore.Key(Scope.parent, None, None, 'parent'))) @@ -214,7 +214,6 @@ class TestMongoKeyValueStore(object): def test_write(self): yield (self._check_write, KeyValueStore.Key(Scope.content, None, None, 'foo'), 'new_data') - yield (self._check_write, KeyValueStore.Key(Scope.content, None, None, 'location'), Location('i4x://org/course/category/name@new_version')) yield (self._check_write, KeyValueStore.Key(Scope.children, None, None, 'children'), []) yield (self._check_write, KeyValueStore.Key(Scope.settings, None, None, 'meta'), 'new_settings') @@ -240,7 +239,6 @@ class TestMongoKeyValueStore(object): def test_delete(self): yield (self._check_delete_key_error, KeyValueStore.Key(Scope.content, None, None, 'foo')) - yield (self._check_delete_default, KeyValueStore.Key(Scope.content, None, None, 'location'), Location(None)) yield (self._check_delete_default, KeyValueStore.Key(Scope.children, None, None, 'children'), []) yield (self._check_delete_key_error, KeyValueStore.Key(Scope.settings, None, None, 'meta')) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py index d90c3020b4..b299f56711 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py @@ -9,10 +9,11 @@ import unittest import uuid from importlib import import_module -from xblock.core import Scope +from xblock.fields import Scope from xmodule.course_module import CourseDescriptor from xmodule.modulestore.exceptions import InsufficientSpecificationError, ItemNotFoundError, VersionConflictError from xmodule.modulestore.locator import CourseLocator, BlockUsageLocator, VersionTree, DescriptionLocator +from xmodule.modulestore.inheritance import InheritanceMixin from pytz import UTC from path import path import re @@ -31,6 +32,7 @@ class SplitModuleTest(unittest.TestCase): 'db': 'test_xmodule', 'collection': 'modulestore{0}'.format(uuid.uuid4().hex), 'fs_root': '', + 'xblock_mixins': (InheritanceMixin,) } MODULESTORE = { @@ -187,7 +189,7 @@ class SplitModuleCourseTests(SplitModuleTest): self.assertEqual(course.category, 'course') self.assertEqual(len(course.tabs), 6) self.assertEqual(course.display_name, "The Ancient Greek Hero") - self.assertEqual(course.lms.graceperiod, datetime.timedelta(hours=2)) + self.assertEqual(course.graceperiod, datetime.timedelta(hours=2)) self.assertIsNone(course.advertised_start) self.assertEqual(len(course.children), 0) self.assertEqual(course.definition_locator.definition_id, "head12345_11") @@ -893,7 +895,7 @@ class TestCourseCreation(SplitModuleTest): original = modulestore().get_course(original_locator) original_index = modulestore().get_course_index_info(original_locator) fields = {} - for field in original.fields: + for field in original.fields.values(): if field.scope == Scope.content and field.name != 'location': fields[field.name] = getattr(original, field.name) elif field.scope == Scope.settings: diff --git a/common/lib/xmodule/xmodule/modulestore/xml.py b/common/lib/xmodule/xmodule/modulestore/xml.py index 6527a5e34a..dd7888179b 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml.py +++ b/common/lib/xmodule/xmodule/modulestore/xml.py @@ -20,8 +20,11 @@ from xmodule.mako_module import MakoDescriptorSystem from xmodule.x_module import XModuleDescriptor, XMLParsingSystem from xmodule.html_module import HtmlDescriptor +from xblock.fields import ScopeIds +from xblock.field_data import DictFieldData from . import ModuleStoreBase, Location, XML_MODULESTORE_TYPE + from .exceptions import ItemNotFoundError from .inheritance import compute_inherited_metadata @@ -44,7 +47,7 @@ def clean_out_mako_templating(xml_string): class ImportSystem(XMLParsingSystem, MakoDescriptorSystem): def __init__(self, xmlstore, course_id, course_dir, - policy, error_tracker, parent_tracker, + error_tracker, parent_tracker, load_error_modules=True, **kwargs): """ A class that handles loading from xml. Does some munging to ensure that @@ -206,11 +209,14 @@ class ImportSystem(XMLParsingSystem, MakoDescriptorSystem): # policy to be loaded. For now, just add the course_id here... load_item = lambda location: xmlstore.get_instance(course_id, location) resources_fs = OSFS(xmlstore.data_dir / course_dir) - - MakoDescriptorSystem.__init__(self, load_item, resources_fs, - error_tracker, render_template, **kwargs) - XMLParsingSystem.__init__(self, load_item, resources_fs, - error_tracker, process_xml, policy, **kwargs) + super(ImportSystem, self).__init__( + load_item=load_item, + resources_fs=resources_fs, + render_template=render_template, + error_tracker=error_tracker, + process_xml=process_xml, + **kwargs + ) class ParentTracker(object): @@ -412,13 +418,14 @@ class XMLModuleStore(ModuleStoreBase): course_id = CourseDescriptor.make_id(org, course, url_name) system = ImportSystem( - self, - course_id, - course_dir, - policy, - tracker, - self.parent_trackers[course_id], - self.load_error_modules, + xmlstore=self, + course_id=course_id, + course_dir=course_dir, + error_tracker=tracker, + parent_tracker=self.parent_trackers[course_id], + load_error_modules=self.load_error_modules, + policy=policy, + mixins=self.xblock_mixins, ) course_descriptor = system.process_xml(etree.tostring(course_data, encoding='unicode')) @@ -467,9 +474,13 @@ class XMLModuleStore(ModuleStoreBase): # tabs are referenced in policy.json through a 'slug' which is just the filename without the .html suffix slug = os.path.splitext(os.path.basename(filepath))[0] loc = Location('i4x', course_descriptor.location.org, course_descriptor.location.course, category, slug) - module = HtmlDescriptor( - system, - {'data': html, 'location': loc, 'category': category} + module = system.construct_xblock_from_class( + HtmlDescriptor, + DictFieldData({'data': html, 'location': loc, 'category': category}), + # We're loading a descriptor, so student_id is meaningless + # We also don't have separate notions of definition and usage ids yet, + # so we use the location for both + ScopeIds(None, category, loc, loc), ) # VS[compat]: # Hack because we need to pull in the 'display_name' for static tabs (because we need to edit them) diff --git a/common/lib/xmodule/xmodule/modulestore/xml_importer.py b/common/lib/xmodule/xmodule/modulestore/xml_importer.py index beb255fb14..a9a7601e33 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml_importer.py +++ b/common/lib/xmodule/xmodule/modulestore/xml_importer.py @@ -3,7 +3,7 @@ import os import mimetypes from path import path -from xblock.core import Scope +from xblock.fields import Scope from .xml import XMLModuleStore, ImportSystem, ParentTracker from xmodule.modulestore import Location @@ -25,7 +25,7 @@ def import_static_content(modules, course_loc, course_data_path, static_content_ verbose = True - for dirname, dirnames, filenames in os.walk(static_dir): + for dirname, _, filenames in os.walk(static_dir): for filename in filenames: try: @@ -91,7 +91,8 @@ def import_from_xml(store, data_dir, course_dirs=None, data_dir, default_class=default_class, course_dirs=course_dirs, - load_error_modules=load_error_modules + load_error_modules=load_error_modules, + xblock_mixins=store.xblock_mixins, ) # NOTE: the XmlModuleStore does not implement get_items() which would be a preferable means @@ -120,7 +121,7 @@ def import_from_xml(store, data_dir, course_dirs=None, # Quick scan to get course module as we need some info from there. Also we need to make sure that the # course module is committed first into the store for module in xml_module_store.modules[course_id].itervalues(): - if module.category == 'course': + if module.scope_ids.block_type == 'course': course_data_path = path(data_dir) / module.data_dir course_location = module.location @@ -129,9 +130,9 @@ def import_from_xml(store, data_dir, course_dirs=None, module = remap_namespace(module, target_location_namespace) if not do_import_static: - module.lms.static_asset_path = module.data_dir # for old-style xblock where this was actually linked to kvs - module._model_data['static_asset_path'] = module.data_dir - log.debug('course static_asset_path={0}'.format(module.lms.static_asset_path)) + module.static_asset_path = module.data_dir # for old-style xblock where this was actually linked to kvs + module.save() + log.debug('course static_asset_path={0}'.format(module.static_asset_path)) log.debug('course data_dir={0}'.format(module.data_dir)) @@ -177,7 +178,7 @@ def import_from_xml(store, data_dir, course_dirs=None, # finally loop through all the modules for module in xml_module_store.modules[course_id].itervalues(): - if module.category == 'course': + if module.scope_ids.block_type == 'course': # we've already saved the course module up at the top of the loop # so just skip over it in the inner loop continue @@ -195,9 +196,15 @@ def import_from_xml(store, data_dir, course_dirs=None, # now import any 'draft' items if draft_store is not None: - import_course_draft(xml_module_store, store, draft_store, course_data_path, - static_content_store, course_location, target_location_namespace if target_location_namespace - else course_location) + import_course_draft( + xml_module_store, + store, + draft_store, + course_data_path, + static_content_store, + course_location, + target_location_namespace if target_location_namespace else course_location + ) finally: # turn back on all write signalling @@ -217,13 +224,13 @@ def import_module(module, store, course_data_path, static_content_store, logging.debug('processing import of module {0}...'.format(module.location.url())) content = {} - for field in module.fields: + for field in module.fields.values(): if field.scope != Scope.content: continue try: - content[field.name] = module._model_data[field.name] + content[field.name] = module._field_data.get(module, field.name) except KeyError: - # Ignore any missing keys in _model_data + # Ignore any missing keys in _field_data pass module_data = {} @@ -274,13 +281,13 @@ def import_course_draft(xml_module_store, store, draft_store, course_data_path, # create a new 'System' object which will manage the importing errorlog = make_error_tracker() system = ImportSystem( - xml_module_store, - target_location_namespace.course_id, - draft_dir, - {}, - errorlog.tracker, - ParentTracker(), - None, + xmlstore=xml_module_store, + course_id=target_location_namespace.course_id, + course_dir=draft_dir, + policy={}, + error_tracker=errorlog.tracker, + parent_tracker=ParentTracker(), + load_error_modules=False, ) # now walk the /vertical directory where each file in there will be a draft copy of the Vertical @@ -368,15 +375,30 @@ def remap_namespace(module, target_location_namespace): # This looks a bit wonky as we need to also change the 'name' of the imported course to be what # the caller passed in if module.location.category != 'course': - module.location = module.location._replace(tag=target_location_namespace.tag, org=target_location_namespace.org, - course=target_location_namespace.course) + new_location = module.location._replace( + tag=target_location_namespace.tag, + org=target_location_namespace.org, + course=target_location_namespace.course + ) + module.scope_ids = module.scope_ids._replace( + def_id=new_location, + usage_id=new_location + ) else: original_location = module.location # # module is a course module # - module.location = module.location._replace(tag=target_location_namespace.tag, org=target_location_namespace.org, - course=target_location_namespace.course, name=target_location_namespace.name) + new_location = module.location._replace( + tag=target_location_namespace.tag, + org=target_location_namespace.org, + course=target_location_namespace.course, + name=target_location_namespace.name + ) + module.scope_ids = module.scope_ids._replace( + def_id=new_location, + usage_id=new_location + ) # # There is more re-namespacing work we have to do when importing course modules # @@ -401,8 +423,11 @@ def remap_namespace(module, target_location_namespace): new_locs = [] for child in children_locs: child_loc = Location(child) - new_child_loc = child_loc._replace(tag=target_location_namespace.tag, org=target_location_namespace.org, - course=target_location_namespace.course) + new_child_loc = child_loc._replace( + tag=target_location_namespace.tag, + org=target_location_namespace.org, + course=target_location_namespace.course + ) new_locs.append(new_child_loc.url()) @@ -501,10 +526,10 @@ def validate_course_policy(module_store, course_id): warn_cnt = 0 for module in module_store.modules[course_id].itervalues(): if module.location.category == 'course': - if not 'rerandomize' in module._model_data: + if not module._field_data.has(module, 'rerandomize'): warn_cnt += 1 print 'WARN: course policy does not specify value for "rerandomize" whose default is now "never". The behavior of your course may change.' - if not 'showanswer' in module._model_data: + if not module._field_data.has(module, 'showanswer'): warn_cnt += 1 print 'WARN: course policy does not specify value for "showanswer" whose default is now "finished". The behavior of your course may change.' return warn_cnt diff --git a/common/lib/xmodule/xmodule/peer_grading_module.py b/common/lib/xmodule/xmodule/peer_grading_module.py index ae4a8b87de..3a4bbdb9f6 100644 --- a/common/lib/xmodule/xmodule/peer_grading_module.py +++ b/common/lib/xmodule/xmodule/peer_grading_module.py @@ -10,7 +10,7 @@ from .x_module import XModule from xmodule.raw_module import RawDescriptor from xmodule.modulestore.exceptions import ItemNotFoundError from .timeinfo import TimeInfo -from xblock.core import Dict, String, Scope, Boolean, Float +from xblock.fields import Dict, String, Scope, Boolean, Float from xmodule.fields import Date, Timedelta from xmodule.open_ended_grading_classes.peer_grading_service import PeerGradingService, GradingServiceError, MockPeerGradingService @@ -108,9 +108,9 @@ class PeerGradingModule(PeerGradingFields, XModule): log.error("Linked location {0} for peer grading module {1} does not exist".format( self.link_to_location, self.location)) raise - due_date = self.linked_problem.lms.due + due_date = self.linked_problem.due if due_date: - self.lms.due = due_date + self.due = due_date try: self.timeinfo = TimeInfo(self.due, self.graceperiod) @@ -532,8 +532,8 @@ class PeerGradingModule(PeerGradingFields, XModule): except Exception: continue if descriptor: - problem['due'] = descriptor.lms.due - grace_period = descriptor.lms.graceperiod + problem['due'] = descriptor.due + grace_period = descriptor.graceperiod try: problem_timeinfo = TimeInfo(problem['due'], grace_period) except Exception: diff --git a/common/lib/xmodule/xmodule/plugin.py b/common/lib/xmodule/xmodule/plugin.py deleted file mode 100644 index 5cf9c647aa..0000000000 --- a/common/lib/xmodule/xmodule/plugin.py +++ /dev/null @@ -1,64 +0,0 @@ -import pkg_resources -import logging - -log = logging.getLogger(__name__) - -class PluginNotFoundError(Exception): - pass - - -class Plugin(object): - """ - Base class for a system that uses entry_points to load plugins. - - Implementing classes are expected to have the following attributes: - - entry_point: The name of the entry point to load plugins from - """ - - _plugin_cache = None - - @classmethod - def load_class(cls, identifier, default=None): - """ - Loads a single class instance specified by identifier. If identifier - specifies more than a single class, then logs a warning and returns the - first class identified. - - If default is not None, will return default if no entry_point matching - identifier is found. Otherwise, will raise a ModuleMissingError - """ - if cls._plugin_cache is None: - cls._plugin_cache = {} - - if identifier not in cls._plugin_cache: - identifier = identifier.lower() - classes = list(pkg_resources.iter_entry_points( - cls.entry_point, name=identifier)) - - if len(classes) > 1: - log.warning("Found multiple classes for {entry_point} with " - "identifier {id}: {classes}. " - "Returning the first one.".format( - entry_point=cls.entry_point, - id=identifier, - classes=", ".join( - class_.module_name for class_ in classes))) - - if len(classes) == 0: - if default is not None: - return default - raise PluginNotFoundError(identifier) - - cls._plugin_cache[identifier] = classes[0].load() - return cls._plugin_cache[identifier] - - @classmethod - def load_classes(cls): - """ - Returns a list of containing the identifiers and their corresponding classes for all - of the available instances of this plugin - """ - return [(class_.name, class_.load()) - for class_ - in pkg_resources.iter_entry_points(cls.entry_point)] diff --git a/common/lib/xmodule/xmodule/poll_module.py b/common/lib/xmodule/xmodule/poll_module.py index 8e7407752a..86852cd698 100644 --- a/common/lib/xmodule/xmodule/poll_module.py +++ b/common/lib/xmodule/xmodule/poll_module.py @@ -19,7 +19,7 @@ from xmodule.x_module import XModule from xmodule.stringify import stringify_children from xmodule.mako_module import MakoModuleDescriptor from xmodule.xml_module import XmlDescriptor -from xblock.core import Scope, String, Dict, Boolean, List +from xblock.fields import Scope, String, Dict, Boolean, List log = logging.getLogger(__name__) @@ -30,7 +30,7 @@ class PollFields(object): voted = Boolean(help="Whether this student has voted on the poll", scope=Scope.user_state, default=False) poll_answer = String(help="Student answer", scope=Scope.user_state, default='') - poll_answers = Dict(help="All possible answers for the poll fro other students", scope=Scope.content) + poll_answers = Dict(help="All possible answers for the poll fro other students", scope=Scope.user_state_summary) answers = List(help="Poll answers from xml", scope=Scope.content, default=[]) question = String(help="Poll question", scope=Scope.content, default='') diff --git a/common/lib/xmodule/xmodule/randomize_module.py b/common/lib/xmodule/xmodule/randomize_module.py index 04b9a6e215..38535d38bb 100644 --- a/common/lib/xmodule/xmodule/randomize_module.py +++ b/common/lib/xmodule/xmodule/randomize_module.py @@ -6,7 +6,7 @@ from xmodule.seq_module import SequenceDescriptor from lxml import etree -from xblock.core import Scope, Integer +from xblock.fields import Scope, Integer log = logging.getLogger('mitx.' + __name__) diff --git a/common/lib/xmodule/xmodule/raw_module.py b/common/lib/xmodule/xmodule/raw_module.py index 4c6ddb5b51..42c20344b6 100644 --- a/common/lib/xmodule/xmodule/raw_module.py +++ b/common/lib/xmodule/xmodule/raw_module.py @@ -3,7 +3,7 @@ from xmodule.editing_module import XMLEditingDescriptor from xmodule.xml_module import XmlDescriptor import logging import sys -from xblock.core import String, Scope +from xblock.fields import String, Scope from exceptions import SerializationError log = logging.getLogger(__name__) diff --git a/common/lib/xmodule/xmodule/seq_module.py b/common/lib/xmodule/xmodule/seq_module.py index 580475e1ae..ebd49d75cf 100644 --- a/common/lib/xmodule/xmodule/seq_module.py +++ b/common/lib/xmodule/xmodule/seq_module.py @@ -8,7 +8,7 @@ from xmodule.xml_module import XmlDescriptor from xmodule.x_module import XModule from xmodule.progress import Progress from xmodule.exceptions import NotFoundError -from xblock.core import Integer, Scope +from xblock.fields import Integer, Scope from pkg_resources import resource_string log = logging.getLogger(__name__) @@ -40,8 +40,8 @@ class SequenceModule(SequenceFields, XModule): XModule.__init__(self, *args, **kwargs) # if position is specified in system, then use that instead - if self.system.get('position'): - self.position = int(self.system.get('position')) + if getattr(self.system, 'position', None) is not None: + self.position = int(self.system.position) self.rendered = False diff --git a/common/lib/xmodule/xmodule/tests/__init__.py b/common/lib/xmodule/xmodule/tests/__init__.py index df197edb87..fefa668a56 100644 --- a/common/lib/xmodule/xmodule/tests/__init__.py +++ b/common/lib/xmodule/xmodule/tests/__init__.py @@ -18,7 +18,10 @@ from mock import Mock from path import path import calc -from xmodule.x_module import ModuleSystem, XModuleDescriptor +from xblock.field_data import DictFieldData +from xmodule.x_module import ModuleSystem, XModuleDescriptor, DescriptorSystem +from xmodule.modulestore.inheritance import InheritanceMixin +from xmodule.mako_module import MakoDescriptorSystem # Location of common test DATA directory @@ -61,9 +64,22 @@ def get_test_system(): debug=True, xqueue={'interface': None, 'callback_url': '/', 'default_queuename': 'testqueue', 'waittime': 10, 'construct_callback' : Mock(side_effect="/")}, node_path=os.environ.get("NODE_PATH", "/usr/local/lib/node_modules"), - xblock_model_data=lambda descriptor: descriptor._model_data, + xblock_field_data=lambda descriptor: descriptor._field_data, anonymous_student_id='student', - open_ended_grading_interface= open_ended_grading_interface + open_ended_grading_interface=open_ended_grading_interface + ) + + +def get_test_descriptor_system(): + """ + Construct a test DescriptorSystem instance. + """ + return MakoDescriptorSystem( + load_item=Mock(), + resources_fs=Mock(), + error_tracker=Mock(), + render_template=lambda template, context: repr(context), + mixins=(InheritanceMixin,), ) @@ -89,7 +105,7 @@ class PostData(object): class LogicTest(unittest.TestCase): """Base class for testing xmodule logic.""" descriptor_class = None - raw_model_data = {} + raw_field_data = {} def setUp(self): class EmptyClass: @@ -102,7 +118,8 @@ class LogicTest(unittest.TestCase): self.xmodule_class = self.descriptor_class.module_class self.xmodule = self.xmodule_class( - self.system, self.descriptor, self.raw_model_data) + self.descriptor, self.system, DictFieldData(self.raw_field_data), Mock() + ) def ajax_request(self, dispatch, data): """Call Xmodule.handle_ajax.""" diff --git a/common/lib/xmodule/xmodule/tests/test_annotatable_module.py b/common/lib/xmodule/xmodule/tests/test_annotatable_module.py index 6993841ab2..2b454ff45b 100644 --- a/common/lib/xmodule/xmodule/tests/test_annotatable_module.py +++ b/common/lib/xmodule/xmodule/tests/test_annotatable_module.py @@ -5,6 +5,8 @@ import unittest from lxml import etree from mock import Mock +from xblock.field_data import DictFieldData +from xblock.fields import ScopeIds from xmodule.annotatable_module import AnnotatableModule from xmodule.modulestore import Location @@ -29,10 +31,15 @@ class AnnotatableModuleTestCase(unittest.TestCase): ''' descriptor = Mock() - module_data = {'data': sample_xml, 'location': location} + field_data = DictFieldData({'data': sample_xml}) def setUp(self): - self.annotatable = AnnotatableModule(get_test_system(), self.descriptor, self.module_data) + self.annotatable = AnnotatableModule( + self.descriptor, + get_test_system(), + self.field_data, + ScopeIds(None, None, None, None) + ) def test_annotation_data_attr(self): el = etree.fromstring('test') diff --git a/common/lib/xmodule/xmodule/tests/test_capa_module.py b/common/lib/xmodule/xmodule/tests/test_capa_module.py index 92d30fac8c..c47cfc2ca8 100644 --- a/common/lib/xmodule/xmodule/tests/test_capa_module.py +++ b/common/lib/xmodule/xmodule/tests/test_capa_module.py @@ -18,6 +18,8 @@ from capa.responsetypes import (StudentInputError, LoncapaProblemError, ResponseError) from xmodule.capa_module import CapaModule, ComplexEncoder from xmodule.modulestore import Location +from xblock.field_data import DictFieldData +from xblock.fields import ScopeIds from django.http import QueryDict @@ -95,34 +97,39 @@ class CapaFactory(object): """ location = Location(["i4x", "edX", "capa_test", "problem", "SampleProblem{0}".format(CapaFactory.next_num())]) - model_data = {'data': CapaFactory.sample_problem_xml, 'location': location} + field_data = {'data': CapaFactory.sample_problem_xml} if graceperiod is not None: - model_data['graceperiod'] = graceperiod + field_data['graceperiod'] = graceperiod if due is not None: - model_data['due'] = due + field_data['due'] = due if max_attempts is not None: - model_data['max_attempts'] = max_attempts + field_data['max_attempts'] = max_attempts if showanswer is not None: - model_data['showanswer'] = showanswer + field_data['showanswer'] = showanswer if force_save_button is not None: - model_data['force_save_button'] = force_save_button + field_data['force_save_button'] = force_save_button if rerandomize is not None: - model_data['rerandomize'] = rerandomize + field_data['rerandomize'] = rerandomize if done is not None: - model_data['done'] = done + field_data['done'] = done descriptor = Mock(weight="1") if problem_state is not None: - model_data.update(problem_state) + field_data.update(problem_state) if attempts is not None: # converting to int here because I keep putting "0" and "1" in the tests # since everything else is a string. - model_data['attempts'] = int(attempts) + field_data['attempts'] = int(attempts) system = get_test_system() system.render_template = Mock(return_value="
      Test Template HTML
      ") - module = CapaModule(system, descriptor, model_data) + module = CapaModule( + descriptor, + system, + DictFieldData(field_data), + ScopeIds(None, None, location, location), + ) if correct: # TODO: probably better to actually set the internal state properly, but... 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 4eadca110c..1b3fd36f45 100644 --- a/common/lib/xmodule/xmodule/tests/test_combined_open_ended.py +++ b/common/lib/xmodule/xmodule/tests/test_combined_open_ended.py @@ -28,6 +28,9 @@ from xmodule.tests.test_util_open_ended import ( MOCK_INSTANCE_STATE, TEST_STATE_SA, TEST_STATE_AI, TEST_STATE_AI2, TEST_STATE_AI2_INVALID, TEST_STATE_SINGLE, TEST_STATE_PE_SINGLE ) + +from xblock.field_data import DictFieldData +from xblock.fields import ScopeIds import capa.xqueue_interface as xqueue_interface @@ -418,13 +421,13 @@ class CombinedOpenEndedModuleTest(unittest.TestCase): test_system = get_test_system() test_system.open_ended_grading_interface = None combinedoe_container = CombinedOpenEndedModule( - test_system, - descriptor, - model_data={ + descriptor=descriptor, + runtime=test_system, + field_data=DictFieldData({ 'data': full_definition, 'weight': '1', - 'location': location - } + }), + scope_ids=ScopeIds(None, None, None, None), ) def setUp(self): diff --git a/common/lib/xmodule/xmodule/tests/test_conditional.py b/common/lib/xmodule/xmodule/tests/test_conditional.py index 95472a62ed..040af7a230 100644 --- a/common/lib/xmodule/xmodule/tests/test_conditional.py +++ b/common/lib/xmodule/xmodule/tests/test_conditional.py @@ -6,6 +6,8 @@ import unittest from fs.memoryfs import MemoryFS from mock import Mock, patch +from xblock.field_data import DictFieldData +from xblock.fields import ScopeIds from xmodule.error_module import NonStaffErrorDescriptor from xmodule.modulestore import Location from xmodule.modulestore.xml import ImportSystem, XMLModuleStore @@ -87,8 +89,13 @@ class ConditionalFactory(object): # construct conditional module: cond_location = Location(["i4x", "edX", "conditional_test", "conditional", "SampleConditional"]) - model_data = {'data': '', 'location': cond_location} - cond_module = ConditionalModule(system, cond_descriptor, model_data) + field_data = DictFieldData({'data': '', 'location': cond_location}) + cond_module = ConditionalModule( + cond_descriptor, + system, + field_data, + ScopeIds(None, None, cond_location, cond_location) + ) # return dict: return {'cond_module': cond_module, diff --git a/common/lib/xmodule/xmodule/tests/test_course_module.py b/common/lib/xmodule/xmodule/tests/test_course_module.py index 48690a8b25..bf83fbd17d 100644 --- a/common/lib/xmodule/xmodule/tests/test_course_module.py +++ b/common/lib/xmodule/xmodule/tests/test_course_module.py @@ -29,12 +29,12 @@ class DummySystem(ImportSystem): parent_tracker = Mock() super(DummySystem, self).__init__( - xmlstore, - course_id, - course_dir, - policy, - error_tracker, - parent_tracker, + xmlstore=xmlstore, + course_id=course_id, + course_dir=course_dir, + policy=policy, + error_tracker=error_tracker, + parent_tracker=parent_tracker, load_error_modules=load_error_modules, ) diff --git a/common/lib/xmodule/xmodule/tests/test_crowdsource_hinter.py b/common/lib/xmodule/xmodule/tests/test_crowdsource_hinter.py index 8347b71076..0369fffe8e 100644 --- a/common/lib/xmodule/xmodule/tests/test_crowdsource_hinter.py +++ b/common/lib/xmodule/xmodule/tests/test_crowdsource_hinter.py @@ -8,6 +8,7 @@ import copy from xmodule.crowdsource_hinter import CrowdsourceHinterModule from xmodule.vertical_module import VerticalModule, VerticalDescriptor +from xblock.field_data import DictFieldData from . import get_test_system @@ -60,12 +61,12 @@ class CHModuleFactory(object): """ A factory method for making CHM's """ - model_data = {'data': CHModuleFactory.sample_problem_xml} + field_data = {'data': CHModuleFactory.sample_problem_xml} if hints is not None: - model_data['hints'] = hints + field_data['hints'] = hints else: - model_data['hints'] = { + field_data['hints'] = { '24.0': {'0': ['Best hint', 40], '3': ['Another hint', 30], '4': ['A third hint', 20], @@ -74,31 +75,31 @@ class CHModuleFactory(object): } if mod_queue is not None: - model_data['mod_queue'] = mod_queue + field_data['mod_queue'] = mod_queue else: - model_data['mod_queue'] = { + field_data['mod_queue'] = { '24.0': {'2': ['A non-approved hint']}, '26.0': {'5': ['Another non-approved hint']} } if previous_answers is not None: - model_data['previous_answers'] = previous_answers + field_data['previous_answers'] = previous_answers else: - model_data['previous_answers'] = [ + field_data['previous_answers'] = [ ['24.0', [0, 3, 4]], ['29.0', []] ] if user_submissions is not None: - model_data['user_submissions'] = user_submissions + field_data['user_submissions'] = user_submissions else: - model_data['user_submissions'] = ['24.0', '29.0'] + field_data['user_submissions'] = ['24.0', '29.0'] if user_voted is not None: - model_data['user_voted'] = user_voted + field_data['user_voted'] = user_voted if moderate is not None: - model_data['moderate'] = moderate + field_data['moderate'] = moderate descriptor = Mock(weight='1') # Make the descriptor have a capa problem child. @@ -138,8 +139,7 @@ class CHModuleFactory(object): if descriptor.name == 'capa': return capa_module system.get_module = fake_get_module - - module = CrowdsourceHinterModule(system, descriptor, model_data) + module = CrowdsourceHinterModule(descriptor, system, DictFieldData(field_data), Mock()) return module @@ -196,10 +196,10 @@ class VerticalWithModulesFactory(object): @staticmethod def create(): """Make a vertical.""" - model_data = {'data': VerticalWithModulesFactory.sample_problem_xml} + field_data = {'data': VerticalWithModulesFactory.sample_problem_xml} system = get_test_system() descriptor = VerticalDescriptor.from_xml(VerticalWithModulesFactory.sample_problem_xml, system) - module = VerticalModule(system, descriptor, model_data) + module = VerticalModule(system, descriptor, field_data) return module diff --git a/common/lib/xmodule/xmodule/tests/test_editing_module.py b/common/lib/xmodule/xmodule/tests/test_editing_module.py index 838a4f9ada..2e59c545a4 100644 --- a/common/lib/xmodule/xmodule/tests/test_editing_module.py +++ b/common/lib/xmodule/xmodule/tests/test_editing_module.py @@ -6,8 +6,10 @@ import logging from mock import Mock from pkg_resources import resource_string from xmodule.editing_module import TabsEditingDescriptor +from xblock.field_data import DictFieldData +from xblock.fields import ScopeIds -from .import get_test_system +from xmodule.tests import get_test_descriptor_system log = logging.getLogger(__name__) @@ -17,7 +19,7 @@ class TabsEditingDescriptorTestCase(unittest.TestCase): def setUp(self): super(TabsEditingDescriptorTestCase, self).setUp() - system = get_test_system() + system = get_test_descriptor_system() system.render_template = Mock(return_value="
      Test Template HTML
      ") self.tabs = [ { @@ -42,9 +44,11 @@ class TabsEditingDescriptorTestCase(unittest.TestCase): ] TabsEditingDescriptor.tabs = self.tabs - self.descriptor = TabsEditingDescriptor( - runtime=system, - model_data={}) + self.descriptor = system.construct_xblock_from_class( + TabsEditingDescriptor, + field_data=DictFieldData({}), + scope_ids=ScopeIds(None, None, None, None), + ) def test_get_css(self): """test get_css""" diff --git a/common/lib/xmodule/xmodule/tests/test_error_module.py b/common/lib/xmodule/xmodule/tests/test_error_module.py index f91bf7cb37..186754e16c 100644 --- a/common/lib/xmodule/xmodule/tests/test_error_module.py +++ b/common/lib/xmodule/xmodule/tests/test_error_module.py @@ -39,7 +39,7 @@ class TestErrorModule(unittest.TestCase, SetupTestErrorModules): descriptor = MagicMock([XModuleDescriptor], system=self.system, location=self.location, - _model_data=self.valid_xml) + _field_data=self.valid_xml) error_descriptor = error_module.ErrorDescriptor.from_descriptor( descriptor, self.error_msg) @@ -74,7 +74,7 @@ class TestNonStaffErrorModule(unittest.TestCase, SetupTestErrorModules): descriptor = MagicMock([XModuleDescriptor], system=self.system, location=self.location, - _model_data=self.valid_xml) + _field_data=self.valid_xml) error_descriptor = error_module.NonStaffErrorDescriptor.from_descriptor( descriptor, self.error_msg) diff --git a/common/lib/xmodule/xmodule/tests/test_export.py b/common/lib/xmodule/xmodule/tests/test_export.py index b0c1617580..1ee059f9fe 100644 --- a/common/lib/xmodule/xmodule/tests/test_export.py +++ b/common/lib/xmodule/xmodule/tests/test_export.py @@ -24,7 +24,8 @@ def strip_filenames(descriptor): Recursively strips 'filename' from all children's definitions. """ print("strip filename from {desc}".format(desc=descriptor.location.url())) - descriptor._model_data.pop('filename', None) + if descriptor._field_data.has(descriptor, 'filename'): + descriptor._field_data.delete(descriptor, 'filename') if hasattr(descriptor, 'xml_attributes'): if 'filename' in descriptor.xml_attributes: diff --git a/common/lib/xmodule/xmodule/tests/test_html_module.py b/common/lib/xmodule/xmodule/tests/test_html_module.py index 4fe0242378..2f2e9114c6 100644 --- a/common/lib/xmodule/xmodule/tests/test_html_module.py +++ b/common/lib/xmodule/xmodule/tests/test_html_module.py @@ -2,6 +2,7 @@ import unittest from mock import Mock +from xblock.field_data import DictFieldData from xmodule.html_module import HtmlModule from . import get_test_system @@ -11,9 +12,9 @@ class HtmlModuleSubstitutionTestCase(unittest.TestCase): def test_substitution_works(self): sample_xml = '''%%USER_ID%%''' - module_data = {'data': sample_xml} + field_data = DictFieldData({'data': sample_xml}) module_system = get_test_system() - module = HtmlModule(module_system, self.descriptor, module_data) + module = HtmlModule(self.descriptor, module_system, field_data, Mock()) self.assertEqual(module.get_html(), str(module_system.anonymous_student_id)) @@ -23,16 +24,17 @@ class HtmlModuleSubstitutionTestCase(unittest.TestCase):

      Hi USER_ID!11!

      ''' - module_data = {'data': sample_xml} - module = HtmlModule(get_test_system(), self.descriptor, module_data) + field_data = DictFieldData({'data': sample_xml}) + module_system = get_test_system() + module = HtmlModule(self.descriptor, module_system, field_data, Mock()) self.assertEqual(module.get_html(), sample_xml) def test_substitution_without_anonymous_student_id(self): sample_xml = '''%%USER_ID%%''' - module_data = {'data': sample_xml} + field_data = DictFieldData({'data': sample_xml}) module_system = get_test_system() module_system.anonymous_student_id = None - module = HtmlModule(module_system, self.descriptor, module_data) + module = HtmlModule(self.descriptor, module_system, field_data, Mock()) self.assertEqual(module.get_html(), sample_xml) diff --git a/common/lib/xmodule/xmodule/tests/test_import.py b/common/lib/xmodule/xmodule/tests/test_import.py index b6758dc917..8f24112bc6 100644 --- a/common/lib/xmodule/xmodule/tests/test_import.py +++ b/common/lib/xmodule/xmodule/tests/test_import.py @@ -15,6 +15,7 @@ from xmodule.modulestore.xml import ImportSystem, XMLModuleStore from xmodule.modulestore.inheritance import compute_inherited_metadata from xmodule.fields import Date from xmodule.tests import DATA_DIR +from xmodule.modulestore.inheritance import InheritanceMixin ORG = 'test_org' @@ -34,13 +35,14 @@ class DummySystem(ImportSystem): parent_tracker = Mock() super(DummySystem, self).__init__( - xmlstore, - course_id, - course_dir, - policy, - error_tracker, - parent_tracker, + xmlstore=xmlstore, + course_id=course_id, + course_dir=course_dir, + policy=policy, + error_tracker=error_tracker, + parent_tracker=parent_tracker, load_error_modules=load_error_modules, + mixins=(InheritanceMixin,) ) def render_template(self, _template, _context): @@ -58,7 +60,7 @@ class BaseCourseTestCase(unittest.TestCase): """Get a test course by directory name. If there's more than one, error.""" print("Importing {0}".format(name)) - modulestore = XMLModuleStore(DATA_DIR, course_dirs=[name]) + modulestore = XMLModuleStore(DATA_DIR, course_dirs=[name], xblock_mixins=(InheritanceMixin,)) courses = modulestore.get_courses() self.assertEquals(len(courses), 1) return courses[0] @@ -76,7 +78,7 @@ class ImportTestCase(BaseCourseTestCase): descriptor = system.process_xml(bad_xml) - self.assertEqual(descriptor.__class__.__name__, 'ErrorDescriptor') + self.assertEqual(descriptor.__class__.__name__, 'ErrorDescriptorWithMixins') def test_unique_url_names(self): '''Check that each error gets its very own url_name''' @@ -102,7 +104,7 @@ class ImportTestCase(BaseCourseTestCase): re_import_descriptor = system.process_xml(tag_xml) self.assertEqual(re_import_descriptor.__class__.__name__, - 'ErrorDescriptor') + 'ErrorDescriptorWithMixins') self.assertEqual(descriptor.contents, re_import_descriptor.contents) @@ -150,15 +152,17 @@ class ImportTestCase(BaseCourseTestCase): compute_inherited_metadata(descriptor) # pylint: disable=W0212 - print(descriptor, descriptor._model_data) - self.assertEqual(descriptor.lms.due, ImportTestCase.date.from_json(v)) + print(descriptor, descriptor._field_data) + self.assertEqual(descriptor.due, ImportTestCase.date.from_json(v)) # Check that the child inherits due correctly child = descriptor.get_children()[0] - self.assertEqual(child.lms.due, ImportTestCase.date.from_json(v)) - self.assertEqual(child._inheritable_metadata, child._inherited_metadata) - self.assertEqual(1, len(child._inherited_metadata)) - self.assertEqual(v, child._inherited_metadata['due']) + self.assertEqual(child.due, ImportTestCase.date.from_json(v)) + # need to convert v to canonical json b4 comparing + self.assertEqual( + ImportTestCase.date.to_json(ImportTestCase.date.from_json(v)), + child.xblock_kvs.inherited_settings['due'] + ) # Now export and check things resource_fs = MemoryFS() @@ -208,15 +212,13 @@ class ImportTestCase(BaseCourseTestCase): descriptor = system.process_xml(start_xml) compute_inherited_metadata(descriptor) - self.assertEqual(descriptor.lms.due, None) + self.assertEqual(descriptor.due, None) # Check that the child does not inherit a value for due child = descriptor.get_children()[0] - self.assertEqual(child.lms.due, None) - # pylint: disable=W0212 - self.assertEqual(child._inheritable_metadata, child._inherited_metadata) + self.assertEqual(child.due, None) self.assertLessEqual( - child.lms.start, + child.start, datetime.datetime.now(UTC()) ) @@ -238,14 +240,16 @@ class ImportTestCase(BaseCourseTestCase): descriptor = system.process_xml(start_xml) child = descriptor.get_children()[0] # pylint: disable=W0212 - child._model_data['due'] = child_due + child._field_data.set(child, 'due', child_due) compute_inherited_metadata(descriptor) - self.assertEqual(descriptor.lms.due, ImportTestCase.date.from_json(course_due)) - self.assertEqual(child.lms.due, ImportTestCase.date.from_json(child_due)) + self.assertEqual(descriptor.due, ImportTestCase.date.from_json(course_due)) + self.assertEqual(child.due, ImportTestCase.date.from_json(child_due)) # Test inherited metadata. Due does not appear here (because explicitly set on child). - self.assertEqual(1, len(child._inheritable_metadata)) - self.assertEqual(course_due, child._inheritable_metadata['due']) + self.assertEqual( + ImportTestCase.date.to_json(ImportTestCase.date.from_json(course_due)), + child.xblock_kvs.inherited_settings['due'] + ) def test_is_pointer_tag(self): """ @@ -280,14 +284,14 @@ class ImportTestCase(BaseCourseTestCase): print("Starting import") course = self.get_course('toy') - def check_for_key(key, node): + def check_for_key(key, node, value): "recursive check for presence of key" print("Checking {0}".format(node.location.url())) - self.assertTrue(key in node._model_data) + self.assertEqual(getattr(node, key), value) for c in node.get_children(): - check_for_key(key, c) + check_for_key(key, c, value) - check_for_key('graceperiod', course) + check_for_key('graceperiod', course, course.graceperiod) def test_policy_loading(self): """Make sure that when two courses share content with the same @@ -310,7 +314,7 @@ class ImportTestCase(BaseCourseTestCase): # Also check that keys from policy are run through the # appropriate attribute maps -- 'graded' should be True, not 'true' - self.assertEqual(toy.lms.graded, True) + self.assertEqual(toy.graded, True) def test_definition_loading(self): """When two courses share the same org and course name and diff --git a/common/lib/xmodule/xmodule/tests/test_poll.py b/common/lib/xmodule/xmodule/tests/test_poll.py index bbd0c3bb12..ea9fba7948 100644 --- a/common/lib/xmodule/xmodule/tests/test_poll.py +++ b/common/lib/xmodule/xmodule/tests/test_poll.py @@ -7,7 +7,7 @@ from . import LogicTest class PollModuleTest(LogicTest): """Logic tests for Poll Xmodule.""" descriptor_class = PollDescriptor - raw_model_data = { + raw_field_data = { 'poll_answers': {'Yes': 1, 'Dont_know': 0, 'No': 0}, 'voted': False, 'poll_answer': '' diff --git a/common/lib/xmodule/xmodule/tests/test_progress.py b/common/lib/xmodule/xmodule/tests/test_progress.py index e7af7a0c09..053768a007 100644 --- a/common/lib/xmodule/xmodule/tests/test_progress.py +++ b/common/lib/xmodule/xmodule/tests/test_progress.py @@ -1,6 +1,9 @@ """Module progress tests""" import unittest +from mock import Mock + +from xblock.field_data import DictFieldData from xmodule.progress import Progress from xmodule import x_module @@ -134,6 +137,6 @@ class ModuleProgressTest(unittest.TestCase): ''' def test_xmodule_default(self): '''Make sure default get_progress exists, returns None''' - xm = x_module.XModule(get_test_system(), None, {'location': 'a://b/c/d/e'}) + xm = x_module.XModule(None, get_test_system(), DictFieldData({'location': 'a://b/c/d/e'}), Mock()) p = xm.get_progress() self.assertEqual(p, None) diff --git a/common/lib/xmodule/xmodule/tests/test_video.py b/common/lib/xmodule/xmodule/tests/test_video.py index a6a7d86510..708c874fc3 100644 --- a/common/lib/xmodule/xmodule/tests/test_video.py +++ b/common/lib/xmodule/xmodule/tests/test_video.py @@ -14,21 +14,25 @@ the course, section, subsection, unit, etc. """ import unittest +from mock import Mock + from . import LogicTest from lxml import etree -from .import get_test_system from xmodule.modulestore import Location from xmodule.video_module import VideoDescriptor, _create_youtube_string from .test_import import DummySystem +from xblock.field_data import DictFieldData +from xblock.fields import ScopeIds from textwrap import dedent +from xmodule.tests import get_test_descriptor_system class VideoModuleTest(LogicTest): """Logic tests for Video Xmodule.""" descriptor_class = VideoDescriptor - raw_model_data = { + raw_field_data = { 'data': '