diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 1338cacd3b..cd43777e96 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -7,6 +7,10 @@ the top. Include a label indicating the component affected. Blades: Took videoalpha out of alpha, replacing the old video player +Common: Allow instructors to input complicated expressions as answers to +`NumericalResponse`s. Prior to the change only numbers were allowed, now any +answer from '1/3' to 'sqrt(12)*(1-1/3^2+1/5/3^2)' are valid. + LMS: Enable beta instructor dashboard. The beta dashboard is a rearchitecture of the existing instructor dashboard and is available by clicking a link at the top right of the existing dashboard. diff --git a/cms/djangoapps/contentstore/features/grading.feature b/cms/djangoapps/contentstore/features/grading.feature index 4b5cacc159..0e0ce561ee 100644 --- a/cms/djangoapps/contentstore/features/grading.feature +++ b/cms/djangoapps/contentstore/features/grading.feature @@ -93,3 +93,9 @@ Feature: Course Grading And I press the "Save" notification button And I reload the page Then I see the highest grade range is "Good" + + Scenario: User cannot edit failing grade range name + Given I have opened a new course in Studio + And I have populated the course + And I am viewing the grading settings + Then I cannot edit the "Fail" grade range diff --git a/cms/djangoapps/contentstore/features/grading.py b/cms/djangoapps/contentstore/features/grading.py index 40cba61edc..bedab86bd9 100644 --- a/cms/djangoapps/contentstore/features/grading.py +++ b/cms/djangoapps/contentstore/features/grading.py @@ -4,6 +4,7 @@ from lettuce import world, step from common import * from terrain.steps import reload_the_page +from selenium.common.exceptions import InvalidElementStateException @step(u'I am viewing the grading settings') @@ -130,6 +131,18 @@ def i_see_highest_grade_range(_step, range_name): grade = world.css_find(range_css).first assert grade.value == range_name + +@step(u'I cannot edit the "Fail" grade range$') +def cannot_edit_fail(_step): + range_css = 'span.letter-grade' + ranges = world.css_find(range_css) + assert len(ranges) == 2 + try: + ranges.last.value = 'Failure' + assert False, "Should not be able to edit failing range" + except InvalidElementStateException: + pass # We should get this exception on failing to edit the element + def get_type_index(name): name_id = '#course-grading-assignment-name' all_types = world.css_find(name_id) diff --git a/cms/djangoapps/contentstore/features/video-editor.py b/cms/djangoapps/contentstore/features/video-editor.py index f9d433fc02..312e2d545f 100644 --- a/cms/djangoapps/contentstore/features/video-editor.py +++ b/cms/djangoapps/contentstore/features/video-editor.py @@ -5,7 +5,7 @@ from lettuce import world, step from terrain.steps import reload_the_page -@step('I have set "show captions" to (.*)') +@step('I have set "show captions" to (.*)$') def set_show_captions(step, setting): world.css_click('a.edit-button') world.wait_for(lambda _driver: world.css_visible('a.save-button')) @@ -13,7 +13,7 @@ def set_show_captions(step, setting): world.css_click('a.save-button') -@step('when I view the (video.*) it (.*) show the captions') +@step('when I view the (video.*) it (.*) show the captions$') def shows_captions(_step, video_type, show_captions): # Prevent cookies from overriding course settings world.browser.cookies.delete('hide_captions') @@ -39,7 +39,7 @@ def correct_video_settings(_step): ['Youtube ID for 1.5x speed', '', False]]) -@step('my video display name change is persisted on save') +@step('my video display name change is persisted on save$') def video_name_persisted(step): world.css_click('a.save-button') reload_the_page(step) diff --git a/cms/djangoapps/contentstore/features/video.py b/cms/djangoapps/contentstore/features/video.py index 2c3d2cdfa9..afa9953c90 100644 --- a/cms/djangoapps/contentstore/features/video.py +++ b/cms/djangoapps/contentstore/features/video.py @@ -19,25 +19,25 @@ def i_created_a_video_component(step): ) -@step('when I view the (.*) it does not have autoplay enabled') +@step('when I view the (.*) it does not have autoplay enabled$') def does_not_autoplay(_step, video_type): assert world.css_find('.%s' % video_type)[0]['data-autoplay'] == 'False' assert world.css_has_class('.video_control', 'play') -@step('creating a video takes a single click') +@step('creating a video takes a single click$') def video_takes_a_single_click(_step): assert(not world.is_css_present('.xmodule_VideoModule')) world.css_click("a[data-category='video']") assert(world.is_css_present('.xmodule_VideoModule')) -@step('I edit the component') +@step('I edit the component$') def i_edit_the_component(_step): world.edit_component() -@step('I have (hidden|toggled) captions') +@step('I have (hidden|toggled) captions$') def hide_or_show_captions(step, shown): button_css = 'a.hide-subtitles' if shown == 'hidden': @@ -54,7 +54,7 @@ def hide_or_show_captions(step, shown): world.css_click(button_css) -@step('I have created a video with only XML data') +@step('I have created a video with only XML data$') def xml_only_video(step): # Create a new video *without* metadata. This requires a certain # amount of rummaging to make sure all the correct data is present @@ -84,7 +84,7 @@ def xml_only_video(step): reload_the_page(step) -@step('The correct Youtube video is shown') +@step('The correct Youtube video is shown$') def the_youtube_video_is_shown(_step): ele = world.css_find('.video').first assert ele['data-streams'].split(':')[1] == world.scenario_dict['YOUTUBE_ID'] diff --git a/cms/static/js/views/settings/settings_grading_view.js b/cms/static/js/views/settings/settings_grading_view.js index 8c2af25f8c..b6fac04899 100644 --- a/cms/static/js/views/settings/settings_grading_view.js +++ b/cms/static/js/views/settings/settings_grading_view.js @@ -8,7 +8,7 @@ CMS.Views.Settings.Grading = CMS.Views.ValidatingView.extend({ // Leaving change in as fallback for older browsers "change input" : "updateModel", "change textarea" : "updateModel", - "input span[contenteditable]" : "updateDesignation", + "input span[contenteditable=true]" : "updateDesignation", "click .settings-extra header" : "showSettingsExtras", "click .new-grade-button" : "addNewGrade", "click .remove-button" : "removeGrade", @@ -20,7 +20,7 @@ CMS.Views.Settings.Grading = CMS.Views.ValidatingView.extend({ initialize : function() { // load template for grading view var self = this; - this.gradeCutoffTemplate = _.template('
  • ' + + this.gradeCutoffTemplate = _.template('
  • ' + '<%= descriptor %>' + '' + '<% if (removable) {%>remove<% ;} %>' + @@ -168,9 +168,12 @@ CMS.Views.Settings.Grading = CMS.Views.ValidatingView.extend({ }, this); // add fail which is not in data - var failBar = this.gradeCutoffTemplate({ descriptor : this.failLabel(), - width : nextWidth, removable : false}); - $(failBar).find("span[contenteditable=true]").attr("contenteditable", false); + var failBar = $(this.gradeCutoffTemplate({ + descriptor : this.failLabel(), + width : nextWidth, + removable : false + })); + failBar.find("span[contenteditable=true]").attr("contenteditable", false); gradelist.append(failBar); gradelist.children().last().resizable({ handles: "e", diff --git a/common/djangoapps/static_replace/__init__.py b/common/djangoapps/static_replace/__init__.py index 63c576cdd2..d7f2df8322 100644 --- a/common/djangoapps/static_replace/__init__.py +++ b/common/djangoapps/static_replace/__init__.py @@ -119,7 +119,15 @@ def replace_static_urls(text, data_directory, course_id=None): elif course_id and modulestore().get_modulestore_type(course_id) != XML_MODULESTORE_TYPE: # first look in the static file pipeline and see if we are trying to reference # a piece of static content which is in the mitx repo (e.g. JS associated with an xmodule) - if staticfiles_storage.exists(rest): + + exists_in_staticfiles_storage = False + try: + exists_in_staticfiles_storage = staticfiles_storage.exists(rest) + except Exception as err: + log.warning("staticfiles_storage couldn't find path {0}: {1}".format( + rest, str(err))) + + if exists_in_staticfiles_storage: url = staticfiles_storage.url(rest) else: # if not, then assume it's courseware specific content and then look in the @@ -142,6 +150,7 @@ def replace_static_urls(text, data_directory, course_id=None): return "".join([quote, url, quote]) + return re.sub( _url_replace_regex('/static/(?!{data_dir})'.format(data_dir=data_directory)), replace_static_url, diff --git a/common/lib/capa/capa/responsetypes.py b/common/lib/capa/capa/responsetypes.py index c6e7e4d013..731230ecc1 100644 --- a/common/lib/capa/capa/responsetypes.py +++ b/common/lib/capa/capa/responsetypes.py @@ -17,6 +17,7 @@ import logging import numbers import numpy import os +from pyparsing import ParseException import sys import random import re @@ -826,45 +827,89 @@ class NumericalResponse(LoncapaResponse): required_attributes = ['answer'] max_inputfields = 1 + def __init__(self, *args, **kwargs): + self.correct_answer = '' + self.tolerance = '0' # Default value + super(NumericalResponse, self).__init__(*args, **kwargs) + def setup_response(self): xml = self.xml context = self.context self.correct_answer = contextualize_text(xml.get('answer'), context) + + # Find the tolerance + tolerance_xml = xml.xpath( + '//*[@id=$id]//responseparam[@type="tolerance"]/@default', + id=xml.get('id') + ) + if tolerance_xml: # If it isn't an empty list... + self.tolerance = contextualize_text(tolerance_xml[0], context) + + def get_staff_ans(self): + """ + Given the staff answer as a string, find its float value. + + Use `evaluator` for this, but for backward compatability, try the + built-in method `complex` (which used to be the standard). + """ + try: - self.tolerance_xml = xml.xpath( - '//*[@id=$id]//responseparam[@type="tolerance"]/@default', - id=xml.get('id'))[0] - self.tolerance = contextualize_text(self.tolerance_xml, context) - except IndexError: # xpath found an empty list, so (...)[0] is the error - self.tolerance = '0' + correct_ans = complex(self.correct_answer) + except ValueError: + # When `correct_answer` is not of the form X+Yj, it raises a + # `ValueError`. Then test if instead it is a math expression. + # `complex` seems to only generate `ValueErrors`, only catch these. + try: + correct_ans = evaluator({}, {}, self.correct_answer) + except Exception: + log.debug("Content error--answer '%s' is not a valid number", self.correct_answer) + raise StudentInputError( + "There was a problem with the staff answer to this problem" + ) + + return correct_ans def get_score(self, student_answers): '''Grade a numeric response ''' student_answer = student_answers[self.answer_id] + correct_float = self.get_staff_ans() + + general_exception = StudentInputError( + u"Could not interpret '{0}' as a number".format(cgi.escape(student_answer)) + ) + + # Begin `evaluator` block + # Catch a bunch of exceptions and give nicer messages to the student. try: - correct_ans = complex(self.correct_answer) - except ValueError: - log.debug("Content error--answer '{0}' is not a valid complex number".format( - self.correct_answer)) + student_float = evaluator({}, {}, student_answer) + except UndefinedVariable as undef_var: raise StudentInputError( - "There was a problem with the staff answer to this problem") - - try: - correct = compare_with_tolerance( - evaluator(dict(), dict(), student_answer), - correct_ans, self.tolerance) - # We should catch this explicitly. - # I think this is just pyparsing.ParseException, calc.UndefinedVariable: - # But we'd need to confirm - except: - # Use the traceback-preserving version of re-raising with a - # different type - type, value, traceback = sys.exc_info() - - raise StudentInputError, ("Could not interpret '%s' as a number" % - cgi.escape(student_answer)), traceback + u"You may not use variables ({0}) in numerical problems".format(undef_var.message) + ) + except ValueError as val_err: + if 'factorial' in val_err.message: + # This is thrown when fact() or factorial() is used in an answer + # that evaluates on negative and/or non-integer inputs + # ve.message will be: `factorial() only accepts integral values` or + # `factorial() not defined for negative values` + raise StudentInputError( + ("factorial function evaluated outside its domain:" + "'{0}'").format(cgi.escape(student_answer)) + ) + else: + raise general_exception + except ParseException: + raise StudentInputError( + u"Invalid math syntax: '{0}'".format(cgi.escape(student_answer)) + ) + except Exception: + raise general_exception + # End `evaluator` block -- we figured out the student's answer! + correct = compare_with_tolerance( + student_float, correct_float, self.tolerance + ) if correct: return CorrectMap(self.answer_id, 'correct') else: @@ -1691,18 +1736,26 @@ class FormulaResponse(LoncapaResponse): required_attributes = ['answer', 'samples'] max_inputfields = 1 + def __init__(self, *args, **kwargs): + self.correct_answer = '' + self.samples = '' + self.tolerance = '1e-5' # Default value + self.case_sensitive = False + super(FormulaResponse, self).__init__(*args, **kwargs) + def setup_response(self): xml = self.xml context = self.context self.correct_answer = contextualize_text(xml.get('answer'), context) self.samples = contextualize_text(xml.get('samples'), context) - try: - self.tolerance_xml = xml.xpath( - '//*[@id=$id]//responseparam[@type="tolerance"]/@default', - id=xml.get('id'))[0] - self.tolerance = contextualize_text(self.tolerance_xml, context) - except Exception: - self.tolerance = '0.00001' + + # Find the tolerance + tolerance_xml = xml.xpath( + '//*[@id=$id]//responseparam[@type="tolerance"]/@default', + id=xml.get('id') + ) + if tolerance_xml: # If it isn't an empty list... + self.tolerance = contextualize_text(tolerance_xml[0], context) ts = xml.get('type') if ts is None: @@ -1734,7 +1787,7 @@ class FormulaResponse(LoncapaResponse): ranges = dict(zip(variables, sranges)) for _ in range(numsamples): instructor_variables = self.strip_dict(dict(self.context)) - student_variables = dict() + student_variables = {} # ranges give numerical ranges for testing for var in ranges: # TODO: allow specified ranges (i.e. integers and complex numbers) for random variables @@ -1746,7 +1799,7 @@ class FormulaResponse(LoncapaResponse): # Call `evaluator` on the instructor's answer and get a number instructor_result = evaluator( - instructor_variables, dict(), + instructor_variables, {}, expected, case_sensitive=self.case_sensitive ) try: @@ -1756,7 +1809,7 @@ class FormulaResponse(LoncapaResponse): # Call `evaluator` on the student's answer; look for exceptions student_result = evaluator( student_variables, - dict(), + {}, given, case_sensitive=self.case_sensitive ) @@ -2422,7 +2475,7 @@ class ChoiceTextResponse(LoncapaResponse): # if all that is important is verifying numericality try: partial_correct = compare_with_tolerance( - evaluator(dict(), dict(), answer_value), + evaluator({}, {}, answer_value), correct_ans, tolerance ) diff --git a/common/lib/capa/capa/tests/test_responsetypes.py b/common/lib/capa/capa/tests/test_responsetypes.py index a756dc640e..fd056f884e 100644 --- a/common/lib/capa/capa/tests/test_responsetypes.py +++ b/common/lib/capa/capa/tests/test_responsetypes.py @@ -5,12 +5,14 @@ Tests of responsetypes from datetime import datetime import json import os +import pyparsing import random import unittest import textwrap import mock from . import new_loncapa_problem, test_system +import calc from capa.responsetypes import LoncapaProblemError, \ StudentInputError, ResponseError @@ -22,7 +24,7 @@ from pytz import UTC class ResponseTest(unittest.TestCase): - """ Base class for tests of capa responses.""" + """Base class for tests of capa responses.""" xml_factory_class = None @@ -442,91 +444,6 @@ class FormulaResponseTest(ResponseTest): self.assert_grade(problem, '2*x', 'correct') self.assert_grade(problem, '3*x', 'incorrect') - def test_parallel_resistors(self): - """ - Test parallel resistors - """ - sample_dict = {'R1': (10, 10), 'R2': (2, 2), 'R3': (5, 5), 'R4': (1, 1)} - - # Test problem - problem = self.build_problem(sample_dict=sample_dict, - num_samples=10, - tolerance=0.01, - answer="R1||R2") - # Expect answer to be marked correct - input_formula = "R1||R2" - self.assert_grade(problem, input_formula, "correct") - - # Expect random number to be marked incorrect - input_formula = "13" - self.assert_grade(problem, input_formula, "incorrect") - - # Expect incorrect answer marked incorrect - input_formula = "R3||R4" - self.assert_grade(problem, input_formula, "incorrect") - - def test_default_variables(self): - """ - Test the default variables provided in calc.py - - which are: j (complex number), e, pi, k, c, T, q - """ - - # Sample x in the range [-10,10] - sample_dict = {'x': (-10, 10)} - default_variables = [('j', 2, 3), ('e', 2, 3), ('pi', 2, 3), ('c', 2, 3), ('T', 2, 3), - ('k', 2 * 10 ** 23, 3 * 10 ** 23), # note k = scipy.constants.k = 1.3806488e-23 - ('q', 2 * 10 ** 19, 3 * 10 ** 19)] # note k = scipy.constants.e = 1.602176565e-19 - for (var, cscalar, iscalar) in default_variables: - # The expected solution is numerically equivalent to cscalar*var - correct = '{0}*x*{1}'.format(cscalar, var) - incorrect = '{0}*x*{1}'.format(iscalar, var) - problem = self.build_problem(sample_dict=sample_dict, - num_samples=10, - tolerance=0.01, - answer=correct) - - # Expect that the inputs are graded correctly - self.assert_grade(problem, correct, 'correct', - msg="Failed on variable {0}; the given, correct answer was {1} but graded 'incorrect'".format(var, correct)) - self.assert_grade(problem, incorrect, 'incorrect', - msg="Failed on variable {0}; the given, incorrect answer was {1} but graded 'correct'".format(var, incorrect)) - - def test_default_functions(self): - """ - Test the default functions provided in common/lib/capa/capa/calc.py - - which are: - sin, cos, tan, sqrt, log10, log2, ln, - arccos, arcsin, arctan, abs, - fact, factorial - """ - w = random.randint(3, 10) - sample_dict = {'x': (-10, 10), # Sample x in the range [-10,10] - 'y': (1, 10), # Sample y in the range [1,10] - logs, arccos need positive inputs - 'z': (-1, 1), # Sample z in the range [1,10] - for arcsin, arctan - 'w': (w, w)} # Sample w is a random, positive integer - factorial needs a positive, integer input, - # and the way formularesponse is defined, we can only specify a float range - - default_functions = [('sin', 2, 3, 'x'), ('cos', 2, 3, 'x'), ('tan', 2, 3, 'x'), ('sqrt', 2, 3, 'y'), ('log10', 2, 3, 'y'), - ('log2', 2, 3, 'y'), ('ln', 2, 3, 'y'), ('arccos', 2, 3, 'z'), ('arcsin', 2, 3, 'z'), ('arctan', 2, 3, 'x'), - ('abs', 2, 3, 'x'), ('fact', 2, 3, 'w'), ('factorial', 2, 3, 'w')] - for (func, cscalar, iscalar, var) in default_functions: - print 'func is: {0}'.format(func) - # The expected solution is numerically equivalent to cscalar*func(var) - correct = '{0}*{1}({2})'.format(cscalar, func, var) - incorrect = '{0}*{1}({2})'.format(iscalar, func, var) - problem = self.build_problem(sample_dict=sample_dict, - num_samples=10, - tolerance=0.01, - answer=correct) - - # Expect that the inputs are graded correctly - self.assert_grade(problem, correct, 'correct', - msg="Failed on function {0}; the given, correct answer was {1} but graded 'incorrect'".format(func, correct)) - self.assert_grade(problem, incorrect, 'incorrect', - msg="Failed on function {0}; the given, incorrect answer was {1} but graded 'correct'".format(func, incorrect)) - def test_grade_infinity(self): """ Test that a large input on a problem with relative tolerance isn't @@ -885,92 +802,118 @@ class NumericalResponseTest(ResponseTest): from capa.tests.response_xml_factory import NumericalResponseXMLFactory xml_factory_class = NumericalResponseXMLFactory + # We blend the line between integration (using evaluator) and exclusively + # unit testing the NumericalResponse (mocking out the evaluator) + # For simple things its not worth the effort. def test_grade_exact(self): - problem = self.build_problem(question_text="What is 2 + 2?", - explanation="The answer is 4", - answer=4) + problem = self.build_problem(answer=4) correct_responses = ["4", "4.0", "4.00"] incorrect_responses = ["", "3.9", "4.1", "0"] self.assert_multiple_grade(problem, correct_responses, incorrect_responses) def test_grade_decimal_tolerance(self): - problem = self.build_problem(question_text="What is 2 + 2 approximately?", - explanation="The answer is 4", - answer=4, - tolerance=0.1) + problem = self.build_problem(answer=4, tolerance=0.1) correct_responses = ["4.0", "4.00", "4.09", "3.91"] incorrect_responses = ["", "4.11", "3.89", "0"] self.assert_multiple_grade(problem, correct_responses, incorrect_responses) def test_grade_percent_tolerance(self): - problem = self.build_problem(question_text="What is 2 + 2 approximately?", - explanation="The answer is 4", - answer=4, - tolerance="10%") + problem = self.build_problem(answer=4, tolerance="10%") correct_responses = ["4.0", "4.3", "3.7", "4.30", "3.70"] incorrect_responses = ["", "4.5", "3.5", "0"] self.assert_multiple_grade(problem, correct_responses, incorrect_responses) - def test_grade_infinity(self): - # This resolves a bug where a problem with relative tolerance would - # pass with any arbitrarily large student answer. - problem = self.build_problem(question_text="What is 2 + 2 approximately?", - explanation="The answer is 4", - answer=4, - tolerance="10%") - correct_responses = [] - incorrect_responses = ["1e999", "-1e999"] - self.assert_multiple_grade(problem, correct_responses, incorrect_responses) - - def test_grade_nan(self): - # Attempt to produce a value which causes the student's answer to be - # evaluated to nan. See if this is resolved correctly. - problem = self.build_problem(question_text="What is 2 + 2 approximately?", - explanation="The answer is 4", - answer=4, - tolerance="10%") - correct_responses = [] - # Right now these evaluate to `nan` - # `4 + nan` should be incorrect - incorrect_responses = ["0*1e999", "4 + 0*1e999"] - self.assert_multiple_grade(problem, correct_responses, incorrect_responses) - def test_grade_with_script(self): script_text = "computed_response = math.sqrt(4)" - problem = self.build_problem(question_text="What is sqrt(4)?", - explanation="The answer is 2", - answer="$computed_response", - script=script_text) + problem = self.build_problem(answer="$computed_response", script=script_text) correct_responses = ["2", "2.0"] incorrect_responses = ["", "2.01", "1.99", "0"] self.assert_multiple_grade(problem, correct_responses, incorrect_responses) - def test_grade_with_script_and_tolerance(self): - script_text = "computed_response = math.sqrt(4)" - problem = self.build_problem(question_text="What is sqrt(4)?", - explanation="The answer is 2", - answer="$computed_response", - tolerance="0.1", - script=script_text) - correct_responses = ["2", "2.0", "2.05", "1.95"] - incorrect_responses = ["", "2.11", "1.89", "0"] - self.assert_multiple_grade(problem, correct_responses, incorrect_responses) - - def test_exponential_answer(self): - problem = self.build_problem(question_text="What 5 * 10?", - explanation="The answer is 50", - answer="5e+1") - correct_responses = ["50", "50.0", "5e1", "5e+1", "50e0", "500e-1"] - incorrect_responses = ["", "3.9", "4.1", "0", "5.01e1"] - self.assert_multiple_grade(problem, correct_responses, incorrect_responses) - def test_raises_zero_division_err(self): - """See if division by zero is handled correctly""" - problem = self.build_problem(question_text="What 5 * 10?", - explanation="The answer is 50", - answer="5e+1") # Answer doesn't matter + """See if division by zero is handled correctly.""" + problem = self.build_problem(answer="1") # Answer doesn't matter input_dict = {'1_2_1': '1/0'} - self.assertRaises(StudentInputError, problem.grade_answers, input_dict) + with self.assertRaises(StudentInputError): + problem.grade_answers(input_dict) + + def test_staff_inputs_expressions(self): + """Test that staff may enter in an expression as the answer.""" + problem = self.build_problem(answer="1/3", tolerance=1e-3) + correct_responses = ["1/3", "0.333333"] + incorrect_responses = [] + self.assert_multiple_grade(problem, correct_responses, incorrect_responses) + + def test_staff_inputs_expressions_legacy(self): + """Test that staff may enter in a complex number as the answer.""" + problem = self.build_problem(answer="1+1j", tolerance=1e-3) + self.assert_grade(problem, '1+j', 'correct') + + @mock.patch('capa.responsetypes.log') + def test_staff_inputs_bad_syntax(self, mock_log): + """Test that staff may enter in a complex number as the answer.""" + staff_ans = "clearly bad syntax )[+1e" + problem = self.build_problem(answer=staff_ans, tolerance=1e-3) + + msg = "There was a problem with the staff answer to this problem" + with self.assertRaisesRegexp(StudentInputError, msg): + self.assert_grade(problem, '1+j', 'correct') + + mock_log.debug.assert_called_once_with( + "Content error--answer '%s' is not a valid number", staff_ans + ) + + def test_grade_infinity(self): + """ + Check that infinity doesn't automatically get marked correct. + + This resolves a bug where a problem with relative tolerance would + pass with any arbitrarily large student answer. + """ + mapping = { + 'some big input': float('inf'), + 'some neg input': -float('inf'), + 'weird NaN input': float('nan'), + '4': 4 + } + + def evaluator_side_effect(_, __, math_string): + """Look up the given response for `math_string`.""" + return mapping[math_string] + + problem = self.build_problem(answer=4, tolerance='10%') + + with mock.patch('capa.responsetypes.evaluator') as mock_eval: + mock_eval.side_effect = evaluator_side_effect + self.assert_grade(problem, 'some big input', 'incorrect') + self.assert_grade(problem, 'some neg input', 'incorrect') + self.assert_grade(problem, 'weird NaN input', 'incorrect') + + def test_err_handling(self): + """ + See that `StudentInputError`s are raised when things go wrong. + """ + problem = self.build_problem(answer=4) + + errors = [ # (exception raised, message to student) + (calc.UndefinedVariable("x"), r"You may not use variables \(x\) in numerical problems"), + (ValueError("factorial() mess-up"), "factorial function evaluated outside its domain"), + (ValueError(), "Could not interpret '.*' as a number"), + (pyparsing.ParseException("oopsie"), "Invalid math syntax"), + (ZeroDivisionError(), "Could not interpret '.*' as a number") + ] + + with mock.patch('capa.responsetypes.evaluator') as mock_eval: + for err, msg_regex in errors: + + def evaluator_side_effect(_, __, math_string): + """Raise an error only for the student input.""" + if math_string != '4': + raise err + mock_eval.side_effect = evaluator_side_effect + + with self.assertRaisesRegexp(StudentInputError, msg_regex): + problem.grade_answers({'1_2_1': 'foobar'}) class CustomResponseTest(ResponseTest): diff --git a/common/lib/xmodule/xmodule/css/capa/display.scss b/common/lib/xmodule/xmodule/css/capa/display.scss index 9d35e8cd40..2d8fb765d9 100644 --- a/common/lib/xmodule/xmodule/css/capa/display.scss +++ b/common/lib/xmodule/xmodule/css/capa/display.scss @@ -577,7 +577,7 @@ section.problem { section.action { margin-top: 20px; - .save, .check, .show { + .save, .check, .show, .reset { height: ($baseline*2); font-weight: 600; vertical-align: middle; diff --git a/common/lib/xmodule/xmodule/modulestore/django.py b/common/lib/xmodule/xmodule/modulestore/django.py index f1235668dc..adafbc1253 100644 --- a/common/lib/xmodule/xmodule/modulestore/django.py +++ b/common/lib/xmodule/xmodule/modulestore/django.py @@ -8,6 +8,7 @@ from __future__ import absolute_import from importlib import import_module from django.conf import settings +from xmodule.modulestore.loc_mapper_store import LocMapperStore _MODULESTORES = {} @@ -55,6 +56,21 @@ def modulestore(name='default'): return _MODULESTORES[name] +_loc_singleton = None +def loc_mapper(): + """ + Get the loc mapper which bidirectionally maps Locations to Locators. Used like modulestore() as + a singleton accessor. + """ + # pylint: disable=W0603 + global _loc_singleton + # pylint: disable=W0212 + if _loc_singleton is None: + # instantiate + _loc_singleton = LocMapperStore(settings.modulestore_options) + return _loc_singleton + + def clear_existing_modulestores(): """ Clear the existing modulestore instances, causing @@ -93,3 +109,4 @@ def editable_modulestore(name='default'): else: return None + diff --git a/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py b/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py new file mode 100644 index 0000000000..5967ee3d9d --- /dev/null +++ b/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py @@ -0,0 +1,382 @@ +''' +Method for converting among our differing Location/Locator whatever reprs +''' +from random import randint +import re +import pymongo + +from xmodule.modulestore.exceptions import InvalidLocationError, ItemNotFoundError, DuplicateItemError +from xmodule.modulestore.locator import BlockUsageLocator +from xmodule.modulestore.mongo import draft +from xmodule.modulestore import Location + + +class LocMapperStore(object): + ''' + This store persists mappings among the addressing schemes. At this time, it's between the old i4x Location + tuples and the split mongo Course and Block Locator schemes. + + edX has used several different addressing schemes. The original ones were organically created based on + immediate needs and were overly restrictive esp wrt course ids. These were slightly extended to support + some types of blocks may need to have draft states during editing to keep live courses from seeing the wip. + A later refactoring generalized course ids to enable governance and more complex naming, branch naming with + anything able to be in any branch. + + The expectation is that the configuration will have this use the same store as whatever is the default + or dominant store, but that's not a requirement. This store creates its own connection. + ''' + + # C0103: varnames and attrs must be >= 3 chars, but db defined by long time usage + # pylint: disable = C0103 + def __init__(self, host, db, collection, port=27017, user=None, password=None, **kwargs): + ''' + Constructor + ''' + self.db = pymongo.database.Database( + pymongo.MongoClient( + host=host, + port=port, + tz_aware=True, + **kwargs + ), + db + ) + if user is not None and password is not None: + self.db.authenticate(user, password) + + self.location_map = self.db[collection + '.location_map'] + self.location_map.write_concern = {'w': 1} + + # location_map functions + def create_map_entry(self, course_location, course_id=None, draft_branch='draft', prod_branch='published', + block_map=None): + """ + Add a new entry to map this course_location to the new style CourseLocator.course_id. If course_id is not + provided, it creates the default map of using org.course.name from the location (just like course_id) if + the location.cateogry = 'course'; otherwise, it uses org.course. + + You can create more than one mapping to the + same course_id target. In that case, the reverse translate will be arbitrary (no guarantee of which wins). + The use + case for more than one mapping is to map both org/course/run and org/course to the same new course_id thus + making a default for org/course. When querying for just org/course, the translator will prefer any entry + which does not have a name in the _id; otherwise, it will return an arbitrary match. + + Note: the opposite is not true. That is, it never makes sense to use 2 different CourseLocator.course_id + keys to index the same old Locator org/course/.. pattern. There's no checking to ensure you don't do this. + + NOTE: if there's already an entry w the given course_location, this may either overwrite that entry or + throw an error depending on how mongo is configured. + + :param course_location: a Location preferably whose category is 'course'. Unlike the other + map methods, this one doesn't take the old-style course_id. It should be called with + a course location not a block location; however, if called w/ a non-course Location, it creates + a "default" map for the org/course pair to a new course_id. + :param course_id: the CourseLocator style course_id + :param draft_branch: the branch name to assign for drafts. This is hardcoded because old mongo had + a fixed notion that there was 2 and only 2 versions for modules: draft and production. The old mongo + did not, however, require that a draft version exist. The new one, however, does require a draft to + exist. + :param prod_branch: the branch name to assign for the production (live) copy. In old mongo, every course + had to have a production version (whereas new split mongo does not require that until the author's ready + to publish). + :param block_map: an optional map to specify preferred names for blocks where the keys are the + Location block names and the values are the BlockUsageLocator.block_id. + """ + if course_id is None: + if course_location.category == 'course': + course_id = "{0.org}.{0.course}.{0.name}".format(course_location) + else: + course_id = "{0.org}.{0.course}".format(course_location) + # very like _interpret_location_id but w/o the _id + location_id = {'org': course_location.org, 'course': course_location.course} + if course_location.category == 'course': + location_id['name'] = course_location.name + + self.location_map.insert({ + '_id': location_id, + 'course_id': course_id, + 'draft_branch': draft_branch, + 'prod_branch': prod_branch, + 'block_map': block_map or {}, + }) + + def translate_location(self, old_style_course_id, location, published=True, add_entry_if_missing=True): + """ + Translate the given module location to a Locator. If the mapping has the run id in it, then you + should provide old_style_course_id with that run id in it to disambiguate the mapping if there exists more + than one entry in the mapping table for the org.course. + + The rationale for auto adding entries was that there should be a reasonable default translation + if the code just trips into this w/o creating translations. The downfall is that ambiguous course + locations may generate conflicting block_ids. + + Will raise ItemNotFoundError if there's no mapping and add_entry_if_missing is False. + + :param old_style_course_id: the course_id used in old mongo not the new one (optional, will use location) + :param location: a Location pointing to a module + :param published: a boolean to indicate whether the caller wants the draft or published branch. + :param add_entry_if_missing: a boolean as to whether to raise ItemNotFoundError or to create an entry if + the course + or block is not found in the map. + + NOTE: unlike old mongo, draft branches contain the whole course; so, it applies to all category + of locations including course. + """ + location_id = self._interpret_location_course_id(old_style_course_id, location) + + maps = self.location_map.find(location_id).sort('_id.name', pymongo.ASCENDING) + if maps.count() == 0: + if add_entry_if_missing: + # create a new map + course_location = location.replace(category='course', name=location_id['_id.name']) + self.create_map_entry(course_location) + entry = self.location_map.find_one(location_id) + else: + raise ItemNotFoundError() + elif maps.count() > 1: + # if more than one, prefer the one w/o a name if that exists. Otherwise, choose the first (alphabetically) + entry = maps[0] + else: + entry = maps[0] + + if published: + branch = entry['prod_branch'] + else: + branch = entry['draft_branch'] + + usage_id = entry['block_map'].get(location.name) + if usage_id is None: + if add_entry_if_missing: + usage_id = self._add_to_block_map(location, location_id, entry['block_map']) + else: + raise ItemNotFoundError() + elif isinstance(usage_id, dict): + # name is not unique, look through for the right category + if location.category in usage_id: + usage_id = usage_id[location.category] + elif add_entry_if_missing: + usage_id = self._add_to_block_map(location, location_id, entry['block_map']) + else: + raise ItemNotFoundError() + else: + raise InvalidLocationError() + + return BlockUsageLocator(course_id=entry['course_id'], branch=branch, usage_id=usage_id) + + def translate_locator_to_location(self, locator): + """ + Returns an old style Location for the given Locator if there's an appropriate entry in the + mapping collection. Note, it requires that the course was previously mapped (a side effect of + translate_location or explicitly via create_map_entry) and + the block's usage_id was previously stored in the + map (a side effect of translate_location or via add|update_block_location). + + If there are no matches, it returns None. + + If there's more than one location to locator mapping to the same course_id, it looks for the first + one with a mapping for the block usage_id and picks that arbitrary course location. + + :param locator: a BlockUsageLocator + """ + # This does not require that the course exist in any modulestore + # only that it has a mapping entry. + maps = self.location_map.find({'course_id': locator.course_id}) + # look for one which maps to this block usage_id + if maps.count() == 0: + return None + for candidate in maps: + for old_name, cat_to_usage in candidate['block_map'].iteritems(): + for category, usage_id in cat_to_usage.iteritems(): + if usage_id == locator.usage_id: + # figure out revision + # enforce the draft only if category in [..] logic + if category in draft.DIRECT_ONLY_CATEGORIES: + revision = None + elif locator.branch == candidate['draft_branch']: + revision = draft.DRAFT + else: + revision = None + return Location( + 'i4x', + candidate['_id']['org'], + candidate['_id']['course'], + category, + old_name, + revision) + return None + + def add_block_location_translator(self, location, old_course_id=None, usage_id=None): + """ + Similar to translate_location which adds an entry if none is found, but this cannot create a new + course mapping entry, only a block within such a mapping entry. If it finds no existing + course maps, it raises ItemNotFoundError. + + In the case that there are more than one mapping record for the course identified by location, this + method adds the mapping to all matching records! (translate_location only adds to one) + + It allows the caller to specify + the new-style usage_id for the target rather than having the translate concoct its own. + If the provided usage_id already exists in one of the found maps for the org/course, this function + raises DuplicateItemError unless the old item id == the new one. + + If the caller does not provide a usage_id and there exists an entry in one of the course variants, + it will use that entry. If more than one variant uses conflicting entries, it will raise DuplicateItemError. + + Returns the usage_id used in the mapping + + :param location: a fully specified Location + :param old_course_id: the old-style org/course or org/course/run string (optional) + :param usage_id: the desired new block_id. If left as None, this will generate one as per translate_location + """ + location_id = self._interpret_location_course_id(old_course_id, location) + + maps = self.location_map.find(location_id) + if maps.count() == 0: + raise ItemNotFoundError() + + # turn maps from cursor to list + map_list = list(maps) + # check whether there's already a usage_id for this location (and it agrees w/ any passed in or found) + for map_entry in map_list: + if (location.name in map_entry['block_map'] and + location.category in map_entry['block_map'][location.name]): + if usage_id is None: + usage_id = map_entry['block_map'][location.name][location.category] + elif usage_id != map_entry['block_map'][location.name][location.category]: + raise DuplicateItemError() + + computed_usage_id = usage_id + + # update the maps (and generate a usage_id if it's not been set yet) + for map_entry in map_list: + if computed_usage_id is None: + computed_usage_id = self._add_to_block_map(location, location_id, map_entry['block_map']) + elif (location.name not in map_entry['block_map'] or + location.category not in map_entry['block_map'][location.name]): + alt_usage_id = self._verify_uniqueness(computed_usage_id, map_entry['block_map']) + if alt_usage_id != computed_usage_id: + if usage_id is not None: + raise DuplicateItemError() + else: + # revise already set ones and add to remaining ones + computed_usage_id = self.update_block_location_translator( + location, + alt_usage_id, + old_course_id, + True + ) + + map_entry['block_map'].setdefault(location.name, {})[location.category] = computed_usage_id + self.location_map.update({'_id': map_entry['_id']}, {'$set': {'block_map': map_entry['block_map']}}) + + return computed_usage_id + + def update_block_location_translator(self, location, usage_id, old_course_id=None, autogenerated_usage_id=False): + """ + Update all existing maps from location's block to the new usage_id. Used for changing the usage_id, + thus the usage_id is required. + + Returns the usage_id. (which is primarily useful in the case of autogenerated_usage_id) + + :param location: a fully specified Location + :param usage_id: the desired new block_id. + :param old_course_id: the old-style org/course or org/course/run string (optional) + :param autogenerated_usage_id: a flag used mostly for internal calls to indicate that this usage_id + was autogenerated and thus can be overridden if it's not unique. If you set this flag, the stored + usage_id may not be the one you submitted. + """ + location_id = self._interpret_location_course_id(old_course_id, location) + + maps = self.location_map.find(location_id) + for map_entry in maps: + # handle noop of renaming to same name + if (location.name in map_entry['block_map'] and + map_entry['block_map'][location.name].get(location.category) == usage_id): + continue + alt_usage_id = self._verify_uniqueness(usage_id, map_entry['block_map']) + if alt_usage_id != usage_id: + if autogenerated_usage_id: + # revise already set ones and add to remaining ones + usage_id = self.update_block_location_translator(location, alt_usage_id, old_course_id, True) + return usage_id + else: + raise DuplicateItemError() + + if location.category in map_entry['block_map'].setdefault(location.name, {}): + map_entry['block_map'][location.name][location.category] = usage_id + self.location_map.update({'_id': map_entry['_id']}, {'$set': {'block_map': map_entry['block_map']}}) + + return usage_id + + def delete_block_location_translator(self, location, old_course_id=None): + """ + Remove all existing maps from location's block. + + :param location: a fully specified Location + :param old_course_id: the old-style org/course or org/course/run string (optional) + """ + location_id = self._interpret_location_course_id(old_course_id, location) + + maps = self.location_map.find(location_id) + for map_entry in maps: + if location.category in map_entry['block_map'].setdefault(location.name, {}): + if len(map_entry['block_map'][location.name]) == 1: + del map_entry['block_map'][location.name] + else: + del map_entry['block_map'][location.name][location.category] + self.location_map.update({'_id': map_entry['_id']}, {'$set': {'block_map': map_entry['block_map']}}) + + def _add_to_block_map(self, location, location_id, block_map): + '''add the given location to the block_map and persist it''' + if self._block_id_is_guid(location.name): + # This makes the ids more meaningful with a small probability of name collision. + # The downside is that if there's more than one course mapped to from the same org/course root + # the block ids will likely be out of sync and collide from an id perspective. HOWEVER, + # if there are few == org/course roots or their content is unrelated, this will work well. + usage_id = self._verify_uniqueness(location.category + location.name[:3], block_map) + block_map.setdefault(location.name, {})[location.category] = usage_id + self.location_map.update(location_id, {'$set': {'block_map': block_map}}) + return usage_id + + def _interpret_location_course_id(self, course_id, location): + """ + Take the old style course id (org/course/run) and return a dict for querying the mapping table. + If the course_id is empty, it uses location, but this may result in an inadequate id. + + :param course_id: old style 'org/course/run' id from Location.course_id where Location.category = 'course' + :param location: a Location object which may be to a module or a course. Provides partial info + if course_id is omitted. + """ + if course_id: + # re doesn't allow ?P<_id.org> and ilk + matched = re.match(r'([^/]+)/([^/]+)/([^/]+)', course_id) + return dict(zip(['_id.org', '_id.course', '_id.name'], matched.groups())) + + location_id = {'_id.org': location.org, '_id.course': location.course} + if location.category == 'course': + location_id['_id.name'] = location.name + return location_id + + def _block_id_is_guid(self, name): + """ + Does the given name look like it's a guid? + """ + return len(name) == 32 and re.search(r'[^0-9A-Fa-f]', name) is None + + def _verify_uniqueness(self, name, block_map): + ''' + Verify that the name doesn't occur elsewhere in block_map. If it does, keep adding to it until + it's unique. + ''' + for targets in block_map.itervalues(): + if isinstance(targets, dict): + for values in targets.itervalues(): + if values == name: + name += str(randint(0, 9)) + return self._verify_uniqueness(name, block_map) + + elif targets == name: + name += str(randint(0, 9)) + return self._verify_uniqueness(name, block_map) + return name diff --git a/common/lib/xmodule/xmodule/modulestore/mixed.py b/common/lib/xmodule/xmodule/modulestore/mixed.py index 9548323115..cc4ba7a699 100644 --- a/common/lib/xmodule/xmodule/modulestore/mixed.py +++ b/common/lib/xmodule/xmodule/modulestore/mixed.py @@ -7,7 +7,7 @@ IMPORTANT: This modulestore only supports READONLY applications, e.g. LMS """ from . import ModuleStoreBase -from django import create_modulestore_instance +from xmodule.modulestore.django import create_modulestore_instance import logging log = logging.getLogger(__name__) diff --git a/common/lib/xmodule/xmodule/modulestore/parsers.py b/common/lib/xmodule/xmodule/modulestore/parsers.py index 9308894b86..4996cc4e8a 100644 --- a/common/lib/xmodule/xmodule/modulestore/parsers.py +++ b/common/lib/xmodule/xmodule/modulestore/parsers.py @@ -19,10 +19,10 @@ def parse_url(string): Examples: 'edx://version/0123FFFF' - 'edx://edu.mit.eecs.6002x' - 'edx://edu.mit.eecs.6002x/branch/published' - 'edx://edu.mit.eecs.6002x/branch/published/version/519665f6223ebd6980884f2b/block/HW3' - 'edx://edu.mit.eecs.6002x/branch/published/block/HW3' + 'edx://mit.eecs.6002x' + 'edx://mit.eecs.6002x;published' + 'edx://mit.eecs.6002x;published/block/HW3' + 'edx://mit.eecs.6002x;published/version/000eee12345/block/HW3' This returns None if string cannot be parsed. @@ -97,11 +97,11 @@ def parse_course_id(string): Examples of valid course_ids: - 'edu.mit.eecs.6002x' - 'edu.mit.eecs.6002x/branch/published' - 'edu.mit.eecs.6002x/block/HW3' - 'edu.mit.eecs.6002x/branch/published/block/HW3' - 'edu.mit.eecs.6002x/branch/published/version/519665f6223ebd6980884f2b/block/HW3' + 'mit.eecs.6002x' + 'mit.eecs.6002x/branch/published' + 'mit.eecs.6002x/block/HW3' + 'mit.eecs.6002x/branch/published/block/HW3' + 'mit.eecs.6002x/branch/published/version/519665f6223ebd6980884f2b/block/HW3' Syntax: diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index 7038cdf865..7e151a6649 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py @@ -10,9 +10,8 @@ from xmodule.errortracker import null_error_tracker from xmodule.x_module import XModuleDescriptor from xmodule.modulestore.locator import BlockUsageLocator, DescriptionLocator, CourseLocator, VersionTree from xmodule.modulestore.exceptions import InsufficientSpecificationError, VersionConflictError -from xmodule.modulestore import inheritance +from xmodule.modulestore import inheritance, ModuleStoreBase -from .. import ModuleStoreBase from ..exceptions import ItemNotFoundError from .definition_lazy_loader import DefinitionLazyLoader from .caching_descriptor_system import CachingDescriptorSystem @@ -62,14 +61,11 @@ class SplitMongoModuleStore(ModuleStoreBase): **kwargs ), db) - # TODO add caching of structures to thread_cache to prevent repeated fetches (but not index b/c - # it changes w/o having a change in id) self.course_index = self.db[collection + '.active_versions'] self.structures = self.db[collection + '.structures'] self.definitions = self.db[collection + '.definitions'] - # ??? Code review question: those familiar w/ python threading. Should I instead - # use django cache? How should I expire entries? + # Code review question: How should I expire entries? # _add_cache could use a lru mechanism to control the cache size? self.thread_cache = threading.local() @@ -1178,6 +1174,7 @@ class SplitMongoModuleStore(ModuleStoreBase): else: return DescriptionLocator(definition['_id']) + def _block_matches(self, value, qualifiers): ''' Return True or False depending on whether the value (block contents) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_location_mapper.py b/common/lib/xmodule/xmodule/modulestore/tests/test_location_mapper.py new file mode 100644 index 0000000000..599bd6da5e --- /dev/null +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_location_mapper.py @@ -0,0 +1,507 @@ +''' +Created on Aug 5, 2013 + +@author: dmitchell +''' +import unittest +import uuid +from xmodule.modulestore import Location +from xmodule.modulestore.locator import BlockUsageLocator +from xmodule.modulestore.exceptions import ItemNotFoundError, DuplicateItemError +from xmodule.modulestore.loc_mapper_store import LocMapperStore + + +class TestLocationMapper(unittest.TestCase): + """ + Test the location to locator mapper + """ + + def setUp(self): + modulestore_options = { + 'host': 'localhost', + 'db': 'test_xmodule', + 'collection': 'modulestore{0}'.format(uuid.uuid4().hex), + } + + # pylint: disable=W0142 + TestLocationMapper.loc_store = LocMapperStore(**modulestore_options) + + def tearDown(self): + dbref = TestLocationMapper.loc_store.db + dbref.drop_collection(TestLocationMapper.loc_store.location_map) + dbref.connection.close() + TestLocationMapper.loc_store = None + + def test_create_map(self): + org = 'foo_org' + course = 'bar_course' + loc_mapper().create_map_entry(Location('i4x', org, course, 'course', 'baz_run')) + entry = loc_mapper().location_map.find_one({ + '_id': {'org': org, 'course': course, 'name': 'baz_run'} + }) + self.assertIsNotNone(entry, "Didn't find entry") + self.assertEqual(entry['course_id'], '{}.{}.baz_run'.format(org, course)) + self.assertEqual(entry['draft_branch'], 'draft') + self.assertEqual(entry['prod_branch'], 'published') + self.assertEqual(entry['block_map'], {}) + + # ensure create_entry does the right thing when not given a course (creates org/course + # rather than org/course/run course_id) + loc_mapper().create_map_entry(Location('i4x', org, course, 'vertical', 'baz_vert')) + entry = loc_mapper().location_map.find_one({ + '_id': {'org': org, 'course': course} + }) + self.assertIsNotNone(entry, "Didn't find entry") + self.assertEqual(entry['course_id'], '{}.{}'.format(org, course)) + + course = 'quux_course' + # oldname: {category: newname} + block_map = {'abc123': {'problem': 'problem2'}} + loc_mapper().create_map_entry( + Location('i4x', org, course, 'problem', 'abc123', 'draft'), + 'foo_org.geek_dept.quux_course.baz_run', + 'wip', + 'live', + block_map) + entry = loc_mapper().location_map.find_one({ + '_id': {'org': org, 'course': course} + }) + self.assertIsNotNone(entry, "Didn't find entry") + self.assertEqual(entry['course_id'], 'foo_org.geek_dept.quux_course.baz_run') + self.assertEqual(entry['draft_branch'], 'wip') + self.assertEqual(entry['prod_branch'], 'live') + self.assertEqual(entry['block_map'], block_map) + + def test_translate_location_read_only(self): + """ + Test the variants of translate_location which don't create entries, just decode + """ + # lookup before there are any maps + org = 'foo_org' + course = 'bar_course' + old_style_course_id = '{}/{}/{}'.format(org, course, 'baz_run') + with self.assertRaises(ItemNotFoundError): + _ = loc_mapper().translate_location( + old_style_course_id, + Location('i4x', org, course, 'problem', 'abc123'), + add_entry_if_missing=False + ) + + new_style_course_id = '{}.geek_dept.{}.baz_run'.format(org, course) + block_map = {'abc123': {'problem': 'problem2'}} + loc_mapper().create_map_entry( + Location('i4x', org, course, 'course', 'baz_run'), + new_style_course_id, + block_map=block_map + ) + # only one course matches + prob_locator = loc_mapper().translate_location( + old_style_course_id, + Location('i4x', org, course, 'problem', 'abc123'), + add_entry_if_missing=False + ) + self.assertEqual(prob_locator.course_id, new_style_course_id) + self.assertEqual(prob_locator.branch, 'published') + self.assertEqual(prob_locator.usage_id, 'problem2') + # look for w/ only the Location (works b/c there's only one possible course match) + prob_locator = loc_mapper().translate_location( + None, + Location('i4x', org, course, 'problem', 'abc123'), + add_entry_if_missing=False + ) + self.assertEqual(prob_locator.course_id, new_style_course_id) + # look for non-existent problem + with self.assertRaises(ItemNotFoundError): + prob_locator = loc_mapper().translate_location( + None, + Location('i4x', org, course, 'problem', '1def23'), + add_entry_if_missing=False + ) + + # add a distractor course + block_map = {'abc123': {'problem': 'problem3'}} + loc_mapper().create_map_entry( + Location('i4x', org, course, 'course', 'delta_run'), + '{}.geek_dept.{}.{}'.format(org, course, 'delta_run'), + block_map=block_map + ) + prob_locator = loc_mapper().translate_location( + old_style_course_id, + Location('i4x', org, course, 'problem', 'abc123'), + add_entry_if_missing=False + ) + self.assertEqual(prob_locator.course_id, new_style_course_id) + self.assertEqual(prob_locator.usage_id, 'problem2') + # look for w/ only the Location (not unique; so, just verify it returns something) + prob_locator = loc_mapper().translate_location( + None, + Location('i4x', org, course, 'problem', 'abc123'), + add_entry_if_missing=False + ) + self.assertIsNotNone(prob_locator, "couldn't find ambiguous location") + + # add a default course pointing to the delta_run + loc_mapper().create_map_entry( + Location('i4x', org, course, 'problem', '789abc123efg456'), + '{}.geek_dept.{}.{}'.format(org, course, 'delta_run'), + block_map=block_map + ) + # now the ambiguous query should return delta + prob_locator = loc_mapper().translate_location( + None, + Location('i4x', org, course, 'problem', 'abc123'), + add_entry_if_missing=False + ) + self.assertEqual(prob_locator.course_id, '{}.geek_dept.{}.{}'.format(org, course, 'delta_run')) + self.assertEqual(prob_locator.usage_id, 'problem3') + + # get the draft one (I'm sorry this is getting long) + prob_locator = loc_mapper().translate_location( + None, + Location('i4x', org, course, 'problem', 'abc123'), + published=False, + add_entry_if_missing=False + ) + self.assertEqual(prob_locator.course_id, '{}.geek_dept.{}.{}'.format(org, course, 'delta_run')) + self.assertEqual(prob_locator.usage_id, 'problem3') + self.assertEqual(prob_locator.branch, 'draft') + + def test_translate_location_dwim(self): + """ + Test the location translation mechanisms which try to do-what-i-mean by creating new + entries for never seen queries. + """ + org = 'foo_org' + course = 'bar_course' + old_style_course_id = '{}/{}/{}'.format(org, course, 'baz_run') + problem_name = 'abc123abc123abc123abc123abc123f9' + location = Location('i4x', org, course, 'problem', problem_name) + prob_locator = loc_mapper().translate_location( + old_style_course_id, + location, + add_entry_if_missing=True + ) + new_style_course_id = '{}.{}.{}'.format(org, course, 'baz_run') + self.assertEqual(prob_locator.course_id, new_style_course_id) + self.assertEqual(prob_locator.branch, 'published') + self.assertEqual(prob_locator.usage_id, 'problemabc') + # look for w/ only the Location (works b/c there's only one possible course match) + prob_locator = loc_mapper().translate_location( + None, + location, + add_entry_if_missing=True + ) + self.assertEqual(prob_locator.course_id, new_style_course_id) + + # add a distractor course + loc_mapper().create_map_entry( + Location('i4x', org, course, 'course', 'delta_run'), + '{}.geek_dept.{}.{}'.format(org, course, 'delta_run'), + block_map={problem_name: {'problem': 'problem3'}} + ) + prob_locator = loc_mapper().translate_location( + old_style_course_id, + location, + add_entry_if_missing=True + ) + self.assertEqual(prob_locator.course_id, new_style_course_id) + self.assertEqual(prob_locator.usage_id, 'problemabc') + # look for w/ only the Location (not unique; so, just verify it returns something) + prob_locator = loc_mapper().translate_location( + None, + location, + add_entry_if_missing=True + ) + self.assertIsNotNone(prob_locator, "couldn't find ambiguous location") + + # add a default course pointing to the delta_run + loc_mapper().create_map_entry( + Location('i4x', org, course, 'problem', '789abc123efg456'), + '{}.geek_dept.{}.{}'.format(org, course, 'delta_run'), + block_map={problem_name: {'problem': 'problem3'}} + ) + # now the ambiguous query should return delta + prob_locator = loc_mapper().translate_location( + None, + location, + add_entry_if_missing=False + ) + self.assertEqual(prob_locator.course_id, '{}.geek_dept.{}.{}'.format(org, course, 'delta_run')) + self.assertEqual(prob_locator.usage_id, 'problem3') + + def test_translate_locator(self): + """ + tests translate_locator_to_location(BlockUsageLocator) + """ + # lookup for non-existent course + org = 'foo_org' + course = 'bar_course' + new_style_course_id = '{}.geek_dept.{}.baz_run'.format(org, course) + prob_locator = BlockUsageLocator( + course_id=new_style_course_id, + usage_id='problem2' + ) + prob_location = loc_mapper().translate_locator_to_location(prob_locator) + self.assertIsNone(prob_location, 'found entry in empty map table') + + loc_mapper().create_map_entry( + Location('i4x', org, course, 'course', 'baz_run'), + new_style_course_id, + block_map={ + 'abc123': {'problem': 'problem2'}, + '48f23a10395384929234': {'chapter': 'chapter48f'} + } + ) + # only one course matches + prob_location = loc_mapper().translate_locator_to_location(prob_locator) + # default branch + self.assertEqual(prob_location, Location('i4x', org, course, 'problem', 'abc123', None)) + # explicit branch + prob_locator = BlockUsageLocator(prob_locator, branch='draft') + prob_location = loc_mapper().translate_locator_to_location(prob_locator) + self.assertEqual(prob_location, Location('i4x', org, course, 'problem', 'abc123', 'draft')) + prob_locator = BlockUsageLocator( + course_id=new_style_course_id, usage_id='problem2', branch='production' + ) + prob_location = loc_mapper().translate_locator_to_location(prob_locator) + self.assertEqual(prob_location, Location('i4x', org, course, 'problem', 'abc123', None)) + # same for chapter except chapter cannot be draft in old system + chap_locator = BlockUsageLocator( + course_id=new_style_course_id, + usage_id='chapter48f' + ) + chap_location = loc_mapper().translate_locator_to_location(chap_locator) + self.assertEqual(chap_location, Location('i4x', org, course, 'chapter', '48f23a10395384929234')) + # explicit branch + chap_locator = BlockUsageLocator(chap_locator, branch='draft') + chap_location = loc_mapper().translate_locator_to_location(chap_locator) + self.assertEqual(chap_location, Location('i4x', org, course, 'chapter', '48f23a10395384929234')) + chap_locator = BlockUsageLocator( + course_id=new_style_course_id, usage_id='chapter48f', branch='production' + ) + chap_location = loc_mapper().translate_locator_to_location(chap_locator) + self.assertEqual(chap_location, Location('i4x', org, course, 'chapter', '48f23a10395384929234')) + + # look for non-existent problem + prob_locator2 = BlockUsageLocator( + course_id=new_style_course_id, + branch='draft', + usage_id='problem3' + ) + prob_location = loc_mapper().translate_locator_to_location(prob_locator2) + self.assertIsNone(prob_location, 'Found non-existent problem') + + # add a distractor course + new_style_course_id = '{}.geek_dept.{}.{}'.format(org, course, 'delta_run') + loc_mapper().create_map_entry( + Location('i4x', org, course, 'course', 'delta_run'), + new_style_course_id, + block_map={'abc123': {'problem': 'problem3'}} + ) + prob_location = loc_mapper().translate_locator_to_location(prob_locator) + self.assertEqual(prob_location, Location('i4x', org, course, 'problem', 'abc123', None)) + + # add a default course pointing to the delta_run + loc_mapper().create_map_entry( + Location('i4x', org, course, 'problem', '789abc123efg456'), + new_style_course_id, + block_map={'abc123': {'problem': 'problem3'}} + ) + # now query delta (2 entries point to it) + prob_locator = BlockUsageLocator( + course_id=new_style_course_id, + branch='production', + usage_id='problem3' + ) + prob_location = loc_mapper().translate_locator_to_location(prob_locator) + self.assertEqual(prob_location, Location('i4x', org, course, 'problem', 'abc123')) + + def test_add_block(self): + """ + Test add_block_location_translator(location, old_course_id=None, usage_id=None) + """ + # call w/ no matching courses + org = 'foo_org' + course = 'bar_course' + old_style_course_id = '{}/{}/{}'.format(org, course, 'baz_run') + problem_name = 'abc123abc123abc123abc123abc123f9' + location = Location('i4x', org, course, 'problem', problem_name) + with self.assertRaises(ItemNotFoundError): + loc_mapper().add_block_location_translator(location) + with self.assertRaises(ItemNotFoundError): + loc_mapper().add_block_location_translator(location, old_style_course_id) + + # w/ one matching course + new_style_course_id = '{}.{}.{}'.format(org, course, 'baz_run') + loc_mapper().create_map_entry( + Location('i4x', org, course, 'course', 'baz_run'), + new_style_course_id, + ) + new_usage_id = loc_mapper().add_block_location_translator(location) + self.assertEqual(new_usage_id, 'problemabc') + # look it up + translated_loc = loc_mapper().translate_location(old_style_course_id, location, add_entry_if_missing=False) + self.assertEqual(translated_loc.course_id, new_style_course_id) + self.assertEqual(translated_loc.usage_id, new_usage_id) + + # w/ one distractor which has one entry already + new_style_course_id = '{}.geek_dept.{}.{}'.format(org, course, 'delta_run') + loc_mapper().create_map_entry( + Location('i4x', org, course, 'course', 'delta_run'), + new_style_course_id, + block_map={'48f23a10395384929234': {'chapter': 'chapter48f'}} + ) + # try adding the one added before + new_usage_id2 = loc_mapper().add_block_location_translator(location) + self.assertEqual(new_usage_id, new_usage_id2) + # it should be in the distractor now + new_location = loc_mapper().translate_locator_to_location( + BlockUsageLocator(course_id=new_style_course_id, usage_id=new_usage_id2) + ) + self.assertEqual(new_location, location) + # add one close to the existing chapter (cause name collision) + location = Location('i4x', org, course, 'chapter', '48f23a103953849292341234567890ab') + new_usage_id = loc_mapper().add_block_location_translator(location) + self.assertRegexpMatches(new_usage_id, r'^chapter48f\d') + # retrievable from both courses + new_location = loc_mapper().translate_locator_to_location( + BlockUsageLocator(course_id=new_style_course_id, usage_id=new_usage_id) + ) + self.assertEqual(new_location, location) + new_location = loc_mapper().translate_locator_to_location( + BlockUsageLocator(course_id='{}.{}.{}'.format(org, course, 'baz_run'), usage_id=new_usage_id) + ) + self.assertEqual(new_location, location) + + # provoke duplicate item errors + location = location.replace(name='44f23a103953849292341234567890ab') + with self.assertRaises(DuplicateItemError): + loc_mapper().add_block_location_translator(location, usage_id=new_usage_id) + new_usage_id = loc_mapper().add_block_location_translator(location, old_course_id=old_style_course_id) + other_course_old_style = '{}/{}/{}'.format(org, course, 'delta_run') + new_usage_id2 = loc_mapper().add_block_location_translator( + location, + old_course_id=other_course_old_style, + usage_id='{}b'.format(new_usage_id) + ) + with self.assertRaises(DuplicateItemError): + loc_mapper().add_block_location_translator(location) + + def test_update_block(self): + """ + test update_block_location_translator(location, usage_id, old_course_id=None) + """ + org = 'foo_org' + course = 'bar_course' + new_style_course_id = '{}.geek_dept.{}.baz_run'.format(org, course) + loc_mapper().create_map_entry( + Location('i4x', org, course, 'course', 'baz_run'), + new_style_course_id, + block_map={ + 'abc123': {'problem': 'problem2'}, + '48f23a10395384929234': {'chapter': 'chapter48f'}, + '1': {'chapter': 'chapter1', 'problem': 'problem1'}, + } + ) + new_style_course_id2 = '{}.geek_dept.{}.delta_run'.format(org, course) + loc_mapper().create_map_entry( + Location('i4x', org, course, 'course', 'delta_run'), + new_style_course_id2, + block_map={ + 'abc123': {'problem': 'problem3'}, + '48f23a10395384929234': {'chapter': 'chapter48b'}, + '1': {'chapter': 'chapter2', 'problem': 'problem2'}, + } + ) + location = Location('i4x', org, course, 'problem', '1') + # change in all courses to same value + loc_mapper().update_block_location_translator(location, 'problem1') + trans_loc = loc_mapper().translate_locator_to_location( + BlockUsageLocator(course_id=new_style_course_id, usage_id='problem1') + ) + self.assertEqual(trans_loc, location) + trans_loc = loc_mapper().translate_locator_to_location( + BlockUsageLocator(course_id=new_style_course_id2, usage_id='problem1') + ) + self.assertEqual(trans_loc, location) + # try to change to overwrite used usage_id + location = Location('i4x', org, course, 'chapter', '48f23a10395384929234') + with self.assertRaises(DuplicateItemError): + loc_mapper().update_block_location_translator(location, 'chapter2') + # just change the one course + loc_mapper().update_block_location_translator(location, 'chapter2', '{}/{}/{}'.format(org, course, 'baz_run')) + trans_loc = loc_mapper().translate_locator_to_location( + BlockUsageLocator(course_id=new_style_course_id, usage_id='chapter2') + ) + self.assertEqual(trans_loc.name, '48f23a10395384929234') + # but this still points to the old + trans_loc = loc_mapper().translate_locator_to_location( + BlockUsageLocator(course_id=new_style_course_id2, usage_id='chapter2') + ) + self.assertEqual(trans_loc.name, '1') + + def test_delete_block(self): + """ + test delete_block_location_translator(location, old_course_id=None) + """ + org = 'foo_org' + course = 'bar_course' + new_style_course_id = '{}.geek_dept.{}.baz_run'.format(org, course) + loc_mapper().create_map_entry( + Location('i4x', org, course, 'course', 'baz_run'), + new_style_course_id, + block_map={ + 'abc123': {'problem': 'problem2'}, + '48f23a10395384929234': {'chapter': 'chapter48f'}, + '1': {'chapter': 'chapter1', 'problem': 'problem1'}, + } + ) + new_style_course_id2 = '{}.geek_dept.{}.delta_run'.format(org, course) + loc_mapper().create_map_entry( + Location('i4x', org, course, 'course', 'delta_run'), + new_style_course_id2, + block_map={ + 'abc123': {'problem': 'problem3'}, + '48f23a10395384929234': {'chapter': 'chapter48b'}, + '1': {'chapter': 'chapter2', 'problem': 'problem2'}, + } + ) + location = Location('i4x', org, course, 'problem', '1') + # delete from all courses + loc_mapper().delete_block_location_translator(location) + self.assertIsNone(loc_mapper().translate_locator_to_location( + BlockUsageLocator(course_id=new_style_course_id, usage_id='problem1') + )) + self.assertIsNone(loc_mapper().translate_locator_to_location( + BlockUsageLocator(course_id=new_style_course_id2, usage_id='problem2') + )) + # delete from one course + location = location.replace(name='abc123') + loc_mapper().delete_block_location_translator(location, '{}/{}/{}'.format(org, course, 'baz_run')) + with self.assertRaises(ItemNotFoundError): + loc_mapper().translate_location( + '{}/{}/{}'.format(org, course, 'baz_run'), + location, + add_entry_if_missing=False + ) + locator = loc_mapper().translate_location( + '{}/{}/{}'.format(org, course, 'delta_run'), + location, + add_entry_if_missing=False + ) + self.assertEqual(locator.usage_id, 'problem3') + + +#================================== +# functions to mock existing services +def loc_mapper(): + """ + Mocks the global location mapper. + """ + return TestLocationMapper.loc_store + + +def render_to_template_mock(*_args): + """ + Mocks the mako render_to_template w/ a noop + """ diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_locators.py b/common/lib/xmodule/xmodule/modulestore/tests/test_locators.py index 654de26db4..862b05cd53 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_locators.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_locators.py @@ -252,12 +252,18 @@ class LocatorTest(TestCase): def check_course_locn_fields(self, testobj, msg, version_guid=None, course_id=None, branch=None): + """ + Checks the version, course_id, and branch in testobj + """ self.assertEqual(testobj.version_guid, version_guid, msg) self.assertEqual(testobj.course_id, course_id, msg) self.assertEqual(testobj.branch, branch, msg) def check_block_locn_fields(self, testobj, msg, version_guid=None, course_id=None, branch=None, block=None): + """ + Does adds a block id check over and above the check_course_locn_fields tests + """ self.check_course_locn_fields(testobj, msg, version_guid, course_id, branch) self.assertEqual(testobj.usage_id, block) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py index e04ef9c363..da400e9f0a 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py @@ -13,7 +13,7 @@ HOST = 'localhost' PORT = 27017 DB = 'test_mongo_%s' % uuid4().hex COLLECTION = 'modulestore' -FS_ROOT = DATA_DIR # TODO (vshnayder): will need a real fs_root for testing load_item +FS_ROOT = DATA_DIR DEFAULT_CLASS = 'xmodule.raw_module.RawDescriptor' RENDER_TEMPLATE = lambda t_n, d, ctx = None, nsp = 'main': '' @@ -54,6 +54,9 @@ class TestMixedModuleStore(object): '''Tests!''' @classmethod def setupClass(cls): + """ + Set up the database for testing + """ cls.connection = pymongo.connection.Connection(HOST, PORT) cls.connection.drop_database(DB) cls.fake_location = Location(['i4x', 'foo', 'bar', 'vertical', 'baz']) @@ -66,10 +69,16 @@ class TestMixedModuleStore(object): @classmethod def teardownClass(cls): + """ + Clear out database after test has completed + """ cls.destroy_db(cls.connection) @staticmethod def initdb(): + """ + Initialize the database and import one test course into it + """ # connect to the db _options = {} _options.update(OPTIONS) @@ -92,7 +101,9 @@ class TestMixedModuleStore(object): @staticmethod def destroy_db(connection): - # Destroy the test db. + """ + Destroy the test db. + """ connection.drop_database(DB) def setUp(self): @@ -204,6 +215,7 @@ class TestMixedModuleStore(object): module = self.store.get_course(XML_COURSEID2) assert_equals(module.location.course, 'simple') + # pylint: disable=E1101 def test_get_parent_locations(self): parents = self.store.get_parent_locations( Location(['i4x', self.import_org, self.import_course, 'chapter', 'Overview']), @@ -223,6 +235,7 @@ class TestMixedModuleStore(object): assert_equals(Location(parents[0]).course, 'toy') assert_equals(Location(parents[0]).name, '2012_Fall') + # pylint: disable=W0212 def test_set_modulestore_configuration(self): config = {'foo': 'bar'} self.store.set_modulestore_configuration(config) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py index 58c48f673e..17036a16bf 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py @@ -120,8 +120,30 @@ class TestMongoModuleStore(object): '{0} is a template course'.format(course) ) + def test_static_tab_names(self): + courses = self.store.get_courses() + + def get_tab_name(index): + """ + Helper function for pulling out the name of a given static tab. + + Assumes the information is desired for courses[1] ('toy' course). + """ + return courses[1].tabs[index]['name'] + + # There was a bug where model.save was not getting called after the static tab name + # was set set for tabs that have a URL slug. 'Syllabus' and 'Resources' fall into that + # category, but for completeness, I'm also testing 'Course Info' and 'Discussion' (no url slug). + assert_equals('Course Info', get_tab_name(1)) + assert_equals('Syllabus', get_tab_name(2)) + assert_equals('Resources', get_tab_name(3)) + assert_equals('Discussion', get_tab_name(4)) + class TestMongoKeyValueStore(object): + """ + Tests for MongoKeyValueStore. + """ def setUp(self): self.data = {'foo': 'foo_value'} @@ -131,6 +153,9 @@ class TestMongoKeyValueStore(object): self.kvs = MongoKeyValueStore(self.data, self.children, self.metadata, self.location, 'category') def _check_read(self, key, expected_value): + """ + Asserts the get and has methods. + """ assert_equals(expected_value, self.kvs.get(key)) assert self.kvs.has(key) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py index 71361a6fce..d90c3020b4 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py @@ -992,8 +992,8 @@ class TestInheritance(SplitModuleTest): # This mocks the django.modulestore() function and is intended purely to disentangle # the tests from django def modulestore(): - def load_function(path): - module_path, _, name = path.rpartition('.') + def load_function(engine_path): + module_path, _, name = engine_path.rpartition('.') return getattr(import_module(module_path), name) if SplitModuleTest.modulestore is None: diff --git a/common/lib/xmodule/xmodule/modulestore/xml.py b/common/lib/xmodule/xmodule/modulestore/xml.py index 89c3299394..4c2011a3e5 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml.py +++ b/common/lib/xmodule/xmodule/modulestore/xml.py @@ -479,6 +479,7 @@ class XMLModuleStore(ModuleStoreBase): if tab.get('url_slug') == slug: module.display_name = tab['name'] module.data_dir = course_dir + module.save() self.modules[course_descriptor.id][module.location] = module except Exception, e: logging.exception("Failed to load {0}. Skipping... Exception: {1}".format(filepath, str(e))) diff --git a/common/lib/xmodule/xmodule/modulestore/xml_importer.py b/common/lib/xmodule/xmodule/modulestore/xml_importer.py index fa9228bed3..7bea0fdcac 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml_importer.py +++ b/common/lib/xmodule/xmodule/modulestore/xml_importer.py @@ -242,9 +242,37 @@ def import_course_draft(xml_module_store, store, draft_store, course_data_path, for dirname, dirnames, filenames in os.walk(draft_dir + "/vertical"): for filename in filenames: module_path = os.path.join(dirname, filename) - with open(module_path) as f: + with open(module_path, 'r') as f: try: - xml = f.read().decode('utf-8') + # note, on local dev it seems like OSX will put some extra files in + # the directory with "quarantine" information. These files are + # binary files and will throw exceptions when we try to parse + # the file as an XML string. Let's make sure we're + # dealing with a string before ingesting + data = f.read() + + try: + xml = data.decode('utf-8') + except UnicodeDecodeError, err: + # seems like on OSX localdev, the OS is making quarantine files + # in the unzip directory when importing courses + # so if we blindly try to enumerate through the directory, we'll try + # to process a bunch of binary quarantine files (which are prefixed with a '._' character + # which will dump a bunch of exceptions to the output, although they are harmless. + # + # Reading online docs there doesn't seem to be a good means to detect a 'hidden' + # file that works well across all OS environments. So for now, I'm using + # OSX's utilization of a leading '.' in the filename to indicate a system hidden + # file. + # + # Better yet would be a way to figure out if this is a binary file, but I + # haven't found a good way to do this yet. + # + if filename.startswith('._'): + continue + # Not a 'hidden file', then re-raise exception + raise err + descriptor = system.process_xml(xml) def _import_module(module): diff --git a/common/lib/xmodule/xmodule/open_ended_grading_classes/open_ended_module.py b/common/lib/xmodule/xmodule/open_ended_grading_classes/open_ended_module.py index 924ca2c23d..1bc8bddbc1 100644 --- a/common/lib/xmodule/xmodule/open_ended_grading_classes/open_ended_module.py +++ b/common/lib/xmodule/xmodule/open_ended_grading_classes/open_ended_module.py @@ -527,7 +527,7 @@ class OpenEndedModule(openendedchild.OpenEndedChild): feedback = "".join(feedback_items) else: feedback = feedback_items - score = int(median(score_result['score'])) + score = int(round(median(score_result['score']))) else: # This is for instructor and ML grading feedback, rubric_score = self._format_feedback(score_result, system) diff --git a/common/lib/xmodule/xmodule/tests/test_combined_open_ended.py b/common/lib/xmodule/xmodule/tests/test_combined_open_ended.py index 05e8df0ad8..035f88c046 100644 --- a/common/lib/xmodule/xmodule/tests/test_combined_open_ended.py +++ b/common/lib/xmodule/xmodule/tests/test_combined_open_ended.py @@ -291,6 +291,30 @@ class OpenEndedModuleTest(unittest.TestCase): 'xqueue_body': json.dumps(score_msg)} self.openendedmodule.update_score(get, self.test_system) + def update_score_multiple(self): + self.openendedmodule.new_history_entry("New Entry") + feedback = { + "success": True, + "feedback": "Grader Feedback" + } + score_msg = { + 'correct': True, + 'score': [0, 1], + 'msg': 'Grader Message', + 'feedback': [json.dumps(feedback), json.dumps(feedback)], + 'grader_type': 'PE', + 'grader_id': ['1', '2'], + 'submission_id': '1', + 'success': True, + 'rubric_scores': [[0], [0]], + 'rubric_scores_complete': [True, True], + 'rubric_xml': [etree.tostring(self.rubric), etree.tostring(self.rubric)] + } + get = {'queuekey': "abcd", + 'xqueue_body': json.dumps(score_msg)} + self.openendedmodule.update_score(get, self.test_system) + + def test_latest_post_assessment(self): self.update_score_single() assessment = self.openendedmodule.latest_post_assessment(self.test_system) @@ -298,11 +322,19 @@ class OpenEndedModuleTest(unittest.TestCase): # check for errors self.assertFalse('errors' in assessment) - def test_update_score(self): + def test_update_score_single(self): self.update_score_single() score = self.openendedmodule.latest_score() self.assertEqual(score, 4) + def test_update_score_multiple(self): + """ + Tests that a score of [0, 1] gets aggregated to 1. A change in behavior added by @jbau + """ + self.update_score_multiple() + score = self.openendedmodule.latest_score() + self.assertEquals(score, 1) + class CombinedOpenEndedModuleTest(unittest.TestCase): """ diff --git a/common/static/coffee/src/logger.coffee b/common/static/coffee/src/logger.coffee index 980d293ad8..5a13ca8264 100644 --- a/common/static/coffee/src/logger.coffee +++ b/common/static/coffee/src/logger.coffee @@ -42,5 +42,7 @@ class @Logger page: window.location.href async: false -# Keeping this for compatibility issue only. + +# log_event exists for compatibility reasons +# and will soon be deprecated. @log_event = Logger.log diff --git a/common/static/js/pdfviewer.js b/common/static/js/pdfviewer.js index c81709fa0f..258541e0ad 100644 --- a/common/static/js/pdfviewer.js +++ b/common/static/js/pdfviewer.js @@ -157,7 +157,7 @@ PDFJS.disableWorker = true; } // Update logging: - log_event("book", { "type" : "gotopage", "old" : pageNum, "new" : num }); + Logger.log("book", { "type" : "gotopage", "old" : pageNum, "new" : num }); parentElement = viewerElement; while (parentElement.hasChildNodes()) @@ -207,7 +207,7 @@ PDFJS.disableWorker = true; if (pageNum <= 1) return; renderPage(pageNum - 1); - log_event("book", { "type" : "prevpage", "new" : pageNum }); + Logger.log("book", { "type" : "prevpage", "new" : pageNum }); } // Go to next page @@ -215,7 +215,7 @@ PDFJS.disableWorker = true; if (pageNum >= pdfDocument.numPages) return; renderPage(pageNum + 1); - log_event("book", { "type" : "nextpage", "new" : pageNum }); + Logger.log("book", { "type" : "nextpage", "new" : pageNum }); } selectScaleOption = function(value) { diff --git a/common/test/data/toy/course/2012_Fall.xml b/common/test/data/toy/course/2012_Fall.xml index 9b14d49dcd..ec75ef0b9d 100644 --- a/common/test/data/toy/course/2012_Fall.xml +++ b/common/test/data/toy/course/2012_Fall.xml @@ -7,6 +7,7 @@ +