diff --git a/brew-formulas.txt b/brew-formulas.txt index 61e3d33e3a..0aed9645d0 100644 --- a/brew-formulas.txt +++ b/brew-formulas.txt @@ -6,3 +6,4 @@ gfortran python yuicompressor node +graphviz diff --git a/cms/static/sass/_base.scss b/cms/static/sass/_base.scss index 2ea98473d1..7d16f0c6f9 100644 --- a/cms/static/sass/_base.scss +++ b/cms/static/sass/_base.scss @@ -12,7 +12,6 @@ $bright-blue: #3c8ebf; $orange: #f96e5b; $yellow: #fff8af; $cream: #F6EFD4; -$mit-red: #933; $border-color: #ddd; @mixin hide-text { diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index a99b46fd13..cab57e6819 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -102,7 +102,7 @@ def main_index(request, extra_context={}, user=None): def course_from_id(course_id): """Return the CourseDescriptor corresponding to this course_id""" course_loc = CourseDescriptor.id_to_location(course_id) - return modulestore().get_item(course_loc) + return modulestore().get_instance(course_id, course_loc) def press(request): diff --git a/common/lib/capa/capa/xqueue_interface.py b/common/lib/capa/capa/xqueue_interface.py index 10f36636b8..2930eb682d 100644 --- a/common/lib/capa/capa/xqueue_interface.py +++ b/common/lib/capa/capa/xqueue_interface.py @@ -83,6 +83,9 @@ class XQueueInterface(object): if error and (msg == 'login_required'): # Log in, then try again self._login() + if files_to_upload is not None: + for f in files_to_upload: # Need to rewind file pointers + f.seek(0) (error, msg) = self._send_to_queue(header, body, files_to_upload) return (error, msg) diff --git a/common/lib/xmodule/xmodule/course_module.py b/common/lib/xmodule/xmodule/course_module.py index 4729de905d..a9f29b9a13 100644 --- a/common/lib/xmodule/xmodule/course_module.py +++ b/common/lib/xmodule/xmodule/course_module.py @@ -1,6 +1,7 @@ from fs.errors import ResourceNotFoundError import time import logging +import requests from lxml import etree from xmodule.util.decorators import lazyproperty @@ -15,18 +16,44 @@ class CourseDescriptor(SequenceDescriptor): module_class = SequenceModule class Textbook: - def __init__(self, title, table_of_contents_url): + def __init__(self, title, book_url): self.title = title - self.table_of_contents_url = table_of_contents_url + self.book_url = book_url + self.table_of_contents = self._get_toc_from_s3() @classmethod def from_xml_object(cls, xml_object): - return cls(xml_object.get('title'), xml_object.get('table_of_contents_url')) + return cls(xml_object.get('title'), xml_object.get('book_url')) @property def table_of_contents(self): - raw_table_of_contents = open(self.table_of_contents_url, 'r') # TODO: This will need to come from S3 - table_of_contents = etree.parse(raw_table_of_contents).getroot() + return self.table_of_contents + + def _get_toc_from_s3(self): + ''' + Accesses the textbook's table of contents (default name "toc.xml") at the URL self.book_url + + Returns XML tree representation of the table of contents + ''' + toc_url = self.book_url + 'toc.xml' + + # Get the table of contents from S3 + log.info("Retrieving textbook table of contents from %s" % toc_url) + try: + r = requests.get(toc_url) + except Exception as err: + msg = 'Error %s: Unable to retrieve textbook table of contents at %s' % (err, toc_url) + log.error(msg) + raise Exception(msg) + + # TOC is XML. Parse it + try: + table_of_contents = etree.fromstring(r.text) + except Exception as err: + msg = 'Error %s: Unable to parse XML for textbook table of contents at %s' % (err, toc_url) + log.error(msg) + raise Exception(msg) + return table_of_contents @@ -134,6 +161,10 @@ class CourseDescriptor(SequenceDescriptor): 'all_descriptors' : all_descriptors,} + @staticmethod + def make_id(org, course, url_name): + return '/'.join([org, course, url_name]) + @staticmethod def id_to_location(course_id): '''Convert the given course_id (org/course/name) to a location object. diff --git a/common/lib/xmodule/xmodule/css/capa/display.scss b/common/lib/xmodule/xmodule/css/capa/display.scss index 666ca57800..e08001f6ea 100644 --- a/common/lib/xmodule/xmodule/css/capa/display.scss +++ b/common/lib/xmodule/xmodule/css/capa/display.scss @@ -1,12 +1,6 @@ h2 { margin-top: 0; margin-bottom: 15px; - width: flex-grid(2, 9); - padding-right: flex-gutter(9); - border-right: 1px dashed #ddd; - @include box-sizing(border-box); - display: table-cell; - vertical-align: top; &.problem-header { section.staff { @@ -15,12 +9,6 @@ h2 { } } - @media screen and (max-width:1120px) { - display: block; - width: auto; - border-right: 0; - } - @media print { display: block; width: auto; @@ -29,16 +17,6 @@ h2 { } section.problem { - display: table-cell; - width: flex-grid(7, 9); - padding-left: flex-gutter(9); - - @media screen and (max-width:1120px) { - display: block; - width: auto; - padding: 0; - } - @media print { display: block; width: auto; @@ -292,4 +270,10 @@ section.problem { border: 1px solid #ccc; padding: lh(); } + + section.action { + input.save { + @extend .blue-button; + } + } } diff --git a/common/lib/xmodule/xmodule/css/video/display.scss b/common/lib/xmodule/xmodule/css/video/display.scss index 0b4cf883bf..848294b699 100644 --- a/common/lib/xmodule/xmodule/css/video/display.scss +++ b/common/lib/xmodule/xmodule/css/video/display.scss @@ -305,11 +305,11 @@ div.video { @include box-shadow(0 1px 0 #333); a.ui-slider-handle { - background: $mit-red url(../images/slider-handle.png) center center no-repeat; + background: $pink url(../images/slider-handle.png) center center no-repeat; @include background-size(50%); - border: 1px solid darken($mit-red, 20%); + border: 1px solid darken($pink, 20%); @include border-radius(15px); - @include box-shadow(inset 0 1px 0 lighten($mit-red, 10%)); + @include box-shadow(inset 0 1px 0 lighten($pink, 10%)); cursor: pointer; height: 15px; left: -6px; @@ -408,6 +408,7 @@ div.video { cursor: pointer; margin-bottom: 8px; padding: 0; + line-height: lh(); &.current { color: #333; @@ -415,7 +416,7 @@ div.video { } &:hover { - color: $mit-red; + color: $blue; } &:empty { diff --git a/common/lib/xmodule/xmodule/modulestore/__init__.py b/common/lib/xmodule/xmodule/modulestore/__init__.py index 6c77127d7e..d6dd85deea 100644 --- a/common/lib/xmodule/xmodule/modulestore/__init__.py +++ b/common/lib/xmodule/xmodule/modulestore/__init__.py @@ -223,6 +223,13 @@ class ModuleStore(object): """ raise NotImplementedError + def get_instance(self, course_id, location): + """ + Get an instance of this location, with policy for course_id applied. + TODO (vshnayder): this may want to live outside the modulestore eventually + """ + raise NotImplementedError + def get_item_errors(self, location): """ Return a list of (msg, exception-or-None) errors that the modulestore @@ -331,7 +338,8 @@ class ModuleStoreBase(ModuleStore): and datastores. """ # check that item is present and raise the promised exceptions if needed - self.get_item(location) + # TODO (vshnayder): post-launch, make errors properties of items + #self.get_item(location) errorlog = self._get_errorlog(location) return errorlog.errors diff --git a/common/lib/xmodule/xmodule/modulestore/mongo.py b/common/lib/xmodule/xmodule/modulestore/mongo.py index caada39f3e..7aa05e474f 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo.py @@ -217,6 +217,13 @@ class MongoModuleStore(ModuleStoreBase): item = self._find_one(location) return self._load_items([item], depth)[0] + def get_instance(self, course_id, location): + """ + TODO (vshnayder): implement policy tracking in mongo. + For now, just delegate to get_item and ignore policy. + """ + return self.get_item(location) + def get_items(self, location, depth=0): items = self.collection.find( location_to_query(location), diff --git a/common/lib/xmodule/xmodule/modulestore/xml.py b/common/lib/xmodule/xmodule/modulestore/xml.py index a77266113f..e0fecb243d 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml.py +++ b/common/lib/xmodule/xmodule/modulestore/xml.py @@ -3,6 +3,7 @@ import logging import os import re +from collections import defaultdict from fs.osfs import OSFS from importlib import import_module from lxml import etree @@ -33,7 +34,7 @@ def clean_out_mako_templating(xml_string): return xml_string class ImportSystem(XMLParsingSystem, MakoDescriptorSystem): - def __init__(self, xmlstore, org, course, course_dir, + def __init__(self, xmlstore, course_id, course_dir, policy, error_tracker, **kwargs): """ A class that handles loading from xml. Does some munging to ensure that @@ -43,6 +44,7 @@ class ImportSystem(XMLParsingSystem, MakoDescriptorSystem): """ self.unnamed_modules = 0 self.used_slugs = set() + self.org, self.course, self.url_name = course_id.split('/') def process_xml(xml): """Takes an xml string, and returns a XModuleDescriptor created from @@ -80,21 +82,24 @@ class ImportSystem(XMLParsingSystem, MakoDescriptorSystem): xml_data.set('url_name', slug) descriptor = XModuleDescriptor.load_from_xml( - etree.tostring(xml_data), self, org, - course, xmlstore.default_class) + etree.tostring(xml_data), self, self.org, + self.course, xmlstore.default_class) #log.debug('==> importing descriptor location %s' % # repr(descriptor.location)) descriptor.metadata['data_dir'] = course_dir - xmlstore.modules[descriptor.location] = descriptor + xmlstore.modules[course_id][descriptor.location] = descriptor if xmlstore.eager: descriptor.get_children() return descriptor render_template = lambda: '' - load_item = xmlstore.get_item + # TODO (vshnayder): we are somewhat architecturally confused in the loading code: + # load_item should actually be get_instance, because it expects the course-specific + # 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, @@ -127,7 +132,7 @@ class XMLModuleStore(ModuleStoreBase): self.eager = eager self.data_dir = path(data_dir) - self.modules = {} # location -> XModuleDescriptor + self.modules = defaultdict(dict) # course_id -> dict(location -> XModuleDescriptor) self.courses = {} # course_dir -> XModuleDescriptor for the course if default_class is None: @@ -236,14 +241,24 @@ class XMLModuleStore(ModuleStoreBase): tracker(msg) course = course_dir - url_name = course_data.get('url_name') + url_name = course_data.get('url_name', course_data.get('slug')) if url_name: policy_path = self.data_dir / course_dir / 'policies' / '{0}.json'.format(url_name) policy = self.load_policy(policy_path, tracker) else: policy = {} + # VS[compat] : 'name' is deprecated, but support it for now... + if course_data.get('name'): + url_name = Location.clean(course_data.get('name')) + tracker("'name' is deprecated for module xml. Please use " + "display_name and url_name.") + else: + raise ValueError("Can't load a course without a 'url_name' " + "(or 'name') set. Set url_name.") - system = ImportSystem(self, org, course, course_dir, policy, tracker) + + course_id = CourseDescriptor.make_id(org, course, url_name) + system = ImportSystem(self, course_id, course_dir, policy, tracker) course_descriptor = system.process_xml(etree.tostring(course_data)) @@ -257,6 +272,27 @@ class XMLModuleStore(ModuleStoreBase): return course_descriptor + def get_instance(self, course_id, location, depth=0): + """ + Returns an XModuleDescriptor instance for the item at + location, with the policy for course_id. (In case two xml + dirs have different content at the same location, return the + one for this course_id.) + + If any segment of the location is None except revision, raises + xmodule.modulestore.exceptions.InsufficientSpecificationError + + If no object is found at that location, raises + xmodule.modulestore.exceptions.ItemNotFoundError + + location: Something that can be passed to Location + """ + location = Location(location) + try: + return self.modules[course_id][location] + except KeyError: + raise ItemNotFoundError(location) + def get_item(self, location, depth=0): """ Returns an XModuleDescriptor instance for the item at location. @@ -271,11 +307,8 @@ class XMLModuleStore(ModuleStoreBase): location: Something that can be passed to Location """ - location = Location(location) - try: - return self.modules[location] - except KeyError: - raise ItemNotFoundError(location) + raise NotImplementedError("XMLModuleStores can't guarantee that definitions" + " are unique. Use get_instance.") def get_courses(self, depth=0): diff --git a/common/lib/xmodule/xmodule/modulestore/xml_importer.py b/common/lib/xmodule/xmodule/modulestore/xml_importer.py index 891db7e994..89f94d8cdb 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml_importer.py +++ b/common/lib/xmodule/xmodule/modulestore/xml_importer.py @@ -22,21 +22,22 @@ def import_from_xml(store, data_dir, course_dirs=None, eager=True, eager=eager, course_dirs=course_dirs ) - for module in module_store.modules.itervalues(): + for course_id in module_store.modules.keys(): + for module in module_store.modules[course_id].itervalues(): - # TODO (cpennington): This forces import to overrite the same items. - # This should in the future create new revisions of the items on import - try: - store.create_item(module.location) - except DuplicateItemError: - log.exception('Item already exists at %s' % module.location.url()) - pass - if 'data' in module.definition: - store.update_item(module.location, module.definition['data']) - if 'children' in module.definition: - store.update_children(module.location, module.definition['children']) - # NOTE: It's important to use own_metadata here to avoid writing - # inherited metadata everywhere. - store.update_metadata(module.location, dict(module.own_metadata)) + # TODO (cpennington): This forces import to overrite the same items. + # This should in the future create new revisions of the items on import + try: + store.create_item(module.location) + except DuplicateItemError: + log.exception('Item already exists at %s' % module.location.url()) + pass + if 'data' in module.definition: + store.update_item(module.location, module.definition['data']) + if 'children' in module.definition: + store.update_children(module.location, module.definition['children']) + # NOTE: It's important to use own_metadata here to avoid writing + # inherited metadata everywhere. + store.update_metadata(module.location, dict(module.own_metadata)) return module_store diff --git a/common/lib/xmodule/xmodule/tests/__init__.py b/common/lib/xmodule/xmodule/tests/__init__.py index a3eac0258e..2a380bb8be 100644 --- a/common/lib/xmodule/xmodule/tests/__init__.py +++ b/common/lib/xmodule/xmodule/tests/__init__.py @@ -8,6 +8,7 @@ import unittest import os import fs +import fs.osfs import json import json diff --git a/common/lib/xmodule/xmodule/tests/test_export.py b/common/lib/xmodule/xmodule/tests/test_export.py index 268c3d1062..2520d95937 100644 --- a/common/lib/xmodule/xmodule/tests/test_export.py +++ b/common/lib/xmodule/xmodule/tests/test_export.py @@ -84,20 +84,22 @@ class RoundTripTestCase(unittest.TestCase): strip_filenames(exported_course) self.assertEquals(initial_course, exported_course) + self.assertEquals(initial_course.id, exported_course.id) + course_id = initial_course.id print "Checking key equality" - self.assertEquals(sorted(initial_import.modules.keys()), - sorted(second_import.modules.keys())) + self.assertEquals(sorted(initial_import.modules[course_id].keys()), + sorted(second_import.modules[course_id].keys())) print "Checking module equality" - for location in initial_import.modules.keys(): + for location in initial_import.modules[course_id].keys(): print "Checking", location if location.category == 'html': print ("Skipping html modules--they can't import in" " final form without writing files...") continue - self.assertEquals(initial_import.modules[location], - second_import.modules[location]) + self.assertEquals(initial_import.modules[course_id][location], + second_import.modules[course_id][location]) def setUp(self): diff --git a/common/lib/xmodule/xmodule/tests/test_import.py b/common/lib/xmodule/xmodule/tests/test_import.py index 2061b63fb6..34e4767a62 100644 --- a/common/lib/xmodule/xmodule/tests/test_import.py +++ b/common/lib/xmodule/xmodule/tests/test_import.py @@ -207,3 +207,48 @@ class ImportTestCase(unittest.TestCase): check_for_key(key, c) check_for_key('graceperiod', course) + + + def test_policy_loading(self): + """Make sure that when two courses share content with the same + org and course names, policy applies to the right one.""" + + def get_course(name): + print "Importing {0}".format(name) + + modulestore = XMLModuleStore(DATA_DIR, eager=True, course_dirs=[name]) + courses = modulestore.get_courses() + self.assertEquals(len(courses), 1) + return courses[0] + + toy = get_course('toy') + two_toys = get_course('two_toys') + + self.assertEqual(toy.url_name, "2012_Fall") + self.assertEqual(two_toys.url_name, "TT_2012_Fall") + + toy_ch = toy.get_children()[0] + two_toys_ch = two_toys.get_children()[0] + + self.assertEqual(toy_ch.display_name, "Overview") + self.assertEqual(two_toys_ch.display_name, "Two Toy Overview") + + + def test_definition_loading(self): + """When two courses share the same org and course name and + both have a module with the same url_name, the definitions shouldn't clash. + + TODO (vshnayder): once we have a CMS, this shouldn't + happen--locations should uniquely name definitions. But in + our imperfect XML world, it can (and likely will) happen.""" + + modulestore = XMLModuleStore(DATA_DIR, eager=True, course_dirs=['toy', 'two_toys']) + + toy_id = "edX/toy/2012_Fall" + two_toy_id = "edX/toy/TT_2012_Fall" + + location = Location(["i4x", "edX", "toy", "video", "Welcome"]) + toy_video = modulestore.get_instance(toy_id, location) + two_toy_video = modulestore.get_instance(two_toy_id, location) + self.assertEqual(toy_video.metadata['youtube'], "1.0:p2Q6BrNhdh8") + self.assertEqual(two_toy_video.metadata['youtube'], "1.0:p2Q6BrNhdh9") diff --git a/common/test/data/two_toys/README b/common/test/data/two_toys/README new file mode 100644 index 0000000000..ef1a501ee8 --- /dev/null +++ b/common/test/data/two_toys/README @@ -0,0 +1 @@ +A copy of the toy course, with different metadata. Used to test policy loading for course with identical org course fields and shared content. diff --git a/common/test/data/two_toys/chapter/Overview.xml b/common/test/data/two_toys/chapter/Overview.xml new file mode 100644 index 0000000000..a738a9eec3 --- /dev/null +++ b/common/test/data/two_toys/chapter/Overview.xml @@ -0,0 +1,4 @@ + + + diff --git a/common/test/data/two_toys/course.xml b/common/test/data/two_toys/course.xml new file mode 100644 index 0000000000..655859ab8b --- /dev/null +++ b/common/test/data/two_toys/course.xml @@ -0,0 +1 @@ + diff --git a/common/test/data/two_toys/course/TT_2012_Fall.xml b/common/test/data/two_toys/course/TT_2012_Fall.xml new file mode 100644 index 0000000000..b6dbd48b4c --- /dev/null +++ b/common/test/data/two_toys/course/TT_2012_Fall.xml @@ -0,0 +1,3 @@ + + + diff --git a/common/test/data/two_toys/policies/TT_2012_Fall.json b/common/test/data/two_toys/policies/TT_2012_Fall.json new file mode 100644 index 0000000000..2831767b32 --- /dev/null +++ b/common/test/data/two_toys/policies/TT_2012_Fall.json @@ -0,0 +1,23 @@ +{ + "course/TT_2012_Fall": { + "graceperiod": "2 days 5 hours 59 minutes 59 seconds", + "start": "2015-07-17T12:00", + "display_name": "Two Toys Course" + }, + "chapter/Overview": { + "display_name": "Two Toy Overview" + }, + "videosequence/Toy_Videos": { + "display_name": "Toy Videos", + "format": "Lecture Sequence" + }, + "html/toylab": { + "display_name": "Toy lab" + }, + "video/Video_Resources": { + "display_name": "Video Resources" + }, + "video/Welcome": { + "display_name": "Welcome" + } +} diff --git a/common/test/data/two_toys/video/Video_Resources.xml b/common/test/data/two_toys/video/Video_Resources.xml new file mode 100644 index 0000000000..88657ed79f --- /dev/null +++ b/common/test/data/two_toys/video/Video_Resources.xml @@ -0,0 +1 @@ +