From a0a516720782cfbb4858996f6150049a89ba95f8 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Wed, 28 Nov 2012 11:12:06 -0500 Subject: [PATCH 1/7] Hook up testing for self assessment modules --- .../xmodule/xmodule/self_assessment_module.py | 25 +++++-------- common/lib/xmodule/xmodule/tests/__init__.py | 2 +- .../xmodule/xmodule/tests/test_progress.py | 4 +-- .../xmodule/tests/test_self_assessment.py | 35 +++++++++++++++++++ 4 files changed, 46 insertions(+), 20 deletions(-) create mode 100644 common/lib/xmodule/xmodule/tests/test_self_assessment.py diff --git a/common/lib/xmodule/xmodule/self_assessment_module.py b/common/lib/xmodule/xmodule/self_assessment_module.py index 2edf5467b2..d76786007a 100644 --- a/common/lib/xmodule/xmodule/self_assessment_module.py +++ b/common/lib/xmodule/xmodule/self_assessment_module.py @@ -109,15 +109,13 @@ class SelfAssessmentModule(XModule): self.state = instance_state.get('state', 'initial') + self.attempts = instance_state.get('attempts', 0) + self.max_attempts = int(self.metadata.get('attempts', MAX_ATTEMPTS)) + # Used for progress / grading. Currently get credit just for # completion (doesn't matter if you self-assessed correct/incorrect). - self._max_score = int(self.metadata.get('max_score', MAX_SCORE)) - self.attempts = instance_state.get('attempts', 0) - - self.max_attempts = int(self.metadata.get('attempts', MAX_ATTEMPTS)) - self.rubric = definition['rubric'] self.prompt = definition['prompt'] self.submit_message = definition['submitmessage'] @@ -149,26 +147,19 @@ class SelfAssessmentModule(XModule): # cdodge: perform link substitutions for any references to course static content (e.g. images) return rewrite_links(html, self.rewrite_content_links) - def get_score(self): - """ - Returns dict with 'score' key - """ - return {'score': self.get_last_score()} - def max_score(self): """ Return max_score """ return self._max_score - def get_last_score(self): + def get_score(self): """ Returns the last score in the list """ - last_score=0 - if(len(self.scores)>0): - last_score=self.scores[len(self.scores)-1] - return last_score + if len(self.scores) > 0: + return self.scores[-1] + return 0 def get_progress(self): ''' @@ -176,7 +167,7 @@ class SelfAssessmentModule(XModule): ''' if self._max_score > 0: try: - return Progress(self.get_last_score(), self._max_score) + return Progress(self.get_score(), self._max_score) except Exception as err: log.exception("Got bad progress") return None diff --git a/common/lib/xmodule/xmodule/tests/__init__.py b/common/lib/xmodule/xmodule/tests/__init__.py index ed64c45118..80b933e06d 100644 --- a/common/lib/xmodule/xmodule/tests/__init__.py +++ b/common/lib/xmodule/xmodule/tests/__init__.py @@ -19,7 +19,7 @@ import xmodule from xmodule.x_module import ModuleSystem from mock import Mock -i4xs = ModuleSystem( +test_system = ModuleSystem( ajax_url='courses/course_id/modx/a_location', track_function=Mock(), get_module=Mock(), diff --git a/common/lib/xmodule/xmodule/tests/test_progress.py b/common/lib/xmodule/xmodule/tests/test_progress.py index 94a0a19d7c..cb011cdc2b 100644 --- a/common/lib/xmodule/xmodule/tests/test_progress.py +++ b/common/lib/xmodule/xmodule/tests/test_progress.py @@ -5,7 +5,7 @@ import unittest from xmodule.progress import Progress from xmodule import x_module -from . import i4xs +from . import test_system class ProgressTest(unittest.TestCase): ''' Test that basic Progress objects work. A Progress represents a @@ -133,6 +133,6 @@ class ModuleProgressTest(unittest.TestCase): ''' def test_xmodule_default(self): '''Make sure default get_progress exists, returns None''' - xm = x_module.XModule(i4xs, 'a://b/c/d/e', None, {}) + xm = x_module.XModule(test_system, 'a://b/c/d/e', None, {}) p = xm.get_progress() self.assertEqual(p, None) diff --git a/common/lib/xmodule/xmodule/tests/test_self_assessment.py b/common/lib/xmodule/xmodule/tests/test_self_assessment.py new file mode 100644 index 0000000000..701a7f15fe --- /dev/null +++ b/common/lib/xmodule/xmodule/tests/test_self_assessment.py @@ -0,0 +1,35 @@ +import json +from mock import Mock +import unittest + +from xmodule.self_assessment_module import SelfAssessmentModule +from xmodule.modulestore import Location + +from . import test_system + +class SelfAssessmentTest(unittest.TestCase): + + definition = {'rubric': 'A rubric', + 'prompt': 'Who?', + 'submitmessage': 'Shall we submit now?', + 'hintprompt': 'Consider this...'} + + location = Location(["i4x", "edX", "sa_test", "selfassessment", + "SampleQuestion"]) + + metadata = {} + + descriptor = Mock() + + def test_import(self): + state = json.dumps({'student_answers': [], + 'scores': [], + 'hints': [], + 'state': 'initial', + 'attempts': 0}) + + module = SelfAssessmentModule(test_system, self.location, + self.definition, self.descriptor, + state, {}) + + self.assertEqual(module.get_score(), 0) From f8e45e0a8ea4549cf8873d6d23e01d746048f96e Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Wed, 28 Nov 2012 13:21:48 -0500 Subject: [PATCH 2/7] Add tests for old logic --- common/lib/xmodule/xmodule/tests/__init__.py | 5 ++- .../xmodule/tests/test_self_assessment.py | 38 ++++++++++++++----- 2 files changed, 32 insertions(+), 11 deletions(-) diff --git a/common/lib/xmodule/xmodule/tests/__init__.py b/common/lib/xmodule/xmodule/tests/__init__.py index 80b933e06d..a07f1ddfaf 100644 --- a/common/lib/xmodule/xmodule/tests/__init__.py +++ b/common/lib/xmodule/xmodule/tests/__init__.py @@ -4,7 +4,7 @@ unittests for xmodule Run like this: rake test_common/lib/xmodule - + """ import unittest @@ -23,7 +23,8 @@ test_system = ModuleSystem( ajax_url='courses/course_id/modx/a_location', track_function=Mock(), get_module=Mock(), - render_template=Mock(), + # "render" to just the context... + render_template=lambda template, context: str(context), replace_urls=Mock(), user=Mock(), filestore=Mock(), diff --git a/common/lib/xmodule/xmodule/tests/test_self_assessment.py b/common/lib/xmodule/xmodule/tests/test_self_assessment.py index 701a7f15fe..0ddcda8764 100644 --- a/common/lib/xmodule/xmodule/tests/test_self_assessment.py +++ b/common/lib/xmodule/xmodule/tests/test_self_assessment.py @@ -12,24 +12,44 @@ class SelfAssessmentTest(unittest.TestCase): definition = {'rubric': 'A rubric', 'prompt': 'Who?', 'submitmessage': 'Shall we submit now?', - 'hintprompt': 'Consider this...'} + 'hintprompt': 'Consider this...', + } location = Location(["i4x", "edX", "sa_test", "selfassessment", "SampleQuestion"]) - metadata = {} + metadata = {'attempts': '10'} descriptor = Mock() def test_import(self): - state = json.dumps({'student_answers': [], - 'scores': [], - 'hints': [], - 'state': 'initial', - 'attempts': 0}) + state = json.dumps({'student_answers': ["Answer 1", "answer 2", "answer 3"], + 'scores': [0, 1], + 'hints': ['o hai'], + 'state': SelfAssessmentModule.ASSESSING, + 'attempts': 2}) module = SelfAssessmentModule(test_system, self.location, self.definition, self.descriptor, - state, {}) + state, {}, metadata=self.metadata) - self.assertEqual(module.get_score(), 0) + self.assertEqual(module.get_score(), 1) + + + self.assertTrue('answer 3' in module.get_html()) + self.assertFalse('answer 2' in module.get_html()) + + module.save_assessment({'assessment': '0'}) + self.assertEqual(module.state, module.REQUEST_HINT) + + module.save_hint({'hint': 'hint for ans 3'}) + self.assertEqual(module.state, module.DONE) + + d = module.reset({}) + self.assertTrue(d['success']) + self.assertEqual(module.state, module.INITIAL) + + # if we now assess as right, skip the REQUEST_HINT state + module.save_answer({'student_answer': 'answer 4'}) + module.save_assessment({'assessment': '1'}) + self.assertEqual(module.state, module.DONE) From e3b9238eefc6debfdec5d696a2b3b81fd2ccceed Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Wed, 28 Nov 2012 14:08:18 -0500 Subject: [PATCH 3/7] reorder imports --- common/lib/xmodule/xmodule/self_assessment_module.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/common/lib/xmodule/xmodule/self_assessment_module.py b/common/lib/xmodule/xmodule/self_assessment_module.py index d76786007a..d8994d1187 100644 --- a/common/lib/xmodule/xmodule/self_assessment_module.py +++ b/common/lib/xmodule/xmodule/self_assessment_module.py @@ -7,20 +7,21 @@ Parses xml definition file--see below for exact format. import copy from fs.errors import ResourceNotFoundError +import itertools +import json import logging -import os -import sys from lxml import etree from lxml.html import rewrite_links from path import path -import json -from progress import Progress +import os +import sys from pkg_resources import resource_string from .capa_module import only_one, ComplexEncoder from .editing_module import EditingDescriptor from .html_checker import check_html +from progress import Progress from .stringify import stringify_children from .x_module import XModule from .xml_module import XmlDescriptor From 4fd1a2fa1a6d2f04ba15aa53663e30c2fc878161 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Wed, 28 Nov 2012 14:09:09 -0500 Subject: [PATCH 4/7] Fix state tracking for self-assessment. now keep each set of responses in a separate dictionary --- .../xmodule/xmodule/self_assessment_module.py | 139 ++++++++++++++---- .../xmodule/tests/test_self_assessment.py | 3 +- 2 files changed, 112 insertions(+), 30 deletions(-) diff --git a/common/lib/xmodule/xmodule/self_assessment_module.py b/common/lib/xmodule/xmodule/self_assessment_module.py index d8994d1187..cf9e4a88c1 100644 --- a/common/lib/xmodule/xmodule/self_assessment_module.py +++ b/common/lib/xmodule/xmodule/self_assessment_module.py @@ -53,6 +53,8 @@ class SelfAssessmentModule(XModule): submissions too.) """ + STATE_VERSION = 1 + # states INITIAL = 'initial' ASSESSING = 'assessing' @@ -103,10 +105,13 @@ class SelfAssessmentModule(XModule): else: instance_state = {} - # Note: score responses are on scale from 0 to max_score - self.student_answers = instance_state.get('student_answers', []) - self.scores = instance_state.get('scores', []) - self.hints = instance_state.get('hints', []) + instance_state = self.convert_state_to_current_format(instance_state) + + # History is a list of tuples of (answer, score, hint), where hint may be + # None for any element, and score and hint can be None for the last (current) + # element. + # Scores are on scale from 0 to max_score + self.history = instance_state.get('history', []) self.state = instance_state.get('state', 'initial') @@ -122,14 +127,95 @@ class SelfAssessmentModule(XModule): self.submit_message = definition['submitmessage'] self.hint_prompt = definition['hintprompt'] + + def latest_answer(self): + """None if not available""" + if not self.history: + return None + return self.history[-1].get('answer') + + def latest_score(self): + """None if not available""" + if not self.history: + return None + return self.history[-1].get('score') + + def latest_hint(self): + """None if not available""" + if not self.history: + return None + return self.history[-1].get('hint') + + def new_history_entry(self, answer): + self.history.append({'answer': answer}) + + def record_latest_score(self, score): + """Assumes that state is right, so we're adding a score to the latest + history element""" + self.history[-1]['score'] = score + + def record_latest_hint(self, hint): + """Assumes that state is right, so we're adding a score to the latest + history element""" + self.history[-1]['hint'] = hint + + + @staticmethod + def convert_state_to_current_format(old_state): + """ + This module used to use a problematic state representation. This method + converts that into the new format. + + Args: + old_state: dict of state, as passed in. May be old. + + Returns: + new_state: dict of new state + """ + if old_state.get('version', 0) == SelfAssessmentModule.STATE_VERSION: + # already current + return old_state + + # for now, there's only one older format. + + new_state = {'version': SelfAssessmentModule.STATE_VERSION} + + def copy_if_present(key): + if key in old_state: + new_state[key] = old_state[key] + + for to_copy in ['attempts', 'state']: + copy_if_present(to_copy) + + # The answers, scores, and hints need to be kept together to avoid them + # getting out of sync. + + # NOTE: Since there's only one problem with a few hundred submissions + # in production so far, not trying to be smart about matching up hints + # and submissions in cases where they got out of sync. + + student_answers = old_state.get('student_answers', []) + scores = old_state.get('scores', []) + hints = old_state.get('hints', []) + + new_state['history'] = [ + {'answer': answer, + 'score': score, + 'hint': hint} + for answer, score, hint in itertools.izip_longest( + student_answers, scores, hints)] + return new_state + + def _allow_reset(self): """Can the module be reset?""" return self.state == self.DONE and self.attempts < self.max_attempts def get_html(self): #set context variables and render template - if self.state != self.INITIAL and self.student_answers: - previous_answer = self.student_answers[-1] + if self.state != self.INITIAL: + latest = self.latest_answer() + previous_answer = latest if latest is not None else '' else: previous_answer = '' @@ -158,9 +244,9 @@ class SelfAssessmentModule(XModule): """ Returns the last score in the list """ - if len(self.scores) > 0: - return self.scores[-1] - return 0 + score = self.latest_score() + return {'score': score if score is not None else 0, + 'total': self._max_score} def get_progress(self): ''' @@ -168,7 +254,7 @@ class SelfAssessmentModule(XModule): ''' if self._max_score > 0: try: - return Progress(self.get_score(), self._max_score) + return Progress(self.get_score()['score'], self._max_score) except Exception as err: log.exception("Got bad progress") return None @@ -242,9 +328,10 @@ class SelfAssessmentModule(XModule): if self.state in (self.INITIAL, self.ASSESSING): return '' - if self.state == self.DONE and len(self.hints) > 0: + if self.state == self.DONE: # display the previous hint - hint = self.hints[-1] + latest = self.latest_hint() + hint = latest if latest is not None else '' else: hint = '' @@ -287,7 +374,8 @@ class SelfAssessmentModule(XModule): if self.state != self.INITIAL: return self.out_of_sync_error(get) - self.student_answers.append(get['student_answer']) + # add new history element with answer and empty score and hint. + self.new_history_entry(get['student_answer']) self.state = self.ASSESSING return { @@ -310,18 +398,15 @@ class SelfAssessmentModule(XModule): 'message_html' only if success is true """ - n_answers = len(self.student_answers) - n_scores = len(self.scores) - if (self.state != self.ASSESSING or n_answers != n_scores + 1): - msg = "%d answers, %d scores" % (n_answers, n_scores) - return self.out_of_sync_error(get, msg) + if self.state != self.ASSESSING: + return self.out_of_sync_error(get) try: score = int(get['assessment']) - except: + except ValueError: return {'success': False, 'error': "Non-integer score value"} - self.scores.append(score) + self.record_latest_score(score) d = {'success': True,} @@ -352,7 +437,7 @@ class SelfAssessmentModule(XModule): # the same number of hints and answers. return self.out_of_sync_error(get) - self.hints.append(get['hint'].lower()) + self.record_latest_hint(get['hint']) self.state = self.DONE # increment attempts @@ -362,9 +447,8 @@ class SelfAssessmentModule(XModule): event_info = { 'selfassessment_id': self.location.url(), 'state': { - 'student_answers': self.student_answers, - 'score': self.scores, - 'hints': self.hints, + 'version': self.STATE_VERSION, + 'history': self.history, } } self.system.track_function('save_hint', event_info) @@ -399,12 +483,11 @@ class SelfAssessmentModule(XModule): """ state = { - 'student_answers': self.student_answers, - 'hints': self.hints, + 'version': self.STATE_VERSION, + 'history': self.history, 'state': self.state, - 'scores': self.scores, 'max_score': self._max_score, - 'attempts': self.attempts + 'attempts': self.attempts, } return json.dumps(state) diff --git a/common/lib/xmodule/xmodule/tests/test_self_assessment.py b/common/lib/xmodule/xmodule/tests/test_self_assessment.py index 0ddcda8764..055f75ed97 100644 --- a/common/lib/xmodule/xmodule/tests/test_self_assessment.py +++ b/common/lib/xmodule/xmodule/tests/test_self_assessment.py @@ -33,8 +33,7 @@ class SelfAssessmentTest(unittest.TestCase): self.definition, self.descriptor, state, {}, metadata=self.metadata) - self.assertEqual(module.get_score(), 1) - + self.assertEqual(module.get_score(), 0) self.assertTrue('answer 3' in module.get_html()) self.assertFalse('answer 2' in module.get_html()) From b41b45971256f314887019dbb46bef1ea49fa0e6 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Wed, 28 Nov 2012 14:09:32 -0500 Subject: [PATCH 5/7] update docstring on x_module.get_score to match reality --- common/lib/xmodule/xmodule/x_module.py | 38 +++++++++++++++++--------- 1 file changed, 25 insertions(+), 13 deletions(-) diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index 2b2e709bcb..6f3fb73356 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -233,17 +233,17 @@ class XModule(HTMLSnippet): self._loaded_children = [c for c in children if c is not None] return self._loaded_children - + def get_children_locations(self): ''' Returns the locations of each of child modules. - + Overriding this changes the behavior of get_children and anything that uses get_children, such as get_display_items. - + This method will not instantiate the modules of the children unless absolutely necessary, so it is cheaper to call than get_children - + These children will be the same children returned by the descriptor unless descriptor.has_dynamic_children() is true. ''' @@ -288,8 +288,20 @@ class XModule(HTMLSnippet): return '{}' def get_score(self): - ''' Score the student received on the problem. - ''' + """ + Score the student received on the problem, or None if there is no + score. + + Returns: + dictionary + {'score': integer, from 0 to get_max_score(), + 'total': get_max_score()} + + NOTE (vshnayder): not sure if this was the intended return value, but + that's what it's doing now. I suspect that we really want it to just + return a number. Would need to change (at least) capa and + modx_dispatch to match if we did that. + """ return None def max_score(self): @@ -319,7 +331,7 @@ class XModule(HTMLSnippet): get is a dictionary-like object ''' return "" - # cdodge: added to support dynamic substitutions of + # cdodge: added to support dynamic substitutions of # links for courseware assets (e.g. images). is passed through from lxml.html parser def rewrite_content_links(self, link): # see if we start with our format, e.g. 'xasset:' @@ -408,7 +420,7 @@ class XModuleDescriptor(Plugin, HTMLSnippet, ResourceTemplates): # cdodge: this is a list of metadata names which are 'system' metadata # and should not be edited by an end-user system_metadata_fields = [ 'data_dir' ] - + # A list of descriptor attributes that must be equal for the descriptors to # be equal equality_attributes = ('definition', 'metadata', 'location', @@ -562,18 +574,18 @@ class XModuleDescriptor(Plugin, HTMLSnippet, ResourceTemplates): self, metadata=self.metadata ) - - + + def has_dynamic_children(self): """ Returns True if this descriptor has dynamic children for a given student when the module is created. - + Returns False if the children of this descriptor are the same - children that the module will return for any student. + children that the module will return for any student. """ return False - + # ================================= JSON PARSING =========================== @staticmethod From 0a1b731dbcd660db2dabb4144948093c564d7d1c Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Wed, 28 Nov 2012 14:18:44 -0500 Subject: [PATCH 6/7] properly reset answer area on module reset --- common/lib/xmodule/xmodule/js/src/selfassessment/display.coffee | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/lib/xmodule/xmodule/js/src/selfassessment/display.coffee b/common/lib/xmodule/xmodule/js/src/selfassessment/display.coffee index 951eb42fce..5b70ab29aa 100644 --- a/common/lib/xmodule/xmodule/js/src/selfassessment/display.coffee +++ b/common/lib/xmodule/xmodule/js/src/selfassessment/display.coffee @@ -120,7 +120,7 @@ class @SelfAssessment if @state == 'done' $.postWithPrefix "#{@ajax_url}/reset", {}, (response) => if response.success - @answer_area.html('') + @answer_area.val('') @rubric_wrapper.html('') @hint_wrapper.html('') @message_wrapper.html('') From 4b58cb956084f022511e4f03bbef24c486b9e229 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Wed, 28 Nov 2012 15:04:07 -0500 Subject: [PATCH 7/7] Fix attempt tracking, fix test - increments attempts on any transition to DONE state --- .../xmodule/xmodule/self_assessment_module.py | 26 +++++++++++++------ .../xmodule/tests/test_self_assessment.py | 2 +- 2 files changed, 19 insertions(+), 9 deletions(-) diff --git a/common/lib/xmodule/xmodule/self_assessment_module.py b/common/lib/xmodule/xmodule/self_assessment_module.py index cf9e4a88c1..8498a210cd 100644 --- a/common/lib/xmodule/xmodule/self_assessment_module.py +++ b/common/lib/xmodule/xmodule/self_assessment_module.py @@ -160,6 +160,19 @@ class SelfAssessmentModule(XModule): self.history[-1]['hint'] = hint + def change_state(self, new_state): + """ + A centralized place for state changes--allows for hooks. If the + current state matches the old state, don't run any hooks. + """ + if self.state == new_state: + return + + self.state = new_state + + if self.state == self.DONE: + self.attempts += 1 + @staticmethod def convert_state_to_current_format(old_state): """ @@ -376,7 +389,7 @@ class SelfAssessmentModule(XModule): # add new history element with answer and empty score and hint. self.new_history_entry(get['student_answer']) - self.state = self.ASSESSING + self.change_state(self.ASSESSING) return { 'success': True, @@ -411,11 +424,11 @@ class SelfAssessmentModule(XModule): d = {'success': True,} if score == self.max_score(): - self.state = self.DONE + self.change_state(self.DONE) d['message_html'] = self.get_message_html() d['allow_reset'] = self._allow_reset() else: - self.state = self.REQUEST_HINT + self.change_state(self.REQUEST_HINT) d['hint_html'] = self.get_hint_html() d['state'] = self.state @@ -438,10 +451,7 @@ class SelfAssessmentModule(XModule): return self.out_of_sync_error(get) self.record_latest_hint(get['hint']) - self.state = self.DONE - - # increment attempts - self.attempts = self.attempts + 1 + self.change_state(self.DONE) # To the tracking logs! event_info = { @@ -473,7 +483,7 @@ class SelfAssessmentModule(XModule): 'success': False, 'error': 'Too many attempts.' } - self.state = self.INITIAL + self.change_state(self.INITIAL) return {'success': True} diff --git a/common/lib/xmodule/xmodule/tests/test_self_assessment.py b/common/lib/xmodule/xmodule/tests/test_self_assessment.py index 055f75ed97..d89190b1e0 100644 --- a/common/lib/xmodule/xmodule/tests/test_self_assessment.py +++ b/common/lib/xmodule/xmodule/tests/test_self_assessment.py @@ -33,7 +33,7 @@ class SelfAssessmentTest(unittest.TestCase): self.definition, self.descriptor, state, {}, metadata=self.metadata) - self.assertEqual(module.get_score(), 0) + self.assertEqual(module.get_score()['score'], 0) self.assertTrue('answer 3' in module.get_html()) self.assertFalse('answer 2' in module.get_html())