From 199f94aa993b6dd26df051becd38c6290dcb3634 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 19 Jun 2012 11:24:22 -0400 Subject: [PATCH 01/14] Conform to new TODO standards --- cms/djangoapps/contentstore/views.py | 2 +- doc/code_standards.txt | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/cms/djangoapps/contentstore/views.py b/cms/djangoapps/contentstore/views.py index e29c41ea59..ad846fb369 100644 --- a/cms/djangoapps/contentstore/views.py +++ b/cms/djangoapps/contentstore/views.py @@ -4,7 +4,7 @@ from django.contrib.auth.decorators import login_required def index(request): - # FIXME (cpennington): These need to be read in from the active user + # TODO (cpennington): These need to be read in from the active user org = 'mit.edu' course = '6002xs12' course = keystore().get_item(['i4x', org, course, 'Course', None]) diff --git a/doc/code_standards.txt b/doc/code_standards.txt index 02953b3677..a6a3b3e556 100644 --- a/doc/code_standards.txt +++ b/doc/code_standards.txt @@ -83,3 +83,6 @@ no hard standards. * When impossible, it should live in the github repo. * Discussion should live on github, Basecamp or Pivotal, depending on context. +* Notes for later fixes should in general be put into Pivotal as stories. + If they are left in the code, they should be prefixed by + # TODO () From 0379d1cb9577c181e186cc709622f79690cf76fe Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 19 Jun 2012 11:25:29 -0400 Subject: [PATCH 02/14] Search for course specifically in navigation view --- cms/djangoapps/contentstore/views.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cms/djangoapps/contentstore/views.py b/cms/djangoapps/contentstore/views.py index ad846fb369..a87520ab13 100644 --- a/cms/djangoapps/contentstore/views.py +++ b/cms/djangoapps/contentstore/views.py @@ -7,6 +7,7 @@ def index(request): # TODO (cpennington): These need to be read in from the active user org = 'mit.edu' course = '6002xs12' - course = keystore().get_item(['i4x', org, course, 'Course', None]) + name = '6.002 Spring 2012' + course = keystore().get_item(['i4x', org, course, 'Course', name]) weeks = course.get_children() return render_to_response('index.html', {'weeks': weeks}) From 703103e7675dba223e8dd883945bd3f5d2ab44ba Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 19 Jun 2012 11:25:41 -0400 Subject: [PATCH 03/14] Get rid of references to askbot --- cms/envs/common.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/cms/envs/common.py b/cms/envs/common.py index 8d402b6fa9..80056af1d2 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -27,15 +27,12 @@ from path import path PROJECT_ROOT = path(__file__).abspath().dirname().dirname() # /mitx/cms COMMON_ROOT = PROJECT_ROOT.dirname() / "common" ENV_ROOT = PROJECT_ROOT.dirname().dirname() # virtualenv dir /mitx is in -ASKBOT_ROOT = ENV_ROOT / "askbot-devel" COURSES_ROOT = ENV_ROOT / "data" # FIXME: To support multiple courses, we should walk the courses dir at startup DATA_DIR = COURSES_ROOT sys.path.append(ENV_ROOT) -sys.path.append(ASKBOT_ROOT) -sys.path.append(ASKBOT_ROOT / "askbot" / "deps") sys.path.append(PROJECT_ROOT / 'djangoapps') sys.path.append(PROJECT_ROOT / 'lib') sys.path.append(COMMON_ROOT / 'djangoapps') From f1ffff1dc0dba30bb8ed2d509107117a5375a728 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 19 Jun 2012 11:27:29 -0400 Subject: [PATCH 04/14] Cleanup and test Location, and add the ability to specify a revision --- common/lib/keystore/__init__.py | 108 ++++++++++++++++++---- common/lib/keystore/exceptions.py | 3 + common/lib/keystore/test/test_location.py | 52 +++++++++++ 3 files changed, 144 insertions(+), 19 deletions(-) create mode 100644 common/lib/keystore/test/test_location.py diff --git a/common/lib/keystore/__init__.py b/common/lib/keystore/__init__.py index 61c797241d..592cde7b4d 100644 --- a/common/lib/keystore/__init__.py +++ b/common/lib/keystore/__init__.py @@ -10,47 +10,117 @@ the following attributes: revision: What revision of the item this is """ +import re +from .exceptions import InvalidLocationError + +URL_RE = re.compile(""" + (?P[^:]+):// + (?P[^/]+)/ + (?P[^/]+)/ + (?P[^/]+)/ + (?P[^/]+) + (/(?P[^/]+))? + """, re.VERBOSE) + class Location(object): - ''' Encodes a location. - Can be: - * String (url) - * Tuple - * Dictionary + ''' + Encodes a location. + + Locations representations of URLs of the + form {tag}://{org}/{course}/{category}/{name}[/{revision}] + + However, they can also be represented a dictionaries (specifying each component), + tuples or list (specified in order), or as strings of the url ''' def __init__(self, location): + """ + Create a new location that is a clone of the specifed one. + + location - Can be any of the following types: + string: should be of the form {tag}://{org}/{course}/{category}/{name}[/{revision}] + list: should be of the form [tag, org, course, category, name, revision] + dict: should be of the form { + 'tag': tag, + 'org': org, + 'course': course, + 'category': category, + 'name': name, + 'revision': revision, + } + Location: another Location object + + None of the components of a location may contain the '/' character + """ self.update(location) def update(self, location): + """ + Update this instance with data from another Location object. + + location: can take the same forms as specified by `__init__` + """ + self.tag = self.org = self.course = self.category = self.name = self.revision = None + if isinstance(location, basestring): - self.tag = location.split('/')[0][:-1] - (self.org, self.course, self.category, self.name) = location.split('/')[2:] + match = URL_RE.match(location) + if match is None: + raise InvalidLocationError(location) + else: + self.update(match.groupdict()) elif isinstance(location, list): - (self.tag, self.org, self.course, self.category, self.name) = location + if len(location) not in (5, 6): + raise InvalidLocationError(location) + + (self.tag, self.org, self.course, self.category, self.name) = location[0:5] + self.revision = location[5] if len(location) == 6 else None elif isinstance(location, dict): - self.tag = location['tag'] - self.org = location['org'] - self.course = location['course'] - self.category = location['category'] - self.name = location['name'] + try: + self.tag = location['tag'] + self.org = location['org'] + self.course = location['course'] + self.category = location['category'] + self.name = location['name'] + except KeyError: + raise InvalidLocationError(location) + self.revision = location.get('revision') elif isinstance(location, Location): self.update(location.list()) + else: + raise InvalidLocationError(location) + + for val in self.list(): + if val is not None and '/' in val: + raise InvalidLocationError(location) + + def __str__(self): + return self.url() def url(self): - return "{tag}://{org}/{course}/{category}/{name}".format(**self.dict()) + """ + Return a string containing the URL for this location + """ + url = "{tag}://{org}/{course}/{category}/{name}".format(**self.dict()) + if self.revision: + url += "/" + self.revision + return url def list(self): - return [self.tag, self.org, self.course, self.category, self.name] + """ + Return a list representing this location + """ + return [self.tag, self.org, self.course, self.category, self.name, self.revision] def dict(self): + """ + Return a dictionary representing this location + """ return {'tag': self.tag, 'org': self.org, 'course': self.course, 'category': self.category, - 'name': self.name} - - def to_json(self): - return self.dict() + 'name': self.name, + 'revision': self.revision} class KeyStore(object): diff --git a/common/lib/keystore/exceptions.py b/common/lib/keystore/exceptions.py index 08fd9b11d0..4c8c55ffe9 100644 --- a/common/lib/keystore/exceptions.py +++ b/common/lib/keystore/exceptions.py @@ -9,3 +9,6 @@ class ItemNotFoundError(Exception): class InsufficientSpecificationError(Exception): pass + +class InvalidLocationError(Exception): + pass diff --git a/common/lib/keystore/test/test_location.py b/common/lib/keystore/test/test_location.py new file mode 100644 index 0000000000..f10f03c0b0 --- /dev/null +++ b/common/lib/keystore/test/test_location.py @@ -0,0 +1,52 @@ +from nose.tools import assert_equals, assert_raises +from keystore import Location +from keystore.exceptions import InvalidLocationError + + +def check_string_roundtrip(url): + assert_equals(url, Location(url).url()) + assert_equals(url, str(Location(url))) + + +def test_string_roundtrip(): + check_string_roundtrip("tag://org/course/category/name") + check_string_roundtrip("tag://org/course/category/name/revision") + check_string_roundtrip("tag://org/course/category/name with spaces/revision") + + +def test_dict(): + input_dict = { + 'tag': 'tag', + 'course': 'course', + 'category': 'category', + 'name': 'name', + 'org': 'org' + } + 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(): + input_list = ['tag', 'org', 'course', 'category', 'name'] + 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_invalid_locations(): + assert_raises(InvalidLocationError, Location, "foo") + assert_raises(InvalidLocationError, Location, ["foo", "bar"]) + assert_raises(InvalidLocationError, Location, ["foo", "bar", "baz", "blat", "foo/bar"]) + assert_raises(InvalidLocationError, Location, None) From 6daa0f1aa06092598892cb37a7f3c5f3541fa0c2 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 19 Jun 2012 11:27:44 -0400 Subject: [PATCH 05/14] Fix string layout for readability --- cms/manage.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/cms/manage.py b/cms/manage.py index 3e4eedc9ff..f8773c0641 100644 --- a/cms/manage.py +++ b/cms/manage.py @@ -5,7 +5,9 @@ try: imp.find_module('settings') # Assumed to be in the same directory. except ImportError: import sys - sys.stderr.write("Error: Can't find the file 'settings.py' in the directory containing %r. It appears you've customized things.\nYou'll have to run django-admin.py, passing it your settings module.\n" % __file__) + sys.stderr.write("Error: Can't find the file 'settings.py' in the directory containing %r. " + "It appears you've customized things.\nYou'll have to run django-admin.py, " + "passing it your settings module.\n" % __file__) sys.exit(1) import settings From 58085f8ed98c59a2d1569099e833f5002c3e5f5d Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 19 Jun 2012 11:27:56 -0400 Subject: [PATCH 06/14] Remove unused urls --- cms/urls.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/cms/urls.py b/cms/urls.py index 781c2c261f..d2e6415827 100644 --- a/cms/urls.py +++ b/cms/urls.py @@ -5,7 +5,5 @@ from django.conf.urls.defaults import patterns, url # admin.autodiscover() urlpatterns = patterns('', - url(r'^(?P[^/]+)/(?P[^/]+)/calendar/', 'contentstore.views.calendar', name='calendar'), - url(r'^accounts/login/', 'instructor.views.do_login', name='login'), url(r'^$', 'contentstore.views.index', name='index'), ) From 677c25ee687167430b0fbc1e88cbe6d6f6173a26 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 19 Jun 2012 11:28:22 -0400 Subject: [PATCH 07/14] Remove unused code --- common/lib/keystore/__init__.py | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/common/lib/keystore/__init__.py b/common/lib/keystore/__init__.py index 592cde7b4d..f5ca6f4164 100644 --- a/common/lib/keystore/__init__.py +++ b/common/lib/keystore/__init__.py @@ -165,19 +165,3 @@ class KeyStore(object): children: A list of child item identifiers """ raise NotImplementedError - - -class KeyStoreItem(object): - """ - An object from a KeyStore, which can be saved back to that keystore - """ - def __init__(self, location, children, data, editor, parents, revision): - self.location = location - self.children = children - self.data = data - self.editor = editor - self.parents = parents - self.revision = revision - - def save(self): - raise NotImplementedError From 5562def5b7137be944fdbc93bcd7e7b32ea0fbf2 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 19 Jun 2012 11:29:48 -0400 Subject: [PATCH 08/14] Add documentation of mongo query syntax usage --- common/lib/keystore/mongo.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/common/lib/keystore/mongo.py b/common/lib/keystore/mongo.py index d9760909c9..9b6327c8e9 100644 --- a/common/lib/keystore/mongo.py +++ b/common/lib/keystore/mongo.py @@ -67,6 +67,9 @@ class MongoKeyStore(KeyStore): location: Something that can be passed to Location data: A nested dictionary of problem data """ + + # See http://www.mongodb.org/display/DOCS/Updating for + # atomic update syntax self.collection.update( {'location': Location(location).dict()}, {'$set': {'data': data}} @@ -80,6 +83,9 @@ class MongoKeyStore(KeyStore): location: Something that can be passed to Location children: A list of child item identifiers """ + + # See http://www.mongodb.org/display/DOCS/Updating for + # atomic update syntax self.collection.update( {'location': Location(location).dict()}, {'$set': {'children': children}} From 6fb35c4773db0b5920a51cdd5f0b6d6eb316b01f Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 19 Jun 2012 11:31:13 -0400 Subject: [PATCH 09/14] Cleanup intertwined descriptor and keystore code --- cms/envs/dev.py | 8 +-- cms/templates/widgets/navigation.html | 12 +++-- common/lib/keystore/__init__.py | 32 +++++------- common/lib/keystore/django.py | 16 +++--- common/lib/keystore/mongo.py | 47 +++++++++-------- common/lib/xmodule/seq_module.py | 17 ------- common/lib/xmodule/setup.py | 5 +- common/lib/xmodule/x_module.py | 72 +++++++++++++++++++++------ doc/overview.md | 34 +++++++++++-- 9 files changed, 147 insertions(+), 96 deletions(-) diff --git a/cms/envs/dev.py b/cms/envs/dev.py index 332f52f145..16bed60729 100644 --- a/cms/envs/dev.py +++ b/cms/envs/dev.py @@ -7,9 +7,11 @@ DEBUG = True TEMPLATE_DEBUG = DEBUG KEYSTORE = { - 'host': 'localhost', - 'db': 'mongo_base', - 'collection': 'key_store', + 'default': { + 'host': 'localhost', + 'db': 'mongo_base', + 'collection': 'key_store', + } } DATABASES = { diff --git a/cms/templates/widgets/navigation.html b/cms/templates/widgets/navigation.html index 1f75dab470..2d5af9ead1 100644 --- a/cms/templates/widgets/navigation.html +++ b/cms/templates/widgets/navigation.html @@ -40,14 +40,18 @@

${week.name}

    - % for goal in week.get_goals(): -
  • ${goal.name}:${goal.data}
    • - % endfor + % if week.goals: + % for goal in week.goals: +
  • ${goal}
    • + % endfor + % else: +
  • Please create a learning goal for this week
    • + % endif
    - % for module in week.get_non_goals(): + % for module in week.get_children():
  • ${module.name} handle diff --git a/common/lib/keystore/__init__.py b/common/lib/keystore/__init__.py index f5ca6f4164..0e77a02a87 100644 --- a/common/lib/keystore/__init__.py +++ b/common/lib/keystore/__init__.py @@ -1,13 +1,6 @@ """ -This module provides an abstraction for working objects that conceptually have -the following attributes: - - location: An identifier for an item, of which there might be many revisions - children: A list of urls for other items required to fully define this object - data: A set of nested data needed to define this object - editor: The editor/owner of the object - parents: Url pointers for objects that this object was derived from - revision: What revision of the item this is +This module provides an abstraction for working with XModuleDescriptors +that are stored in a database an accessible using their Location as an identifier """ import re @@ -123,27 +116,26 @@ class Location(object): 'revision': self.revision} -class KeyStore(object): +class ModuleStore(object): + """ + An abstract interface for a database backend that stores XModuleDescriptor instances + """ def get_item(self, location): """ - Returns an XModuleDescriptor instance for the item at location + Returns an XModuleDescriptor instance for the item at location. + If location.revision is None, returns the most item with the most + recent revision + If any segment of the location is None except revision, raises + keystore.exceptions.InsufficientSpecificationError If no object is found at that location, raises keystore.exceptions.ItemNotFoundError - Searches for all matches of a partially specifed location, but raises an - keystore.exceptions.InsufficientSpecificationError if more - than a single object matches the query. - location: Something that can be passed to Location """ raise NotImplementedError + # TODO (cpennington): Replace with clone_item def create_item(self, location, editor): - """ - Create an empty item at the specified location with the supplied editor - - location: Something that can be passed to Location - """ raise NotImplementedError def update_item(self, location, data): diff --git a/common/lib/keystore/django.py b/common/lib/keystore/django.py index b88c74b8a3..2ba3f0756e 100644 --- a/common/lib/keystore/django.py +++ b/common/lib/keystore/django.py @@ -1,21 +1,21 @@ """ Module that provides a connection to the keystore specified in the django settings. -Passes settings.KEYSTORE as kwargs to MongoKeyStore +Passes settings.KEYSTORE as kwargs to MongoModuleStore """ from __future__ import absolute_import from django.conf import settings -from .mongo import MongoKeyStore +from .mongo import MongoModuleStore -_KEYSTORE = None +_KEYSTORES = {} -def keystore(): - global _KEYSTORE +def keystore(name='default'): + global _KEYSTORES - if _KEYSTORE is None: - _KEYSTORE = MongoKeyStore(**settings.KEYSTORE) + if name not in _KEYSTORES: + _KEYSTORES[name] = MongoModuleStore(**settings.KEYSTORE[name]) - return _KEYSTORE + return _KEYSTORES[name] diff --git a/common/lib/keystore/mongo.py b/common/lib/keystore/mongo.py index 9b6327c8e9..1bef298fde 100644 --- a/common/lib/keystore/mongo.py +++ b/common/lib/keystore/mongo.py @@ -1,12 +1,12 @@ import pymongo -from . import KeyStore, Location +from . import ModuleStore, Location from .exceptions import ItemNotFoundError, InsufficientSpecificationError -from xmodule.x_module import XModuleDescriptor +from xmodule.x_module import XModuleDescriptor, XModuleSystem -class MongoKeyStore(KeyStore): +class MongoModuleStore(ModuleStore): """ - A Mongodb backed KeyStore + A Mongodb backed ModuleStore """ def __init__(self, host, db, collection, port=27017): self.collection = pymongo.connection.Connection( @@ -19,34 +19,33 @@ class MongoKeyStore(KeyStore): def get_item(self, location): """ - Returns an XModuleDescriptor instance for the item at location + Returns an XModuleDescriptor instance for the item at location. + If location.revision is None, returns the most item with the most + recent revision + If any segment of the location is None except revision, raises + keystore.exceptions.InsufficientSpecificationError If no object is found at that location, raises keystore.exceptions.ItemNotFoundError - Searches for all matches of a partially specifed location, but raises an - keystore.exceptions.InsufficientSpecificationError if more - than a single object matches the query. - location: Something that can be passed to Location """ - query = dict( - ('location.{key}'.format(key=key), val) - for (key, val) - in Location(location).dict().items() - if val is not None - ) - items = self.collection.find( + + query = {} + for key, val in Location(location).dict().iteritems(): + if key != 'revision' and val is None: + raise InsufficientSpecificationError(location) + + if val is not None: + query['location.{key}'.format(key=key)] = val + + item = self.collection.find_one( query, sort=[('revision', pymongo.ASCENDING)], - limit=1, ) - if items.count() > 1: - raise InsufficientSpecificationError(location) - - if items.count() == 0: + if item is None: raise ItemNotFoundError(location) - return XModuleDescriptor.load_from_json(items[0], self.get_item) + return XModuleDescriptor.load_from_json(item, XModuleSystem(self.get_item)) def create_item(self, location, editor): """ @@ -72,7 +71,7 @@ class MongoKeyStore(KeyStore): # atomic update syntax self.collection.update( {'location': Location(location).dict()}, - {'$set': {'data': data}} + {'$set': {'definition.data': data}} ) def update_children(self, location, children): @@ -88,5 +87,5 @@ class MongoKeyStore(KeyStore): # atomic update syntax self.collection.update( {'location': Location(location).dict()}, - {'$set': {'children': children}} + {'$set': {'definition.children': children}} ) diff --git a/common/lib/xmodule/seq_module.py b/common/lib/xmodule/seq_module.py index 91ff6d2671..b394227aa7 100644 --- a/common/lib/xmodule/seq_module.py +++ b/common/lib/xmodule/seq_module.py @@ -97,22 +97,5 @@ class Module(XModule): self.rendered = False -class WeekDescriptor(XModuleDescriptor): - - def get_goals(self): - """ - Return a list of Goal XModuleDescriptors that are children - of this Week - """ - return [child for child in self.get_children() if child.type == 'Goal'] - - def get_non_goals(self): - """ - Return a list of non-Goal XModuleDescriptors that are children of - this Week - """ - return [child for child in self.get_children() if child.type != 'Goal'] - - class SectionDescriptor(XModuleDescriptor): pass diff --git a/common/lib/xmodule/setup.py b/common/lib/xmodule/setup.py index 1140037259..7f3370ed37 100644 --- a/common/lib/xmodule/setup.py +++ b/common/lib/xmodule/setup.py @@ -5,10 +5,13 @@ setup( version="0.1", packages=find_packages(), install_requires=['distribute'], + + # See http://guide.python-distribute.org/creation.html#entry-points + # for a description of entry_points entry_points={ 'xmodule.v1': [ "Course = seq_module:SectionDescriptor", - "Week = seq_module:WeekDescriptor", + "Week = seq_module:SectionDescriptor", "Section = seq_module:SectionDescriptor", "LectureSequence = seq_module:SectionDescriptor", "Lab = seq_module:SectionDescriptor", diff --git a/common/lib/xmodule/x_module.py b/common/lib/xmodule/x_module.py index 3560eecbdc..9f960843d9 100644 --- a/common/lib/xmodule/x_module.py +++ b/common/lib/xmodule/x_module.py @@ -21,7 +21,7 @@ class Plugin(object): 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]))) + classes=", ".join(class_.module_name for class_ in classes))) if len(classes) == 0: raise ModuleMissingError(identifier) @@ -125,47 +125,82 @@ class XModule(object): class XModuleDescriptor(Plugin): + """ + An XModuleDescriptor is a specification for an element of a course. This could + be a problem, an organizational element (a group of content), or a segment of video, + for example. + XModuleDescriptors are independent and agnostic to the current student state on a + problem. They handle the editing interface used by instructors to create a problem, + and can generate XModules (which do know about student state). + """ entry_point = "xmodule.v1" @staticmethod - def load_from_json(json_data, load_item): + def load_from_json(json_data, system): + """ + This method instantiates the correct subclass of XModuleDescriptor based + on the contents of json_data. + + json_data must contain a 'location' element, and must be suitable to be + passed into the subclasses `from_json` method. + """ class_ = XModuleDescriptor.load_class(json_data['location']['category']) - return class_.from_json(json_data, load_item) + return class_.from_json(json_data, system) @classmethod - def from_json(cls, json_data, load_item): + def from_json(cls, json_data, system): """ Creates an instance of this descriptor from the supplied json_data. + This may be overridden by subclasses json_data: Json data specifying the data, children, and metadata for the descriptor - load_item: A function that takes an i4x url and returns a module descriptor + system: An XModuleSystem for interacting with external resources """ - return cls(load_item=load_item, **json_data) + return cls(system=system, **json_data) def __init__(self, - load_item, - data=None, - children=None, + system, + definition=None, **kwargs): - self.load_item = load_item - self.data = data if data is not None else {} - self.children = children if children is not None else [] + """ + Construct a new XModuleDescriptor. The only required arguments are the + system, used for interaction with external resources, and the definition, + which specifies all the data needed to edit and display the problem (but none + of the associated metadata that handles recordkeeping around the problem). + + This allows for maximal flexibility to add to the interface while preserving + backwards compatibility. + + system: An XModuleSystem for interacting with external resources + definition: A dict containing `data` and `children` representing the problem definition + + Current arguments passed in kwargs: + location: A keystore.Location object indicating the name and ownership of this problem + goals: A list of strings of learning goals associated with this module + """ + self.system = system + self.definition = definition if definition is not None else {} self.name = Location(kwargs.get('location')).name self.type = Location(kwargs.get('location')).category + + # For now, we represent goals as a list of strings, but this + # is one of the things that we are going to be iterating on heavily + # to find the best teaching method + self.goals = kwargs.get('goals', []) + self._child_instances = None def get_children(self, categories=None): """Returns a list of XModuleDescriptor instances for the children of this module""" if self._child_instances is None: - self._child_instances = [self.load_item(child) for child in self.children] + self._child_instances = [self.system.load_item(child) for child in self.definition['children']] if categories is None: return self._child_instances else: return [child for child in self._child_instances if child.type in categories] - def get_xml(self): ''' For conversions between JSON and legacy XML representations. ''' @@ -192,3 +227,12 @@ class XModuleDescriptor(Plugin): # Full ==> what we edit # ''' # raise NotImplementedError + + +class DescriptorSystem(object): + def __init__(self, load_item): + """ + load_item: Takes a Location and returns and XModuleDescriptor + """ + + self.load_item = load_item diff --git a/doc/overview.md b/doc/overview.md index 304d5161b0..6d187dca91 100644 --- a/doc/overview.md +++ b/doc/overview.md @@ -44,10 +44,27 @@ You should be familiar with the following. If you're not, go read some docs... ### Common libraries -- x_modules -- generic learning modules. *x* can be sequence, video, template, html, vertical, capa, etc. These are the things that one puts inside sections in the course structure. Modules know how to render themselves to html, how to score themselves, and handle ajax calls from the front end. - - x_modules take a 'system context' parameter, which helps isolate xmodules from any particular application, so they can be used in many places. The modules should make no references to Django (though there are still a few left). The system context knows how to render things, track events, complain about 404s, etc. - - TODO: document the system context interface--it's different in `x_module.XModule.__init__` and in `x_module tests.py` (do this in the code, not here) - - in `common/lib/xmodule` +- xmodule: generic learning modules. *x* can be sequence, video, template, html, + vertical, capa, etc. These are the things that one puts inside sections + in the course structure. + + - XModuleDescriptor: This defines the problem and all data and UI needed to edit + that problem. It is unaware of any student data, but can be used to retrieve + an XModule, which is aware of that student state. + + - XModule: The XModule is a problem instance that is particular to a student. It knows + how to render itself to html to display the problem, how to score itself, + and how to handle ajax calls from the front end. + + - Both XModule and XModuleDescriptor take system context parameters. These are named + ModuleSystem and DescriptorSystem respectively. These help isolate the XModules + from any interactions with external resources that they require. + + For instance, the DescriptorSystem has a function to load an XModuleDescriptor + from a Location object, and the ModuleSystem knows how to render things, + track events, and complain about 404s + - TODO: document the system context interface--it's different in `x_module.XModule.__init__` and in `x_module tests.py` (do this in the code, not here) + - in `common/lib/xmodule` - capa modules -- defines `LoncapaProblem` and many related things. - in `common/lib/capa` @@ -76,7 +93,14 @@ The LMS is a django site, with root in `lms/`. It runs in many different enviro - See `lms/urls.py` for the wirings of urls to views. -- Tracking: there is support for basic tracking of client-side events in `lms/djangoapps/track`. +- Tracking: there is support for basic tracking of client-side events in `lms/djangoapps/track`. + +### CMS + +The CMS is a django site, with root in `cms`. It can run in a number of different +environments, defined in `cms/envs`. + +- Core rendering path: Still TBD ### Other modules From 7914baccaa660617b1bf306d94c8cfbe4b848beb Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 19 Jun 2012 11:36:22 -0400 Subject: [PATCH 10/14] Change name of XModuleSystem to DescriptorSystem at usage sites --- common/lib/keystore/mongo.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/common/lib/keystore/mongo.py b/common/lib/keystore/mongo.py index 1bef298fde..29115a33a7 100644 --- a/common/lib/keystore/mongo.py +++ b/common/lib/keystore/mongo.py @@ -1,7 +1,7 @@ import pymongo from . import ModuleStore, Location from .exceptions import ItemNotFoundError, InsufficientSpecificationError -from xmodule.x_module import XModuleDescriptor, XModuleSystem +from xmodule.x_module import XModuleDescriptor, DescriptorSystem class MongoModuleStore(ModuleStore): @@ -45,7 +45,7 @@ class MongoModuleStore(ModuleStore): if item is None: raise ItemNotFoundError(location) - return XModuleDescriptor.load_from_json(item, XModuleSystem(self.get_item)) + return XModuleDescriptor.load_from_json(item, DescriptorSystem(self.get_item)) def create_item(self, location, editor): """ From b91a1d48b6ef79c6ee163025786a135019a4cbda Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 19 Jun 2012 11:44:54 -0400 Subject: [PATCH 11/14] Remove reference to instructor module --- cms/envs/common.py | 1 - 1 file changed, 1 deletion(-) diff --git a/cms/envs/common.py b/cms/envs/common.py index 80056af1d2..c3c8ee85a8 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -142,5 +142,4 @@ INSTALLED_APPS = ( 'django.contrib.messages', 'django.contrib.staticfiles', 'contentstore', - 'instructor', ) From 47bf71ae10ad3c8814debe661768972cbbf94f05 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 19 Jun 2012 14:17:05 -0400 Subject: [PATCH 12/14] Remove extra word in doc string --- common/lib/keystore/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/lib/keystore/__init__.py b/common/lib/keystore/__init__.py index 0e77a02a87..fc06a4d780 100644 --- a/common/lib/keystore/__init__.py +++ b/common/lib/keystore/__init__.py @@ -123,7 +123,7 @@ class ModuleStore(object): def get_item(self, location): """ Returns an XModuleDescriptor instance for the item at location. - If location.revision is None, returns the most item with the most + If location.revision is None, returns the item with the most recent revision If any segment of the location is None except revision, raises From 282372736d7869b601fd11a9d88726fd819365fc Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 19 Jun 2012 14:24:14 -0400 Subject: [PATCH 13/14] Add comment about None in Locations --- common/lib/keystore/__init__.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/common/lib/keystore/__init__.py b/common/lib/keystore/__init__.py index fc06a4d780..2605424517 100644 --- a/common/lib/keystore/__init__.py +++ b/common/lib/keystore/__init__.py @@ -44,6 +44,9 @@ class Location(object): Location: another Location object None of the components of a location may contain the '/' character + + Components may be set to None, which may be interpreted by some contexts to mean + wildcard selection """ self.update(location) From ed6a658afeb088df13739d7b72b63022d69eb614 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 20 Jun 2012 08:33:46 -0400 Subject: [PATCH 14/14] Add note about optional revision in location --- common/lib/keystore/__init__.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/common/lib/keystore/__init__.py b/common/lib/keystore/__init__.py index 2605424517..801ba57f80 100644 --- a/common/lib/keystore/__init__.py +++ b/common/lib/keystore/__init__.py @@ -43,6 +43,8 @@ class Location(object): } Location: another Location object + In both the dict and list forms, the revision is optional, and can be ommitted. + None of the components of a location may contain the '/' character Components may be set to None, which may be interpreted by some contexts to mean