Merge pull request #126 from MITx/victor/progress-tracking

Victor/progress tracking

common/lib/capa/capa_problem.py

@@ -168,7 +168,8 @@ class LoncapaProblem(object):
     def get_score(self):
         '''
         Compute score for this problem.  The score is the number of points awarded.
-        Returns an integer, from 0 to get_max_score().
+        Returns a dictionary {'score': integer, from 0 to get_max_score(),
+                              'total': get_max_score()}.
         '''
         correct = 0
         for key in self.correct_map:

common/lib/xmodule/progress.py

+'''
+Progress class for modules.  Represents where a student is in a module.
+'''
+
+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)

common/lib/xmodule/seq_module.py

 import json
+import logging
 
 from lxml import etree
 
 from x_module import XModule, XModuleDescriptor
+from xmodule.progress import Progress
+
+log = logging.getLogger("mitx.common.lib.seq_module")
 
 # HACK: This shouldn't be hard-coded to two types
 # OBSOLETE: This obsoletes 'type'
@@ -37,6 +41,16 @@ class Module(XModule):
         self.render()
         return self.destroy_js
 
+    def get_progress(self):
+        ''' Return the total progress, adding total done and total available.
+        (assumes that each submodule uses the same "units" for progress.)
+        '''
+        # TODO: Cache progress or children array?
+        children = self.get_children()
+        progresses = [child.get_progress() for child in children]
+        progress = reduce(Progress.add_counts, progresses)
+        return progress
+
     def handle_ajax(self, dispatch, get):		# TODO: bounds checking
         ''' get = request.POST instance '''
         if dispatch=='goto_position':
@@ -53,10 +67,15 @@ class Module(XModule):
         titles = ["\n".join([i.get("name").strip() for i in e.iter() if i.get("name") is not None]) \
                        for e in self.xmltree]
 
+        children = self.get_children()
+        progresses = [child.get_progress() for child in children]
+        
         self.contents = self.rendered_children()
 
-        for contents, title in zip(self.contents, titles):
+        for contents, title, progress in zip(self.contents, titles, progresses):
             contents['title'] = title
+            contents['progress_str'] = str(progress) if progress is not None else ""
+            contents['progress_stat'] = progress.ternary_str() if progress is not None else ""
 
         for (content, element_class) in zip(self.contents, child_classes):
             new_class = 'other'
@@ -68,16 +87,17 @@ class Module(XModule):
         # Split </script> tags -- browsers handle this as end
         # of script, even if it occurs mid-string. Do this after json.dumps()ing
         # so that we can be sure of the quotations being used
-        params={'items':json.dumps(self.contents).replace('</script>', '<"+"/script>'),
-                'id':self.item_id,
+        params={'items': json.dumps(self.contents).replace('</script>', '<"+"/script>'),
+                'id': self.item_id,
                 'position': self.position,
-                'titles':titles,
-                'tag':self.xmltree.tag}
+                'titles': titles,
+                'tag': self.xmltree.tag}
 
         if self.xmltree.tag in ['sequential', 'videosequence']:
             self.content = self.system.render_template('seq_module.html', params)
         if self.xmltree.tag == 'tab':
             self.content = self.system.render_template('tab_module.html', params)
+        log.debug("rendered content: %s", content)
         self.rendered = True
 
     def __init__(self, system, xml, item_id, state=None):

common/lib/xmodule/tests.py

@@ -13,8 +13,9 @@ import numpy
 import xmodule
 import capa.calc as calc
 import capa.capa_problem as lcp
-from xmodule import graders
+from xmodule import graders, x_module
 from xmodule.graders import Score, aggregate_scores
+from xmodule.progress import Progress
 from nose.plugins.skip import SkipTest
 
 class I4xSystem(object):
@@ -26,7 +27,9 @@ class I4xSystem(object):
     def __init__(self):
         self.ajax_url = '/'
         self.track_function = lambda x: None
+        self.filestore = None
         self.render_function = lambda x: {} # Probably incorrect
+        self.module_from_xml = lambda x: None  # May need a real impl...
         self.exception404 = Exception
         self.DEBUG = True
     def __repr__(self):
@@ -488,3 +491,118 @@ class GraderTest(unittest.TestCase):
         
         #TODO: How do we test failure cases? The parser only logs an error when it can't parse something. Maybe it should throw exceptions?
 
+# --------------------------------------------------------------------------
+# Module progress tests
+        
+class ProgressTest(unittest.TestCase):
+    ''' Test that basic Progress objects work.  A Progress represents a
+    fraction between 0 and 1.
+    '''
+    not_started = Progress(0, 17)
+    part_done = Progress(2, 6)
+    half_done = Progress(3, 6)
+    also_half_done = Progress(1, 2)
+    done = Progress(7, 7)
+    
+    def test_create_object(self):
+        # These should work:
+        p = Progress(0, 2)
+        p = Progress(1, 2)
+        p = Progress(2, 2)
+
+        p = Progress(2.5, 5.0)
+        p = Progress(3.7, 12.3333)
+        
+        # These shouldn't
+        self.assertRaises(ValueError, Progress, 0, 0)
+        self.assertRaises(ValueError, Progress, 2, 0)
+        self.assertRaises(ValueError, Progress, 1, -2)
+        self.assertRaises(ValueError, Progress, 3, 2)
+        self.assertRaises(ValueError, Progress, -2, 5)
+
+        self.assertRaises(TypeError, Progress, 0, "all")
+        # check complex numbers just for the heck of it :)
+        self.assertRaises(TypeError, Progress, 2j, 3)
+
+    def test_frac(self):
+        p = Progress(1, 2)
+        (a, b) = p.frac()
+        self.assertEqual(a, 1)
+        self.assertEqual(b, 2)
+
+    def test_percent(self):
+        self.assertEqual(self.not_started.percent(), 0)
+        self.assertAlmostEqual(self.part_done.percent(), 33.33333333333333)
+        self.assertEqual(self.half_done.percent(), 50)
+        self.assertEqual(self.done.percent(), 100)
+
+        self.assertEqual(self.half_done.percent(), self.also_half_done.percent())
+
+    def test_started(self):
+        self.assertFalse(self.not_started.started())
+
+        self.assertTrue(self.part_done.started())
+        self.assertTrue(self.half_done.started())
+        self.assertTrue(self.done.started())
+
+    def test_inprogress(self):
+        # only true if working on it
+        self.assertFalse(self.done.inprogress())
+        self.assertFalse(self.not_started.inprogress())
+
+        self.assertTrue(self.part_done.inprogress())
+        self.assertTrue(self.half_done.inprogress())
+
+    def test_done(self):
+        self.assertTrue(self.done.done())
+        self.assertFalse(self.half_done.done())
+        self.assertFalse(self.not_started.done())
+        
+    def test_str(self):
+        self.assertEqual(str(self.not_started), "0/17")
+        self.assertEqual(str(self.part_done), "2/6")
+        self.assertEqual(str(self.done), "7/7")
+
+    def test_ternary_str(self):
+        self.assertEqual(self.not_started.ternary_str(), "none")
+        self.assertEqual(self.half_done.ternary_str(), "in_progress")
+        self.assertEqual(self.done.ternary_str(), "done")
+
+    def test_add(self):
+        '''Test the Progress.add_counts() method'''
+        p = Progress(0, 2)
+        p2 = Progress(1, 3)
+        p3 = Progress(2, 5)
+        pNone = None
+        add = lambda a, b: Progress.add_counts(a, b).frac()
+
+        self.assertEqual(add(p, p), (0, 4))
+        self.assertEqual(add(p, p2), (1, 5))
+        self.assertEqual(add(p2, p3), (3, 8))
+        
+        self.assertEqual(add(p2, pNone), p2.frac())
+        self.assertEqual(add(pNone, p2), p2.frac())
+
+    def test_equality(self):
+        '''Test that comparing Progress objects for equality
+        works correctly.'''
+        p = Progress(1, 2)
+        p2 = Progress(2, 4)
+        p3 = Progress(1, 2)
+        self.assertTrue(p == p3)
+        self.assertFalse(p == p2)
+
+        # Check != while we're at it
+        self.assertTrue(p != p2)
+        self.assertFalse(p != p3)
+
+
+class ModuleProgressTest(unittest.TestCase):
+    ''' Test that get_progress() does the right thing for the different modules
+    '''
+    def test_xmodule_default(self):
+        '''Make sure default get_progress exists, returns None'''
+        xm = x_module.XModule(i4xs, "<dummy />", "dummy")
+        p = xm.get_progress()
+        self.assertEqual(p, None)
+        

common/lib/xmodule/vertical_module.py

 import json
 
 from x_module import XModule, XModuleDescriptor
+from xmodule.progress import Progress
 from lxml import etree
 
 class ModuleDescriptor(XModuleDescriptor):
     pass
 
 class Module(XModule):
+    ''' Layout module for laying out submodules vertically.'''
     id_attribute = 'id'
 
     def get_state(self):
@@ -21,6 +23,13 @@ class Module(XModule):
             'items': self.contents
         })
 
+    def get_progress(self):
+        # TODO: Cache progress or children array?
+        children = self.get_children()
+        progresses = [child.get_progress() for child in children]
+        progress = reduce(Progress.add_counts, progresses)
+        return progress
+
     def __init__(self, system, xml, item_id, state=None):
         XModule.__init__(self, system, xml, item_id, state)
         xmltree=etree.fromstring(xml)

common/lib/xmodule/x_module.py

 from lxml import etree
 import pkg_resources
 import logging
+
 from keystore import Location
+from progress import Progress
 
 log = logging.getLogger('mitx.' + __name__)
 
@@ -57,6 +59,13 @@ class XModule(object):
         else: 
             raise "We should iterate through children and find a default name"
 
+    def get_children(self):
+        '''
+        Return module instances for all the children of this module.
+        '''
+        children = [self.module_from_xml(e) for e in self.__xmltree]
+        return children            
+
     def rendered_children(self):
         '''
         Render all children. 
@@ -90,6 +99,7 @@ class XModule(object):
             self.tracker = system.track_function
             self.filestore = system.filestore
             self.render_function = system.render_function
+            self.module_from_xml = system.module_from_xml
             self.DEBUG = system.DEBUG
         self.system = system
 
@@ -118,6 +128,15 @@ class XModule(object):
         '''
         return "Unimplemented"
 
+    def get_progress(self):
+        ''' Return a progress.Progress object that represents how far the student has gone
+        in this module.  Must be implemented to get correct progress tracking behavior in
+        nesting modules like sequence and vertical.
+
+        If this module has no notion of progress, return None.
+        '''
+        return None
+
     def handle_ajax(self, dispatch, get):
         ''' dispatch is last part of the URL. 
             get is a dictionary-like object ''' 

lms/djangoapps/courseware/grades.py

@@ -174,6 +174,8 @@ def get_score(user, problem, cache, coursename=None):
     else:
         ## HACK 1: We shouldn't specifically reference capa_module
         ## HACK 2: Backwards-compatibility: This should be written when a grade is saved, and removed from the system
+        # TODO: These are no longer correct params for I4xSystem -- figure out what this code
+        # does, clean it up.
         from module_render import I4xSystem
         system = I4xSystem(None, None, None, coursename=coursename)
         total=float(xmodule.capa_module.Module(system, etree.tostring(problem), "id").max_score())

lms/djangoapps/courseware/module_render.py

@@ -34,7 +34,8 @@ class I4xSystem(object):
     and user, or other environment-specific info.
     '''
     def __init__(self, ajax_url, track_function, render_function,
-                 render_template, request=None, filestore=None):
+                 module_from_xml, render_template, request=None,
+                 filestore=None):
         '''
         Create a closure around the system environment.
 
@@ -43,6 +44,8 @@ class I4xSystem(object):
                          or otherwise tracking the event.
                          TODO: Not used, and has inconsistent args in different
                          files.  Update or remove.
+        module_from_xml - function that takes (module_xml) and returns a corresponding
+                          module instance object.
         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'
@@ -62,6 +65,7 @@ class I4xSystem(object):
             if settings.DEBUG:
                 log.info("[courseware.module_render.I4xSystem] filestore path = %s",
                          filestore)
+        self.module_from_xml = module_from_xml
         self.render_function = render_function
         self.render_template = render_template
         self.exception404 = Http404
@@ -127,6 +131,18 @@ def grade_histogram(module_id):
         return []
     return grades
 
+
+def make_module_from_xml_fn(user, request, student_module_cache, position):
+    '''Create the make_from_xml() function'''
+    def module_from_xml(xml):
+        '''Modules need a way to convert xml to instance objects.
+        Pass the rest of the context through.'''
+        (instance, sm, module_type) = get_module(
+            user, request, xml, student_module_cache, position)
+        return instance
+    return module_from_xml
+
+
 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
@@ -165,6 +181,9 @@ def get_module(user, request, module_xml, student_module_cache, position=None):
     # Setup system context for module instance
     ajax_url = settings.MITX_ROOT_URL + '/modx/' + module_type + '/' + module_id + '/'
 
+    module_from_xml = make_module_from_xml_fn(
+        user, request, student_module_cache, position)
+    
     system = I4xSystem(track_function = make_track_function(request), 
                        render_function = lambda xml: render_x_module(
                            user, request, xml, student_module_cache, position),
@@ -172,6 +191,7 @@ def get_module(user, request, module_xml, student_module_cache, position=None):
                        ajax_url = ajax_url,
                        request = request,
                        filestore = OSFS(data_root),
+                       module_from_xml = module_from_xml,
                        )
     # pass position specified in URL to module through I4xSystem
     system.set('position', position) 
@@ -295,9 +315,17 @@ def modx_dispatch(request, module=None, dispatch=None, id=None):
             response = HttpResponse(json.dumps({'success': error_msg}))
         return response
 
+    # TODO: This doesn't have a cache of child student modules.  Just
+    # passing the current one.  If ajax calls end up needing children,
+    # this won't work (but fixing it may cause performance issues...)
+    # Figure out :)
+    module_from_xml = make_module_from_xml_fn(
+        request.user, request, [s], None)
+
     # Create the module
     system = I4xSystem(track_function = make_track_function(request), 
-                       render_function = None, 
+                       render_function = None,
+                       module_from_xml = module_from_xml,
                        render_template = render_to_string,
                        ajax_url = ajax_url,
                        request = request,
@@ -316,7 +344,11 @@ def modx_dispatch(request, module=None, dispatch=None, id=None):
         return response
 
     # Let the module handle the AJAX
-    ajax_return = instance.handle_ajax(dispatch, request.POST)
+    try:
+        ajax_return = instance.handle_ajax(dispatch, request.POST)
+    except:
+        log.exception("error processing ajax call")
+        raise
 
     # Save the state back to the database
     s.state = instance.get_state()

lms/static/coffee/src/modules/problem.coffee

@@ -17,12 +17,20 @@ class @Problem
     @$('section.action input.save').click @save
     @$('input.math').keyup(@refreshMath).each(@refreshMath)
 
+  update_progress: (response) =>
+    if response.progress_changed
+        @element.attr progress: response.progress
+        @element.trigger('progressChanged')
+
   render: (content) ->
     if content
       @element.html(content)
       @bind()
     else
-      @element.load @content_url, @bind
+      $.postWithPrefix "/modx/problem/#{@id}/problem_get", '', (response) =>
+        @element.html(response.html)
+        @bind()
+      
 
   check: =>
     Logger.log 'problem_check', @answers
@@ -30,19 +38,22 @@ class @Problem
       switch response.success
         when 'incorrect', 'correct'
           @render(response.contents)
+          @update_progress response
         else
           alert(response.success)
 
   reset: =>
     Logger.log 'problem_reset', @answers
-    $.postWithPrefix "/modx/problem/#{@id}/problem_reset", id: @id, (content) =>
-      @render(content)
+    $.postWithPrefix "/modx/problem/#{@id}/problem_reset", id: @id, (response) =>
+        @render(response.html)
+        @update_progress response
 
   show: =>
     if !@element.hasClass 'showed'
       Logger.log 'problem_show', problem: @id
       $.postWithPrefix "/modx/problem/#{@id}/problem_show", (response) =>
-        $.each response, (key, value) =>
+        answers = response.answers
+        $.each answers, (key, value) =>
           if $.isArray(value)
             for choice in value
               @$("label[for='input_#{key}_#{choice}']").attr correct_answer: 'true'
@@ -51,6 +62,7 @@ class @Problem
         MathJax.Hub.Queue ["Typeset", MathJax.Hub]
         @$('.show').val 'Hide Answer'
         @element.addClass 'showed'
+        @update_progress response
     else
       @$('[id^=answer_], [id^=solution_]').text ''
       @$('[correct_answer]').attr correct_answer: null
@@ -62,6 +74,7 @@ class @Problem
     $.postWithPrefix "/modx/problem/#{@id}/problem_save", @answers, (response) =>
       if response.success
         alert 'Saved'
+      @update_progress response
 
   refreshMath: (event, element) =>
     element = event.target unless element

lms/static/coffee/src/modules/sequence.coffee

@@ -2,6 +2,7 @@ class @Sequence
   constructor: (@id, @elements, @tag, position) ->
     @element = $("#sequence_#{@id}")
     @buildNavigation()
+    @initProgress()
     @bind()
     @render position
 
@@ -11,11 +12,52 @@ class @Sequence
   bind: ->
     @$('#sequence-list a').click @goto
 
+  initProgress: ->
+    @progressTable = {}  # "#problem_#{id}" -> progress
+
+
+  hookUpProgressEvent: ->
+    $('.problems-wrapper').bind 'progressChanged', @updateProgress
+
+  mergeProgress: (p1, p2) ->
+    if p1 == "done" and p2 == "done"
+      return "done"
+    # not done, so if any progress on either, in_progress
+    w1 = p1 == "done" or p1 == "in_progress"
+    w2 = p2 == "done" or p2 == "in_progress"
+    if w1 or w2
+      return "in_progress"
+
+    return "none"
+
+  updateProgress: =>
+    new_progress = "none"
+    _this = this
+    $('.problems-wrapper').each (index) ->
+      progress = $(this).attr 'progress'
+      new_progress = _this.mergeProgress progress, new_progress
+
+    @progressTable[@position] = new_progress
+    @setProgress(new_progress, @link_for(@position))
+
+  setProgress: (progress, element) ->
+      element.removeClass('progress-none')
+             .removeClass('progress-some')
+             .removeClass('progress-done')
+      switch progress
+        when 'none' then element.addClass('progress-none')
+        when 'in_progress' then element.addClass('progress-some')
+        when 'done' then element.addClass('progress-done')
+  
   buildNavigation: ->
     $.each @elements, (index, item) =>
       link = $('<a>').attr class: "seq_#{item.type}_inactive", 'data-element': index + 1
       title = $('<p>').html(item.title)
+      # TODO: add item.progress_str either to the title or somewhere else.
+      # Make sure it gets updated after ajax calls
       list_item = $('<li>').append(link.append(title))
+      @setProgress item.progress_stat, link
+        
       @$('#sequence-list').append list_item
 
   toggleArrows: =>
@@ -36,13 +78,14 @@ class @Sequence
       if @position != undefined
         @mark_visited @position
         $.postWithPrefix "/modx/#{@tag}/#{@id}/goto_position", position: new_position
-
+        
       @mark_active new_position
       @$('#seq_content').html @elements[new_position - 1].content
 
       MathJax.Hub.Queue(["Typeset", MathJax.Hub])
       @position = new_position
       @toggleArrows()
+      @hookUpProgressEvent()
       @element.trigger 'contentChanged'
 
   goto: (event) =>
@@ -67,7 +110,17 @@ class @Sequence
     @$("#sequence-list a[data-element=#{position}]")
 
   mark_visited: (position) ->
-    @link_for(position).attr class: "seq_#{@elements[position - 1].type}_visited"
+    # Don't overwrite class attribute to avoid changing Progress class
+    type = @elements[position - 1].type
+    element = @link_for(position)
+    element.removeClass("seq_#{type}_inactive")
+    .removeClass("seq_#{type}_active")
+    .addClass("seq_#{type}_visited")
 
   mark_active: (position) ->
-    @link_for(position).attr class: "seq_#{@elements[position - 1].type}_active"
+    # Don't overwrite class attribute to avoid changing Progress class
+    type = @elements[position - 1].type
+    element = @link_for(position)
+    element.removeClass("seq_#{type}_inactive")
+    .removeClass("seq_#{type}_visited")
+    .addClass("seq_#{type}_active")