From 66657db0bee2e26a3966e6ca04b2483f88918754 Mon Sep 17 00:00:00 2001 From: Peter Baratta Date: Mon, 4 Mar 2013 09:49:41 -0700 Subject: [PATCH 01/27] Added support for superscripts in variables and fixed bug with normal subscripted variables raised to powers --- lms/lib/symmath/formula.py | 60 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/lms/lib/symmath/formula.py b/lms/lib/symmath/formula.py index c34156da52..db74d5b271 100644 --- a/lms/lib/symmath/formula.py +++ b/lms/lib/symmath/formula.py @@ -247,6 +247,65 @@ class formula(object): fix_hat(k) fix_hat(xml) + def flatten_pmathml(xml): + ''' + Give the text version of PMathML elements + ''' + tag = gettag(xml) + if tag == 'mn': return xml.text + elif tag == 'mi': return xml.text + # elif tag == 'msub': return '_'.join([flatten_pmathml(y) for y in xml]) + # elif tag == 'msup': return '^'.join([flatten_pmathml(y) for y in xml]) + elif tag == 'mrow': return ''.join([flatten_pmathml(y) for y in xml]) + raise Exception, '[flatten_pmathml] unknown tag %s' % tag + + # find "tagged" superscripts + # they have the character \u200b in the superscript + # replace them with a__b so snuggle doesn't get confused + def fix_superscripts(xml): + for k in xml: + tag = gettag(k) + + # match node to a superscript + if (tag == 'msup' and + len(k) == 2 and gettag(k[1]) == 'mrow' and + gettag(k[1][0]) == 'mo' and k[1][0].text == u'\u200b'): # whew + + k[1].remove(k[1][0]) + newk = etree.Element('mi') + newk.text = '%s__%s' % (flatten_pmathml(k[0]), flatten_pmathml(k[1])) + xml.replace(k, newk) + + if (tag == 'msubsup' and + len(k) == 3 and gettag(k[2]) == 'mrow' and + gettag(k[2][0]) == 'mo' and k[2][0].text == u'\u200b'): # whew + + k[2].remove(k[2][0]) + newk = etree.Element('mi') + newk.text = '%s_%s__%s' % (flatten_pmathml(k[0]), flatten_pmathml(k[1]), flatten_pmathml(k[2])) + xml.replace(k, newk) + + fix_superscripts(k) + fix_superscripts(xml) + + # Snuggle returns an error when it sees an + # replace such elements with an , except the first element is of + # the form a_b. I.e. map a_b^c => (a_b)^c + def fix_msubsup(parent): + for child in parent: + # fix msubsup + if (gettag(child) == 'msubsup' and len(child) == 3): + newchild = etree.Element('msup') + newbase = etree.Element('mi') + newbase.text = '%s_%s' % (flatten_pmathml(child[0]), flatten_pmathml(child[1])) + newexp = child[2] + newchild.append(newbase) + newchild.append(newexp) + parent.replace(child, newchild) + + fix_msubsup(child) + fix_msubsup(xml) + self.xml = xml return self.xml @@ -257,6 +316,7 @@ class formula(object): try: xml = self.preprocess_pmathml(self.expr) except Exception, err: + # print 'Err %s while preprocessing; expr=%s' % (err, self.expr) return "Error! Cannot process pmathml" pmathml = etree.tostring(xml, pretty_print=True) self.the_pmathml = pmathml From c6545eb092d7bcbe2d934ac2753d6fb8113f0468 Mon Sep 17 00:00:00 2001 From: Peter Baratta Date: Wed, 6 Mar 2013 06:21:08 -0700 Subject: [PATCH 02/27] Begin to document symmath as we go --- .../js/capa/symbolic_mathjax_preprocessor.js | 22 +++++++ .../course_data_formats/symbolic_response.rst | 26 ++++++++ lms/lib/symmath/formula.py | 59 +++++++++++++++++-- 3 files changed, 101 insertions(+), 6 deletions(-) create mode 100644 common/static/js/capa/symbolic_mathjax_preprocessor.js create mode 100644 doc/public/course_data_formats/symbolic_response.rst diff --git a/common/static/js/capa/symbolic_mathjax_preprocessor.js b/common/static/js/capa/symbolic_mathjax_preprocessor.js new file mode 100644 index 0000000000..19104553dc --- /dev/null +++ b/common/static/js/capa/symbolic_mathjax_preprocessor.js @@ -0,0 +1,22 @@ +window.SymbolicMathjaxPreprocessor = function () { + this.fn = function (eqn) { + // flags and config + var superscriptsOn = true; + + if (superscriptsOn) { + // find instances of "__" and make them superscripts ("^") and tag them + // as such. Specifcally replace instances of "__X" or "__{XYZ}" with + // "^{CHAR$1}", marking superscripts as different from powers + + // a zero width space--this is an invisible character that no one would + // use, that gets passed through MathJax and to the server + var c = "\u200b"; + eqn = eqn.replace(/__(?:([^\{])|\{([^\}]+)\})/g, '^{' + c + '$1$2}'); + + // NOTE: MathJax supports '\class{name}{mathcode}' but not for asciimath + // input, which is too bad. This would be preferable to the char tag + } + + return eqn; + }; +}; diff --git a/doc/public/course_data_formats/symbolic_response.rst b/doc/public/course_data_formats/symbolic_response.rst new file mode 100644 index 0000000000..773821766e --- /dev/null +++ b/doc/public/course_data_formats/symbolic_response.rst @@ -0,0 +1,26 @@ +################# +Symbolic Response +################# + +This document plans to document features that the current symbolic response +supports. In general it allows the input and validation of math expressions, +up to commutativity and some identities. + + +******** +Features +******** + +This is a partial list of features, to be revised as we go along: + * sub and superscripts: an expression following the ``^`` character + indicates exponentiation. To use superscripts in variables, the syntax + is ``b_x__d`` for the variable ``b`` with subscript ``x`` and super + ``d``. + + An example of a problem:: + + + + + + It's a bit of a pain to enter that. diff --git a/lms/lib/symmath/formula.py b/lms/lib/symmath/formula.py index 7c4ea084d6..914a65d1b0 100644 --- a/lms/lib/symmath/formula.py +++ b/lms/lib/symmath/formula.py @@ -248,14 +248,21 @@ class formula(object): fix_hat(xml) def flatten_pmathml(xml): - ''' - Give the text version of PMathML elements + ''' Give the text version of certain PMathML elements + + Sometimes MathML will be given with each letter separated (it + doesn't know if its implicit multiplication or what). From an xml + node, find the (text only) variable name it represents. So it takes + + m + a + x + + and returns 'max', for easier use later on. ''' tag = gettag(xml) if tag == 'mn': return xml.text elif tag == 'mi': return xml.text - # elif tag == 'msub': return '_'.join([flatten_pmathml(y) for y in xml]) - # elif tag == 'msup': return '^'.join([flatten_pmathml(y) for y in xml]) elif tag == 'mrow': return ''.join([flatten_pmathml(y) for y in xml]) raise Exception, '[flatten_pmathml] unknown tag %s' % tag @@ -263,23 +270,63 @@ class formula(object): # they have the character \u200b in the superscript # replace them with a__b so snuggle doesn't get confused def fix_superscripts(xml): + ''' Look for and replace sup elements with 'X__Y' or 'X_Y__Z' + + In the javascript, variables with '__X' in them had an invisible + character inserted into the sup (to distinguish from powers) + E.g. normal: + + a + b + c + + to be interpreted '(a_b)^c' (nothing done by this method) + + And modified: + + b + x + + ​ + d + + + to be interpreted 'a_b__c' + + also: + + x + + ​ + B + + + to be 'x__B' + ''' for k in xml: tag = gettag(k) - # match node to a superscript + # match things like the last example-- + # the second item in msub is an mrow with the first + # character equal to \u200b if (tag == 'msup' and len(k) == 2 and gettag(k[1]) == 'mrow' and gettag(k[1][0]) == 'mo' and k[1][0].text == u'\u200b'): # whew + # replace the msup with 'X__Y' k[1].remove(k[1][0]) newk = etree.Element('mi') newk.text = '%s__%s' % (flatten_pmathml(k[0]), flatten_pmathml(k[1])) xml.replace(k, newk) + # match things like the middle example- + # the third item in msubsup is an mrow with the first + # character equal to \u200b if (tag == 'msubsup' and len(k) == 3 and gettag(k[2]) == 'mrow' and gettag(k[2][0]) == 'mo' and k[2][0].text == u'\u200b'): # whew + # replace the msubsup with 'X_Y__Z' k[2].remove(k[2][0]) newk = etree.Element('mi') newk.text = '%s_%s__%s' % (flatten_pmathml(k[0]), flatten_pmathml(k[1]), flatten_pmathml(k[2])) @@ -316,7 +363,7 @@ class formula(object): try: xml = self.preprocess_pmathml(self.expr) except Exception, err: - # print 'Err %s while preprocessing; expr=%s' % (err, self.expr) + log.warning('Err %s while preprocessing; expr=%s' % (err, self.expr)) return "Error! Cannot process pmathml" pmathml = etree.tostring(xml, pretty_print=True) self.the_pmathml = pmathml From 49f85211fa5c5550897d25aceb786ac82d1259ee Mon Sep 17 00:00:00 2001 From: Peter Baratta Date: Fri, 8 Mar 2013 03:39:34 -0700 Subject: [PATCH 03/27] More documentation for the javascript --- .../js/capa/symbolic_mathjax_preprocessor.js | 45 ++++++++++++------- 1 file changed, 29 insertions(+), 16 deletions(-) diff --git a/common/static/js/capa/symbolic_mathjax_preprocessor.js b/common/static/js/capa/symbolic_mathjax_preprocessor.js index 19104553dc..766e5efc03 100644 --- a/common/static/js/capa/symbolic_mathjax_preprocessor.js +++ b/common/static/js/capa/symbolic_mathjax_preprocessor.js @@ -1,22 +1,35 @@ +/* This file defines a processor in between the student's math input + (AsciiMath) and what is read by MathJax. It allows for our own + customizations, such as use of the syntax "a_b__x" in superscripts, or + possibly coloring certain variables, etc&. + + It is used in the definition like the following: + + + + +*/ window.SymbolicMathjaxPreprocessor = function () { - this.fn = function (eqn) { - // flags and config - var superscriptsOn = true; + this.fn = function (eqn) { + // flags and config + var superscriptsOn = true; - if (superscriptsOn) { - // find instances of "__" and make them superscripts ("^") and tag them - // as such. Specifcally replace instances of "__X" or "__{XYZ}" with - // "^{CHAR$1}", marking superscripts as different from powers + if (superscriptsOn) { + // find instances of "__" and make them superscripts ("^") and tag them + // as such. Specifcally replace instances of "__X" or "__{XYZ}" with + // "^{CHAR$1}", marking superscripts as different from powers - // a zero width space--this is an invisible character that no one would - // use, that gets passed through MathJax and to the server - var c = "\u200b"; - eqn = eqn.replace(/__(?:([^\{])|\{([^\}]+)\})/g, '^{' + c + '$1$2}'); + // a zero width space--this is an invisible character that no one would + // use, that gets passed through MathJax and to the server + var c = "\u200b"; + eqn = eqn.replace(/__(?:([^\{])|\{([^\}]+)\})/g, '^{' + c + '$1$2}'); - // NOTE: MathJax supports '\class{name}{mathcode}' but not for asciimath - // input, which is too bad. This would be preferable to the char tag - } + // NOTE: MathJax supports '\class{name}{mathcode}' but not for asciimath + // input, which is too bad. This would be preferable to this char tag + } - return eqn; - }; + return eqn; + }; }; From 75b561267c0f8162f9e69b8c085da0544c30bc6b Mon Sep 17 00:00:00 2001 From: Peter Baratta Date: Thu, 14 Mar 2013 05:09:15 -0600 Subject: [PATCH 04/27] Script feature fix --- .../course_data_formats/symbolic_response.rst | 20 ++- lms/lib/symmath/formula.py | 25 ++++ lms/lib/symmath/test_formula.py | 115 ++++++++++++++++++ 3 files changed, 157 insertions(+), 3 deletions(-) create mode 100644 lms/lib/symmath/test_formula.py diff --git a/doc/public/course_data_formats/symbolic_response.rst b/doc/public/course_data_formats/symbolic_response.rst index 773821766e..8463faab3c 100644 --- a/doc/public/course_data_formats/symbolic_response.rst +++ b/doc/public/course_data_formats/symbolic_response.rst @@ -19,8 +19,22 @@ This is a partial list of features, to be revised as we go along: An example of a problem:: - - - + + + It's a bit of a pain to enter that. + + * The script-style math variant. What would be outputted in latex if you + entered ``\mathcal{N}``. This is used in some variables. + + An example:: + + + + + + There is no fancy preprocessing needed, but if you had superscripts or + something, you would need to include that part. diff --git a/lms/lib/symmath/formula.py b/lms/lib/symmath/formula.py index 914a65d1b0..604941ffdd 100644 --- a/lms/lib/symmath/formula.py +++ b/lms/lib/symmath/formula.py @@ -74,6 +74,15 @@ def to_latex(x): # LatexPrinter._print_dot = _print_dot xs = latex(x) xs = xs.replace(r'\XI', 'XI') # workaround for strange greek + + # substitute back into latex form for scripts + # literally something of the form + # 'scriptN' becomes '\\mathcal{N}' + # note: can't use something akin to the _print_hat method above because we sometimes get 'script(N)__B' or more complicated terms + xs = re.sub(r'script([a-zA-Z0-9]+)', + '\\mathcal{\\1}', + xs) + #return '%s{}{}' % (xs[1:-1]) if xs[0] == '$': return '[mathjax]%s[/mathjax]
' % (xs[1:-1]) # for sympy v6 @@ -106,6 +115,7 @@ def my_sympify(expr, normphase=False, matrix=False, abcsym=False, do_qubit=False 'i': sympy.I, # lowercase i is also sqrt(-1) 'Q': sympy.Symbol('Q'), # otherwise it is a sympy "ask key" 'I': sympy.Symbol('I'), # otherwise it is sqrt(-1) + 'N': sympy.Symbol('N'), # or it is some kind of sympy function #'X':sympy.sympify('Matrix([[0,1],[1,0]])'), #'Y':sympy.sympify('Matrix([[0,-I],[I,0]])'), #'Z':sympy.sympify('Matrix([[1,0],[0,-1]])'), @@ -266,6 +276,21 @@ class formula(object): elif tag == 'mrow': return ''.join([flatten_pmathml(y) for y in xml]) raise Exception, '[flatten_pmathml] unknown tag %s' % tag + def fix_mathvariant(parent): + '''Fix certain kinds of math variants + + Literally replace N + with 'scriptN'. There have been problems using script_N or script(N) + ''' + for child in parent: + if (gettag(child) == 'mstyle' and child.get('mathvariant') == 'script'): + newchild = etree.Element('mi') + newchild.text = 'script%s' % flatten_pmathml(child[0]) + parent.replace(child, newchild) + fix_mathvariant(child) + fix_mathvariant(xml) + + # find "tagged" superscripts # they have the character \u200b in the superscript # replace them with a__b so snuggle doesn't get confused diff --git a/lms/lib/symmath/test_formula.py b/lms/lib/symmath/test_formula.py new file mode 100644 index 0000000000..d3f16ed6b3 --- /dev/null +++ b/lms/lib/symmath/test_formula.py @@ -0,0 +1,115 @@ +""" +Tests of symbolic math +""" + + +import unittest +import formula +import re +from lxml import etree + +def stripXML(xml): + xml = xml.replace('\n', '') + xml = re.sub(r'\> +\<', '><', xml) + return xml + +class FormulaTest(unittest.TestCase): + # for readability later + mathml_start = '' + mathml_end = '' + + def setUp(self): + self.formulaInstance = formula.formula('') + + def test_replace_mathvariants(self): + expr = ''' + + N +''' + + expected = 'scriptN' + + # wrap + expr = stripXML(self.mathml_start + expr + self.mathml_end) + expected = stripXML(self.mathml_start + expected + self.mathml_end) + + # process the expression + xml = etree.fromstring(expr) + xml = self.formulaInstance.preprocess_pmathml(xml) + test = etree.tostring(xml) + + # success? + self.assertEqual(test, expected) + + + def test_fix_simple_superscripts(self): + expr = ''' + + a + + ​ + b + +''' + + expected = 'a__b' + + # wrap + expr = stripXML(self.mathml_start + expr + self.mathml_end) + expected = stripXML(self.mathml_start + expected + self.mathml_end) + + # process the expression + xml = etree.fromstring(expr) + xml = self.formulaInstance.preprocess_pmathml(xml) + test = etree.tostring(xml) + + # success? + self.assertEqual(test, expected) + + def test_fix_complex_superscripts(self): + expr = ''' + + a + b + + ​ + c + +''' + + expected = 'a_b__c' + + # wrap + expr = stripXML(self.mathml_start + expr + self.mathml_end) + expected = stripXML(self.mathml_start + expected + self.mathml_end) + + # process the expression + xml = etree.fromstring(expr) + xml = self.formulaInstance.preprocess_pmathml(xml) + test = etree.tostring(xml) + + # success? + self.assertEqual(test, expected) + + + def test_fix_msubsup(self): + expr = ''' + + a + b + c +''' + + expected = 'a_bc' # which is (a_b)^c + + # wrap + expr = stripXML(self.mathml_start + expr + self.mathml_end) + expected = stripXML(self.mathml_start + expected + self.mathml_end) + + # process the expression + xml = etree.fromstring(expr) + xml = self.formulaInstance.preprocess_pmathml(xml) + test = etree.tostring(xml) + + # success? + self.assertEqual(test, expected) From df935d422d31fcf34489f8b0fa501a4ac627212a Mon Sep 17 00:00:00 2001 From: Vik Paruchuri Date: Tue, 26 Mar 2013 09:52:26 -0400 Subject: [PATCH 05/27] Fix issues with open ended image grading and peer grading centralized module finder. --- .../open_ended_grading_classes/openendedchild.py | 4 ---- lms/djangoapps/courseware/module_render.py | 10 +++------- lms/djangoapps/open_ended_grading/views.py | 2 +- 3 files changed, 4 insertions(+), 12 deletions(-) diff --git a/common/lib/xmodule/xmodule/open_ended_grading_classes/openendedchild.py b/common/lib/xmodule/xmodule/open_ended_grading_classes/openendedchild.py index 2e49565bec..b9341f0cbe 100644 --- a/common/lib/xmodule/xmodule/open_ended_grading_classes/openendedchild.py +++ b/common/lib/xmodule/xmodule/open_ended_grading_classes/openendedchild.py @@ -357,10 +357,6 @@ class OpenEndedChild(object): if get_data['can_upload_files'] in ['true', '1']: has_file_to_upload = True file = get_data['student_file'][0] - if self.system.track_fuction: - self.system.track_function('open_ended_image_upload', {'filename': file.name}) - else: - log.info("No tracking function found when uploading image.") uploaded_to_s3, image_ok, s3_public_url = self.upload_image_to_s3(file) if uploaded_to_s3: image_tag = self.generate_image_tag_from_url(s3_public_url, file.name) diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index 973940d784..a1c09d3d83 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/lms/djangoapps/courseware/module_render.py @@ -208,9 +208,6 @@ def get_module_for_descriptor(user, request, descriptor, model_data_cache, cours 'waittime': settings.XQUEUE_WAITTIME_BETWEEN_REQUESTS } - def get_or_default(key, default): - getattr(settings, key, default) - #This is a hacky way to pass settings to the combined open ended xmodule #It needs an S3 interface to upload images to S3 #It needs the open ended grading interface in order to get peer grading to be done @@ -226,12 +223,11 @@ def get_module_for_descriptor(user, request, descriptor, model_data_cache, cours open_ended_grading_interface['mock_staff_grading'] = settings.MOCK_STAFF_GRADING if is_descriptor_combined_open_ended: s3_interface = { - 'access_key' : get_or_default('AWS_ACCESS_KEY_ID',''), - 'secret_access_key' : get_or_default('AWS_SECRET_ACCESS_KEY',''), - 'storage_bucket_name' : get_or_default('AWS_STORAGE_BUCKET_NAME','') + 'access_key' : getattr(settings,'AWS_ACCESS_KEY_ID',''), + 'secret_access_key' : getattr(settings,'AWS_SECRET_ACCESS_KEY',''), + 'storage_bucket_name' : getattr(settings,'AWS_STORAGE_BUCKET_NAME','') } - def inner_get_module(descriptor): """ Delegate to get_module. It does an access check, so may return None diff --git a/lms/djangoapps/open_ended_grading/views.py b/lms/djangoapps/open_ended_grading/views.py index 65cfe22ed0..78da00bf2b 100644 --- a/lms/djangoapps/open_ended_grading/views.py +++ b/lms/djangoapps/open_ended_grading/views.py @@ -111,7 +111,7 @@ def peer_grading(request, course_id): #Get the peer grading modules currently in the course items = modulestore().get_items(['i4x', None, course_id_parts[1], 'peergrading', None]) #See if any of the modules are centralized modules (ie display info from multiple problems) - items = [i for i in items if i.metadata.get("use_for_single_location", True) in false_dict] + items = [i for i in items if getattr(i,"use_for_single_location", True) in false_dict] #Get the first one item_location = items[0].location #Generate a url for the first module and redirect the user to it From d4615da555f77a15ba7c4f70d380f813f195a6f7 Mon Sep 17 00:00:00 2001 From: Vik Paruchuri Date: Tue, 26 Mar 2013 09:57:52 -0400 Subject: [PATCH 06/27] Adjust max image dim, add in safety for rewriting links --- .../combined_open_ended_modulev1.py | 6 +++++- .../open_ended_image_submission.py | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/common/lib/xmodule/xmodule/open_ended_grading_classes/combined_open_ended_modulev1.py b/common/lib/xmodule/xmodule/open_ended_grading_classes/combined_open_ended_modulev1.py index 98a54601de..c7df87fd45 100644 --- a/common/lib/xmodule/xmodule/open_ended_grading_classes/combined_open_ended_modulev1.py +++ b/common/lib/xmodule/xmodule/open_ended_grading_classes/combined_open_ended_modulev1.py @@ -363,7 +363,11 @@ class CombinedOpenEndedV1Module(): """ self.update_task_states() html = self.current_task.get_html(self.system) - return_html = rewrite_links(html, self.rewrite_content_links) + return_html = html + try: + return_html = rewrite_links(html, self.rewrite_content_links) + except: + pass return return_html def get_current_attributes(self, task_number): diff --git a/common/lib/xmodule/xmodule/open_ended_grading_classes/open_ended_image_submission.py b/common/lib/xmodule/xmodule/open_ended_grading_classes/open_ended_image_submission.py index 6956f336a5..759645840f 100644 --- a/common/lib/xmodule/xmodule/open_ended_grading_classes/open_ended_image_submission.py +++ b/common/lib/xmodule/xmodule/open_ended_grading_classes/open_ended_image_submission.py @@ -36,7 +36,7 @@ ALLOWABLE_IMAGE_SUFFIXES = [ ] #Maximum allowed dimensions (x and y) for an uploaded image -MAX_ALLOWED_IMAGE_DIM = 1500 +MAX_ALLOWED_IMAGE_DIM = 2000 #Dimensions to which image is resized before it is evaluated for color count, etc MAX_IMAGE_DIM = 150 From 8afe2eb001a925bd49e9e5fb9678c3572e47ad0e Mon Sep 17 00:00:00 2001 From: Vik Paruchuri Date: Tue, 26 Mar 2013 10:35:47 -0400 Subject: [PATCH 07/27] Increase max score allowed --- .../open_ended_grading_classes/combined_open_ended_modulev1.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/lib/xmodule/xmodule/open_ended_grading_classes/combined_open_ended_modulev1.py b/common/lib/xmodule/xmodule/open_ended_grading_classes/combined_open_ended_modulev1.py index c7df87fd45..f88fd9ab82 100644 --- a/common/lib/xmodule/xmodule/open_ended_grading_classes/combined_open_ended_modulev1.py +++ b/common/lib/xmodule/xmodule/open_ended_grading_classes/combined_open_ended_modulev1.py @@ -24,7 +24,7 @@ MAX_ATTEMPTS = 1 MAX_SCORE = 1 #The highest score allowed for the overall xmodule and for each rubric point -MAX_SCORE_ALLOWED = 3 +MAX_SCORE_ALLOWED = 50 #If true, default behavior is to score module as a practice problem. Otherwise, no grade at all is shown in progress #Metadata overrides this. From 97cb4910a7b8d36123941538776a1d53ec4be034 Mon Sep 17 00:00:00 2001 From: Vik Paruchuri Date: Tue, 26 Mar 2013 11:04:14 -0400 Subject: [PATCH 08/27] Add in default bucket, edit image url checks --- .../open_ended_grading_classes/open_ended_image_submission.py | 2 +- lms/djangoapps/courseware/module_render.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/common/lib/xmodule/xmodule/open_ended_grading_classes/open_ended_image_submission.py b/common/lib/xmodule/xmodule/open_ended_grading_classes/open_ended_image_submission.py index 759645840f..2eb9502269 100644 --- a/common/lib/xmodule/xmodule/open_ended_grading_classes/open_ended_image_submission.py +++ b/common/lib/xmodule/xmodule/open_ended_grading_classes/open_ended_image_submission.py @@ -178,7 +178,7 @@ class URLProperties(object): Runs all available url tests @return: True if URL passes tests, false if not. """ - url_is_okay = self.check_suffix() and self.check_if_parses() and self.check_domain() + url_is_okay = self.check_suffix() and self.check_if_parses() return url_is_okay def check_domain(self): diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index a1c09d3d83..15f95f1beb 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/lms/djangoapps/courseware/module_render.py @@ -225,7 +225,7 @@ def get_module_for_descriptor(user, request, descriptor, model_data_cache, cours s3_interface = { 'access_key' : getattr(settings,'AWS_ACCESS_KEY_ID',''), 'secret_access_key' : getattr(settings,'AWS_SECRET_ACCESS_KEY',''), - 'storage_bucket_name' : getattr(settings,'AWS_STORAGE_BUCKET_NAME','') + 'storage_bucket_name' : getattr(settings,'AWS_STORAGE_BUCKET_NAME','openended') } def inner_get_module(descriptor): From 967cf7e6f36e98b1db7c8560e535c25c1327f476 Mon Sep 17 00:00:00 2001 From: Diana Huang Date: Tue, 26 Mar 2013 14:43:41 -0400 Subject: [PATCH 09/27] Fix a problem where trying to show image response answers was causing 500 errors. Add test to verify that this won't happen again. --- common/lib/capa/capa/responsetypes.py | 42 ++++++++++++++++--- .../lib/capa/capa/tests/test_responsetypes.py | 11 +++++ 2 files changed, 48 insertions(+), 5 deletions(-) diff --git a/common/lib/capa/capa/responsetypes.py b/common/lib/capa/capa/responsetypes.py index 8ab716735c..2035c42661 100644 --- a/common/lib/capa/capa/responsetypes.py +++ b/common/lib/capa/capa/responsetypes.py @@ -1961,9 +1961,10 @@ class ImageResponse(LoncapaResponse): self.ielements = self.inputfields self.answer_ids = [ie.get('id') for ie in self.ielements] + def get_score(self, student_answers): correct_map = CorrectMap() - expectedset = self.get_answers() + expectedset = self.get_mapped_answers() for aid in self.answer_ids: # loop through IDs of # fields in our stanza given = student_answers[ @@ -2018,11 +2019,42 @@ class ImageResponse(LoncapaResponse): break return correct_map - def get_answers(self): - return ( + def get_mapped_answers(self): + ''' + Returns the internal representation of the answers + + Input: + None + Returns: + tuple (dict, dict) - + rectangles (dict) - a map of inputs to the defined rectangle for that input + regions (dict) - a map of inputs to the defined region for that input + ''' + answers = ( dict([(ie.get('id'), ie.get( 'rectangle')) for ie in self.ielements]), dict([(ie.get('id'), ie.get('regions')) for ie in self.ielements])) + return answers + + def get_answers(self): + ''' + Returns the external representation of the answers + + Input: + None + Returns: + dict (str, (str, str)) - a map of inputs to a tuple of their rectange + and their regions + ''' + answers = {} + for ie in self.ielements: + ie_id = ie.get('id') + answers[ie_id] = (ie.get('rectangle'), ie.get('regions')) + + return answers + + + #----------------------------------------------------------------------------- @@ -2087,8 +2119,8 @@ class AnnotationResponse(LoncapaResponse): correct_option = self._find_option_with_choice( inputfield, 'correct') if correct_option is not None: - answer_map[inputfield.get( - 'id')] = correct_option.get('description') + input_id = inputfield.get('id') + answer_map[input_id] = correct_option.get('description') return answer_map def _get_max_points(self): diff --git a/common/lib/capa/capa/tests/test_responsetypes.py b/common/lib/capa/capa/tests/test_responsetypes.py index 0c007f83b2..e009c26aef 100644 --- a/common/lib/capa/capa/tests/test_responsetypes.py +++ b/common/lib/capa/capa/tests/test_responsetypes.py @@ -36,6 +36,10 @@ class ResponseTest(unittest.TestCase): correct_map = problem.grade_answers(input_dict) self.assertEquals(correct_map.get_correctness('1_2_1'), expected_correctness) + def assert_answer_format(self, problem): + answers = problem.get_question_answers() + self.assertTrue(answers['1_2_1'] is not None) + def assert_multiple_grade(self, problem, correct_answers, incorrect_answers): for input_str in correct_answers: result = problem.grade_answers({'1_2_1': input_str}).get_correctness('1_2_1') @@ -166,6 +170,13 @@ class ImageResponseTest(ResponseTest): incorrect_inputs = ["[0,0]", "[600,300]"] self.assert_multiple_grade(problem, correct_inputs, incorrect_inputs) + def test_show_answer(self): + rectangle_str = "(100,100)-(200,200)" + region_str = "[[10,10], [20,10], [20, 30]]" + + problem = self.build_problem(regions=region_str, rectangle=rectangle_str) + self.assert_answer_format(problem) + class SymbolicResponseTest(unittest.TestCase): def test_sr_grade(self): From 84f2cc8af6713b3d41efc545a7fd42a5a80e3b92 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Andr=C3=A9s=20Rocha?= Date: Tue, 26 Mar 2013 15:08:22 -0400 Subject: [PATCH 10/27] Display advertised date correctly if it is an ISO date --- common/lib/xmodule/xmodule/course_module.py | 11 ++++++++++- .../xmodule/xmodule/tests/test_course_module.py | 16 ++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/common/lib/xmodule/xmodule/course_module.py b/common/lib/xmodule/xmodule/course_module.py index 7999f8d6da..6f3b8e94c9 100644 --- a/common/lib/xmodule/xmodule/course_module.py +++ b/common/lib/xmodule/xmodule/course_module.py @@ -635,8 +635,17 @@ class CourseDescriptor(CourseFields, SequenceDescriptor): @property def start_date_text(self): + def try_parse_iso_8601(text): + try: + result = datetime.strptime(text, "%Y-%m-%dT%H:%M") + result = result.strftime("%b %d, %Y") + except ValueError: + result = text.title() + + return result + if isinstance(self.advertised_start, basestring): - return self.advertised_start + return try_parse_iso_8601(self.advertised_start) elif self.advertised_start is None and self.start is None: return 'TBD' else: diff --git a/common/lib/xmodule/xmodule/tests/test_course_module.py b/common/lib/xmodule/xmodule/tests/test_course_module.py index e1de8a1ed0..eda9cf386c 100644 --- a/common/lib/xmodule/xmodule/tests/test_course_module.py +++ b/common/lib/xmodule/xmodule/tests/test_course_module.py @@ -1,5 +1,6 @@ import unittest from time import strptime + from fs.memoryfs import MemoryFS from mock import Mock, patch @@ -108,7 +109,22 @@ class IsNewCourseTestCase(unittest.TestCase): print "Comparing %s to %s" % (a, b) assertion(a_score, b_score) + @patch('xmodule.course_module.time.gmtime') + def test_start_date_text(self, gmtime_mock): + gmtime_mock.return_value = NOW + settings = [ + # start, advertized, result + ('2012-12-02T12:00', None, 'Dec 02, 2012'), + ('2012-12-02T12:00', '2011-11-01T12:00', 'Nov 01, 2011'), + ('2012-12-02T12:00', 'Spring 2012', 'Spring 2012'), + ('2012-12-02T12:00', 'November, 2011', 'November, 2011'), + ] + + for s in settings: + d = self.get_dummy_course(start=s[0], advertised_start=s[1]) + print "Checking start=%s advertised=%s" % (s[0], s[1]) + self.assertEqual(d.start_date_text, s[2]) @patch('xmodule.course_module.time.gmtime') def test_is_newish(self, gmtime_mock): From 90553a1b1d842600588fe0ecab9402b15c11de3c Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 26 Mar 2013 16:19:34 -0400 Subject: [PATCH 11/27] Use get_many and set_many to cut down on the number of metadata trees to retrieve, and only retrieve them once per call to _load_items --- .../lib/xmodule/xmodule/modulestore/mongo.py | 73 +++++++++++-------- .../xmodule/modulestore/tests/test_mongo.py | 58 ++++++++++++++- 2 files changed, 100 insertions(+), 31 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/mongo.py b/common/lib/xmodule/xmodule/modulestore/mongo.py index b76251bb99..27a5ffbc26 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo.py @@ -9,6 +9,7 @@ from fs.osfs import OSFS from itertools import repeat from path import path from datetime import datetime +from operator import attrgetter from importlib import import_module from xmodule.errortracker import null_error_tracker, exc_info_to_str @@ -107,7 +108,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem): references to metadata_inheritance_tree """ def __init__(self, modulestore, module_data, default_class, resources_fs, - error_tracker, render_template, metadata_inheritance_tree = None): + error_tracker, render_template, metadata_cache = None): """ modulestore: the module store that can be used to retrieve additional modules @@ -132,7 +133,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem): # cdodge: other Systems have a course_id attribute defined. To keep things consistent, let's # define an attribute here as well, even though it's None self.course_id = None - self.metadata_inheritance_tree = metadata_inheritance_tree + self.metadata_cache = metadata_cache def load_item(self, location): location = Location(location) @@ -165,8 +166,8 @@ class CachingDescriptorSystem(MakoDescriptorSystem): model_data = DbModel(kvs, class_, None, MongoUsage(self.course_id, location)) module = class_(self, location, model_data) - if self.metadata_inheritance_tree is not None: - metadata_to_inherit = self.metadata_inheritance_tree.get('parent_metadata', {}).get(location.url(), {}) + if self.metadata_cache is not None: + metadata_to_inherit = self.metadata_cache.get(metadata_cache_key(location), {}).get('parent_metadata', {}).get(location.url(), {}) inherit_metadata(module, metadata_to_inherit) return module except: @@ -206,6 +207,9 @@ def namedtuple_to_son(namedtuple, prefix=''): return son +metadata_cache_key = attrgetter('org', 'course') + + class MongoModuleStore(ModuleStoreBase): """ A Mongodb backed ModuleStore @@ -278,6 +282,7 @@ class MongoModuleStore(ModuleStoreBase): # now traverse the tree and compute down the inherited metadata metadata_to_inherit = {} + def _compute_inherited_metadata(url): my_metadata = {} # check for presence of metadata key. Note that a given module may not yet be fully formed. @@ -293,7 +298,7 @@ class MongoModuleStore(ModuleStoreBase): # go through all the children and recurse, but only if we have # in the result set. Remember results will not contain leaf nodes - for child in results_by_url[url].get('definition',{}).get('children',[]): + for child in results_by_url[url].get('definition', {}).get('children', []): if child in results_by_url: new_child_metadata = copy.deepcopy(my_metadata) new_child_metadata.update(results_by_url[child].get('metadata', {})) @@ -304,42 +309,42 @@ class MongoModuleStore(ModuleStoreBase): # this is likely a leaf node, so let's record what metadata we need to inherit metadata_to_inherit[child] = my_metadata - if root is not None: _compute_inherited_metadata(root) return {'parent_metadata': metadata_to_inherit, - 'timestamp' : datetime.now()} + 'timestamp': datetime.now()} - def get_cached_metadata_inheritance_tree(self, location, force_refresh=False): + def get_cached_metadata_inheritance_trees(self, locations, force_refresh=False): ''' TODO (cdodge) This method can be deleted when the 'split module store' work has been completed ''' - key_name = '{0}/{1}'.format(location.org, location.course) - tree = None - if self.metadata_inheritance_cache is not None: - tree = self.metadata_inheritance_cache.get(key_name) + trees = {} + if locations and self.metadata_inheritance_cache is not None and not force_refresh: + trees = self.metadata_inheritance_cache.get_many(list(set([metadata_cache_key(loc) for loc in locations]))) else: # This is to help guard against an accident prod runtime without a cache logging.warning('Running MongoModuleStore without metadata_inheritance_cache. This should not happen in production!') - if tree is None or force_refresh: - tree = self.get_metadata_inheritance_tree(location) - if self.metadata_inheritance_cache is not None: - self.metadata_inheritance_cache.set(key_name, tree) + to_cache = {} + for loc in locations: + if metadata_cache_key(loc) not in trees: + to_cache[metadata_cache_key(loc)] = trees[metadata_cache_key(loc)] = self.get_metadata_inheritance_tree(loc) - return tree + if to_cache and self.metadata_inheritance_cache is not None: + self.metadata_inheritance_cache.set_many(to_cache) + + return trees def refresh_cached_metadata_inheritance_tree(self, location): pseudo_course_id = '/'.join([location.org, location.course]) if pseudo_course_id not in self.ignore_write_events_on_courses: - self.get_cached_metadata_inheritance_tree(location, force_refresh = True) + self.get_cached_metadata_inheritance_trees([location], force_refresh=True) def clear_cached_metadata_inheritance_tree(self, location): - key_name = '{0}/{1}'.format(location.org, location.course) if self.metadata_inheritance_cache is not None: - self.metadata_inheritance_cache.delete(key_name) + self.metadata_inheritance_cache.delete(metadata_cache_key(location)) def _clean_item_data(self, item): """ @@ -385,7 +390,18 @@ class MongoModuleStore(ModuleStoreBase): return data - def _load_item(self, item, data_cache, should_apply_metadata_inheritence=True): + def _cache_metadata_inheritance(self, items, depth, force_refresh=False): + """ + Retrieves all course metadata inheritance trees needed to load items + """ + + locations = [ + Location(item['location']) for item in items + if not (item['location']['category'] == 'course' and depth == 0) + ] + return self.get_cached_metadata_inheritance_trees(locations, force_refresh=force_refresh) + + def _load_item(self, item, data_cache, metadata_cache): """ Load an XModuleDescriptor from item, using the children stored in data_cache """ @@ -399,9 +415,6 @@ class MongoModuleStore(ModuleStoreBase): metadata_inheritance_tree = None - if should_apply_metadata_inheritence: - metadata_inheritance_tree = self.get_cached_metadata_inheritance_tree(Location(item['location'])) - # TODO (cdodge): When the 'split module store' work has been completed, we should remove # the 'metadata_inheritance_tree' parameter system = CachingDescriptorSystem( @@ -411,7 +424,7 @@ class MongoModuleStore(ModuleStoreBase): resource_fs, self.error_tracker, self.render_template, - metadata_inheritance_tree = metadata_inheritance_tree + metadata_cache, ) return system.load_item(item['location']) @@ -421,11 +434,11 @@ class MongoModuleStore(ModuleStoreBase): to specified depth """ data_cache = self._cache_children(items, depth) + inheritance_cache = self._cache_metadata_inheritance(items, depth) # if we are loading a course object, if we're not prefetching children (depth != 0) then don't - # bother with the metadata inheritence - return [self._load_item(item, data_cache, - should_apply_metadata_inheritence=(item['location']['category'] != 'course' or depth != 0)) for item in items] + # bother with the metadata inheritence + return [self._load_item(item, data_cache, inheritance_cache) for item in items] def get_courses(self): ''' @@ -631,7 +644,7 @@ class MongoModuleStore(ModuleStoreBase): self._update_single_item(location, {'metadata': metadata}) # recompute (and update) the metadata inheritance tree which is cached - self.refresh_cached_metadata_inheritance_tree(loc) + self.refresh_cached_metadata_inheritance_tree(loc) def delete_item(self, location): """ @@ -654,7 +667,7 @@ class MongoModuleStore(ModuleStoreBase): # from overriding our default value set in the init method. safe=self.collection.safe) # recompute (and update) the metadata inheritance tree which is cached - self.refresh_cached_metadata_inheritance_tree(Location(location)) + self.refresh_cached_metadata_inheritance_tree(Location(location)) def get_parent_locations(self, location, course_id): '''Find all locations that are the parents of this location in this diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py index 6f6f47ba85..3e29c07ea4 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py @@ -1,6 +1,7 @@ import pymongo -from nose.tools import assert_equals, assert_raises, assert_not_equals, with_setup +from mock import Mock +from nose.tools import assert_equals, assert_raises, assert_not_equals, with_setup, assert_false from pprint import pprint from xmodule.modulestore import Location @@ -102,3 +103,58 @@ class TestMongoModuleStore(object): def test_path_to_location(self): '''Make sure that path_to_location works''' check_path_to_location(self.store) + + def test_metadata_inheritance_query_count(self): + ''' + When retrieving items from mongo, we should only query the cache a number of times + equal to the number of courses being retrieved from. + + We should also not query + ''' + self.store.metadata_inheritance_cache = Mock() + get_many = self.store.metadata_inheritance_cache.get_many + set_many = self.store.metadata_inheritance_cache.set_many + get_many.return_value = {('edX', 'toy'): {}} + + self.store.get_item(Location("i4x://edX/toy/course/2012_Fall"), depth=0) + assert_false(get_many.called) + assert_false(set_many.called) + get_many.reset_mock() + + self.store.get_item(Location("i4x://edX/toy/course/2012_Fall"), depth=3) + get_many.assert_called_with([('edX', 'toy')]) + assert_equals(0, set_many.call_count) + get_many.reset_mock() + + self.store.get_items(Location('i4x', 'edX', None, 'course', None), depth=0) + assert_false(get_many.called) + assert_false(set_many.called) + get_many.reset_mock() + + self.store.get_items(Location('i4x', 'edX', None, 'course', None), depth=3) + assert_equals(1, get_many.call_count) + assert_equals([('edX', 'simple'), ('edX', 'toy')], sorted(get_many.call_args[0][0])) + assert_equals(1, set_many.call_count) + assert_equals([('edX', 'simple')], sorted(set_many.call_args[0][0].keys())) + get_many.reset_mock() + + self.store.get_items(Location('i4x', 'edX', None, None, None), depth=0) + assert_equals(1, get_many.call_count) + assert_equals([('edX', 'simple'), ('edX', 'toy')], sorted(get_many.call_args[0][0])) + assert_equals(1, set_many.call_count) + assert_equals([('edX', 'simple')], sorted(set_many.call_args[0][0].keys())) + get_many.reset_mock() + + def test_metadata_inheritance_query_count_forced_refresh(self): + self.store.metadata_inheritance_cache = Mock() + get_many = self.store.metadata_inheritance_cache.get_many + set_many = self.store.metadata_inheritance_cache.set_many + get_many.return_value = {('edX', 'toy'): {}} + + self.store.get_cached_metadata_inheritance_trees( + [Location("i4x://edX/toy/course/2012_Fall"), Location("i4x://edX/simple/course/2012_Fall")], + True + ) + assert_false(get_many.called) + assert_equals(1, set_many.call_count) + assert_equals([('edX', 'simple'), ('edX', 'toy')], sorted(set_many.call_args[0][0].keys())) From 1f11508ac6b4057a137d35a11835db4f6d231588 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 26 Mar 2013 16:30:55 -0400 Subject: [PATCH 12/27] Pylint cleanup --- .../lib/xmodule/xmodule/modulestore/mongo.py | 44 +++++++++++++------ 1 file changed, 31 insertions(+), 13 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/mongo.py b/common/lib/xmodule/xmodule/modulestore/mongo.py index 27a5ffbc26..47049b9fd6 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo.py @@ -97,6 +97,7 @@ class MongoKeyValueStore(KeyValueStore): else: return False + MongoUsage = namedtuple('MongoUsage', 'id, def_id') @@ -108,7 +109,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem): references to metadata_inheritance_tree """ def __init__(self, modulestore, module_data, default_class, resources_fs, - error_tracker, render_template, metadata_cache = None): + error_tracker, render_template, metadata_cache=None): """ modulestore: the module store that can be used to retrieve additional modules @@ -136,6 +137,9 @@ class CachingDescriptorSystem(MakoDescriptorSystem): self.metadata_cache = metadata_cache def load_item(self, location): + """ + Return an XModule instance for the specified location + """ location = Location(location) json_data = self.module_data.get(location) if json_data is None: @@ -197,12 +201,12 @@ def location_to_query(location, wildcard=True): return query -def namedtuple_to_son(namedtuple, prefix=''): +def namedtuple_to_son(ntuple, prefix=''): """ Converts a namedtuple into a SON object with the same key order """ son = SON() - for idx, field_name in enumerate(namedtuple._fields): + for idx, field_name in enumerate(ntuple._fields): son[prefix + field_name] = namedtuple[idx] return son @@ -232,7 +236,6 @@ class MongoModuleStore(ModuleStoreBase): if user is not None and password is not None: self.collection.database.authenticate(user, password) - # Force mongo to report errors, at the expense of performance self.collection.safe = True @@ -262,7 +265,7 @@ class MongoModuleStore(ModuleStoreBase): query = { '_id.org': location.org, '_id.course': location.course, - '_id.category': {'$in': [ 'course', 'chapter', 'sequential', 'vertical']} + '_id.category': {'$in': ['course', 'chapter', 'sequential', 'vertical']} } # we just want the Location, children, and metadata record_filter = {'_id': 1, 'definition.children': 1, 'metadata': 1} @@ -284,6 +287,9 @@ class MongoModuleStore(ModuleStoreBase): metadata_to_inherit = {} def _compute_inherited_metadata(url): + """ + Helper method for computing inherited metadata for a specific location url + """ my_metadata = {} # check for presence of metadata key. Note that a given module may not yet be fully formed. # example: update_item -> update_children -> update_metadata sequence on new item create @@ -325,12 +331,14 @@ class MongoModuleStore(ModuleStoreBase): trees = self.metadata_inheritance_cache.get_many(list(set([metadata_cache_key(loc) for loc in locations]))) else: # This is to help guard against an accident prod runtime without a cache - logging.warning('Running MongoModuleStore without metadata_inheritance_cache. This should not happen in production!') + logging.warning('Running MongoModuleStore without metadata_inheritance_cache. ' + 'This should not happen in production!') to_cache = {} for loc in locations: - if metadata_cache_key(loc) not in trees: - to_cache[metadata_cache_key(loc)] = trees[metadata_cache_key(loc)] = self.get_metadata_inheritance_tree(loc) + cache_key = metadata_cache_key(loc) + if cache_key not in trees: + to_cache[cache_key] = trees[cache_key] = self.get_metadata_inheritance_tree(loc) if to_cache and self.metadata_inheritance_cache is not None: self.metadata_inheritance_cache.set_many(to_cache) @@ -338,11 +346,19 @@ class MongoModuleStore(ModuleStoreBase): return trees def refresh_cached_metadata_inheritance_tree(self, location): + """ + Refresh the cached metadata inheritance tree for the org/course combination + for location + """ pseudo_course_id = '/'.join([location.org, location.course]) if pseudo_course_id not in self.ignore_write_events_on_courses: - self.get_cached_metadata_inheritance_trees([location], force_refresh=True) + self.get_cached_metadata_inheritance_trees([location], force_refresh=True) def clear_cached_metadata_inheritance_tree(self, location): + """ + Delete the cached metadata inheritance tree for the org/course combination + for location + """ if self.metadata_inheritance_cache is not None: self.metadata_inheritance_cache.delete(metadata_cache_key(location)) @@ -372,7 +388,7 @@ class MongoModuleStore(ModuleStoreBase): data[Location(item['location'])] = item if depth == 0: - break; + break # Load all children by id. See # http://www.mongodb.org/display/DOCS/Advanced+Queries#AdvancedQueries-%24or @@ -413,8 +429,6 @@ class MongoModuleStore(ModuleStoreBase): resource_fs = OSFS(root) - metadata_inheritance_tree = None - # TODO (cdodge): When the 'split module store' work has been completed, we should remove # the 'metadata_inheritance_tree' parameter system = CachingDescriptorSystem( @@ -572,7 +586,8 @@ class MongoModuleStore(ModuleStoreBase): raise Exception('Could not find course at {0}'.format(course_search_location)) if found_cnt > 1: - raise Exception('Found more than one course at {0}. There should only be one!!! Dump = {1}'.format(course_search_location, courses)) + raise Exception('Found more than one course at {0}. There should only be one!!! ' + 'Dump = {1}'.format(course_search_location, courses)) return courses[0] @@ -688,4 +703,7 @@ class MongoModuleStore(ModuleStoreBase): # DraftModuleStore is first, because it needs to intercept calls to MongoModuleStore class DraftMongoModuleStore(DraftModuleStore, MongoModuleStore): + """ + Version of MongoModuleStore with draft capability mixed in + """ pass From e0343342b0d20f87818ec74535a0f96e8e91c70c Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 26 Mar 2013 16:36:58 -0400 Subject: [PATCH 13/27] Fix typo during pylint fixes --- common/lib/xmodule/xmodule/modulestore/mongo.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/common/lib/xmodule/xmodule/modulestore/mongo.py b/common/lib/xmodule/xmodule/modulestore/mongo.py index 47049b9fd6..fdc34913ee 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo.py @@ -207,7 +207,7 @@ def namedtuple_to_son(ntuple, prefix=''): """ son = SON() for idx, field_name in enumerate(ntuple._fields): - son[prefix + field_name] = namedtuple[idx] + son[prefix + field_name] = ntuple[idx] return son @@ -703,6 +703,9 @@ class MongoModuleStore(ModuleStoreBase): # DraftModuleStore is first, because it needs to intercept calls to MongoModuleStore class DraftMongoModuleStore(DraftModuleStore, MongoModuleStore): + """ + Version of MongoModuleStore with draft capability mixed in + """ """ Version of MongoModuleStore with draft capability mixed in """ From b975d4d90ce1a5e3fc79ab18c6eec96b2052bea3 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 26 Mar 2013 16:43:58 -0400 Subject: [PATCH 14/27] Fix tests --- cms/djangoapps/contentstore/tests/test_contentstore.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index edb20561bc..e6b5933d66 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -211,7 +211,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): new_loc = descriptor.location._replace(org='MITx', course='999') print "Checking {0} should now also be at {1}".format(descriptor.location.url(), new_loc.url()) resp = self.client.get(reverse('edit_unit', kwargs={'location': new_loc.url()})) - self.assertEqual(resp.status_code, 200) + self.assertEqual(resp.status_code, 200) def test_delete_course(self): import_from_xml(modulestore(), 'common/test/data/', ['full']) @@ -328,11 +328,11 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): self.assertEqual(wrapper.counter, 4) # make sure we pre-fetched a known sequential which should be at depth=2 - self.assertTrue(Location(['i4x', 'edX', 'full', 'sequential', + self.assertTrue(Location(['i4x', 'edX', 'full', 'sequential', 'Administrivia_and_Circuit_Elements', None]) in course.system.module_data) # make sure we don't have a specific vertical which should be at depth=3 - self.assertFalse(Location(['i4x', 'edX', 'full', 'vertical', 'vertical_58', + self.assertFalse(Location(['i4x', 'edX', 'full', 'vertical', 'vertical_58', None]) in course.system.module_data) def test_export_course_with_unknown_metadata(self): @@ -556,7 +556,7 @@ class ContentStoreTest(ModuleStoreTestCase): module_store.update_children(parent.location, parent.children + [new_component_location.url()]) # flush the cache - module_store.get_cached_metadata_inheritance_tree(new_component_location, -1) + module_store.refresh_cached_metadata_inheritance_tree(new_component_location) new_module = module_store.get_item(new_component_location) # check for grace period definition which should be defined at the course level @@ -571,7 +571,7 @@ class ContentStoreTest(ModuleStoreTestCase): module_store.update_metadata(new_module.location, own_metadata(new_module)) # flush the cache and refetch - module_store.get_cached_metadata_inheritance_tree(new_component_location, -1) + module_store.refresh_cached_metadata_inheritance_tree(new_component_location) new_module = module_store.get_item(new_component_location) self.assertEqual(timedelta(1), new_module.lms.graceperiod) From a44ecdfcd6e01ea6f38b82f6c9348a955808c87f Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Tue, 26 Mar 2013 16:45:47 -0400 Subject: [PATCH 15/27] if we parse an invalid location in the content store middleware, then return a 404, not a 500 --- common/djangoapps/contentserver/middleware.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/common/djangoapps/contentserver/middleware.py b/common/djangoapps/contentserver/middleware.py index c5e887801e..e8674a1e9e 100644 --- a/common/djangoapps/contentserver/middleware.py +++ b/common/djangoapps/contentserver/middleware.py @@ -5,6 +5,7 @@ from django.http import HttpResponse, Http404, HttpResponseNotModified from xmodule.contentstore.django import contentstore from xmodule.contentstore.content import StaticContent, XASSET_LOCATION_TAG +from xmodule.modulestore import InvalidLocationError from cache_toolbox.core import get_cached_content, set_cached_content from xmodule.exceptions import NotFoundError @@ -13,7 +14,13 @@ class StaticContentServer(object): def process_request(self, request): # look to see if the request is prefixed with 'c4x' tag if request.path.startswith('/' + XASSET_LOCATION_TAG + '/'): - loc = StaticContent.get_location_from_path(request.path) + try: + loc = StaticContent.get_location_from_path(request.path) + except InvalidLocationError: + response = HttpResponse() + response.status_code = 404 + return response + # first look in our cache so we don't have to round-trip to the DB content = get_cached_content(loc) if content is None: From b0e2c82ad3619bea30674562e347cd76b9856de4 Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Tue, 26 Mar 2013 20:02:29 -0400 Subject: [PATCH 16/27] actually.. return a 400 rather than a 404 because the request is malformed. Also add unit test. --- cms/djangoapps/contentstore/tests/test_contentstore.py | 4 ++++ common/djangoapps/contentserver/middleware.py | 3 ++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index edb20561bc..a8cde71379 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -213,6 +213,10 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): resp = self.client.get(reverse('edit_unit', kwargs={'location': new_loc.url()})) self.assertEqual(resp.status_code, 200) + def test_bad_contentstore_request(self): + resp = self.client.get('http://localhost:8001/c4x/CDX/123123/asset/&images_circuits_Lab7Solution2.png') + self.assertEqual(resp.status_code, 400) + def test_delete_course(self): import_from_xml(modulestore(), 'common/test/data/', ['full']) diff --git a/common/djangoapps/contentserver/middleware.py b/common/djangoapps/contentserver/middleware.py index e8674a1e9e..8e9e70046d 100644 --- a/common/djangoapps/contentserver/middleware.py +++ b/common/djangoapps/contentserver/middleware.py @@ -17,8 +17,9 @@ class StaticContentServer(object): try: loc = StaticContent.get_location_from_path(request.path) except InvalidLocationError: + # return a 'Bad Request' to browser as we have a malformed Location response = HttpResponse() - response.status_code = 404 + response.status_code = 400 return response # first look in our cache so we don't have to round-trip to the DB From 195fd2d1fe3b591e8bd5380707272a170a3b000d Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Tue, 26 Mar 2013 23:48:06 -0400 Subject: [PATCH 17/27] optimize the result-set that gets returned from Mongo on metadata inheritence. We just need the fields which are actually inheritable, so no need to return anything else as it gets filtered out during the computation --- common/lib/xmodule/xmodule/modulestore/mongo.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/mongo.py b/common/lib/xmodule/xmodule/modulestore/mongo.py index fdc34913ee..38b15ab76e 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo.py @@ -267,8 +267,13 @@ class MongoModuleStore(ModuleStoreBase): '_id.course': location.course, '_id.category': {'$in': ['course', 'chapter', 'sequential', 'vertical']} } - # we just want the Location, children, and metadata - record_filter = {'_id': 1, 'definition.children': 1, 'metadata': 1} + # we just want the Location, children, and inheritable metadata + record_filter = {'_id': 1, 'definition.children': 1} + + # just get the inheritable metadata since that is all we need for the computation + # this minimizes both data pushed over the wire + for attr in INHERITABLE_METADATA: + record_filter['metadata.{0}'.format(attr)] = 1 # call out to the DB resultset = self.collection.find(query, record_filter) From 2120481738489d872db41916b75b0336a77a3a9e Mon Sep 17 00:00:00 2001 From: Brian Talbot Date: Wed, 27 Mar 2013 01:34:25 -0400 Subject: [PATCH 18/27] studio - corrected JQ selector for smoothscrolling in-page links --- cms/static/js/base.js | 4 ++-- cms/templates/howitworks.html | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/cms/static/js/base.js b/cms/static/js/base.js index bd8dc0bae8..7466233331 100644 --- a/cms/static/js/base.js +++ b/cms/static/js/base.js @@ -81,7 +81,7 @@ $(document).ready(function () { }); // general link management - smooth scrolling page links - $('a[rel*="view"]').bind('click', linkSmoothScroll); + $('a[rel*="view"][href^="#"]').bind('click', smoothScrollLink); // toggling overview section details @@ -148,7 +148,7 @@ $(document).ready(function () { }); }); -function linkSmoothScroll(e) { +function smoothScrollLink(e) { (e).preventDefault(); $.smoothScroll({ diff --git a/cms/templates/howitworks.html b/cms/templates/howitworks.html index 1cf9b17710..7a819fceba 100644 --- a/cms/templates/howitworks.html +++ b/cms/templates/howitworks.html @@ -151,7 +151,7 @@
Simple two-level outline to organize your couse. Drag and drop, and see your course at a glance.
- + ␡ close modal @@ -164,7 +164,7 @@
Quickly create videos, text snippets, inline discussions, and a variety of problem types.
- + ␡ close modal @@ -177,7 +177,7 @@
Simply set the date of a section or subsection, and Studio will publish it to your students for you.
- + ␡ close modal From 2c0e5b82ff2535770a5ca605aa1b1bd521c756d4 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 27 Mar 2013 07:29:22 -0400 Subject: [PATCH 19/27] Return a 403 when an anonymous user attempts to hit modx_dispatch. Fixes https://www.pivotaltracker.com/story/show/46916015 and https://www.pivotaltracker.com/story/show/46916029 --- lms/djangoapps/courseware/module_render.py | 4 +++ .../courseware/tests/test_module_render.py | 31 +++++++++---------- 2 files changed, 19 insertions(+), 16 deletions(-) diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index 973940d784..4747f7b341 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/lms/djangoapps/courseware/module_render.py @@ -8,6 +8,7 @@ from functools import partial from django.conf import settings from django.contrib.auth.models import User +from django.core.exceptions import PermissionDenied from django.core.urlresolvers import reverse from django.http import Http404 from django.http import HttpResponse @@ -412,6 +413,9 @@ def modx_dispatch(request, dispatch, location, course_id): if not Location.is_valid(location): raise Http404("Invalid location") + if not request.user.is_authenticated(): + raise PermissionDenied + # Check for submitted files and basic file size checks p = request.POST.copy() if request.FILES: diff --git a/lms/djangoapps/courseware/tests/test_module_render.py b/lms/djangoapps/courseware/tests/test_module_render.py index 3a3a7ac5ea..90ca796a2f 100644 --- a/lms/djangoapps/courseware/tests/test_module_render.py +++ b/lms/djangoapps/courseware/tests/test_module_render.py @@ -1,14 +1,7 @@ -import logging -from mock import MagicMock, patch +from mock import MagicMock import json -import factory -import unittest -from nose.tools import set_trace -from django.http import Http404, HttpResponse, HttpRequest -from django.conf import settings -from django.contrib.auth.models import User -from django.test.client import Client +from django.http import Http404, HttpResponse from django.conf import settings from django.test import TestCase from django.test.client import RequestFactory @@ -16,13 +9,9 @@ from django.core.urlresolvers import reverse from django.test.utils import override_settings from xmodule.modulestore.exceptions import ItemNotFoundError -from xmodule.exceptions import NotFoundError -from xmodule.modulestore import Location import courseware.module_render as render -from xmodule.modulestore.django import modulestore, _MODULESTORES -from xmodule.seq_module import SequenceModule +from xmodule.modulestore.django import modulestore from courseware.tests.tests import PageLoader -from student.models import Registration from courseware.model_data import ModelDataCache from .factories import UserFactory @@ -52,7 +41,6 @@ TEST_DATA_XML_MODULESTORE = xml_store_config(TEST_DATA_DIR) class ModuleRenderTestCase(PageLoader): def setUp(self): self.location = ['i4x', 'edX', 'toy', 'chapter', 'Overview'] - self._MODULESTORES = {} self.course_id = 'edX/toy/2012_Fall' self.toy_course = modulestore().get_course(self.course_id) @@ -104,12 +92,23 @@ class ModuleRenderTestCase(PageLoader): self.assertEquals(render.get_score_bucket(11, 10), 'incorrect') self.assertEquals(render.get_score_bucket(-1, 10), 'incorrect') + def test_anonymous_modx_dispatch(self): + dispatch_url = reverse( + 'modx_dispatch', + args=[ + 'edX/toy/2012_Fall', + 'i4x://edX/toy/videosequence/Toy_Videos', + 'goto_position' + ] + ) + response = self.client.post(dispatch_url, {'position': 2}) + self.assertEquals(403, response.status_code) + @override_settings(MODULESTORE=TEST_DATA_XML_MODULESTORE) class TestTOC(TestCase): """Check the Table of Contents for a course""" def setUp(self): - self._MODULESTORES = {} # Toy courses should be loaded self.course_name = 'edX/toy/2012_Fall' From 521843876efb005303e8ff7423442eb9830ab99e Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 27 Mar 2013 08:10:25 -0400 Subject: [PATCH 20/27] Make the django_comment_client return errors that can't be parsed as JSON just as simple strings when in an ajax context --- .../django_comment_client/middleware.py | 16 +++++++++- .../tests/test_middleware.py | 32 +++++++++---------- 2 files changed, 30 insertions(+), 18 deletions(-) diff --git a/lms/djangoapps/django_comment_client/middleware.py b/lms/djangoapps/django_comment_client/middleware.py index abf2d40cab..b9efc1589e 100644 --- a/lms/djangoapps/django_comment_client/middleware.py +++ b/lms/djangoapps/django_comment_client/middleware.py @@ -1,10 +1,24 @@ from comment_client import CommentClientError from django_comment_client.utils import JsonError import json +import logging + +log = logging.getLogger(__name__) class AjaxExceptionMiddleware(object): + """ + Middleware that captures CommentClientErrors during ajax requests + and tranforms them into json responses + """ def process_exception(self, request, exception): + """ + Processes CommentClientErrors in ajax requests. If the request is an ajax request, + returns a http response that encodes the error as json + """ if isinstance(exception, CommentClientError) and request.is_ajax(): - return JsonError(json.loads(exception.message)) + try: + return JsonError(json.loads(exception.message)) + except ValueError: + return JsonError(exception.message) return None diff --git a/lms/djangoapps/django_comment_client/tests/test_middleware.py b/lms/djangoapps/django_comment_client/tests/test_middleware.py index 55e4c72c75..ab9517c160 100644 --- a/lms/djangoapps/django_comment_client/tests/test_middleware.py +++ b/lms/djangoapps/django_comment_client/tests/test_middleware.py @@ -1,7 +1,3 @@ -import string -import random -import collections - from django.test import TestCase import comment_client @@ -13,17 +9,19 @@ class AjaxExceptionTestCase(TestCase): # TODO: check whether the correct error message is produced. # The error message should be the same as the argument to CommentClientError - def setUp(self): - self.a = middleware.AjaxExceptionMiddleware() - self.request1 = django.http.HttpRequest() - self.request0 = django.http.HttpRequest() - self.exception1 = comment_client.CommentClientError('{}') - self.exception0 = ValueError() - self.request1.META['HTTP_X_REQUESTED_WITH'] = "XMLHttpRequest" - self.request0.META['HTTP_X_REQUESTED_WITH'] = "SHADOWFAX" + def setUp(self): + self.a = middleware.AjaxExceptionMiddleware() + self.request1 = django.http.HttpRequest() + self.request0 = django.http.HttpRequest() + self.exception1 = comment_client.CommentClientError('{}') + self.exception2 = comment_client.CommentClientError('Foo!') + self.exception0 = ValueError() + self.request1.META['HTTP_X_REQUESTED_WITH'] = "XMLHttpRequest" + self.request0.META['HTTP_X_REQUESTED_WITH'] = "SHADOWFAX" - def test_process_exception(self): - self.assertIsInstance(self.a.process_exception(self.request1, self.exception1), middleware.JsonError) - self.assertIsNone(self.a.process_exception(self.request1, self.exception0)) - self.assertIsNone(self.a.process_exception(self.request0, self.exception1)) - self.assertIsNone(self.a.process_exception(self.request0, self.exception0)) + def test_process_exception(self): + self.assertIsInstance(self.a.process_exception(self.request1, self.exception1), middleware.JsonError) + self.assertIsInstance(self.a.process_exception(self.request1, self.exception2), middleware.JsonError) + self.assertIsNone(self.a.process_exception(self.request1, self.exception0)) + self.assertIsNone(self.a.process_exception(self.request0, self.exception1)) + self.assertIsNone(self.a.process_exception(self.request0, self.exception0)) From 7101c76016e4c42b18ba5858556b836da6fde66b Mon Sep 17 00:00:00 2001 From: Vik Paruchuri Date: Wed, 27 Mar 2013 12:02:32 -0400 Subject: [PATCH 21/27] comment on rewrite links change --- .../combined_open_ended_modulev1.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/common/lib/xmodule/xmodule/open_ended_grading_classes/combined_open_ended_modulev1.py b/common/lib/xmodule/xmodule/open_ended_grading_classes/combined_open_ended_modulev1.py index f88fd9ab82..6fe37b9525 100644 --- a/common/lib/xmodule/xmodule/open_ended_grading_classes/combined_open_ended_modulev1.py +++ b/common/lib/xmodule/xmodule/open_ended_grading_classes/combined_open_ended_modulev1.py @@ -365,6 +365,10 @@ class CombinedOpenEndedV1Module(): html = self.current_task.get_html(self.system) return_html = html try: + #Without try except block, get this error: + # File "/home/vik/mitx_all/mitx/common/lib/xmodule/xmodule/x_module.py", line 263, in rewrite_content_links + # if link.startswith(XASSET_SRCREF_PREFIX): + # Placing try except so that if the error is fixed, this code will start working again. return_html = rewrite_links(html, self.rewrite_content_links) except: pass @@ -786,7 +790,7 @@ class CombinedOpenEndedV1Descriptor(): template_dir_name = "combinedopenended" def __init__(self, system): - self.system =system + self.system = system @classmethod def definition_from_xml(cls, xml_object, system): From 15ea32b095abe4d033075640121ad418ced0179d Mon Sep 17 00:00:00 2001 From: Will Daly Date: Wed, 27 Mar 2013 12:53:58 -0400 Subject: [PATCH 22/27] Fixed bug 294, caused by unicode encoding error when creating logging strings. Added unit tests that verify the fix. --- common/djangoapps/student/tests/test_login.py | 107 ++++++++++++++++++ common/djangoapps/student/views.py | 8 +- 2 files changed, 111 insertions(+), 4 deletions(-) create mode 100644 common/djangoapps/student/tests/test_login.py diff --git a/common/djangoapps/student/tests/test_login.py b/common/djangoapps/student/tests/test_login.py new file mode 100644 index 0000000000..dda58a4462 --- /dev/null +++ b/common/djangoapps/student/tests/test_login.py @@ -0,0 +1,107 @@ +from django.test import TestCase +from django.test.client import Client +from django.core.urlresolvers import reverse +from django.contrib.auth.models import User +from student.models import Registration, UserProfile +import json + +class LoginTest(TestCase): + ''' + Test student.views.login_user() view + ''' + + def setUp(self): + + # Create one user and save it to the database + self.user = User.objects.create_user('test', 'test@edx.org', 'test_password') + self.user.is_active = True + self.user.save() + + # Create a registration for the user + Registration().register(self.user) + + # Create a profile for the user + UserProfile(user=self.user).save() + + # Create the test client + self.client = Client() + + # Store the login url + self.url = reverse('login') + + def test_login_success(self): + response = self._login_response('test@edx.org', 'test_password') + self._assert_response(response, success=True) + + def test_login_success_unicode_email(self): + unicode_email = u'test@edx.org' + unichr(40960) + + self.user.email = unicode_email + self.user.save() + + response = self._login_response(unicode_email, 'test_password') + self._assert_response(response, success=True) + + + def test_login_fail_no_user_exists(self): + response = self._login_response('not_a_user@edx.org', 'test_password') + self._assert_response(response, success=False, + value='Email or password is incorrect') + + def test_login_fail_wrong_password(self): + response = self._login_response('test@edx.org', 'wrong_password') + self._assert_response(response, success=False, + value='Email or password is incorrect') + + def test_login_not_activated(self): + + # De-activate the user + self.user.is_active = False + self.user.save() + + # Should now be unable to login + response = self._login_response('test@edx.org', 'test_password') + self._assert_response(response, success=False, + value="This account has not been activated") + + + def test_login_unicode_email(self): + unicode_email = u'test@edx.org' + unichr(40960) + response = self._login_response(unicode_email, 'test_password') + self._assert_response(response, success=False) + + def test_login_unicode_password(self): + unicode_password = u'test_password' + unichr(1972) + response = self._login_response('test@edx.org', unicode_password) + self._assert_response(response, success=False) + + def _login_response(self, email, password): + post_params = {'email': email, 'password': password} + return self.client.post(self.url, post_params) + + def _assert_response(self, response, success=None, value=None): + ''' + Assert that the response had status 200 and returned a valid + JSON-parseable dict. + + If success is provided, assert that the response had that + value for 'success' in the JSON dict. + + If value is provided, assert that the response contained that + value for 'value' in the JSON dict. + ''' + self.assertEqual(response.status_code, 200) + + try: + response_dict = json.loads(response.content) + except ValueError: + self.fail("Could not parse response content as JSON: %s" + % str(response.content)) + + if success is not None: + self.assertEqual(response_dict['success'], success) + + if value is not None: + msg = ("'%s' did not contain '%s'" % + (str(response_dict['value']), str(value))) + self.assertTrue(value in response_dict['value'], msg) diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index 5dbaf5d2c2..84730421e8 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -369,14 +369,14 @@ def login_user(request, error=""): try: user = User.objects.get(email=email) except User.DoesNotExist: - log.warning("Login failed - Unknown user email: {0}".format(email)) + log.warning(u"Login failed - Unknown user email: {0}".format(email)) return HttpResponse(json.dumps({'success': False, 'value': 'Email or password is incorrect.'})) # TODO: User error message username = user.username user = authenticate(username=username, password=password) if user is None: - log.warning("Login failed - password for {0} is invalid".format(email)) + log.warning(u"Login failed - password for {0} is invalid".format(email)) return HttpResponse(json.dumps({'success': False, 'value': 'Email or password is incorrect.'})) @@ -392,7 +392,7 @@ def login_user(request, error=""): log.critical("Login failed - Could not create session. Is memcached running?") log.exception(e) - log.info("Login success - {0} ({1})".format(username, email)) + log.info(u"Login success - {0} ({1})".format(username, email)) try_change_enrollment(request) @@ -400,7 +400,7 @@ def login_user(request, error=""): return HttpResponse(json.dumps({'success': True})) - log.warning("Login failed - Account not active for user {0}, resending activation".format(username)) + log.warning(u"Login failed - Account not active for user {0}, resending activation".format(username)) reactivation_email_for_user(user) not_activated_msg = "This account has not been activated. We have " + \ From 227a5e8266ddc72e9719eb2b6035a12ee0788c56 Mon Sep 17 00:00:00 2001 From: cahrens Date: Wed, 27 Mar 2013 12:56:06 -0400 Subject: [PATCH 23/27] Delete converters, move unit tests to test_fields, add new additional test cases. --- .../tests/test_course_settings.py | 75 +++-------------- .../models/settings/course_details.py | 28 +++++-- .../models/settings/course_grading.py | 2 - common/djangoapps/util/converters.py | 37 --------- common/lib/xmodule/xmodule/fields.py | 1 - .../lib/xmodule/xmodule/tests/test_fields.py | 80 +++++++++++++++++++ 6 files changed, 113 insertions(+), 110 deletions(-) delete mode 100644 common/djangoapps/util/converters.py create mode 100644 common/lib/xmodule/xmodule/tests/test_fields.py diff --git a/cms/djangoapps/contentstore/tests/test_course_settings.py b/cms/djangoapps/contentstore/tests/test_course_settings.py index 2e7bc5db83..fe90ad18aa 100644 --- a/cms/djangoapps/contentstore/tests/test_course_settings.py +++ b/cms/djangoapps/contentstore/tests/test_course_settings.py @@ -1,8 +1,6 @@ import datetime import json import copy -from util import converters -from util.converters import jsdate_to_time from django.contrib.auth.models import User from django.test.client import Client @@ -15,69 +13,13 @@ from models.settings.course_details import (CourseDetails, from models.settings.course_grading import CourseGradingModel from contentstore.utils import get_modulestore -from django.test import TestCase from .utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory from models.settings.course_metadata import CourseMetadata from xmodule.modulestore.xml_importer import import_from_xml from xmodule.modulestore.django import modulestore -import time - - -# YYYY-MM-DDThh:mm:ss.s+/-HH:MM -class ConvertersTestCase(TestCase): - @staticmethod - def struct_to_datetime(struct_time): - return datetime.datetime(struct_time.tm_year, struct_time.tm_mon, - struct_time.tm_mday, struct_time.tm_hour, - struct_time.tm_min, struct_time.tm_sec, tzinfo=UTC()) - - def compare_dates(self, date1, date2, expected_delta): - dt1 = ConvertersTestCase.struct_to_datetime(date1) - dt2 = ConvertersTestCase.struct_to_datetime(date2) - self.assertEqual(dt1 - dt2, expected_delta, str(date1) + "-" - + str(date2) + "!=" + str(expected_delta)) - - def test_iso_to_struct(self): - '''Test conversion from iso compatible date strings to struct_time''' - self.compare_dates(converters.jsdate_to_time("2013-01-01"), - converters.jsdate_to_time("2012-12-31"), - datetime.timedelta(days=1)) - self.compare_dates(converters.jsdate_to_time("2013-01-01T00"), - converters.jsdate_to_time("2012-12-31T23"), - datetime.timedelta(hours=1)) - self.compare_dates(converters.jsdate_to_time("2013-01-01T00:00"), - converters.jsdate_to_time("2012-12-31T23:59"), - datetime.timedelta(minutes=1)) - self.compare_dates(converters.jsdate_to_time("2013-01-01T00:00:00"), - converters.jsdate_to_time("2012-12-31T23:59:59"), - datetime.timedelta(seconds=1)) - self.compare_dates(converters.jsdate_to_time("2013-01-01T00:00:00Z"), - converters.jsdate_to_time("2012-12-31T23:59:59Z"), - datetime.timedelta(seconds=1)) - self.compare_dates( - converters.jsdate_to_time("2012-12-31T23:00:01-01:00"), - converters.jsdate_to_time("2013-01-01T00:00:00+01:00"), - datetime.timedelta(hours=1, seconds=1)) - - def test_struct_to_iso(self): - ''' - Test converting time reprs to iso dates - ''' - self.assertEqual( - converters.time_to_isodate( - time.strptime("2012-12-31T23:59:59Z", "%Y-%m-%dT%H:%M:%SZ")), - "2012-12-31T23:59:59Z") - self.assertEqual( - converters.time_to_isodate( - jsdate_to_time("2012-12-31T23:59:59Z")), - "2012-12-31T23:59:59Z") - self.assertEqual( - converters.time_to_isodate( - jsdate_to_time("2012-12-31T23:00:01-01:00")), - "2013-01-01T00:00:01Z") - +from xmodule.fields import Date class CourseTestCase(ModuleStoreTestCase): def setUp(self): @@ -206,17 +148,24 @@ class CourseDetailsViewTest(CourseTestCase): self.assertEqual(details['intro_video'], encoded.get('intro_video', None), context + " intro_video not ==") self.assertEqual(details['effort'], encoded['effort'], context + " efforts not ==") + @staticmethod + def struct_to_datetime(struct_time): + return datetime.datetime(struct_time.tm_year, struct_time.tm_mon, + struct_time.tm_mday, struct_time.tm_hour, + struct_time.tm_min, struct_time.tm_sec, tzinfo=UTC()) + def compare_date_fields(self, details, encoded, context, field): if details[field] is not None: + date = Date() if field in encoded and encoded[field] is not None: - encoded_encoded = jsdate_to_time(encoded[field]) - dt1 = ConvertersTestCase.struct_to_datetime(encoded_encoded) + encoded_encoded = date.from_json(encoded[field]) + dt1 = CourseDetailsViewTest.struct_to_datetime(encoded_encoded) if isinstance(details[field], datetime.datetime): dt2 = details[field] else: - details_encoded = jsdate_to_time(details[field]) - dt2 = ConvertersTestCase.struct_to_datetime(details_encoded) + details_encoded = date.from_json(details[field]) + dt2 = CourseDetailsViewTest.struct_to_datetime(details_encoded) expected_delta = datetime.timedelta(0) self.assertEqual(dt1 - dt2, expected_delta, str(dt1) + "!=" + str(dt2) + " at " + context) diff --git a/cms/djangoapps/models/settings/course_details.py b/cms/djangoapps/models/settings/course_details.py index d3cd5fe164..09d57774ab 100644 --- a/cms/djangoapps/models/settings/course_details.py +++ b/cms/djangoapps/models/settings/course_details.py @@ -1,14 +1,14 @@ -from xmodule.modulestore.django import modulestore from xmodule.modulestore import Location from xmodule.modulestore.exceptions import ItemNotFoundError from xmodule.modulestore.inheritance import own_metadata import json from json.encoder import JSONEncoder import time +import calendar from contentstore.utils import get_modulestore -from util.converters import jsdate_to_time, time_to_date from models.settings import course_grading from contentstore.utils import update_item +from xmodule.fields import Date import re import logging @@ -81,8 +81,14 @@ class CourseDetails(object): dirty = False + # In the descriptor's setter, the date is converted to JSON using Date's to_json method. + # Calling to_json on something that is already JSON doesn't work. Since reaching directly + # into the model is nasty, convert the JSON Date to a Python date, which is what the + # setter expects as input. + date = Date() + if 'start_date' in jsondict: - converted = jsdate_to_time(jsondict['start_date']) + converted = date.from_json(jsondict['start_date']) else: converted = None if converted != descriptor.start: @@ -90,7 +96,7 @@ class CourseDetails(object): descriptor.start = converted if 'end_date' in jsondict: - converted = jsdate_to_time(jsondict['end_date']) + converted = date.from_json(jsondict['end_date']) else: converted = None @@ -99,7 +105,7 @@ class CourseDetails(object): descriptor.end = converted if 'enrollment_start' in jsondict: - converted = jsdate_to_time(jsondict['enrollment_start']) + converted = date.from_json(jsondict['enrollment_start']) else: converted = None @@ -108,7 +114,7 @@ class CourseDetails(object): descriptor.enrollment_start = converted if 'enrollment_end' in jsondict: - converted = jsdate_to_time(jsondict['enrollment_end']) + converted = date.from_json(jsondict['enrollment_end']) else: converted = None @@ -172,12 +178,20 @@ class CourseDetails(object): # TODO move to a more general util? Is there a better way to do the isinstance model check? class CourseSettingsEncoder(json.JSONEncoder): + @staticmethod + def time_to_date(time_obj): + """ + Convert a time.time_struct to a true universal time (can pass to js Date + constructor) + """ + return calendar.timegm(time_obj) * 1000 + def default(self, obj): if isinstance(obj, CourseDetails) or isinstance(obj, course_grading.CourseGradingModel): return obj.__dict__ elif isinstance(obj, Location): return obj.dict() elif isinstance(obj, time.struct_time): - return time_to_date(obj) + return CourseSettingsEncoder.time_to_date(obj) else: return JSONEncoder.default(self, obj) diff --git a/cms/djangoapps/models/settings/course_grading.py b/cms/djangoapps/models/settings/course_grading.py index b20fb71f66..ee9b4ac0eb 100644 --- a/cms/djangoapps/models/settings/course_grading.py +++ b/cms/djangoapps/models/settings/course_grading.py @@ -1,7 +1,5 @@ from xmodule.modulestore import Location from contentstore.utils import get_modulestore -import re -from util import converters from datetime import timedelta diff --git a/common/djangoapps/util/converters.py b/common/djangoapps/util/converters.py deleted file mode 100644 index 212cceb77d..0000000000 --- a/common/djangoapps/util/converters.py +++ /dev/null @@ -1,37 +0,0 @@ -import time -import datetime -import calendar -import dateutil.parser - - -def time_to_date(time_obj): - """ - Convert a time.time_struct to a true universal time (can pass to js Date - constructor) - """ - return calendar.timegm(time_obj) * 1000 - - -def time_to_isodate(source): - '''Convert to an iso date''' - if isinstance(source, time.struct_time): - return time.strftime('%Y-%m-%dT%H:%M:%SZ', source) - elif isinstance(source, datetime): - return source.isoformat() + 'Z' - - -def jsdate_to_time(field): - """ - Convert a universal time (iso format) or msec since epoch to a time obj - """ - if field is None: - return field - elif isinstance(field, basestring): - d = dateutil.parser.parse(field) - return d.utctimetuple() - elif isinstance(field, (int, long, float)): - return time.gmtime(field / 1000) - elif isinstance(field, time.struct_time): - return field - else: - raise ValueError("Couldn't convert %r to time" % field) diff --git a/common/lib/xmodule/xmodule/fields.py b/common/lib/xmodule/xmodule/fields.py index 0abe850d68..ea857933fc 100644 --- a/common/lib/xmodule/xmodule/fields.py +++ b/common/lib/xmodule/xmodule/fields.py @@ -14,7 +14,6 @@ class Date(ModelType): ''' Date fields know how to parse and produce json (iso) compatible formats. ''' - # NB: these are copies of util.converters.* def from_json(self, field): """ Parse an optional metadata key containing a time: if present, complain diff --git a/common/lib/xmodule/xmodule/tests/test_fields.py b/common/lib/xmodule/xmodule/tests/test_fields.py new file mode 100644 index 0000000000..7c8872efc1 --- /dev/null +++ b/common/lib/xmodule/xmodule/tests/test_fields.py @@ -0,0 +1,80 @@ +"""Tests for Date class defined in fields.py.""" +import datetime +import unittest +from django.utils.timezone import UTC +from xmodule.fields import Date +import time + +class DateTest(unittest.TestCase): + date = Date() + + @staticmethod + def struct_to_datetime(struct_time): + return datetime.datetime(struct_time.tm_year, struct_time.tm_mon, + struct_time.tm_mday, struct_time.tm_hour, + struct_time.tm_min, struct_time.tm_sec, tzinfo=UTC()) + + def compare_dates(self, date1, date2, expected_delta): + dt1 = DateTest.struct_to_datetime(date1) + dt2 = DateTest.struct_to_datetime(date2) + self.assertEqual(dt1 - dt2, expected_delta, str(date1) + "-" + + str(date2) + "!=" + str(expected_delta)) + + def test_from_json(self): + '''Test conversion from iso compatible date strings to struct_time''' + self.compare_dates( + DateTest.date.from_json("2013-01-01"), + DateTest.date.from_json("2012-12-31"), + datetime.timedelta(days=1)) + self.compare_dates( + DateTest.date.from_json("2013-01-01T00"), + DateTest.date.from_json("2012-12-31T23"), + datetime.timedelta(hours=1)) + self.compare_dates( + DateTest.date.from_json("2013-01-01T00:00"), + DateTest.date.from_json("2012-12-31T23:59"), + datetime.timedelta(minutes=1)) + self.compare_dates( + DateTest.date.from_json("2013-01-01T00:00:00"), + DateTest.date.from_json("2012-12-31T23:59:59"), + datetime.timedelta(seconds=1)) + self.compare_dates( + DateTest.date.from_json("2013-01-01T00:00:00Z"), + DateTest.date.from_json("2012-12-31T23:59:59Z"), + datetime.timedelta(seconds=1)) + self.compare_dates( + DateTest.date.from_json("2012-12-31T23:00:01-01:00"), + DateTest.date.from_json("2013-01-01T00:00:00+01:00"), + datetime.timedelta(hours=1, seconds=1)) + + def test_return_None(self): + self.assertIsNone(DateTest.date.from_json("")) + self.assertIsNone(DateTest.date.from_json(None)) + self.assertIsNone(DateTest.date.from_json(['unknown value'])) + + def test_old_due_date_format(self): + current = datetime.datetime.today() + self.assertEqual( + time.struct_time((current.year, 3, 12, 12, 0, 0, 1, 71, 0)), + DateTest.date.from_json("March 12 12:00")) + self.assertEqual( + time.struct_time((current.year, 12, 4, 16, 30, 0, 2, 338, 0)), + DateTest.date.from_json("December 4 16:30")) + + def test_to_json(self): + ''' + Test converting time reprs to iso dates + ''' + self.assertEqual( + DateTest.date.to_json( + time.strptime("2012-12-31T23:59:59Z", "%Y-%m-%dT%H:%M:%SZ")), + "2012-12-31T23:59:59Z") + self.assertEqual( + DateTest.date.to_json( + DateTest.date.from_json("2012-12-31T23:59:59Z")), + "2012-12-31T23:59:59Z") + self.assertEqual( + DateTest.date.to_json( + DateTest.date.from_json("2012-12-31T23:00:01-01:00")), + "2013-01-01T00:00:01Z") + From cddc868656d784da1db5585879c9518918b6a512 Mon Sep 17 00:00:00 2001 From: Will Daly Date: Wed, 27 Mar 2013 13:01:10 -0400 Subject: [PATCH 24/27] Login URL resolves differently in LMS and CMS, which breaks login_test when loaded by rake test_cms I moved the test into lms/courseware/tests so they run correctly. --- .../student => lms/djangoapps/courseware}/tests/test_login.py | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename {common/djangoapps/student => lms/djangoapps/courseware}/tests/test_login.py (100%) diff --git a/common/djangoapps/student/tests/test_login.py b/lms/djangoapps/courseware/tests/test_login.py similarity index 100% rename from common/djangoapps/student/tests/test_login.py rename to lms/djangoapps/courseware/tests/test_login.py From 22537ffd3b05269b688972e7e2ad81e118cc1da7 Mon Sep 17 00:00:00 2001 From: cahrens Date: Wed, 27 Mar 2013 14:51:39 -0400 Subject: [PATCH 25/27] Don't need to convert to milliseconds. --- cms/djangoapps/models/settings/course_details.py | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/cms/djangoapps/models/settings/course_details.py b/cms/djangoapps/models/settings/course_details.py index 09d57774ab..b45f5bd343 100644 --- a/cms/djangoapps/models/settings/course_details.py +++ b/cms/djangoapps/models/settings/course_details.py @@ -178,20 +178,12 @@ class CourseDetails(object): # TODO move to a more general util? Is there a better way to do the isinstance model check? class CourseSettingsEncoder(json.JSONEncoder): - @staticmethod - def time_to_date(time_obj): - """ - Convert a time.time_struct to a true universal time (can pass to js Date - constructor) - """ - return calendar.timegm(time_obj) * 1000 - def default(self, obj): if isinstance(obj, CourseDetails) or isinstance(obj, course_grading.CourseGradingModel): return obj.__dict__ elif isinstance(obj, Location): return obj.dict() elif isinstance(obj, time.struct_time): - return CourseSettingsEncoder.time_to_date(obj) + return Date().to_json(obj) else: return JSONEncoder.default(self, obj) From 5c78218b1360bf9e0eb6bcee41cbc44e1aeb1dac Mon Sep 17 00:00:00 2001 From: cahrens Date: Wed, 27 Mar 2013 14:52:27 -0400 Subject: [PATCH 26/27] Don't need to convert to milliseconds. --- cms/djangoapps/models/settings/course_details.py | 1 - 1 file changed, 1 deletion(-) diff --git a/cms/djangoapps/models/settings/course_details.py b/cms/djangoapps/models/settings/course_details.py index b45f5bd343..876000c7fe 100644 --- a/cms/djangoapps/models/settings/course_details.py +++ b/cms/djangoapps/models/settings/course_details.py @@ -4,7 +4,6 @@ from xmodule.modulestore.inheritance import own_metadata import json from json.encoder import JSONEncoder import time -import calendar from contentstore.utils import get_modulestore from models.settings import course_grading from contentstore.utils import update_item From 122c8567c5d370a6e54e075d4e736c96bcfef646 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 27 Mar 2013 15:00:08 -0400 Subject: [PATCH 27/27] An integrity error while creating an enrollment just means that our work has already been done. Fixes https://www.pivotaltracker.com/story/show/46915947 --- common/djangoapps/student/views.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index 5dbaf5d2c2..d0deffd7b9 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -325,7 +325,12 @@ def change_enrollment(request): "course:{0}".format(course_num), "run:{0}".format(run)]) - enrollment, created = CourseEnrollment.objects.get_or_create(user=user, course_id=course.id) + try: + enrollment, created = CourseEnrollment.objects.get_or_create(user=user, course_id=course.id) + except IntegrityError: + # If we've already created this enrollment in a separate transaction, + # then just continue + pass return {'success': True} elif action == "unenroll":