Clean and refactor courseware/views.py and module_render.py

* Refactor index() so that it makes sense to me and hopefully others :)
* Rename preloaded cache of student modules to student_module_cache
* Fix line length and whitespace throughout
* add docstrings and other comments
* a few behavior-preserving tweaks to the code to make it clearer.
* Separate codepaths for with-module and without-module in index view
* Remove default chapter + section, since they don't exist anyway in course.xml

lms/djangoapps/courseware/module_render.py

@@ -26,8 +26,29 @@ class I4xSystem(object):
     This is an abstraction such that x_modules can function independent 
     of the courseware (e.g. import into other types of courseware, LMS, 
     or if we want to have a sandbox server for user-contributed content)
+
+    I4xSystem objects are passed to x_modules to provide access to system
+    functionality.
+    '''
+    def __init__(self, ajax_url, track_function, render_function,
+                 render_template, filestore=None):
+        '''
+        Create a closure around the system environment.
+
+        ajax_url - the url where ajax calls to the encapsulating module go.
+        track_function - function of (event_type, event), intended for logging
+                         or otherwise tracking the event.
+                         TODO: Not used, and has inconsistent args in different
+                         files.  Update or remove.
+        render_function - function that takes (module_xml) and renders it,
+                          returning a dictionary with a context for rendering the
+                          module to html.  Dictionary will contain keys 'content'
+                          and 'type'.
+        render_template - a function that takes (template_file, context), and returns
+                          rendered html.
+        filestore - A filestore ojbect.  Defaults to an instance of OSFS based at
+                    settings.DATA_DIR.
         '''
-    def __init__(self, ajax_url, track_function, render_function, render_template, filestore=None):
         self.ajax_url = ajax_url
         self.track_function = track_function
         if not filestore: 
@@ -35,37 +56,47 @@ class I4xSystem(object):
         else:
             self.filestore = filestore
             if settings.DEBUG:
-                log.info("[courseware.module_render.I4xSystem] filestore path = %s" % filestore)
+                log.info("[courseware.module_render.I4xSystem] filestore path = %s",
+                         filestore)
         self.render_function = render_function
         self.render_template = render_template
         self.exception404 = Http404
         self.DEBUG = settings.DEBUG
 
-    def get(self,attr):			# uniform access to attributes (like etree)
+    def get(self, attr):
+        '''	provide uniform access to attributes (like etree).'''
         return self.__dict__.get(attr)
-    def set(self,attr,val):		# uniform access to attributes (like etree)
+    
+    def set(self,attr,val):
+        '''provide uniform access to attributes (like etree)'''
         self.__dict__[attr] = val
+
     def __repr__(self):
         return repr(self.__dict__)
+
     def __str__(self):
         return str(self.__dict__)
 
-def object_cache(cache, user, module_type, module_id):
-    # We don't look up on user -- all queries include user
-    # Additional lookup would require a DB hit the way Django 
-    # is broken. 
+def smod_cache_lookup(cache, module_type, module_id):
+    '''
+    Look for a student module with the given type and id in the cache.
+
+    cache -- list of student modules
+
+    returns first found object, or None
+    '''
     for o in cache: 
-        if o.module_type == module_type and \
-                o.module_id == module_id:
+        if o.module_type == module_type and o.module_id == module_id:
             return o
     return None
 
 def make_track_function(request):
     ''' We want the capa problem (and other modules) to be able to
     track/log what happens inside them without adding dependencies on
-    Django or the rest of the codebase. We do this by passing a
-    tracking function to them. This generates a closure for each request 
-    that gives a clean interface on both sides. 
+    Django or the rest of the codebase.
+
+    To do this in a clean way, we pass a tracking function to the module,
+    which calls it to log events.
     '''
     import track.views
 
@@ -80,80 +111,86 @@ def grade_histogram(module_id):
     from django.db import connection
     cursor = connection.cursor()
 
-    cursor.execute("select courseware_studentmodule.grade,COUNT(courseware_studentmodule.student_id) from courseware_studentmodule where courseware_studentmodule.module_id=%s group by courseware_studentmodule.grade", [module_id])
+    q = """SELECT courseware_studentmodule.grade,
+                  COUNT(courseware_studentmodule.student_id)
+    FROM courseware_studentmodule
+    WHERE courseware_studentmodule.module_id=%s
+    GROUP BY courseware_studentmodule.grade"""
+    # Passing module_id this way prevents sql-injection.
+    cursor.execute(q, [module_id])
 
     grades = list(cursor.fetchall())
-    grades.sort(key=lambda x:x[0]) # Probably not necessary
-    if (len(grades) == 1 and grades[0][0] is None):
+    grades.sort(key=lambda x: x[0])          # Add ORDER BY to sql query?
+    if len(grades) == 1 and grades[0][0] is None:
         return []
     return grades
 
-def get_module(user, request, xml_module, module_object_preload, position=None):
-    ''' Get the appropriate xmodule and StudentModule.
+def get_module(user, request, module_xml, student_module_cache, position=None):
+    ''' Get an instance of the xmodule class corresponding to module_xml,
+    setting the state based on an existing StudentModule, or creating one if none
+    exists.
 
     Arguments:
       - user                  : current django User
       - request               : current django HTTPrequest
-      - xml_module            : lxml etree of xml subtree for the current module
-      - module_object_preload : list of StudentModule objects, one of which may match this module type and id
-      - position   	      : extra information from URL for user-specified position within module
+      - module_xml            : lxml etree of xml subtree for the requested module
+      - student_module_cache  : list of StudentModule objects, one of which may
+                                match this module type and id
+      - position   	          : extra information from URL for user-specified
+                                position within module
 
     Returns:
       - a tuple (xmodule instance, student module, module type).
-
     '''
-    module_type=xml_module.tag
-    module_class=xmodule.get_module_class(module_type)
-    module_id=xml_module.get('id') #module_class.id_attribute) or "" 
-
-    # Grab state from database
-    smod = object_cache(module_object_preload, 
-                        user, 
-                        module_type, 
-                        module_id)
-
-    if not smod: # If nothing in the database...
-        state=None
-    else:
-        state = smod.state
+    module_type = module_xml.tag
+    module_class = xmodule.get_module_class(module_type)
+    module_id = module_xml.get('id')
+
+    # Grab xmodule state from StudentModule cache
+    smod = smod_cache_lookup(student_module_cache, module_type, module_id)
+    state = smod.state if smod else None
 
-    # get coursename if stored
+    # get coursename if present in request
     coursename = multicourse_settings.get_coursename_from_request(request)
 
     if coursename and settings.ENABLE_MULTICOURSE:
-        xp = multicourse_settings.get_course_xmlpath(coursename)	# path to XML for the course
+        # path to XML for the course
+        xp = multicourse_settings.get_course_xmlpath(coursename)
         data_root = settings.DATA_DIR + xp
     else:
         data_root = settings.DATA_DIR
 
-    # Create a new instance
-    ajax_url = settings.MITX_ROOT_URL + '/modx/'+module_type+'/'+module_id+'/'
+    # Setup system context for module instance
+    ajax_url = settings.MITX_ROOT_URL + '/modx/' + module_type + '/' + module_id + '/'
+    def render_function(module_xml):
+        return render_x_module(user, request, module_xml, student_module_cache, position)
     
     system = I4xSystem(track_function = make_track_function(request), 
-                       render_function = lambda x: render_x_module(user, request, x, module_object_preload, position), 
+                       render_function = render_function, 
                        render_template = render_to_string,
                        ajax_url = ajax_url,
                        filestore = OSFS(data_root),
                        )
-    system.set('position',position)	# pass URL specified position along to module, through I4xSystem
-    instance=module_class(system, 
-                          etree.tostring(xml_module), 
+    # pass position specified in URL to module through I4xSystem
+    system.set('position', position) 
+    instance = module_class(system, 
+                            etree.tostring(module_xml), 
                             module_id, 
                             state=state)
     
-    # If instance wasn't already in the database, and this
-    # isn't a guest user, create it
+    # If StudentModule for this instance wasn't already in the database,
+    # and this isn't a guest user, create it.
     if not smod and user.is_authenticated():
-        smod=StudentModule(student=user, 
-                           module_type = module_type,
-                           module_id=module_id, 
-                           state=instance.get_state())
+        smod = StudentModule(student=user, module_type = module_type,
+                           module_id=module_id, state=instance.get_state())
         smod.save()
-        module_object_preload.append(smod)
+        # Add to cache. The caller and the system context have references
+        # to it, so the change persists past the return
+        student_module_cache.append(smod)
 
     return (instance, smod, module_type)
 
-def render_x_module(user, request, xml_module, module_object_preload, position=None):
+def render_x_module(user, request, module_xml, student_module_cache, position=None):
     ''' Generic module for extensions. This renders to HTML.
 
     modules include sequential, vertical, problem, video, html
@@ -164,37 +201,36 @@ def render_x_module(user, request, xml_module, module_object_preload, position=N
 
       - user                  : current django User
       - request               : current django HTTPrequest
-      - xml_module            : lxml etree of xml subtree for the current module
-      - module_object_preload : list of StudentModule objects, one of which may match this module type and id
+      - module_xml            : lxml etree of xml subtree for the current module
+      - student_module_cache : list of StudentModule objects, one of which may match this module type and id
       - position   	      : extra information from URL for user-specified position within module
 
     Returns:
 
-      -  dict which is context for HTML rendering of the specified module
-
+      - dict which is context for HTML rendering of the specified module.  Will have
+      key 'content', and will have 'type' key if passed a valid module.
     '''
-    if xml_module==None :
-        return {"content":""}
+    if module_xml is None :
+        return {"content": ""}
 
-    (instance, smod, module_type) = get_module(user, request, xml_module, module_object_preload, position)
+    (instance, smod, module_type) = get_module(
+        user, request, module_xml, student_module_cache, position)
 
-    # Grab content
     content = instance.get_html()
 
     # special extra information about each problem, only for users who are staff 
     if settings.MITX_FEATURES.get('DISPLAY_HISTOGRAMS_TO_STAFF') and user.is_staff:
-        module_id = xml_module.get('id')
+        module_id = module_xml.get('id')
         histogram = grade_histogram(module_id)
         render_histogram = len(histogram) > 0
-        content=content+render_to_string("staff_problem_info.html", {'xml':etree.tostring(xml_module), 
-                                                                     'module_id' : module_id,
+        staff_context = {'xml': etree.tostring(module_xml), 
+                         'module_id': module_id,
                          'histogram': json.dumps(histogram),
-                                                                     'render_histogram' : render_histogram})
-
-    content = {'content':content,
-               'type':module_type}
+                         'render_histogram': render_histogram}
+        content += render_to_string("staff_problem_info.html", staff_context)
 
-    return content
+    context = {'content': content, 'type': module_type}
+    return context
 
 def modx_dispatch(request, module=None, dispatch=None, id=None):
     ''' Generic view for extensions. This is where AJAX calls go.
@@ -210,32 +246,38 @@ def modx_dispatch(request, module=None, dispatch=None, id=None):
       - id -- the module id.  Used to look up the student module.
             e.g. filenamexformularesponse
     '''
+    # ''' (fix emacs broken parsing)
     if not request.user.is_authenticated():
         return redirect('/')
 
+    # python concats adjacent strings
+    error_msg = ("We're sorry, this module is temporarily unavailable."
+                 "Our staff is working to fix it as soon as possible")
+
+
     # Grab the student information for the module from the database
     s = StudentModule.objects.filter(student=request.user, 
                                      module_id=id)
-    #s = StudentModule.get_with_caching(request.user, id)
-    if len(s) == 0 or s is None:
-        log.debug("Couldnt find module for user and id " + str(module) + " " + str(request.user) + " "+ str(id))
+
+    # s = StudentModule.get_with_caching(request.user, id)
+    if s is None or len(s) == 0:
+        log.debug("Couldn't find module '%s' for user '%s' and id '%s'",
+                  module, request.user, id)
         raise Http404
     s = s[0]
 
     oldgrade = s.grade
     oldstate = s.state
 
-    # TODO: if dispatch is left at default value None, this will go boom.  What's the correct
-    # behavior?
-    dispatch=dispatch.split('?')[0]
-
-    ajax_url = settings.MITX_ROOT_URL + '/modx/'+module+'/'+id+'/'
+    # If there are arguments, get rid of them
+    if '?' in dispatch:
+        dispatch = dispatch.split('?')[0]
 
-    # get coursename if stored
+    ajax_url = '{root}/modx/{module}/{id}'.format(root = settings.MITX_ROOT_URL,
+                                                  module=module, id=id)
     coursename = multicourse_settings.get_coursename_from_request(request)
-
     if coursename and settings.ENABLE_MULTICOURSE:
-        xp = multicourse_settings.get_course_xmlpath(coursename)	# path to XML for the course
+        xp = multicourse_settings.get_course_xmlpath(coursename)
         data_root = settings.DATA_DIR + xp
     else:
         data_root = settings.DATA_DIR
@@ -244,11 +286,13 @@ def modx_dispatch(request, module=None, dispatch=None, id=None):
     try:
         xml = content_parser.module_xml(request.user, module, 'id', id, coursename)
     except:
-        log.exception("Unable to load module during ajax call. module=%s, dispatch=%s, id=%s" % (module, dispatch, id))
+        log.exception(
+            "Unable to load module during ajax call. module=%s, dispatch=%s, id=%s",
+            module, dispatch, id)
         if accepts(request, 'text/html'):
             return render_to_response("module-error.html", {})
         else:
-            response = HttpResponse(json.dumps({'success': "We're sorry, this module is temporarily unavailable. Our staff is working to fix it as soon as possible"}))
+            response = HttpResponse(json.dumps({'success': error_msg}))
         return response
 
     # Create the module
@@ -260,24 +304,23 @@ def modx_dispatch(request, module=None, dispatch=None, id=None):
                        )
 
     try:
-        instance=xmodule.get_module_class(module)(system, 
-                                                             xml, 
-                                                             id, 
-                                                             state=oldstate)
+        module_class = xmodule.get_module_class(module)
+        instance = module_class(system, xml, id, state=oldstate)
     except:
         log.exception("Unable to load module instance during ajax call")
         if accepts(request, 'text/html'):
             return render_to_response("module-error.html", {})
         else:
-            response = HttpResponse(json.dumps({'success': "We're sorry, this module is temporarily unavailable. Our staff is working to fix it as soon as possible"}))
+            response = HttpResponse(json.dumps({'success': error_msg}))
         return response
 
     # Let the module handle the AJAX
-    ajax_return=instance.handle_ajax(dispatch, request.POST)
+    ajax_return = instance.handle_ajax(dispatch, request.POST)
+
     # Save the state back to the database
-    s.state=instance.get_state()
+    s.state = instance.get_state()
     if instance.get_score(): 
-        s.grade=instance.get_score()['score']
+        s.grade = instance.get_score()['score']
     if s.grade != oldgrade or s.state != oldstate:
         s.save()
     # Return whatever the module wanted to return to the client/caller

lms/djangoapps/courseware/views.py

@@ -41,18 +41,18 @@ def gradebook(request):
     coursename = multicourse_settings.get_coursename_from_request(request)
 
     student_objects = User.objects.all()[:100]
-    student_info = [{'username' :s.username,
-                     'id' : s.id,
+    student_info = [{'username': s.username,
+                     'id': s.id,
                      'email': s.email,
-                     'grade_info' : grades.grade_sheet(s,coursename), 
-                     'realname' : UserProfile.objects.get(user = s).name
+                     'grade_info': grades.grade_sheet(s, coursename),
+                     'realname': UserProfile.objects.get(user = s).name
                      } for s in student_objects]
 
-    return render_to_response('gradebook.html',{'students':student_info})
+    return render_to_response('gradebook.html', {'students': student_info})
 
 @login_required
 @cache_control(no_cache=True, no_store=True, must_revalidate=True)
-def profile(request, student_id = None):
+def profile(request, student_id=None):
     ''' User profile. Show username, location, etc, as well as grades .
         We need to allow the user to change some of these settings .'''
 
@@ -67,36 +67,41 @@ def profile(request, student_id = None):
 
     coursename = multicourse_settings.get_coursename_from_request(request)
 
-    context={'name':user_info.name,
-             'username':student.username,
-             'location':user_info.location,
-             'language':user_info.language,
-             'email':student.email,
-             'format_url_params' : content_parser.format_url_params,
-             'csrf':csrf(request)['csrf_token']
+    context = {'name': user_info.name,
+               'username': student.username,
+               'location': user_info.location,
+               'language': user_info.language,
+               'email': student.email,
+               'format_url_params': content_parser.format_url_params,
+               'csrf': csrf(request)['csrf_token']
                }
-    context.update(grades.grade_sheet(student,coursename))
+    context.update(grades.grade_sheet(student, coursename))
 
     return render_to_response('profile.html', context)
 
-def render_accordion(request,course,chapter,section):
+
+def render_accordion(request, course, chapter, section):
     ''' Draws navigation bar. Takes current position in accordion as
         parameter. Returns (initialization_javascript, content)'''
     if not course:
         course = "6.002 Spring 2012"
 
-    toc=content_parser.toc_from_xml(content_parser.course_file(request.user,course), chapter, section)
-    active_chapter=1
+    toc = content_parser.toc_from_xml(
+        content_parser.course_file(request.user, course), chapter, section)
+
+    active_chapter = 1
     for i in range(len(toc)):
         if toc[i]['active']:
-            active_chapter=i
-    context=dict([['active_chapter',active_chapter],
-                  ['toc',toc], 
-                  ['course_name',course],
-                  ['format_url_params',content_parser.format_url_params],
-                  ['csrf',csrf(request)['csrf_token']]] + \
+            active_chapter = i
+
+    context=dict([('active_chapter', active_chapter),
+                  ('toc', toc),
+                  ('course_name', course),
+                  ('format_url_params', content_parser.format_url_params),
+                  ('csrf', csrf(request)['csrf_token'])] +
                      template_imports.items())
-    return render_to_string('accordion.html',context)
+    return render_to_string('accordion.html', context)
+
 
 @cache_control(no_cache=True, no_store=True, must_revalidate=True)
 def render_section(request, section):
@@ -122,13 +127,13 @@ def render_section(request, section):
     module_ids = dom.xpath("//@id")
 
     if user.is_authenticated():
-        module_object_preload = list(StudentModule.objects.filter(student=user, 
+        student_module_cache = list(StudentModule.objects.filter(student=user,
                                                                   module_id__in=module_ids))
     else:
-        module_object_preload = []
+        student_module_cache = []
 
     try:
-        module = render_x_module(user, request, dom, module_object_preload)
+        module = render_x_module(user, request, dom, student_module_cache)
     except:
         log.exception("Unable to load module")
         context.update({
@@ -138,18 +143,67 @@ def render_section(request, section):
         return render_to_response('courseware.html', context)
 
     context.update({
-        'init':module.get('init_js', ''),
-        'content':module['content'],
+        'init': module.get('init_js', ''),
+        'content': module['content'],
     })
 
     result = render_to_response('courseware.html', context)
     return result
 
+def get_course(request, course):
+    ''' Figure out what the correct course is.
+
+    Needed to preserve backwards compatibility with non-multi-course version.
+    TODO: Can this go away once multicourse becomes standard?
+    '''
+
+    if course==None:
+        if not settings.ENABLE_MULTICOURSE:
+            course = "6.002 Spring 2012"
+        elif 'coursename' in request.session:
+            course = request.session['coursename']
+        else:
+            course = settings.COURSE_DEFAULT
+    return course
+
+def get_module_xml(user, course, chapter, section):
+    ''' Look up the module xml for the given course/chapter/section path.
+
+    Takes the user to look up the course file.
+
+    Returns None if there was a problem, or the lxml etree for the module.
+    '''
+    try:
+        # this is the course.xml etree
+        dom = content_parser.course_file(user, course)
+    except:
+        log.exception("Unable to parse courseware xml")
+        return None
+
+    # this is the module's parent's etree
+    path = "//course[@name=$course]/chapter[@name=$chapter]//section[@name=$section]"
+    dom_module = dom.xpath(path, course=course, chapter=chapter, section=section)
+
+    module_wrapper = dom_module[0] if len(dom_module) > 0 else None
+    if module_wrapper is None:
+        module = None
+    elif module_wrapper.get("src"):
+        module = content_parser.section_file(
+            user=user, section=module_wrapper.get("src"), coursename=course)
+    else:
+        # Copy the element out of the module's etree
+        module = etree.XML(etree.tostring(module_wrapper[0]))
+    return module
+
 
 @ensure_csrf_cookie
 @cache_control(no_cache=True, no_store=True, must_revalidate=True)
-def index(request, course=None, chapter="Using the System", section="Hints",position=None): 
+def index(request, course=None, chapter=None, section=None,
+          position=None):
     ''' Displays courseware accordion, and any associated content.
+    If course, chapter, and section aren't all specified, just returns
+    the accordion.  If they are specified, returns an error if they don't
+    point to a valid module.
 
     Arguments:
 
@@ -162,110 +216,113 @@ def index(request, course=None, chapter="Using the System", section="Hints",posi
     Returns:
 
      - HTTPresponse
+    '''
+    def clean(s):
+        ''' Fixes URLs -- we convert spaces to _ in URLs to prevent
+        funny encoding characters and keep the URLs readable.  This undoes
+        that transformation.
 
+        TODO: Properly replace underscores.  (Q: what is properly?)
         '''
-    user = request.user
-    if not settings.COURSEWARE_ENABLED:
-        return redirect('/')
+        return s.replace('_', ' ')
 
-    if course==None:
-        if not settings.ENABLE_MULTICOURSE:
-            course = "6.002 Spring 2012"
-        elif 'coursename' in request.session:
-            course = request.session['coursename']
+    def get_submodule_ids(module_xml):
+        '''
+        Get a list with ids of the modules within this module.
+        '''
+        return module_xml.xpath("//@id")
+
+    def preload_student_modules(module_xml):
+        '''
+        Find any StudentModule objects for this user that match
+        one of the given module_ids.  Used as a cache to avoid having
+        each rendered module hit the db separately.
+
+        Returns the list, or None on error.
+        '''
+        if request.user.is_authenticated():
+            module_ids = get_submodule_ids(module_xml)
+            return list(StudentModule.objects.filter(student=request.user,
+                                                     module_id__in=module_ids))
         else:
-            course = settings.COURSE_DEFAULT
+            return []
 
-    # Fixes URLs -- we don't get funny encoding characters from spaces
-    # so they remain readable
-    ## TODO: Properly replace underscores
-    course=course.replace("_"," ")
-    chapter=chapter.replace("_"," ")
-    section=section.replace("_"," ")
+    def get_module_context():
+        '''
+        Look up the module object and render it.  If all goes well, returns
+        {'init': module-init-js, 'content': module-rendered-content}
 
-    # use multicourse module to determine if "course" is valid
-    #if course!=settings.COURSE_NAME.replace('_',' '):
-    if not multicourse_settings.is_valid_course(course):
-        return redirect('/')
+        If there's an error, returns
+        {'content': module-error message}
+        '''
+        # Can't modify variables of outer scope, so need new ones
+        chapter_ = clean(chapter)
+        section_ = clean(section)
 
-    request.session['coursename'] = course		# keep track of current course being viewed in django's request.session
+        user = request.user
         
-    try:
-        # this is the course.xml etree 
-        dom = content_parser.course_file(user,course)	# also pass course to it, for course-specific XML path
-    except:
-        log.exception("Unable to parse courseware xml")
-        return render_to_response('courseware-error.html', {})
+        module_xml = get_module_xml(user, course, chapter_, section_)
+        if module_xml is None:
+            log.exception("couldn't get module_xml: course/chapter/section: '%s/%s/%s'",
+                          course, chapter_, section_)
+            return {'content' : render_to_string("module-error.html", {})}
         
-    # this is the module's parent's etree
-    dom_module = dom.xpath("//course[@name=$course]/chapter[@name=$chapter]//section[@name=$section]", 
-                           course=course, chapter=chapter, section=section)
+        student_module_cache = preload_student_modules(module_xml)
 
-    #print "DM", dom_module
+        try:
+            module_context = render_x_module(user, request, module_xml,
+                                             student_module_cache, position)
+        except:
+            log.exception("Unable to load module")
+            return {'content' : render_to_string("module-error.html", {})}
 
-    if len(dom_module) == 0:
-        module_wrapper = None
-    else:
-        module_wrapper = dom_module[0]
+        return {'init': module_context.get('init_js', ''),
+                'content': module_context['content']}
     
-    if module_wrapper is None:
-        module = None
-    elif module_wrapper.get("src"):
-        module = content_parser.section_file(user=user, section=module_wrapper.get("src"), coursename=course)
-    else:
-        # this is the module's etree
-        module = etree.XML(etree.tostring(module_wrapper[0])) # Copy the element out of the tree
+    if not settings.COURSEWARE_ENABLED:
+        return redirect('/')
 
-    module_ids = []
-    if module is not None:
-        module_ids = module.xpath("//@id", 
-                                  course=course, chapter=chapter, section=section)
+    course = clean(get_course(request, course))
+    if not multicourse_settings.is_valid_course(course):
+        return redirect('/')
 
-    if user.is_authenticated():
-        module_object_preload = list(StudentModule.objects.filter(student=user, 
-                                                                  module_id__in=module_ids))
-    else:
-        module_object_preload = []
+    # keep track of current course being viewed in django's request.session
+    request.session['coursename'] = course
 
     context = {
         'csrf': csrf(request)['csrf_token'],
         'accordion': render_accordion(request, course, chapter, section),
-        'COURSE_TITLE':multicourse_settings.get_course_title(course),
-    }
-
-    try:
-        module_context = render_x_module(user, request, module, module_object_preload, position)
-    except:
-        log.exception("Unable to load module")
-        context.update({
+        'COURSE_TITLE': multicourse_settings.get_course_title(course),
         'init': '',
-            'content': render_to_string("module-error.html", {}),
-        })
-        return render_to_response('courseware.html', context)
+        'content': ''
+    }
 
-    context.update({
-        'init': module_context.get('init_js', ''),
-        'content': module_context['content'],
-    })
+    look_for_module = chapter is not None and section is not None
+    if look_for_module:
+        context.update(get_module_context())
 
     result = render_to_response('courseware.html', context)
     return result
 
 def jump_to(request, probname=None):
     '''
-    Jump to viewing a specific problem.  The problem is specified by a problem name - currently the filename (minus .xml)
-    of the problem.  Maybe this should change to a more generic tag, eg "name" given as an attribute in <problem>.
+    Jump to viewing a specific problem.  The problem is specified by a
+    problem name - currently the filename (minus .xml) of the problem.
+    Maybe this should change to a more generic tag, eg "name" given as
+    an attribute in <problem>.
 
-    We do the jump by (1) reading course.xml to find the first instance of <problem> with the given filename, then
-    (2) finding the parent element of the problem, then (3) rendering that parent element with a specific computed position
-    value (if it is <sequential>).
+    We do the jump by (1) reading course.xml to find the first
+    instance of <problem> with the given filename, then (2) finding
+    the parent element of the problem, then (3) rendering that parent
+    element with a specific computed position value (if it is
+    <sequential>).
 
     '''
     # get coursename if stored
     coursename = multicourse_settings.get_coursename_from_request(request)
 
     # begin by getting course.xml tree
-    xml = content_parser.course_file(request.user,coursename)
+    xml = content_parser.course_file(request.user, coursename)
 
     # look for problem of given name
     pxml = xml.xpath('//problem[@filename="%s"]' % probname)
@@ -279,12 +336,16 @@ def jump_to(request, probname=None):
     section = None
     branch = parent
     for k in range(4):	# max depth of recursion
-        if branch.tag=='section': section = branch.get('name')
-        if branch.tag=='chapter': chapter = branch.get('name')
+        if branch.tag == 'section':
+            section = branch.get('name')
+        if branch.tag == 'chapter':
+            chapter = branch.get('name')
         branch = branch.getparent()
 
     position = None
-    if parent.tag=='sequential':
-        position = parent.index(pxml)+1	# position in sequence
+    if parent.tag == 'sequential':
+        position = parent.index(pxml) + 1	# position in sequence
 
-    return index(request,course=coursename,chapter=chapter,section=section,position=position)
+    return index(request,
+                 course=coursename, chapter=chapter,
+                 section=section, position=position)