From e1ac5e9f30080b6e9a83ac10120ee9bc524b7fcb Mon Sep 17 00:00:00 2001 From: Piotr Mitros Date: Mon, 7 May 2012 20:13:30 -0400 Subject: [PATCH 1/4] Moved extra settings into state. Added fs module for aux content --- djangoapps/courseware/module_render.py | 37 ++++++++++++++----- djangoapps/courseware/modules/capa_module.py | 4 +- djangoapps/courseware/modules/html_module.py | 8 ++-- .../courseware/modules/schematic_module.py | 4 +- djangoapps/courseware/modules/seq_module.py | 5 +-- .../courseware/modules/template_module.py | 5 +-- .../courseware/modules/vertical_module.py | 5 ++- djangoapps/courseware/modules/video_module.py | 4 +- djangoapps/courseware/modules/x_module.py | 10 ++--- 9 files changed, 49 insertions(+), 33 deletions(-) diff --git a/djangoapps/courseware/module_render.py b/djangoapps/courseware/module_render.py index 2924aec9df..134bff8501 100644 --- a/djangoapps/courseware/module_render.py +++ b/djangoapps/courseware/module_render.py @@ -18,8 +18,12 @@ from django.http import HttpResponse from django.shortcuts import redirect from django.template import Context from django.template import Context, loader + +from fs.osfs import OSFS + from mitxmako.shortcuts import render_to_response, render_to_string + from models import StudentModule from student.models import UserProfile import track.views @@ -30,6 +34,13 @@ import courseware.modules log = logging.getLogger("mitx.courseware") +class I4xSystem(object): + def __init__(self, **args): + self.ajax_url = args['ajax_url'] + self.track_function = args['track_function'] + self.filestore = OSFS(settings.DATA_DIR) + self.render_function = args['render_function'] + 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 @@ -76,12 +87,15 @@ def modx_dispatch(request, module=None, dispatch=None, id=None): xml = content_parser.module_xml(request.user, module, 'id', id) # Create the module - instance=courseware.modules.get_module_class(module)(xml, + system = I4xSystem(track_function = make_track_function(request), + render_function = None, + ajax_url = ajax_url, + filestore = None + ) + instance=courseware.modules.get_module_class(module)(system, + xml, id, - ajax_url=ajax_url, - state=oldstate, - track_function = make_track_function(request), - render_function = None) + state=oldstate) # Let the module handle the AJAX ajax_return=instance.handle_ajax(dispatch, request.POST) # Save the state back to the database @@ -128,12 +142,15 @@ def render_x_module(user, request, xml_module, module_object_preload): # Create a new instance ajax_url = '/modx/'+module_type+'/'+module_id+'/' - instance=module_class(etree.tostring(xml_module), + system = I4xSystem(track_function = make_track_function(request), + render_function = lambda x: render_module(user, request, x, module_object_preload), + ajax_url = ajax_url, + filestore = None + ) + instance=module_class(system, + etree.tostring(xml_module), module_id, - ajax_url=ajax_url, - state=state, - track_function = make_track_function(request), - render_function = lambda x: render_module(user, request, x, module_object_preload)) + state=state) # If instance wasn't already in the database, create it if not smod: diff --git a/djangoapps/courseware/modules/capa_module.py b/djangoapps/courseware/modules/capa_module.py index 86958dcf1c..c6deca64f6 100644 --- a/djangoapps/courseware/modules/capa_module.py +++ b/djangoapps/courseware/modules/capa_module.py @@ -131,8 +131,8 @@ class Module(XModule): return html - def __init__(self, xml, item_id, ajax_url=None, track_url=None, state=None, track_function=None, render_function = None, meta = None): - XModule.__init__(self, xml, item_id, ajax_url, track_url, state, track_function, render_function) + def __init__(self, system, xml, item_id, state=None): + XModule.__init__(self, system, xml, item_id, state) self.attempts = 0 self.max_attempts = None diff --git a/djangoapps/courseware/modules/html_module.py b/djangoapps/courseware/modules/html_module.py index 4194f73e74..08a7b268a9 100644 --- a/djangoapps/courseware/modules/html_module.py +++ b/djangoapps/courseware/modules/html_module.py @@ -24,13 +24,13 @@ class Module(XModule): textlist=[i for i in textlist if type(i)==str] return "".join(textlist) try: - filename=settings.DATA_DIR+"html/"+self.filename - return open(filename).read() + filename="html/"+self.filename + return self.filestore.open(filename).read() except: # For backwards compatibility. TODO: Remove return render_to_string(self.filename, {'id': self.item_id}) - def __init__(self, xml, item_id, ajax_url=None, track_url=None, state=None, track_function=None, render_function = None): - XModule.__init__(self, xml, item_id, ajax_url, track_url, state, track_function, render_function) + def __init__(self, system, xml, item_id, state=None): + XModule.__init__(self, system, xml, item_id, state) xmltree=etree.fromstring(xml) self.filename = None filename_l=xmltree.xpath("/html/@filename") diff --git a/djangoapps/courseware/modules/schematic_module.py b/djangoapps/courseware/modules/schematic_module.py index e253f1acc6..5fef265e01 100644 --- a/djangoapps/courseware/modules/schematic_module.py +++ b/djangoapps/courseware/modules/schematic_module.py @@ -19,6 +19,6 @@ class Module(XModule): def get_html(self): return ''.format(item_id=self.item_id) - def __init__(self, xml, item_id, ajax_url=None, track_url=None, state=None, render_function = None): - XModule.__init__(self, xml, item_id, ajax_url, track_url, state, render_function) + def __init__(self, system, xml, item_id, state=None): + XModule.__init__(self, system, xml, item_id, state) diff --git a/djangoapps/courseware/modules/seq_module.py b/djangoapps/courseware/modules/seq_module.py index 02796a9768..276931d9f3 100644 --- a/djangoapps/courseware/modules/seq_module.py +++ b/djangoapps/courseware/modules/seq_module.py @@ -107,9 +107,8 @@ class Module(XModule): self.rendered = True - - def __init__(self, xml, item_id, ajax_url=None, track_url=None, state=None, track_function=None, render_function = None): - XModule.__init__(self, xml, item_id, ajax_url, track_url, state, track_function, render_function) + def __init__(self, system, xml, item_id, state=None): + XModule.__init__(self, system, xml, item_id, state) self.xmltree=etree.fromstring(xml) self.position = 1 diff --git a/djangoapps/courseware/modules/template_module.py b/djangoapps/courseware/modules/template_module.py index d9dbb613f0..07e920b5a3 100644 --- a/djangoapps/courseware/modules/template_module.py +++ b/djangoapps/courseware/modules/template_module.py @@ -20,10 +20,9 @@ class Module(XModule): def get_html(self): return self.html - def __init__(self, xml, item_id, ajax_url=None, track_url=None, state=None, track_function=None, render_function = None): - XModule.__init__(self, xml, item_id, ajax_url, track_url, state, track_function, render_function) + def __init__(self, system, xml, item_id, state=None): + XModule.__init__(self, system, xml, item_id, state) xmltree = etree.fromstring(xml) filename = xmltree.tag params = dict(xmltree.items()) -# print params self.html = render_to_string(filename, params, namespace = 'custom_tags') diff --git a/djangoapps/courseware/modules/vertical_module.py b/djangoapps/courseware/modules/vertical_module.py index c068cb9a76..0867460e95 100644 --- a/djangoapps/courseware/modules/vertical_module.py +++ b/djangoapps/courseware/modules/vertical_module.py @@ -26,8 +26,9 @@ class Module(XModule): def get_destroy_js(self): return self.destroy_js_text - def __init__(self, xml, item_id, ajax_url=None, track_url=None, state=None, track_function=None, render_function = None): - XModule.__init__(self, xml, item_id, ajax_url, track_url, state, track_function, render_function) + + def __init__(self, system, xml, item_id, state=None): + XModule.__init__(self, system, xml, item_id, state) xmltree=etree.fromstring(xml) self.contents=[(e.get("name"),self.render_function(e)) \ for e in xmltree] diff --git a/djangoapps/courseware/modules/video_module.py b/djangoapps/courseware/modules/video_module.py index a893d1f3bb..aa44bd0ab2 100644 --- a/djangoapps/courseware/modules/video_module.py +++ b/djangoapps/courseware/modules/video_module.py @@ -58,8 +58,8 @@ class Module(XModule): def get_destroy_js(self): return "videoDestroy(\"{0}\");".format(self.item_id)+self.annotations_destroy - def __init__(self, xml, item_id, ajax_url=None, track_url=None, state=None, track_function=None, render_function = None): - XModule.__init__(self, xml, item_id, ajax_url, track_url, state, track_function, render_function) + def __init__(self, system, xml, item_id, state=None): + XModule.__init__(self, system, xml, item_id, state) xmltree=etree.fromstring(xml) self.youtube = xmltree.get('youtube') self.name = xmltree.get('name') diff --git a/djangoapps/courseware/modules/x_module.py b/djangoapps/courseware/modules/x_module.py index 4a379253c4..df96763d26 100644 --- a/djangoapps/courseware/modules/x_module.py +++ b/djangoapps/courseware/modules/x_module.py @@ -45,13 +45,13 @@ class XModule(object): get is a dictionary-like object ''' return "" - def __init__(self, xml, item_id, ajax_url=None, track_url=None, state=None, track_function=None, render_function = None): + def __init__(self, system, xml, item_id, track_url=None, state=None): ''' In most cases, you must pass state or xml''' self.xml = xml self.item_id = item_id - self.ajax_url = ajax_url - self.track_url = track_url self.state = state - self.tracker = track_function - self.render_function = render_function + self.ajax_url = system.ajax_url + self.tracker = system.track_function + self.filestore = system.filestore + self.render_function = system.render_function From aab9b15cd471e5726e360add50759f8c59c98714 Mon Sep 17 00:00:00 2001 From: Piotr Mitros Date: Mon, 7 May 2012 20:59:08 -0400 Subject: [PATCH 2/4] Refactor for reusability: x_modules take common parameters as an object. capa_problem to take a file object and not filename. --- djangoapps/courseware/capa/capa_problem.py | 11 ++--------- djangoapps/courseware/module_render.py | 1 + djangoapps/courseware/modules/capa_module.py | 19 +++++-------------- djangoapps/courseware/modules/html_module.py | 2 -- djangoapps/courseware/modules/seq_module.py | 7 +------ .../courseware/modules/template_module.py | 1 + .../courseware/modules/vertical_module.py | 2 -- djangoapps/courseware/modules/video_module.py | 2 -- djangoapps/courseware/modules/x_module.py | 1 + 9 files changed, 11 insertions(+), 35 deletions(-) diff --git a/djangoapps/courseware/capa/capa_problem.py b/djangoapps/courseware/capa/capa_problem.py index c5a81e8100..9835b598b2 100644 --- a/djangoapps/courseware/capa/capa_problem.py +++ b/djangoapps/courseware/capa/capa_problem.py @@ -53,13 +53,12 @@ html_special_response = {"textline":textline.render, "schematic":schematic.render} class LoncapaProblem(object): - def __init__(self, filename, id=None, state=None, seed=None): + def __init__(self, fileobject, id=None, state=None, seed=None): ## Initialize class variables from state self.seed = None self.student_answers = dict() self.correct_map = dict() self.done = False - self.filename = filename if seed != None: self.seed = seed @@ -69,7 +68,6 @@ class LoncapaProblem(object): else: print "NO ID" raise Exception("This should never happen (183)") - #self.problem_id = filename if state: if 'seed' in state: @@ -81,17 +79,12 @@ class LoncapaProblem(object): if 'done' in state: self.done = state['done'] -# print self.seed - # TODO: Does this deplete the Linux entropy pool? Is this fast enough? if not self.seed: self.seed=struct.unpack('i', os.urandom(4))[0] -# print filename, self.seed, seed - ## Parse XML file - #log.debug(u"LoncapaProblem() opening file {0}".format(filename)) - file_text = open(filename).read() + file_text = fileobject.read() # Convert startouttext and endouttext to proper # TODO: Do with XML operations file_text = re.sub("startouttext\s*/","text",file_text) diff --git a/djangoapps/courseware/module_render.py b/djangoapps/courseware/module_render.py index 134bff8501..8ce4294f8e 100644 --- a/djangoapps/courseware/module_render.py +++ b/djangoapps/courseware/module_render.py @@ -40,6 +40,7 @@ class I4xSystem(object): self.track_function = args['track_function'] self.filestore = OSFS(settings.DATA_DIR) self.render_function = args['render_function'] + self.exception404 = Http404 def object_cache(cache, user, module_type, module_id): # We don't look up on user -- all queries include user diff --git a/djangoapps/courseware/modules/capa_module.py b/djangoapps/courseware/modules/capa_module.py index c6deca64f6..8865038021 100644 --- a/djangoapps/courseware/modules/capa_module.py +++ b/djangoapps/courseware/modules/capa_module.py @@ -92,7 +92,6 @@ class Module(XModule): # User submitted a problem, and hasn't reset. We don't want # more submissions. if self.lcp.done and self.rerandomize == "always": - #print "!" check_button = False save_button = False @@ -193,7 +192,7 @@ class Module(XModule): seed = 1 else: seed = None - self.lcp=LoncapaProblem(filename, self.item_id, state, seed = seed) + self.lcp=LoncapaProblem(open(filename), self.item_id, state, seed = seed) def handle_ajax(self, dispatch, get): if dispatch=='problem_get': @@ -242,7 +241,7 @@ class Module(XModule): if self.show_answer == 'closed' and not self.closed(): return False print "aa", self.show_answer - raise Http404 + raise Http404 #TODO: Not 404 def get_answer(self, get): if not self.answer_available(): @@ -270,15 +269,12 @@ class Module(XModule): for key in get: answers['_'.join(key.split('_')[1:])]=get[key] -# print "XXX", answers, get - event_info['answers']=answers # Too late. Cannot submit if self.closed(): event_info['failure']='closed' self.tracker('save_problem_check_fail', event_info) - print "cp" raise Http404 # Problem submitted. Student should reset before checking @@ -286,7 +282,6 @@ class Module(XModule): if self.lcp.done and self.rerandomize == "always": event_info['failure']='unreset' self.tracker('save_problem_check_fail', event_info) - print "cpdr" raise Http404 try: @@ -295,15 +290,11 @@ class Module(XModule): filename = self.lcp.filename correct_map = self.lcp.grade_answers(answers) except StudentInputError as inst: - self.lcp = LoncapaProblem(filename, id=lcp_id, state=old_state) + self.lcp = LoncapaProblem(open(filename), id=lcp_id, state=old_state) traceback.print_exc() -# print {'error':sys.exc_info(), -# 'answers':answers, -# 'seed':self.lcp.seed, -# 'filename':self.lcp.filename} return json.dumps({'success':inst.message}) except: - self.lcp = LoncapaProblem(filename, id=lcp_id, state=old_state) + self.lcp = LoncapaProblem(open(filename), id=lcp_id, state=old_state) traceback.print_exc() return json.dumps({'success':'Unknown Error'}) @@ -383,7 +374,7 @@ class Module(XModule): self.lcp.seed=None filename=settings.DATA_DIR+"problems/"+self.filename+".xml" - self.lcp=LoncapaProblem(filename, self.item_id, self.lcp.get_state()) + self.lcp=LoncapaProblem(open(filename), self.item_id, self.lcp.get_state()) event_info['new_state']=self.lcp.get_state() self.tracker('reset_problem', event_info) diff --git a/djangoapps/courseware/modules/html_module.py b/djangoapps/courseware/modules/html_module.py index 08a7b268a9..77bcbb4bbc 100644 --- a/djangoapps/courseware/modules/html_module.py +++ b/djangoapps/courseware/modules/html_module.py @@ -1,7 +1,5 @@ import json -## TODO: Abstract out from Django -from django.conf import settings from mitxmako.shortcuts import render_to_response, render_to_string from x_module import XModule diff --git a/djangoapps/courseware/modules/seq_module.py b/djangoapps/courseware/modules/seq_module.py index 276931d9f3..161c95b604 100644 --- a/djangoapps/courseware/modules/seq_module.py +++ b/djangoapps/courseware/modules/seq_module.py @@ -2,9 +2,6 @@ import json from lxml import etree -## TODO: Abstract out from Django -from django.http import Http404 -from django.conf import settings from mitxmako.shortcuts import render_to_response, render_to_string from x_module import XModule @@ -38,12 +35,10 @@ class Module(XModule): return self.destroy_js def handle_ajax(self, dispatch, get): - print "GET", get - print "DISPATCH", dispatch if dispatch=='goto_position': self.position = int(get['position']) return json.dumps({'success':True}) - raise Http404() + raise self.system.exception404 def render(self): if self.rendered: diff --git a/djangoapps/courseware/modules/template_module.py b/djangoapps/courseware/modules/template_module.py index 07e920b5a3..41556eb9d4 100644 --- a/djangoapps/courseware/modules/template_module.py +++ b/djangoapps/courseware/modules/template_module.py @@ -14,6 +14,7 @@ class Module(XModule): @classmethod def get_xml_tags(c): + ## TODO: Abstract out from filesystem tags = os.listdir(settings.DATA_DIR+'/custom_tags') return tags diff --git a/djangoapps/courseware/modules/vertical_module.py b/djangoapps/courseware/modules/vertical_module.py index 0867460e95..f64e45fe7f 100644 --- a/djangoapps/courseware/modules/vertical_module.py +++ b/djangoapps/courseware/modules/vertical_module.py @@ -1,7 +1,5 @@ import json -## TODO: Abstract out from Django -from django.conf import settings from mitxmako.shortcuts import render_to_response, render_to_string from x_module import XModule diff --git a/djangoapps/courseware/modules/video_module.py b/djangoapps/courseware/modules/video_module.py index aa44bd0ab2..c678838f2b 100644 --- a/djangoapps/courseware/modules/video_module.py +++ b/djangoapps/courseware/modules/video_module.py @@ -3,8 +3,6 @@ import logging from lxml import etree -## TODO: Abstract out from Django -from django.conf import settings from mitxmako.shortcuts import render_to_response, render_to_string from x_module import XModule diff --git a/djangoapps/courseware/modules/x_module.py b/djangoapps/courseware/modules/x_module.py index df96763d26..b21894b36b 100644 --- a/djangoapps/courseware/modules/x_module.py +++ b/djangoapps/courseware/modules/x_module.py @@ -55,3 +55,4 @@ class XModule(object): self.tracker = system.track_function self.filestore = system.filestore self.render_function = system.render_function + self.system = system From f187a9e312e78a226c7f27396a232c5214326ad1 Mon Sep 17 00:00:00 2001 From: Piotr Mitros Date: Mon, 7 May 2012 21:24:40 -0400 Subject: [PATCH 3/4] Capa no longer requires local filestore --- djangoapps/courseware/modules/capa_module.py | 28 +++++++++----------- 1 file changed, 12 insertions(+), 16 deletions(-) diff --git a/djangoapps/courseware/modules/capa_module.py b/djangoapps/courseware/modules/capa_module.py index 8865038021..5c2f6be5c3 100644 --- a/djangoapps/courseware/modules/capa_module.py +++ b/djangoapps/courseware/modules/capa_module.py @@ -16,9 +16,7 @@ import traceback from lxml import etree ## TODO: Abstract out from Django -from django.conf import settings -from mitxmako.shortcuts import render_to_response, render_to_string -from django.http import Http404 +from mitxmako.shortcuts import render_to_string from x_module import XModule from courseware.capa.capa_problem import LoncapaProblem, StudentInputError @@ -184,15 +182,14 @@ class Module(XModule): if state!=None and 'attempts' in state: self.attempts=state['attempts'] - self.filename=content_parser.item(dom2.xpath('/problem/@filename')) - filename=settings.DATA_DIR+"/problems/"+self.filename+".xml" + self.filename="problems/"+content_parser.item(dom2.xpath('/problem/@filename'))+".xml" self.name=content_parser.item(dom2.xpath('/problem/@name')) self.weight=content_parser.item(dom2.xpath('/problem/@weight')) if self.rerandomize == 'never': seed = 1 else: seed = None - self.lcp=LoncapaProblem(open(filename), self.item_id, state, seed = seed) + self.lcp=LoncapaProblem(self.filestore.open(self.filename), self.item_id, state, seed = seed) def handle_ajax(self, dispatch, get): if dispatch=='problem_get': @@ -241,16 +238,15 @@ class Module(XModule): if self.show_answer == 'closed' and not self.closed(): return False print "aa", self.show_answer - raise Http404 #TODO: Not 404 + raise self.system.exception404 #TODO: Not 404 def get_answer(self, get): if not self.answer_available(): - raise Http404 + raise self.system.exception404 else: return json.dumps(self.lcp.get_question_answers(), cls=ComplexEncoder) - # Figure out if we should move these to capa_problem? def get_problem(self, get): ''' Same as get_problem_html -- if we want to reconfirm we @@ -275,27 +271,27 @@ class Module(XModule): if self.closed(): event_info['failure']='closed' self.tracker('save_problem_check_fail', event_info) - raise Http404 + raise self.system.exception404 # Problem submitted. Student should reset before checking # again. if self.lcp.done and self.rerandomize == "always": event_info['failure']='unreset' self.tracker('save_problem_check_fail', event_info) - raise Http404 + raise self.system.exception404 try: old_state = self.lcp.get_state() lcp_id = self.lcp.problem_id - filename = self.lcp.filename correct_map = self.lcp.grade_answers(answers) except StudentInputError as inst: - self.lcp = LoncapaProblem(open(filename), id=lcp_id, state=old_state) + self.lcp = LoncapaProblem(self.filestore.open(self.filename), id=lcp_id, state=old_state) traceback.print_exc() return json.dumps({'success':inst.message}) except: - self.lcp = LoncapaProblem(open(filename), id=lcp_id, state=old_state) + self.lcp = LoncapaProblem(self.filestore.open(self.filename), id=lcp_id, state=old_state) traceback.print_exc() + raise return json.dumps({'success':'Unknown Error'}) @@ -373,8 +369,8 @@ class Module(XModule): self.lcp.questions=dict() # Detailed info about questions in problem instance. TODO: Should be by id and not lid. self.lcp.seed=None - filename=settings.DATA_DIR+"problems/"+self.filename+".xml" - self.lcp=LoncapaProblem(open(filename), self.item_id, self.lcp.get_state()) + filename="problems/"+self.filename+".xml" + self.lcp=LoncapaProblem(self.filestore.open(filename), self.item_id, self.lcp.get_state()) event_info['new_state']=self.lcp.get_state() self.tracker('reset_problem', event_info) From f511a5e0dd4756b11aa896b62cba2864816cd759 Mon Sep 17 00:00:00 2001 From: Piotr Mitros Date: Wed, 9 May 2012 17:36:56 -0400 Subject: [PATCH 4/4] Changed parameters from **args based on code review --- djangoapps/courseware/module_render.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/djangoapps/courseware/module_render.py b/djangoapps/courseware/module_render.py index 8ce4294f8e..ac76eb08ae 100644 --- a/djangoapps/courseware/module_render.py +++ b/djangoapps/courseware/module_render.py @@ -35,11 +35,11 @@ import courseware.modules log = logging.getLogger("mitx.courseware") class I4xSystem(object): - def __init__(self, **args): - self.ajax_url = args['ajax_url'] - self.track_function = args['track_function'] + def __init__(self, ajax_url, track_function, render_function, filestore=None): + self.ajax_url = ajax_url + self.track_function = track_function self.filestore = OSFS(settings.DATA_DIR) - self.render_function = args['render_function'] + self.render_function = render_function self.exception404 = Http404 def object_cache(cache, user, module_type, module_id):