From b6bd293266bd0c5e28fbca52c5198c59f2589edf Mon Sep 17 00:00:00 2001 From: Ned Batchelder Date: Thu, 3 Oct 2013 15:46:51 -0400 Subject: [PATCH 1/6] Clean up some pre-existing formatting. --- .../lib/xmodule/xmodule/conditional_module.py | 3 +- .../xmodule/xmodule/modulestore/__init__.py | 4 +-- .../xmodule/modulestore/inheritance.py | 31 ++++++++++++------- .../xmodule/modulestore/split_mongo/split.py | 6 ++-- common/lib/xmodule/xmodule/modulestore/xml.py | 22 ++++++++----- .../xmodule/xmodule/peer_grading_module.py | 4 +-- .../lib/xmodule/xmodule/tests/test_import.py | 9 ++---- common/lib/xmodule/xmodule/xml_module.py | 5 ++- common/test/data/simple/course.xml | 13 ++++---- .../commands/dump_course_structure.py | 2 +- 10 files changed, 54 insertions(+), 45 deletions(-) diff --git a/common/lib/xmodule/xmodule/conditional_module.py b/common/lib/xmodule/xmodule/conditional_module.py index f9b47da7ea..d015136f50 100644 --- a/common/lib/xmodule/xmodule/conditional_module.py +++ b/common/lib/xmodule/xmodule/conditional_module.py @@ -221,8 +221,7 @@ class ConditionalDescriptor(ConditionalFields, SequenceDescriptor): show_tag_list = [] for child in xml_object: if child.tag == 'show': - location = ConditionalDescriptor.parse_sources( - child, system) + location = ConditionalDescriptor.parse_sources(child, system) children.extend(location) show_tag_list.extend(location) else: diff --git a/common/lib/xmodule/xmodule/modulestore/__init__.py b/common/lib/xmodule/xmodule/modulestore/__init__.py index e3386608dc..3029449ace 100644 --- a/common/lib/xmodule/xmodule/modulestore/__init__.py +++ b/common/lib/xmodule/xmodule/modulestore/__init__.py @@ -163,8 +163,8 @@ class Location(_LocationBase): def check_list(list_): def check(val, regexp): if val is not None and regexp.search(val) is not None: - log.debug('invalid characters val="%s", list_="%s"' % (val, list_)) - raise InvalidLocationError("Invalid characters in '%s'." % (val)) + log.debug('invalid characters val=%r, list_=%r', val, list_) + raise InvalidLocationError("Invalid characters in {!r}.".format(val)) list_ = list(list_) for val in list_[:4] + [list_[5]]: diff --git a/common/lib/xmodule/xmodule/modulestore/inheritance.py b/common/lib/xmodule/xmodule/modulestore/inheritance.py index 5839dd6d1c..d23a13af6f 100644 --- a/common/lib/xmodule/xmodule/modulestore/inheritance.py +++ b/common/lib/xmodule/xmodule/modulestore/inheritance.py @@ -7,20 +7,22 @@ from xblock.runtime import KeyValueStore class InheritanceMixin(XBlockMixin): - """Field definitions for inheritable fields""" + """Field definitions for inheritable fields.""" graded = Boolean( help="Whether this module contributes to the final course grade", + scope=Scope.settings, default=False, - scope=Scope.settings ) - start = Date( help="Start time when this module is visible", default=datetime(2030, 1, 1, tzinfo=UTC), scope=Scope.settings ) - due = Date(help="Date that this problem is due by", scope=Scope.settings) + due = Date( + help="Date that this problem is due by", + scope=Scope.settings, + ) extended_due = Date( help="Date that this problem is due by for a particular student. This " "can be set by an instructor, and will override the global due " @@ -29,31 +31,38 @@ class InheritanceMixin(XBlockMixin): default=None, scope=Scope.user_state, ) - giturl = String(help="url root for course data git repository", 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 + scope=Scope.settings, ) showanswer = String( help="When to show the problem answer to the student", scope=Scope.settings, - default="finished" + default="finished", ) rerandomize = String( help="When to rerandomize the problem", + scope=Scope.settings, default="never", - scope=Scope.settings ) days_early_for_beta = Float( help="Number of days early to show content to beta users", + scope=Scope.settings, default=None, - scope=Scope.settings ) - static_asset_path = String(help="Path to use for static assets - overrides Studio c4x://", scope=Scope.settings, default='') + static_asset_path = String( + help="Path to use for static assets - overrides Studio c4x://", + scope=Scope.settings, + default='', + ) text_customization = Dict( help="String customization substitutions for particular locations", - scope=Scope.settings + scope=Scope.settings, ) use_latex_compiler = Boolean( help="Enable LaTeX templates?", diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index 1a92f961ec..04f15727a4 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py @@ -485,7 +485,8 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): } """ course = self._lookup_course(course_locator)['structure'] - return {'original_version': course['original_version'], + return { + 'original_version': course['original_version'], 'previous_version': course['previous_version'], 'edited_by': course['edited_by'], 'edited_on': course['edited_on'] @@ -1328,8 +1329,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): if depth is None or depth > 0: depth = depth - 1 if depth is not None else None for child in block_map[block_id]['fields'].get('children', []): - descendent_map = self.descendants(block_map, child, depth, - descendent_map) + descendent_map = self.descendants(block_map, child, depth, descendent_map) return descendent_map diff --git a/common/lib/xmodule/xmodule/modulestore/xml.py b/common/lib/xmodule/xmodule/modulestore/xml.py index 02df55569a..ec31fc5763 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml.py +++ b/common/lib/xmodule/xmodule/modulestore/xml.py @@ -174,12 +174,17 @@ class ImportSystem(XMLParsingSystem, MakoDescriptorSystem): # Didn't load properly. Fall back on loading as an error # descriptor. This should never error due to formatting. - msg = "Error loading from xml. " + unicode(err)[:200] - log.warning(msg) - # Normally, we don't want lots of exception traces in our logs from common - # content problems. But if you're debugging the xml loading code itself, - # uncomment the next line. - # log.exception(msg) + msg = "Error loading from xml. %s" + log.warning( + msg, + unicode(err)[:200], + # Normally, we don't want lots of exception traces in our logs from common + # content problems. But if you're debugging the xml loading code itself, + # uncomment the next line. + # exc_info=True + ) + + msg = msg % (unicode(err)[:200]) self.error_tracker(msg) err_msg = msg + "\n" + exc_info_to_str(sys.exc_info()) @@ -195,7 +200,7 @@ class ImportSystem(XMLParsingSystem, MakoDescriptorSystem): xmlstore.modules[course_id][descriptor.location] = descriptor - if hasattr(descriptor, 'children'): + if descriptor.has_children: for child in descriptor.get_children(): parent_tracker.add_parent(child.location, descriptor.location) @@ -368,7 +373,8 @@ class XMLModuleStore(ModuleStoreReadBase): String representation - for debugging ''' return '' % ( - self.data_dir, len(self.courses), len(self.modules)) + self.data_dir, len(self.courses), len(self.modules) + ) def load_policy(self, policy_path, tracker): """ diff --git a/common/lib/xmodule/xmodule/peer_grading_module.py b/common/lib/xmodule/xmodule/peer_grading_module.py index 95835a0ba4..ed80db2aae 100644 --- a/common/lib/xmodule/xmodule/peer_grading_module.py +++ b/common/lib/xmodule/xmodule/peer_grading_module.py @@ -660,8 +660,8 @@ class PeerGradingDescriptor(PeerGradingFields, RawDescriptor): metadata_translations = { 'is_graded': 'graded', 'attempts': 'max_attempts', - 'due_data' : 'due' - } + 'due_data': 'due' + } @property def non_editable_metadata_fields(self): diff --git a/common/lib/xmodule/xmodule/tests/test_import.py b/common/lib/xmodule/xmodule/tests/test_import.py index 3a192318a6..11615afd0a 100644 --- a/common/lib/xmodule/xmodule/tests/test_import.py +++ b/common/lib/xmodule/xmodule/tests/test_import.py @@ -119,13 +119,10 @@ class ImportTestCase(BaseCourseTestCase): tag_xml = descriptor.export_to_xml(resource_fs) re_import_descriptor = system.process_xml(tag_xml) - self.assertEqual(re_import_descriptor.__class__.__name__, - 'ErrorDescriptorWithMixins') + self.assertEqual(re_import_descriptor.__class__.__name__, 'ErrorDescriptorWithMixins') - self.assertEqual(descriptor.contents, - re_import_descriptor.contents) - self.assertEqual(descriptor.error_msg, - re_import_descriptor.error_msg) + self.assertEqual(descriptor.contents, re_import_descriptor.contents) + self.assertEqual(descriptor.error_msg, re_import_descriptor.error_msg) def test_fixed_xml_tag(self): """Make sure a tag that's been fixed exports as the original tag type""" diff --git a/common/lib/xmodule/xmodule/xml_module.py b/common/lib/xmodule/xmodule/xml_module.py index 47682b8ea2..e63d56da40 100644 --- a/common/lib/xmodule/xmodule/xml_module.py +++ b/common/lib/xmodule/xmodule/xml_module.py @@ -6,7 +6,7 @@ import sys from lxml import etree from xblock.fields import Dict, Scope, ScopeIds -from xmodule.x_module import (XModuleDescriptor, policy_key) +from xmodule.x_module import XModuleDescriptor, policy_key from xmodule.modulestore import Location from xmodule.modulestore.inheritance import own_metadata, InheritanceKeyValueStore from xmodule.modulestore.xml_exporter import EdxJSONEncoder @@ -153,8 +153,7 @@ class XmlDescriptor(XModuleDescriptor): xml_object: An etree Element """ - raise NotImplementedError( - "%s does not implement definition_from_xml" % cls.__name__) + raise NotImplementedError("%s does not implement definition_from_xml" % cls.__name__) @classmethod def clean_metadata_from_xml(cls, xml_object): diff --git a/common/test/data/simple/course.xml b/common/test/data/simple/course.xml index 529528ca0a..5cd6d54d29 100644 --- a/common/test/data/simple/course.xml +++ b/common/test/data/simple/course.xml @@ -6,7 +6,7 @@ ''' - output = VideoDescriptor.from_xml(xml_data, module_system) + output = VideoDescriptor.from_xml(xml_data, module_system, Mock()) self.assert_attributes_equal(output, { 'youtube_id_0_75': '', 'youtube_id_1_0': 'p2Q6BrNhdh8', @@ -276,7 +277,7 @@ class VideoDescriptorImportTestCase(unittest.TestCase): """ module_system = DummySystem(load_error_modules=True) xml_data = '' - output = VideoDescriptor.from_xml(xml_data, module_system) + output = VideoDescriptor.from_xml(xml_data, module_system, Mock()) self.assert_attributes_equal(output, { 'youtube_id_0_75': '', 'youtube_id_1_0': 'OEoXaMPEzfM', @@ -310,7 +311,7 @@ class VideoDescriptorImportTestCase(unittest.TestCase): youtube_id_1_0=""OEoXaMPEzf10"" /> ''' - output = VideoDescriptor.from_xml(xml_data, module_system) + output = VideoDescriptor.from_xml(xml_data, module_system, Mock()) self.assert_attributes_equal(output, { 'youtube_id_0_75': 'OEoXaMPEzf65', 'youtube_id_1_0': 'OEoXaMPEzf10', @@ -332,7 +333,7 @@ class VideoDescriptorImportTestCase(unittest.TestCase): youtube="1.0:"p2Q6BrNhdh8",1.25:"1EeWXzPdhSA""> ''' - output = VideoDescriptor.from_xml(xml_data, module_system) + output = VideoDescriptor.from_xml(xml_data, module_system, Mock()) self.assert_attributes_equal(output, { 'youtube_id_0_75': '', 'youtube_id_1_0': 'p2Q6BrNhdh8', @@ -362,7 +363,7 @@ class VideoDescriptorImportTestCase(unittest.TestCase): """ - output = VideoDescriptor.from_xml(xml_data, module_system) + output = VideoDescriptor.from_xml(xml_data, module_system, Mock()) self.assert_attributes_equal(output, { 'youtube_id_0_75': 'izygArpw-Qo', 'youtube_id_1_0': 'p2Q6BrNhdh8', @@ -391,7 +392,7 @@ class VideoDescriptorImportTestCase(unittest.TestCase): """ - video = VideoDescriptor.from_xml(xml_data, module_system) + video = VideoDescriptor.from_xml(xml_data, module_system, Mock()) self.assert_attributes_equal(video, { 'youtube_id_0_75': 'izygArpw-Qo', 'youtube_id_1_0': 'p2Q6BrNhdh8', @@ -420,7 +421,7 @@ class VideoDescriptorImportTestCase(unittest.TestCase): """ - video = VideoDescriptor.from_xml(xml_data, module_system) + video = VideoDescriptor.from_xml(xml_data, module_system, Mock()) self.assert_attributes_equal(video, { 'youtube_id_0_75': 'izygArpw-Qo', 'youtube_id_1_0': 'p2Q6BrNhdh8', diff --git a/common/lib/xmodule/xmodule/tests/test_xblock_wrappers.py b/common/lib/xmodule/xmodule/tests/test_xblock_wrappers.py index c8ac5fcdd3..a5247b97de 100644 --- a/common/lib/xmodule/xmodule/tests/test_xblock_wrappers.py +++ b/common/lib/xmodule/xmodule/tests/test_xblock_wrappers.py @@ -13,6 +13,8 @@ from mock import Mock from xblock.field_data import DictFieldData from xblock.fields import ScopeIds +from xmodule.modulestore import Location + from xmodule.x_module import ModuleSystem, XModule, XModuleDescriptor from xmodule.mako_module import MakoDescriptorSystem from xmodule.annotatable_module import AnnotatableDescriptor @@ -75,7 +77,7 @@ class TestXBlockWrapper(object): return get_test_system() def leaf_descriptor(self, descriptor_cls): - location = 'i4x://org/course/category/name' + location = Location('i4x://org/course/category/name') runtime = get_test_descriptor_system() return runtime.construct_xblock_from_class( descriptor_cls, @@ -100,7 +102,7 @@ class TestXBlockWrapper(object): def container_descriptor(self, descriptor_cls, depth): """Return an instance of `descriptor_cls` with `depth` levels of children""" - location = 'i4x://org/course/category/name' + location = Location('i4x://org/course/category/name') runtime = get_test_descriptor_system() if depth == 0: diff --git a/common/lib/xmodule/xmodule/tests/test_xml_module.py b/common/lib/xmodule/xmodule/tests/test_xml_module.py index ba48da5819..62ebf4dce0 100644 --- a/common/lib/xmodule/xmodule/tests/test_xml_module.py +++ b/common/lib/xmodule/xmodule/tests/test_xml_module.py @@ -70,13 +70,13 @@ class InheritingFieldDataTest(unittest.TestCase): def setUp(self): self.system = get_test_descriptor_system() self.all_blocks = {} - self.system.load_item = self.all_blocks.get + self.system.get_block = self.all_blocks.get self.field_data = InheritingFieldData( inheritable_names=['inherited'], kvs=DictKeyValueStore({}), ) - def get_block(self, usage_id=None): + def get_a_block(self, usage_id=None): """Construct an XBlock for testing with.""" scope_ids = Mock() if usage_id is None: @@ -92,13 +92,13 @@ class InheritingFieldDataTest(unittest.TestCase): def test_default_value(self): # Blocks with nothing set with return the fields' defaults. - block = self.get_block() + block = self.get_a_block() self.assertEqual(block.inherited, "the default") self.assertEqual(block.not_inherited, "nothing") def test_set_value(self): # If you set a value, that's what you get back. - block = self.get_block() + block = self.get_a_block() block.inherited = "Changed!" block.not_inherited = "New Value!" self.assertEqual(block.inherited, "Changed!") @@ -106,34 +106,34 @@ class InheritingFieldDataTest(unittest.TestCase): def test_inherited(self): # A child with get a value inherited from the parent. - parent = self.get_block(usage_id="parent") + parent = self.get_a_block(usage_id="parent") parent.inherited = "Changed!" self.assertEqual(parent.inherited, "Changed!") - child = self.get_block(usage_id="child") + child = self.get_a_block(usage_id="child") child.parent = "parent" self.assertEqual(child.inherited, "Changed!") def test_inherited_across_generations(self): # A child with get a value inherited from a great-grandparent. - parent = self.get_block(usage_id="parent") + parent = self.get_a_block(usage_id="parent") parent.inherited = "Changed!" self.assertEqual(parent.inherited, "Changed!") parent_id = "parent" for child_num in range(10): usage_id = "child_{}".format(child_num) - child = self.get_block(usage_id=usage_id) + child = self.get_a_block(usage_id=usage_id) child.parent = "parent" self.assertEqual(child.inherited, "Changed!") parent_id = usage_id def test_not_inherited(self): # Fields not in the inherited_names list won't be inherited. - parent = self.get_block(usage_id="parent") + parent = self.get_a_block(usage_id="parent") parent.not_inherited = "Changed!" self.assertEqual(parent.not_inherited, "Changed!") - child = self.get_block(usage_id="child") + child = self.get_a_block(usage_id="child") child.parent = "parent" self.assertEqual(child.not_inherited, "nothing") diff --git a/common/lib/xmodule/xmodule/tests/xml/__init__.py b/common/lib/xmodule/xmodule/tests/xml/__init__.py index 624a61d13f..9131459d08 100644 --- a/common/lib/xmodule/xmodule/tests/xml/__init__.py +++ b/common/lib/xmodule/xmodule/tests/xml/__init__.py @@ -4,9 +4,12 @@ Xml parsing tests for XModules import pprint from mock import Mock -from xmodule.x_module import XMLParsingSystem +from xmodule.x_module import XMLParsingSystem, policy_key from xmodule.mako_module import MakoDescriptorSystem -from xmodule.modulestore.xml import create_block_from_xml +from xmodule.modulestore.xml import create_block_from_xml, LocationReader, CourseLocationGenerator +from xmodule.modulestore import Location + +from xblock.runtime import KvsFieldData, DictKeyValueStore class InMemorySystem(XMLParsingSystem, MakoDescriptorSystem): # pylint: disable=abstract-method @@ -18,26 +21,37 @@ class InMemorySystem(XMLParsingSystem, MakoDescriptorSystem): # pylint: disable self.course = xml_import_data.course self.default_class = xml_import_data.default_class self._descriptors = {} + + def get_policy(usage_id): + """Return the policy data for the specified usage""" + return xml_import_data.policy.get(policy_key(usage_id), {}) + super(InMemorySystem, self).__init__( - policy=xml_import_data.policy, + get_policy=get_policy, process_xml=self.process_xml, load_item=self.load_item, error_tracker=Mock(), resources_fs=xml_import_data.filesystem, mixins=xml_import_data.xblock_mixins, select=xml_import_data.xblock_select, - render_template=lambda template, context: pprint.pformat((template, context)) + render_template=lambda template, context: pprint.pformat((template, context)), + field_data=KvsFieldData(DictKeyValueStore()), + id_reader=LocationReader(), ) def process_xml(self, xml): # pylint: disable=method-hidden """Parse `xml` as an XBlock, and add it to `self._descriptors`""" - descriptor = create_block_from_xml(xml, self, self.org, self.course, self.default_class) + descriptor = create_block_from_xml( + xml, + self, + CourseLocationGenerator(self.org, self.course), + ) self._descriptors[descriptor.location.url()] = descriptor return descriptor def load_item(self, location): # pylint: disable=method-hidden """Return the descriptor loaded for `location`""" - return self._descriptors[location] + return self._descriptors[Location(location).url()] class XModuleXmlImportTest(object): diff --git a/common/lib/xmodule/xmodule/video_module.py b/common/lib/xmodule/xmodule/video_module.py index cbd859b6dd..364a8ee417 100644 --- a/common/lib/xmodule/xmodule/video_module.py +++ b/common/lib/xmodule/xmodule/video_module.py @@ -30,7 +30,7 @@ from xblock.fields import Scope, String, Boolean, List, Integer, ScopeIds from xmodule.fields import RelativeTime from xmodule.modulestore.inheritance import InheritanceKeyValueStore -from xblock.runtime import DbModel +from xblock.runtime import KvsFieldData log = logging.getLogger(__name__) @@ -214,7 +214,7 @@ class VideoDescriptor(VideoFields, TabsEditingDescriptor, EmptyDataRawDescriptor del self.data @classmethod - def from_xml(cls, xml_data, system, org=None, course=None): + def from_xml(cls, xml_data, system, id_generator): """ Creates an instance of this descriptor from the supplied xml_data. This may be overridden by subclasses @@ -227,14 +227,13 @@ class VideoDescriptor(VideoFields, TabsEditingDescriptor, EmptyDataRawDescriptor """ xml_object = etree.fromstring(xml_data) url_name = xml_object.get('url_name', xml_object.get('slug')) - location = Location( - 'i4x', org, course, 'video', url_name - ) + block_type = 'video' + definition_id = id_generator.create_definition(block_type, url_name) + usage_id = id_generator.create_usage(definition_id) if is_pointer_tag(xml_object): filepath = cls._format_filepath(xml_object.tag, name_to_pathname(url_name)) - xml_data = etree.tostring(cls.load_file(filepath, system.resources_fs, location)) + xml_data = etree.tostring(cls.load_file(filepath, system.resources_fs, usage_id)) field_data = cls._parse_video_xml(xml_data) - field_data['location'] = location kvs = InheritanceKeyValueStore(initial_values=field_data) field_data = KvsFieldData(kvs) video = system.construct_xblock_from_class( @@ -242,7 +241,7 @@ class VideoDescriptor(VideoFields, TabsEditingDescriptor, EmptyDataRawDescriptor # 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), + ScopeIds(None, block_type, definition_id, usage_id), field_data, ) return video diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index ffc6862115..e7bc5c47fb 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -10,18 +10,19 @@ from pkg_resources import resource_listdir, resource_string, resource_isdir from webob import Response from webob.multidict import MultiDict -from xmodule.modulestore import Location -from xmodule.modulestore.exceptions import ItemNotFoundError, InsufficientSpecificationError, InvalidLocationError - from xblock.core import XBlock from xblock.fields import Scope, Integer, Float, List, XBlockMixin, String from xblock.fragment import Fragment from xblock.plugin import default_select -from xblock.runtime import Runtime +from xblock.runtime import Runtime, MemoryIdManager from xmodule.fields import RelativeTime + from xmodule.errortracker import exc_info_to_str +from xmodule.modulestore import Location +from xmodule.modulestore.exceptions import ItemNotFoundError, InsufficientSpecificationError, InvalidLocationError from xmodule.modulestore.locator import BlockUsageLocator + log = logging.getLogger(__name__) @@ -650,28 +651,30 @@ class XModuleDescriptor(XModuleMixin, HTMLSnippet, ResourceTemplates, XBlock): # ================================= XML PARSING ============================ @classmethod - def parse_xml(cls, node, runtime, keys): + def parse_xml(cls, node, runtime, keys, id_generator): """ Interpret the parsed XML in `node`, creating an XModuleDescriptor. """ xml = etree.tostring(node) # TODO: change from_xml to not take org and course, it can use self.system. - block = cls.from_xml(xml, runtime, runtime.org, runtime.course) + block = cls.from_xml(xml, runtime, id_generator) return block @classmethod - def from_xml(cls, xml_data, system, org=None, course=None): + def from_xml(cls, xml_data, system, id_generator): """ Creates an instance of this descriptor from the supplied xml_data. This may be overridden by subclasses. - xml_data: A string of xml that will be translated into data and children - for this module + Args: + xml_data (str): A string of xml that will be translated into data and children + for this module - system is an XMLParsingSystem + system (:class:`.XMLParsingSystem): + + id_generator (:class:`xblock.runtime.IdGenerator`): Used to generate the + usage_ids and definition_ids when loading this xml - org and course are optional strings that will be used in the generated - module's url identifiers """ raise NotImplementedError('Modules must implement from_xml to be parsable from xml') @@ -872,7 +875,9 @@ class DescriptorSystem(ConfigurableFragmentWrapper, Runtime): # pylint: disable Base class for :class:`Runtime`s to be used with :class:`XModuleDescriptor`s """ - def __init__(self, load_item, resources_fs, error_tracker, **kwargs): + def __init__( + self, load_item, resources_fs, error_tracker, get_policy=None, **kwargs + ): """ load_item: Takes a Location and returns an XModuleDescriptor @@ -907,15 +912,20 @@ class DescriptorSystem(ConfigurableFragmentWrapper, Runtime): # pylint: disable NOTE: To avoid duplication, do not call the tracker on errors that you're about to re-raise---let the caller track them. - """ - # Right now, usage_store is unused, and field_data is always supplanted - # with an explicit field_data during construct_xblock, so None's suffice. - super(DescriptorSystem, self).__init__(usage_store=None, field_data=None, **kwargs) + get_policy: a function that takes a usage id and returns a dict of + policy to apply. + + """ + super(DescriptorSystem, self).__init__(**kwargs) self.load_item = load_item self.resources_fs = resources_fs self.error_tracker = error_tracker + if get_policy: + self.get_policy = get_policy + else: + self.get_policy = lambda u: {} def get_block(self, usage_id): """See documentation for `xblock.runtime:Runtime.get_block`""" @@ -978,17 +988,14 @@ class DescriptorSystem(ConfigurableFragmentWrapper, Runtime): # pylint: disable class XMLParsingSystem(DescriptorSystem): - def __init__(self, process_xml, policy, **kwargs): + def __init__(self, process_xml, **kwargs): """ - policy: a policy dictionary for overriding xml metadata - process_xml: Takes an xml string, and returns a XModuleDescriptor created from that xml """ super(XMLParsingSystem, self).__init__(**kwargs) self.process_xml = process_xml - self.policy = policy class ModuleSystem(ConfigurableFragmentWrapper, Runtime): # pylint: disable=abstract-method @@ -1010,7 +1017,9 @@ class ModuleSystem(ConfigurableFragmentWrapper, Runtime): # pylint: disable=abs anonymous_student_id='', course_id=None, open_ended_grading_interface=None, s3_interface=None, cache=None, can_execute_unsafe_code=None, replace_course_urls=None, - replace_jump_to_id_urls=None, error_descriptor_class=None, get_real_user=None, **kwargs): + replace_jump_to_id_urls=None, error_descriptor_class=None, get_real_user=None, + field_data=None, + **kwargs): """ Create a closure around the system environment. @@ -1062,11 +1071,12 @@ class ModuleSystem(ConfigurableFragmentWrapper, Runtime): # pylint: disable=abs get_real_user - function that takes `anonymous_student_id` and returns real user_id, associated with `anonymous_student_id`. + field_data - the `FieldData` to use for backing XBlock storage. """ - # Right now, usage_store is unused, and field_data is always supplanted - # with an explicit field_data during construct_xblock, so None's suffice. - super(ModuleSystem, self).__init__(usage_store=None, field_data=None, **kwargs) + # Usage_store is unused, and field_data is often supplanted with an + # explicit field_data during construct_xblock. + super(ModuleSystem, self).__init__(id_reader=None, field_data=field_data, **kwargs) self.STATIC_URL = static_url self.xqueue = xqueue diff --git a/common/lib/xmodule/xmodule/xml_module.py b/common/lib/xmodule/xmodule/xml_module.py index 4581a7ae89..c659b7787b 100644 --- a/common/lib/xmodule/xmodule/xml_module.py +++ b/common/lib/xmodule/xmodule/xml_module.py @@ -176,7 +176,7 @@ class XmlDescriptor(XModuleDescriptor): return etree.parse(file_object, parser=edx_xml_parser).getroot() @classmethod - def load_file(cls, filepath, fs, location): + def load_file(cls, filepath, fs, def_id): ''' Open the specified file in fs, and call cls.file_to_xml on it, returning the lxml object. @@ -189,11 +189,11 @@ class XmlDescriptor(XModuleDescriptor): except Exception as err: # Add info about where we are, but keep the traceback msg = 'Unable to load file contents at path %s for item %s: %s ' % ( - filepath, location.url(), str(err)) + filepath, def_id, err) raise Exception, msg, sys.exc_info()[2] @classmethod - def load_definition(cls, xml_object, system, location): + def load_definition(cls, xml_object, system, def_id): '''Load a descriptor definition from the specified xml_object. Subclasses should not need to override this except in special cases (e.g. html module)''' @@ -219,7 +219,7 @@ class XmlDescriptor(XModuleDescriptor): filepath = candidate break - definition_xml = cls.load_file(filepath, system.resources_fs, location) + definition_xml = cls.load_file(filepath, system.resources_fs, def_id) definition_metadata = get_metadata_from_xml(definition_xml) cls.clean_metadata_from_xml(definition_xml) @@ -268,7 +268,7 @@ class XmlDescriptor(XModuleDescriptor): metadata[attr] = value @classmethod - def from_xml(cls, xml_data, system, org=None, course=None): + def from_xml(cls, xml_data, system, id_generator): """ Creates an instance of this descriptor from the supplied xml_data. This may be overridden by subclasses @@ -276,26 +276,25 @@ class XmlDescriptor(XModuleDescriptor): xml_data: A string of xml that will be translated into data and children for this module system: A DescriptorSystem for interacting with external resources - org and course are optional strings that will be used in the generated modules - url identifiers """ xml_object = etree.fromstring(xml_data) # VS[compat] -- just have the url_name lookup, once translation is done url_name = xml_object.get('url_name', xml_object.get('slug')) - location = Location('i4x', org, course, xml_object.tag, url_name) + def_id = id_generator.create_definition(xml_object.tag, url_name) + usage_id = id_generator.create_usage(def_id) # VS[compat] -- detect new-style each-in-a-file mode if is_pointer_tag(xml_object): # new style: # read the actual definition file--named using url_name.replace(':','/') filepath = cls._format_filepath(xml_object.tag, name_to_pathname(url_name)) - definition_xml = cls.load_file(filepath, system.resources_fs, location) + definition_xml = cls.load_file(filepath, system.resources_fs, def_id) else: definition_xml = xml_object filepath = None - definition, children = cls.load_definition(definition_xml, system, location) # note this removes metadata + definition, children = cls.load_definition(definition_xml, system, def_id) # note this removes metadata # VS[compat] -- make Ike's github preview links work in both old and # new file layouts @@ -312,13 +311,11 @@ class XmlDescriptor(XModuleDescriptor): try: metadata.update(json.loads(dmdata)) except Exception as err: - log.debug('Error %s in loading metadata %s' % (err, dmdata)) + log.debug('Error in loading metadata %r', dmdata, exc_info=True) metadata['definition_metadata_err'] = str(err) # Set/override any metadata specified by policy - k = policy_key(location) - if k in system.policy: - cls.apply_policy(metadata, system.policy[k]) + cls.apply_policy(metadata, system.get_policy(usage_id)) field_data = {} field_data.update(metadata) @@ -326,17 +323,13 @@ class XmlDescriptor(XModuleDescriptor): field_data['children'] = children field_data['xml_attributes']['filename'] = definition.get('filename', ['', None]) # for git link - field_data['location'] = location - field_data['category'] = xml_object.tag kvs = InheritanceKeyValueStore(initial_values=field_data) field_data = KvsFieldData(kvs) return system.construct_xblock_from_class( cls, # 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), + ScopeIds(None, xml_object.tag, def_id, usage_id), field_data, ) diff --git a/lms/djangoapps/courseware/management/commands/dump_course_structure.py b/lms/djangoapps/courseware/management/commands/dump_course_structure.py index 63eb27e51f..6814b3cfe4 100644 --- a/lms/djangoapps/courseware/management/commands/dump_course_structure.py +++ b/lms/djangoapps/courseware/management/commands/dump_course_structure.py @@ -95,7 +95,7 @@ def dump_module(module, destination=None, inherited=False, defaults=False): destination[module.location.url()] = { 'category': module.location.category, - 'children': module.children if hasattr(module, 'children') else [], + 'children': [str(child) for child in getattr(module, 'children', [])], 'metadata': filtered_metadata, } diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index 4f8a08e82b..e717743ba0 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/lms/djangoapps/courseware/module_render.py @@ -29,7 +29,7 @@ from util.json_request import JsonResponse from util.sandboxing import can_execute_unsafe_code from xblock.core import XBlock from xblock.fields import Scope -from xblock.runtime import KvsFieldData +from xblock.runtime import KvsFieldData, KeyValueStore from xblock.exceptions import NoSuchHandlerError from xblock.django.request import django_to_webob_request, webob_to_django_response from xmodule.error_module import ErrorDescriptor, NonStaffErrorDescriptor diff --git a/requirements/edx/github.txt b/requirements/edx/github.txt index 19ed7b283c..3bb782bb26 100644 --- a/requirements/edx/github.txt +++ b/requirements/edx/github.txt @@ -15,7 +15,7 @@ -e git+https://github.com/eventbrite/zendesk.git@d53fe0e81b623f084e91776bcf6369f8b7b63879#egg=zendesk # Our libraries: --e git+https://github.com/edx/XBlock.git@cd77808aadd3ea1c2027ca8c0aa5624d8ccccc52#egg=XBlock +-e git+https://github.com/edx/XBlock.git@a1a3e76b269d15b7bbd11976d8aef63e1db6c4c2#egg=XBlock -e git+https://github.com/edx/codejail.git@e3d98f9455#egg=codejail -e git+https://github.com/edx/diff-cover.git@v0.2.6#egg=diff_cover -e git+https://github.com/edx/js-test-tool.git@v0.1.5#egg=js_test_tool From a55724d1172c092b5bf96f986e1be512f0a80ce9 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 14 Jan 2014 12:52:07 -0500 Subject: [PATCH 6/6] Prevent unicode data from being injected into a Location using the _replace function --- .../xmodule/xmodule/modulestore/__init__.py | 39 ++- .../modulestore/tests/test_location.py | 304 ++++++++++-------- common/lib/xmodule/xmodule/modulestore/xml.py | 12 +- .../xmodule/modulestore/xml_importer.py | 4 +- requirements/edx/base.txt | 1 - requirements/edx/github.txt | 3 + 6 files changed, 201 insertions(+), 162 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/__init__.py b/common/lib/xmodule/xmodule/modulestore/__init__.py index 4ec10f7ae5..0a1cf9ee10 100644 --- a/common/lib/xmodule/xmodule/modulestore/__init__.py +++ b/common/lib/xmodule/xmodule/modulestore/__init__.py @@ -40,6 +40,21 @@ INVALID_HTML_CHARS = re.compile(r"[^\w-]") _LocationBase = namedtuple('LocationBase', 'tag org course category name revision') +def _check_location_part(val, regexp): + """ + Check that `regexp` doesn't match inside `val`. If it does, raise an exception + + Args: + val (string): The value to check + regexp (re.RegexObject): The regular expression specifying invalid characters + + Raises: + InvalidLocationError: Raised if any invalid character is found in `val` + """ + if val is not None and regexp.search(val) is not None: + raise InvalidLocationError("Invalid characters in {!r}.".format(val)) + + class Location(_LocationBase): ''' Encodes a location. @@ -145,7 +160,6 @@ class Location(_LocationBase): Components may be set to None, which may be interpreted in some contexts to mean wildcard selection. """ - if (org is None and course is None and category is None and name is None and revision is None): location = loc_or_tag else: @@ -161,23 +175,18 @@ class Location(_LocationBase): check_list(list_) def check_list(list_): - def check(val, regexp): - if val is not None and regexp.search(val) is not None: - log.debug('invalid characters val=%r, list_=%r', val, list_) - raise InvalidLocationError("Invalid characters in {!r}.".format(val)) - list_ = list(list_) for val in list_[:4] + [list_[5]]: - check(val, INVALID_CHARS) + _check_location_part(val, INVALID_CHARS) # names allow colons - check(list_[4], INVALID_CHARS_NAME) + _check_location_part(list_[4], INVALID_CHARS_NAME) if isinstance(location, Location): return location elif isinstance(location, basestring): match = URL_RE.match(location) if match is None: - log.debug("location %r doesn't match URL" % location) + log.debug("location %r doesn't match URL", location) raise InvalidLocationError(location) groups = match.groupdict() check_dict(groups) @@ -249,6 +258,18 @@ class Location(_LocationBase): return "/".join([self.org, self.course, self.name]) + def _replace(self, **kwargs): + """ + Return a new :class:`Location` with values replaced + by the values specified in `**kwargs` + """ + for name, value in kwargs.iteritems(): + if name == 'name': + _check_location_part(value, INVALID_CHARS_NAME) + else: + _check_location_part(value, INVALID_CHARS) + return super(Location, self)._replace(**kwargs) + def replace(self, **kwargs): ''' Expose a public method for replacing location elements diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_location.py b/common/lib/xmodule/xmodule/modulestore/tests/test_location.py index c190559c73..5f4983e1bf 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_location.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_location.py @@ -1,170 +1,188 @@ -from nose.tools import assert_equals, assert_raises, assert_not_equals # pylint: disable=E0611 +import ddt + +from unittest import TestCase from xmodule.modulestore import Location from xmodule.modulestore.exceptions import InvalidLocationError - -def check_string_roundtrip(url): - assert_equals(url, Location(url).url()) - assert_equals(url, str(Location(url))) +# Pairs for testing the clean* functions. +# The first item in the tuple is the input string. +# The second item in the tuple is what the result of +# replacement should be. +GENERAL_PAIRS = [ + ('', ''), + (' ', '_'), + ('abc,', 'abc_'), + ('ab fg!@//\\aj', 'ab_fg_aj'), + (u"ab\xA9", "ab_"), # no unicode allowed for now +] -def test_string_roundtrip(): - check_string_roundtrip("tag://org/course/category/name") - check_string_roundtrip("tag://org/course/category/name@revision") - - -input_dict = { - 'tag': 'tag', - 'course': 'course', - 'category': 'category', - 'name': 'name', - 'org': 'org' -} - - -also_valid_dict = { - 'tag': 'tag', - 'course': 'course', - 'category': 'category', - 'name': 'name:more_name', - 'org': 'org' -} - - -input_list = ['tag', 'org', 'course', 'category', 'name'] - -input_str = "tag://org/course/category/name" -input_str_rev = "tag://org/course/category/name@revision" - -valid = (input_list, input_dict, input_str, input_str_rev, also_valid_dict) - -invalid_dict = { - 'tag': 'tag', - 'course': 'course', - 'category': 'category', - 'name': 'name@more_name', - 'org': 'org' -} - -invalid_dict2 = { - 'tag': 'tag', - 'course': 'course', - 'category': 'category', - 'name': 'name ', # extra space - 'org': 'org' -} - -invalid = ("foo", ["foo"], ["foo", "bar"], - ["foo", "bar", "baz", "blat:blat", "foo:bar"], # ':' ok in name, not in category - "tag://org/course/category/name with spaces@revision", - "tag://org/course/category/name/with/slashes@revision", - invalid_dict, - invalid_dict2) - - -def test_is_valid(): - for v in valid: - assert_equals(Location.is_valid(v), True) - - for v in invalid: - assert_equals(Location.is_valid(v), False) - - -def test_dict(): - assert_equals("tag://org/course/category/name", Location(input_dict).url()) - assert_equals(dict(revision=None, **input_dict), Location(input_dict).dict()) - - input_dict['revision'] = 'revision' - assert_equals("tag://org/course/category/name@revision", Location(input_dict).url()) - assert_equals(input_dict, Location(input_dict).dict()) - - -def test_list(): - assert_equals("tag://org/course/category/name", Location(input_list).url()) - assert_equals(input_list + [None], Location(input_list).list()) - - input_list.append('revision') - assert_equals("tag://org/course/category/name@revision", Location(input_list).url()) - assert_equals(input_list, Location(input_list).list()) - - -def test_location(): - input_list = ['tag', 'org', 'course', 'category', 'name'] - assert_equals("tag://org/course/category/name", Location(Location(input_list)).url()) - - -def test_none(): - assert_equals([None] * 6, Location(None).list()) - - -def test_invalid_locations(): - assert_raises(InvalidLocationError, Location, "foo") - assert_raises(InvalidLocationError, Location, ["foo", "bar"]) - assert_raises(InvalidLocationError, Location, ["foo", "bar", "baz", "blat/blat", "foo"]) - assert_raises(InvalidLocationError, Location, ["foo", "bar", "baz", "blat", "foo/bar"]) - assert_raises(InvalidLocationError, Location, "tag://org/course/category/name with spaces@revision") - assert_raises(InvalidLocationError, Location, "tag://org/course/category/name/revision") - - -def test_equality(): - assert_equals( - Location('tag', 'org', 'course', 'category', 'name'), - Location('tag', 'org', 'course', 'category', 'name') +@ddt.ddt +class TestLocations(TestCase): + @ddt.data( + "tag://org/course/category/name", + "tag://org/course/category/name@revision" ) + def test_string_roundtrip(self, url): + self.assertEquals(url, Location(url).url()) + self.assertEquals(url, str(Location(url))) - assert_not_equals( - Location('tag', 'org', 'course', 'category', 'name1'), - Location('tag', 'org', 'course', 'category', 'name') + @ddt.data( + { + 'tag': 'tag', + 'course': 'course', + 'category': 'category', + 'name': 'name', + 'org': 'org' + }, + { + 'tag': 'tag', + 'course': 'course', + 'category': 'category', + 'name': 'name:more_name', + 'org': 'org' + }, + ['tag', 'org', 'course', 'category', 'name'], + "tag://org/course/category/name", + "tag://org/course/category/name@revision", + u"tag://org/course/category/name", + u"tag://org/course/category/name@revision", ) + def test_is_valid(self, loc): + self.assertTrue(Location.is_valid(loc)) -# All the cleaning functions should do the same thing with these -general_pairs = [('', ''), - (' ', '_'), - ('abc,', 'abc_'), - ('ab fg!@//\\aj', 'ab_fg_aj'), - (u"ab\xA9", "ab_"), # no unicode allowed for now - ] + @ddt.data( + { + 'tag': 'tag', + 'course': 'course', + 'category': 'category', + 'name': 'name@more_name', + 'org': 'org' + }, + { + 'tag': 'tag', + 'course': 'course', + 'category': 'category', + 'name': 'name ', # extra space + 'org': 'org' + }, + "foo", + ["foo"], + ["foo", "bar"], + ["foo", "bar", "baz", "blat:blat", "foo:bar"], # ':' ok in name, not in category + "tag://org/course/category/name with spaces@revision", + "tag://org/course/category/name/with/slashes@revision", + u"tag://org/course/category/name\xae", # No non-ascii characters for now + u"tag://org/course/category\xae/name", # No non-ascii characters for now + ) + def test_is_invalid(self, loc): + self.assertFalse(Location.is_valid(loc)) + def test_dict(self): + input_dict = { + 'tag': 'tag', + 'course': 'course', + 'category': 'category', + 'name': 'name', + 'org': 'org' + } -def test_clean(): - pairs = general_pairs + [ + self.assertEquals("tag://org/course/category/name", Location(input_dict).url()) + self.assertEquals(dict(revision=None, **input_dict), Location(input_dict).dict()) + + input_dict['revision'] = 'revision' + self.assertEquals("tag://org/course/category/name@revision", Location(input_dict).url()) + self.assertEquals(input_dict, Location(input_dict).dict()) + + def test_list(self): + input_list = ['tag', 'org', 'course', 'category', 'name'] + self.assertEquals("tag://org/course/category/name", Location(input_list).url()) + self.assertEquals(input_list + [None], Location(input_list).list()) + + input_list.append('revision') + self.assertEquals("tag://org/course/category/name@revision", Location(input_list).url()) + self.assertEquals(input_list, Location(input_list).list()) + + def test_location(self): + input_list = ['tag', 'org', 'course', 'category', 'name'] + self.assertEquals("tag://org/course/category/name", Location(Location(input_list)).url()) + + def test_none(self): + self.assertEquals([None] * 6, Location(None).list()) + + @ddt.data( + "foo", + ["foo", "bar"], + ["foo", "bar", "baz", "blat/blat", "foo"], + ["foo", "bar", "baz", "blat", "foo/bar"], + "tag://org/course/category/name with spaces@revision", + "tag://org/course/category/name/revision", + ) + def test_invalid_locations(self, loc): + with self.assertRaises(InvalidLocationError): + Location(loc) + + def test_equality(self): + self.assertEquals( + Location('tag', 'org', 'course', 'category', 'name'), + Location('tag', 'org', 'course', 'category', 'name') + ) + + self.assertNotEquals( + Location('tag', 'org', 'course', 'category', 'name1'), + Location('tag', 'org', 'course', 'category', 'name') + ) + + @ddt.data( ('a:b', 'a_b'), # no colons in non-name components ('a-b', 'a-b'), # dashes ok ('a.b', 'a.b'), # dot ok - ] - for input, output in pairs: - assert_equals(Location.clean(input), output) + *GENERAL_PAIRS + ) + def test_clean(self, pair): + self.assertEquals(Location.clean(pair[0]), pair[1]) - -def test_clean_for_url_name(): - pairs = general_pairs + [ + @ddt.data( ('a:b', 'a:b'), # colons ok in names ('a-b', 'a-b'), # dashes ok in names ('a.b', 'a.b'), # dot ok in names - ] - for input, output in pairs: - assert_equals(Location.clean_for_url_name(input), output) + *GENERAL_PAIRS + ) + def test_clean_for_url_name(self, pair): + self.assertEquals(Location.clean_for_url_name(pair[0]), pair[1]) - -def test_clean_for_html(): - pairs = general_pairs + [ + @ddt.data( ("a:b", "a_b"), # no colons for html use ("a-b", "a-b"), # dashes ok (though need to be replaced in various use locations. ugh.) ('a.b', 'a_b'), # no dots. - ] - for input, output in pairs: - assert_equals(Location.clean_for_html(input), output) + *GENERAL_PAIRS + ) + def test_clean_for_html(self, pair): + self.assertEquals(Location.clean_for_html(pair[0]), pair[1]) + def test_html_id(self): + loc = Location("tag://org/course/cat/name:more_name@rev") + self.assertEquals(loc.html_id(), "tag-org-course-cat-name_more_name-rev") -def test_html_id(): - loc = Location("tag://org/course/cat/name:more_name@rev") - assert_equals(loc.html_id(), "tag-org-course-cat-name_more_name-rev") + def test_course_id(self): + loc = Location('i4x', 'mitX', '103', 'course', 'test2') + self.assertEquals('mitX/103/test2', loc.course_id) + loc = Location('i4x', 'mitX', '103', '_not_a_course', 'test2') + with self.assertRaises(InvalidLocationError): + loc.course_id # pylint: disable=pointless-statement -def test_course_id(): - loc = Location('i4x', 'mitX', '103', 'course', 'test2') - assert_equals('mitX/103/test2', loc.course_id) + def test_replacement(self): + self.assertEquals( + Location('t://o/c/c/n@r')._replace(name='new_name'), + Location('t://o/c/c/new_name@r'), + ) - loc = Location('i4x', 'mitX', '103', '_not_a_course', 'test2') - with assert_raises(InvalidLocationError): - loc.course_id + with self.assertRaises(InvalidLocationError): + Location('t://o/c/c/n@r')._replace(name=u'name\xae') + + @ddt.data('org', 'course', 'category', 'name', 'revision') + def test_immutable(self, attr): + loc = Location('t://o/c/c/n@r') + with self.assertRaises(AttributeError): + setattr(loc, attr, attr) diff --git a/common/lib/xmodule/xmodule/modulestore/xml.py b/common/lib/xmodule/xmodule/modulestore/xml.py index 4318e2fbdc..13079bf9b3 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml.py +++ b/common/lib/xmodule/xmodule/modulestore/xml.py @@ -205,11 +205,11 @@ class ImportSystem(XMLParsingSystem, MakoDescriptorSystem): descriptor.data_dir = course_dir - xmlstore.modules[course_id][descriptor.location] = descriptor + xmlstore.modules[course_id][descriptor.scope_ids.usage_id] = descriptor if descriptor.has_children: for child in descriptor.get_children(): - parent_tracker.add_parent(child.location, descriptor.location) + parent_tracker.add_parent(child.scope_ids.usage_id, descriptor.scope_ids.usage_id) # After setting up the descriptor, save any changes that we have # made to attributes on the descriptor to the underlying KeyValueStore. @@ -412,8 +412,8 @@ class XMLModuleStore(ModuleStoreReadBase): if course_descriptor is not None and not isinstance(course_descriptor, ErrorDescriptor): self.courses[course_dir] = course_descriptor - self._location_errors[course_descriptor.location] = errorlog - self.parent_trackers[course_descriptor.id].make_known(course_descriptor.location) + self._location_errors[course_descriptor.scope_ids.usage_id] = errorlog + self.parent_trackers[course_descriptor.id].make_known(course_descriptor.scope_ids.usage_id) else: # Didn't load course. Instead, save the errors elsewhere. self.errored_courses[course_dir] = errorlog @@ -570,7 +570,7 @@ class XMLModuleStore(ModuleStoreReadBase): html = f.read().decode('utf-8') # 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) + loc = course_descriptor.scope_ids.usage_id._replace(category=category, name=slug) module = system.construct_xblock_from_class( HtmlDescriptor, # We're loading a descriptor, so student_id is meaningless @@ -588,7 +588,7 @@ class XMLModuleStore(ModuleStoreReadBase): module.display_name = tab['name'] module.data_dir = course_dir module.save() - self.modules[course_descriptor.id][module.location] = module + self.modules[course_descriptor.id][module.scope_ids.usage_id] = module except Exception, e: logging.exception("Failed to load %s. Skipping... \ Exception: %s", filepath, unicode(e)) diff --git a/common/lib/xmodule/xmodule/modulestore/xml_importer.py b/common/lib/xmodule/xmodule/modulestore/xml_importer.py index ac574c6756..47dedecc6b 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml_importer.py +++ b/common/lib/xmodule/xmodule/modulestore/xml_importer.py @@ -310,9 +310,7 @@ def import_module( source_course_location, dest_course_location, allow_not_found=False, do_import_static=True): - logging.debug('processing import of module {url}...'.format( - url=module.location.url() - )) + logging.debug('processing import of module {}...'.format(module.location.url())) content = {} for field in module.fields.values(): diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 16cd5ecd64..7dd8b7f1fd 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -93,7 +93,6 @@ transifex-client==0.9.1 # Used for testing coverage==3.7 -ddt==0.4.0 factory_boy==2.2.1 mock==1.0.1 nosexcover==1.0.7 diff --git a/requirements/edx/github.txt b/requirements/edx/github.txt index 3bb782bb26..f53b600013 100644 --- a/requirements/edx/github.txt +++ b/requirements/edx/github.txt @@ -22,3 +22,6 @@ -e git+https://github.com/edx/django-waffle.git@823a102e48#egg=django-waffle -e git+https://github.com/edx/event-tracking.git@f0211d702d#egg=event-tracking -e git+https://github.com/edx/bok-choy.git@bc6f1adbe439618162079f1004b2b3db3b6f8916#egg=bok_choy + +# Move back to upstream release once https://github.com/txels/ddt/pull/13 is merged +-e git+https://github.com/edx/ddt.git@9e8010b8777aa40b848fdb76de6e60081616325a#egg=ddt \ No newline at end of file