Merge master

common/djangoapps/external_auth/admin.py

@@ -0,0 +1,8 @@
+'''
+django admin pages for courseware model
+'''
+
+from external_auth.models import *
+from django.contrib import admin
+
+admin.site.register(ExternalAuthMap)

common/djangoapps/external_auth/models.py

@@ -0,0 +1,31 @@
+"""
+WE'RE USING MIGRATIONS!
+
+If you make changes to this model, be sure to create an appropriate migration
+file and check it in at the same time as your model changes. To do that,
+
+1. Go to the mitx dir
+2. django-admin.py schemamigration student --auto --settings=lms.envs.dev --pythonpath=. description_of_your_change
+3. Add the migration file created in mitx/common/djangoapps/external_auth/migrations/
+"""
+
+from django.db import models
+from django.contrib.auth.models import User
+
+class ExternalAuthMap(models.Model):
+    class Meta:
+        unique_together = (('external_id', 'external_domain'), )
+    external_id = models.CharField(max_length=255, db_index=True)
+    external_domain = models.CharField(max_length=255, db_index=True)
+    external_credentials = models.TextField(blank=True)  # JSON dictionary 
+    external_email = models.CharField(max_length=255, db_index=True)
+    external_name = models.CharField(blank=True,max_length=255, db_index=True)
+    user = models.OneToOneField(User, unique=True, db_index=True, null=True)
+    internal_password = models.CharField(blank=True, max_length=31)	# randomly generated
+    dtcreated = models.DateTimeField('creation date',auto_now_add=True)
+    dtsignup = models.DateTimeField('signup date',null=True)		# set after signup
+    
+    def __unicode__(self):
+        s = "[%s] = (%s / %s)" % (self.external_id, self.external_name, self.external_email)
+        return s
+    

common/djangoapps/external_auth/views.py

@@ -0,0 +1,219 @@
+import json
+import logging
+import random
+import re
+import string
+
+from external_auth.models import ExternalAuthMap
+
+from django.conf import settings
+from django.contrib.auth import REDIRECT_FIELD_NAME, authenticate, login
+from django.contrib.auth.models import Group
+from django.contrib.auth.models import User
+
+from django.core.urlresolvers import reverse
+from django.http import HttpResponse, HttpResponseRedirect
+from django.shortcuts import render_to_response
+from django.shortcuts import redirect
+from django.template import RequestContext
+from mitxmako.shortcuts import render_to_response, render_to_string
+try:
+    from django.views.decorators.csrf import csrf_exempt
+except ImportError:
+    from django.contrib.csrf.middleware import csrf_exempt
+from django_future.csrf import ensure_csrf_cookie
+from util.cache import cache_if_anonymous
+    
+from django_openid_auth import auth as openid_auth
+from openid.consumer.consumer import (Consumer, SUCCESS, CANCEL, FAILURE)
+import django_openid_auth.views as openid_views
+
+import student.views as student_views
+
+log = logging.getLogger("mitx.external_auth")
+
+@csrf_exempt
+def default_render_failure(request, message, status=403, template_name='extauth_failure.html', exception=None):
+    """Render an Openid error page to the user."""
+    message = "In openid_failure " + message
+    log.debug(message)
+    data = render_to_string( template_name, dict(message=message, exception=exception))
+    return HttpResponse(data, status=status)
+
+#-----------------------------------------------------------------------------
+# Openid
+
+def edXauth_generate_password(length=12, chars=string.letters + string.digits):
+    """Generate internal password for externally authenticated user"""
+    return ''.join([random.choice(chars) for i in range(length)])
+
+@csrf_exempt
+def edXauth_openid_login_complete(request,  redirect_field_name=REDIRECT_FIELD_NAME, render_failure=None):
+    """Complete the openid login process"""
+
+    redirect_to = request.REQUEST.get(redirect_field_name, '')
+    render_failure = render_failure or \
+                     getattr(settings, 'OPENID_RENDER_FAILURE', None) or \
+                     default_render_failure
+                                                   
+    openid_response = openid_views.parse_openid_response(request)
+    if not openid_response:
+        return render_failure(request, 'This is an OpenID relying party endpoint.')
+
+    if openid_response.status == SUCCESS:
+        external_id = openid_response.identity_url
+        oid_backend =  openid_auth.OpenIDBackend()
+        details = oid_backend._extract_user_details(openid_response)
+
+        log.debug('openid success, details=%s' % details)
+
+        return edXauth_external_login_or_signup(request,
+                                                external_id, 
+                                                "openid:%s" % settings.OPENID_SSO_SERVER_URL,
+                                                details,
+                                                details.get('email',''),
+                                                '%s %s' % (details.get('first_name',''),details.get('last_name',''))
+                                                )
+                                 
+    return render_failure(request, 'Openid failure')
+
+#-----------------------------------------------------------------------------
+# generic external auth login or signup
+
+def edXauth_external_login_or_signup(request, external_id, external_domain, credentials, email, fullname,
+                                     retfun=None):
+    # see if we have a map from this external_id to an edX username
+    try:
+        eamap = ExternalAuthMap.objects.get(external_id = external_id,
+                                            external_domain = external_domain,
+                                            )
+        log.debug('Found eamap=%s' % eamap)
+    except ExternalAuthMap.DoesNotExist:
+        # go render form for creating edX user
+        eamap = ExternalAuthMap(external_id = external_id,
+                                external_domain = external_domain,
+                                external_credentials = json.dumps(credentials),
+                                )
+        eamap.external_email = email
+        eamap.external_name = fullname
+        eamap.internal_password = edXauth_generate_password()
+        log.debug('created eamap=%s' % eamap)
+
+        eamap.save()
+
+    internal_user = eamap.user
+    if internal_user is None:
+        log.debug('ExtAuth: no user for %s yet, doing signup' % eamap.external_email)
+        return edXauth_signup(request, eamap)
+    
+    uname = internal_user.username
+    user = authenticate(username=uname, password=eamap.internal_password)
+    if user is None:
+        log.warning("External Auth Login failed for %s / %s" % (uname,eamap.internal_password))
+        return edXauth_signup(request, eamap)
+
+    if not user.is_active:
+        log.warning("External Auth: user %s is not active" % (uname))
+        # TODO: improve error page
+        return render_failure(request, 'Account not yet activated: please look for link in your email')
+     
+    login(request, user)
+    request.session.set_expiry(0)
+    student_views.try_change_enrollment(request)
+    log.info("Login success - {0} ({1})".format(user.username, user.email))
+    if retfun is None:
+        return redirect('/')
+    return retfun()
+        
+    
+#-----------------------------------------------------------------------------
+# generic external auth signup
+
+@ensure_csrf_cookie
+@cache_if_anonymous
+def edXauth_signup(request, eamap=None):
+    """
+    Present form to complete for signup via external authentication.
+    Even though the user has external credentials, he/she still needs
+    to create an account on the edX system, and fill in the user
+    registration form.
+
+    eamap is an ExteralAuthMap object, specifying the external user
+    for which to complete the signup.
+    """
+    
+    if eamap is None:
+        pass
+
+    request.session['ExternalAuthMap'] = eamap	# save this for use by student.views.create_account
+    
+    context = {'has_extauth_info': True,
+               'show_signup_immediately' : True,
+               'extauth_email': eamap.external_email,
+               'extauth_username' : eamap.external_name.replace(' ',''), # default - conjoin name, no spaces
+               'extauth_name': eamap.external_name,
+               }
+    
+    log.debug('ExtAuth: doing signup for %s' % eamap.external_email)
+
+    return student_views.main_index(extra_context=context)
+
+#-----------------------------------------------------------------------------
+# MIT SSL
+
+def ssl_dn_extract_info(dn):
+    '''
+    Extract username, email address (may be anyuser@anydomain.com) and full name
+    from the SSL DN string.  Return (user,email,fullname) if successful, and None
+    otherwise.
+    '''
+    ss = re.search('/emailAddress=(.*)@([^/]+)', dn)
+    if ss:
+        user = ss.group(1)
+        email = "%s@%s" % (user, ss.group(2))
+    else:
+        return None
+    ss = re.search('/CN=([^/]+)/', dn)
+    if ss:
+        fullname = ss.group(1)
+    else:
+        return None
+    return (user, email, fullname)
+
+@csrf_exempt
+def edXauth_ssl_login(request):
+    """
+    This is called by student.views.index when MITX_FEATURES['AUTH_USE_MIT_CERTIFICATES'] = True
+
+    Used for MIT user authentication.  This presumes the web server (nginx) has been configured 
+    to require specific client certificates.
+
+    If the incoming protocol is HTTPS (SSL) then authenticate via client certificate.  
+    The certificate provides user email and fullname; this populates the ExternalAuthMap.
+    The user is nevertheless still asked to complete the edX signup.  
+
+    Else continues on with student.views.main_index, and no authentication.
+    """
+    certkey = "SSL_CLIENT_S_DN"	 # specify the request.META field to use
+    
+    cert = request.META.get(certkey,'')
+    if not cert:
+        cert = request.META.get('HTTP_'+certkey,'')
+    if not cert:
+        try:
+            cert = request._req.subprocess_env.get(certkey,'')	 # try the direct apache2 SSL key
+        except Exception as err:
+            pass
+    if not cert:
+        # no certificate information - go onward to main index
+        return student_views.main_index()
+
+    (user, email, fullname) = ssl_dn_extract_info(cert)
+    
+    return edXauth_external_login_or_signup(request, 
+                                            external_id=email, 
+                                            external_domain="ssl:MIT", 
+                                            credentials=cert, 
+                                            email=email,
+                                            fullname=fullname,
+                                            retfun = student_views.main_index)

common/djangoapps/student/views.py

@@ -8,7 +8,6 @@ import uuid
 import feedparser
 import urllib
 import itertools
-from collections import defaultdict
 
 from django.conf import settings
 from django.contrib.auth import logout, authenticate, login
@@ -23,7 +22,7 @@ from django.http import HttpResponse, Http404
 from django.shortcuts import redirect
 from mitxmako.shortcuts import render_to_response, render_to_string
 from django.core.urlresolvers import reverse
-from BeautifulSoup import BeautifulSoup
+from bs4 import BeautifulSoup
 from django.core.cache import cache
 
 from django_future.csrf import ensure_csrf_cookie
@@ -37,6 +36,7 @@ from xmodule.modulestore.exceptions import ItemNotFoundError
 from models import Registration, UserProfile, PendingNameChange, PendingEmailChange, CourseEnrollment
 from datetime import date
 from collections import namedtuple
+from courseware.courses import course_staff_group_name, has_staff_access_to_course, get_courses_by_university
 
 log = logging.getLogger("mitx.student")
 Article = namedtuple('Article', 'title url author image deck publication publish_date')
@@ -60,6 +60,19 @@ def index(request):
     if settings.COURSEWARE_ENABLED and request.user.is_authenticated():
         return redirect(reverse('dashboard'))
 
+    if settings.MITX_FEATURES.get('AUTH_USE_MIT_CERTIFICATES'):
+        from external_auth.views import edXauth_ssl_login
+        return edXauth_ssl_login(request)
+
+    return main_index(user=request.user)
+
+def main_index(extra_context = {}, user=None):
+    '''
+    Render the edX main page.
+
+    extra_context is used to allow immediate display of certain modal windows, eg signup,
+    as used by external_auth.
+    '''
     feed_data = cache.get("students_index_rss_feed_data")
     if feed_data == None:
         if hasattr(settings, 'RSS_URL'):
@@ -75,13 +88,11 @@ def index(request):
         entry.image = soup.img['src'] if soup.img else None
         entry.summary = soup.getText()
 
-    universities = defaultdict(list)
-    courses = sorted(modulestore().get_courses(), key=lambda course: course.number)
-    for course in courses:
-        universities[course.org].append(course)
-
-    return render_to_response('index.html', {'universities': universities, 'entries': entries})
-
+    # The course selection work is done in courseware.courses.
+    universities = get_courses_by_university(None)
+    context = {'universities': universities, 'entries': entries}
+    context.update(extra_context)
+    return render_to_response('index.html', context)
 
 def course_from_id(id):
     course_loc = CourseDescriptor.id_to_location(id)
@@ -170,6 +181,14 @@ def change_enrollment(request):
                       .format(user.username, enrollment.course_id))
             return {'success': False, 'error': 'The course requested does not exist.'}
 
+        if settings.MITX_FEATURES.get('ACCESS_REQUIRE_STAFF_FOR_COURSE'):
+            # require that user be in the staff_* group (or be an overall admin) to be able to enroll
+            # eg staff_6.002x or staff_6.00x
+            if not has_staff_access_to_course(user,course):
+                staff_group = course_staff_group_name(course)
+                log.debug('user %s denied enrollment to %s ; not in %s' % (user,course.location.url(),staff_group))
+                return {'success': False, 'error' : '%s membership required to access course.' % staff_group}
+
         enrollment, created = CourseEnrollment.objects.get_or_create(user=user, course_id=course.id)
         return {'success': True}
 
@@ -256,11 +275,26 @@ def change_setting(request):
 
 @ensure_csrf_cookie
 def create_account(request, post_override=None):
-    ''' JSON call to enroll in the course. '''
+    '''
+    JSON call to create new edX account.
+    Used by form in signup_modal.html, which is included into navigation.html
+    '''
     js = {'success': False}
 
     post_vars = post_override if post_override else request.POST
 
+    # if doing signup for an external authorization, then get email, password, name from the eamap
+    # don't use the ones from the form, since the user could have hacked those
+    DoExternalAuth = 'ExternalAuthMap' in request.session
+    if DoExternalAuth:
+        eamap = request.session['ExternalAuthMap']
+        email = eamap.external_email
+        name = eamap.external_name
+        password = eamap.internal_password
+        post_vars = dict(post_vars.items())
+        post_vars.update(dict(email=email, name=name, password=password))
+        log.debug('extauth test: post_vars = %s' % post_vars)
+
     # Confirm we have a properly formed request
     for a in ['username', 'email', 'password', 'name']:
         if a not in post_vars:
@@ -355,8 +389,9 @@ def create_account(request, post_override=None):
          'key': r.activation_key,
          }
 
+    # composes activation email
     subject = render_to_string('emails/activation_email_subject.txt', d)
-        # Email subject *must not* contain newlines
+    # Email subject *must not* contain newlines
     subject = ''.join(subject.splitlines())
     message = render_to_string('emails/activation_email.txt', d)
 
@@ -381,6 +416,17 @@ def create_account(request, post_override=None):
 
     try_change_enrollment(request)
 
+    if DoExternalAuth:
+        eamap.user = login_user
+        eamap.dtsignup = datetime.datetime.now()
+        eamap.save()
+        log.debug('Updated ExternalAuthMap for %s to be %s' % (post_vars['username'],eamap))
+
+        if settings.MITX_FEATURES.get('BYPASS_ACTIVATION_EMAIL_FOR_EXTAUTH'):
+            log.debug('bypassing activation email')
+            login_user.is_active = True
+            login_user.save()
+
     js = {'success': True}
     return HttpResponse(json.dumps(js), mimetype="application/json")
 

common/djangoapps/track/models.py

@@ -1,3 +1,21 @@
 from django.db import models
 
-# Create your models here.
+from django.db import models
+
+class TrackingLog(models.Model):
+    dtcreated = models.DateTimeField('creation date',auto_now_add=True)
+    username = models.CharField(max_length=32,blank=True)
+    ip = models.CharField(max_length=32,blank=True)
+    event_source = models.CharField(max_length=32)
+    event_type = models.CharField(max_length=32,blank=True)
+    event = models.TextField(blank=True)
+    agent = models.CharField(max_length=256,blank=True)
+    page = models.CharField(max_length=32,blank=True,null=True)
+    time = models.DateTimeField('event time')
+
+    def __unicode__(self):
+        s = "[%s] %s@%s: %s | %s | %s | %s" % (self.time, self.username, self.ip, self.event_source,
+                                               self.event_type, self.page, self.event)
+        return s
+
+

common/djangoapps/track/views.py

@@ -2,19 +2,32 @@ import json
 import logging
 import os
 import datetime
+import dateutil.parser
 
-# Create your views here.
+from django.contrib.auth.decorators import login_required
 from django.http import HttpResponse
 from django.http import Http404
+from django.shortcuts import redirect
 from django.conf import settings
+from mitxmako.shortcuts import render_to_response
+
+from django_future.csrf import ensure_csrf_cookie
+from track.models import TrackingLog
 
 log = logging.getLogger("tracking")
 
+LOGFIELDS = ['username','ip','event_source','event_type','event','agent','page','time']
 
 def log_event(event):
     event_str = json.dumps(event)
     log.info(event_str[:settings.TRACK_MAX_EVENT])
-
+    if settings.MITX_FEATURES.get('ENABLE_SQL_TRACKING_LOGS'):
+        event['time'] = dateutil.parser.parse(event['time'])
+        tldat = TrackingLog(**dict( (x,event[x]) for x in LOGFIELDS ))
+        try:
+            tldat.save()
+        except Exception as err:
+            log.exception(err)
 
 def user_track(request):
     try:  # TODO: Do the same for many of the optional META parameters
@@ -70,4 +83,16 @@ def server_track(request, event_type, event, page=None):
         "page": page,
         "time": datetime.datetime.utcnow().isoformat(),
         }
+
+    if event_type=="/event_logs" and request.user.is_staff:	# don't log
+        return
     log_event(event)
+
+@login_required
+@ensure_csrf_cookie
+def view_tracking_log(request):
+    if not request.user.is_staff:
+        return redirect('/')
+    record_instances = TrackingLog.objects.all().order_by('-time')[0:100]
+    return render_to_response('tracking_log.html',{'records':record_instances})
+

common/djangoapps/xmodule_modifiers.py

@@ -1,9 +1,15 @@
+import re
 import json
+import logging
+
 from django.conf import settings
 from functools import wraps
 from static_replace import replace_urls
 from mitxmako.shortcuts import render_to_string
+from xmodule.seq_module import SequenceModule
+from xmodule.vertical_module import VerticalModule
 
+log = logging.getLogger("mitx.xmodule_modifiers")
 
 def wrap_xmodule(get_html, module, template):
     """
@@ -69,29 +75,32 @@ def add_histogram(get_html, module):
     the output of the old get_html function with additional information
     for admin users only, including a histogram of student answers and the
     definition of the xmodule
+
+    Does nothing if module is a SequenceModule
     """
     @wraps(get_html)
     def _get_html():
+
+        if type(module) in [SequenceModule, VerticalModule]:	# TODO: make this more general, eg use an XModule attribute instead
+            return get_html()
+
         module_id = module.id
         histogram = grade_histogram(module_id)
         render_histogram = len(histogram) > 0
 
-        # TODO: fixme - no filename in module.xml in general (this code block for edx4edx)
-        # the following if block is for summer 2012 edX course development; it will change when the CMS comes online
-        if settings.MITX_FEATURES.get('DISPLAY_EDIT_LINK') and settings.DEBUG and module_xml.get('filename') is not None:
-            coursename = multicourse_settings.get_coursename_from_request(request)
-            github_url = multicourse_settings.get_course_github_url(coursename)
-            fn = module_xml.get('filename')
-            if module_xml.tag=='problem': fn = 'problems/' + fn	# grrr
-            edit_link = (github_url + '/tree/master/' + fn) if github_url is not None else None
-            if module_xml.tag=='problem': edit_link += '.xml'	# grrr
+        # TODO (ichuang): Remove after fall 2012 LMS migration done
+        if settings.MITX_FEATURES.get('ENABLE_LMS_MIGRATION'):
+            [filepath, filename] = module.definition.get('filename','')
+            osfs = module.system.filestore
+            if osfs.exists(filename):
+                filepath = filename	# if original, unmangled filename exists then use it (github doesn't like symlinks)
+            data_dir = osfs.root_path.rsplit('/')[-1]
+            edit_link = "https://github.com/MITx/%s/tree/master/%s" % (data_dir,filepath)
         else:
             edit_link = False
 
-        # Cast module.definition and module.metadata to dicts so that json can dump them
-        # even though they are lazily loaded
-        staff_context = {'definition': json.dumps(dict(module.definition), indent=4),
-                         'metadata': json.dumps(dict(module.metadata), indent=4),
+        staff_context = {'definition': module.definition.get('data'),
+                         'metadata': json.dumps(module.metadata, indent=4),
                          'element_id': module.location.html_id(),
                          'edit_link': edit_link,
                          'histogram': json.dumps(histogram),
@@ -100,3 +109,4 @@ def add_histogram(get_html, module):
         return render_to_string("staff_problem_info.html", staff_context)
 
     return _get_html
+

common/lib/capa/capa/capa_problem.py

@@ -294,20 +294,30 @@ class LoncapaProblem(object):
                 try:
                     ifp = self.system.filestore.open(file)  # open using ModuleSystem OSFS filestore
                 except Exception as err:
-                    log.error('Error %s in problem xml include: %s' % (err, etree.tostring(inc, pretty_print=True)))
-                    log.error('Cannot find file %s in %s' % (file, self.system.filestore))
-                    if not self.system.get('DEBUG'):		# if debugging, don't fail - just log error
+                    log.error('Error %s in problem xml include: %s' % (
+                            err, etree.tostring(inc, pretty_print=True)))
+                    log.error('Cannot find file %s in %s' % (
+                            file, self.system.filestore))
+                    # if debugging, don't fail - just log error
+                    # TODO (vshnayder): need real error handling, display to users
+                    if not self.system.get('DEBUG'):
                         raise
-                    else: continue
+                    else:
+                        continue
                 try:
                     incxml = etree.XML(ifp.read())		# read in and convert to XML
                 except Exception as err:
-                    log.error('Error %s in problem xml include: %s' % (err, etree.tostring(inc, pretty_print=True)))
+                    log.error('Error %s in problem xml include: %s' % (
+                            err, etree.tostring(inc, pretty_print=True)))
                     log.error('Cannot parse XML in %s' % (file))
-                    if not self.system.get('DEBUG'):		# if debugging, don't fail - just log error
+                    # if debugging, don't fail - just log error
+                    # TODO (vshnayder): same as above
+                    if not self.system.get('DEBUG'):
                         raise
-                    else: continue
-                parent = inc.getparent()			# insert  new XML into tree in place of inlcude
+                    else:
+                        continue
+                # insert new XML into tree in place of inlcude
+                parent = inc.getparent()
                 parent.insert(parent.index(inc), incxml)
                 parent.remove(inc)
                 log.debug('Included %s into %s' % (file, self.problem_id))
@@ -335,7 +345,7 @@ class LoncapaProblem(object):
             # path is an absolute path or a path relative to the data dir
             dir = os.path.join(self.system.filestore.root_path, dir)
             abs_dir = os.path.normpath(dir)
-            log.debug("appending to path: %s" % abs_dir)
+            #log.debug("appending to path: %s" % abs_dir)
             path.append(abs_dir)
 
         return path

common/lib/supertrace.py

@@ -0,0 +1,52 @@
+"""
+A handy util to print a django-debug-screen-like stack trace with
+values of local variables.
+"""
+
+import sys, traceback
+from django.utils.encoding import smart_unicode
+
+
+def supertrace(max_len=160):
+    """
+    Print the usual traceback information, followed by a listing of all the
+    local variables in each frame.  Should be called from an exception handler.
+
+    if max_len is not None, will print up to max_len chars for each local variable.
+
+    (cite: modified from somewhere on stackoverflow)
+    """
+    tb = sys.exc_info()[2]
+    while True:
+        if not tb.tb_next:
+            break
+        tb = tb.tb_next
+    stack = []
+    frame = tb.tb_frame
+    while frame:
+        stack.append(f)
+        frame = frame.f_back
+    stack.reverse()
+    # First print the regular traceback
+    traceback.print_exc()
+
+    print "Locals by frame, innermost last"
+    for frame in stack:
+        print
+        print "Frame %s in %s at line %s" % (frame.f_code.co_name,
+                                             frame.f_code.co_filename,
+                                             frame.f_lineno)
+        for key, value in frame.f_locals.items():
+            print ("\t%20s = " % smart_unicode(key, errors='ignore')),
+            # We have to be careful not to cause a new error in our error
+            # printer! Calling str() on an unknown object could cause an
+            # error.
+            try:
+                s = smart_unicode(value, errors='ignore')
+                if max_len is not None:
+                    s = s[:max_len]
+                print s
+            except:
+                print "<ERROR WHILE PRINTING VALUE>"
+
+

common/lib/xmodule/progress.py

@@ -1,157 +0,0 @@
-'''
-Progress class for modules.  Represents where a student is in a module.
-
-Useful things to know:
- - Use Progress.to_js_status_str() to convert a progress into a simple
-   status string to pass to js.
- - Use Progress.to_js_detail_str() to convert a progress into a more detailed
-   string to pass to js.
-
-In particular, these functions have a canonical handing of None.
-
-For most subclassing needs, you should only need to reimplement
-frac() and __str__().
-'''
-
-from collections import namedtuple
-import numbers
-
-
-class Progress(object):
-    '''Represents a progress of a/b (a out of b done)
-
-    a and b must be numeric, but not necessarily integer, with
-    0 <= a <= b and b > 0.
-
-    Progress can only represent Progress for modules where that makes sense.  Other
-    modules (e.g. html) should return None from get_progress().
-
-    TODO: add tag for module type?  Would allow for smarter merging.
-    '''
-
-    def __init__(self, a, b):
-        '''Construct a Progress object.  a and b must be numbers, and must have
-              0 <= a <= b and b > 0
-        '''
-
-        # Want to do all checking at construction time, so explicitly check types
-        if not (isinstance(a, numbers.Number) and
-                isinstance(b, numbers.Number)):
-            raise TypeError('a and b must be numbers.  Passed {0}/{1}'.format(a, b))
-
-        if not (0 <= a <= b and b > 0):
-            raise ValueError(
-                'fraction a/b = {0}/{1} must have 0 <= a <= b and b > 0'.format(a, b))
-
-        self._a = a
-        self._b = b
-
-    def frac(self):
-        ''' Return tuple (a,b) representing progress of a/b'''
-        return (self._a, self._b)
-
-    def percent(self):
-        ''' Returns a percentage progress as a float between 0 and 100.
-
-        subclassing note: implemented in terms of frac(), assumes sanity
-        checking is done at construction time.
-        '''
-        (a, b) = self.frac()
-        return 100.0 * a / b
-
-    def started(self):
-        ''' Returns True if fractional progress is greater than 0.
-
-        subclassing note: implemented in terms of frac(), assumes sanity
-        checking is done at construction time.
-        '''
-        return self.frac()[0] > 0
-
-    def inprogress(self):
-        ''' Returns True if fractional progress is strictly between 0 and 1.
-
-        subclassing note: implemented in terms of frac(), assumes sanity
-        checking is done at construction time.
-        '''
-        (a, b) = self.frac()
-        return a > 0 and a < b
-
-    def done(self):
-        ''' Return True if this represents done.
-
-        subclassing note: implemented in terms of frac(), assumes sanity
-        checking is done at construction time.
-        '''
-        (a, b) = self.frac()
-        return a == b
-
-    def ternary_str(self):
-        ''' Return a string version of this progress: either
-           "none", "in_progress", or "done".
-
-        subclassing note: implemented in terms of frac()
-        '''
-        (a, b) = self.frac()
-        if a == 0:
-            return "none"
-        if a < b:
-            return "in_progress"
-        return "done"
-
-    def __eq__(self, other):
-        ''' Two Progress objects are equal if they have identical values.
-        Implemented in terms of frac()'''
-        if not isinstance(other, Progress):
-            return False
-        (a, b) = self.frac()
-        (a2, b2) = other.frac()
-        return a == a2 and b == b2
-
-    def __ne__(self, other):
-        ''' The opposite of equal'''
-        return not self.__eq__(other)
-
-    def __str__(self):
-        ''' Return a string representation of this string.
-
-        subclassing note: implemented in terms of frac().
-        '''
-        (a, b) = self.frac()
-        return "{0}/{1}".format(a, b)
-
-    @staticmethod
-    def add_counts(a, b):
-        '''Add two progress indicators, assuming that each represents items done:
-        (a / b) + (c / d) = (a + c) / (b + d).
-        If either is None, returns the other.
-        '''
-        if a is None:
-            return b
-        if b is None:
-            return a
-        # get numerators + denominators
-        (n, d) = a.frac()
-        (n2, d2) = b.frac()
-        return Progress(n + n2, d + d2)
-
-    @staticmethod
-    def to_js_status_str(progress):
-        '''
-        Return the "status string" version of the passed Progress
-        object that should be passed to js.  Use this function when
-        sending Progress objects to js to limit dependencies.
-        '''
-        if progress is None:
-            return "NA"
-        return progress.ternary_str()
-
-    @staticmethod
-    def to_js_detail_str(progress):
-        '''
-        Return the "detail string" version of the passed Progress
-        object that should be passed to js.  Use this function when
-        passing Progress objects to js to limit dependencies.
-        '''
-        if progress is None:
-            return "NA"
-        return str(progress)

common/lib/xmodule/setup.py

@@ -25,6 +25,7 @@ setup(
             "discuss = xmodule.backcompat_module:TranslateCustomTagDescriptor",
             "html = xmodule.html_module:HtmlDescriptor",
             "image = xmodule.backcompat_module:TranslateCustomTagDescriptor",
+            "error = xmodule.error_module:ErrorDescriptor",
             "problem = xmodule.capa_module:CapaDescriptor",
             "problemset = xmodule.vertical_module:VerticalDescriptor",
             "section = xmodule.backcompat_module:SemanticSectionDescriptor",

common/lib/xmodule/xmodule/backcompat_module.py

@@ -32,21 +32,25 @@ def process_includes(fn):
                 # read in and convert to XML
                 incxml = etree.XML(ifp.read())
 
-                # insert  new XML into tree in place of inlcude
+                # insert  new XML into tree in place of include
                 parent.insert(parent.index(next_include), incxml)
             except Exception:
-                msg = "Error in problem xml include: %s" % (etree.tostring(next_include, pretty_print=True))
-                log.exception(msg)
-                parent = next_include.getparent()
+                # Log error
+                msg = "Error in problem xml include: %s" % (
+                    etree.tostring(next_include, pretty_print=True))
+                # tell the tracker
+                system.error_tracker(msg)
 
+                # work around
+                parent = next_include.getparent()
                 errorxml = etree.Element('error')
                 messagexml = etree.SubElement(errorxml, 'message')
                 messagexml.text = msg
                 stackxml = etree.SubElement(errorxml, 'stacktrace')
                 stackxml.text = traceback.format_exc()
-
                 # insert error XML in place of include
                 parent.insert(parent.index(next_include), errorxml)
+
             parent.remove(next_include)
 
             next_include = xml_object.find('include')

common/lib/xmodule/xmodule/capa_module.py

@@ -5,6 +5,7 @@ import json
 import logging
 import traceback
 import re
+import sys
 
 from datetime import timedelta
 from lxml import etree
@@ -92,7 +93,8 @@ class CapaModule(XModule):
         display_due_date_string = self.metadata.get('due', None)
         if display_due_date_string is not None:
             self.display_due_date = dateutil.parser.parse(display_due_date_string)
-            #log.debug("Parsed " + display_due_date_string + " to " + str(self.display_due_date))
+            #log.debug("Parsed " + display_due_date_string +
+            #          " to " + str(self.display_due_date))
         else:
             self.display_due_date = None
 
@@ -100,7 +102,8 @@ class CapaModule(XModule):
         if grace_period_string is not None and self.display_due_date:
             self.grace_period = parse_timedelta(grace_period_string)
             self.close_date = self.display_due_date + self.grace_period
-            #log.debug("Then parsed " + grace_period_string + " to closing date" + str(self.close_date))
+            #log.debug("Then parsed " + grace_period_string +
+            #          " to closing date" + str(self.close_date))
         else:
             self.grace_period = None
             self.close_date = self.display_due_date
@@ -139,10 +142,16 @@ class CapaModule(XModule):
         try:
             self.lcp = LoncapaProblem(self.definition['data'], self.location.html_id(),
                                       instance_state, seed=seed, system=self.system)
-        except Exception:
-            msg = 'cannot create LoncapaProblem %s' % self.location.url()
-            log.exception(msg)
+        except Exception as err:
+            msg = 'cannot create LoncapaProblem {loc}: {err}'.format(
+                loc=self.location.url(), err=err)
+            # TODO (vshnayder): do modules need error handlers too?
+            # We shouldn't be switching on DEBUG.
             if self.system.DEBUG:
+                log.error(msg)
+                # TODO (vshnayder): This logic should be general, not here--and may
+                # want to preserve the data instead of replacing it.
+                # e.g. in the CMS
                 msg = '<p>%s</p>' % msg.replace('<', '&lt;')
                 msg += '<p><pre>%s</pre></p>' % traceback.format_exc().replace('<', '&lt;')
                 # create a dummy problem with error message instead of failing
@@ -153,7 +162,8 @@ class CapaModule(XModule):
                     problem_text, self.location.html_id(),
                     instance_state, seed=seed, system=self.system)
             else:
-                raise
+                # add extra info and raise
+                raise Exception(msg), None, sys.exc_info()[2]
 
     @property
     def rerandomize(self):
@@ -192,6 +202,7 @@ class CapaModule(XModule):
             try:
                 return Progress(score, total)
             except Exception as err:
+                # TODO (vshnayder): why is this still here? still needed?
                 if self.system.DEBUG:
                     return None
                 raise
@@ -211,6 +222,7 @@ class CapaModule(XModule):
         try:
             html = self.lcp.get_html()
         except Exception, err:
+            # TODO (vshnayder): another switch on DEBUG.
             if self.system.DEBUG:
                 log.exception(err)
                 msg = (
@@ -560,6 +572,7 @@ class CapaDescriptor(RawDescriptor):
             'problems/' + path[8:],
             path[8:],
         ]
+
     @classmethod
     def split_to_file(cls, xml_object):
         '''Problems always written in their own files'''

common/lib/xmodule/xmodule/course_module.py

@@ -1,7 +1,9 @@
+from fs.errors import ResourceNotFoundError
 import time
 import dateutil.parser
 import logging
 
+from xmodule.graders import load_grading_policy
 from xmodule.modulestore import Location
 from xmodule.seq_module import SequenceDescriptor, SequenceModule
 
@@ -14,20 +16,57 @@ class CourseDescriptor(SequenceDescriptor):
 
     def __init__(self, system, definition=None, **kwargs):
         super(CourseDescriptor, self).__init__(system, definition, **kwargs)
+        
+        self._grader = None
+        self._grade_cutoffs = None
 
+        msg = None
         try:
             self.start = time.strptime(self.metadata["start"], "%Y-%m-%dT%H:%M")
         except KeyError:
             self.start = time.gmtime(0) #The epoch
-            log.critical("Course loaded without a start date. %s", self.id)
+            msg = "Course loaded without a start date. id = %s" % self.id
+            log.critical(msg)
         except ValueError as e:
             self.start = time.gmtime(0) #The epoch
-            log.critical("Course loaded with a bad start date. %s '%s'",
-                         self.id, e)
+            msg = "Course loaded with a bad start date. %s '%s'" % (self.id, e)
+            log.critical(msg)
+
+        # Don't call the tracker from the exception handler.
+        if msg is not None:
+            system.error_tracker(msg)
+
 
     def has_started(self):
         return time.gmtime() > self.start
-
+    
+    @property
+    def grader(self):
+        self.__load_grading_policy()
+        return self._grader
+    
+    @property
+    def grade_cutoffs(self):
+        self.__load_grading_policy()
+        return self._grade_cutoffs
+    
+    
+    def __load_grading_policy(self):
+        if not self._grader or not self._grade_cutoffs:
+            policy_string = ""
+            
+            try:
+                with self.system.resources_fs.open("grading_policy.json") as grading_policy_file:
+                    policy_string = grading_policy_file.read()
+            except (IOError, ResourceNotFoundError):
+                log.warning("Unable to load course settings file from grading_policy.json in course " + self.id)
+            
+            grading_policy = load_grading_policy(policy_string)
+            
+            self._grader = grading_policy['GRADER']
+            self._grade_cutoffs = grading_policy['GRADE_CUTOFFS']
+    
+    
     @staticmethod
     def id_to_location(course_id):
         '''Convert the given course_id (org/course/name) to a location object.
@@ -72,3 +111,4 @@ class CourseDescriptor(SequenceDescriptor):
     @property
     def org(self):
         return self.location.org
+

common/lib/xmodule/xmodule/css/sequence/display.scss

@@ -2,9 +2,8 @@ nav.sequence-nav {
   // TODO (cpennington): This doesn't work anymore. XModules aren't able to
   // import from external sources.
   @extend .topbar;
-
-  border-bottom: 1px solid darken($cream, 20%);
-  margin-bottom: $body-line-height;
+  border-bottom: 1px solid $border-color;
+  margin: (-(lh())) (-(lh())) lh() (-(lh()));
   position: relative;
   @include border-top-right-radius(4px);
 
@@ -12,6 +11,8 @@ nav.sequence-nav {
     @include box-sizing(border-box);
     display: table;
     height: 100%;
+    margin: 0;
+    padding-left: 3px;
     padding-right: flex-grid(1, 9);
     width: 100%;
 
@@ -20,135 +21,104 @@ nav.sequence-nav {
     }
 
     li {
-      border-left: 1px solid darken($cream, 20%);
       display: table-cell;
       min-width: 20px;
 
-      &:first-child {
-        border-left: none;
-      }
-
-      .inactive {
-        background-repeat: no-repeat;
-
-        &:hover {
-          background-color: lighten($cream, 3%);
-        }
-      }
-
-      .visited {
-        background-color: #DCCDA2;
-        background-repeat: no-repeat;
-        @include box-shadow(inset 0 0 3px darken(#dccda2, 10%));
-
-        &:hover {
-          background-color: $cream;
-          background-position: center center;
-        }
-      }
-
-      .active {
-        background-color: #fff;
-        background-repeat: no-repeat;
-        @include box-shadow(0 1px 0 #fff);
-
-        &:hover {
-          background-color: #fff;
-          background-position: center;
-        }
-      }
-
       a {
-        background-position: center center;
-        border: none;
+        background-position: center;
+        background-repeat: no-repeat;
+        border: 1px solid transparent;
+        border-bottom: none;
+        @include border-radius(3px 3px 0 0);
         cursor: pointer;
         display: block;
-        height: 17px;
+        height: 10px;
         padding: 15px 0 14px;
         position: relative;
-        @include transition(all, .4s, $ease-in-out-quad);
+        @include transition();
         width: 100%;
 
-        &.progress {
-          border-bottom-style: solid;
-          border-bottom-width: 4px;
+        &:hover {
+          background-repeat: no-repeat;
+          background-position: center;
+          background-color: #F6F6F6;
+        }
+
+        &.visited {
+          background-color: #F6F6F6;
+
+          &:hover {
+            background-position: center center;
+          }
+        }
+
+        &.active {
+          border-color: $border-color;
+          @include box-shadow(0 2px 0 #fff);
+          background-color: #fff;
+          z-index: 9;
+
+          &:hover {
+            background-position: center;
+            background-color: #fff;
+          }
         }
 
         &.progress-none {
-          @extend .progress;
-          border-bottom-color: red;
+          background-color: lighten(red, 50%);
         }
 
         &.progress-some {
-          @extend .progress;
-          border-bottom-color: yellow;
+          background-color: yellow;
         }
 
         &.progress-done {
-          @extend .progress;
-          border-bottom-color: green;
+          background-color: green;
         }
 
         //video
         &.seq_video {
           &.inactive {
-            @extend .inactive;
             background-image: url('../images/sequence-nav/video-icon-normal.png');
-            background-position: center;
           }
 
           &.visited {
-            @extend .visited;
             background-image: url('../images/sequence-nav/video-icon-visited.png');
-            background-position: center;
           }
 
           &.active {
             @extend .active;
             background-image: url('../images/sequence-nav/video-icon-current.png');
-            background-position: center;
           }
         }
 
         //other
         &.seq_other {
           &.inactive {
-            @extend .inactive;
             background-image: url('../images/sequence-nav/document-icon-normal.png');
-            background-position: center;
           }
 
           &.visited {
-            @extend .visited;
             background-image: url('../images/sequence-nav/document-icon-visited.png');
-            background-position: center;
           }
 
           &.active {
-            @extend .active;
             background-image: url('../images/sequence-nav/document-icon-current.png');
-            background-position: center;
           }
         }
 
         //vertical & problems
         &.seq_vertical, &.seq_problem {
           &.inactive {
-            @extend .inactive;
             background-image: url('../images/sequence-nav/list-icon-normal.png');
-            background-position: center;
           }
 
           &.visited {
-            @extend .visited;
             background-image: url('../images/sequence-nav/list-icon-visited.png');
-            background-position: center;
           }
 
           &.active {
-            @extend .active;
             background-image: url('../images/sequence-nav/list-icon-current.png');
-            background-position: center;
           }
         }
 
@@ -156,6 +126,7 @@ nav.sequence-nav {
           background: #333;
           color: #fff;
           display: none;
+          font-family: $sans-serif;
           line-height: lh();
           left: 0px;
           opacity: 0;
@@ -206,27 +177,29 @@ nav.sequence-nav {
     right: 0;
     top: 0;
     width: flex-grid(1, 9);
+    border: 1px solid $border-color;
+    border-bottom: 0;
+    @include border-radius(3px 3px 0 0);
 
     li {
       float: left;
+      margin-bottom: 0;
       width: 50%;
 
       &.prev, &.next {
 
         a {
-          background-color: darken($cream, 5%);
-          background-position: center center;
+          background-position: center;
           background-repeat: no-repeat;
-          border-left: 1px solid darken(#f6efd4, 20%);
-          @include box-shadow(inset 1px 0 0 lighten(#f6efd4, 5%));
-          @include box-sizing(border-box);
-          cursor: pointer;
           display: block;
+          height: 10px;
+          padding: 15px 0 14px;
           text-indent: -9999px;
           @include transition(all, .2s, $ease-in-out-quad);
 
           &:hover {
             opacity: .5;
+            background-color: #f4f4f4;
           }
 
           &.disabled {
@@ -239,20 +212,13 @@ nav.sequence-nav {
       &.prev {
         a {
           background-image: url('../images/sequence-nav/previous-icon.png');
-
-          &:hover {
-            background-color: $cream;
-          }
         }
       }
 
       &.next {
         a {
+          border-left: 1px solid lighten($border-color, 10%);
           background-image: url('../images/sequence-nav/next-icon.png');
-
-          &:hover {
-            background-color: $cream;
-          }
         }
       }
     }
@@ -273,11 +239,8 @@ nav.sequence-bottom {
 
   ul {
     @extend .clearfix;
-    background-color: darken(#F6EFD4, 5%);
-    background-color: darken($cream, 5%);
-    border: 1px solid darken(#f6efd4, 20%);
+    border: 1px solid $border-color;
     @include border-radius(3px);
-    @include box-shadow(inset 0 0 0 1px lighten(#f6efd4, 5%));
     @include inline-block();
 
     li {
@@ -297,14 +260,13 @@ nav.sequence-bottom {
           width: 45px;
 
           &:hover {
-            background-color: $cream;
-            color: darken($cream, 60%);
+            background-color: #ddd;
+            color: #000;
             opacity: .5;
             text-decoration: none;
           }
 
           &.disabled {
-            background-color: lighten($cream, 10%);
             opacity: .4;
           }
         }
@@ -313,7 +275,7 @@ nav.sequence-bottom {
       &.prev {
         a {
           background-image: url('../images/sequence-nav/previous-icon.png');
-          border-right: 1px solid darken(#f6efd4, 20%);
+          border-right: 1px solid lighten($border-color, 10%);
 
           &:hover {
             background-color: none;

common/lib/xmodule/xmodule/css/video/display.scss

@@ -114,14 +114,13 @@ div.video {
         @extend .dullify;
         float: left;
         list-style: none;
-        margin-right: lh();
+        margin: 0 lh() 0 0;
         padding: 0;
 
         li {
           float: left;
           margin-bottom: 0;
 
-
           a {
             border-bottom: none;
             border-right: 1px solid #000;
@@ -183,6 +182,8 @@ div.video {
             ol.video_speeds {
               display: block;
               opacity: 1;
+              padding: 0;
+              margin: 0;
             }
           }
 
@@ -210,6 +211,7 @@ div.video {
               font-weight: normal;
               letter-spacing: 1px;
               padding: 0 lh(.25) 0 lh(.5);
+              line-height: 46px;
               text-transform: uppercase;
             }
 
@@ -218,6 +220,7 @@ div.video {
               font-weight: bold;
               margin-bottom: 0;
               padding: 0 lh(.5) 0 0;
+              line-height: 46px;
             }
 
             &:hover, &:active, &:focus {
@@ -422,10 +425,12 @@ div.video {
   }
 
   ol.subtitles {
+    padding-left: 0;
     float: left;
     max-height: 460px;
     overflow: auto;
     width: flex-grid(3, 9);
+    margin: 0;
 
     li {
       border: 0;

common/lib/xmodule/xmodule/editing_module.py

@@ -0,0 +1,29 @@
+from pkg_resources import resource_string
+from lxml import etree
+from xmodule.mako_module import MakoModuleDescriptor
+import logging
+
+log = logging.getLogger(__name__)
+
+class EditingDescriptor(MakoModuleDescriptor):
+    """
+    Module that provides a raw editing view of its data and children.  It does not
+    perform any validation on its definition---just passes it along to the browser.
+
+    This class is intended to be used as a mixin.
+    """
+    mako_template = "widgets/raw-edit.html"
+
+    js = {'coffee': [resource_string(__name__, 'js/src/raw/edit.coffee')]}
+    js_module_name = "RawDescriptor"
+
+    def get_context(self):
+        return {
+            'module': self,
+            'data': self.definition.get('data', ''),
+    # TODO (vshnayder): allow children and metadata to be edited.
+    #'children' : self.definition.get('children, ''),
+
+    # TODO: show both own metadata and inherited?
+    #'metadata' : self.own_metadata,
+        }

common/lib/xmodule/xmodule/error_module.py

@@ -0,0 +1,89 @@
+import sys
+import logging
+
+from pkg_resources import resource_string
+from lxml import etree
+from xmodule.x_module import XModule
+from xmodule.mako_module import MakoModuleDescriptor
+from xmodule.xml_module import XmlDescriptor
+from xmodule.editing_module import EditingDescriptor
+from xmodule.errortracker import exc_info_to_str
+
+
+log = logging.getLogger(__name__)
+
+class ErrorModule(XModule):
+    def get_html(self):
+        '''Show an error.
+        TODO (vshnayder): proper style, divs, etc.
+        '''
+        # staff get to see all the details
+        return self.system.render_template('module-error.html', {
+            'data' : self.definition['data']['contents'],
+            'error' : self.definition['data']['error_msg'],
+            'is_staff' : self.system.is_staff,
+            })
+
+class ErrorDescriptor(EditingDescriptor):
+    """
+    Module that provides a raw editing view of broken xml.
+    """
+    module_class = ErrorModule
+
+    @classmethod
+    def from_xml(cls, xml_data, system, org=None, course=None,
+                 error_msg='Error not available'):
+        '''Create an instance of this descriptor from the supplied data.
+
+        Does not try to parse the data--just stores it.
+
+        Takes an extra, optional, parameter--the error that caused an
+        issue.  (should be a string, or convert usefully into one).
+        '''
+        # Use a nested inner dictionary because 'data' is hardcoded
+        inner = {}
+        definition = {'data': inner}
+        inner['error_msg'] = str(error_msg)
+
+        try:
+            # If this is already an error tag, don't want to re-wrap it.
+            xml_obj = etree.fromstring(xml_data)
+            if xml_obj.tag == 'error':
+                xml_data = xml_obj.text
+                error_node = xml_obj.find('error_msg')
+                if error_node is not None:
+                    inner['error_msg'] = error_node.text
+                else:
+                    inner['error_msg'] = 'Error not available'
+
+        except etree.XMLSyntaxError:
+            # Save the error to display later--overrides other problems
+            inner['error_msg'] = exc_info_to_str(sys.exc_info())
+
+        inner['contents'] = xml_data
+        # TODO (vshnayder): Do we need a unique slug here?  Just pick a random
+        # 64-bit num?
+        location = ['i4x', org, course, 'error', 'slug']
+        metadata = {}  # stays in the xml_data
+
+        return cls(system, definition, location=location, metadata=metadata)
+
+    def export_to_xml(self, resource_fs):
+        '''
+        If the definition data is invalid xml, export it wrapped in an "error"
+        tag.  If it is valid, export without the wrapper.
+
+        NOTE: There may still be problems with the valid xml--it could be
+        missing required attributes, could have the wrong tags, refer to missing
+        files, etc.  That would just get re-wrapped on import.
+        '''
+        try:
+           xml = etree.fromstring(self.definition['data']['contents'])
+           return etree.tostring(xml)
+        except etree.XMLSyntaxError:
+            # still not valid.
+            root = etree.Element('error')
+            root.text = self.definition['data']['contents']
+            err_node = etree.SubElement(root, 'error_msg')
+            err_node.text = self.definition['data']['error_msg']
+            return etree.tostring(root)

common/lib/xmodule/xmodule/errorhandlers.py

@@ -1,45 +0,0 @@
-import logging
-import sys
-
-log = logging.getLogger(__name__)
-
-def in_exception_handler():
-    '''Is there an active exception?'''
-    return sys.exc_info() != (None, None, None)
-
-def strict_error_handler(msg, exc_info=None):
-    '''
-    Do not let errors pass.  If exc_info is not None, ignore msg, and just
-    re-raise.  Otherwise, check if we are in an exception-handling context.
-    If so, re-raise.  Otherwise, raise Exception(msg).
-
-    Meant for use in validation, where any errors should trap.
-    '''
-    if exc_info is not None:
-        raise exc_info[0], exc_info[1], exc_info[2]
-
-    if in_exception_handler():
-        raise
-
-    raise Exception(msg)
-
-
-def logging_error_handler(msg, exc_info=None):
-    '''Log all errors, but otherwise let them pass, relying on the caller to
-    workaround.'''
-    if exc_info is not None:
-        log.exception(msg, exc_info=exc_info)
-        return
-
-    if in_exception_handler():
-        log.exception(msg)
-        return
-
-    log.error(msg)
-
-
-def ignore_errors_handler(msg, exc_info=None):
-    '''Ignore all errors, relying on the caller to workaround.
-    Meant for use in the LMS, where an error in one part of the course
-    shouldn't bring down the whole system'''
-    pass

common/lib/xmodule/xmodule/errortracker.py

@@ -0,0 +1,44 @@
+import logging
+import sys
+import traceback
+
+from collections import namedtuple
+
+log = logging.getLogger(__name__)
+
+ErrorLog = namedtuple('ErrorLog', 'tracker errors')
+
+def exc_info_to_str(exc_info):
+    """Given some exception info, convert it into a string using
+    the traceback.format_exception() function.
+    """
+    return ''.join(traceback.format_exception(*exc_info))
+
+def in_exception_handler():
+    '''Is there an active exception?'''
+    return sys.exc_info() != (None, None, None)
+
+
+def make_error_tracker():
+    '''Return an ErrorLog (named tuple), with fields (tracker, errors), where
+    the logger appends a tuple (message, exception_str) to the errors on every
+    call.  exception_str is in the format returned by traceback.format_exception.
+
+    error_list is a simple list.  If the caller modifies it, info
+    will be lost.
+    '''
+    errors = []
+
+    def error_tracker(msg):
+        '''Log errors'''
+        exc_str = ''
+        if in_exception_handler():
+            exc_str = exc_info_to_str(sys.exc_info())
+
+        errors.append((msg, exc_str))
+
+    return ErrorLog(error_tracker, errors)
+
+def null_error_tracker(msg):
+    '''A dummy error tracker that just ignores the messages'''
+    pass

common/lib/xmodule/xmodule/graders.py

@@ -1,4 +1,5 @@
 import abc
+import json
 import logging
 
 from collections import namedtuple
@@ -9,6 +10,69 @@ log = logging.getLogger("mitx.courseware")
 # Section either indicates the name of the problem or the name of the section
 Score = namedtuple("Score", "earned possible graded section")
 
+def load_grading_policy(course_policy_string):
+    """
+    This loads a grading policy from a string (usually read from a file),
+    which can be a JSON object or an empty string.
+    
+    The JSON object can have the keys GRADER and GRADE_CUTOFFS. If either is
+    missing, it reverts to the default.
+    """
+    
+    default_policy_string = """
+    {
+        "GRADER" : [
+            {
+                "type" : "Homework",
+                "min_count" : 12,
+                "drop_count" : 2,
+                "short_label" : "HW",
+                "weight" : 0.15
+            },
+            {
+                "type" : "Lab",
+                "min_count" : 12,
+                "drop_count" : 2,
+                "category" : "Labs",
+                "weight" : 0.15
+            },
+            {
+                "type" : "Midterm",
+                "name" : "Midterm Exam",
+                "short_label" : "Midterm",
+                "weight" : 0.3
+            },
+            {
+                "type" : "Final",
+                "name" : "Final Exam",
+                "short_label" : "Final",
+                "weight" : 0.4
+            }
+        ],
+        "GRADE_CUTOFFS" : {
+            "A" : 0.87,
+            "B" : 0.7,
+            "C" : 0.6
+        }
+    }
+    """
+    
+    # Load the global settings as a dictionary
+    grading_policy = json.loads(default_policy_string)
+
+    # Load the course policies as a dictionary
+    course_policy = {}
+    if course_policy_string:
+        course_policy = json.loads(course_policy_string)
+    
+    # Override any global settings with the course settings
+    grading_policy.update(course_policy)
+
+    # Here is where we should parse any configurations, so that we can fail early
+    grading_policy['GRADER'] = grader_from_conf(grading_policy['GRADER'])
+    
+    return grading_policy
+    
 
 def aggregate_scores(scores, section_name="summary"):
     """

common/lib/xmodule/xmodule/html_checker.py

@@ -0,0 +1,14 @@
+from lxml import etree
+
+def check_html(html):
+    '''
+    Check whether the passed in html string can be parsed by lxml.
+    Return bool success.
+    '''
+    parser = etree.HTMLParser()
+    try:
+        etree.fromstring(html, parser)
+        return True
+    except Exception as err:
+        pass
+    return False

common/lib/xmodule/xmodule/html_module.py

@@ -1,13 +1,18 @@
+import copy
+from fs.errors import ResourceNotFoundError
 import logging
 import os
+import sys
 from lxml import etree
 
-from xmodule.x_module import XModule
-from xmodule.raw_module import RawDescriptor
+from .x_module import XModule
+from .xml_module import XmlDescriptor
+from .editing_module import EditingDescriptor
+from .stringify import stringify_children
+from .html_checker import check_html
 
 log = logging.getLogger("mitx.courseware")
 
-
 class HtmlModule(XModule):
     def get_html(self):
         return self.html
@@ -19,33 +24,118 @@ class HtmlModule(XModule):
         self.html = self.definition['data']
 
 
-class HtmlDescriptor(RawDescriptor):
+class HtmlDescriptor(XmlDescriptor, EditingDescriptor):
     """
     Module for putting raw html in a course
     """
     mako_template = "widgets/html-edit.html"
     module_class = HtmlModule
-    filename_extension = "html"
+    filename_extension = "xml"
 
-    # TODO (cpennington): Delete this method once all fall 2012 course are being
-    # edited in the cms
+    # VS[compat] TODO (cpennington): Delete this method once all fall 2012 course
+    # are being edited in the cms
     @classmethod
     def backcompat_paths(cls, path):
-        if path.endswith('.html.html'):
-            path = path[:-5]
+        origpath = path
+        if path.endswith('.html.xml'):
+            path = path[:-9] + '.html'  #backcompat--look for html instead of xml
         candidates = []
         while os.sep in path:
             candidates.append(path)
             _, _, path = path.partition(os.sep)
 
+        # also look for .html versions instead of .xml
+        if origpath.endswith('.xml'):
+            candidates.append(origpath[:-4] + '.html')
         return candidates
 
+    # NOTE: html descriptors are special.  We do not want to parse and
+    # export them ourselves, because that can break things (e.g. lxml
+    # adds body tags when it exports, but they should just be html
+    # snippets that will be included in the middle of pages.
+
     @classmethod
-    def file_to_xml(cls, file_object):
-        parser = etree.HTMLParser()
-        return etree.parse(file_object, parser).getroot()
+    def load_definition(cls, xml_object, system, location):
+        '''Load a descriptor from the specified xml_object:
+
+        If there is a filename attribute, load it as a string, and
+        log a warning if it is not parseable by etree.HTMLParser.
+
+        If there is not a filename attribute, the definition is the body
+        of the xml_object, without the root tag (do not want <html> in the
+        middle of a page)
+        '''
+        filename = xml_object.get('filename')
+        if filename is None:
+            definition_xml = copy.deepcopy(xml_object)
+            cls.clean_metadata_from_xml(definition_xml)
+            return {'data' : stringify_children(definition_xml)}
+        else:
+            filepath = cls._format_filepath(xml_object.tag, filename)
+
+            # VS[compat]
+            # TODO (cpennington): If the file doesn't exist at the right path,
+            # give the class a chance to fix it up. The file will be written out
+            # again in the correct format.  This should go away once the CMS is
+            # online and has imported all current (fall 2012) courses from xml
+            if not system.resources_fs.exists(filepath):
+                candidates = cls.backcompat_paths(filepath)
+                #log.debug("candidates = {0}".format(candidates))
+                for candidate in candidates:
+                    if system.resources_fs.exists(candidate):
+                        filepath = candidate
+                        break
+
+            try:
+                with system.resources_fs.open(filepath) as file:
+                    html = file.read()
+                    # Log a warning if we can't parse the file, but don't error
+                    if not check_html(html):
+                        msg = "Couldn't parse html in {0}.".format(filepath)
+                        log.warning(msg)
+                        system.error_tracker("Warning: " + msg)
+
+                    definition = {'data' : html}
+
+                    # TODO (ichuang): remove this after migration
+                    # for Fall 2012 LMS migration: keep filename (and unmangled filename)
+                    definition['filename'] = [ filepath, filename ]
+
+                    return definition
+
+            except (ResourceNotFoundError) as err:
+                msg = 'Unable to load file contents at path {0}: {1} '.format(
+                    filepath, err)
+                # add more info and re-raise
+                raise Exception(msg), None, sys.exc_info()[2]
 
     @classmethod
     def split_to_file(cls, xml_object):
-        # never include inline html
+        '''Never include inline html'''
         return True
+
+
+    # TODO (vshnayder): make export put things in the right places.
+
+    def definition_to_xml(self, resource_fs):
+        '''If the contents are valid xml, write them to filename.xml.  Otherwise,
+        write just the <html filename=""> tag to filename.xml, and the html
+        string to filename.html.
+        '''
+        try:
+            return etree.fromstring(self.definition['data'])
+        except etree.XMLSyntaxError:
+            pass
+
+        # Not proper format.  Write html to file, return an empty tag
+        filepath = u'{category}/{name}.html'.format(category=self.category,
+                                                    name=self.url_name)
+
+        resource_fs.makedir(os.path.dirname(filepath), allow_recreate=True)
+        with resource_fs.open(filepath, 'w') as file:
+            file.write(self.definition['data'])
+
+        elt = etree.Element('html')
+        elt.set("filename", self.url_name)
+        return elt
+

common/lib/xmodule/xmodule/mako_module.py

@@ -2,10 +2,10 @@ from x_module import XModuleDescriptor, DescriptorSystem
 
 
 class MakoDescriptorSystem(DescriptorSystem):
-    def __init__(self, load_item, resources_fs, error_handler,
-                 render_template):
+    def __init__(self, load_item, resources_fs, error_tracker,
+                 render_template, **kwargs):
         super(MakoDescriptorSystem, self).__init__(
-            load_item, resources_fs, error_handler)
+            load_item, resources_fs, error_tracker, **kwargs)
 
         self.render_template = render_template
 

common/lib/xmodule/xmodule/modulestore/__init__.py

@@ -3,10 +3,13 @@ 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
-from collections import namedtuple
-from .exceptions import InvalidLocationError
 import logging
+import re
+
+from collections import namedtuple
+
+from .exceptions import InvalidLocationError, InsufficientSpecificationError
+from xmodule.errortracker import ErrorLog, make_error_tracker
 
 log = logging.getLogger('mitx.' + 'modulestore')
 
@@ -38,15 +41,15 @@ class Location(_LocationBase):
     '''
     __slots__ = ()
 
-    @classmethod
-    def clean(cls, value):
+    @staticmethod
+    def clean(value):
         """
         Return value, made into a form legal for locations
         """
         return re.sub('_+', '_', INVALID_CHARS.sub('_', value))
 
-    @classmethod
-    def is_valid(cls, value):
+    @staticmethod
+    def is_valid(value):
         '''
         Check if the value is a valid location, in any acceptable format.
         '''
@@ -56,6 +59,21 @@ class Location(_LocationBase):
             return False
         return True
 
+    @staticmethod
+    def ensure_fully_specified(location):
+        '''Make sure location is valid, and fully specified.  Raises
+        InvalidLocationError or InsufficientSpecificationError if not.
+
+        returns a Location object corresponding to location.
+        '''
+        loc = Location(location)
+        for key, val in loc.dict().iteritems():
+            if key != 'revision' and val is None:
+                raise InsufficientSpecificationError(location)
+        return loc
+
+
+
     def __new__(_cls, loc_or_tag=None, org=None, course=None, category=None,
                 name=None, revision=None):
         """
@@ -198,6 +216,18 @@ class ModuleStore(object):
         """
         raise NotImplementedError
 
+    def get_item_errors(self, location):
+        """
+        Return a list of (msg, exception-or-None) errors that the modulestore
+        encountered when loading the item at location.
+
+        location : something that can be passed to Location
+
+        Raises the same exceptions as get_item if the location isn't found or
+        isn't fully specified.
+        """
+        raise NotImplementedError
+
     def get_items(self, location, depth=0):
         """
         Returns a list of XModuleDescriptor instances for the items
@@ -254,25 +284,47 @@ class ModuleStore(object):
         '''
         raise NotImplementedError
 
-    def path_to_location(self, location, course=None, chapter=None, section=None):
-        '''
-        Try to find a course/chapter/section[/position] path to this location.
 
-        raise ItemNotFoundError if the location doesn't exist.
+    def get_parent_locations(self, location):
+        '''Find all locations that are the parents of this location.  Needed
+        for path_to_location().
 
-        If course, chapter, section are not None, restrict search to paths with those
-            components as specified.
-            
-        raise NoPathToItem if the location exists, but isn't accessible via
-        a path that matches the course/chapter/section restrictions.
-
-        In general, a location may be accessible via many paths. This method may
-        return any valid path.
-
-        Return a tuple (course, chapter, section, position).
-
-        If the section a sequence, position should be the position of this location
-        in that sequence.  Otherwise, position should be None.
+        returns an iterable of things that can be passed to Location.
         '''
         raise NotImplementedError
 
+
+class ModuleStoreBase(ModuleStore):
+    '''
+    Implement interface functionality that can be shared.
+    '''
+    def __init__(self):
+        '''
+        Set up the error-tracking logic.
+        '''
+        self._location_errors = {}    # location -> ErrorLog
+
+    def _get_errorlog(self, location):
+        """
+        If we already have an errorlog for this location, return it.  Otherwise,
+        create one.
+        """
+        location = Location(location)
+        if location not in self._location_errors:
+            self._location_errors[location] = make_error_tracker()
+        return self._location_errors[location]
+
+    def get_item_errors(self, location):
+        """
+        Return list of errors for this location, if any.  Raise the same
+        errors as get_item if location isn't present.
+
+        NOTE: For now, the only items that track errors are CourseDescriptors in
+        the xml datastore.  This will return an empty list for all other items
+        and datastores.
+        """
+        # check that item is present and raise the promised exceptions if needed
+        self.get_item(location)
+
+        errorlog = self._get_errorlog(location)
+        return errorlog.errors

common/lib/xmodule/xmodule/modulestore/mongo.py

@@ -6,14 +6,13 @@ from itertools import repeat
 from path import path
 
 from importlib import import_module
-from xmodule.errorhandlers import strict_error_handler
+from xmodule.errortracker import null_error_tracker
 from xmodule.x_module import XModuleDescriptor
 from xmodule.mako_module import MakoDescriptorSystem
-from xmodule.course_module import CourseDescriptor
 from mitxmako.shortcuts import render_to_string
 
-from . import ModuleStore, Location
-from .exceptions import (ItemNotFoundError, InsufficientSpecificationError,
+from . import ModuleStoreBase, Location
+from .exceptions import (ItemNotFoundError,
                          NoPathToItem, DuplicateItemError)
 
 # TODO (cpennington): This code currently operates under the assumption that
@@ -27,7 +26,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem):
     from, with a backup of calling to the underlying modulestore for more data
     """
     def __init__(self, modulestore, module_data, default_class, resources_fs,
-                 error_handler, render_template):
+                 error_tracker, render_template):
         """
         modulestore: the module store that can be used to retrieve additional modules
 
@@ -39,13 +38,13 @@ class CachingDescriptorSystem(MakoDescriptorSystem):
 
         resources_fs: a filesystem, as per MakoDescriptorSystem
 
-        error_handler:
+        error_tracker: a function that logs errors for later display to users
 
         render_template: a function for rendering templates, as per
             MakoDescriptorSystem
         """
         super(CachingDescriptorSystem, self).__init__(
-                self.load_item, resources_fs, error_handler, render_template)
+                self.load_item, resources_fs, error_tracker, render_template)
         self.modulestore = modulestore
         self.module_data = module_data
         self.default_class = default_class
@@ -74,13 +73,17 @@ def location_to_query(location):
     return query
 
 
-class MongoModuleStore(ModuleStore):
+class MongoModuleStore(ModuleStoreBase):
     """
     A Mongodb backed ModuleStore
     """
 
     # TODO (cpennington): Enable non-filesystem filestores
-    def __init__(self, host, db, collection, fs_root, port=27017, default_class=None):
+    def __init__(self, host, db, collection, fs_root, port=27017, default_class=None,
+                 error_tracker=null_error_tracker):
+
+        ModuleStoreBase.__init__(self)
+
         self.collection = pymongo.connection.Connection(
             host=host,
             port=port
@@ -91,13 +94,17 @@ class MongoModuleStore(ModuleStore):
 
         # Force mongo to maintain an index over _id.* that is in the same order
         # that is used when querying by a location
-        self.collection.ensure_index(zip(('_id.' + field for field in Location._fields), repeat(1)))
+        self.collection.ensure_index(
+            zip(('_id.' + field for field in Location._fields), repeat(1)))
 
-        # TODO (vshnayder): default arg default_class=None will make this error
-        module_path, _, class_name = default_class.rpartition('.')
-        class_ = getattr(import_module(module_path), class_name)
-        self.default_class = class_
+        if default_class is not None:
+            module_path, _, class_name = default_class.rpartition('.')
+            class_ = getattr(import_module(module_path), class_name)
+            self.default_class = class_
+        else:
+            self.default_class = None
         self.fs_root = path(fs_root)
+        self.error_tracker = error_tracker
 
     def _clean_item_data(self, item):
         """
@@ -149,7 +156,7 @@ class MongoModuleStore(ModuleStore):
             data_cache,
             self.default_class,
             resource_fs,
-            strict_error_handler,
+            self.error_tracker,
             render_to_string,
         )
         return system.load_item(item['location'])
@@ -172,12 +179,17 @@ class MongoModuleStore(ModuleStore):
         return self.get_items(course_filter)
 
     def _find_one(self, location):
-        '''Look for a given location in the collection.
-        If revision isn't specified, returns the latest.'''
-        return self.collection.find_one(
+        '''Look for a given location in the collection.  If revision is not
+        specified, returns the latest.  If the item is not present, raise
+        ItemNotFoundError.
+        '''
+        item = self.collection.find_one(
             location_to_query(location),
             sort=[('revision', pymongo.ASCENDING)],
         )
+        if item is None:
+            raise ItemNotFoundError(location)
+        return item
 
     def get_item(self, location, depth=0):
         """
@@ -197,14 +209,8 @@ class MongoModuleStore(ModuleStore):
             calls to get_children() to cache. None indicates to cache all descendents.
 
         """
-
-        for key, val in Location(location).dict().iteritems():
-            if key != 'revision' and val is None:
-                raise InsufficientSpecificationError(location)
-
+        location = Location.ensure_fully_specified(location)
         item = self._find_one(location)
-        if item is None:
-            raise ItemNotFoundError(location)
         return self._load_items([item], depth)[0]
 
     def get_items(self, location, depth=0):
@@ -282,96 +288,20 @@ class MongoModuleStore(ModuleStore):
         )
 
     def get_parent_locations(self, location):
-        '''Find all locations that are the parents of this location.
-        Mostly intended for use in path_to_location, but exposed for testing
-        and possible other usefulness.
+        '''Find all locations that are the parents of this location.  Needed
+        for path_to_location().
 
-        returns an iterable of things that can be passed to Location.
+        If there is no data at location in this modulestore, raise
+            ItemNotFoundError.
+
+        returns an iterable of things that can be passed to Location.  This may
+        be empty if there are no parents.
         '''
-        location = Location(location)
-        items = self.collection.find({'definition.children': str(location)},
+        location = Location.ensure_fully_specified(location)
+        # Check that it's actually in this modulestore.
+        item = self._find_one(location)
+        # now get the parents
+        items = self.collection.find({'definition.children': location.url()},
                                     {'_id': True})
         return [i['_id'] for i in items]
 
-    def path_to_location(self, location, course_name=None):
-        '''
-        Try to find a course_id/chapter/section[/position] path to this location.
-        The courseware insists that the first level in the course is chapter,
-        but any kind of module can be a "section".
-
-        location: something that can be passed to Location
-        course_name: [optional].  If not None, restrict search to paths
-            in that course.
-
-        raise ItemNotFoundError if the location doesn't exist.
-
-        raise NoPathToItem if the location exists, but isn't accessible via
-        a chapter/section path in the course(s) being searched.
-
-        Return a tuple (course_id, chapter, section, position) suitable for the
-        courseware index view.
-
-        A location may be accessible via many paths. This method may
-        return any valid path.
-
-        If the section is a sequence, position will be the position
-        of this location in that sequence.  Otherwise, position will
-        be None. TODO (vshnayder): Not true yet.
-        '''
-        # Check that location is present at all
-        if self._find_one(location) is None:
-            raise ItemNotFoundError(location)
-
-        def flatten(xs):
-            '''Convert lisp-style (a, (b, (c, ()))) lists into a python list.
-            Not a general flatten function. '''
-            p = []
-            while xs != ():
-                p.append(xs[0])
-                xs = xs[1]
-            return p
-
-        def find_path_to_course(location, course_name=None):
-            '''Find a path up the location graph to a node with the
-            specified category.  If no path exists, return None.  If a
-            path exists, return it as a list with target location
-            first, and the starting location last.
-            '''
-            # Standard DFS
-
-            # To keep track of where we came from, the work queue has
-            # tuples (location, path-so-far).  To avoid lots of
-            # copying, the path-so-far is stored as a lisp-style
-            # list--nested hd::tl tuples, and flattened at the end.
-            queue = [(location, ())]
-            while len(queue) > 0:
-                (loc, path) = queue.pop()  # Takes from the end
-                loc = Location(loc)
-                # print 'Processing loc={0}, path={1}'.format(loc, path)
-                if loc.category == "course":
-                    if course_name is None or course_name == loc.name:
-                        # Found it!
-                        path = (loc, path)
-                        return flatten(path)
-
-                # otherwise, add parent locations at the end
-                newpath = (loc, path)
-                parents = self.get_parent_locations(loc)
-                queue.extend(zip(parents, repeat(newpath)))
-
-            # If we're here, there is no path
-            return None
-
-        path = find_path_to_course(location, course_name)
-        if path is None:
-            raise(NoPathToItem(location))
-
-        n = len(path)
-        course_id = CourseDescriptor.location_to_id(path[0])
-        chapter = path[1].name if n > 1 else None
-        section = path[2].name if n > 2 else None
-
-        # TODO (vshnayder): not handling position at all yet...
-        position = None
-
-        return (course_id, chapter, section, position)

common/lib/xmodule/xmodule/modulestore/search.py

@@ -0,0 +1,97 @@
+from itertools import repeat
+
+from xmodule.course_module import CourseDescriptor
+
+from .exceptions import (ItemNotFoundError, NoPathToItem)
+from . import ModuleStore, Location
+
+
+def path_to_location(modulestore, location, course_name=None):
+    '''
+    Try to find a course_id/chapter/section[/position] path to location in
+    modulestore.  The courseware insists that the first level in the course is
+    chapter, but any kind of module can be a "section".
+
+    location: something that can be passed to Location
+    course_name: [optional].  If not None, restrict search to paths
+        in that course.
+
+    raise ItemNotFoundError if the location doesn't exist.
+
+    raise NoPathToItem if the location exists, but isn't accessible via
+    a chapter/section path in the course(s) being searched.
+
+    Return a tuple (course_id, chapter, section, position) suitable for the
+    courseware index view.
+
+    A location may be accessible via many paths. This method may
+    return any valid path.
+
+    If the section is a sequence, position will be the position
+    of this location in that sequence.  Otherwise, position will
+    be None. TODO (vshnayder): Not true yet.
+    '''
+
+    def flatten(xs):
+        '''Convert lisp-style (a, (b, (c, ()))) list into a python list.
+        Not a general flatten function. '''
+        p = []
+        while xs != ():
+            p.append(xs[0])
+            xs = xs[1]
+        return p
+
+    def find_path_to_course(location, course_name=None):
+        '''Find a path up the location graph to a node with the
+        specified category.
+
+        If no path exists, return None.
+
+        If a path exists, return it as a list with target location first, and
+        the starting location last.
+        '''
+        # Standard DFS
+
+        # To keep track of where we came from, the work queue has
+        # tuples (location, path-so-far).  To avoid lots of
+        # copying, the path-so-far is stored as a lisp-style
+        # list--nested hd::tl tuples, and flattened at the end.
+        queue = [(location, ())]
+        while len(queue) > 0:
+            (loc, path) = queue.pop()  # Takes from the end
+            loc = Location(loc)
+
+            # get_parent_locations should raise ItemNotFoundError if location
+            # isn't found so we don't have to do it explicitly.  Call this
+            # first to make sure the location is there (even if it's a course, and
+            # we would otherwise immediately exit).
+            parents = modulestore.get_parent_locations(loc)
+
+            # print 'Processing loc={0}, path={1}'.format(loc, path)
+            if loc.category == "course":
+                if course_name is None or course_name == loc.name:
+                    # Found it!
+                    path = (loc, path)
+                    return flatten(path)
+
+            # otherwise, add parent locations at the end
+            newpath = (loc, path)
+            queue.extend(zip(parents, repeat(newpath)))
+
+        # If we're here, there is no path
+        return None
+
+    path = find_path_to_course(location, course_name)
+    if path is None:
+        raise(NoPathToItem(location))
+
+    n = len(path)
+    course_id = CourseDescriptor.location_to_id(path[0])
+    # pull out the location names
+    chapter = path[1].name if n > 1 else None
+    section = path[2].name if n > 2 else None
+
+    # TODO (vshnayder): not handling position at all yet...
+    position = None
+
+    return (course_id, chapter, section, position)

common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py

@@ -8,6 +8,7 @@ from xmodule.modulestore import Location
 from xmodule.modulestore.exceptions import InvalidLocationError, ItemNotFoundError, NoPathToItem
 from xmodule.modulestore.mongo import MongoModuleStore
 from xmodule.modulestore.xml_importer import import_from_xml
+from xmodule.modulestore.search import path_to_location
 
 # from ~/mitx_all/mitx/common/lib/xmodule/xmodule/modulestore/tests/
 # to   ~/mitx_all/mitx/common/test
@@ -28,7 +29,7 @@ DEFAULT_CLASS = 'xmodule.raw_module.RawDescriptor'
 
 
 class TestMongoModuleStore(object):
-
+    '''Tests!'''
     @classmethod
     def setupClass(cls):
         cls.connection = pymongo.connection.Connection(HOST, PORT)
@@ -67,7 +68,7 @@ class TestMongoModuleStore(object):
 
     def test_init(self):
         '''Make sure the db loads, and print all the locations in the db.
-        Call this directly from failing tests to see what's loaded'''
+        Call this directly from failing tests to see what is loaded'''
         ids = list(self.connection[DB][COLLECTION].find({}, {'_id': True}))
 
         pprint([Location(i['_id']).url() for i in ids])
@@ -93,8 +94,6 @@ class TestMongoModuleStore(object):
             self.store.get_item("i4x://edX/toy/video/Welcome"),
             None)
 
-
-
     def test_find_one(self):
         assert_not_equals(
             self.store._find_one(Location("i4x://edX/toy/course/2012_Fall")),
@@ -117,13 +116,13 @@ class TestMongoModuleStore(object):
              ("edX/toy/2012_Fall", "Overview", "Toy_Videos", None)),
             )
         for location, expected in should_work:
-            assert_equals(self.store.path_to_location(location), expected)
+            assert_equals(path_to_location(self.store, location), expected)
 
         not_found = (
-            "i4x://edX/toy/video/WelcomeX",
+            "i4x://edX/toy/video/WelcomeX", "i4x://edX/toy/course/NotHome"
             )
         for location in not_found:
-            assert_raises(ItemNotFoundError, self.store.path_to_location, location)
+            assert_raises(ItemNotFoundError, path_to_location, self.store, location)
 
         # Since our test files are valid, there shouldn't be any
         # elements with no path to them.  But we can look for them in
@@ -132,5 +131,5 @@ class TestMongoModuleStore(object):
             "i4x://edX/simple/video/Lost_Video",
             )
         for location in no_path:
-            assert_raises(NoPathToItem, self.store.path_to_location, location, "toy")
+            assert_raises(NoPathToItem, path_to_location, self.store, location, "toy")
 

common/lib/xmodule/xmodule/modulestore/xml.py

@@ -1,16 +1,18 @@
 import logging
+import os
+import re
+
 from fs.osfs import OSFS
 from importlib import import_module
 from lxml import etree
 from path import path
-from xmodule.errorhandlers import logging_error_handler
+from xmodule.errortracker import ErrorLog, make_error_tracker
 from xmodule.x_module import XModuleDescriptor, XMLParsingSystem
+from xmodule.course_module import CourseDescriptor
 from xmodule.mako_module import MakoDescriptorSystem
 from cStringIO import StringIO
-import os
-import re
 
-from . import ModuleStore, Location
+from . import ModuleStoreBase, Location
 from .exceptions import ItemNotFoundError
 
 etree.set_default_parser(
@@ -19,7 +21,6 @@ etree.set_default_parser(
 
 log = logging.getLogger('mitx.' + __name__)
 
-
 # VS[compat]
 # TODO (cpennington): Remove this once all fall 2012 courses have been imported
 # into the cms from xml
@@ -29,7 +30,7 @@ def clean_out_mako_templating(xml_string):
     return xml_string
 
 class ImportSystem(XMLParsingSystem, MakoDescriptorSystem):
-    def __init__(self, xmlstore, org, course, course_dir, error_handler):
+    def __init__(self, xmlstore, org, course, course_dir, error_tracker, **kwargs):
         """
         A class that handles loading from xml.  Does some munging to ensure that
         all elements have unique slugs.
@@ -40,6 +41,9 @@ class ImportSystem(XMLParsingSystem, MakoDescriptorSystem):
         self.used_slugs = set()
 
         def process_xml(xml):
+            """Takes an xml string, and returns a XModuleDescriptor created from
+            that xml.
+            """
             try:
                 # VS[compat]
                 # TODO (cpennington): Remove this once all fall 2012 courses
@@ -70,37 +74,36 @@ class ImportSystem(XMLParsingSystem, MakoDescriptorSystem):
                 # log.debug('-> slug=%s' % slug)
                 xml_data.set('url_name', slug)
 
-            module = XModuleDescriptor.load_from_xml(
+            descriptor = XModuleDescriptor.load_from_xml(
                 etree.tostring(xml_data), self, org,
                 course, xmlstore.default_class)
 
-            #log.debug('==> importing module location %s' % repr(module.location))
-            module.metadata['data_dir'] = course_dir
+            #log.debug('==> importing descriptor location %s' %
+            #          repr(descriptor.location))
+            descriptor.metadata['data_dir'] = course_dir
 
-            xmlstore.modules[module.location] = module
+            xmlstore.modules[descriptor.location] = descriptor
 
             if xmlstore.eager:
-                module.get_children()
-            return module
+                descriptor.get_children()
+            return descriptor
 
         render_template = lambda: ''
         load_item = xmlstore.get_item
         resources_fs = OSFS(xmlstore.data_dir / course_dir)
 
         MakoDescriptorSystem.__init__(self, load_item, resources_fs,
-                                      error_handler, render_template)
+                                      error_tracker, render_template, **kwargs)
         XMLParsingSystem.__init__(self, load_item, resources_fs,
-                                  error_handler, process_xml)
+                                  error_tracker, process_xml, **kwargs)
 
 
-
-class XMLModuleStore(ModuleStore):
+class XMLModuleStore(ModuleStoreBase):
     """
     An XML backed ModuleStore
     """
     def __init__(self, data_dir, default_class=None, eager=False,
-                 course_dirs=None,
-                 error_handler=logging_error_handler):
+                 course_dirs=None):
         """
         Initialize an XMLModuleStore from data_dir
 
@@ -114,17 +117,13 @@ class XMLModuleStore(ModuleStore):
 
         course_dirs: If specified, the list of course_dirs to load. Otherwise,
             load all course dirs
-
-        error_handler: The error handler used here and in the underlying
-                DescriptorSystem.  By default, raise exceptions for all errors.
-                See the comments in x_module.py:DescriptorSystem
         """
+        ModuleStoreBase.__init__(self)
 
         self.eager = eager
         self.data_dir = path(data_dir)
         self.modules = {}  # location -> XModuleDescriptor
         self.courses = {}  # course_dir -> XModuleDescriptor for the course
-        self.error_handler = error_handler
 
         if default_class is None:
             self.default_class = None
@@ -147,16 +146,32 @@ class XMLModuleStore(ModuleStore):
                            os.path.exists(self.data_dir / d / "course.xml")]
 
         for course_dir in course_dirs:
-            try:
-                course_descriptor = self.load_course(course_dir)
-                self.courses[course_dir] = course_descriptor
-            except:
-                msg = "Failed to load course '%s'" % course_dir
-                log.exception(msg)
-                error_handler(msg)
+            self.try_load_course(course_dir)
 
+    def try_load_course(self,course_dir):
+        '''
+        Load a course, keeping track of errors as we go along.
+        '''
+        try:
+            # Special-case code here, since we don't have a location for the
+            # course before it loads.
+            # So, make a tracker to track load-time errors, then put in the right
+            # place after the course loads and we have its location
+            errorlog = make_error_tracker()
+            course_descriptor = self.load_course(course_dir, errorlog.tracker)
+            self.courses[course_dir] = course_descriptor
+            self._location_errors[course_descriptor.location] = errorlog
+        except:
+            msg = "Failed to load course '%s'" % course_dir
+            log.exception(msg)
 
-    def load_course(self, course_dir):
+    def __unicode__(self):
+        '''
+        String representation - for debugging
+        '''
+        return '<XMLModuleStore>data_dir=%s, %d courses, %d modules' % (self.data_dir,len(self.courses),len(self.modules))
+
+    def load_course(self, course_dir, tracker):
         """
         Load a course into this module store
         course_path: Course directory name
@@ -190,13 +205,13 @@ class XMLModuleStore(ModuleStore):
                         ))
                 course = course_dir
 
-            system = ImportSystem(self, org, course, course_dir,
-                                  self.error_handler)
+            system = ImportSystem(self, org, course, course_dir, tracker)
 
             course_descriptor = system.process_xml(etree.tostring(course_data))
             log.debug('========> Done with course import from {0}'.format(course_dir))
             return course_descriptor
 
+
     def get_item(self, location, depth=0):
         """
         Returns an XModuleDescriptor instance for the item at location.
@@ -217,15 +232,19 @@ class XMLModuleStore(ModuleStore):
         except KeyError:
             raise ItemNotFoundError(location)
 
+
     def get_courses(self, depth=0):
         """
-        Returns a list of course descriptors
+        Returns a list of course descriptors.  If there were errors on loading,
+        some of these may be ErrorDescriptors instead.
         """
         return self.courses.values()
 
+
     def create_item(self, location):
         raise NotImplementedError("XMLModuleStores are read-only")
 
+
     def update_item(self, location, data):
         """
         Set the data in the item specified by the location to
@@ -236,6 +255,7 @@ class XMLModuleStore(ModuleStore):
         """
         raise NotImplementedError("XMLModuleStores are read-only")
 
+
     def update_children(self, location, children):
         """
         Set the children for the item specified by the location to
@@ -246,6 +266,7 @@ class XMLModuleStore(ModuleStore):
         """
         raise NotImplementedError("XMLModuleStores are read-only")
 
+
     def update_metadata(self, location, metadata):
         """
         Set the metadata for the item specified by the location to

common/lib/xmodule/xmodule/modulestore/xml_importer.py

@@ -35,6 +35,8 @@ def import_from_xml(store, data_dir, course_dirs=None, eager=True,
             store.update_item(module.location, module.definition['data'])
         if 'children' in module.definition:
             store.update_children(module.location, module.definition['children'])
-        store.update_metadata(module.location, dict(module.metadata))
+        # 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

common/lib/xmodule/xmodule/raw_module.py

@@ -1,27 +1,16 @@
-from pkg_resources import resource_string
 from lxml import etree
-from xmodule.mako_module import MakoModuleDescriptor
+from xmodule.editing_module import EditingDescriptor
 from xmodule.xml_module import XmlDescriptor
 import logging
+import sys
 
 log = logging.getLogger(__name__)
 
-
-class RawDescriptor(MakoModuleDescriptor, XmlDescriptor):
+class RawDescriptor(XmlDescriptor, EditingDescriptor):
     """
-    Module that provides a raw editing view of its data and children
+    Module that provides a raw editing view of its data and children.  It
+    requires that the definition xml is valid.
     """
-    mako_template = "widgets/raw-edit.html"
-
-    js = {'coffee': [resource_string(__name__, 'js/src/raw/edit.coffee')]}
-    js_module_name = "RawDescriptor"
-
-    def get_context(self):
-        return {
-            'module': self,
-            'data': self.definition['data'],
-        }
-
     @classmethod
     def definition_from_xml(cls, xml_object, system):
         return {'data': etree.tostring(xml_object)}
@@ -30,13 +19,12 @@ class RawDescriptor(MakoModuleDescriptor, XmlDescriptor):
         try:
             return etree.fromstring(self.definition['data'])
         except etree.XMLSyntaxError as err:
+            # Can't recover here, so just add some info and
+            # re-raise
             lines = self.definition['data'].split('\n')
             line, offset = err.position
             msg = ("Unable to create xml for problem {loc}. "
                    "Context: '{context}'".format(
                     context=lines[line - 1][offset - 40:offset + 40],
                     loc=self.location))
-            log.exception(msg)
-            self.system.error_handler(msg)
-            # no workaround possible, so just re-raise
-            raise
+            raise Exception, msg, sys.exc_info()[2]

common/lib/xmodule/xmodule/stringify.py

@@ -0,0 +1,20 @@
+from itertools import chain
+from lxml import etree
+
+def stringify_children(node):
+    '''
+    Return all contents of an xml tree, without the outside tags.
+    e.g. if node is parse of
+        "<html a="b" foo="bar">Hi <div>there <span>Bruce</span><b>!</b></div><html>"
+    should return
+        "Hi <div>there <span>Bruce</span><b>!</b></div>"
+
+    fixed from
+    http://stackoverflow.com/questions/4624062/get-all-text-inside-a-tag-in-lxml
+    '''
+    parts = ([node.text] +
+            list(chain(*([etree.tostring(c), c.tail]
+                         for c in node.getchildren())
+                         )))
+    # filter removes possible Nones in texts and tails
+    return ''.join(filter(None, parts))

common/lib/xmodule/xmodule/template_module.py

@@ -7,16 +7,14 @@ from mako.template import Template
 class CustomTagModule(XModule):
     """
     This module supports tags of the form
-    <customtag option="val" option2="val2">
-        <impl>$tagname</impl>
-    </customtag>
+    <customtag option="val" option2="val2" impl="tagname"/>
 
     In this case, $tagname should refer to a file in data/custom_tags, which contains
     a mako template that uses ${option} and ${option2} for the content.
 
     For instance:
 
-    data/custom_tags/book::
+    data/mycourse/custom_tags/book::
         More information given in <a href="/book/${page}">the text</a>
 
     course.xml::
@@ -34,7 +32,18 @@ class CustomTagModule(XModule):
                          instance_state, shared_state, **kwargs)
 
         xmltree = etree.fromstring(self.definition['data'])
-        template_name = xmltree.attrib['impl']
+        if 'impl' in xmltree.attrib:
+            template_name = xmltree.attrib['impl']
+        else:
+            # VS[compat]  backwards compatibility with old nested customtag structure
+            child_impl = xmltree.find('impl')
+            if child_impl is not None:
+                template_name = child_impl.text
+            else:
+                # TODO (vshnayder): better exception type
+                raise Exception("Could not find impl attribute in customtag {0}"
+                                .format(location))
+
         params = dict(xmltree.items())
         with self.system.filestore.open(
                 'custom_tags/{name}'.format(name=template_name)) as template:

common/lib/xmodule/xmodule/tests/__init__.py

@@ -31,7 +31,8 @@ i4xs = ModuleSystem(
     user=Mock(),
     filestore=fs.osfs.OSFS(os.path.dirname(os.path.realpath(__file__))),
     debug=True,
-    xqueue_callback_url='/'
+    xqueue_callback_url='/',
+    is_staff=False
 )
 
 

common/lib/xmodule/xmodule/tests/test_export.py

@@ -1,5 +1,6 @@
 from xmodule.modulestore.xml import XMLModuleStore
 from nose.tools import assert_equals
+from nose import SkipTest
 from tempfile import mkdtemp
 from fs.osfs import OSFS
 
@@ -26,3 +27,10 @@ def check_export_roundtrip(data_dir):
     for location in initial_import.modules.keys():
         print "Checking", location
         assert_equals(initial_import.modules[location], second_import.modules[location])
+
+
+def test_toy_roundtrip():
+    dir = ""
+    # TODO: add paths and make this run.
+    raise SkipTest()
+    check_export_roundtrip(dir)

common/lib/xmodule/xmodule/tests/test_import.py

@@ -0,0 +1,140 @@
+from path import path
+import unittest
+from fs.memoryfs import MemoryFS
+
+from lxml import etree
+
+from xmodule.x_module import XMLParsingSystem, XModuleDescriptor
+from xmodule.errortracker import make_error_tracker
+from xmodule.modulestore import Location
+from xmodule.modulestore.exceptions import ItemNotFoundError
+
+ORG = 'test_org'
+COURSE = 'test_course'
+
+class DummySystem(XMLParsingSystem):
+    def __init__(self):
+
+        self.modules = {}
+        self.resources_fs = MemoryFS()
+        self.errorlog = make_error_tracker()
+
+        def load_item(loc):
+            loc = Location(loc)
+            if loc in self.modules:
+                return self.modules[loc]
+
+            print "modules: "
+            print self.modules
+            raise ItemNotFoundError("Can't find item at loc: {0}".format(loc))
+
+        def process_xml(xml):
+            print "loading {0}".format(xml)
+            descriptor = XModuleDescriptor.load_from_xml(xml, self, ORG, COURSE, None)
+            # Need to save module so we can find it later
+            self.modules[descriptor.location] = descriptor
+
+            # always eager
+            descriptor.get_children()
+            return descriptor
+
+
+        XMLParsingSystem.__init__(self, load_item, self.resources_fs,
+                                  self.errorlog.tracker, process_xml)
+
+    def render_template(self, template, context):
+            raise Exception("Shouldn't be called")
+
+
+
+
+class ImportTestCase(unittest.TestCase):
+    '''Make sure module imports work properly, including for malformed inputs'''
+
+
+    @staticmethod
+    def get_system():
+        '''Get a dummy system'''
+        return DummySystem()
+
+    def test_fallback(self):
+        '''Make sure that malformed xml loads as an ErrorDescriptor.'''
+
+        bad_xml = '''<sequential display_name="oops"><video url="hi"></sequential>'''
+
+        system = self.get_system()
+
+        descriptor = XModuleDescriptor.load_from_xml(bad_xml, system, 'org', 'course',
+                                                     None)
+
+        self.assertEqual(descriptor.__class__.__name__,
+                         'ErrorDescriptor')
+
+    def test_reimport(self):
+        '''Make sure an already-exported error xml tag loads properly'''
+
+        self.maxDiff = None
+        bad_xml = '''<sequential display_name="oops"><video url="hi"></sequential>'''
+        system = self.get_system()
+        descriptor = XModuleDescriptor.load_from_xml(bad_xml, system, 'org', 'course',
+                                                     None)
+        resource_fs = None
+        tag_xml = descriptor.export_to_xml(resource_fs)
+        re_import_descriptor = XModuleDescriptor.load_from_xml(tag_xml, system,
+                                                               'org', 'course',
+                                                               None)
+        self.assertEqual(re_import_descriptor.__class__.__name__,
+                         'ErrorDescriptor')
+
+        self.assertEqual(descriptor.definition['data'],
+                         re_import_descriptor.definition['data'])
+
+    def test_fixed_xml_tag(self):
+        """Make sure a tag that's been fixed exports as the original tag type"""
+
+        # create a error tag with valid xml contents
+        root = etree.Element('error')
+        good_xml = '''<sequential display_name="fixed"><video url="hi"/></sequential>'''
+        root.text = good_xml
+
+        xml_str_in = etree.tostring(root)
+
+        # load it
+        system = self.get_system()
+        descriptor = XModuleDescriptor.load_from_xml(xml_str_in, system, 'org', 'course',
+                                                     None)
+        # export it
+        resource_fs = None
+        xml_str_out = descriptor.export_to_xml(resource_fs)
+
+        # Now make sure the exported xml is a sequential
+        xml_out = etree.fromstring(xml_str_out)
+        self.assertEqual(xml_out.tag, 'sequential')
+
+    def test_metadata_inherit(self):
+        """Make sure metadata inherits properly"""
+        system = self.get_system()
+        v = "1 hour"
+        start_xml = '''<course graceperiod="{grace}" url_name="test1" display_name="myseq">
+                      <chapter url="hi" url_name="ch" display_name="CH">
+                       <html url_name="h" display_name="H">Two houses, ...</html></chapter>
+                   </course>'''.format(grace=v)
+        descriptor = XModuleDescriptor.load_from_xml(start_xml, system,
+                                                     'org', 'course')
+
+        print "Errors: {0}".format(system.errorlog.errors)
+        print descriptor, descriptor.metadata
+        self.assertEqual(descriptor.metadata['graceperiod'], v)
+
+        # Check that the child inherits correctly
+        child = descriptor.get_children()[0]
+        self.assertEqual(child.metadata['graceperiod'], v)
+
+        # Now export and see if the chapter tag has a graceperiod attribute
+        resource_fs = MemoryFS()
+        exported_xml = descriptor.export_to_xml(resource_fs)
+        print "Exported xml:", exported_xml
+        root = etree.fromstring(exported_xml)
+        chapter_tag = root[0]
+        self.assertEqual(chapter_tag.tag, 'chapter')
+        self.assertFalse('graceperiod' in chapter_tag.attrib)

common/lib/xmodule/xmodule/tests/test_stringify.py

@@ -0,0 +1,10 @@
+from nose.tools import assert_equals
+from lxml import etree
+from xmodule.stringify import stringify_children
+
+def test_stringify():
+    text = 'Hi <div x="foo">there <span>Bruce</span><b>!</b></div>'
+    html = '''<html a="b" foo="bar">{0}</html>'''.format(text)
+    xml = etree.fromstring(html)
+    out = stringify_children(xml)
+    assert_equals(out, text)

common/lib/xmodule/xmodule/video_module.py

@@ -23,11 +23,12 @@ class VideoModule(XModule):
     css = {'scss': [resource_string(__name__, 'css/video/display.scss')]}
     js_module_name = "Video"
 
-    def __init__(self, system, location, definition, instance_state=None, shared_state=None, **kwargs):
-        XModule.__init__(self, system, location, definition, instance_state, shared_state, **kwargs)
+    def __init__(self, system, location, definition,
+                 instance_state=None, shared_state=None, **kwargs):
+        XModule.__init__(self, system, location, definition,
+                         instance_state, shared_state, **kwargs)
         xmltree = etree.fromstring(self.definition['data'])
         self.youtube = xmltree.get('youtube')
-        self.name = xmltree.get('name')
         self.position = 0
 
         if instance_state is not None:
@@ -71,7 +72,7 @@ class VideoModule(XModule):
             'streams': self.video_list(),
             'id': self.location.html_id(),
             'position': self.position,
-            'name': self.name,
+            'display_name': self.display_name,
             # TODO (cpennington): This won't work when we move to data that isn't on the filesystem
             'data_dir': self.metadata['data_dir'],
         })

common/lib/xmodule/xmodule/x_module.py

@@ -1,10 +1,14 @@
-from lxml import etree
-import pkg_resources
 import logging
+import pkg_resources
+import sys
+
+from fs.errors import ResourceNotFoundError
+from functools import partial
+from lxml import etree
+from lxml.etree import XMLSyntaxError
 
 from xmodule.modulestore import Location
-
-from functools import partial
+from xmodule.errortracker import exc_info_to_str
 
 log = logging.getLogger('mitx.' + __name__)
 
@@ -187,13 +191,21 @@ class XModule(HTMLSnippet):
         self.instance_state = instance_state
         self.shared_state = shared_state
         self.id = self.location.url()
-        self.name = self.location.name
+        self.url_name = self.location.name
         self.category = self.location.category
         self.metadata = kwargs.get('metadata', {})
         self._loaded_children = None
 
-    def get_name(self):
-        return self.name
+    @property
+    def display_name(self):
+        '''
+        Return a display name for the module: use display_name if defined in
+        metadata, otherwise convert the url name.
+        '''
+        return self.metadata.get('display_name',
+                                 self.url_name.replace('_', ' '))
+    def __unicode__(self):
+        return '<x_module(name=%s, category=%s, id=%s)>' % (self.name, self.category, self.id)
 
     def get_children(self):
         '''
@@ -338,6 +350,8 @@ class XModuleDescriptor(Plugin, HTMLSnippet):
                     module
                 display_name: The name to use for displaying this module to the
                     user
+                url_name: The name to use for this module in urls and other places
+                    where a unique name is needed.
                 format: The format of this module ('Homework', 'Lab', etc)
                 graded (bool): Whether this module is should be graded or not
                 start (string): The date for which this module will be available
@@ -352,13 +366,30 @@ class XModuleDescriptor(Plugin, HTMLSnippet):
         self.metadata = kwargs.get('metadata', {})
         self.definition = definition if definition is not None else {}
         self.location = Location(kwargs.get('location'))
-        self.name = self.location.name
+        self.url_name = self.location.name
         self.category = self.location.category
         self.shared_state_key = kwargs.get('shared_state_key')
 
         self._child_instances = None
         self._inherited_metadata = set()
 
+    @property
+    def display_name(self):
+        '''
+        Return a display name for the module: use display_name if defined in
+        metadata, otherwise convert the url name.
+        '''
+        return self.metadata.get('display_name',
+                                 self.url_name.replace('_', ' '))
+
+    @property
+    def own_metadata(self):
+        """
+        Return the metadata that is not inherited, but was defined on this module.
+        """
+        return dict((k,v) for k,v in self.metadata.items()
+                    if k not in self._inherited_metadata)
+
     def inherit_metadata(self, metadata):
         """
         Updates this module with metadata inherited from a containing module.
@@ -443,16 +474,32 @@ class XModuleDescriptor(Plugin, HTMLSnippet):
         system is an XMLParsingSystem
 
         org and course are optional strings that will be used in the generated
-            modules url identifiers
+            module's url identifiers
         """
-        class_ = XModuleDescriptor.load_class(
-            etree.fromstring(xml_data).tag,
-            default_class
-        )
-        # leave next line, commented out - useful for low-level debugging
-        # log.debug('[XModuleDescriptor.load_from_xml] tag=%s, class_=%s' % (
-        #        etree.fromstring(xml_data).tag,class_))
-        return class_.from_xml(xml_data, system, org, course)
+        try:
+            class_ = XModuleDescriptor.load_class(
+                etree.fromstring(xml_data).tag,
+                default_class
+                )
+            # leave next line, commented out - useful for low-level debugging
+            # log.debug('[XModuleDescriptor.load_from_xml] tag=%s, class_=%s' % (
+            #        etree.fromstring(xml_data).tag,class_))
+
+            descriptor = class_.from_xml(xml_data, system, org, course)
+        except Exception as err:
+            # Didn't load properly.  Fall back on loading as an error
+            # descriptor.  This should never error due to formatting.
+
+            # Put import here to avoid circular import errors
+            from xmodule.error_module import ErrorDescriptor
+            msg = "Error loading from xml."
+            log.exception(msg)
+            system.error_tracker(msg)
+            err_msg = msg + "\n" + exc_info_to_str(sys.exc_info())
+            descriptor = ErrorDescriptor.from_xml(xml_data, system, org, course,
+                                                  err_msg)
+
+        return descriptor
 
     @classmethod
     def from_xml(cls, xml_data, system, org=None, course=None):
@@ -521,16 +568,19 @@ class XModuleDescriptor(Plugin, HTMLSnippet):
 
 
 class DescriptorSystem(object):
-    def __init__(self, load_item, resources_fs, error_handler):
+    def __init__(self, load_item, resources_fs, error_tracker, **kwargs):
         """
         load_item: Takes a Location and returns an XModuleDescriptor
 
         resources_fs: A Filesystem object that contains all of the
             resources needed for the course
 
-        error_handler: A hook for handling errors in loading the descriptor.
-            Must be a function of (error_msg, exc_info=None).
-            See errorhandlers.py for some simple ones.
+        error_tracker: A hook for tracking errors in loading the descriptor.
+            Used for example to get a list of all non-fatal problems on course
+            load, and display them to the user.
+
+            A function of (error_msg). errortracker.py provides a
+            handy make_error_tracker() function.
 
             Patterns for using the error handler:
                try:
@@ -539,10 +589,8 @@ class DescriptorSystem(object):
                except SomeProblem:
                   msg = 'Grommet {0} is broken'.format(x)
                   log.exception(msg) # don't rely on handler to log
-                  self.system.error_handler(msg)
-                  # if we get here, work around if possible
-                  raise # if no way to work around
-                       OR
+                  self.system.error_tracker(msg)
+                  # work around
                   return 'Oops, couldn't load grommet'
 
                OR, if not in an exception context:
@@ -550,25 +598,27 @@ class DescriptorSystem(object):
                if not check_something(thingy):
                   msg = "thingy {0} is broken".format(thingy)
                   log.critical(msg)
-                  error_handler(msg)
-                  # if we get here, work around
-                  pass   # e.g. if no workaround needed
+                  self.system.error_tracker(msg)
+
+               NOTE: To avoid duplication, do not call the tracker on errors
+               that you're about to re-raise---let the caller track them.
         """
 
         self.load_item = load_item
         self.resources_fs = resources_fs
-        self.error_handler = error_handler
+        self.error_tracker = error_tracker
 
 
 class XMLParsingSystem(DescriptorSystem):
-    def __init__(self, load_item, resources_fs, error_handler, process_xml):
+    def __init__(self, load_item, resources_fs, error_tracker, process_xml, **kwargs):
         """
-        load_item, resources_fs, error_handler: see DescriptorSystem
+        load_item, resources_fs, error_tracker: see DescriptorSystem
 
         process_xml: Takes an xml string, and returns a XModuleDescriptor
             created from that xml
         """
-        DescriptorSystem.__init__(self, load_item, resources_fs, error_handler)
+        DescriptorSystem.__init__(self, load_item, resources_fs, error_tracker,
+                                  **kwargs)
         self.process_xml = process_xml
 
 
@@ -584,10 +634,18 @@ class ModuleSystem(object):
     Note that these functions can be closures over e.g. a django request
     and user, or other environment-specific info.
     '''
-    def __init__(self, ajax_url, track_function,
-                 get_module, render_template, replace_urls,
-                 user=None, filestore=None, debug=False,
-                 xqueue=None):
+    def __init__(self,
+                 ajax_url,
+                 track_function,
+                 get_module,
+                 render_template,
+                 replace_urls,
+                 user=None,
+                 filestore=None,
+                 debug=False,
+                 xqueue_callback_url=None,
+                 xqueue_default_queuename="null",
+                 is_staff=False):
         '''
         Create a closure around the system environment.
 
@@ -613,9 +671,13 @@ class ModuleSystem(object):
         replace_urls - TEMPORARY - A function like static_replace.replace_urls
                          that capa_module can use to fix up the static urls in
                          ajax results.
+
+        is_staff - Is the user making the request a staff user?
+             TODO (vshnayder): this will need to change once we have real user roles.
         '''
         self.ajax_url = ajax_url
-        self.xqueue = xqueue
+        self.xqueue_callback_url = xqueue_callback_url
+        self.xqueue_default_queuename = xqueue_default_queuename
         self.track_function = track_function
         self.filestore = filestore
         self.get_module = get_module
@@ -623,6 +685,7 @@ class ModuleSystem(object):
         self.DEBUG = self.debug = debug
         self.seed = user.id if user is not None else 0
         self.replace_urls = replace_urls
+        self.is_staff = is_staff
 
     def get(self, attr):
         '''	provide uniform access to attributes (like etree).'''

common/lib/xmodule/xmodule/xml_module.py

@@ -1,4 +1,3 @@
-from collections import MutableMapping
 from xmodule.x_module import XModuleDescriptor
 from xmodule.modulestore import Location
 from lxml import etree
@@ -8,74 +7,12 @@ import traceback
 from collections import namedtuple
 from fs.errors import ResourceNotFoundError
 import os
+import sys
 
 log = logging.getLogger(__name__)
 
-# TODO (cpennington): This was implemented in an attempt to improve performance,
-# but the actual improvement wasn't measured (and it was implemented late at night).
-# We should check if it hurts, and whether there's a better way of doing lazy loading
-
-
-class LazyLoadingDict(MutableMapping):
-    """
-    A dictionary object that lazily loads its contents from a provided
-    function on reads (of members that haven't already been set).
-    """
-
-    def __init__(self, loader):
-        '''
-        On the first read from this dictionary, it will call loader() to
-        populate its contents.  loader() must return something dict-like. Any
-        elements set before the first read will be preserved.
-        '''
-        self._contents = {}
-        self._loaded = False
-        self._loader = loader
-        self._deleted = set()
-
-    def __getitem__(self, name):
-        if not (self._loaded or name in self._contents or name in self._deleted):
-            self.load()
-
-        return self._contents[name]
-
-    def __setitem__(self, name, value):
-        self._contents[name] = value
-        self._deleted.discard(name)
-
-    def __delitem__(self, name):
-        del self._contents[name]
-        self._deleted.add(name)
-
-    def __contains__(self, name):
-        self.load()
-        return name in self._contents
-
-    def __len__(self):
-        self.load()
-        return len(self._contents)
-
-    def __iter__(self):
-        self.load()
-        return iter(self._contents)
-
-    def __repr__(self):
-        self.load()
-        return repr(self._contents)
-
-    def load(self):
-        if self._loaded:
-            return
-
-        loaded_contents = self._loader()
-        loaded_contents.update(self._contents)
-        self._contents = loaded_contents
-        self._loaded = True
-
-
 _AttrMapBase = namedtuple('_AttrMap', 'metadata_key to_metadata from_metadata')
 
-
 class AttrMap(_AttrMapBase):
     """
     A class that specifies a metadata_key, and two functions:
@@ -104,6 +41,7 @@ class XmlDescriptor(XModuleDescriptor):
     # to definition_from_xml, and from the xml returned by definition_to_xml
     metadata_attributes = ('format', 'graceperiod', 'showanswer', 'rerandomize',
         'start', 'due', 'graded', 'display_name', 'url_name', 'hide_from_toc',
+        'ispublic', 	# if True, then course is listed for all users; see 
         # VS[compat] Remove once unused.
         'name', 'slug')
 
@@ -163,6 +101,52 @@ class XmlDescriptor(XModuleDescriptor):
         """
         return etree.parse(file_object).getroot()
 
+    @classmethod
+    def load_definition(cls, xml_object, system, location):
+        '''Load a descriptor definition from the specified xml_object.
+        Subclasses should not need to override this except in special
+        cases (e.g. html module)'''
+
+        filename = xml_object.get('filename')
+        if filename is None:
+            definition_xml = copy.deepcopy(xml_object)
+            filepath = ''
+        else:
+            filepath = cls._format_filepath(xml_object.tag, filename)
+
+            # VS[compat]
+            # TODO (cpennington): If the file doesn't exist at the right path,
+            # give the class a chance to fix it up. The file will be written out
+            # again in the correct format.  This should go away once the CMS is
+            # online and has imported all current (fall 2012) courses from xml
+            if not system.resources_fs.exists(filepath) and hasattr(
+                    cls,
+                    'backcompat_paths'):
+                candidates = cls.backcompat_paths(filepath)
+                for candidate in candidates:
+                    if system.resources_fs.exists(candidate):
+                        filepath = candidate
+                        break
+
+            try:
+                with system.resources_fs.open(filepath) as file:
+                    definition_xml = cls.file_to_xml(file)
+            except Exception:
+                msg = 'Unable to load file contents at path %s for item %s' % (
+                    filepath, location.url())
+                # Add info about where we are, but keep the traceback
+                raise Exception, msg, sys.exc_info()[2]
+
+        cls.clean_metadata_from_xml(definition_xml)
+        definition = cls.definition_from_xml(definition_xml, system)
+
+        # TODO (ichuang): remove this after migration
+        # for Fall 2012 LMS migration: keep filename (and unmangled filename)
+        definition['filename'] = [ filepath, filename ]
+
+        return definition
+
+
     @classmethod
     def from_xml(cls, xml_data, system, org=None, course=None):
         """
@@ -180,7 +164,7 @@ class XmlDescriptor(XModuleDescriptor):
         slug = xml_object.get('url_name', xml_object.get('slug'))
         location = Location('i4x', org, course, xml_object.tag, slug)
 
-        def metadata_loader():
+        def load_metadata():
             metadata = {}
             for attr in cls.metadata_attributes:
                 val = xml_object.get(attr)
@@ -192,49 +176,15 @@ class XmlDescriptor(XModuleDescriptor):
                     metadata[attr_map.metadata_key] = attr_map.to_metadata(val)
             return metadata
 
-        def definition_loader():
-            filename = xml_object.get('filename')
-            if filename is None:
-                definition_xml = copy.deepcopy(xml_object)
-            else:
-                filepath = cls._format_filepath(xml_object.tag, filename)
-
-                # VS[compat]
-                # TODO (cpennington): If the file doesn't exist at the right path,
-                # give the class a chance to fix it up. The file will be written out again
-                # in the correct format.
-                # This should go away once the CMS is online and has imported all current (fall 2012)
-                # courses from xml
-                if not system.resources_fs.exists(filepath) and hasattr(cls, 'backcompat_paths'):
-                    candidates = cls.backcompat_paths(filepath)
-                    for candidate in candidates:
-                        if system.resources_fs.exists(candidate):
-                            filepath = candidate
-                            break
-
-                try:
-                    with system.resources_fs.open(filepath) as file:
-                        definition_xml = cls.file_to_xml(file)
-                except (ResourceNotFoundError, etree.XMLSyntaxError):
-                    msg = 'Unable to load file contents at path %s for item %s' % (filepath, location.url())
-                    log.exception(msg)
-                    system.error_handler(msg)
-                    # if error_handler didn't reraise, work around problem.
-                    error_elem = etree.Element('error')
-                    message_elem = etree.SubElement(error_elem, 'error_message')
-                    message_elem.text = msg
-                    stack_elem = etree.SubElement(error_elem, 'stack_trace')
-                    stack_elem.text = traceback.format_exc()
-                    return {'data': etree.tostring(error_elem)}
-
-            cls.clean_metadata_from_xml(definition_xml)
-            return cls.definition_from_xml(definition_xml, system)
-
+        definition = cls.load_definition(xml_object, system, location)
+        metadata = load_metadata()
+        # VS[compat] -- just have the url_name lookup once translation is done
+        slug = xml_object.get('url_name', xml_object.get('slug'))
         return cls(
             system,
-            LazyLoadingDict(definition_loader),
+            definition,
             location=location,
-            metadata=LazyLoadingDict(metadata_loader),
+            metadata=metadata,
         )
 
     @classmethod
@@ -282,8 +232,8 @@ class XmlDescriptor(XModuleDescriptor):
 
         # Write it to a file if necessary
         if self.split_to_file(xml_object):
-            # Put this object in it's own file
-            filepath = self.__class__._format_filepath(self.category, self.name)
+            # Put this object in its own file
+            filepath = self.__class__._format_filepath(self.category, self.url_name)
             resource_fs.makedir(os.path.dirname(filepath), allow_recreate=True)
             with resource_fs.open(filepath, 'w') as file:
                 file.write(etree.tostring(xml_object, pretty_print=True))
@@ -296,10 +246,10 @@ class XmlDescriptor(XModuleDescriptor):
             xml_object.tail = ''
 
 
-            xml_object.set('filename', self.name)
+            xml_object.set('filename', self.url_name)
 
         # Add the metadata
-        xml_object.set('url_name', self.name)
+        xml_object.set('url_name', self.url_name)
         for attr in self.metadata_attributes:
             attr_map = self.xml_attribute_map.get(attr, AttrMap(attr))
             metadata_key = attr_map.metadata_key