From 4f78c1977f2256fc832514256f4f967040f7eaff Mon Sep 17 00:00:00 2001 From: Peter Baratta Date: Wed, 19 Jun 2013 10:59:24 -0400 Subject: [PATCH 01/55] Allow error messages with non-ascii characters to be handled correctly Also, add a test for this behavior. --- CHANGELOG.rst | 2 ++ common/lib/xmodule/xmodule/capa_module.py | 2 +- .../xmodule/xmodule/tests/test_capa_module.py | 28 +++++++++++++++++++ 3 files changed, 31 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 206be44c87..6a79757c0f 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -42,6 +42,8 @@ setting now run entirely outside the Python sandbox. Blades: Added tests for Video Alpha player. +Common: Have the capa module handle unicode better (especially errors) + Blades: Video Alpha bug fix for speed changing to 1.0 in Firefox. Blades: Additional event tracking added to Video Alpha: fullscreen switch, show/hide diff --git a/common/lib/xmodule/xmodule/capa_module.py b/common/lib/xmodule/xmodule/capa_module.py index d9f7fc61aa..85c935c9e7 100644 --- a/common/lib/xmodule/xmodule/capa_module.py +++ b/common/lib/xmodule/xmodule/capa_module.py @@ -781,7 +781,7 @@ class CapaModule(CapaFields, XModule): # Otherwise, display just an error message, # without a stack trace else: - msg = "Error: %s" % str(inst.message) + msg = u"Error: {msg}".format(msg=inst.message) return {'success': msg} diff --git a/common/lib/xmodule/xmodule/tests/test_capa_module.py b/common/lib/xmodule/xmodule/tests/test_capa_module.py index 696ef58268..85e69cabc1 100644 --- a/common/lib/xmodule/xmodule/tests/test_capa_module.py +++ b/common/lib/xmodule/xmodule/tests/test_capa_module.py @@ -1,3 +1,4 @@ +# -*- coding: utf-8 -*- """Tests of the Capa XModule""" #pylint: disable=C0111 #pylint: disable=R0904 @@ -520,6 +521,33 @@ class CapaModuleTest(unittest.TestCase): # Expect that the number of attempts is NOT incremented self.assertEqual(module.attempts, 1) + def test_check_problem_error_nonascii(self): + + # Try each exception that capa_module should handle + for exception_class in [StudentInputError, + LoncapaProblemError, + ResponseError]: + + # Create the module + module = CapaFactory.create(attempts=1) + + # Ensure that the user is NOT staff + module.system.user_is_staff = False + + # Simulate answering a problem that raises the exception + with patch('capa.capa_problem.LoncapaProblem.grade_answers') as mock_grade: + mock_grade.side_effect = exception_class(u"ȧƈƈḗƞŧḗḓ ŧḗẋŧ ƒǿř ŧḗşŧīƞɠ") + + get_request_dict = {CapaFactory.input_key(): '3.14'} + result = module.check_problem(get_request_dict) + + # Expect an AJAX alert message in 'success' + expected_msg = u'Error: ȧƈƈḗƞŧḗḓ ŧḗẋŧ ƒǿř ŧḗşŧīƞɠ' + self.assertEqual(expected_msg, result['success']) + + # Expect that the number of attempts is NOT incremented + self.assertEqual(module.attempts, 1) + def test_check_problem_error_with_staff_user(self): # Try each exception that capa module should handle From 401dd550e477ca0616313f85aa2f64d64dc88a2b Mon Sep 17 00:00:00 2001 From: Peter Baratta Date: Tue, 18 Jun 2013 13:14:52 -0400 Subject: [PATCH 02/55] Convert many byte strings to unicode; change string formatting --- common/lib/calc/calc.py | 2 +- common/lib/xmodule/xmodule/capa_module.py | 49 +++++++++++++---------- 2 files changed, 28 insertions(+), 23 deletions(-) diff --git a/common/lib/calc/calc.py b/common/lib/calc/calc.py index f0934a9ed5..bbfd9545f6 100644 --- a/common/lib/calc/calc.py +++ b/common/lib/calc/calc.py @@ -93,7 +93,7 @@ def check_variables(string, variables): Pyparsing uses a left-to-right parser, which makes a more elegant approach pretty hopeless. """ - general_whitespace = re.compile('[^\\w]+') + general_whitespace = re.compile('[^\\w]+') # TODO consider non-ascii # List of all alnums in string possible_variables = re.split(general_whitespace, string) bad_variables = [] diff --git a/common/lib/xmodule/xmodule/capa_module.py b/common/lib/xmodule/xmodule/capa_module.py index 85c935c9e7..3bd8331678 100644 --- a/common/lib/xmodule/xmodule/capa_module.py +++ b/common/lib/xmodule/xmodule/capa_module.py @@ -60,7 +60,7 @@ class Randomization(String): class ComplexEncoder(json.JSONEncoder): def default(self, obj): if isinstance(obj, complex): - return "{real:.7g}{imag:+.7g}*j".format(real=obj.real, imag=obj.imag) + return u"{real:.7g}{imag:+.7g}*j".format(real=obj.real, imag=obj.imag) return json.JSONEncoder.default(self, obj) @@ -167,7 +167,7 @@ class CapaModule(CapaFields, XModule): self.seed = self.lcp.seed except Exception as err: - msg = 'cannot create LoncapaProblem {loc}: {err}'.format( + msg = u'cannot create LoncapaProblem {loc}: {err}'.format( loc=self.location.url(), err=err) # TODO (vshnayder): do modules need error handlers too? # We shouldn't be switching on DEBUG. @@ -176,12 +176,15 @@ class CapaModule(CapaFields, XModule): # TODO (vshnayder): This logic should be general, not here--and may # want to preserve the data instead of replacing it. # e.g. in the CMS - msg = '

%s

' % msg.replace('<', '<') - msg += '

%s

' % traceback.format_exc().replace('<', '<') + msg = u'

{msg}

'.format(msg=cgi.escape(msg)) + msg += u'

{tb}

'.format( + tb=cgi.escape(traceback.format_exc())) # create a dummy problem with error message instead of failing - problem_text = ('' - 'Problem %s has an error:%s' % - (self.location.url(), msg)) + problem_text = (u'' + u'Problem {url} has an error:{msg}'.format( + url=self.location.url(), + msg=msg) + ) self.lcp = self.new_lcp(self.get_state_for_lcp(), text=problem_text) else: # add extra info and raise @@ -362,15 +365,14 @@ class CapaModule(CapaFields, XModule): # TODO (vshnayder): another switch on DEBUG. if self.system.DEBUG: msg = ( - '[courseware.capa.capa_module] ' - 'Failed to generate HTML for problem %s' % - (self.location.url())) - msg += '

Error:

%s

' % str(err).replace('<', '<') - msg += '

%s

' % traceback.format_exc().replace('<', '<') + u'[courseware.capa.capa_module] ' + u'Failed to generate HTML for problem {url}'.format( + url=cgi.escape(self.location.url())) + ) + msg += u'

Error:

{msg}

'.format(msg=cgi.escape(err.message)) + msg += u'

{tb}

'.format(tb=cgi.escape(traceback.format_exc())) html = msg - # We're in non-debug mode, and possibly even in production. We want - # to avoid bricking of problem as much as possible else: # We're in non-debug mode, and possibly even in production. We want # to avoid bricking of problem as much as possible @@ -454,8 +456,9 @@ class CapaModule(CapaFields, XModule): html = self.system.render_template('problem.html', context) if encapsulate: - html = '
'.format( - id=self.location.html_id(), ajax_url=self.system.ajax_url) + html + "
" + html = u'
'.format( + id=self.location.html_id(), ajax_url=self.system.ajax_url + ) + html + "
" # now do the substitutions which are filesystem based, e.g. '/static/' prefixes return self.system.replace_urls(html) @@ -641,7 +644,8 @@ class CapaModule(CapaFields, XModule): try: new_answer = {answer_id: self.system.replace_urls(answers[answer_id])} except TypeError: - log.debug('Unable to perform URL substitution on answers[%s]: %s' % (answer_id, answers[answer_id])) + log.debug(u'Unable to perform URL substitution on answers[%s]: %s', + answer_id, answers[answer_id]) new_answer = {answer_id: answers[answer_id]} new_answers.update(new_answer) @@ -693,7 +697,7 @@ class CapaModule(CapaFields, XModule): # will return (key, '', '') # We detect this and raise an error if not name: - raise ValueError("%s must contain at least one underscore" % str(key)) + raise ValueError(u"{key} must contain at least one underscore".format(key=key)) else: # This allows for answers which require more than one value for @@ -711,7 +715,7 @@ class CapaModule(CapaFields, XModule): # If the name already exists, then we don't want # to override it. Raise an error instead if name in answers: - raise ValueError("Key %s already exists in answers dict" % str(name)) + raise ValueError(u"Key {name} already exists in answers dict".format(name=name)) else: answers[name] = val @@ -759,7 +763,8 @@ class CapaModule(CapaFields, XModule): prev_submit_time = self.lcp.get_recentmost_queuetime() waittime_between_requests = self.system.xqueue['waittime'] if (current_time - prev_submit_time).total_seconds() < waittime_between_requests: - msg = 'You must wait at least %d seconds between submissions' % waittime_between_requests + msg = u'You must wait at least {wait} seconds between submissions'.format( + wait=waittime_between_requests) return {'success': msg, 'html': ''} # Prompts a modal dialog in ajax callback try: @@ -776,7 +781,7 @@ class CapaModule(CapaFields, XModule): # the full exception, including traceback, # in the response if self.system.user_is_staff: - msg = "Staff debug info: %s" % traceback.format_exc() + msg = u"Staff debug info: {tb}".format(tb=cgi.escape(traceback.format_exc())) # Otherwise, display just an error message, # without a stack trace @@ -787,7 +792,7 @@ class CapaModule(CapaFields, XModule): except Exception as err: if self.system.DEBUG: - msg = "Error checking problem: " + str(err) + msg = "Error checking problem: " + err.message msg += '\nTraceback:\n' + traceback.format_exc() return {'success': msg} raise From b68e1e207e3fb99980de4eb8bf8b904a7ceabb13 Mon Sep 17 00:00:00 2001 From: Peter Baratta Date: Tue, 18 Jun 2013 13:24:22 -0400 Subject: [PATCH 03/55] Fix some line lengths to make pylint happy --- common/lib/xmodule/xmodule/capa_module.py | 45 ++++++++++++++++------- 1 file changed, 31 insertions(+), 14 deletions(-) diff --git a/common/lib/xmodule/xmodule/capa_module.py b/common/lib/xmodule/xmodule/capa_module.py index 3bd8331678..40f685baee 100644 --- a/common/lib/xmodule/xmodule/capa_module.py +++ b/common/lib/xmodule/xmodule/capa_module.py @@ -22,7 +22,7 @@ from xblock.core import Scope, String, Boolean, Dict, Integer, Float from .fields import Timedelta, Date from django.utils.timezone import UTC -log = logging.getLogger("mitx.courseware") +log = logging.getLogger("mitx.courseware") # pylint: disable=C0103 # Generate this many different variants of problems with rerandomize=per_student @@ -65,17 +65,23 @@ class ComplexEncoder(json.JSONEncoder): class CapaFields(object): - attempts = Integer(help="Number of attempts taken by the student on this problem", default=0, scope=Scope.user_state) + attempts = Integer(help="Number of attempts taken by the student on this problem", + default=0, scope=Scope.user_state) max_attempts = Integer( display_name="Maximum Attempts", - help="Defines the number of times a student can try to answer this problem. If the value is not set, infinite attempts are allowed.", + help=("Defines the number of times a student can try to answer this problem. " + "If the value is not set, infinite attempts are allowed."), values={"min": 0}, scope=Scope.settings ) due = Date(help="Date that this problem is due by", scope=Scope.settings) - graceperiod = Timedelta(help="Amount of time after the due date that submissions will be accepted", scope=Scope.settings) + graceperiod = Timedelta( + help="Amount of time after the due date that submissions will be accepted", + scope=Scope.settings + ) showanswer = String( display_name="Show Answer", - help="Defines when to show the answer to the problem. A default value can be set in Advanced Settings.", + help=("Defines when to show the answer to the problem. " + "A default value can be set in Advanced Settings."), scope=Scope.settings, default="closed", values=[ {"display_name": "Always", "value": "always"}, @@ -86,23 +92,33 @@ class CapaFields(object): {"display_name": "Past Due", "value": "past_due"}, {"display_name": "Never", "value": "never"}] ) - force_save_button = Boolean(help="Whether to force the save button to appear on the page", scope=Scope.settings, default=False) + force_save_button = Boolean( + help="Whether to force the save button to appear on the page", + scope=Scope.settings, default=False + ) rerandomize = Randomization( - display_name="Randomization", help="Defines how often inputs are randomized when a student loads the problem. This setting only applies to problems that can have randomly generated numeric values. A default value can be set in Advanced Settings.", - default="always", scope=Scope.settings, values=[{"display_name": "Always", "value": "always"}, - {"display_name": "On Reset", "value": "onreset"}, - {"display_name": "Never", "value": "never"}, - {"display_name": "Per Student", "value": "per_student"}] + display_name="Randomization", + help="Defines how often inputs are randomized when a student loads the problem. " + "This setting only applies to problems that can have randomly generated numeric values. " + "A default value can be set in Advanced Settings.", + default="always", scope=Scope.settings, values=[ + {"display_name": "Always", "value": "always"}, + {"display_name": "On Reset", "value": "onreset"}, + {"display_name": "Never", "value": "never"}, + {"display_name": "Per Student", "value": "per_student"} + ] ) data = String(help="XML data for the problem", scope=Scope.content) - correct_map = Dict(help="Dictionary with the correctness of current student answers", scope=Scope.user_state, default={}) + correct_map = Dict(help="Dictionary with the correctness of current student answers", + scope=Scope.user_state, default={}) input_state = Dict(help="Dictionary for maintaining the state of inputtypes", scope=Scope.user_state) student_answers = Dict(help="Dictionary with the current student responses", scope=Scope.user_state) done = Boolean(help="Whether the student has answered the problem", scope=Scope.user_state) seed = Integer(help="Random seed for this student", scope=Scope.user_state) weight = Float( display_name="Problem Weight", - help="Defines the number of points each problem is worth. If the value is not set, each response field in the problem is worth one point.", + help=("Defines the number of points each problem is worth. " + "If the value is not set, each response field in the problem is worth one point."), values={"min": 0, "step": .1}, scope=Scope.settings ) @@ -998,7 +1014,8 @@ class CapaDescriptor(CapaFields, RawDescriptor): mako_template = "widgets/problem-edit.html" js = {'coffee': [resource_string(__name__, 'js/src/problem/edit.coffee')]} js_module_name = "MarkdownEditingDescriptor" - css = {'scss': [resource_string(__name__, 'css/editor/edit.scss'), resource_string(__name__, 'css/problem/edit.scss')]} + css = {'scss': [resource_string(__name__, 'css/editor/edit.scss'), + resource_string(__name__, 'css/problem/edit.scss')]} # Capa modules have some additional metadata: # TODO (vshnayder): do problems have any other metadata? Do they From f623e42983545f99a0cf7bd69e7bccccb55e285e Mon Sep 17 00:00:00 2001 From: Peter Baratta Date: Tue, 18 Jun 2013 15:48:55 -0400 Subject: [PATCH 04/55] Fix formatting of docstrings; add more docstrings --- common/lib/xmodule/xmodule/capa_module.py | 170 ++++++++++++------ .../xmodule/xmodule/tests/test_capa_module.py | 16 +- 2 files changed, 126 insertions(+), 60 deletions(-) diff --git a/common/lib/xmodule/xmodule/capa_module.py b/common/lib/xmodule/xmodule/capa_module.py index 40f685baee..b927106b4a 100644 --- a/common/lib/xmodule/xmodule/capa_module.py +++ b/common/lib/xmodule/xmodule/capa_module.py @@ -47,7 +47,13 @@ def randomization_bin(seed, problem_id): class Randomization(String): + """ + Define a field to store how to randomize a problem. + """ def from_json(self, value): + """ + For backward compatability? + """ if value in ("", "true"): return "always" elif value == "false": @@ -58,13 +64,22 @@ class Randomization(String): class ComplexEncoder(json.JSONEncoder): + """ + Extend the JSON encoder to correctly handle complex numbers + """ def default(self, obj): + """ + Print a nicely formatted complex number, or default to the JSON encoder + """ if isinstance(obj, complex): return u"{real:.7g}{imag:+.7g}*j".format(real=obj.real, imag=obj.imag) return json.JSONEncoder.default(self, obj) class CapaFields(object): + """ + Define the possible fields for a Capa problem + """ attempts = Integer(help="Number of attempts taken by the student on this problem", default=0, scope=Scope.user_state) max_attempts = Integer( @@ -130,12 +145,12 @@ class CapaFields(object): class CapaModule(CapaFields, XModule): - ''' + """ An XModule implementing LonCapa format problems, implemented by way of capa.capa_problem.LoncapaProblem CapaModule.__init__ takes the same arguments as xmodule.x_module:XModule.__init__ - ''' + """ icon_class = 'problem' js = {'coffee': [resource_string(__name__, 'js/src/capa/display.coffee'), @@ -150,7 +165,9 @@ class CapaModule(CapaFields, XModule): css = {'scss': [resource_string(__name__, 'css/capa/display.scss')]} def __init__(self, *args, **kwargs): - """ Accepts the same arguments as xmodule.x_module:XModule.__init__ """ + """ + Accepts the same arguments as xmodule.x_module:XModule.__init__ + """ XModule.__init__(self, *args, **kwargs) due_date = self.due @@ -211,7 +228,9 @@ class CapaModule(CapaFields, XModule): assert self.seed is not None def choose_new_seed(self): - """Choose a new seed.""" + """ + Choose a new seed. + """ if self.rerandomize == 'never': self.seed = 1 elif self.rerandomize == "per_student" and hasattr(self.system, 'seed'): @@ -225,6 +244,9 @@ class CapaModule(CapaFields, XModule): self.seed %= MAX_RANDOMIZATION_BINS def new_lcp(self, state, text=None): + """ + Generate a new Loncapa Problem + """ if text is None: text = self.data @@ -237,6 +259,9 @@ class CapaModule(CapaFields, XModule): ) def get_state_for_lcp(self): + """ + Give a dictionary holding the state of the module + """ return { 'done': self.done, 'correct_map': self.correct_map, @@ -246,6 +271,9 @@ class CapaModule(CapaFields, XModule): } def set_state_from_lcp(self): + """ + Set the module's state from the settings in `self.lcp` + """ lcp_state = self.lcp.get_state() self.done = lcp_state['done'] self.correct_map = lcp_state['correct_map'] @@ -254,26 +282,36 @@ class CapaModule(CapaFields, XModule): self.seed = lcp_state['seed'] def get_score(self): + """ + Access the problem's score + """ return self.lcp.get_score() def max_score(self): + """ + Access the problem's max score + """ return self.lcp.get_max_score() def get_progress(self): - ''' For now, just return score / max_score - ''' + """ + For now, just return score / max_score + """ d = self.get_score() score = d['score'] total = d['total'] if total > 0: try: return Progress(score, total) - except Exception: + except (TypeError, ValueError): log.exception("Got bad progress") return None return None def get_html(self): + """ + Return some html with data about the module + """ return self.system.render_template('problem_ajax.html', { 'element_id': self.location.html_id(), 'id': self.id, @@ -284,6 +322,7 @@ class CapaModule(CapaFields, XModule): def check_button_name(self): """ Determine the name for the "check" button. + Usually it is just "Check", but if this is the student's final attempt, change the name to "Final Check" """ @@ -369,12 +408,12 @@ class CapaModule(CapaFields, XModule): def handle_problem_html_error(self, err): """ - Change our problem to a dummy problem containing - a warning message to display to users. + Create a dummy problem to represent any errors. - Returns the HTML to show to users + Change our problem to a dummy problem containing a warning message to + display to users. Returns the HTML to show to users - *err* is the Exception encountered while rendering the problem HTML. + `err` is the Exception encountered while rendering the problem HTML. """ log.exception(err) @@ -434,8 +473,12 @@ class CapaModule(CapaFields, XModule): return html def get_problem_html(self, encapsulate=True): - '''Return html for the problem. Adds check, reset, save buttons - as necessary based on the problem config and state.''' + """ + Return html for the problem. + + Adds check, reset, save buttons as necessary based on the problem config + and state. + """ try: html = self.lcp.get_html() @@ -480,15 +523,16 @@ class CapaModule(CapaFields, XModule): return self.system.replace_urls(html) def handle_ajax(self, dispatch, get): - ''' + """ This is called by courseware.module_render, to handle an AJAX call. - "get" is request.POST. + + `get` is request.POST. Returns a json dictionary: { 'progress_changed' : True/False, 'progress' : 'none'/'in_progress'/'done', } - ''' + """ handlers = { 'problem_get': self.get_problem, 'problem_check': self.check_problem, @@ -527,7 +571,9 @@ class CapaModule(CapaFields, XModule): datetime.datetime.now(UTC()) > self.close_date) def closed(self): - ''' Is the student still allowed to submit answers? ''' + """ + Is the student still allowed to submit answers? + """ if self.max_attempts is not None and self.attempts >= self.max_attempts: return True if self.is_past_due(): @@ -546,18 +592,24 @@ class CapaModule(CapaFields, XModule): return self.lcp.done def is_attempted(self): - """Used by conditional module""" + """ + Has the problem been attempted? + + used by conditional module + """ return self.attempts > 0 def is_correct(self): - """True if full points""" + """ + True iff full points + """ d = self.get_score() return d['score'] == d['total'] def answer_available(self): - ''' + """ Is the user allowed to see an answer? - ''' + """ if self.showanswer == '': return False elif self.showanswer == "never": @@ -589,7 +641,7 @@ class CapaModule(CapaFields, XModule): Delivers grading response (e.g. from asynchronous code checking) to the capa problem, so its score can be updated - 'get' must have a field 'response' which is a string that contains the + `get` must have a field `response` which is a string that contains the grader's response No ajax return is needed. Return empty dict. @@ -603,7 +655,7 @@ class CapaModule(CapaFields, XModule): return dict() # No AJAX return is needed def handle_ungraded_response(self, get): - ''' + """ Delivers a response from the XQueue to the capa problem The score of the problem will not be updated @@ -616,7 +668,7 @@ class CapaModule(CapaFields, XModule): empty dictionary No ajax return is needed, so an empty dict is returned - ''' + """ queuekey = get['queuekey'] score_msg = get['xqueue_body'] # pass along the xqueue message to the problem @@ -625,25 +677,25 @@ class CapaModule(CapaFields, XModule): return dict() def handle_input_ajax(self, get): - ''' + """ Handle ajax calls meant for a particular input in the problem Args: - get (dict) - data that should be passed to the input Returns: - dict containing the response from the input - ''' + """ response = self.lcp.handle_input_ajax(get) # save any state changes that may occur self.set_state_from_lcp() return response def get_answer(self, get): - ''' + """ For the "show answer" button. Returns the answers: {'answers' : answers} - ''' + """ event_info = dict() event_info['problem_id'] = self.location.url() self.system.track_function('showanswer', event_info) @@ -669,40 +721,44 @@ class CapaModule(CapaFields, XModule): # Figure out if we should move these to capa_problem? def get_problem(self, get): - ''' Return results of get_problem_html, as a simple dict for json-ing. + """ + Return results of get_problem_html, as a simple dict for json-ing. + { 'html': } - Used if we want to reconfirm we have the right thing e.g. after - several AJAX calls. - ''' + Used if we want to reconfirm we have the right thing e.g. after + several AJAX calls. + """ return {'html': self.get_problem_html(encapsulate=False)} @staticmethod def make_dict_of_responses(get): - '''Make dictionary of student responses (aka "answers") - get is POST dictionary (Django QueryDict). + """ + Make dictionary of student responses (aka "answers") - The *get* dict has keys of the form 'x_y', which are mapped + `get` is POST dictionary (Django QueryDict). + + The `get` dict has keys of the form 'x_y', which are mapped to key 'y' in the returned dict. For example, 'input_1_2_3' would be mapped to '1_2_3' in the returned dict. Some inputs always expect a list in the returned dict (e.g. checkbox inputs). The convention is that - keys in the *get* dict that end with '[]' will always + keys in the `get` dict that end with '[]' will always have list values in the returned dict. - For example, if the *get* dict contains {'input_1[]': 'test' } + For example, if the `get` dict contains {'input_1[]': 'test' } then the output dict would contain {'1': ['test'] } (the value is a list). Raises an exception if: - A key in the *get* dictionary does not contain >= 1 underscores - (e.g. "input" is invalid; "input_1" is valid) + -A key in the `get` dictionary does not contain at least one underscore + (e.g. "input" is invalid, but "input_1" is valid) - Two keys end up with the same name in the returned dict. - (e.g. 'input_1' and 'input_1[]', which both get mapped - to 'input_1' in the returned dict) - ''' + -Two keys end up with the same name in the returned dict. + (e.g. 'input_1' and 'input_1[]', which both get mapped to 'input_1' + in the returned dict) + """ answers = dict() for key in get: @@ -749,12 +805,13 @@ class CapaModule(CapaFields, XModule): }) def check_problem(self, get): - ''' Checks whether answers to a problem are correct, and - returns a map of correct/incorrect answers: + """ + Checks whether answers to a problem are correct - {'success' : 'correct' | 'incorrect' | AJAX alert msg string, - 'contents' : html} - ''' + Returns a map of correct/incorrect answers: + {'success' : 'correct' | 'incorrect' | AJAX alert msg string, + 'contents' : html} + """ event_info = dict() event_info['state'] = self.lcp.get_state() event_info['problem_id'] = self.location.url() @@ -958,16 +1015,17 @@ class CapaModule(CapaFields, XModule): 'msg': msg} def reset_problem(self, get): - ''' Changes problem state to unfinished -- removes student answers, - and causes problem to rerender itself. + """ + Changes problem state to unfinished -- removes student answers, + and causes problem to rerender itself. - Returns a dictionary of the form: - {'success': True/False, - 'html': Problem HTML string } + Returns a dictionary of the form: + {'success': True/False, + 'html': Problem HTML string } - If an error occurs, the dictionary will also have an - 'error' key containing an error message. - ''' + If an error occurs, the dictionary will also have an + `error` key containing an error message. + """ event_info = dict() event_info['old_state'] = self.lcp.get_state() event_info['problem_id'] = self.location.url() diff --git a/common/lib/xmodule/xmodule/tests/test_capa_module.py b/common/lib/xmodule/xmodule/tests/test_capa_module.py index 85e69cabc1..81df686015 100644 --- a/common/lib/xmodule/xmodule/tests/test_capa_module.py +++ b/common/lib/xmodule/xmodule/tests/test_capa_module.py @@ -1,5 +1,7 @@ # -*- coding: utf-8 -*- -"""Tests of the Capa XModule""" +""" +Tests of the Capa XModule +""" #pylint: disable=C0111 #pylint: disable=R0904 #pylint: disable=C0103 @@ -48,12 +50,16 @@ class CapaFactory(object): @staticmethod def input_key(): - """ Return the input key to use when passing GET parameters """ + """ + Return the input key to use when passing GET parameters + """ return ("input_" + CapaFactory.answer_key()) @staticmethod def answer_key(): - """ Return the key stored in the capa problem answer dict """ + """ + Return the key stored in the capa problem answer dict + """ return ("-".join(['i4x', 'edX', 'capa_test', 'problem', 'SampleProblem%d' % CapaFactory.num]) + "_2_1") @@ -362,7 +368,9 @@ class CapaModuleTest(unittest.TestCase): result = CapaModule.make_dict_of_responses(invalid_get_dict) def _querydict_from_dict(self, param_dict): - """ Create a Django QueryDict from a Python dictionary """ + """ + Create a Django QueryDict from a Python dictionary + """ # QueryDict objects are immutable by default, so we make # a copy that we can update. From e4af7287b6f204dc759f1e9a349bf29a6864a025 Mon Sep 17 00:00:00 2001 From: JonahStanley Date: Tue, 18 Jun 2013 14:04:43 -0400 Subject: [PATCH 05/55] Initial testing for parallelization --- cms/djangoapps/contentstore/tests/test_contentstore.py | 6 ++++++ cms/envs/test.py | 2 +- .../lib/xmodule/xmodule/modulestore/tests/django_utils.py | 1 + 3 files changed, 8 insertions(+), 1 deletion(-) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index d24deacecf..86699ef479 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -43,10 +43,13 @@ from django_comment_common.utils import are_permissions_roles_seeded from xmodule.exceptions import InvalidVersionError import datetime from pytz import UTC +#from uuid import uuid4 TEST_DATA_MODULESTORE = copy.deepcopy(settings.MODULESTORE) TEST_DATA_MODULESTORE['default']['OPTIONS']['fs_root'] = path('common/test/data') TEST_DATA_MODULESTORE['direct']['OPTIONS']['fs_root'] = path('common/test/data') +TEST_DATA_CONTENTSTORE = copy.deepcopy(settings.CONTENTSTORE) +TEST_DATA_CONTENTSTORE['OPTIONS']['db'] = 'test_xcontent_%s' % 4 #uuid4().hex class MongoCollectionFindWrapper(object): @@ -60,6 +63,7 @@ class MongoCollectionFindWrapper(object): @override_settings(MODULESTORE=TEST_DATA_MODULESTORE) +@override_settings(CONTENTSTORE=TEST_DATA_CONTENTSTORE) class ContentStoreToyCourseTest(ModuleStoreTestCase): """ Tests that rely on the toy courses. @@ -83,6 +87,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): self.client = Client() self.client.login(username=uname, password=password) + def check_components_on_page(self, component_types, expected_types): """ Ensure that the right types end up on the page. @@ -809,6 +814,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): export_to_xml(module_store, content_store, location, root_dir, 'test_export') +@override_settings(CONTENTSTORE=TEST_DATA_CONTENTSTORE) class ContentStoreTest(ModuleStoreTestCase): """ Tests for the CMS ContentStore application. diff --git a/cms/envs/test.py b/cms/envs/test.py index 954a553e10..89813dd937 100644 --- a/cms/envs/test.py +++ b/cms/envs/test.py @@ -70,7 +70,7 @@ CONTENTSTORE = { 'ENGINE': 'xmodule.contentstore.mongo.MongoContentStore', 'OPTIONS': { 'host': 'localhost', - 'db': 'test_xmodule', + 'db': 'test_xcontent', }, # allow for additional options that can be keyed on a name, e.g. 'trashcan' 'ADDITIONAL_OPTIONS': { diff --git a/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py b/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py index 04e79ce521..e0e5c1a46f 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py @@ -27,6 +27,7 @@ class ModuleStoreTestCase(TestCase): # Remove everything except templates modulestore.collection.remove(query) + modulestore.collection.drop() @staticmethod def load_templates_if_necessary(): From f90ed69cd792091c9f2d57bce1cbafe0efd51094 Mon Sep 17 00:00:00 2001 From: Jay Zoldak Date: Tue, 18 Jun 2013 15:09:53 -0400 Subject: [PATCH 06/55] move override of MODULESTORE settings into ModuleStore test case class --- cms/djangoapps/contentstore/tests/test_contentstore.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 86699ef479..9c3ec2e3ba 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -45,9 +45,7 @@ import datetime from pytz import UTC #from uuid import uuid4 -TEST_DATA_MODULESTORE = copy.deepcopy(settings.MODULESTORE) -TEST_DATA_MODULESTORE['default']['OPTIONS']['fs_root'] = path('common/test/data') -TEST_DATA_MODULESTORE['direct']['OPTIONS']['fs_root'] = path('common/test/data') + TEST_DATA_CONTENTSTORE = copy.deepcopy(settings.CONTENTSTORE) TEST_DATA_CONTENTSTORE['OPTIONS']['db'] = 'test_xcontent_%s' % 4 #uuid4().hex @@ -62,7 +60,7 @@ class MongoCollectionFindWrapper(object): return self.original(query, *args, **kwargs) -@override_settings(MODULESTORE=TEST_DATA_MODULESTORE) +#@override_settings(MODULESTORE=TEST_DATA_MODULESTORE) @override_settings(CONTENTSTORE=TEST_DATA_CONTENTSTORE) class ContentStoreToyCourseTest(ModuleStoreTestCase): """ @@ -70,6 +68,9 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): TODO: refactor using CourseFactory so they do not. """ def setUp(self): + + settings.MODULESTORE['default']['OPTIONS']['fs_root'] = path('common/test/data') + settings.MODULESTORE['direct']['OPTIONS']['fs_root'] = path('common/test/data') uname = 'testuser' email = 'test+courses@edx.org' password = 'foo' @@ -88,6 +89,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): self.client.login(username=uname, password=password) + def check_components_on_page(self, component_types, expected_types): """ Ensure that the right types end up on the page. From 51f8c0cfebedb7807b04ef849cfb806a0dcdba0e Mon Sep 17 00:00:00 2001 From: JonahStanley Date: Wed, 19 Jun 2013 11:27:22 -0400 Subject: [PATCH 07/55] Added the beginnings of self cleanup --- .../contentstore/tests/test_contentstore.py | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 9c3ec2e3ba..46d6a069ce 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -43,11 +43,12 @@ from django_comment_common.utils import are_permissions_roles_seeded from xmodule.exceptions import InvalidVersionError import datetime from pytz import UTC -#from uuid import uuid4 +from uuid import uuid4 +import pymongo TEST_DATA_CONTENTSTORE = copy.deepcopy(settings.CONTENTSTORE) -TEST_DATA_CONTENTSTORE['OPTIONS']['db'] = 'test_xcontent_%s' % 4 #uuid4().hex +TEST_DATA_CONTENTSTORE['OPTIONS']['db'] = 'test_xcontent_%s' % uuid4().hex class MongoCollectionFindWrapper(object): @@ -88,7 +89,10 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): self.client = Client() self.client.login(username=uname, password=password) - + def tearDown(self): + m = pymongo.MongoClient() + m.drop_database(TEST_DATA_CONTENTSTORE['OPTIONS']['db']) + #contentstore().fs_files.drop() def check_components_on_page(self, component_types, expected_types): """ @@ -449,7 +453,6 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): content_store = contentstore() trash_store = contentstore('trashcan') module_store = modulestore('direct') - import_from_xml(module_store, 'common/test/data/', ['full'], static_content_store=content_store) # look up original (and thumbnail) in content store, should be there after import @@ -853,6 +856,11 @@ class ContentStoreTest(ModuleStoreTestCase): 'display_name': 'Robot Super Course', } + def tearDown(self): + m = pymongo.MongoClient() + m.drop_database(TEST_DATA_CONTENTSTORE['OPTIONS']['db']) + #contentstore().fs_files.drop() + def test_create_course(self): """Test new course creation - happy path""" resp = self.client.post(reverse('create_new_course'), self.course_data) From fa18b48f6eaec45bc65f16f9585fa2555462ad55 Mon Sep 17 00:00:00 2001 From: JonahStanley Date: Thu, 20 Jun 2013 09:05:35 -0400 Subject: [PATCH 08/55] Contentstore singleton is now cleared during teardown --- cms/djangoapps/contentstore/tests/test_contentstore.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 46d6a069ce..b0cbcee032 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -41,6 +41,7 @@ from xmodule.exceptions import NotFoundError from django_comment_common.utils import are_permissions_roles_seeded from xmodule.exceptions import InvalidVersionError +import xmodule.contentstore.django import datetime from pytz import UTC from uuid import uuid4 @@ -92,7 +93,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): def tearDown(self): m = pymongo.MongoClient() m.drop_database(TEST_DATA_CONTENTSTORE['OPTIONS']['db']) - #contentstore().fs_files.drop() + xmodule.contentstore.django._CONTENTSTORE.clear() def check_components_on_page(self, component_types, expected_types): """ @@ -859,7 +860,7 @@ class ContentStoreTest(ModuleStoreTestCase): def tearDown(self): m = pymongo.MongoClient() m.drop_database(TEST_DATA_CONTENTSTORE['OPTIONS']['db']) - #contentstore().fs_files.drop() + xmodule.contentstore.django._CONTENTSTORE.clear() def test_create_course(self): """Test new course creation - happy path""" From cb04f9f0b82dfe46777ceb0584fadb656f7b6780 Mon Sep 17 00:00:00 2001 From: JonahStanley Date: Thu, 20 Jun 2013 17:10:36 -0400 Subject: [PATCH 09/55] Moved port range to rake file --- jenkins/test.sh | 3 --- rakelib/tests.rake | 2 +- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/jenkins/test.sh b/jenkins/test.sh index e5ac4f6f71..2ff32a9911 100755 --- a/jenkins/test.sh +++ b/jenkins/test.sh @@ -60,9 +60,6 @@ fi export PIP_DOWNLOAD_CACHE=/mnt/pip-cache -# Allow django liveserver tests to use a range of ports -export DJANGO_LIVE_TEST_SERVER_ADDRESS=${DJANGO_LIVE_TEST_SERVER_ADDRESS-localhost:8000-9000} - source /mnt/virtualenvs/"$JOB_NAME"/bin/activate bundle install diff --git a/rakelib/tests.rake b/rakelib/tests.rake index f169d28256..c0592cca7a 100644 --- a/rakelib/tests.rake +++ b/rakelib/tests.rake @@ -16,7 +16,7 @@ def run_tests(system, report_dir, test_id=nil, stop_on_failure=true) ENV['NOSE_XUNIT_FILE'] = File.join(report_dir, "nosetests.xml") dirs = Dir["common/djangoapps/*"] + Dir["#{system}/djangoapps/*"] test_id = dirs.join(' ') if test_id.nil? or test_id == '' - cmd = django_admin(system, :test, 'test', '--logging-clear-handlers', test_id) + cmd = django_admin(system, :test, 'test', '--logging-clear-handlers', '--liveserver=localhost:8000-9000', test_id) test_sh(run_under_coverage(cmd, system)) end From 7db93976c5860cf818bc915bb890b1a9c18b6838 Mon Sep 17 00:00:00 2001 From: Peter Baratta Date: Fri, 21 Jun 2013 11:02:25 -0400 Subject: [PATCH 10/55] PR fixes --- common/lib/xmodule/xmodule/capa_module.py | 9 +++------ .../lib/xmodule/xmodule/tests/test_capa_module.py | 14 ++++++++------ 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/common/lib/xmodule/xmodule/capa_module.py b/common/lib/xmodule/xmodule/capa_module.py index b927106b4a..d740a73946 100644 --- a/common/lib/xmodule/xmodule/capa_module.py +++ b/common/lib/xmodule/xmodule/capa_module.py @@ -22,7 +22,7 @@ from xblock.core import Scope, String, Boolean, Dict, Integer, Float from .fields import Timedelta, Date from django.utils.timezone import UTC -log = logging.getLogger("mitx.courseware") # pylint: disable=C0103 +log = logging.getLogger("mitx.courseware") # Generate this many different variants of problems with rerandomize=per_student @@ -51,9 +51,6 @@ class Randomization(String): Define a field to store how to randomize a problem. """ def from_json(self, value): - """ - For backward compatability? - """ if value in ("", "true"): return "always" elif value == "false": @@ -865,8 +862,8 @@ class CapaModule(CapaFields, XModule): except Exception as err: if self.system.DEBUG: - msg = "Error checking problem: " + err.message - msg += '\nTraceback:\n' + traceback.format_exc() + msg = u"Error checking problem: {}".format(err.message) + msg += u'\nTraceback:\n{}'.format(traceback.format_exc()) return {'success': msg} raise diff --git a/common/lib/xmodule/xmodule/tests/test_capa_module.py b/common/lib/xmodule/xmodule/tests/test_capa_module.py index 81df686015..c6ffd32e89 100644 --- a/common/lib/xmodule/xmodule/tests/test_capa_module.py +++ b/common/lib/xmodule/xmodule/tests/test_capa_module.py @@ -505,9 +505,10 @@ class CapaModuleTest(unittest.TestCase): def test_check_problem_error(self): # Try each exception that capa_module should handle - for exception_class in [StudentInputError, - LoncapaProblemError, - ResponseError]: + exception_classes = [StudentInputError, + LoncapaProblemError, + ResponseError] + for exception_class in exception_classes: # Create the module module = CapaFactory.create(attempts=1) @@ -532,9 +533,10 @@ class CapaModuleTest(unittest.TestCase): def test_check_problem_error_nonascii(self): # Try each exception that capa_module should handle - for exception_class in [StudentInputError, - LoncapaProblemError, - ResponseError]: + exception_classes = [StudentInputError, + LoncapaProblemError, + ResponseError] + for exception_class in exception_classes: # Create the module module = CapaFactory.create(attempts=1) From 58fe6d4e8367c570648068d3307cc3105a88edf5 Mon Sep 17 00:00:00 2001 From: JonahStanley Date: Fri, 21 Jun 2013 16:17:33 -0400 Subject: [PATCH 11/55] Cleaned up import and comment --- cms/djangoapps/contentstore/tests/test_contentstore.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index b0cbcee032..6d2055d459 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -45,7 +45,7 @@ import xmodule.contentstore.django import datetime from pytz import UTC from uuid import uuid4 -import pymongo +from pymongo import MongoClient TEST_DATA_CONTENTSTORE = copy.deepcopy(settings.CONTENTSTORE) @@ -62,7 +62,6 @@ class MongoCollectionFindWrapper(object): return self.original(query, *args, **kwargs) -#@override_settings(MODULESTORE=TEST_DATA_MODULESTORE) @override_settings(CONTENTSTORE=TEST_DATA_CONTENTSTORE) class ContentStoreToyCourseTest(ModuleStoreTestCase): """ @@ -91,7 +90,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): self.client.login(username=uname, password=password) def tearDown(self): - m = pymongo.MongoClient() + m = MongoClient() m.drop_database(TEST_DATA_CONTENTSTORE['OPTIONS']['db']) xmodule.contentstore.django._CONTENTSTORE.clear() @@ -858,7 +857,7 @@ class ContentStoreTest(ModuleStoreTestCase): } def tearDown(self): - m = pymongo.MongoClient() + m = MongoClient() m.drop_database(TEST_DATA_CONTENTSTORE['OPTIONS']['db']) xmodule.contentstore.django._CONTENTSTORE.clear() From 5e6de488abaa45f765b5aef48a1b36851a673be1 Mon Sep 17 00:00:00 2001 From: JonahStanley Date: Fri, 21 Jun 2013 16:28:32 -0400 Subject: [PATCH 12/55] Fixed pylint/pep8 violations --- .../contentstore/tests/test_contentstore.py | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 6d2055d459..514b631521 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -90,8 +90,8 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): self.client.login(username=uname, password=password) def tearDown(self): - m = MongoClient() - m.drop_database(TEST_DATA_CONTENTSTORE['OPTIONS']['db']) + mongo = MongoClient() + mongo.drop_database(TEST_DATA_CONTENTSTORE['OPTIONS']['db']) xmodule.contentstore.django._CONTENTSTORE.clear() def check_components_on_page(self, component_types, expected_types): @@ -414,7 +414,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): self.assertGreater(len(all_assets), 0) # make sure we have some thumbnails in our contentstore - all_thumbnails = content_store.get_all_content_thumbnails_for_course(course_location) + content_store.get_all_content_thumbnails_for_course(course_location) # # cdodge: temporarily comment out assertion on thumbnails because many environments @@ -543,7 +543,6 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): all_assets = trash_store.get_all_content_for_course(course_location) self.assertEqual(len(all_assets), 0) - all_thumbnails = trash_store.get_all_content_thumbnails_for_course(course_location) self.assertEqual(len(all_thumbnails), 0) @@ -608,7 +607,6 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): self.assertRaises(InvalidVersionError, draft_store.unpublish, location) - 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) @@ -857,8 +855,8 @@ class ContentStoreTest(ModuleStoreTestCase): } def tearDown(self): - m = MongoClient() - m.drop_database(TEST_DATA_CONTENTSTORE['OPTIONS']['db']) + mongo = MongoClient() + mongo.drop_database(TEST_DATA_CONTENTSTORE['OPTIONS']['db']) xmodule.contentstore.django._CONTENTSTORE.clear() def test_create_course(self): From 3f9a72e6ce805a63d091cc387b44021d079d46c4 Mon Sep 17 00:00:00 2001 From: JonahStanley Date: Fri, 21 Jun 2013 16:32:13 -0400 Subject: [PATCH 13/55] Consolidated imports --- cms/djangoapps/contentstore/tests/test_contentstore.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 514b631521..66fead562e 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -23,7 +23,7 @@ from xmodule.modulestore import Location from xmodule.modulestore.store_utilities import clone_course from xmodule.modulestore.store_utilities import delete_course from xmodule.modulestore.django import modulestore -from xmodule.contentstore.django import contentstore +from xmodule.contentstore.django import contentstore, _CONTENTSTORE from xmodule.templates import update_templates from xmodule.modulestore.xml_exporter import export_to_xml from xmodule.modulestore.xml_importer import import_from_xml, perform_xlint @@ -41,7 +41,6 @@ from xmodule.exceptions import NotFoundError from django_comment_common.utils import are_permissions_roles_seeded from xmodule.exceptions import InvalidVersionError -import xmodule.contentstore.django import datetime from pytz import UTC from uuid import uuid4 @@ -92,7 +91,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): def tearDown(self): mongo = MongoClient() mongo.drop_database(TEST_DATA_CONTENTSTORE['OPTIONS']['db']) - xmodule.contentstore.django._CONTENTSTORE.clear() + _CONTENTSTORE.clear() def check_components_on_page(self, component_types, expected_types): """ @@ -857,7 +856,7 @@ class ContentStoreTest(ModuleStoreTestCase): def tearDown(self): mongo = MongoClient() mongo.drop_database(TEST_DATA_CONTENTSTORE['OPTIONS']['db']) - xmodule.contentstore.django._CONTENTSTORE.clear() + _CONTENTSTORE.clear() def test_create_course(self): """Test new course creation - happy path""" From fa9a8f4af09a27bd88aeea33a81ec0f5086d9363 Mon Sep 17 00:00:00 2001 From: Julian Arni Date: Fri, 21 Jun 2013 18:00:30 -0400 Subject: [PATCH 14/55] Greater dir naming flexibility. Accepts any dirname for the edx-platform repo. Allows the script to be called from any directory, not just $BASE/edx-platform. --- scripts/create-dev-env.sh | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/scripts/create-dev-env.sh b/scripts/create-dev-env.sh index edb0bcdcae..0816b72d21 100755 --- a/scripts/create-dev-env.sh +++ b/scripts/create-dev-env.sh @@ -98,19 +98,23 @@ clone_repos() { set_base_default() { # if PROJECT_HOME not set # 2 possibilities: this is from cloned repo, or not - # this script is in "./scripts" if a git clone - this_repo=$(cd "${BASH_SOURCE%/*}/.." && pwd) - if [[ "${this_repo##*/}" = "edx-platform" && -d "$this_repo/.git" ]]; then - # set BASE one-up from this_repo; - echo "${this_repo%/*}" + + # See if remote's url is named edx-platform (this works for forks too, but + # not if the name was changed). + cd "$( dirname "${BASH_SOURCE[0]}" )" + this_repo=$(basename $(git ls-remote --get-url 2>/dev/null) 2>/dev/null) || + echo -n "" + + if [[ "x$this_repo" = "xedx-platform.git" ]]; then + # We are in the edx repo and already have git installed. Let git do the + # work of finding base dir: + echo "$(dirname $(git rev-parse --show-toplevel))" else echo "$HOME/edx_all" fi } - - ### START PROG=${0##*/} From 85b4a4ccab37e14e6f0543f8b7165e667c0768ef Mon Sep 17 00:00:00 2001 From: Alexander Kryklia Date: Sat, 22 Jun 2013 16:13:40 +0300 Subject: [PATCH 15/55] removes choiceresponse wiping after clicking Show Answer --- common/lib/xmodule/xmodule/js/src/capa/display.coffee | 3 --- 1 file changed, 3 deletions(-) diff --git a/common/lib/xmodule/xmodule/js/src/capa/display.coffee b/common/lib/xmodule/xmodule/js/src/capa/display.coffee index 987d20b65a..f773fc81c4 100644 --- a/common/lib/xmodule/xmodule/js/src/capa/display.coffee +++ b/common/lib/xmodule/xmodule/js/src/capa/display.coffee @@ -364,8 +364,6 @@ class @Problem choicegroup: (element, display, answers) => element = $(element) - element.find('input').attr('disabled', 'disabled') - input_id = element.attr('id').replace(/inputtype_/,'') answer = answers[input_id] for choice in answer @@ -379,7 +377,6 @@ class @Problem inputtypeHideAnswerMethods: choicegroup: (element, display) => element = $(element) - element.find('input').attr('disabled', null) element.find('label').removeClass('choicegroup_correct') javascriptinput: (element, display) => From 4a98e2eda75b1a8b036e4f3f5e035c5049aab776 Mon Sep 17 00:00:00 2001 From: Jason Bau Date: Wed, 5 Jun 2013 23:14:18 -0700 Subject: [PATCH 16/55] Moves user activation away from just clicking on reset password To following the link in the password reset email --- common/djangoapps/student/forms.py | 72 +++++++++++++++++++ common/djangoapps/student/views.py | 31 ++++---- .../registration/password_reset_email.html | 2 +- lms/urls.py | 2 +- 4 files changed, 93 insertions(+), 14 deletions(-) create mode 100644 common/djangoapps/student/forms.py diff --git a/common/djangoapps/student/forms.py b/common/djangoapps/student/forms.py new file mode 100644 index 0000000000..75c89e0a26 --- /dev/null +++ b/common/djangoapps/student/forms.py @@ -0,0 +1,72 @@ +from django import forms +from django.utils.translation import ugettext, ugettext_lazy as _ +from django.template import loader +from django.contrib.auth.models import User +from django.contrib.auth.hashers import UNUSABLE_PASSWORD, is_password_usable, get_hasher +from django.contrib.auth.tokens import default_token_generator +from django.contrib.sites.models import get_current_site +from django.utils.http import int_to_base36 + + + +# This is a literal copy from Django 1.4.5's django.contrib.auth.forms.PasswordResetForm +# I think copy-and-paste here is somewhat better than subclassing and +# just changing the definition of clean_email, because it's less +# likely to be broken by incompatibility with a new django version. +# (If this form is good enough now, a snapshot of it ought to last a while) + +class PasswordResetFormNoActive(forms.Form): + error_messages = { + 'unknown': _("That e-mail address doesn't have an associated " + "user account. Are you sure you've registered?"), + 'unusable': _("The user account associated with this e-mail " + "address cannot reset the password."), + } + email = forms.EmailField(label=_("E-mail"), max_length=75) + + def clean_email(self): + """ + Validates that an active user exists with the given email address. + """ + email = self.cleaned_data["email"] + #The line below contains the only change, removing is_active=True + self.users_cache = User.objects.filter(email__iexact=email) + if not len(self.users_cache): + raise forms.ValidationError(self.error_messages['unknown']) + if any((user.password == UNUSABLE_PASSWORD) + for user in self.users_cache): + raise forms.ValidationError(self.error_messages['unusable']) + return email + + def save(self, domain_override=None, + subject_template_name='registration/password_reset_subject.txt', + email_template_name='registration/password_reset_email.html', + use_https=False, token_generator=default_token_generator, + from_email=None, request=None): + """ + Generates a one-use only link for resetting password and sends to the + user. + """ + from django.core.mail import send_mail + for user in self.users_cache: + if not domain_override: + current_site = get_current_site(request) + site_name = current_site.name + domain = current_site.domain + else: + site_name = domain = domain_override + c = { + 'email': user.email, + 'domain': domain, + 'site_name': site_name, + 'uid': int_to_base36(user.id), + 'user': user, + 'token': token_generator.make_token(user), + 'protocol': use_https and 'https' or 'http', + } + subject = loader.render_to_string(subject_template_name, c) + # Email subject *must not* contain newlines + subject = ''.join(subject.splitlines()) + email = loader.render_to_string(email_template_name, c) + send_mail(subject, email, from_email, [user.email]) + diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index e065333409..50f6d90368 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -11,9 +11,9 @@ import time from django.conf import settings from django.contrib.auth import logout, authenticate, login -from django.contrib.auth.forms import PasswordResetForm from django.contrib.auth.models import User from django.contrib.auth.decorators import login_required +from django.contrib.auth.views import password_reset_confirm from django.core.cache import cache from django.core.context_processors import csrf from django.core.mail import send_mail @@ -24,6 +24,7 @@ from django.http import HttpResponse, HttpResponseBadRequest, HttpResponseForbid from django.shortcuts import redirect from django_future.csrf import ensure_csrf_cookie from django.utils.http import cookie_date +from django.utils.http import base36_to_int from mitxmako.shortcuts import render_to_response, render_to_string from bs4 import BeautifulSoup @@ -34,6 +35,8 @@ from student.models import (Registration, UserProfile, TestCenterUser, TestCente CourseEnrollment, unique_id_for_user, get_testcenter_registration, CourseEnrollmentAllowed) +from student.forms import PasswordResetFormNoActive + from certificates.models import CertificateStatuses, certificate_status_for_student from xmodule.course_module import CourseDescriptor @@ -962,17 +965,7 @@ def password_reset(request): if request.method != "POST": raise Http404 - # By default, Django doesn't allow Users with is_active = False to reset their passwords, - # but this bites people who signed up a long time ago, never activated, and forgot their - # password. So for their sake, we'll auto-activate a user for whom password_reset is called. - try: - user = User.objects.get(email=request.POST['email']) - user.is_active = True - user.save() - except: - log.exception("Tried to auto-activate user to enable password reset, but failed.") - - form = PasswordResetForm(request.POST) + form = PasswordResetFormNoActive(request.POST) if form.is_valid(): form.save(use_https=request.is_secure(), from_email=settings.DEFAULT_FROM_EMAIL, @@ -984,6 +977,20 @@ def password_reset(request): return HttpResponse(json.dumps({'success': False, 'error': 'Invalid e-mail'})) +def password_reset_confirm_wrapper(request, uidb36=None, token=None): + ''' A wrapper around django.contrib.auth.views.password_reset_confirm. + Needed because we want to set the user as active at this step. + ''' + #cribbed from django.contrib.auth.views.password_reset_confirm + try: + uid_int = base36_to_int(uidb36) + user = User.objects.get(id=uid_int) + user.is_active = True + user.save() + except (ValueError, User.DoesNotExist): + pass + return password_reset_confirm(request, uidb36=uidb36, token=token) + def reactivation_email_for_user(user): try: diff --git a/lms/templates/registration/password_reset_email.html b/lms/templates/registration/password_reset_email.html index bf6c3e0891..68073d9ddd 100644 --- a/lms/templates/registration/password_reset_email.html +++ b/lms/templates/registration/password_reset_email.html @@ -3,7 +3,7 @@ {% trans "Please go to the following page and choose a new password:" %} {% block reset_link %} -https://{{domain}}{% url 'django.contrib.auth.views.password_reset_confirm' uidb36=uid token=token %} +https://{{domain}}{% url 'student.views.password_reset_confirm_wrapper' uidb36=uid token=token %} {% endblock %} If you didn't request this change, you can disregard this email - we have not yet reset your password. diff --git a/lms/urls.py b/lms/urls.py index 52ce539f73..50ce35cde0 100644 --- a/lms/urls.py +++ b/lms/urls.py @@ -51,7 +51,7 @@ urlpatterns = ('', # nopep8 url(r'^password_change_done/$', django.contrib.auth.views.password_change_done, name='auth_password_change_done'), url(r'^password_reset_confirm/(?P[0-9A-Za-z]+)-(?P.+)/$', - django.contrib.auth.views.password_reset_confirm, + 'student.views.password_reset_confirm_wrapper', name='auth_password_reset_confirm'), url(r'^password_reset_complete/$', django.contrib.auth.views.password_reset_complete, name='auth_password_reset_complete'), From 83062c0b7dd6b85e6f50ad717ba796c92c5ecb8d Mon Sep 17 00:00:00 2001 From: Jason Bau Date: Mon, 24 Jun 2013 11:54:31 -0700 Subject: [PATCH 17/55] Tests + Now subclass PasswordResetForm instead of copy Changed to subclassing django's PasswordResetForm and overriding clean_password() instead of copy/paste. Less lines to worry about for diff-cover this way =) --- common/djangoapps/student/forms.py | 65 ++------------ common/djangoapps/student/tests/tests.py | 106 ++++++++++++++++++++++- common/djangoapps/student/views.py | 2 +- 3 files changed, 111 insertions(+), 62 deletions(-) diff --git a/common/djangoapps/student/forms.py b/common/djangoapps/student/forms.py index 75c89e0a26..1096092117 100644 --- a/common/djangoapps/student/forms.py +++ b/common/djangoapps/student/forms.py @@ -1,33 +1,15 @@ from django import forms -from django.utils.translation import ugettext, ugettext_lazy as _ -from django.template import loader from django.contrib.auth.models import User -from django.contrib.auth.hashers import UNUSABLE_PASSWORD, is_password_usable, get_hasher -from django.contrib.auth.tokens import default_token_generator -from django.contrib.sites.models import get_current_site -from django.utils.http import int_to_base36 +from django.contrib.auth.forms import PasswordResetForm +from django.contrib.auth.hashers import UNUSABLE_PASSWORD - - -# This is a literal copy from Django 1.4.5's django.contrib.auth.forms.PasswordResetForm -# I think copy-and-paste here is somewhat better than subclassing and -# just changing the definition of clean_email, because it's less -# likely to be broken by incompatibility with a new django version. -# (If this form is good enough now, a snapshot of it ought to last a while) - -class PasswordResetFormNoActive(forms.Form): - error_messages = { - 'unknown': _("That e-mail address doesn't have an associated " - "user account. Are you sure you've registered?"), - 'unusable': _("The user account associated with this e-mail " - "address cannot reset the password."), - } - email = forms.EmailField(label=_("E-mail"), max_length=75) - +class PasswordResetFormNoActive(PasswordResetForm): def clean_email(self): """ - Validates that an active user exists with the given email address. - """ + This is a literal copy from Django 1.4.5's django.contrib.auth.forms.PasswordResetForm + Except removing the requirement of active users + Validates that a user exists with the given email address. + """ email = self.cleaned_data["email"] #The line below contains the only change, removing is_active=True self.users_cache = User.objects.filter(email__iexact=email) @@ -37,36 +19,3 @@ class PasswordResetFormNoActive(forms.Form): for user in self.users_cache): raise forms.ValidationError(self.error_messages['unusable']) return email - - def save(self, domain_override=None, - subject_template_name='registration/password_reset_subject.txt', - email_template_name='registration/password_reset_email.html', - use_https=False, token_generator=default_token_generator, - from_email=None, request=None): - """ - Generates a one-use only link for resetting password and sends to the - user. - """ - from django.core.mail import send_mail - for user in self.users_cache: - if not domain_override: - current_site = get_current_site(request) - site_name = current_site.name - domain = current_site.domain - else: - site_name = domain = domain_override - c = { - 'email': user.email, - 'domain': domain, - 'site_name': site_name, - 'uid': int_to_base36(user.id), - 'user': user, - 'token': token_generator.make_token(user), - 'protocol': use_https and 'https' or 'http', - } - subject = loader.render_to_string(subject_template_name, c) - # Email subject *must not* contain newlines - subject = ''.join(subject.splitlines()) - email = loader.render_to_string(email_template_name, c) - send_mail(subject, email, from_email, [user.email]) - diff --git a/common/djangoapps/student/tests/tests.py b/common/djangoapps/student/tests/tests.py index 4638da44b2..10836122b8 100644 --- a/common/djangoapps/student/tests/tests.py +++ b/common/djangoapps/student/tests/tests.py @@ -5,18 +5,118 @@ when you run "manage.py test". Replace this with more appropriate tests for your application. """ import logging +import json +import re +import unittest +from django import forms +from django.conf import settings from django.test import TestCase -from mock import Mock +from django.test.client import RequestFactory +from django.contrib.auth.models import User +from django.contrib.auth.hashers import UNUSABLE_PASSWORD +from django.template.loader import render_to_string, get_template, TemplateDoesNotExist +from django.core.urlresolvers import is_valid_path + +from mock import Mock, patch +from textwrap import dedent from student.models import unique_id_for_user -from student.views import process_survey_link, _cert_info - +from student.views import process_survey_link, _cert_info, password_reset, password_reset_confirm_wrapper +from student.tests.factories import UserFactory +from student.tests.test_email import mock_render_to_string COURSE_1 = 'edX/toy/2012_Fall' COURSE_2 = 'edx/full/6.002_Spring_2012' log = logging.getLogger(__name__) +try: + get_template('registration/password_reset_email.html') + project_uses_password_reset = True +except TemplateDoesNotExist: + project_uses_password_reset = False + + +class ResetPasswordTests(TestCase): + """ Tests that clicking reset password sends email, and doesn't activate the user + """ + request_factory = RequestFactory() + + def setUp(self): + self.user = UserFactory.create() + self.user.is_active = False + self.user.save() + + self.user_bad_passwd = UserFactory.create() + self.user_bad_passwd.is_active = False + self.user_bad_passwd.password = UNUSABLE_PASSWORD + self.user_bad_passwd.save() + + + @unittest.skipUnless(project_uses_password_reset, dedent("""Skipping Test because CMS has not provided + necessary templates for password reset. If this message is in LMS tests, that is a bug and needs to be fixed.""")) + @patch('student.views.password_reset_confirm') + @patch('django.core.mail.send_mail') + @patch('student.views.render_to_string', Mock(side_effect=mock_render_to_string, autospec=True)) + def test_reset_password_email(self, send_email, reset_confirm): + """Tests sending of reset password email""" + + #First test the bad password user, mainly for diff-cover sake + bad_pwd_req = self.request_factory.post('/password_reset/', {'email': self.user_bad_passwd.email}) + bad_pwd_resp = password_reset(bad_pwd_req) + self.assertEquals(bad_pwd_resp.status_code, 200) + self.assertEquals(bad_pwd_resp.content, json.dumps({'success': False, + 'error': 'Invalid e-mail or user'})) + + #Now test the exception cases with invalid email. + bad_email_req = self.request_factory.post('/password_reset/', {'email': self.user.email+"makeItFail"}) + bad_email_resp = password_reset(bad_email_req) + self.assertEquals(bad_email_resp.status_code, 200) + self.assertEquals(bad_email_resp.content, json.dumps({'success': False, + 'error': 'Invalid e-mail or user'})) + + #Now test the legit case where email should have been sent + good_req = self.request_factory.post('/password_reset/', {'email': self.user.email}) + good_resp = password_reset(good_req) + self.assertEquals(good_resp.status_code, 200) + self.assertEquals(good_resp.content, + json.dumps({'success': True, + 'value': "('registration/password_reset_done.html', [])"})) + + ((subject, msg, from_addr, to_addrs), sm_kwargs) = send_email.call_args + self.assertIn("Password reset", subject) + self.assertIn("You're receiving this e-mail because you requested a password reset", msg) + self.assertEquals(from_addr, settings.DEFAULT_FROM_EMAIL) + self.assertEquals(len(to_addrs), 1) + self.assertIn(self.user.email, to_addrs) + + #test that the user is not active + #it's a bit unsettling that we have to reload the user from the db for this test to work + #but I guess the user is cached here in the instance of ResetPasswordTests + #so the update in the view does not know to update this class. + self.user = User.objects.get(pk=self.user.pk) + self.assertFalse(self.user.is_active) + + #now try to activate the user in the password reset phase + bad_reset_req = self.request_factory.get('/password_reset_confirm/NO-OP/') + bad_reset_resp = password_reset_confirm_wrapper(bad_reset_req, 'NO', 'OP') + (confirm_args, confirm_kwargs) = reset_confirm.call_args + self.assertEquals(confirm_kwargs['uidb36'], 'NO') + self.assertEquals(confirm_kwargs['token'], 'OP') + self.user = User.objects.get(pk=self.user.pk) + self.assertFalse(self.user.is_active) + + reset_match = re.search(r'password_reset_confirm/(?P[0-9A-Za-z]+)-(?P.+)/', msg).groupdict() + good_reset_req = self.request_factory.get('/password_reset_confirm/{0}-{1}/'.format(reset_match['uidb36'], + reset_match['token'])) + good_reset_resp = password_reset_confirm_wrapper(good_reset_req, reset_match['uidb36'], reset_match['token']) + (confirm_args, confirm_kwargs) = reset_confirm.call_args + self.assertEquals(confirm_kwargs['uidb36'], reset_match['uidb36']) + self.assertEquals(confirm_kwargs['token'], reset_match['token']) + self.user = User.objects.get(pk=self.user.pk) + self.assertTrue(self.user.is_active) + + class CourseEndingTest(TestCase): """Test things related to course endings: certificates, surveys, etc""" diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index 50f6d90368..7ae460b438 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -975,7 +975,7 @@ def password_reset(request): 'value': render_to_string('registration/password_reset_done.html', {})})) else: return HttpResponse(json.dumps({'success': False, - 'error': 'Invalid e-mail'})) + 'error': 'Invalid e-mail or user'})) def password_reset_confirm_wrapper(request, uidb36=None, token=None): ''' A wrapper around django.contrib.auth.views.password_reset_confirm. From 3a8f591fe5280146b66918e55daabd674999b507 Mon Sep 17 00:00:00 2001 From: Peter Baratta Date: Mon, 24 Jun 2013 16:36:49 -0400 Subject: [PATCH 18/55] Add tests for the diff coverage; fix one hidden unicode bug --- common/lib/xmodule/xmodule/capa_module.py | 2 +- .../xmodule/xmodule/tests/test_capa_module.py | 85 ++++++++++++++++++- 2 files changed, 83 insertions(+), 4 deletions(-) diff --git a/common/lib/xmodule/xmodule/capa_module.py b/common/lib/xmodule/xmodule/capa_module.py index d740a73946..bb06912f7a 100644 --- a/common/lib/xmodule/xmodule/capa_module.py +++ b/common/lib/xmodule/xmodule/capa_module.py @@ -412,7 +412,7 @@ class CapaModule(CapaFields, XModule): `err` is the Exception encountered while rendering the problem HTML. """ - log.exception(err) + log.exception(err.message) # TODO (vshnayder): another switch on DEBUG. if self.system.DEBUG: diff --git a/common/lib/xmodule/xmodule/tests/test_capa_module.py b/common/lib/xmodule/xmodule/tests/test_capa_module.py index c6ffd32e89..1e84174291 100644 --- a/common/lib/xmodule/xmodule/tests/test_capa_module.py +++ b/common/lib/xmodule/xmodule/tests/test_capa_module.py @@ -11,11 +11,12 @@ import datetime from mock import Mock, patch import unittest import random +import json import xmodule -from capa.responsetypes import StudentInputError, \ - LoncapaProblemError, ResponseError -from xmodule.capa_module import CapaModule +from capa.responsetypes import (StudentInputError, LoncapaProblemError, + ResponseError) +from xmodule.capa_module import CapaModule, ComplexEncoder from xmodule.modulestore import Location from django.http import QueryDict @@ -530,6 +531,32 @@ class CapaModuleTest(unittest.TestCase): # Expect that the number of attempts is NOT incremented self.assertEqual(module.attempts, 1) + def test_check_problem_other_errors(self): + """ + Test that errors other than the expected kinds give an appropriate message. + + See also `test_check_problem_error` for the "expected kinds" or errors. + """ + # Create the module + module = CapaFactory.create(attempts=1) + + # Ensure that the user is NOT staff + module.system.user_is_staff = False + + # Ensure that DEBUG is on + module.system.DEBUG = True + + # Simulate answering a problem that raises the exception + with patch('capa.capa_problem.LoncapaProblem.grade_answers') as mock_grade: + error_msg = u"Superterrible error happened: ☠" + mock_grade.side_effect = Exception(error_msg) + + get_request_dict = {CapaFactory.input_key(): '3.14'} + result = module.check_problem(get_request_dict) + + # Expect an AJAX alert message in 'success' + self.assertTrue(error_msg in result['success']) + def test_check_problem_error_nonascii(self): # Try each exception that capa_module should handle @@ -1059,6 +1086,33 @@ class CapaModuleTest(unittest.TestCase): # Expect that the module has created a new dummy problem with the error self.assertNotEqual(original_problem, module.lcp) + def test_get_problem_html_error_w_debug(self): + """ + Test the html response when an error occurs with DEBUG on + """ + module = CapaFactory.create() + + # Simulate throwing an exception when the capa problem + # is asked to render itself as HTML + error_msg = u"Superterrible error happened: ☠" + module.lcp.get_html = Mock(side_effect=Exception(error_msg)) + + # Stub out the get_test_system rendering function + module.system.render_template = Mock(return_value="
Test Template HTML
") + + # Make sure DEBUG is on + module.system.DEBUG = True + + # Try to render the module with DEBUG turned on + html = module.get_problem_html() + + self.assertTrue(html is not None) + + # Check the rendering context + render_args, _ = module.system.render_template.call_args + context = render_args[1] + self.assertTrue(error_msg in context['problem']['html']) + def test_random_seed_no_change(self): # Run the test for each possible rerandomize value @@ -1164,3 +1218,28 @@ class CapaModuleTest(unittest.TestCase): for i in range(200): module = CapaFactory.create(rerandomize=rerandomize) assert 0 <= module.seed < 1000 + + @patch('xmodule.capa_module.log') + @patch('xmodule.capa_module.Progress') + def test_get_progress_error(self, mock_progress, mock_log): + """ + Check that an exception given in `Progress` produces a `log.exception` call. + """ + error_types = [TypeError, ValueError] + for error_type in error_types: + mock_progress.side_effect = error_type + module = CapaFactory.create() + self.assertIsNone(module.get_progress()) + mock_log.exception.assert_called_once_with('Got bad progress') + mock_log.reset_mock() + + +class ComplexEncoderTest(unittest.TestCase): + def test_default(self): + """ + Check that complex numbers can be encoded into JSON. + """ + complex_num = 1 - 1j + expected_str = '1-1*j' + json_str = json.dumps(complex_num, cls=ComplexEncoder) + self.assertEqual(expected_str, json_str[1:-1]) # ignore quotes From 332a440539928e7bd8d9d9fab3dd8d5f475f4b97 Mon Sep 17 00:00:00 2001 From: Brian Wilson Date: Wed, 19 Jun 2013 18:09:18 -0400 Subject: [PATCH 19/55] Enable per-student background tasks. --- lms/djangoapps/instructor_task/api_helper.py | 7 +- lms/djangoapps/instructor_task/models.py | 21 +++++- .../instructor_task/tests/test_api.py | 2 +- .../instructor_task/tests/test_integration.py | 2 +- .../instructor_task/tests/test_tasks.py | 71 +++++++++++++++---- .../instructor_task/tests/test_views.py | 5 +- .../courseware/instructor_dashboard.html | 6 +- 7 files changed, 87 insertions(+), 27 deletions(-) diff --git a/lms/djangoapps/instructor_task/api_helper.py b/lms/djangoapps/instructor_task/api_helper.py index f9febd17d7..2795fd08c1 100644 --- a/lms/djangoapps/instructor_task/api_helper.py +++ b/lms/djangoapps/instructor_task/api_helper.py @@ -2,8 +2,6 @@ import hashlib import json import logging -from django.db import transaction - from celery.result import AsyncResult from celery.states import READY_STATES, SUCCESS, FAILURE, REVOKED @@ -30,7 +28,6 @@ def _task_is_running(course_id, task_type, task_key): return len(runningTasks) > 0 -@transaction.autocommit def _reserve_task(course_id, task_type, task_key, task_input, requester): """ Creates a database entry to indicate that a task is in progress. @@ -39,9 +36,9 @@ def _reserve_task(course_id, task_type, task_key, task_input, requester): Includes the creation of an arbitrary value for task_id, to be submitted with the task call to celery. - Autocommit annotation makes sure the database entry is committed. + The InstructorTask.create method makes sure the InstructorTask entry is committed. When called from any view that is wrapped by TransactionMiddleware, - and thus in a "commit-on-success" transaction, this autocommit here + and thus in a "commit-on-success" transaction, an autocommit buried within here will cause any pending transaction to be committed by a successful save here. Any future database operations will take place in a separate transaction. diff --git a/lms/djangoapps/instructor_task/models.py b/lms/djangoapps/instructor_task/models.py index f01cc4e3ad..b28a9a3d83 100644 --- a/lms/djangoapps/instructor_task/models.py +++ b/lms/djangoapps/instructor_task/models.py @@ -72,6 +72,16 @@ class InstructorTask(models.Model): @classmethod def create(cls, course_id, task_type, task_key, task_input, requester): + """ + Create an instance of InstructorTask. + + The InstructorTask.save_now method makes sure the InstructorTask entry is committed. + When called from any view that is wrapped by TransactionMiddleware, + and thus in a "commit-on-success" transaction, an autocommit buried within here + will cause any pending transaction to be committed by a successful + save here. Any future database operations will take place in a + separate transaction. + """ # create the task_id here, and pass it into celery: task_id = str(uuid4()) @@ -99,7 +109,16 @@ class InstructorTask(models.Model): @transaction.autocommit def save_now(self): - """Writes InstructorTask immediately, ensuring the transaction is committed.""" + """ + Writes InstructorTask immediately, ensuring the transaction is committed. + + Autocommit annotation makes sure the database entry is committed. + When called from any view that is wrapped by TransactionMiddleware, + and thus in a "commit-on-success" transaction, this autocommit here + will cause any pending transaction to be committed by a successful + save here. Any future database operations will take place in a + separate transaction. + """ self.save() @staticmethod diff --git a/lms/djangoapps/instructor_task/tests/test_api.py b/lms/djangoapps/instructor_task/tests/test_api.py index 841fdca8a0..1e40c51c4b 100644 --- a/lms/djangoapps/instructor_task/tests/test_api.py +++ b/lms/djangoapps/instructor_task/tests/test_api.py @@ -22,7 +22,7 @@ from instructor_task.tests.test_base import (InstructorTaskTestCase, class InstructorTaskReportTest(InstructorTaskTestCase): """ - Tests API and view methods that involve the reporting of status for background tasks. + Tests API methods that involve the reporting of status for background tasks. """ def test_get_running_instructor_tasks(self): diff --git a/lms/djangoapps/instructor_task/tests/test_integration.py b/lms/djangoapps/instructor_task/tests/test_integration.py index 5a17e32329..9b56663753 100644 --- a/lms/djangoapps/instructor_task/tests/test_integration.py +++ b/lms/djangoapps/instructor_task/tests/test_integration.py @@ -1,5 +1,5 @@ """ -Integration Tests for LMS instructor-initiated background tasks +Integration Tests for LMS instructor-initiated background tasks. Runs tasks on answers to course problems to validate that code paths actually work. diff --git a/lms/djangoapps/instructor_task/tests/test_tasks.py b/lms/djangoapps/instructor_task/tests/test_tasks.py index c59a7065ae..090c114720 100644 --- a/lms/djangoapps/instructor_task/tests/test_tasks.py +++ b/lms/djangoapps/instructor_task/tests/test_tasks.py @@ -1,5 +1,5 @@ """ -Unit tests for LMS instructor-initiated background tasks, +Unit tests for LMS instructor-initiated background tasks. Runs tasks on answers to course problems to validate that code paths actually work. @@ -7,6 +7,7 @@ paths actually work. """ import json from uuid import uuid4 +from unittest import skip from mock import Mock, patch @@ -62,6 +63,7 @@ class TestInstructorTasks(InstructorTaskModuleTestCase): } def _run_task_with_mock_celery(self, task_function, entry_id, task_id, expected_failure_message=None): + """Submit a task and mock how celery provides a current_task.""" self.current_task = Mock() self.current_task.request = Mock() self.current_task.request.id = task_id @@ -73,7 +75,7 @@ class TestInstructorTasks(InstructorTaskModuleTestCase): return task_function(entry_id, self._get_xmodule_instance_args()) def _test_missing_current_task(self, task_function): - # run without (mock) Celery running + """Check that a task_function fails when celery doesn't provide a current_task.""" task_entry = self._create_input_entry() with self.assertRaises(UpdateProblemModuleStateError): task_function(task_entry.id, self._get_xmodule_instance_args()) @@ -88,7 +90,7 @@ class TestInstructorTasks(InstructorTaskModuleTestCase): self._test_missing_current_task(delete_problem_state) def _test_undefined_problem(self, task_function): - # run with celery, but no problem defined + """Run with celery, but no problem defined.""" task_entry = self._create_input_entry() with self.assertRaises(ItemNotFoundError): self._run_task_with_mock_celery(task_function, task_entry.id, task_entry.task_id) @@ -103,7 +105,7 @@ class TestInstructorTasks(InstructorTaskModuleTestCase): self._test_undefined_problem(delete_problem_state) def _test_run_with_task(self, task_function, action_name, expected_num_updated): - # run with some StudentModules for the problem + """Run a task and check the number of StudentModules processed.""" task_entry = self._create_input_entry() status = self._run_task_with_mock_celery(task_function, task_entry.id, task_entry.task_id) # check return value @@ -118,7 +120,7 @@ class TestInstructorTasks(InstructorTaskModuleTestCase): self.assertEquals(entry.task_state, SUCCESS) def _test_run_with_no_state(self, task_function, action_name): - # run with no StudentModules for the problem + """Run with no StudentModules defined for the current problem.""" self.define_option_problem(PROBLEM_URL_NAME) self._test_run_with_task(task_function, action_name, 0) @@ -185,7 +187,7 @@ class TestInstructorTasks(InstructorTaskModuleTestCase): module_state_key=self.problem_url) def _test_reset_with_student(self, use_email): - # run with some StudentModules for the problem + """Run a reset task for one student, with several StudentModules for the problem defined.""" num_students = 10 initial_attempts = 3 input_state = json.dumps({'attempts': initial_attempts}) @@ -233,8 +235,7 @@ class TestInstructorTasks(InstructorTaskModuleTestCase): self._test_reset_with_student(True) def _test_run_with_failure(self, task_function, expected_message): - # run with no StudentModules for the problem, - # because we will fail before entering the loop. + """Run a task and trigger an artificial failure with give message.""" task_entry = self._create_input_entry() self.define_option_problem(PROBLEM_URL_NAME) with self.assertRaises(TestTaskFailure): @@ -256,8 +257,10 @@ class TestInstructorTasks(InstructorTaskModuleTestCase): self._test_run_with_failure(delete_problem_state, 'We expected this to fail') def _test_run_with_long_error_msg(self, task_function): - # run with an error message that is so long it will require - # truncation (as well as the jettisoning of the traceback). + """ + Run with an error message that is so long it will require + truncation (as well as the jettisoning of the traceback). + """ task_entry = self._create_input_entry() self.define_option_problem(PROBLEM_URL_NAME) expected_message = "x" * 1500 @@ -282,9 +285,11 @@ class TestInstructorTasks(InstructorTaskModuleTestCase): self._test_run_with_long_error_msg(delete_problem_state) def _test_run_with_short_error_msg(self, task_function): - # run with an error message that is short enough to fit - # in the output, but long enough that the traceback won't. - # Confirm that the traceback is truncated. + """ + Run with an error message that is short enough to fit + in the output, but long enough that the traceback won't. + Confirm that the traceback is truncated. + """ task_entry = self._create_input_entry() self.define_option_problem(PROBLEM_URL_NAME) expected_message = "x" * 900 @@ -330,3 +335,43 @@ class TestInstructorTasks(InstructorTaskModuleTestCase): self.assertEquals(output['exception'], 'ValueError') self.assertTrue("Length of task output is too long" in output['message']) self.assertTrue('traceback' not in output) + + @skip + def test_rescoring_unrescorable(self): + # TODO: this test needs to have Mako templates initialized + # to make sure that the creation of an XModule works. + input_state = json.dumps({'done': True}) + num_students = 1 + self._create_students_with_state(num_students, input_state) + task_entry = self._create_input_entry() + with self.assertRaises(UpdateProblemModuleStateError): + self._run_task_with_mock_celery(rescore_problem, task_entry.id, task_entry.task_id) + # check values stored in table: + entry = InstructorTask.objects.get(id=task_entry.id) + output = json.loads(entry.task_output) + self.assertEquals(output['exception'], "UpdateProblemModuleStateError") + self.assertEquals(output['message'], "Specified problem does not support rescoring.") + self.assertGreater(len(output['traceback']), 0) + + @skip + def test_rescoring_success(self): + # TODO: this test needs to have Mako templates initialized + # to make sure that the creation of an XModule works. + input_state = json.dumps({'done': True}) + num_students = 10 + self._create_students_with_state(num_students, input_state) + task_entry = self._create_input_entry() + mock_instance = Mock() + mock_instance.rescore_problem = Mock({'success': 'correct'}) + # TODO: figure out why this mock is not working.... + with patch('courseware.module_render.get_module_for_descriptor_internal') as mock_get_module: + mock_get_module.return_value = mock_instance + self._run_task_with_mock_celery(rescore_problem, task_entry.id, task_entry.task_id) + # check return value + entry = InstructorTask.objects.get(id=task_entry.id) + output = json.loads(entry.task_output) + self.assertEquals(output.get('attempted'), num_students) + self.assertEquals(output.get('updated'), num_students) + self.assertEquals(output.get('total'), num_students) + self.assertEquals(output.get('action_name'), 'rescored') + self.assertGreater('duration_ms', 0) diff --git a/lms/djangoapps/instructor_task/tests/test_views.py b/lms/djangoapps/instructor_task/tests/test_views.py index 9020bf6e60..41de314abd 100644 --- a/lms/djangoapps/instructor_task/tests/test_views.py +++ b/lms/djangoapps/instructor_task/tests/test_views.py @@ -1,6 +1,6 @@ """ -Test for LMS instructor background task queue management +Test for LMS instructor background task views. """ import json from celery.states import SUCCESS, FAILURE, REVOKED, PENDING @@ -18,7 +18,7 @@ from instructor_task.views import instructor_task_status, get_task_completion_in class InstructorTaskReportTest(InstructorTaskTestCase): """ - Tests API and view methods that involve the reporting of status for background tasks. + Tests view methods that involve the reporting of status for background tasks. """ def _get_instructor_task_status(self, task_id): @@ -263,4 +263,3 @@ class InstructorTaskReportTest(InstructorTaskTestCase): succeeded, message = get_task_completion_info(instructor_task) self.assertFalse(succeeded) self.assertEquals(message, "Problem rescored for 2 of 3 students (out of 5)") - diff --git a/lms/templates/courseware/instructor_dashboard.html b/lms/templates/courseware/instructor_dashboard.html index ef1eb174fc..d541962906 100644 --- a/lms/templates/courseware/instructor_dashboard.html +++ b/lms/templates/courseware/instructor_dashboard.html @@ -249,7 +249,7 @@ function goto( mode)

Then select an action: - %if settings.MITX_FEATURES.get('ENABLE_COURSE_BACKGROUND_TASKS'): + %if settings.MITX_FEATURES.get('ENABLE_INSTRUCTOR_BACKGROUND_TASKS'): %endif

@@ -260,9 +260,9 @@ function goto( mode)

%endif - %if settings.MITX_FEATURES.get('ENABLE_COURSE_BACKGROUND_TASKS'): + %if settings.MITX_FEATURES.get('ENABLE_INSTRUCTOR_BACKGROUND_TASKS'):

Rescoring runs in the background, and status for active tasks will appear in a table below. - To see status for all tasks submitted for this course and student, click on this button: + To see status for all tasks submitted for this problem and student, click on this button:

From ddc986f775e5eb2cf5bb30644ae7d934140805cb Mon Sep 17 00:00:00 2001 From: David Baumgold Date: Tue, 25 Jun 2013 11:25:29 -0400 Subject: [PATCH 20/55] Call event.preventDefault() on notification action buttons But allow you to specify that the event should not be prevented --- .../coffee/spec/views/feedback_spec.coffee | 39 +++++++++++++++++++ cms/static/js/views/feedback.js | 14 ++++++- 2 files changed, 51 insertions(+), 2 deletions(-) diff --git a/cms/static/coffee/spec/views/feedback_spec.coffee b/cms/static/coffee/spec/views/feedback_spec.coffee index e5916c5ed3..adec11e2a7 100644 --- a/cms/static/coffee/spec/views/feedback_spec.coffee +++ b/cms/static/coffee/spec/views/feedback_spec.coffee @@ -17,6 +17,16 @@ beforeEach -> return text.test(trimmedText) else return trimmedText.indexOf(text) != -1; + toHaveBeenPrevented: -> + # remove this when we upgrade jasmine-jquery + eventName = @actual.eventName + selector = @actual.selector + @message = -> + [ + "Expected event #{eventName} to have been prevented on #{selector}", + "Expected event #{eventName} not to have been prevented on #{selector}" + ] + return jasmine.JQuery.events.wasPrevented(selector, eventName) describe "CMS.Views.SystemFeedback", -> beforeEach -> @@ -123,6 +133,35 @@ describe "CMS.Views.SystemFeedback click events", -> it "should apply class to secondary action", -> expect(@view.$(".action-secondary")).toHaveClass("cancel-button") + it "should preventDefault on primary action", -> + spyOnEvent(".action-primary", "click") + @view.$(".action-primary").click() + expect("click").toHaveBeenPreventedOn(".action-primary") + + it "should preventDefault on secondary action", -> + spyOnEvent(".action-secondary", "click") + @view.$(".action-secondary").click() + expect("click").toHaveBeenPreventedOn(".action-secondary") + +describe "CMS.Views.SystemFeedback not preventing events", -> + beforeEach -> + @clickSpy = jasmine.createSpy('clickSpy') + @view = new CMS.Views.Alert.Confirmation( + title: "It's all good" + message: "No reason for this alert" + actions: + primary: + text: "Whatever" + click: @clickSpy + preventDefault: false + ) + @view.show() + + it "should not preventDefault", -> + spyOnEvent(".action-primary", "click") + @view.$(".action-primary").click() + expect("click").not.toHaveBeenPreventedOn(".action-primary") + expect(@clickSpy).toHaveBeenCalled() describe "CMS.Views.SystemFeedback multiple secondary actions", -> beforeEach -> diff --git a/cms/static/js/views/feedback.js b/cms/static/js/views/feedback.js index 3f161d5b1f..3bfeeb5af2 100644 --- a/cms/static/js/views/feedback.js +++ b/cms/static/js/views/feedback.js @@ -10,8 +10,12 @@ CMS.Views.SystemFeedback = Backbone.View.extend({ minShown: 0, // length of time after this view has been shown before it can be hidden (milliseconds) maxShown: Infinity // length of time after this view has been shown before it will be automatically hidden (milliseconds) - /* could also have an "actions" hash: here is an example demonstrating - the expected structure + /* Could also have an "actions" hash: here is an example demonstrating + the expected structure. For each action, by default the framework + will call preventDefault on the click event before the function is + run; to make it not do that, just pass `preventDefault: false` in + the action object. + actions: { primary: { "text": "Save", @@ -106,6 +110,9 @@ CMS.Views.SystemFeedback = Backbone.View.extend({ if(!actions) { return; } var primary = actions.primary; if(!primary) { return; } + if(primary.preventDefault !== false) { + event.preventDefault(); + } if(primary.click) { primary.click.call(event.target, this, event); } @@ -121,6 +128,9 @@ CMS.Views.SystemFeedback = Backbone.View.extend({ i = _.indexOf(this.$(".action-secondary"), event.target); } var secondary = secondaryList[i]; + if(secondary.preventDefault !== false) { + event.preventDefault(); + } if(secondary.click) { secondary.click.call(event.target, this, event); } From a9a7f97d9b694078ccc23706d29a1fcb11dcc74a Mon Sep 17 00:00:00 2001 From: Brian Wilson Date: Tue, 25 Jun 2013 11:32:45 -0400 Subject: [PATCH 21/55] Update CHANGELOG for per-student problem rescoring. --- CHANGELOG.rst | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index cb8eec738f..21b8c9f90b 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -5,6 +5,11 @@ These are notable changes in edx-platform. This is a rolling list of changes, in roughly chronological order, most recent first. Add your entries at or near the top. Include a label indicating the component affected. +LMS: Problem rescoring. Added options on the Grades tab of the +Instructor Dashboard to allow a particular student's submission for a +particular problem to be rescored. Provides an option to see a +history of background tasks for a given problem and student. + Blades: Small UX fix on capa multiple-choice problems. Make labels only as wide as the text to reduce accidental choice selections. From 8a9125f121a1b983b33d4c7f8cd16deeee8335cf Mon Sep 17 00:00:00 2001 From: JonahStanley Date: Tue, 25 Jun 2013 11:33:46 -0400 Subject: [PATCH 22/55] Test Mongo database is now unique and destroyed in teardown --- common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py index c5ef0d751a..44e69fb0ed 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py @@ -13,11 +13,12 @@ from xmodule.templates import update_templates from .test_modulestore import check_path_to_location from . import DATA_DIR +from uuid import uuid4 HOST = 'localhost' PORT = 27017 -DB = 'test' +DB = 'test_mongo_%s' % uuid4().hex COLLECTION = 'modulestore' FS_ROOT = DATA_DIR # TODO (vshnayder): will need a real fs_root for testing load_item DEFAULT_CLASS = 'xmodule.raw_module.RawDescriptor' @@ -39,7 +40,8 @@ class TestMongoModuleStore(object): @classmethod def teardownClass(cls): - pass + cls.connection = pymongo.connection.Connection(HOST, PORT) + cls.connection.drop_database(DB) @staticmethod def initdb(): From c0805c334d1d5bbfe6582e8042fae1ad5ae75f77 Mon Sep 17 00:00:00 2001 From: Will Daly Date: Tue, 25 Jun 2013 13:23:13 -0400 Subject: [PATCH 23/55] Updated diff-cover to version 0.1.3 to fix a bug --- requirements/edx/github.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements/edx/github.txt b/requirements/edx/github.txt index 5ce748e7b5..f64568dc10 100644 --- a/requirements/edx/github.txt +++ b/requirements/edx/github.txt @@ -10,4 +10,4 @@ # Our libraries: -e git+https://github.com/edx/XBlock.git@4d8735e883#egg=XBlock -e git+https://github.com/edx/codejail.git@0a1b468#egg=codejail --e git+https://github.com/edx/diff-cover.git@v0.1.2#egg=diff_cover +-e git+https://github.com/edx/diff-cover.git@v0.1.3#egg=diff_cover From 2f02496c8f14e51eaaa8180ee0acfec9f375cb3f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Andr=C3=A9s=20Rocha?= Date: Thu, 13 Jun 2013 13:54:51 -0400 Subject: [PATCH 24/55] Reorder imports on module_render --- lms/djangoapps/courseware/module_render.py | 30 ++++++++++++---------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index 3ffb1d1b1d..15a6ad2dab 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/lms/djangoapps/courseware/module_render.py @@ -2,8 +2,6 @@ import json import logging import re import sys -import static_replace - from functools import partial from django.conf import settings @@ -15,27 +13,31 @@ from django.http import Http404 from django.http import HttpResponse, HttpResponseBadRequest from django.views.decorators.csrf import csrf_exempt +import pyparsing from requests.auth import HTTPBasicAuth +from statsd import statsd from capa.xqueue_interface import XQueueInterface -from courseware.masquerade import setup_masquerade -from courseware.access import has_access from mitxmako.shortcuts import render_to_string -from .models import StudentModule -from psychometrics.psychoanalyze import make_psychometrics_data_update_handler -from student.models import unique_id_for_user +from xblock.runtime import DbModel +from xmodule.error_module import ErrorDescriptor, NonStaffErrorDescriptor from xmodule.errortracker import exc_info_to_str from xmodule.exceptions import NotFoundError, ProcessingError from xmodule.modulestore import Location from xmodule.modulestore.django import modulestore -from xmodule.x_module import ModuleSystem -from xmodule.error_module import ErrorDescriptor, NonStaffErrorDescriptor -from xblock.runtime import DbModel -from xmodule_modifiers import replace_course_urls, replace_static_urls, add_histogram, wrap_xmodule -from .model_data import LmsKeyValueStore, LmsUsage, ModelDataCache - from xmodule.modulestore.exceptions import ItemNotFoundError -from statsd import statsd +from xmodule.x_module import ModuleSystem +from xmodule_modifiers import replace_course_urls, replace_static_urls, add_histogram, wrap_xmodule + +import static_replace +from psychometrics.psychoanalyze import make_psychometrics_data_update_handler +from student.models import unique_id_for_user + +from courseware.access import has_access +from courseware.masquerade import setup_masquerade +from courseware.model_data import LmsKeyValueStore, LmsUsage, ModelDataCache +from courseware.models import StudentModule + log = logging.getLogger(__name__) From e4ee1c6c9b1527ade4cb7c584d7bb5c7fa1c6753 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Andr=C3=A9s=20Rocha?= Date: Mon, 24 Jun 2013 15:29:54 -0400 Subject: [PATCH 25/55] Rename arguments of modx_dispatch and handle_ajax related functions Refactor a bit modx_dispatch --- common/lib/capa/capa/capa_problem.py | 8 +- common/lib/capa/capa/inputtypes.py | 22 ++-- common/lib/capa/capa/tests/test_inputtypes.py | 8 +- common/lib/xmodule/xmodule/capa_module.py | 73 +++++++------ .../xmodule/combined_open_ended_module.py | 5 +- .../lib/xmodule/xmodule/conditional_module.py | 2 +- .../combined_open_ended_modulev1.py | 40 +++---- .../open_ended_module.py | 42 ++++---- .../openendedchild.py | 40 +++---- .../self_assessment_module.py | 34 +++--- .../xmodule/xmodule/peer_grading_module.py | 73 ++++++------- common/lib/xmodule/xmodule/poll_module.py | 4 +- common/lib/xmodule/xmodule/seq_module.py | 4 +- .../lib/xmodule/xmodule/tests/test_logic.py | 4 +- .../lib/xmodule/xmodule/timelimit_module.py | 3 +- common/lib/xmodule/xmodule/video_module.py | 4 +- .../lib/xmodule/xmodule/videoalpha_module.py | 4 +- .../lib/xmodule/xmodule/word_cloud_module.py | 6 +- common/lib/xmodule/xmodule/x_module.py | 4 +- lms/djangoapps/courseware/module_render.py | 100 +++++++++++------- lms/urls.py | 4 +- 21 files changed, 254 insertions(+), 230 deletions(-) diff --git a/common/lib/capa/capa/capa_problem.py b/common/lib/capa/capa/capa_problem.py index d620bac60a..2c813f49d5 100644 --- a/common/lib/capa/capa/capa_problem.py +++ b/common/lib/capa/capa/capa_problem.py @@ -373,7 +373,7 @@ class LoncapaProblem(object): html = contextualize_text(etree.tostring(self._extract_html(self.tree)), self.context) return html - def handle_input_ajax(self, get): + def handle_input_ajax(self, data): ''' InputTypes can support specialized AJAX calls. Find the correct input and pass along the correct data @@ -381,10 +381,10 @@ class LoncapaProblem(object): ''' # pull out the id - input_id = get['input_id'] + input_id = data['input_id'] if self.inputs[input_id]: - dispatch = get['dispatch'] - return self.inputs[input_id].handle_ajax(dispatch, get) + dispatch = data['dispatch'] + return self.inputs[input_id].handle_ajax(dispatch, data) else: log.warning("Could not find matching input for id: %s" % input_id) return {} diff --git a/common/lib/capa/capa/inputtypes.py b/common/lib/capa/capa/inputtypes.py index f026568da1..4c40a2cd3e 100644 --- a/common/lib/capa/capa/inputtypes.py +++ b/common/lib/capa/capa/inputtypes.py @@ -223,13 +223,13 @@ class InputTypeBase(object): """ pass - def handle_ajax(self, dispatch, get): + def handle_ajax(self, dispatch, data): """ InputTypes that need to handle specialized AJAX should override this. Input: dispatch: a string that can be used to determine how to handle the data passed in - get: a dictionary containing the data that was sent with the ajax call + data: a dictionary containing the data that was sent with the ajax call Output: a dictionary object that can be serialized into JSON. This will be sent back to the Javascript. @@ -677,20 +677,20 @@ class MatlabInput(CodeInput): self.queue_len = 1 self.msg = self.plot_submitted_msg - def handle_ajax(self, dispatch, get): + def handle_ajax(self, dispatch, data): ''' Handle AJAX calls directed to this input Args: - dispatch (str) - indicates how we want this ajax call to be handled - - get (dict) - dictionary of key-value pairs that contain useful data + - data (dict) - dictionary of key-value pairs that contain useful data Returns: dict - 'success' - whether or not we successfully queued this submission - 'message' - message to be rendered in case of error ''' if dispatch == 'plot': - return self._plot_data(get) + return self._plot_data(data) return {} def ungraded_response(self, queue_msg, queuekey): @@ -751,7 +751,7 @@ class MatlabInput(CodeInput): msg = result['msg'] return msg - def _plot_data(self, get): + def _plot_data(self, data): ''' AJAX handler for the plot button Args: @@ -765,7 +765,7 @@ class MatlabInput(CodeInput): return {'success': False, 'message': 'Cannot connect to the queue'} # pull relevant info out of get - response = get['submission'] + response = data['submission'] # construct xqueue headers qinterface = self.system.xqueue['interface'] @@ -951,16 +951,16 @@ class ChemicalEquationInput(InputTypeBase): """ return {'previewer': '/static/js/capa/chemical_equation_preview.js', } - def handle_ajax(self, dispatch, get): + def handle_ajax(self, dispatch, data): ''' Since we only have chemcalc preview this input, check to see if it matches the corresponding dispatch and send it through if it does ''' if dispatch == 'preview_chemcalc': - return self.preview_chemcalc(get) + return self.preview_chemcalc(data) return {} - def preview_chemcalc(self, get): + def preview_chemcalc(self, data): """ Render an html preview of a chemical formula or equation. get should contain a key 'formula' and value 'some formula string'. @@ -974,7 +974,7 @@ class ChemicalEquationInput(InputTypeBase): result = {'preview': '', 'error': ''} - formula = get['formula'] + formula = data['formula'] if formula is None: result['error'] = "No formula specified." return result diff --git a/common/lib/capa/capa/tests/test_inputtypes.py b/common/lib/capa/capa/tests/test_inputtypes.py index 313eb28249..1b52d41890 100644 --- a/common/lib/capa/capa/tests/test_inputtypes.py +++ b/common/lib/capa/capa/tests/test_inputtypes.py @@ -467,8 +467,8 @@ class MatlabTest(unittest.TestCase): self.assertEqual(context, expected) def test_plot_data(self): - get = {'submission': 'x = 1234;'} - response = self.the_input.handle_ajax("plot", get) + data = {'submission': 'x = 1234;'} + response = self.the_input.handle_ajax("plot", data) test_system().xqueue['interface'].send_to_queue.assert_called_with(header=ANY, body=ANY) @@ -477,10 +477,10 @@ class MatlabTest(unittest.TestCase): self.assertEqual(self.the_input.input_state['queuestate'], 'queued') def test_plot_data_failure(self): - get = {'submission': 'x = 1234;'} + data = {'submission': 'x = 1234;'} error_message = 'Error message!' test_system().xqueue['interface'].send_to_queue.return_value = (1, error_message) - response = self.the_input.handle_ajax("plot", get) + response = self.the_input.handle_ajax("plot", data) self.assertFalse(response['success']) self.assertEqual(response['message'], error_message) self.assertTrue('queuekey' not in self.the_input.input_state) diff --git a/common/lib/xmodule/xmodule/capa_module.py b/common/lib/xmodule/xmodule/capa_module.py index bb06912f7a..eeb8f19439 100644 --- a/common/lib/xmodule/xmodule/capa_module.py +++ b/common/lib/xmodule/xmodule/capa_module.py @@ -519,11 +519,11 @@ class CapaModule(CapaFields, XModule): # now do the substitutions which are filesystem based, e.g. '/static/' prefixes return self.system.replace_urls(html) - def handle_ajax(self, dispatch, get): + def handle_ajax(self, dispatch, data): """ This is called by courseware.module_render, to handle an AJAX call. - `get` is request.POST. + `data` is request.POST. Returns a json dictionary: { 'progress_changed' : True/False, @@ -547,18 +547,19 @@ class CapaModule(CapaFields, XModule): before = self.get_progress() try: - d = handlers[dispatch](get) - + result = handlers[dispatch](data) except Exception as err: _, _, traceback_obj = sys.exc_info() - raise ProcessingError, err.message, traceback_obj + raise ProcessingError(err.message, traceback_obj) after = self.get_progress() - d.update({ + + result.update({ 'progress_changed': after != before, 'progress_status': Progress.to_js_status_str(after), }) - return json.dumps(d, cls=ComplexEncoder) + + return json.dumps(result, cls=ComplexEncoder) def is_past_due(self): """ @@ -633,32 +634,32 @@ class CapaModule(CapaFields, XModule): return False - def update_score(self, get): + def update_score(self, data): """ Delivers grading response (e.g. from asynchronous code checking) to the capa problem, so its score can be updated - `get` must have a field `response` which is a string that contains the + 'data' must have a key 'response' which is a string that contains the grader's response No ajax return is needed. Return empty dict. """ - queuekey = get['queuekey'] - score_msg = get['xqueue_body'] + queuekey = data['queuekey'] + score_msg = data['xqueue_body'] self.lcp.update_score(score_msg, queuekey) self.set_state_from_lcp() self.publish_grade() return dict() # No AJAX return is needed - def handle_ungraded_response(self, get): + def handle_ungraded_response(self, data): """ Delivers a response from the XQueue to the capa problem The score of the problem will not be updated Args: - - get (dict) must contain keys: + - data (dict) must contain keys: queuekey - a key specific to this response xqueue_body - the body of the response Returns: @@ -666,28 +667,30 @@ class CapaModule(CapaFields, XModule): No ajax return is needed, so an empty dict is returned """ - queuekey = get['queuekey'] - score_msg = get['xqueue_body'] + queuekey = data['queuekey'] + score_msg = data['xqueue_body'] + # pass along the xqueue message to the problem self.lcp.ungraded_response(score_msg, queuekey) self.set_state_from_lcp() return dict() - def handle_input_ajax(self, get): + def handle_input_ajax(self, data): """ Handle ajax calls meant for a particular input in the problem Args: - - get (dict) - data that should be passed to the input + - data (dict) - data that should be passed to the input Returns: - dict containing the response from the input """ - response = self.lcp.handle_input_ajax(get) + response = self.lcp.handle_input_ajax(data) + # save any state changes that may occur self.set_state_from_lcp() return response - def get_answer(self, get): + def get_answer(self, data): """ For the "show answer" button. @@ -717,10 +720,9 @@ class CapaModule(CapaFields, XModule): return {'answers': new_answers} # Figure out if we should move these to capa_problem? - def get_problem(self, get): + def get_problem(self, _data): """ Return results of get_problem_html, as a simple dict for json-ing. - { 'html': } Used if we want to reconfirm we have the right thing e.g. after @@ -729,27 +731,27 @@ class CapaModule(CapaFields, XModule): return {'html': self.get_problem_html(encapsulate=False)} @staticmethod - def make_dict_of_responses(get): + def make_dict_of_responses(data): """ Make dictionary of student responses (aka "answers") - `get` is POST dictionary (Django QueryDict). + `data` is POST dictionary (Django QueryDict). - The `get` dict has keys of the form 'x_y', which are mapped + The `data` dict has keys of the form 'x_y', which are mapped to key 'y' in the returned dict. For example, 'input_1_2_3' would be mapped to '1_2_3' in the returned dict. Some inputs always expect a list in the returned dict (e.g. checkbox inputs). The convention is that - keys in the `get` dict that end with '[]' will always + keys in the `data` dict that end with '[]' will always have list values in the returned dict. - For example, if the `get` dict contains {'input_1[]': 'test' } + For example, if the `data` dict contains {'input_1[]': 'test' } then the output dict would contain {'1': ['test'] } (the value is a list). Raises an exception if: - -A key in the `get` dictionary does not contain at least one underscore + -A key in the `data` dictionary does not contain at least one underscore (e.g. "input" is invalid, but "input_1" is valid) -Two keys end up with the same name in the returned dict. @@ -758,7 +760,7 @@ class CapaModule(CapaFields, XModule): """ answers = dict() - for key in get: + for key in data: # e.g. input_resistor_1 ==> resistor_1 _, _, name = key.partition('_') @@ -777,9 +779,9 @@ class CapaModule(CapaFields, XModule): name = name[:-2] if is_list_key else name if is_list_key: - val = get.getlist(key) + val = data.getlist(key) else: - val = get[key] + val = data[key] # If the name already exists, then we don't want # to override it. Raise an error instead @@ -801,7 +803,7 @@ class CapaModule(CapaFields, XModule): 'max_value': score['total'], }) - def check_problem(self, get): + def check_problem(self, data): """ Checks whether answers to a problem are correct @@ -813,8 +815,9 @@ class CapaModule(CapaFields, XModule): event_info['state'] = self.lcp.get_state() event_info['problem_id'] = self.location.url() - answers = self.make_dict_of_responses(get) + answers = self.make_dict_of_responses(data) event_info['answers'] = convert_files_to_filenames(answers) + # Too late. Cannot submit if self.closed(): event_info['failure'] = 'closed' @@ -972,7 +975,7 @@ class CapaModule(CapaFields, XModule): return {'success': success} - def save_problem(self, get): + def save_problem(self, data): """ Save the passed in answers. Returns a dict { 'success' : bool, 'msg' : message } @@ -982,7 +985,7 @@ class CapaModule(CapaFields, XModule): event_info['state'] = self.lcp.get_state() event_info['problem_id'] = self.location.url() - answers = self.make_dict_of_responses(get) + answers = self.make_dict_of_responses(data) event_info['answers'] = answers # Too late. Cannot submit @@ -1011,7 +1014,7 @@ class CapaModule(CapaFields, XModule): return {'success': True, 'msg': msg} - def reset_problem(self, get): + def reset_problem(self, _data): """ Changes problem state to unfinished -- removes student answers, and causes problem to rerender itself. diff --git a/common/lib/xmodule/xmodule/combined_open_ended_module.py b/common/lib/xmodule/xmodule/combined_open_ended_module.py index 68285cae0d..52d98f032e 100644 --- a/common/lib/xmodule/xmodule/combined_open_ended_module.py +++ b/common/lib/xmodule/xmodule/combined_open_ended_module.py @@ -204,9 +204,9 @@ class CombinedOpenEndedModule(CombinedOpenEndedFields, XModule): return_value = self.child_module.get_html() return return_value - def handle_ajax(self, dispatch, get): + def handle_ajax(self, dispatch, data): self.save_instance_data() - return_value = self.child_module.handle_ajax(dispatch, get) + return_value = self.child_module.handle_ajax(dispatch, data) self.save_instance_data() return return_value @@ -266,4 +266,3 @@ class CombinedOpenEndedDescriptor(CombinedOpenEndedFields, RawDescriptor): non_editable_fields.extend([CombinedOpenEndedDescriptor.due, CombinedOpenEndedDescriptor.graceperiod, CombinedOpenEndedDescriptor.markdown, CombinedOpenEndedDescriptor.version]) return non_editable_fields - diff --git a/common/lib/xmodule/xmodule/conditional_module.py b/common/lib/xmodule/xmodule/conditional_module.py index 6dc86880ae..5bdc8e7797 100644 --- a/common/lib/xmodule/xmodule/conditional_module.py +++ b/common/lib/xmodule/xmodule/conditional_module.py @@ -135,7 +135,7 @@ class ConditionalModule(ConditionalFields, XModule): 'depends': ';'.join(self.required_html_ids) }) - def handle_ajax(self, dispatch, post): + def handle_ajax(self, _dispatch, _data): """This is called by courseware.moduleodule_render, to handle an AJAX call. """ 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 9fc438d4c0..538901890c 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 @@ -500,10 +500,10 @@ class CombinedOpenEndedV1Module(): pass return return_html - def get_rubric(self, get): + def get_rubric(self, _data): """ Gets the results of a given grader via ajax. - Input: AJAX get dictionary + Input: AJAX data dictionary Output: Dictionary to be rendered via ajax that contains the result html. """ all_responses = [] @@ -532,10 +532,10 @@ class CombinedOpenEndedV1Module(): html = self.system.render_template('{0}/combined_open_ended_results.html'.format(self.TEMPLATE_DIR), context) return {'html': html, 'success': True} - def get_legend(self, get): + def get_legend(self, _data): """ Gets the results of a given grader via ajax. - Input: AJAX get dictionary + Input: AJAX data dictionary Output: Dictionary to be rendered via ajax that contains the result html. """ context = { @@ -544,10 +544,10 @@ class CombinedOpenEndedV1Module(): html = self.system.render_template('{0}/combined_open_ended_legend.html'.format(self.TEMPLATE_DIR), context) return {'html': html, 'success': True} - def get_results(self, get): + def get_results(self, _data): """ Gets the results of a given grader via ajax. - Input: AJAX get dictionary + Input: AJAX data dictionary Output: Dictionary to be rendered via ajax that contains the result html. """ self.update_task_states() @@ -588,19 +588,19 @@ class CombinedOpenEndedV1Module(): html = self.system.render_template('{0}/combined_open_ended_results.html'.format(self.TEMPLATE_DIR), context) return {'html': html, 'success': True} - def get_status_ajax(self, get): + def get_status_ajax(self, _data): """ Gets the results of a given grader via ajax. - Input: AJAX get dictionary + Input: AJAX data dictionary Output: Dictionary to be rendered via ajax that contains the result html. """ html = self.get_status(True) return {'html': html, 'success': True} - def handle_ajax(self, dispatch, get): + def handle_ajax(self, dispatch, data): """ This is called by courseware.module_render, to handle an AJAX call. - "get" is request.POST. + "data" is request.POST. Returns a json dictionary: { 'progress_changed' : True/False, @@ -618,30 +618,30 @@ class CombinedOpenEndedV1Module(): } if dispatch not in handlers: - return_html = self.current_task.handle_ajax(dispatch, get, self.system) + return_html = self.current_task.handle_ajax(dispatch, data, self.system) return self.update_task_states_ajax(return_html) - d = handlers[dispatch](get) + d = handlers[dispatch](data) return json.dumps(d, cls=ComplexEncoder) - def next_problem(self, get): + def next_problem(self, _data): """ Called via ajax to advance to the next problem. - Input: AJAX get request. + Input: AJAX data request. Output: Dictionary to be rendered """ self.update_task_states() return {'success': True, 'html': self.get_html_nonsystem(), 'allow_reset': self.ready_to_reset} - def reset(self, get): + def reset(self, data): """ If resetting is allowed, reset the state of the combined open ended module. - Input: AJAX get dictionary + Input: AJAX data dictionary Output: AJAX dictionary to tbe rendered """ if self.state != self.DONE: if not self.ready_to_reset: - return self.out_of_sync_error(get) + return self.out_of_sync_error(data) if self.student_attempts > self.attempts: return { @@ -789,13 +789,13 @@ class CombinedOpenEndedV1Module(): return progress_object - def out_of_sync_error(self, get, msg=''): + def out_of_sync_error(self, data, msg=''): """ return dict out-of-sync error message, and also log. """ #This is a dev_facing_error - log.warning("Combined module state out sync. state: %r, get: %r. %s", - self.state, get, msg) + log.warning("Combined module state out sync. state: %r, data: %r. %s", + self.state, data, msg) #This is a student_facing_error return {'success': False, 'error': 'The problem state got out-of-sync. Please try reloading the page.'} 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 2ac55a8318..0f0851fbf7 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 @@ -122,17 +122,17 @@ class OpenEndedModule(openendedchild.OpenEndedChild): self.payload = {'grader_payload': updated_grader_payload} - def skip_post_assessment(self, get, system): + def skip_post_assessment(self, _data, system): """ Ajax function that allows one to skip the post assessment phase - @param get: AJAX dictionary + @param data: AJAX dictionary @param system: ModuleSystem @return: Success indicator """ self.child_state = self.DONE return {'success': True} - def message_post(self, get, system): + def message_post(self, data, system): """ Handles a student message post (a reaction to the grade they received from an open ended grader type) Returns a boolean success/fail and an error message @@ -141,7 +141,7 @@ class OpenEndedModule(openendedchild.OpenEndedChild): event_info = dict() event_info['problem_id'] = self.location_string event_info['student_id'] = system.anonymous_student_id - event_info['survey_responses'] = get + event_info['survey_responses'] = data survey_responses = event_info['survey_responses'] for tag in ['feedback', 'submission_id', 'grader_id', 'score']: @@ -587,10 +587,10 @@ class OpenEndedModule(openendedchild.OpenEndedChild): html = system.render_template('{0}/open_ended_evaluation.html'.format(self.TEMPLATE_DIR), context) return html - def handle_ajax(self, dispatch, get, system): + def handle_ajax(self, dispatch, data, system): ''' This is called by courseware.module_render, to handle an AJAX call. - "get" is request.POST. + "data" is request.POST. Returns a json dictionary: { 'progress_changed' : True/False, @@ -612,7 +612,7 @@ class OpenEndedModule(openendedchild.OpenEndedChild): return json.dumps({'error': 'Error handling action. Please try again.', 'success': False}) before = self.get_progress() - d = handlers[dispatch](get, system) + d = handlers[dispatch](data, system) after = self.get_progress() d.update({ 'progress_changed': after != before, @@ -620,20 +620,20 @@ class OpenEndedModule(openendedchild.OpenEndedChild): }) return json.dumps(d, cls=ComplexEncoder) - def check_for_score(self, get, system): + def check_for_score(self, _data, system): """ Checks to see if a score has been received yet. - @param get: AJAX get dictionary + @param data: AJAX dictionary @param system: Modulesystem (needed to align with other ajax functions) @return: Returns the current state """ state = self.child_state return {'state': state} - def save_answer(self, get, system): + def save_answer(self, data, system): """ Saves a student answer - @param get: AJAX get dictionary + @param data: AJAX dictionary @param system: modulesystem @return: Success indicator """ @@ -644,17 +644,17 @@ class OpenEndedModule(openendedchild.OpenEndedChild): return msg if self.child_state != self.INITIAL: - return self.out_of_sync_error(get) + return self.out_of_sync_error(data) # add new history element with answer and empty score and hint. - success, get = self.append_image_to_student_answer(get) + success, data = self.append_image_to_student_answer(data) error_message = "" if success: success, allowed_to_submit, error_message = self.check_if_student_can_submit() if allowed_to_submit: - get['student_answer'] = OpenEndedModule.sanitize_html(get['student_answer']) - self.new_history_entry(get['student_answer']) - self.send_to_grader(get['student_answer'], system) + data['student_answer'] = OpenEndedModule.sanitize_html(data['student_answer']) + self.new_history_entry(data['student_answer']) + self.send_to_grader(data['student_answer'], system) self.change_state(self.ASSESSING) else: # Error message already defined @@ -666,17 +666,17 @@ class OpenEndedModule(openendedchild.OpenEndedChild): return { 'success': success, 'error': error_message, - 'student_response': get['student_answer'] + 'student_response': data['student_answer'] } - def update_score(self, get, system): + def update_score(self, data, system): """ Updates the current score via ajax. Called by xqueue. - Input: AJAX get dictionary, modulesystem + Input: AJAX data dictionary, modulesystem Output: None """ - queuekey = get['queuekey'] - score_msg = get['xqueue_body'] + queuekey = data['queuekey'] + score_msg = data['xqueue_body'] # TODO: Remove need for cmap self._update_score(score_msg, queuekey, system) 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 4f524d2cd7..047ab0244c 100644 --- a/common/lib/xmodule/xmodule/open_ended_grading_classes/openendedchild.py +++ b/common/lib/xmodule/xmodule/open_ended_grading_classes/openendedchild.py @@ -272,13 +272,13 @@ class OpenEndedChild(object): return None return None - def out_of_sync_error(self, get, msg=''): + def out_of_sync_error(self, data, msg=''): """ return dict out-of-sync error message, and also log. """ # This is a dev_facing_error - log.warning("Open ended child state out sync. state: %r, get: %r. %s", - self.child_state, get, msg) + log.warning("Open ended child state out sync. state: %r, data: %r. %s", + self.child_state, data, msg) # This is a student_facing_error return {'success': False, 'error': 'The problem state got out-of-sync. Please try reloading the page.'} @@ -345,24 +345,24 @@ class OpenEndedChild(object): return success, image_ok, s3_public_url - def check_for_image_and_upload(self, get_data): + def check_for_image_and_upload(self, data): """ Checks to see if an image was passed back in the AJAX query. If so, it will upload it to S3 - @param get_data: AJAX get data - @return: Success, whether or not a file was in the get dictionary, + @param data: AJAX data + @return: Success, whether or not a file was in the data dictionary, and the html corresponding to the uploaded image """ has_file_to_upload = False uploaded_to_s3 = False image_tag = "" image_ok = False - if 'can_upload_files' in get_data: - if get_data['can_upload_files'] in ['true', '1']: + if 'can_upload_files' in data: + if data['can_upload_files'] in ['true', '1']: has_file_to_upload = True - file = get_data['student_file'][0] - uploaded_to_s3, image_ok, s3_public_url = self.upload_image_to_s3(file) + student_file = data['student_file'][0] + uploaded_to_s3, image_ok, s3_public_url = self.upload_image_to_s3(student_file) if uploaded_to_s3: - image_tag = self.generate_image_tag_from_url(s3_public_url, file.name) + image_tag = self.generate_image_tag_from_url(s3_public_url, student_file.name) return has_file_to_upload, uploaded_to_s3, image_ok, image_tag @@ -371,27 +371,27 @@ class OpenEndedChild(object): Makes an image tag from a given URL @param s3_public_url: URL of the image @param image_name: Name of the image - @return: Boolean success, updated AJAX get data + @return: Boolean success, updated AJAX data """ image_template = """ {1} """.format(s3_public_url, image_name) return image_template - def append_image_to_student_answer(self, get_data): + def append_image_to_student_answer(self, data): """ Adds an image to a student answer after uploading it to S3 - @param get_data: AJAx get data - @return: Boolean success, updated AJAX get data + @param data: AJAx data + @return: Boolean success, updated AJAX data """ overall_success = False if not self.accept_file_upload: # If the question does not accept file uploads, do not do anything - return True, get_data + return True, data - has_file_to_upload, uploaded_to_s3, image_ok, image_tag = self.check_for_image_and_upload(get_data) + has_file_to_upload, uploaded_to_s3, image_ok, image_tag = self.check_for_image_and_upload(data) if uploaded_to_s3 and has_file_to_upload and image_ok: - get_data['student_answer'] += image_tag + data['student_answer'] += image_tag overall_success = True elif has_file_to_upload and not uploaded_to_s3 and image_ok: # In this case, an image was submitted by the student, but the image could not be uploaded to S3. Likely @@ -403,12 +403,12 @@ class OpenEndedChild(object): overall_success = True elif not has_file_to_upload: # If there is no file to upload, probably the student has embedded the link in the answer text - success, get_data['student_answer'] = self.check_for_url_in_text(get_data['student_answer']) + success, data['student_answer'] = self.check_for_url_in_text(data['student_answer']) overall_success = success # log.debug("Has file: {0} Uploaded: {1} Image Ok: {2}".format(has_file_to_upload, uploaded_to_s3, image_ok)) - return overall_success, get_data + return overall_success, data def check_for_url_in_text(self, string): """ diff --git a/common/lib/xmodule/xmodule/open_ended_grading_classes/self_assessment_module.py b/common/lib/xmodule/xmodule/open_ended_grading_classes/self_assessment_module.py index 7beca7a72f..a5498289e2 100644 --- a/common/lib/xmodule/xmodule/open_ended_grading_classes/self_assessment_module.py +++ b/common/lib/xmodule/xmodule/open_ended_grading_classes/self_assessment_module.py @@ -75,10 +75,10 @@ class SelfAssessmentModule(openendedchild.OpenEndedChild): html = system.render_template('{0}/self_assessment_prompt.html'.format(self.TEMPLATE_DIR), context) return html - def handle_ajax(self, dispatch, get, system): + def handle_ajax(self, dispatch, data, system): """ This is called by courseware.module_render, to handle an AJAX call. - "get" is request.POST. + "data" is request.POST. Returns a json dictionary: { 'progress_changed' : True/False, @@ -99,7 +99,7 @@ class SelfAssessmentModule(openendedchild.OpenEndedChild): return json.dumps({'error': 'Error handling action. Please try again.', 'success': False}) before = self.get_progress() - d = handlers[dispatch](get, system) + d = handlers[dispatch](data, system) after = self.get_progress() d.update({ 'progress_changed': after != before, @@ -160,12 +160,12 @@ class SelfAssessmentModule(openendedchild.OpenEndedChild): return system.render_template('{0}/self_assessment_hint.html'.format(self.TEMPLATE_DIR), context) - def save_answer(self, get, system): + def save_answer(self, data, system): """ After the answer is submitted, show the rubric. Args: - get: the GET dictionary passed to the ajax request. Should contain + data: the request dictionary passed to the ajax request. Should contain a key 'student_answer' Returns: @@ -178,16 +178,16 @@ class SelfAssessmentModule(openendedchild.OpenEndedChild): return msg if self.child_state != self.INITIAL: - return self.out_of_sync_error(get) + return self.out_of_sync_error(data) error_message = "" # add new history element with answer and empty score and hint. - success, get = self.append_image_to_student_answer(get) + success, data = self.append_image_to_student_answer(data) if success: success, allowed_to_submit, error_message = self.check_if_student_can_submit() if allowed_to_submit: - get['student_answer'] = SelfAssessmentModule.sanitize_html(get['student_answer']) - self.new_history_entry(get['student_answer']) + data['student_answer'] = SelfAssessmentModule.sanitize_html(data['student_answer']) + self.new_history_entry(data['student_answer']) self.change_state(self.ASSESSING) else: # Error message already defined @@ -200,10 +200,10 @@ class SelfAssessmentModule(openendedchild.OpenEndedChild): 'success': success, 'rubric_html': self.get_rubric_html(system), 'error': error_message, - 'student_response': get['student_answer'], + 'student_response': data['student_answer'], } - def save_assessment(self, get, system): + def save_assessment(self, data, _system): """ Save the assessment. If the student said they're right, don't ask for a hint, and go straight to the done state. Otherwise, do ask for a hint. @@ -219,11 +219,11 @@ class SelfAssessmentModule(openendedchild.OpenEndedChild): """ if self.child_state != self.ASSESSING: - return self.out_of_sync_error(get) + return self.out_of_sync_error(data) try: - score = int(get['assessment']) - score_list = get.getlist('score_list[]') + score = int(data['assessment']) + score_list = data.getlist('score_list[]') for i in xrange(0, len(score_list)): score_list[i] = int(score_list[i]) except ValueError: @@ -244,7 +244,7 @@ class SelfAssessmentModule(openendedchild.OpenEndedChild): d['state'] = self.child_state return d - def save_hint(self, get, system): + def save_hint(self, data, _system): ''' Not used currently, as hints have been removed from the system. Save the hint. @@ -258,9 +258,9 @@ class SelfAssessmentModule(openendedchild.OpenEndedChild): if self.child_state != self.POST_ASSESSMENT: # Note: because we only ask for hints on wrong answers, may not have # the same number of hints and answers. - return self.out_of_sync_error(get) + return self.out_of_sync_error(data) - self.record_latest_post_assessment(get['hint']) + self.record_latest_post_assessment(data['hint']) self.change_state(self.DONE) return {'success': True, diff --git a/common/lib/xmodule/xmodule/peer_grading_module.py b/common/lib/xmodule/xmodule/peer_grading_module.py index a13fef8e40..7df444a892 100644 --- a/common/lib/xmodule/xmodule/peer_grading_module.py +++ b/common/lib/xmodule/xmodule/peer_grading_module.py @@ -133,8 +133,8 @@ class PeerGradingModule(PeerGradingFields, XModule): """ return {'success': False, 'error': msg} - def _check_required(self, get, required): - actual = set(get.keys()) + def _check_required(self, data, required): + actual = set(data.keys()) missing = required - actual if len(missing) > 0: return False, "Missing required keys: {0}".format(', '.join(missing)) @@ -153,7 +153,7 @@ class PeerGradingModule(PeerGradingFields, XModule): else: return self.peer_grading_problem({'location': self.link_to_location})['html'] - def handle_ajax(self, dispatch, get): + def handle_ajax(self, dispatch, data): """ Needs to be implemented by child modules. Handles AJAX events. @return: @@ -173,7 +173,7 @@ class PeerGradingModule(PeerGradingFields, XModule): # This is a dev_facing_error return json.dumps({'error': 'Error handling action. Please try again.', 'success': False}) - d = handlers[dispatch](get) + d = handlers[dispatch](data) return json.dumps(d, cls=ComplexEncoder) @@ -244,7 +244,7 @@ class PeerGradingModule(PeerGradingFields, XModule): max_grade = self.max_grade return max_grade - def get_next_submission(self, get): + def get_next_submission(self, data): """ Makes a call to the grading controller for the next essay that should be graded Returns a json dict with the following keys: @@ -263,11 +263,11 @@ class PeerGradingModule(PeerGradingFields, XModule): 'error': if success is False, will have an error message with more info. """ required = set(['location']) - success, message = self._check_required(get, required) + success, message = self._check_required(data, required) if not success: return self._err_response(message) grader_id = self.system.anonymous_student_id - location = get['location'] + location = data['location'] try: response = self.peer_gs.get_next_submission(location, grader_id) @@ -280,7 +280,7 @@ class PeerGradingModule(PeerGradingFields, XModule): return {'success': False, 'error': EXTERNAL_GRADER_NO_CONTACT_ERROR} - def save_grade(self, get): + def save_grade(self, data): """ Saves the grade of a given submission. Input: @@ -298,18 +298,18 @@ class PeerGradingModule(PeerGradingFields, XModule): required = set(['location', 'submission_id', 'submission_key', 'score', 'feedback', 'rubric_scores[]', 'submission_flagged']) - success, message = self._check_required(get, required) + success, message = self._check_required(data, required) if not success: return self._err_response(message) grader_id = self.system.anonymous_student_id - location = get.get('location') - submission_id = get.get('submission_id') - score = get.get('score') - feedback = get.get('feedback') - submission_key = get.get('submission_key') - rubric_scores = get.getlist('rubric_scores[]') - submission_flagged = get.get('submission_flagged') + location = data.get('location') + submission_id = data.get('submission_id') + score = data.get('score') + feedback = data.get('feedback') + submission_key = data.get('submission_key') + rubric_scores = data.getlist('rubric_scores[]') + submission_flagged = data.get('submission_flagged') try: response = self.peer_gs.save_grade(location, grader_id, submission_id, @@ -328,7 +328,7 @@ class PeerGradingModule(PeerGradingFields, XModule): 'error': EXTERNAL_GRADER_NO_CONTACT_ERROR } - def is_student_calibrated(self, get): + def is_student_calibrated(self, data): """ Calls the grading controller to see if the given student is calibrated on the given problem @@ -347,12 +347,12 @@ class PeerGradingModule(PeerGradingFields, XModule): """ required = set(['location']) - success, message = self._check_required(get, required) + success, message = self._check_required(data, required) if not success: return self._err_response(message) grader_id = self.system.anonymous_student_id - location = get['location'] + location = data['location'] try: response = self.peer_gs.is_student_calibrated(location, grader_id) @@ -367,7 +367,7 @@ class PeerGradingModule(PeerGradingFields, XModule): 'error': EXTERNAL_GRADER_NO_CONTACT_ERROR } - def show_calibration_essay(self, get): + def show_calibration_essay(self, data): """ Fetch the next calibration essay from the grading controller and return it Inputs: @@ -392,13 +392,13 @@ class PeerGradingModule(PeerGradingFields, XModule): """ required = set(['location']) - success, message = self._check_required(get, required) + success, message = self._check_required(data, required) if not success: return self._err_response(message) grader_id = self.system.anonymous_student_id - location = get['location'] + location = data['location'] try: response = self.peer_gs.show_calibration_essay(location, grader_id) return response @@ -417,8 +417,7 @@ class PeerGradingModule(PeerGradingFields, XModule): return {'success': False, 'error': 'Error displaying submission. Please notify course staff.'} - - def save_calibration_essay(self, get): + def save_calibration_essay(self, data): """ Saves the grader's grade of a given calibration. Input: @@ -437,17 +436,17 @@ class PeerGradingModule(PeerGradingFields, XModule): """ required = set(['location', 'submission_id', 'submission_key', 'score', 'feedback', 'rubric_scores[]']) - success, message = self._check_required(get, required) + success, message = self._check_required(data, required) if not success: return self._err_response(message) grader_id = self.system.anonymous_student_id - location = get.get('location') - calibration_essay_id = get.get('submission_id') - submission_key = get.get('submission_key') - score = get.get('score') - feedback = get.get('feedback') - rubric_scores = get.getlist('rubric_scores[]') + location = data.get('location') + calibration_essay_id = data.get('submission_id') + submission_key = data.get('submission_key') + score = data.get('score') + feedback = data.get('feedback') + rubric_scores = data.getlist('rubric_scores[]') try: response = self.peer_gs.save_calibration_essay(location, grader_id, calibration_essay_id, @@ -473,8 +472,7 @@ class PeerGradingModule(PeerGradingFields, XModule): }) return html - - def peer_grading(self, get=None): + def peer_grading(self, _data=None): ''' Show a peer grading interface ''' @@ -553,11 +551,11 @@ class PeerGradingModule(PeerGradingFields, XModule): return html - def peer_grading_problem(self, get=None): + def peer_grading_problem(self, data=None): ''' Show individual problem interface ''' - if get is None or get.get('location') is None: + if data is None or data.get('location') is None: if not self.use_for_single_location: # This is an error case, because it must be set to use a single location to be called without get parameters # This is a dev_facing_error @@ -566,8 +564,8 @@ class PeerGradingModule(PeerGradingFields, XModule): return {'html': "", 'success': False} problem_location = self.link_to_location - elif get.get('location') is not None: - problem_location = get.get('location') + elif data.get('location') is not None: + problem_location = data.get('location') ajax_url = self.ajax_url html = self.system.render_template('peer_grading/peer_grading_problem.html', { @@ -617,4 +615,3 @@ class PeerGradingDescriptor(PeerGradingFields, RawDescriptor): non_editable_fields.extend([PeerGradingFields.due_date, PeerGradingFields.grace_period_string, PeerGradingFields.max_grade]) return non_editable_fields - diff --git a/common/lib/xmodule/xmodule/poll_module.py b/common/lib/xmodule/xmodule/poll_module.py index 9f2359865a..ca12f239ab 100644 --- a/common/lib/xmodule/xmodule/poll_module.py +++ b/common/lib/xmodule/xmodule/poll_module.py @@ -47,12 +47,12 @@ class PollModule(PollFields, XModule): css = {'scss': [resource_string(__name__, 'css/poll/display.scss')]} js_module_name = "Poll" - def handle_ajax(self, dispatch, get): + def handle_ajax(self, dispatch, data): """Ajax handler. Args: dispatch: string request slug - get: dict request get parameters + data: dict request data parameters Returns: json string diff --git a/common/lib/xmodule/xmodule/seq_module.py b/common/lib/xmodule/xmodule/seq_module.py index 580f51f6dd..088967ebc0 100644 --- a/common/lib/xmodule/xmodule/seq_module.py +++ b/common/lib/xmodule/xmodule/seq_module.py @@ -62,10 +62,10 @@ class SequenceModule(SequenceFields, XModule): progress = reduce(Progress.add_counts, progresses) return progress - def handle_ajax(self, dispatch, get): # TODO: bounds checking + def handle_ajax(self, dispatch, data): # TODO: bounds checking ''' get = request.POST instance ''' if dispatch == 'goto_position': - self.position = int(get['position']) + self.position = int(data['position']) return json.dumps({'success': True}) raise NotFoundError('Unexpected dispatch type') diff --git a/common/lib/xmodule/xmodule/tests/test_logic.py b/common/lib/xmodule/xmodule/tests/test_logic.py index e62b9a1cee..9be533885c 100644 --- a/common/lib/xmodule/xmodule/tests/test_logic.py +++ b/common/lib/xmodule/xmodule/tests/test_logic.py @@ -40,9 +40,9 @@ class LogicTest(unittest.TestCase): self.raw_model_data ) - def ajax_request(self, dispatch, get): + def ajax_request(self, dispatch, data): """Call Xmodule.handle_ajax.""" - return json.loads(self.xmodule.handle_ajax(dispatch, get)) + return json.loads(self.xmodule.handle_ajax(dispatch, data)) class PollModuleTest(LogicTest): diff --git a/common/lib/xmodule/xmodule/timelimit_module.py b/common/lib/xmodule/xmodule/timelimit_module.py index 9446176f01..3f52ae0baa 100644 --- a/common/lib/xmodule/xmodule/timelimit_module.py +++ b/common/lib/xmodule/xmodule/timelimit_module.py @@ -98,7 +98,7 @@ class TimeLimitModule(TimeLimitFields, XModule): progress = reduce(Progress.add_counts, progresses) return progress - def handle_ajax(self, dispatch, get): + def handle_ajax(self, _dispatch, _data): raise NotFoundError('Unexpected dispatch type') def render(self): @@ -141,4 +141,3 @@ class TimeLimitDescriptor(TimeLimitFields, XMLEditingDescriptor, XmlDescriptor): xml_object.append( etree.fromstring(child.export_to_xml(resource_fs))) return xml_object - diff --git a/common/lib/xmodule/xmodule/video_module.py b/common/lib/xmodule/xmodule/video_module.py index 6344da7994..04daaea3f2 100644 --- a/common/lib/xmodule/xmodule/video_module.py +++ b/common/lib/xmodule/xmodule/video_module.py @@ -54,9 +54,9 @@ class VideoModule(VideoFields, XModule): def __init__(self, *args, **kwargs): XModule.__init__(self, *args, **kwargs) - def handle_ajax(self, dispatch, get): + def handle_ajax(self, dispatch, data): """This is not being called right now and we raise 404 error.""" - log.debug(u"GET {0}".format(get)) + log.debug(u"GET {0}".format(data)) log.debug(u"DISPATCH {0}".format(dispatch)) raise Http404() diff --git a/common/lib/xmodule/xmodule/videoalpha_module.py b/common/lib/xmodule/xmodule/videoalpha_module.py index a64e094a58..6b27bcda2b 100644 --- a/common/lib/xmodule/xmodule/videoalpha_module.py +++ b/common/lib/xmodule/xmodule/videoalpha_module.py @@ -125,9 +125,9 @@ class VideoAlphaModule(VideoAlphaFields, XModule): return parse_time(xmltree.get('start_time')), parse_time(xmltree.get('end_time')) - def handle_ajax(self, dispatch, get): + def handle_ajax(self, dispatch, data): """This is not being called right now and we raise 404 error.""" - log.debug(u"GET {0}".format(get)) + log.debug(u"GET {0}".format(data)) log.debug(u"DISPATCH {0}".format(dispatch)) raise Http404() diff --git a/common/lib/xmodule/xmodule/word_cloud_module.py b/common/lib/xmodule/xmodule/word_cloud_module.py index ac5b3051de..a7f3f92795 100644 --- a/common/lib/xmodule/xmodule/word_cloud_module.py +++ b/common/lib/xmodule/xmodule/word_cloud_module.py @@ -168,12 +168,12 @@ class WordCloudModule(WordCloudFields, XModule): )[:amount] ) - def handle_ajax(self, dispatch, post): + def handle_ajax(self, dispatch, data): """Ajax handler. Args: dispatch: string request slug - post: dict request get parameters + data: dict request get parameters Returns: json string @@ -187,7 +187,7 @@ class WordCloudModule(WordCloudFields, XModule): # Student words from client. # FIXME: we must use raw JSON, not a post data (multipart/form-data) - raw_student_words = post.getlist('student_words[]') + raw_student_words = data.getlist('student_words[]') student_words = filter(None, map(self.good_word, raw_student_words)) self.student_words = student_words diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index f5705bf662..0f5bbf4f2e 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -272,9 +272,9 @@ class XModule(XModuleFields, HTMLSnippet, XBlock): ''' return None - def handle_ajax(self, _dispatch, _get): + def handle_ajax(self, _dispatch, _data): ''' dispatch is last part of the URL. - get is a dictionary-like object ''' + data is a dictionary-like object with the content of the request''' return "" diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index 15a6ad2dab..d17efa3697 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/lms/djangoapps/courseware/module_render.py @@ -223,7 +223,7 @@ def get_module_for_descriptor_internal(user, descriptor, model_data_cache, cours relative_xqueue_callback_url = reverse('xqueue_callback', kwargs=dict(course_id=course_id, userid=str(user.id), - id=descriptor.location.url(), + mod_id=descriptor.location.url(), dispatch=dispatch), ) return xqueue_callback_url_prefix + relative_xqueue_callback_url @@ -399,40 +399,47 @@ def get_module_for_descriptor_internal(user, descriptor, model_data_cache, cours @csrf_exempt -def xqueue_callback(request, course_id, userid, id, dispatch): +def xqueue_callback(request, course_id, userid, mod_id, dispatch): ''' Entry point for graded results from the queueing system. ''' + data = request.POST.copy() + # Test xqueue package, which we expect to be: # xpackage = {'xqueue_header': json.dumps({'lms_key':'secretkey',...}), # 'xqueue_body' : 'Message from grader'} - get = request.POST.copy() for key in ['xqueue_header', 'xqueue_body']: - if not get.has_key(key): + if key not in data: raise Http404 - header = json.loads(get['xqueue_header']) - if not isinstance(header, dict) or not header.has_key('lms_key'): + + header = json.loads(data['xqueue_header']) + if not isinstance(header, dict) or 'lms_key' not in header: raise Http404 # Retrieve target StudentModule user = User.objects.get(id=userid) - - model_data_cache = ModelDataCache.cache_for_descriptor_descendents(course_id, - user, modulestore().get_instance(course_id, id), depth=0, select_for_update=True) - instance = get_module(user, request, id, model_data_cache, course_id, grade_bucket_type='xqueue') + model_data_cache = ModelDataCache.cache_for_descriptor_descendents( + course_id, + user, + modulestore().get_instance(course_id, mod_id), + depth=0, + select_for_update=True + ) + instance = get_module(user, request, mod_id, model_data_cache, course_id, grade_bucket_type='xqueue') if instance is None: - log.debug("No module {0} for user {1}--access denied?".format(id, user)) + msg = "No module {0} for user {1}--access denied?".format(mod_id, user) + log.debug(msg) raise Http404 - # Transfer 'queuekey' from xqueue response header to 'get'. This is required to - # use the interface defined by 'handle_ajax' - get.update({'queuekey': header['lms_key']}) + # Transfer 'queuekey' from xqueue response header to the data. + # This is required to use the interface defined by 'handle_ajax' + data.update({'queuekey': header['lms_key']}) # We go through the "AJAX" path - # So far, the only dispatch from xqueue will be 'score_update' + # So far, the only dispatch from xqueue will be 'score_update' try: # Can ignore the return value--not used for xqueue_callback - instance.handle_ajax(dispatch, get) + instance.handle_ajax(dispatch, data) except: log.exception("error processing ajax call") raise @@ -466,23 +473,15 @@ def modx_dispatch(request, dispatch, location, course_id): if not request.user.is_authenticated(): raise PermissionDenied - # Check for submitted files and basic file size checks - p = request.POST.copy() - if request.FILES: - for fileinput_id in request.FILES.keys(): - inputfiles = request.FILES.getlist(fileinput_id) + # Get the submitted data + data = request.POST.copy() - if len(inputfiles) > settings.MAX_FILEUPLOADS_PER_INPUT: - too_many_files_msg = 'Submission aborted! Maximum %d files may be submitted at once' % \ - settings.MAX_FILEUPLOADS_PER_INPUT - return HttpResponse(json.dumps({'success': too_many_files_msg})) - - for inputfile in inputfiles: - if inputfile.size > settings.STUDENT_FILEUPLOAD_MAX_SIZE: # Bytes - file_too_big_msg = 'Submission aborted! Your file "%s" is too large (max size: %d MB)' % \ - (inputfile.name, settings.STUDENT_FILEUPLOAD_MAX_SIZE / (1000 ** 2)) - return HttpResponse(json.dumps({'success': file_too_big_msg})) - p[fileinput_id] = inputfiles + # Get and check submitted files + files = request.FILES or {} + error_msg = _check_files_limits(files) + if error_msg: + return HttpResponse(json.dumps({'success': error_msg})) + data.update(files) # Merge files into data dictionary try: descriptor = modulestore().get_instance(course_id, location) @@ -495,8 +494,11 @@ def modx_dispatch(request, dispatch, location, course_id): ) raise Http404 - model_data_cache = ModelDataCache.cache_for_descriptor_descendents(course_id, - request.user, descriptor) + model_data_cache = ModelDataCache.cache_for_descriptor_descendents( + course_id, + request.user, + descriptor + ) instance = get_module(request.user, request, location, model_data_cache, course_id, grade_bucket_type='ajax') if instance is None: @@ -507,7 +509,7 @@ def modx_dispatch(request, dispatch, location, course_id): # Let the module handle the AJAX try: - ajax_return = instance.handle_ajax(dispatch, p) + ajax_return = instance.handle_ajax(dispatch, data) # If we can't find the module, respond with a 404 except NotFoundError: @@ -529,7 +531,6 @@ def modx_dispatch(request, dispatch, location, course_id): return HttpResponse(ajax_return) - def get_score_bucket(grade, max_grade): """ Function to split arbitrary score ranges into 3 buckets. @@ -542,3 +543,30 @@ def get_score_bucket(grade, max_grade): score_bucket = "correct" return score_bucket + + +def _check_files_limits(files): + """ + Check if the files in a request are under the limits defined by + `settings.MAX_FILEUPLOADS_PER_INPUT` and + `settings.STUDENT_FILEUPLOAD_MAX_SIZE`. + + Returns None if files are correct or an error messages otherwise. + """ + for fileinput_id in files.keys(): + inputfiles = files.getlist(fileinput_id) + + # Check number of files submitted + if len(inputfiles) > settings.MAX_FILEUPLOADS_PER_INPUT: + msg = 'Submission aborted! Maximum %d files may be submitted at once' %\ + settings.MAX_FILEUPLOADS_PER_INPUT + return msg + + # Check file sizes + for inputfile in inputfiles: + if inputfile.size > settings.STUDENT_FILEUPLOAD_MAX_SIZE: # Bytes + msg = 'Submission aborted! Your file "%s" is too large (max size: %d MB)' %\ + (inputfile.name, settings.STUDENT_FILEUPLOAD_MAX_SIZE / (1000 ** 2)) + return msg + + return None diff --git a/lms/urls.py b/lms/urls.py index 52ce539f73..88916bd334 100644 --- a/lms/urls.py +++ b/lms/urls.py @@ -188,7 +188,7 @@ if settings.COURSEWARE_ENABLED: # into the database. url(r'^software-licenses$', 'licenses.views.user_software_license', name="user_software_license"), - url(r'^courses/(?P[^/]+/[^/]+/[^/]+)/xqueue/(?P[^/]*)/(?P.*?)/(?P[^/]*)$', + url(r'^courses/(?P[^/]+/[^/]+/[^/]+)/xqueue/(?P[^/]*)/(?P.*?)/(?P[^/]*)$', 'courseware.module_render.xqueue_callback', name='xqueue_callback'), url(r'^change_setting$', 'student.views.change_setting', @@ -438,5 +438,3 @@ if settings.DEBUG: #Custom error pages handler404 = 'static_template_view.views.render_404' handler500 = 'static_template_view.views.render_500' - - From ed62c5a6f944f7d917fba7819f243da0c9fac23f Mon Sep 17 00:00:00 2001 From: Ned Batchelder Date: Tue, 25 Jun 2013 14:23:16 -0400 Subject: [PATCH 26/55] Fix LMS-500: Random class in random module was None Deleting the module object isn't needed to replace it, and deleting a module object causes all of its attributes to be set to None. --- common/lib/capa/capa/safe_exec/safe_exec.py | 1 - .../lib/capa/capa/tests/test_responsetypes.py | 18 ++++++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/common/lib/capa/capa/safe_exec/safe_exec.py b/common/lib/capa/capa/safe_exec/safe_exec.py index 3ab8f0bf9e..be33bcaa5b 100644 --- a/common/lib/capa/capa/safe_exec/safe_exec.py +++ b/common/lib/capa/capa/safe_exec/safe_exec.py @@ -18,7 +18,6 @@ import random as random_module import sys random = random_module.Random(%r) random.Random = random_module.Random -del random_module sys.modules['random'] = random """ diff --git a/common/lib/capa/capa/tests/test_responsetypes.py b/common/lib/capa/capa/tests/test_responsetypes.py index 68be54b6af..594e2ca629 100644 --- a/common/lib/capa/capa/tests/test_responsetypes.py +++ b/common/lib/capa/capa/tests/test_responsetypes.py @@ -1266,6 +1266,24 @@ class CustomResponseTest(ResponseTest): msg = correct_map.get_msg('1_2_1') self.assertEqual(msg, self._get_random_number_result(problem.seed)) + def test_random_isnt_none(self): + # Bug LMS-500 says random.seed(10) fails with: + # File "", line 61, in + # File "/usr/lib/python2.7/random.py", line 116, in seed + # super(Random, self).seed(a) + # TypeError: must be type, not None + + r = random.Random() + r.seed(10) + num = r.randint(0, 1e9) + + script = textwrap.dedent(""" + random.seed(10) + num = random.randint(0, 1e9) + """) + problem = self.build_problem(script=script) + self.assertEqual(problem.context['num'], num) + def test_module_imports_inline(self): ''' Check that the correct modules are available to custom From bcbce3eff0bec489c718ea8cf414f3c38f54527a Mon Sep 17 00:00:00 2001 From: Renzo Lucioni Date: Mon, 24 Jun 2013 16:53:01 -0400 Subject: [PATCH 27/55] Add handful of events to the Segment.io whitelist --- common/lib/xmodule/xmodule/js/src/capa/display.coffee | 8 ++++---- common/static/coffee/src/logger.coffee | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/common/lib/xmodule/xmodule/js/src/capa/display.coffee b/common/lib/xmodule/xmodule/js/src/capa/display.coffee index f773fc81c4..6b355459e9 100644 --- a/common/lib/xmodule/xmodule/js/src/capa/display.coffee +++ b/common/lib/xmodule/xmodule/js/src/capa/display.coffee @@ -138,7 +138,7 @@ class @Problem # maybe preferable to consolidate all dispatches to use FormData ### check_fd: => - Logger.log 'problem_check', @answers + Logger.log 'problem_check', answers: @answers # If there are no file inputs in the problem, we can fall back on @check if $('input:file').length == 0 @@ -212,7 +212,7 @@ class @Problem $.ajaxWithPrefix("#{@url}/problem_check", settings) check: => - Logger.log 'problem_check', @answers + Logger.log 'problem_check', answers: @answers $.postWithPrefix "#{@url}/problem_check", @answers, (response) => switch response.success when 'incorrect', 'correct' @@ -224,7 +224,7 @@ class @Problem @gentle_alert response.success reset: => - Logger.log 'problem_reset', @answers + Logger.log 'problem_reset', answers: @answers $.postWithPrefix "#{@url}/problem_reset", id: @id, (response) => @render(response.html) @updateProgress response @@ -284,7 +284,7 @@ class @Problem @el.find('.capa_alert').css(opacity: 0).animate(opacity: 1, 700) save: => - Logger.log 'problem_save', @answers + Logger.log 'problem_save', answers: @answers $.postWithPrefix "#{@url}/problem_save", @answers, (response) => saveMessage = response.msg @gentle_alert saveMessage diff --git a/common/static/coffee/src/logger.coffee b/common/static/coffee/src/logger.coffee index 6da4929fb0..dbc2b8e004 100644 --- a/common/static/coffee/src/logger.coffee +++ b/common/static/coffee/src/logger.coffee @@ -1,6 +1,6 @@ class @Logger # events we want sent to Segment.io for tracking - SEGMENT_IO_WHITELIST = ["seq_goto", "seq_next", "seq_prev"] + SEGMENT_IO_WHITELIST = ["seq_goto", "seq_next", "seq_prev", "problem_check", "problem_reset", "problem_show", "problem_save"] @log: (event_type, data) -> if event_type in SEGMENT_IO_WHITELIST From 84f4361d522c9898b69fa3e16c0540126673b5cc Mon Sep 17 00:00:00 2001 From: Renzo Lucioni Date: Tue, 25 Jun 2013 15:15:28 -0400 Subject: [PATCH 28/55] Avoid changing format of data sent to our logs, and prevent problem_check event from firing twice --- common/lib/xmodule/xmodule/js/src/capa/display.coffee | 9 +++++---- common/static/coffee/src/logger.coffee | 8 ++++++-- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/common/lib/xmodule/xmodule/js/src/capa/display.coffee b/common/lib/xmodule/xmodule/js/src/capa/display.coffee index 6b355459e9..bf6aba0a21 100644 --- a/common/lib/xmodule/xmodule/js/src/capa/display.coffee +++ b/common/lib/xmodule/xmodule/js/src/capa/display.coffee @@ -138,7 +138,7 @@ class @Problem # maybe preferable to consolidate all dispatches to use FormData ### check_fd: => - Logger.log 'problem_check', answers: @answers + Logger.log 'problem_check', @answers # If there are no file inputs in the problem, we can fall back on @check if $('input:file').length == 0 @@ -212,7 +212,8 @@ class @Problem $.ajaxWithPrefix("#{@url}/problem_check", settings) check: => - Logger.log 'problem_check', answers: @answers + # Calling check from within check_fd will result in firing the 'problem_check' event twice + # Logger.log 'problem_check', @answers $.postWithPrefix "#{@url}/problem_check", @answers, (response) => switch response.success when 'incorrect', 'correct' @@ -224,7 +225,7 @@ class @Problem @gentle_alert response.success reset: => - Logger.log 'problem_reset', answers: @answers + Logger.log 'problem_reset', @answers $.postWithPrefix "#{@url}/problem_reset", id: @id, (response) => @render(response.html) @updateProgress response @@ -284,7 +285,7 @@ class @Problem @el.find('.capa_alert').css(opacity: 0).animate(opacity: 1, 700) save: => - Logger.log 'problem_save', answers: @answers + Logger.log 'problem_save', @answers $.postWithPrefix "#{@url}/problem_save", @answers, (response) => saveMessage = response.msg @gentle_alert saveMessage diff --git a/common/static/coffee/src/logger.coffee b/common/static/coffee/src/logger.coffee index dbc2b8e004..f2dfef5132 100644 --- a/common/static/coffee/src/logger.coffee +++ b/common/static/coffee/src/logger.coffee @@ -3,9 +3,13 @@ class @Logger SEGMENT_IO_WHITELIST = ["seq_goto", "seq_next", "seq_prev", "problem_check", "problem_reset", "problem_show", "problem_save"] @log: (event_type, data) -> + # Segment.io event tracking if event_type in SEGMENT_IO_WHITELIST - # Segment.io event tracking - analytics.track event_type, data + # to avoid changing the format of data sent to our servers, we only massage it here + if typeof data isnt 'object' or data is null + analytics.track event_type, value: data + else + analytics.track event_type, data $.getWithPrefix '/event', event_type: event_type From 401dc82c46085c6663c67e60d6da32291afb8028 Mon Sep 17 00:00:00 2001 From: cahrens Date: Tue, 25 Jun 2013 15:35:47 -0400 Subject: [PATCH 29/55] Don't mention edge in the subject line; use same message for edx and edge. --- cms/templates/emails/activation_email_subject.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cms/templates/emails/activation_email_subject.txt b/cms/templates/emails/activation_email_subject.txt index 0b0fb2ffe9..f4ffdccb14 100644 --- a/cms/templates/emails/activation_email_subject.txt +++ b/cms/templates/emails/activation_email_subject.txt @@ -1 +1 @@ -Your account for edX edge +Your account for edX Studio From 881d63dae7177adc4ff9e0cad595eac37d18a604 Mon Sep 17 00:00:00 2001 From: Renzo Lucioni Date: Tue, 25 Jun 2013 16:04:00 -0400 Subject: [PATCH 30/55] Fixed Jasmine tests in light of Logger changes, and wrote test to cover case where data passed is not a dictionary --- common/static/coffee/spec/logger_spec.coffee | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/common/static/coffee/spec/logger_spec.coffee b/common/static/coffee/spec/logger_spec.coffee index 8866daa570..7fe734d8b5 100644 --- a/common/static/coffee/spec/logger_spec.coffee +++ b/common/static/coffee/spec/logger_spec.coffee @@ -3,10 +3,15 @@ describe 'Logger', -> expect(window.log_event).toBe Logger.log describe 'log', -> - it 'sends an event to Segment.io, if the event is whitelisted', -> + it 'sends an event to Segment.io, if the event is whitelisted and the data is not a dictionary', -> spyOn(analytics, 'track') Logger.log 'seq_goto', 'data' - expect(analytics.track).toHaveBeenCalledWith 'seq_goto', 'data' + expect(analytics.track).toHaveBeenCalledWith 'seq_goto', value: 'data' + + it 'sends an event to Segment.io, if the event is whitelisted and the data is a dictionary', -> + spyOn(analytics, 'track') + Logger.log 'seq_goto', value: 'data' + expect(analytics.track).toHaveBeenCalledWith 'seq_goto', value: 'data' it 'send a request to log event', -> spyOn $, 'getWithPrefix' From 6c66736e3c51a3340e2322957e5885207bcf48dc Mon Sep 17 00:00:00 2001 From: Jay Zoldak Date: Tue, 25 Jun 2013 16:56:36 -0400 Subject: [PATCH 31/55] Specify a different xcontent mongo db for the acceptance tests --- cms/envs/acceptance.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/cms/envs/acceptance.py b/cms/envs/acceptance.py index 6293219f43..871b744282 100644 --- a/cms/envs/acceptance.py +++ b/cms/envs/acceptance.py @@ -40,6 +40,21 @@ MODULESTORE = { 'OPTIONS': MODULESTORE_OPTIONS } } + +CONTENTSTORE = { + 'ENGINE': 'xmodule.contentstore.mongo.MongoContentStore', + 'OPTIONS': { + 'host': 'localhost', + 'db': 'acceptance_xcontent', + }, + # allow for additional options that can be keyed on a name, e.g. 'trashcan' + 'ADDITIONAL_OPTIONS': { + 'trashcan': { + 'bucket': 'trash_fs' + } + } +} + # Set this up so that rake lms[acceptance] and running the # harvest command both use the same (test) database # which they can flush without messing up your dev db From 3f49da385f1275f5cf24959008103e89a29e050d Mon Sep 17 00:00:00 2001 From: Renzo Lucioni Date: Tue, 25 Jun 2013 17:22:05 -0400 Subject: [PATCH 32/55] Swap Logger call from check_fd to check --- common/lib/xmodule/xmodule/js/src/capa/display.coffee | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/common/lib/xmodule/xmodule/js/src/capa/display.coffee b/common/lib/xmodule/xmodule/js/src/capa/display.coffee index bf6aba0a21..1f3be9e5e9 100644 --- a/common/lib/xmodule/xmodule/js/src/capa/display.coffee +++ b/common/lib/xmodule/xmodule/js/src/capa/display.coffee @@ -138,7 +138,8 @@ class @Problem # maybe preferable to consolidate all dispatches to use FormData ### check_fd: => - Logger.log 'problem_check', @answers + # Calling check from check_fd will result in firing the 'problem_check' event twice, since it is also called in the check function. + #Logger.log 'problem_check', @answers # If there are no file inputs in the problem, we can fall back on @check if $('input:file').length == 0 @@ -212,8 +213,7 @@ class @Problem $.ajaxWithPrefix("#{@url}/problem_check", settings) check: => - # Calling check from within check_fd will result in firing the 'problem_check' event twice - # Logger.log 'problem_check', @answers + Logger.log 'problem_check', @answers $.postWithPrefix "#{@url}/problem_check", @answers, (response) => switch response.success when 'incorrect', 'correct' From 8b23eeca7e8992ac56db892d19cf909c10f9b717 Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Mon, 24 Jun 2013 15:53:24 -0400 Subject: [PATCH 33/55] Minor pylint/whitespace changes --- cms/djangoapps/contentstore/features/video-editor.py | 2 +- cms/djangoapps/contentstore/utils.py | 2 +- cms/templates/overview.html | 3 ++- common/lib/xmodule/xmodule/modulestore/draft.py | 4 ++-- common/lib/xmodule/xmodule/modulestore/mongo.py | 2 +- .../combined_open_ended_modulev1.py | 2 +- common/lib/xmodule/xmodule/tests/test_import.py | 10 ++++++---- common/lib/xmodule/xmodule/tests/test_xml_module.py | 4 ++-- common/lib/xmodule/xmodule/xml_module.py | 9 ++++----- lms/djangoapps/courseware/access.py | 4 +--- lms/djangoapps/courseware/features/problems.py | 6 +++--- lms/djangoapps/courseware/module_render.py | 2 +- 12 files changed, 25 insertions(+), 25 deletions(-) diff --git a/cms/djangoapps/contentstore/features/video-editor.py b/cms/djangoapps/contentstore/features/video-editor.py index 987b4959b8..a6865fdd6d 100644 --- a/cms/djangoapps/contentstore/features/video-editor.py +++ b/cms/djangoapps/contentstore/features/video-editor.py @@ -1,5 +1,5 @@ # disable missing docstring -#pylint: disable=C0111 +# pylint: disable=C0111 from lettuce import world, step diff --git a/cms/djangoapps/contentstore/utils.py b/cms/djangoapps/contentstore/utils.py index 0bfa70e4f5..c9c40ab95d 100644 --- a/cms/djangoapps/contentstore/utils.py +++ b/cms/djangoapps/contentstore/utils.py @@ -10,7 +10,7 @@ from xmodule.modulestore.draft import DIRECT_ONLY_CATEGORIES log = logging.getLogger(__name__) -#In order to instantiate an open ended tab automatically, need to have this data +# In order to instantiate an open ended tab automatically, need to have this data OPEN_ENDED_PANEL = {"name": "Open Ended Panel", "type": "open_ended"} NOTES_PANEL = {"name": "My Notes", "type": "notes"} EXTRA_TAB_PANELS = dict([(p['type'], p) for p in [OPEN_ENDED_PANEL, NOTES_PANEL]]) diff --git a/cms/templates/overview.html b/cms/templates/overview.html index 43d0afc263..a504d50019 100644 --- a/cms/templates/overview.html +++ b/cms/templates/overview.html @@ -167,7 +167,8 @@ %else: Will Release: ${date_utils.get_default_time_display(section.lms.start)} - Edit + Edit %endif diff --git a/common/lib/xmodule/xmodule/modulestore/draft.py b/common/lib/xmodule/xmodule/modulestore/draft.py index 94823b0be4..41c8e2ec1e 100644 --- a/common/lib/xmodule/xmodule/modulestore/draft.py +++ b/common/lib/xmodule/xmodule/modulestore/draft.py @@ -101,12 +101,12 @@ class DraftModuleStore(ModuleStoreBase): draft_items = super(DraftModuleStore, self).get_items(draft_loc, course_id=course_id, depth=depth) items = super(DraftModuleStore, self).get_items(location, course_id=course_id, depth=depth) - draft_locs_found = set(item.location._replace(revision=None) for item in draft_items) + draft_locs_found = set(item.location.replace(revision=None) for item in draft_items) non_draft_items = [ item for item in items if (item.location.revision != DRAFT - and item.location._replace(revision=None) not in draft_locs_found) + and item.location.replace(revision=None) not in draft_locs_found) ] return [wrap_draft(item) for item in draft_items + non_draft_items] diff --git a/common/lib/xmodule/xmodule/modulestore/mongo.py b/common/lib/xmodule/xmodule/modulestore/mongo.py index 40288a933b..32323d5892 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo.py @@ -195,7 +195,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem): if self.cached_metadata is not None: # parent container pointers don't differentiate between draft and non-draft # so when we do the lookup, we should do so with a non-draft location - non_draft_loc = location._replace(revision=None) + non_draft_loc = location.replace(revision=None) metadata_to_inherit = self.cached_metadata.get(non_draft_loc.url(), {}) inherit_metadata(module, metadata_to_inherit) return module 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 538901890c..1fe62035e6 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 @@ -646,7 +646,7 @@ class CombinedOpenEndedV1Module(): if self.student_attempts > self.attempts: return { 'success': False, - #This is a student_facing_error + # This is a student_facing_error 'error': ( 'You have attempted this question {0} times. ' 'You are only allowed to attempt it {1} times.' diff --git a/common/lib/xmodule/xmodule/tests/test_import.py b/common/lib/xmodule/xmodule/tests/test_import.py index 4e9a9f9600..79b49c65ae 100644 --- a/common/lib/xmodule/xmodule/tests/test_import.py +++ b/common/lib/xmodule/xmodule/tests/test_import.py @@ -157,9 +157,10 @@ class ImportTestCase(BaseCourseTestCase): self.assertEqual(child.lms.due, ImportTestCase.date.from_json(v)) self.assertEqual(child._inheritable_metadata, child._inherited_metadata) self.assertEqual(2, len(child._inherited_metadata)) - self.assertLessEqual(ImportTestCase.date.from_json( - child._inherited_metadata['start']), - datetime.datetime.now(UTC())) + self.assertLessEqual( + ImportTestCase.date.from_json(child._inherited_metadata['start']), + datetime.datetime.now(UTC()) + ) self.assertEqual(v, child._inherited_metadata['due']) # Now export and check things @@ -221,7 +222,8 @@ class ImportTestCase(BaseCourseTestCase): # why do these tests look in the internal structure v just calling child.start? self.assertLessEqual( ImportTestCase.date.from_json(child._inherited_metadata['start']), - datetime.datetime.now(UTC())) + datetime.datetime.now(UTC()) + ) def test_metadata_override_default(self): """ diff --git a/common/lib/xmodule/xmodule/tests/test_xml_module.py b/common/lib/xmodule/xmodule/tests/test_xml_module.py index 6ec82275af..6581ce58f6 100644 --- a/common/lib/xmodule/xmodule/tests/test_xml_module.py +++ b/common/lib/xmodule/xmodule/tests/test_xml_module.py @@ -248,7 +248,7 @@ class TestDeserializeFloat(TestDeserialize): test_field = Float def test_deserialize(self): - self.assertDeserializeEqual( -2, '-2') + self.assertDeserializeEqual(-2, '-2') self.assertDeserializeEqual("450", '"450"') self.assertDeserializeEqual(-2.78, '-2.78') self.assertDeserializeEqual("0.45", '"0.45"') @@ -256,7 +256,7 @@ class TestDeserializeFloat(TestDeserialize): # False can be parsed as a float (converts to 0) self.assertDeserializeEqual(False, 'false') # True can be parsed as a float (converts to 1) - self.assertDeserializeEqual( True, 'true') + self.assertDeserializeEqual(True, 'true') def test_deserialize_unsupported_types(self): self.assertDeserializeEqual('[3]', '[3]') diff --git a/common/lib/xmodule/xmodule/xml_module.py b/common/lib/xmodule/xmodule/xml_module.py index 33120ec180..2a7a15d434 100644 --- a/common/lib/xmodule/xmodule/xml_module.py +++ b/common/lib/xmodule/xmodule/xml_module.py @@ -141,9 +141,9 @@ class XmlDescriptor(XModuleDescriptor): # Related: What's the right behavior for clean_metadata? metadata_attributes = ('format', 'graceperiod', 'showanswer', 'rerandomize', 'start', 'due', 'graded', 'display_name', 'url_name', 'hide_from_toc', - 'ispublic', # if True, then course is listed for all users; see - 'xqa_key', # for xqaa server access - 'giturl', # url of git server for origin of file + 'ispublic', # if True, then course is listed for all users; see + 'xqa_key', # for xqaa server access + 'giturl', # url of git server for origin of file # information about testcenter exams is a dict (of dicts), not a string, # so it cannot be easily exportable as a course element's attribute. 'testcenter_info', @@ -347,7 +347,7 @@ class XmlDescriptor(XModuleDescriptor): model_data['children'] = children model_data['xml_attributes'] = {} - model_data['xml_attributes']['filename'] = definition.get('filename', ['', None]) # for git link + model_data['xml_attributes']['filename'] = definition.get('filename', ['', None]) # for git link for key, value in metadata.items(): if key not in set(f.name for f in cls.fields + cls.lms.fields): model_data['xml_attributes'][key] = value @@ -409,7 +409,6 @@ class XmlDescriptor(XModuleDescriptor): # don't want e.g. data_dir if attr not in self.metadata_to_strip and attr not in self.metadata_to_export_to_policy: val = val_for_xml(attr) - #logging.debug('location.category = {0}, attr = {1}'.format(self.location.category, attr)) try: xml_object.set(attr, val) except Exception, e: diff --git a/lms/djangoapps/courseware/access.py b/lms/djangoapps/courseware/access.py index ec90260928..50b536d444 100644 --- a/lms/djangoapps/courseware/access.py +++ b/lms/djangoapps/courseware/access.py @@ -523,10 +523,8 @@ def _adjust_start_date_for_beta_testers(user, descriptor): beta_group = course_beta_test_group_name(descriptor.location) if beta_group in user_groups: debug("Adjust start time: user in group %s", beta_group) - start_as_datetime = descriptor.lms.start delta = timedelta(descriptor.lms.days_early_for_beta) - effective = start_as_datetime - delta - # ...and back to time_struct + effective = descriptor.lms.start - delta return effective return descriptor.lms.start diff --git a/lms/djangoapps/courseware/features/problems.py b/lms/djangoapps/courseware/features/problems.py index 08c5207303..39b99214c8 100644 --- a/lms/djangoapps/courseware/features/problems.py +++ b/lms/djangoapps/courseware/features/problems.py @@ -2,8 +2,8 @@ Steps for problem.feature lettuce tests ''' -#pylint: disable=C0111 -#pylint: disable=W0621 +# pylint: disable=C0111 +# pylint: disable=W0621 from lettuce import world, step from lettuce.django import django_url @@ -135,7 +135,7 @@ def action_button_present(_step, buttonname, doesnt_appear): @step(u'the button with the label "([^"]*)" does( not)? appear') -def button_with_label_present(step, buttonname, doesnt_appear): +def button_with_label_present(_step, buttonname, doesnt_appear): if doesnt_appear: assert world.browser.is_text_not_present(buttonname, wait_time=5) else: diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index d17efa3697..5c12725d0a 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/lms/djangoapps/courseware/module_render.py @@ -354,7 +354,7 @@ def get_module_for_descriptor_internal(user, descriptor, model_data_cache, cours system.set('position', position) system.set('DEBUG', settings.DEBUG) if settings.MITX_FEATURES.get('ENABLE_PSYCHOMETRICS'): - system.set('psychometrics_handler', # set callback for updating PsychometricsData + system.set('psychometrics_handler', # set callback for updating PsychometricsData make_psychometrics_data_update_handler(course_id, user, descriptor.location.url())) try: From b42fe277d4f84dd0ec7192f09f172c641ef16a32 Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Mon, 24 Jun 2013 12:56:03 -0400 Subject: [PATCH 34/55] Add serial commas to modulestore definitions --- cms/envs/acceptance.py | 2 +- cms/envs/dev.py | 2 +- cms/envs/test.py | 2 +- lms/djangoapps/courseware/tests/tests.py | 2 +- lms/envs/acceptance.py | 2 +- lms/envs/cms/dev.py | 2 +- lms/envs/dev_mongo.py | 2 +- 7 files changed, 7 insertions(+), 7 deletions(-) diff --git a/cms/envs/acceptance.py b/cms/envs/acceptance.py index 871b744282..c70ca98902 100644 --- a/cms/envs/acceptance.py +++ b/cms/envs/acceptance.py @@ -23,7 +23,7 @@ MODULESTORE_OPTIONS = { 'db': 'test_xmodule', 'collection': 'acceptance_modulestore', 'fs_root': TEST_ROOT / "data", - 'render_template': 'mitxmako.shortcuts.render_to_string' + 'render_template': 'mitxmako.shortcuts.render_to_string', } MODULESTORE = { diff --git a/cms/envs/dev.py b/cms/envs/dev.py index 2dcb3640ca..26d633484e 100644 --- a/cms/envs/dev.py +++ b/cms/envs/dev.py @@ -22,7 +22,7 @@ modulestore_options = { 'db': 'xmodule', 'collection': 'modulestore', 'fs_root': GITHUB_REPO_ROOT, - 'render_template': 'mitxmako.shortcuts.render_to_string' + 'render_template': 'mitxmako.shortcuts.render_to_string', } MODULESTORE = { diff --git a/cms/envs/test.py b/cms/envs/test.py index 89813dd937..bd833426d6 100644 --- a/cms/envs/test.py +++ b/cms/envs/test.py @@ -48,7 +48,7 @@ MODULESTORE_OPTIONS = { 'db': 'test_xmodule', 'collection': 'test_modulestore', 'fs_root': TEST_ROOT / "data", - 'render_template': 'mitxmako.shortcuts.render_to_string' + 'render_template': 'mitxmako.shortcuts.render_to_string', } MODULESTORE = { diff --git a/lms/djangoapps/courseware/tests/tests.py b/lms/djangoapps/courseware/tests/tests.py index 056a73e7c8..e862ed62c3 100644 --- a/lms/djangoapps/courseware/tests/tests.py +++ b/lms/djangoapps/courseware/tests/tests.py @@ -65,7 +65,7 @@ def mongo_store_config(data_dir): 'db': 'test_xmodule', 'collection': 'modulestore_%s' % uuid4().hex, 'fs_root': data_dir, - 'render_template': 'mitxmako.shortcuts.render_to_string' + 'render_template': 'mitxmako.shortcuts.render_to_string', } } } diff --git a/lms/envs/acceptance.py b/lms/envs/acceptance.py index 700fc89670..3b87bb4326 100644 --- a/lms/envs/acceptance.py +++ b/lms/envs/acceptance.py @@ -24,7 +24,7 @@ modulestore_options = { 'db': 'test_xmodule', 'collection': 'acceptance_modulestore', 'fs_root': TEST_ROOT / "data", - 'render_template': 'mitxmako.shortcuts.render_to_string' + 'render_template': 'mitxmako.shortcuts.render_to_string', } MODULESTORE = { diff --git a/lms/envs/cms/dev.py b/lms/envs/cms/dev.py index f8c43148b0..e55c6d61b5 100644 --- a/lms/envs/cms/dev.py +++ b/lms/envs/cms/dev.py @@ -21,7 +21,7 @@ modulestore_options = { 'db': 'xmodule', 'collection': 'modulestore', 'fs_root': DATA_DIR, - 'render_template': 'mitxmako.shortcuts.render_to_string' + 'render_template': 'mitxmako.shortcuts.render_to_string', } MODULESTORE = { diff --git a/lms/envs/dev_mongo.py b/lms/envs/dev_mongo.py index 1f6b5899f1..dfbf473b45 100644 --- a/lms/envs/dev_mongo.py +++ b/lms/envs/dev_mongo.py @@ -19,7 +19,7 @@ MODULESTORE = { 'db': 'xmodule', 'collection': 'modulestore', 'fs_root': GITHUB_REPO_ROOT, - 'render_template': 'mitxmako.shortcuts.render_to_string' + 'render_template': 'mitxmako.shortcuts.render_to_string', } } } From d9575a0874eb4fefa20face16f70565b52a1da3d Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Mon, 24 Jun 2013 12:57:23 -0400 Subject: [PATCH 35/55] Remove traling commas to make json valid --- common/test/data/graphic_slider_tool/policies/2012_Fall.json | 4 ++-- common/test/data/self_assessment/policies/2012_Fall.json | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/common/test/data/graphic_slider_tool/policies/2012_Fall.json b/common/test/data/graphic_slider_tool/policies/2012_Fall.json index 6958f8432c..9058481dc8 100644 --- a/common/test/data/graphic_slider_tool/policies/2012_Fall.json +++ b/common/test/data/graphic_slider_tool/policies/2012_Fall.json @@ -9,6 +9,6 @@ "display_name": "Overview" }, "graphical_slider_tool/sample_gst": { - "display_name": "Sample GST", - }, + "display_name": "Sample GST" + } } diff --git a/common/test/data/self_assessment/policies/2012_Fall.json b/common/test/data/self_assessment/policies/2012_Fall.json index aae4670296..46529abcee 100644 --- a/common/test/data/self_assessment/policies/2012_Fall.json +++ b/common/test/data/self_assessment/policies/2012_Fall.json @@ -9,6 +9,6 @@ "display_name": "Overview" }, "selfassessment/SampleQuestion": { - "display_name": "Sample Question", - }, + "display_name": "Sample Question" + } } From 734440f4b9e7f6596d870a246dac1c1ca63c2544 Mon Sep 17 00:00:00 2001 From: Jason Bau Date: Tue, 25 Jun 2013 20:21:20 -0700 Subject: [PATCH 36/55] Refactored tests --- common/djangoapps/student/tests/tests.py | 55 ++++++++++++++---------- 1 file changed, 32 insertions(+), 23 deletions(-) diff --git a/common/djangoapps/student/tests/tests.py b/common/djangoapps/student/tests/tests.py index 10836122b8..844ddb536e 100644 --- a/common/djangoapps/student/tests/tests.py +++ b/common/djangoapps/student/tests/tests.py @@ -15,8 +15,11 @@ from django.test import TestCase from django.test.client import RequestFactory from django.contrib.auth.models import User from django.contrib.auth.hashers import UNUSABLE_PASSWORD +from django.contrib.auth.tokens import default_token_generator from django.template.loader import render_to_string, get_template, TemplateDoesNotExist from django.core.urlresolvers import is_valid_path +from django.utils.http import int_to_base36 + from mock import Mock, patch from textwrap import dedent @@ -46,36 +49,40 @@ class ResetPasswordTests(TestCase): self.user = UserFactory.create() self.user.is_active = False self.user.save() + self.token = default_token_generator.make_token(self.user) + self.uidb36 = int_to_base36(self.user.id) self.user_bad_passwd = UserFactory.create() self.user_bad_passwd.is_active = False self.user_bad_passwd.password = UNUSABLE_PASSWORD self.user_bad_passwd.save() + def test_user_bad_password_reset(self): + """Tests password reset behavior for user with password marked UNUSABLE_PASSWORD""" - @unittest.skipUnless(project_uses_password_reset, dedent("""Skipping Test because CMS has not provided - necessary templates for password reset. If this message is in LMS tests, that is a bug and needs to be fixed.""")) - @patch('student.views.password_reset_confirm') - @patch('django.core.mail.send_mail') - @patch('student.views.render_to_string', Mock(side_effect=mock_render_to_string, autospec=True)) - def test_reset_password_email(self, send_email, reset_confirm): - """Tests sending of reset password email""" - - #First test the bad password user, mainly for diff-cover sake bad_pwd_req = self.request_factory.post('/password_reset/', {'email': self.user_bad_passwd.email}) bad_pwd_resp = password_reset(bad_pwd_req) self.assertEquals(bad_pwd_resp.status_code, 200) self.assertEquals(bad_pwd_resp.content, json.dumps({'success': False, 'error': 'Invalid e-mail or user'})) - #Now test the exception cases with invalid email. + def test_nonexist_email_password_reset(self): + """Now test the exception cases with of reset_password called with invalid email.""" + bad_email_req = self.request_factory.post('/password_reset/', {'email': self.user.email+"makeItFail"}) bad_email_resp = password_reset(bad_email_req) self.assertEquals(bad_email_resp.status_code, 200) self.assertEquals(bad_email_resp.content, json.dumps({'success': False, 'error': 'Invalid e-mail or user'})) - #Now test the legit case where email should have been sent + @unittest.skipUnless(project_uses_password_reset, + dedent("""Skipping Test because CMS has not provided necessary templates for password reset. + If LMS tests print this message, that needs to be fixed.""")) + @patch('django.core.mail.send_mail') + @patch('student.views.render_to_string', Mock(side_effect=mock_render_to_string, autospec=True)) + def test_reset_password_email(self, send_email): + """Tests contents of reset password email, and that user is not active""" + good_req = self.request_factory.post('/password_reset/', {'email': self.user.email}) good_resp = password_reset(good_req) self.assertEquals(good_resp.status_code, 200) @@ -91,33 +98,35 @@ class ResetPasswordTests(TestCase): self.assertIn(self.user.email, to_addrs) #test that the user is not active - #it's a bit unsettling that we have to reload the user from the db for this test to work - #but I guess the user is cached here in the instance of ResetPasswordTests - #so the update in the view does not know to update this class. self.user = User.objects.get(pk=self.user.pk) self.assertFalse(self.user.is_active) + reset_match = re.search(r'password_reset_confirm/(?P[0-9A-Za-z]+)-(?P.+)/', msg).groupdict() + + @patch('student.views.password_reset_confirm') + def test_reset_password_bad_token(self, reset_confirm): + """Tests bad token and uidb36 in password reset""" - #now try to activate the user in the password reset phase bad_reset_req = self.request_factory.get('/password_reset_confirm/NO-OP/') - bad_reset_resp = password_reset_confirm_wrapper(bad_reset_req, 'NO', 'OP') + password_reset_confirm_wrapper(bad_reset_req, 'NO', 'OP') (confirm_args, confirm_kwargs) = reset_confirm.call_args self.assertEquals(confirm_kwargs['uidb36'], 'NO') self.assertEquals(confirm_kwargs['token'], 'OP') self.user = User.objects.get(pk=self.user.pk) self.assertFalse(self.user.is_active) - reset_match = re.search(r'password_reset_confirm/(?P[0-9A-Za-z]+)-(?P.+)/', msg).groupdict() - good_reset_req = self.request_factory.get('/password_reset_confirm/{0}-{1}/'.format(reset_match['uidb36'], - reset_match['token'])) - good_reset_resp = password_reset_confirm_wrapper(good_reset_req, reset_match['uidb36'], reset_match['token']) + @patch('student.views.password_reset_confirm') + def test_reset_password_good_token(self, reset_confirm): + """Tests good token and uidb36 in password reset""" + + good_reset_req = self.request_factory.get('/password_reset_confirm/{0}-{1}/'.format(self.uidb36, self.token)) + password_reset_confirm_wrapper(good_reset_req, self.uidb36, self.token) (confirm_args, confirm_kwargs) = reset_confirm.call_args - self.assertEquals(confirm_kwargs['uidb36'], reset_match['uidb36']) - self.assertEquals(confirm_kwargs['token'], reset_match['token']) + self.assertEquals(confirm_kwargs['uidb36'], self.uidb36) + self.assertEquals(confirm_kwargs['token'], self.token) self.user = User.objects.get(pk=self.user.pk) self.assertTrue(self.user.is_active) - class CourseEndingTest(TestCase): """Test things related to course endings: certificates, surveys, etc""" From c41c102b7a467e108f44de4dede411eef057dd54 Mon Sep 17 00:00:00 2001 From: Jason Bau Date: Tue, 25 Jun 2013 21:37:20 -0600 Subject: [PATCH 37/55] Update CHANGELOG.rst --- CHANGELOG.rst | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 21b8c9f90b..4fea30a5c5 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -5,6 +5,10 @@ These are notable changes in edx-platform. This is a rolling list of changes, in roughly chronological order, most recent first. Add your entries at or near the top. Include a label indicating the component affected. +LMS: Users are no longer auto-activated if they click "reset password" +This is now done when they click on the link in the reset password +email they receive (along with usual path through activation email). + LMS: Problem rescoring. Added options on the Grades tab of the Instructor Dashboard to allow a particular student's submission for a particular problem to be rescored. Provides an option to see a From 318372f2c0a3c838fb4b785637375e1e632bf143 Mon Sep 17 00:00:00 2001 From: cahrens Date: Tue, 25 Jun 2013 15:11:55 -0400 Subject: [PATCH 38/55] Introduce course creator group. --- cms/djangoapps/auth/authz.py | 87 ++++++++++++++++--- cms/djangoapps/auth/tests/test_authz.py | 78 +++++++++++++++++ .../contentstore/tests/test_contentstore.py | 73 ++++++++++++---- cms/djangoapps/contentstore/views/course.py | 4 +- 4 files changed, 209 insertions(+), 33 deletions(-) create mode 100644 cms/djangoapps/auth/tests/test_authz.py diff --git a/cms/djangoapps/auth/authz.py b/cms/djangoapps/auth/authz.py index 58b63abd23..f27d2fe559 100644 --- a/cms/djangoapps/auth/authz.py +++ b/cms/djangoapps/auth/authz.py @@ -1,5 +1,6 @@ from django.contrib.auth.models import User, Group from django.core.exceptions import PermissionDenied +from django.conf import settings from xmodule.modulestore import Location @@ -12,6 +13,9 @@ but this implementation should be data compatible with the LMS implementation INSTRUCTOR_ROLE_NAME = 'instructor' STAFF_ROLE_NAME = 'staff' +# This is the group of people who have permission to create new courses on edge or edx. +COURSE_CREATOR_GROUP_NAME = "course_creator_group" + # we're just making a Django group for each location/role combo # to do this we're just creating a Group name which is a formatted string # of those two variables @@ -36,10 +40,10 @@ def get_users_in_course_group_by_role(location, role): return group.user_set.all() -''' -Create all permission groups for a new course and subscribe the caller into those roles -''' def create_all_course_groups(creator, location): + """ + Create all permission groups for a new course and subscribe the caller into those roles + """ create_new_course_group(creator, location, INSTRUCTOR_ROLE_NAME) create_new_course_group(creator, location, STAFF_ROLE_NAME) @@ -56,10 +60,10 @@ def create_new_course_group(creator, location, role): return def _delete_course_group(location): - ''' + """ This is to be called only by either a command line code path or through a app which has already asserted permissions - ''' + """ # remove all memberships instructors = Group.objects.get(name=get_course_groupname_for_role(location, INSTRUCTOR_ROLE_NAME)) for user in instructors.user_set.all(): @@ -72,10 +76,10 @@ def _delete_course_group(location): user.save() def _copy_course_group(source, dest): - ''' + """ This is to be called only by either a command line code path or through an app which has already asserted permissions to do this action - ''' + """ instructors = Group.objects.get(name=get_course_groupname_for_role(source, INSTRUCTOR_ROLE_NAME)) new_instructors_group = Group.objects.get(name=get_course_groupname_for_role(dest, INSTRUCTOR_ROLE_NAME)) for user in instructors.user_set.all(): @@ -94,10 +98,29 @@ def add_user_to_course_group(caller, user, location, role): if not is_user_in_course_group_role(caller, location, INSTRUCTOR_ROLE_NAME): raise PermissionDenied - if user.is_active and user.is_authenticated: - groupname = get_course_groupname_for_role(location, role) + group = Group.objects.get(name=get_course_groupname_for_role(location, role)) + return _add_user_to_group(user, group) - group = Group.objects.get(name=groupname) + +def add_user_to_creator_group(user): + """ + Adds the user to the group of course creators. + + Note that on the edX site, we currently limit course creators to edX staff, and this + method is a no-op in that environment. + """ + (group, created) = Group.objects.get_or_create(name=COURSE_CREATOR_GROUP_NAME) + if created: + group.save() + return _add_user_to_group(user, group) + + +def _add_user_to_group(user, group): + """ + This is to be called only by either a command line code path or through an app which has already + asserted permissions to do this action + """ + if user.is_active and user.is_authenticated: user.groups.add(group) user.save() return True @@ -123,11 +146,24 @@ def remove_user_from_course_group(caller, user, location, role): # see if the user is actually in that role, if not then we don't have to do anything if is_user_in_course_group_role(user, location, role): - groupname = get_course_groupname_for_role(location, role) + _remove_user_from_group(user, get_course_groupname_for_role(location, role)) - group = Group.objects.get(name=groupname) - user.groups.remove(group) - user.save() + +def remove_user_from_creator_group(user): + """ + Removes user from the course creator group. + """ + _remove_user_from_group(user, COURSE_CREATOR_GROUP_NAME) + + +def _remove_user_from_group(user, group_name): + """ + This is to be called only by either a command line code path or through an app which has already + asserted permissions to do this action + """ + group = Group.objects.get(name=group_name) + user.groups.remove(group) + user.save() def is_user_in_course_group_role(user, location, role): @@ -136,3 +172,26 @@ def is_user_in_course_group_role(user, location, role): return user.is_staff or user.groups.filter(name=get_course_groupname_for_role(location, role)).count() > 0 return False + + +def is_user_in_creator_group(user): + """ + Returns true if the user has permissions to create a course. + + Will always return True if user.is_staff is True. + + Note that on the edX site, we currently limit course creators to edX staff. On + other sites, this method checks that the user is in the course creator group. + """ + if user.is_staff: + return True + + # On edx, we only allow edX staff to create courses. This may be relaxed in the future. + if settings.MITX_FEATURES.get('DISABLE_COURSE_CREATION', False): + return False + + # Feature flag for using the creator group setting. Will be removed once the feature is complete. + if settings.MITX_FEATURES.get('ENABLE_CREATOR_GROUP', False): + return user.groups.filter(name=COURSE_CREATOR_GROUP_NAME).count() > 0 + + return True diff --git a/cms/djangoapps/auth/tests/test_authz.py b/cms/djangoapps/auth/tests/test_authz.py new file mode 100644 index 0000000000..4e44471ebf --- /dev/null +++ b/cms/djangoapps/auth/tests/test_authz.py @@ -0,0 +1,78 @@ +""" +Tests authz.py +""" +import mock + +from django.test import TestCase +from django.contrib.auth.models import User + +from auth.authz import add_user_to_creator_group, remove_user_from_creator_group, is_user_in_creator_group + +class CreatorGroupTest(TestCase): + """ + Tests for the course creator group. + """ + def setUp(self): + """ Test case setup """ + self.user = User.objects.create_user('testuser', 'test+courses@edx.org', 'foo') + + def test_creator_group_not_enabled(self): + """ + Tests that is_user_in_creator_group always returns True if ENABLE_CREATOR_GROUP + and DISABLE_COURSE_CREATION are both not turned on. + """ + self.assertTrue(is_user_in_creator_group(self.user)) + + def test_creator_group_enabled_but_empty(self): + """ Tests creator group feature on, but group empty. """ + with mock.patch.dict('django.conf.settings.MITX_FEATURES', {"ENABLE_CREATOR_GROUP" : True}): + self.assertFalse(is_user_in_creator_group(self.user)) + + # Make user staff. This will cause is_user_in_creator_group to return True. + self.user.is_staff = True + self.assertTrue(is_user_in_creator_group(self.user)) + + def test_creator_group_enabled_nonempty(self): + """ Tests creator group feature on, user added. """ + with mock.patch.dict('django.conf.settings.MITX_FEATURES', {"ENABLE_CREATOR_GROUP" : True}): + self.assertTrue(add_user_to_creator_group(self.user)) + self.assertTrue(is_user_in_creator_group(self.user)) + + # check that a user who has not been added to the group still returns false + user_not_added = User.objects.create_user('testuser2', 'test+courses2@edx.org', 'foo2') + self.assertFalse(is_user_in_creator_group(user_not_added)) + + # remove first user from the group and verify that is_user_in_creator_group now returns false + remove_user_from_creator_group(self.user) + self.assertFalse(is_user_in_creator_group(self.user)) + + def test_add_user_not_authenticated(self): + """ + Tests that adding to creator group fails if user is not authenticated + """ + self.user.is_authenticated = False + self.assertFalse(add_user_to_creator_group(self.user)) + + def test_add_user_not_active(self): + """ + Tests that adding to creator group fails if user is not active + """ + self.user.is_active = False + self.assertFalse(add_user_to_creator_group(self.user)) + + def test_course_creation_disabled(self): + """ Tests that the COURSE_CREATION_DISABLED flag overrides course creator group settings. """ + with mock.patch.dict('django.conf.settings.MITX_FEATURES', {'DISABLE_COURSE_CREATION': True, "ENABLE_CREATOR_GROUP" : True}): + # Add user to creator group. + self.assertTrue(add_user_to_creator_group(self.user)) + + # DISABLE_COURSE_CREATION overrides (user is not marked as staff). + self.assertFalse(is_user_in_creator_group(self.user)) + + # Mark as staff. Now is_user_in_creator_group returns true. + self.user.is_staff = True + self.assertTrue(is_user_in_creator_group(self.user)) + + # Remove user from creator group. is_user_in_creator_group still returns true because is_staff=True + remove_user_from_creator_group(self.user) + self.assertTrue(is_user_in_creator_group(self.user)) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 66fead562e..093532c71d 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -1,5 +1,6 @@ import json import shutil +import mock from django.test.client import Client from django.test.utils import override_settings from django.conf import settings @@ -16,6 +17,8 @@ from django.dispatch import Signal from contentstore.utils import get_modulestore from contentstore.tests.utils import parse_json +from auth.authz import add_user_to_creator_group + from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory @@ -860,6 +863,12 @@ class ContentStoreTest(ModuleStoreTestCase): def test_create_course(self): """Test new course creation - happy path""" + self.assert_created_course() + + def assert_created_course(self): + """ + Checks that the course was created properly. + """ resp = self.client.post(reverse('create_new_course'), self.course_data) self.assertEqual(resp.status_code, 200) data = parse_json(resp) @@ -867,41 +876,71 @@ class ContentStoreTest(ModuleStoreTestCase): def test_create_course_check_forum_seeding(self): """Test new course creation and verify forum seeding """ - resp = self.client.post(reverse('create_new_course'), self.course_data) - self.assertEqual(resp.status_code, 200) - data = parse_json(resp) - self.assertEqual(data['id'], 'i4x://MITx/999/course/Robot_Super_Course') + self.assert_created_course() self.assertTrue(are_permissions_roles_seeded('MITx/999/Robot_Super_Course')) def test_create_course_duplicate_course(self): """Test new course creation - error path""" self.client.post(reverse('create_new_course'), self.course_data) + self.assert_course_creation_failed('There is already a course defined with this name.') + + def assert_course_creation_failed(self, error_message): + """ + Checks that the course did not get created + """ resp = self.client.post(reverse('create_new_course'), self.course_data) - data = parse_json(resp) self.assertEqual(resp.status_code, 200) - self.assertEqual(data['ErrMsg'], 'There is already a course defined with this name.') + data = parse_json(resp) + self.assertEqual(data['ErrMsg'], error_message) def test_create_course_duplicate_number(self): """Test new course creation - error path""" self.client.post(reverse('create_new_course'), self.course_data) self.course_data['display_name'] = 'Robot Super Course Two' - resp = self.client.post(reverse('create_new_course'), self.course_data) - data = parse_json(resp) - - self.assertEqual(resp.status_code, 200) - self.assertEqual(data['ErrMsg'], - 'There is already a course defined with the same organization and course number.') + self.assert_course_creation_failed('There is already a course defined with the same organization and course number.') def test_create_course_with_bad_organization(self): """Test new course creation - error path for bad organization name""" self.course_data['org'] = 'University of California, Berkeley' - resp = self.client.post(reverse('create_new_course'), self.course_data) - data = parse_json(resp) + self.assert_course_creation_failed("Unable to create course 'Robot Super Course'.\n\nInvalid characters in 'University of California, Berkeley'.") - self.assertEqual(resp.status_code, 200) - self.assertEqual(data['ErrMsg'], - "Unable to create course 'Robot Super Course'.\n\nInvalid characters in 'University of California, Berkeley'.") + def test_create_course_with_course_creation_disabled_staff(self): + """Test new course creation -- course creation disabled, but staff access.""" + with mock.patch.dict('django.conf.settings.MITX_FEATURES', {'DISABLE_COURSE_CREATION': True}): + self.assert_created_course() + + def test_create_course_with_course_creation_disabled_not_staff(self): + """Test new course creation -- error path for course creation disabled, not staff access.""" + with mock.patch.dict('django.conf.settings.MITX_FEATURES', {'DISABLE_COURSE_CREATION': True}): + self.user.is_staff = False + self.user.save() + self.assert_course_permission_denied() + + def test_create_course_no_course_creators_staff(self): + """Test new course creation -- course creation group enabled, staff, group is empty.""" + with mock.patch.dict('django.conf.settings.MITX_FEATURES', {'ENABLE_CREATOR_GROUP': True}): + self.assert_created_course() + + def test_create_course_no_course_creators_not_staff(self): + """Test new course creation -- error path for course creator group enabled, not staff, group is empty.""" + with mock.patch.dict('django.conf.settings.MITX_FEATURES', {"ENABLE_CREATOR_GROUP" : True}): + self.user.is_staff = False + self.user.save() + self.assert_course_permission_denied() + + def test_create_course_with_course_creator(self): + """Test new course creation -- use course creator group""" + with mock.patch.dict('django.conf.settings.MITX_FEATURES', {"ENABLE_CREATOR_GROUP" : True}): + add_user_to_creator_group(self.user) + self.assert_created_course() + + def assert_course_permission_denied(self): + """ + Checks that the course did not get created due to a PermissionError. + """ + resp = self.client.post(reverse('create_new_course'), self.course_data) + self.assertEqual(resp.status_code, 403) def test_course_index_view_with_no_courses(self): """Test viewing the index page with no courses""" diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index dd7573bad5..8862115c45 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -21,7 +21,7 @@ from contentstore.utils import get_lms_link_for_item, add_extra_panel_tab, remov from models.settings.course_details import CourseDetails, CourseSettingsEncoder from models.settings.course_grading import CourseGradingModel from models.settings.course_metadata import CourseMetadata -from auth.authz import create_all_course_groups +from auth.authz import create_all_course_groups, is_user_in_creator_group from util.json_request import expect_json from .access import has_access, get_location_and_verify_access @@ -81,7 +81,7 @@ def course_index(request, org, course, name): @expect_json def create_new_course(request): - if settings.MITX_FEATURES.get('DISABLE_COURSE_CREATION', False) and not request.user.is_staff: + if not is_user_in_creator_group(request.user): raise PermissionDenied() # This logic is repeated in xmodule/modulestore/tests/factories.py From 2c60a7dbc142b1fbd973eca5931b8f0b00f6b397 Mon Sep 17 00:00:00 2001 From: cahrens Date: Tue, 25 Jun 2013 15:46:08 -0400 Subject: [PATCH 39/55] pep8 cleanup --- cms/djangoapps/auth/tests/test_authz.py | 9 +++++--- .../contentstore/tests/test_contentstore.py | 21 +++++++++---------- 2 files changed, 16 insertions(+), 14 deletions(-) diff --git a/cms/djangoapps/auth/tests/test_authz.py b/cms/djangoapps/auth/tests/test_authz.py index 4e44471ebf..8ecf3689b3 100644 --- a/cms/djangoapps/auth/tests/test_authz.py +++ b/cms/djangoapps/auth/tests/test_authz.py @@ -8,10 +8,12 @@ from django.contrib.auth.models import User from auth.authz import add_user_to_creator_group, remove_user_from_creator_group, is_user_in_creator_group + class CreatorGroupTest(TestCase): """ Tests for the course creator group. """ + def setUp(self): """ Test case setup """ self.user = User.objects.create_user('testuser', 'test+courses@edx.org', 'foo') @@ -25,7 +27,7 @@ class CreatorGroupTest(TestCase): def test_creator_group_enabled_but_empty(self): """ Tests creator group feature on, but group empty. """ - with mock.patch.dict('django.conf.settings.MITX_FEATURES', {"ENABLE_CREATOR_GROUP" : True}): + with mock.patch.dict('django.conf.settings.MITX_FEATURES', {"ENABLE_CREATOR_GROUP": True}): self.assertFalse(is_user_in_creator_group(self.user)) # Make user staff. This will cause is_user_in_creator_group to return True. @@ -34,7 +36,7 @@ class CreatorGroupTest(TestCase): def test_creator_group_enabled_nonempty(self): """ Tests creator group feature on, user added. """ - with mock.patch.dict('django.conf.settings.MITX_FEATURES', {"ENABLE_CREATOR_GROUP" : True}): + with mock.patch.dict('django.conf.settings.MITX_FEATURES', {"ENABLE_CREATOR_GROUP": True}): self.assertTrue(add_user_to_creator_group(self.user)) self.assertTrue(is_user_in_creator_group(self.user)) @@ -62,7 +64,8 @@ class CreatorGroupTest(TestCase): def test_course_creation_disabled(self): """ Tests that the COURSE_CREATION_DISABLED flag overrides course creator group settings. """ - with mock.patch.dict('django.conf.settings.MITX_FEATURES', {'DISABLE_COURSE_CREATION': True, "ENABLE_CREATOR_GROUP" : True}): + with mock.patch.dict('django.conf.settings.MITX_FEATURES', + {'DISABLE_COURSE_CREATION': True, "ENABLE_CREATOR_GROUP": True}): # Add user to creator group. self.assertTrue(add_user_to_creator_group(self.user)) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 093532c71d..ea4b0bbb5b 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -531,7 +531,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): self.assertGreater(len(all_assets), 0) # make sure we have some thumbnails in our trashcan - all_thumbnails = trash_store.get_all_content_thumbnails_for_course(course_location) + _all_thumbnails = trash_store.get_all_content_thumbnails_for_course(course_location) # # cdodge: temporarily comment out assertion on thumbnails because many environments # will not have the jpeg converter installed and this test will fail @@ -592,20 +592,18 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): location = Location('i4x://MITx/999/chapter/neuvo') self.assertRaises(InvalidVersionError, draft_store.clone_item, 'i4x://edx/templates/chapter/Empty', - location) + location) direct_store.clone_item('i4x://edx/templates/chapter/Empty', location) - self.assertRaises(InvalidVersionError, draft_store.clone_item, location, - location) + self.assertRaises(InvalidVersionError, draft_store.clone_item, location, location) - self.assertRaises(InvalidVersionError, draft_store.update_item, location, - 'chapter data') + self.assertRaises(InvalidVersionError, draft_store.update_item, location, 'chapter data') # taking advantage of update_children and other functions never checking that the ids are valid self.assertRaises(InvalidVersionError, draft_store.update_children, location, - ['i4x://MITx/999/problem/doesntexist']) + ['i4x://MITx/999/problem/doesntexist']) self.assertRaises(InvalidVersionError, draft_store.update_metadata, location, - {'due': datetime.datetime.now(UTC)}) + {'due': datetime.datetime.now(UTC)}) self.assertRaises(InvalidVersionError, draft_store.unpublish, location) @@ -903,7 +901,8 @@ class ContentStoreTest(ModuleStoreTestCase): def test_create_course_with_bad_organization(self): """Test new course creation - error path for bad organization name""" self.course_data['org'] = 'University of California, Berkeley' - self.assert_course_creation_failed("Unable to create course 'Robot Super Course'.\n\nInvalid characters in 'University of California, Berkeley'.") + self.assert_course_creation_failed( + "Unable to create course 'Robot Super Course'.\n\nInvalid characters in 'University of California, Berkeley'.") def test_create_course_with_course_creation_disabled_staff(self): """Test new course creation -- course creation disabled, but staff access.""" @@ -924,14 +923,14 @@ class ContentStoreTest(ModuleStoreTestCase): def test_create_course_no_course_creators_not_staff(self): """Test new course creation -- error path for course creator group enabled, not staff, group is empty.""" - with mock.patch.dict('django.conf.settings.MITX_FEATURES', {"ENABLE_CREATOR_GROUP" : True}): + with mock.patch.dict('django.conf.settings.MITX_FEATURES', {"ENABLE_CREATOR_GROUP": True}): self.user.is_staff = False self.user.save() self.assert_course_permission_denied() def test_create_course_with_course_creator(self): """Test new course creation -- use course creator group""" - with mock.patch.dict('django.conf.settings.MITX_FEATURES', {"ENABLE_CREATOR_GROUP" : True}): + with mock.patch.dict('django.conf.settings.MITX_FEATURES', {"ENABLE_CREATOR_GROUP": True}): add_user_to_creator_group(self.user) self.assert_created_course() From 190c07d9540c44ee671a7c50322412079c3c0eff Mon Sep 17 00:00:00 2001 From: cahrens Date: Tue, 25 Jun 2013 16:15:47 -0400 Subject: [PATCH 40/55] Add smoke coverage for add and remove of course group permissions. --- cms/djangoapps/auth/tests/test_authz.py | 62 ++++++++++++++++++++++++- 1 file changed, 61 insertions(+), 1 deletion(-) diff --git a/cms/djangoapps/auth/tests/test_authz.py b/cms/djangoapps/auth/tests/test_authz.py index 8ecf3689b3..61ac682908 100644 --- a/cms/djangoapps/auth/tests/test_authz.py +++ b/cms/djangoapps/auth/tests/test_authz.py @@ -5,8 +5,11 @@ import mock from django.test import TestCase from django.contrib.auth.models import User +from django.core.exceptions import PermissionDenied -from auth.authz import add_user_to_creator_group, remove_user_from_creator_group, is_user_in_creator_group +from auth.authz import add_user_to_creator_group, remove_user_from_creator_group, is_user_in_creator_group,\ + create_all_course_groups, add_user_to_course_group, STAFF_ROLE_NAME, INSTRUCTOR_ROLE_NAME,\ + is_user_in_course_group_role, remove_user_from_course_group class CreatorGroupTest(TestCase): @@ -79,3 +82,60 @@ class CreatorGroupTest(TestCase): # Remove user from creator group. is_user_in_creator_group still returns true because is_staff=True remove_user_from_creator_group(self.user) self.assertTrue(is_user_in_creator_group(self.user)) + + +class CourseGroupTest(TestCase): + """ + Tests for instructor and staff groups for a particular course. + """ + + def setUp(self): + """ Test case setup """ + self.creator = User.objects.create_user('testcreator', 'testcreator+courses@edx.org', 'foo') + self.staff = User.objects.create_user('teststaff', 'teststaff+courses@edx.org', 'foo') + self.location = 'i4x', 'mitX', '101', 'course', 'test' + + def test_add_user_to_course_group(self): + """ + Tests adding user to course group (happy path). + """ + # Create groups for a new course (and assign instructor role to the creator). + self.assertFalse(is_user_in_course_group_role(self.creator, self.location, INSTRUCTOR_ROLE_NAME)) + create_all_course_groups(self.creator, self.location) + self.assertTrue(is_user_in_course_group_role(self.creator, self.location, INSTRUCTOR_ROLE_NAME)) + + # Add another user to the staff role. + self.assertFalse(is_user_in_course_group_role(self.staff, self.location, STAFF_ROLE_NAME)) + self.assertTrue(add_user_to_course_group(self.creator, self.staff, self.location, STAFF_ROLE_NAME)) + self.assertTrue(is_user_in_course_group_role(self.staff, self.location, STAFF_ROLE_NAME)) + + def test_add_user_to_course_group_permission_denied(self): + """ + Verifies PermissionDenied if caller of add_user_to_course_group is not instructor role. + """ + create_all_course_groups(self.creator, self.location) + with self.assertRaises(PermissionDenied): + add_user_to_course_group(self.staff, self.staff, self.location, STAFF_ROLE_NAME) + + def remove_user_from_course_group(self): + """ + Tests removing user from course group (happy path). + """ + create_all_course_groups(self.creator, self.location) + + self.assertTrue(add_user_to_course_group(self.creator, self.staff, self.location, STAFF_ROLE_NAME)) + self.assertTrue(is_user_in_course_group_role(self.staff, self.location, STAFF_ROLE_NAME)) + + remove_user_from_course_group(self.creator, self.location, self.staff, STAFF_ROLE_NAME) + self.assertFalse(is_user_in_course_group_role(self.staff, self.location, STAFF_ROLE_NAME)) + + remove_user_from_course_group(self.creator, self.location, self.creator, INSTRUCTOR_ROLE_NAME) + self.assertFalse(is_user_in_course_group_role(self.creator, self.location, INSTRUCTOR_ROLE_NAME)) + + def test_remove_user_from_course_group_permission_denied(self): + """ + Verifies PermissionDenied if caller of remove_user_from_course_group is not instructor role. + """ + create_all_course_groups(self.creator, self.location) + with self.assertRaises(PermissionDenied): + remove_user_from_course_group(self.staff, self.staff, self.location, STAFF_ROLE_NAME) From 4a697a8da171dad2a1056791607e91ad3e1c6dec Mon Sep 17 00:00:00 2001 From: cahrens Date: Tue, 25 Jun 2013 17:07:51 -0400 Subject: [PATCH 41/55] Verify that caller of add or remove from creator group is staff. --- cms/djangoapps/auth/authz.py | 14 ++++++- cms/djangoapps/auth/tests/test_authz.py | 53 ++++++++++++++++++++----- 2 files changed, 56 insertions(+), 11 deletions(-) diff --git a/cms/djangoapps/auth/authz.py b/cms/djangoapps/auth/authz.py index f27d2fe559..a544906875 100644 --- a/cms/djangoapps/auth/authz.py +++ b/cms/djangoapps/auth/authz.py @@ -102,13 +102,18 @@ def add_user_to_course_group(caller, user, location, role): return _add_user_to_group(user, group) -def add_user_to_creator_group(user): +def add_user_to_creator_group(caller, user): """ Adds the user to the group of course creators. + The caller must have staff access to perform this operation. + Note that on the edX site, we currently limit course creators to edX staff, and this method is a no-op in that environment. """ + if not caller.is_active or not caller.is_authenticated or not caller.is_staff: + raise PermissionDenied + (group, created) = Group.objects.get_or_create(name=COURSE_CREATOR_GROUP_NAME) if created: group.save() @@ -149,10 +154,15 @@ def remove_user_from_course_group(caller, user, location, role): _remove_user_from_group(user, get_course_groupname_for_role(location, role)) -def remove_user_from_creator_group(user): +def remove_user_from_creator_group(caller, user): """ Removes user from the course creator group. + + The caller must have staff access to perform this operation. """ + if not caller.is_active or not caller.is_authenticated or not caller.is_staff: + raise PermissionDenied + _remove_user_from_group(user, COURSE_CREATOR_GROUP_NAME) diff --git a/cms/djangoapps/auth/tests/test_authz.py b/cms/djangoapps/auth/tests/test_authz.py index 61ac682908..173155df4c 100644 --- a/cms/djangoapps/auth/tests/test_authz.py +++ b/cms/djangoapps/auth/tests/test_authz.py @@ -20,6 +20,8 @@ class CreatorGroupTest(TestCase): def setUp(self): """ Test case setup """ self.user = User.objects.create_user('testuser', 'test+courses@edx.org', 'foo') + self.admin = User.objects.create_user('Mark', 'admin+courses@edx.org', 'foo') + self.admin.is_staff = True def test_creator_group_not_enabled(self): """ @@ -40,7 +42,7 @@ class CreatorGroupTest(TestCase): def test_creator_group_enabled_nonempty(self): """ Tests creator group feature on, user added. """ with mock.patch.dict('django.conf.settings.MITX_FEATURES', {"ENABLE_CREATOR_GROUP": True}): - self.assertTrue(add_user_to_creator_group(self.user)) + self.assertTrue(add_user_to_creator_group(self.admin, self.user)) self.assertTrue(is_user_in_creator_group(self.user)) # check that a user who has not been added to the group still returns false @@ -48,7 +50,7 @@ class CreatorGroupTest(TestCase): self.assertFalse(is_user_in_creator_group(user_not_added)) # remove first user from the group and verify that is_user_in_creator_group now returns false - remove_user_from_creator_group(self.user) + remove_user_from_creator_group(self.admin, self.user) self.assertFalse(is_user_in_creator_group(self.user)) def test_add_user_not_authenticated(self): @@ -56,21 +58,21 @@ class CreatorGroupTest(TestCase): Tests that adding to creator group fails if user is not authenticated """ self.user.is_authenticated = False - self.assertFalse(add_user_to_creator_group(self.user)) + self.assertFalse(add_user_to_creator_group(self.admin, self.user)) def test_add_user_not_active(self): """ Tests that adding to creator group fails if user is not active """ self.user.is_active = False - self.assertFalse(add_user_to_creator_group(self.user)) + self.assertFalse(add_user_to_creator_group(self.admin, self.user)) def test_course_creation_disabled(self): """ Tests that the COURSE_CREATION_DISABLED flag overrides course creator group settings. """ with mock.patch.dict('django.conf.settings.MITX_FEATURES', {'DISABLE_COURSE_CREATION': True, "ENABLE_CREATOR_GROUP": True}): # Add user to creator group. - self.assertTrue(add_user_to_creator_group(self.user)) + self.assertTrue(add_user_to_creator_group(self.admin, self.user)) # DISABLE_COURSE_CREATION overrides (user is not marked as staff). self.assertFalse(is_user_in_creator_group(self.user)) @@ -80,9 +82,42 @@ class CreatorGroupTest(TestCase): self.assertTrue(is_user_in_creator_group(self.user)) # Remove user from creator group. is_user_in_creator_group still returns true because is_staff=True - remove_user_from_creator_group(self.user) + remove_user_from_creator_group(self.admin, self.user) self.assertTrue(is_user_in_creator_group(self.user)) + def test_add_user_to_group_requires_staff_access(self): + with self.assertRaises(PermissionDenied): + self.admin.is_staff = False + add_user_to_creator_group(self.admin, self.user) + + with self.assertRaises(PermissionDenied): + add_user_to_creator_group(self.user, self.user) + + def test_add_user_to_group_requires_active(self): + with self.assertRaises(PermissionDenied): + self.admin.is_active = False + add_user_to_creator_group(self.admin, self.user) + + def test_add_user_to_group_requires_authenticated(self): + with self.assertRaises(PermissionDenied): + self.admin.is_authenticated = False + add_user_to_creator_group(self.admin, self.user) + + def test_remove_user_from_group_requires_staff_access(self): + with self.assertRaises(PermissionDenied): + self.admin.is_staff = False + remove_user_from_creator_group(self.admin, self.user) + + def test_remove_user_from_group_requires_active(self): + with self.assertRaises(PermissionDenied): + self.admin.is_active = False + remove_user_from_creator_group(self.admin, self.user) + + def test_remove_user_from_group_requires_authenticated(self): + with self.assertRaises(PermissionDenied): + self.admin.is_authenticated = False + remove_user_from_creator_group(self.admin, self.user) + class CourseGroupTest(TestCase): """ @@ -117,7 +152,7 @@ class CourseGroupTest(TestCase): with self.assertRaises(PermissionDenied): add_user_to_course_group(self.staff, self.staff, self.location, STAFF_ROLE_NAME) - def remove_user_from_course_group(self): + def test_remove_user_from_course_group(self): """ Tests removing user from course group (happy path). """ @@ -126,10 +161,10 @@ class CourseGroupTest(TestCase): self.assertTrue(add_user_to_course_group(self.creator, self.staff, self.location, STAFF_ROLE_NAME)) self.assertTrue(is_user_in_course_group_role(self.staff, self.location, STAFF_ROLE_NAME)) - remove_user_from_course_group(self.creator, self.location, self.staff, STAFF_ROLE_NAME) + remove_user_from_course_group(self.creator, self.staff, self.location, STAFF_ROLE_NAME) self.assertFalse(is_user_in_course_group_role(self.staff, self.location, STAFF_ROLE_NAME)) - remove_user_from_course_group(self.creator, self.location, self.creator, INSTRUCTOR_ROLE_NAME) + remove_user_from_course_group(self.creator, self.creator, self.location, INSTRUCTOR_ROLE_NAME) self.assertFalse(is_user_in_course_group_role(self.creator, self.location, INSTRUCTOR_ROLE_NAME)) def test_remove_user_from_course_group_permission_denied(self): From e487521289f4d89dc8164b377b40969cd8912236 Mon Sep 17 00:00:00 2001 From: cahrens Date: Wed, 26 Jun 2013 09:06:12 -0400 Subject: [PATCH 42/55] Update for change in add_user_to_creator_group API. --- cms/djangoapps/contentstore/tests/test_contentstore.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index ea4b0bbb5b..b946aac6bb 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -931,7 +931,7 @@ class ContentStoreTest(ModuleStoreTestCase): def test_create_course_with_course_creator(self): """Test new course creation -- use course creator group""" with mock.patch.dict('django.conf.settings.MITX_FEATURES', {"ENABLE_CREATOR_GROUP": True}): - add_user_to_creator_group(self.user) + add_user_to_creator_group(self.user, self.user) self.assert_created_course() def assert_course_permission_denied(self): From bb8c62d84ce8949a19c08bd4ab8472628fc692ab Mon Sep 17 00:00:00 2001 From: cahrens Date: Wed, 26 Jun 2013 12:33:55 -0400 Subject: [PATCH 43/55] Make the problem handle empty fields and non-integers correctly. --- .../templates/problem/customgrader.yaml | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/common/lib/xmodule/xmodule/templates/problem/customgrader.yaml b/common/lib/xmodule/xmodule/templates/problem/customgrader.yaml index b5b0d71f4d..48feef481b 100644 --- a/common/lib/xmodule/xmodule/templates/problem/customgrader.yaml +++ b/common/lib/xmodule/xmodule/templates/problem/customgrader.yaml @@ -13,15 +13,16 @@ data: | @@ -40,7 +41,7 @@ data: |

Explanation

-

Any set of values on the line \(y = 10 - x\) and \(y = 20 - x\) satisfy these constraints.

+

Any set of integers on the line \(y = 10 - x\) and \(y = 20 - x\) satisfy these constraints.

From 50f837d9d80498b1704c7cdf910c452f1008660a Mon Sep 17 00:00:00 2001 From: Giulio Gratta Date: Wed, 26 Jun 2013 10:22:59 -0700 Subject: [PATCH 44/55] move marketing iframe breakout script to inside existing conditional --- lms/templates/main.html | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/lms/templates/main.html b/lms/templates/main.html index b00446d190..5c0c383b84 100644 --- a/lms/templates/main.html +++ b/lms/templates/main.html @@ -21,17 +21,17 @@ Home | class.stanford.edu % else: edX + + % endif - From 391ed8c96475b45ff7d8f86313241692ed9c543d Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Tue, 25 Jun 2013 11:56:55 -0400 Subject: [PATCH 45/55] Add docstring to CourseDescriptor.__init__ --- common/lib/xmodule/xmodule/course_module.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/common/lib/xmodule/xmodule/course_module.py b/common/lib/xmodule/xmodule/course_module.py index 62ebe12a03..02b44bd018 100644 --- a/common/lib/xmodule/xmodule/course_module.py +++ b/common/lib/xmodule/xmodule/course_module.py @@ -212,6 +212,9 @@ class CourseDescriptor(CourseFields, SequenceDescriptor): template_dir_name = 'course' def __init__(self, *args, **kwargs): + """ + Expects the same arguments as XModuleDescriptor.__init__ + """ super(CourseDescriptor, self).__init__(*args, **kwargs) if self.wiki_slug is None: From ff6ba014ce8bc51ef110cdf135608b74e4a9d70b Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Mon, 24 Jun 2013 14:48:45 -0400 Subject: [PATCH 46/55] Remove noop if statement --- cms/djangoapps/contentstore/module_info_model.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/cms/djangoapps/contentstore/module_info_model.py b/cms/djangoapps/contentstore/module_info_model.py index e361c97875..726d4bb0ce 100644 --- a/cms/djangoapps/contentstore/module_info_model.py +++ b/cms/djangoapps/contentstore/module_info_model.py @@ -5,10 +5,7 @@ from xmodule.modulestore import Location def get_module_info(store, location, parent_location=None, rewrite_static_links=False): try: - if location.revision is None: - module = store.get_item(location) - else: - module = store.get_item(location) + module = store.get_item(location) except ItemNotFoundError: # create a new one template_location = Location(['i4x', 'edx', 'templates', location.category, 'Empty']) From 65b3bcdba689f7f0212d0f38d06948f409344471 Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Tue, 25 Jun 2013 11:21:17 -0400 Subject: [PATCH 47/55] Clean up variable naming --- cms/djangoapps/contentstore/views/assets.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/cms/djangoapps/contentstore/views/assets.py b/cms/djangoapps/contentstore/views/assets.py index 400013b59b..41077abd8f 100644 --- a/cms/djangoapps/contentstore/views/assets.py +++ b/cms/djangoapps/contentstore/views/assets.py @@ -240,13 +240,13 @@ def import_course(request, org, course, name): # find the 'course.xml' file for dirpath, _dirnames, filenames in os.walk(course_dir): - for files in filenames: - if files == 'course.xml': + for filename in filenames: + if filename == 'course.xml': break - if files == 'course.xml': + if filename == 'course.xml': break - if files != 'course.xml': + if filename != 'course.xml': return HttpResponse(json.dumps({'ErrMsg': 'Could not find the course.xml file in the package.'})) logging.debug('found course.xml at {0}'.format(dirpath)) From 1344b1e521afd83494f99930260c00f679e883d1 Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Tue, 25 Jun 2013 11:23:07 -0400 Subject: [PATCH 48/55] Make SessionKeyValueStore variable names clearer --- .../contentstore/views/session_kv_store.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/cms/djangoapps/contentstore/views/session_kv_store.py b/cms/djangoapps/contentstore/views/session_kv_store.py index 309518c27d..54ab25ff54 100644 --- a/cms/djangoapps/contentstore/views/session_kv_store.py +++ b/cms/djangoapps/contentstore/views/session_kv_store.py @@ -2,27 +2,27 @@ from xblock.runtime import KeyValueStore, InvalidScopeError class SessionKeyValueStore(KeyValueStore): - def __init__(self, request, model_data): - self._model_data = model_data + def __init__(self, request, descriptor_model_data): + self._descriptor_model_data = descriptor_model_data self._session = request.session def get(self, key): try: - return self._model_data[key.field_name] + return self._descriptor_model_data[key.field_name] except (KeyError, InvalidScopeError): return self._session[tuple(key)] def set(self, key, value): try: - self._model_data[key.field_name] = value + self._descriptor_model_data[key.field_name] = value except (KeyError, InvalidScopeError): self._session[tuple(key)] = value def delete(self, key): try: - del self._model_data[key.field_name] + del self._descriptor_model_data[key.field_name] except (KeyError, InvalidScopeError): del self._session[tuple(key)] def has(self, key): - return key in self._model_data or key in self._session + return key in self._descriptor_model_data or key in self._session From b985a7f128851171dde1a3f0927701efa2bb404d Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Tue, 25 Jun 2013 11:26:02 -0400 Subject: [PATCH 49/55] Remove unused template --- cms/templates/new_item.html | 19 ------------------- 1 file changed, 19 deletions(-) delete mode 100644 cms/templates/new_item.html diff --git a/cms/templates/new_item.html b/cms/templates/new_item.html deleted file mode 100644 index 45cb157845..0000000000 --- a/cms/templates/new_item.html +++ /dev/null @@ -1,19 +0,0 @@ -
-
${parent_name}
-
${parent_location}
- -
- % for module_type, module_templates in templates: -
-
${module_type}
-
- % for template in module_templates: - ${template.display_name_with_default} - % endfor -
-
- % endfor -
- Cancel -
- From f7d6d149461bdad797082cb58f667d8260da8f29 Mon Sep 17 00:00:00 2001 From: Sarina Canelake Date: Tue, 25 Jun 2013 09:42:28 -0400 Subject: [PATCH 50/55] Catch InvalidLocationError --- lms/djangoapps/courseware/courses.py | 6 +++--- lms/djangoapps/courseware/tests/test_courses.py | 16 ++++++++++++++++ 2 files changed, 19 insertions(+), 3 deletions(-) create mode 100644 lms/djangoapps/courseware/tests/test_courses.py diff --git a/lms/djangoapps/courseware/courses.py b/lms/djangoapps/courseware/courses.py index 71c9630964..ef1b786645 100644 --- a/lms/djangoapps/courseware/courses.py +++ b/lms/djangoapps/courseware/courses.py @@ -12,12 +12,11 @@ from xmodule.modulestore import Location from xmodule.modulestore.django import modulestore from xmodule.contentstore.content import StaticContent from xmodule.modulestore.xml import XMLModuleStore -from xmodule.modulestore.exceptions import ItemNotFoundError +from xmodule.modulestore.exceptions import ItemNotFoundError, InvalidLocationError from courseware.model_data import ModelDataCache from static_replace import replace_static_urls from courseware.access import has_access import branding -from xmodule.modulestore.exceptions import ItemNotFoundError log = logging.getLogger(__name__) @@ -49,7 +48,8 @@ def get_course_by_id(course_id, depth=0): return modulestore().get_instance(course_id, course_loc, depth=depth) except (KeyError, ItemNotFoundError): raise Http404("Course not found.") - + except InvalidLocationError: + raise Http404("Invalid location") def get_course_with_access(user, course_id, action, depth=0): """ diff --git a/lms/djangoapps/courseware/tests/test_courses.py b/lms/djangoapps/courseware/tests/test_courses.py new file mode 100644 index 0000000000..60594602a4 --- /dev/null +++ b/lms/djangoapps/courseware/tests/test_courses.py @@ -0,0 +1,16 @@ +# -*- coding: utf-8 -*- +from django.test import TestCase +from django.http import Http404 +from courseware.courses import get_course_by_id + +class CoursesTest(TestCase): + def test_get_course_by_id_invalid_chars(self): + """ + Test that `get_course_by_id` throws a 404, rather than + an exception, when faced with unexpected characters + (such as unicode characters, and symbols such as = and ' ') + """ + with self.assertRaises(Http404): + get_course_by_id('MITx/foobar/statistics=introduction') + get_course_by_id('MITx/foobar/business and management') + get_course_by_id('MITx/foobar/NiñøJoséMaríáßç') From dec20d76bc33cb185640c424d39f876f724e5e2f Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 26 Jun 2013 16:25:44 -0400 Subject: [PATCH 51/55] Use separate venv for parallel builds --- jenkins/test.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/jenkins/test.sh b/jenkins/test.sh index 05f978e32f..70a9e168bc 100755 --- a/jenkins/test.sh +++ b/jenkins/test.sh @@ -60,7 +60,7 @@ fi export PIP_DOWNLOAD_CACHE=/mnt/pip-cache -source /mnt/virtualenvs/"$JOB_NAME"/bin/activate +source $VIRTUALENV_DIR/bin/activate bundle install From ee5389800ad8300a21c90c78976aa61eebadbba8 Mon Sep 17 00:00:00 2001 From: Peter Fogg Date: Wed, 26 Jun 2013 16:24:04 -0400 Subject: [PATCH 52/55] Fix Lyla showing up everywhere. Previously XML data wasn't parsed in VideoDescriptor.__init__, leading to the defaults being used for video settings. --- .../xmodule/tests/test_video_module.py | 28 ++++++ common/lib/xmodule/xmodule/video_module.py | 90 +++++++++++-------- 2 files changed, 80 insertions(+), 38 deletions(-) diff --git a/common/lib/xmodule/xmodule/tests/test_video_module.py b/common/lib/xmodule/xmodule/tests/test_video_module.py index f516e1a179..e11686176a 100644 --- a/common/lib/xmodule/xmodule/tests/test_video_module.py +++ b/common/lib/xmodule/xmodule/tests/test_video_module.py @@ -1,6 +1,7 @@ # -*- coding: utf-8 -*- import unittest +from xmodule.modulestore import Location from xmodule.video_module import VideoDescriptor from .test_import import DummySystem @@ -10,6 +11,33 @@ class VideoDescriptorImportTestCase(unittest.TestCase): Make sure that VideoDescriptor can import an old XML-based video correctly. """ + def test_constructor(self): + sample_xml = ''' + + ''' + location = Location(["i4x", "edX", "video", "default", + "SampleProblem1"]) + model_data = {'data': sample_xml, + 'location': location} + system = DummySystem(load_error_modules=True) + descriptor = VideoDescriptor(system, model_data) + self.assertEquals(descriptor.youtube_id_0_75, 'izygArpw-Qo') + self.assertEquals(descriptor.youtube_id_1_0, 'p2Q6BrNhdh8') + self.assertEquals(descriptor.youtube_id_1_25, '1EeWXzPdhSA') + self.assertEquals(descriptor.youtube_id_1_5, 'rABDYkeK0x8') + self.assertEquals(descriptor.show_captions, False) + self.assertEquals(descriptor.start_time, 1.0) + self.assertEquals(descriptor.end_time, 60) + self.assertEquals(descriptor.track, 'http://www.example.com/track') + self.assertEquals(descriptor.source, 'http://www.example.com/source.mp4') + def test_from_xml(self): module_system = DummySystem(load_error_modules=True) xml_data = ''' diff --git a/common/lib/xmodule/xmodule/video_module.py b/common/lib/xmodule/xmodule/video_module.py index 04daaea3f2..3c6203107d 100644 --- a/common/lib/xmodule/xmodule/video_module.py +++ b/common/lib/xmodule/xmodule/video_module.py @@ -88,6 +88,13 @@ class VideoDescriptor(VideoFields, module_class = VideoModule template_dir_name = "video" + def __init__(self, *args, **kwargs): + super(VideoDescriptor, self).__init__(*args, **kwargs) + # If we don't have a `youtube_id_1_0`, this is an XML course + # and we parse out the fields. + if self.data and 'youtube_id_1_0' not in self._model_data: + _parse_video_xml(self, self.data) + @property def non_editable_metadata_fields(self): non_editable_fields = super(MetadataOnlyEditingDescriptor, self).non_editable_metadata_fields @@ -108,47 +115,54 @@ class VideoDescriptor(VideoFields, url identifiers """ video = super(VideoDescriptor, cls).from_xml(xml_data, system, org, course) - xml = etree.fromstring(xml_data) - - display_name = xml.get('display_name') - if display_name: - video.display_name = display_name - - youtube = xml.get('youtube') - if youtube: - speeds = _parse_youtube(youtube) - if speeds['0.75']: - video.youtube_id_0_75 = speeds['0.75'] - if speeds['1.00']: - video.youtube_id_1_0 = speeds['1.00'] - if speeds['1.25']: - video.youtube_id_1_25 = speeds['1.25'] - if speeds['1.50']: - video.youtube_id_1_5 = speeds['1.50'] - - show_captions = xml.get('show_captions') - if show_captions: - video.show_captions = json.loads(show_captions) - - source = _get_first_external(xml, 'source') - if source: - video.source = source - - track = _get_first_external(xml, 'track') - if track: - video.track = track - - start_time = _parse_time(xml.get('from')) - if start_time: - video.start_time = start_time - - end_time = _parse_time(xml.get('to')) - if end_time: - video.end_time = end_time - + _parse_video_xml(video, xml_data) return video +def _parse_video_xml(video, xml_data): + """ + Parse video fields out of xml_data. The fields are set if they are + present in the XML. + """ + xml = etree.fromstring(xml_data) + + display_name = xml.get('display_name') + if display_name: + video.display_name = display_name + + youtube = xml.get('youtube') + if youtube: + speeds = _parse_youtube(youtube) + if speeds['0.75']: + video.youtube_id_0_75 = speeds['0.75'] + if speeds['1.00']: + video.youtube_id_1_0 = speeds['1.00'] + if speeds['1.25']: + video.youtube_id_1_25 = speeds['1.25'] + if speeds['1.50']: + video.youtube_id_1_5 = speeds['1.50'] + + show_captions = xml.get('show_captions') + if show_captions: + video.show_captions = json.loads(show_captions) + + source = _get_first_external(xml, 'source') + if source: + video.source = source + + track = _get_first_external(xml, 'track') + if track: + video.track = track + + start_time = _parse_time(xml.get('from')) + if start_time: + video.start_time = start_time + + end_time = _parse_time(xml.get('to')) + if end_time: + video.end_time = end_time + + def _get_first_external(xmltree, tag): """ Returns the src attribute of the nested `tag` in `xmltree`, if it From 2a49d087de272c6724a2f0d1b9aa775d0653784a Mon Sep 17 00:00:00 2001 From: Brian Talbot Date: Thu, 20 Jun 2013 11:21:43 -0400 Subject: [PATCH 53/55] studio - revises transparent Sass color vars to use rgba() method --- cms/static/sass/_variables.scss | 34 ++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/cms/static/sass/_variables.scss b/cms/static/sass/_variables.scss index 14c215c7fd..bad87952d6 100644 --- a/cms/static/sass/_variables.scss +++ b/cms/static/sass/_variables.scss @@ -24,16 +24,16 @@ $f-monospace: 'Bitstream Vera Sans Mono', Consolas, Courier, monospace; // colors - new for re-org $black: rgb(0,0,0); -$black-t0: rgba(0,0,0,0.125); -$black-t1: rgba(0,0,0,0.25); -$black-t2: rgba(0,0,0,0.50); -$black-t3: rgba(0,0,0,0.75); +$black-t0: rgba($black, 0.125); +$black-t1: rgba($black, 0.25); +$black-t2: rgba($black, 0.5); +$black-t3: rgba($black, 0.75); $white: rgb(255,255,255); -$white-t0: rgba(255,255,255,0.125); -$white-t1: rgba(255,255,255,0.25); -$white-t2: rgba(255,255,255,0.50); -$white-t3: rgba(255,255,255,0.75); +$white-t0: rgba($white, 0.125); +$white-t1: rgba($white, 0.25); +$white-t2: rgba($white, 0.5); +$white-t3: rgba($white, 0.75); $gray: rgb(127,127,127); $gray-l1: tint($gray,20%); @@ -63,10 +63,10 @@ $blue-s3: saturate($blue,45%); $blue-u1: desaturate($blue,15%); $blue-u2: desaturate($blue,30%); $blue-u3: desaturate($blue,45%); -$blue-t0: rgba(85, 151, 221,0.125); -$blue-t1: rgba(85, 151, 221,0.25); -$blue-t2: rgba(85, 151, 221,0.50); -$blue-t3: rgba(85, 151, 221,0.75); +$blue-t0: rgba($blue, 0.125); +$blue-t1: rgba($blue, 0.25); +$blue-t2: rgba($blue, 0.50); +$blue-t3: rgba($blue, 0.75); $pink: rgb(183, 37, 103); $pink-l1: tint($pink,20%); @@ -153,10 +153,11 @@ $orange-u1: desaturate($orange,15%); $orange-u2: desaturate($orange,30%); $orange-u3: desaturate($orange,45%); -$shadow: rgba(0,0,0,0.2); -$shadow-l1: rgba(0,0,0,0.1); -$shadow-l2: rgba(0,0,0,0.05); -$shadow-d1: rgba(0,0,0,0.4); +$shadow: rgba($black, 0.2); +$shadow-l1: rgba($black, 0.1); +$shadow-l2: rgba($black, 0.05); +$shadow-d1: rgba($black, 0.4); +$shadow-d2: rgba($black, 0.6); // ==================== @@ -186,4 +187,3 @@ $error-red: rgb(253, 87, 87); // type $sans-serif: $f-sans-serif; $body-line-height: golden-ratio(.875em, 1); - From 306ac482102970a3946e1e3736a6f5f01bb4143f Mon Sep 17 00:00:00 2001 From: dcadams Date: Wed, 26 Jun 2013 14:01:07 -0700 Subject: [PATCH 54/55] Email on enroll/un-enroll actions Optionally email students on enroll/un-enroll actions by instructor from enrollment tab in LMS. --- CHANGELOG.rst | 2 + common/djangoapps/terrain/factories.py | 2 +- .../instructor/tests/test_enrollment.py | 249 ++++++++++++------ lms/djangoapps/instructor/views.py | 137 +++++++++- .../courseware/instructor_dashboard.html | 2 + .../emails/enroll_email_allowedmessage.txt | 13 + .../emails/enroll_email_allowedsubject.txt | 1 + .../emails/enroll_email_enrolledmessage.txt | 8 + .../emails/enroll_email_enrolledsubject.txt | 1 + .../emails/unenroll_email_allowedmessage.txt | 6 + .../emails/unenroll_email_enrolledmessage.txt | 8 + .../emails/unenroll_email_subject.txt | 1 + 12 files changed, 330 insertions(+), 100 deletions(-) create mode 100644 lms/templates/emails/enroll_email_allowedmessage.txt create mode 100644 lms/templates/emails/enroll_email_allowedsubject.txt create mode 100644 lms/templates/emails/enroll_email_enrolledmessage.txt create mode 100644 lms/templates/emails/enroll_email_enrolledsubject.txt create mode 100644 lms/templates/emails/unenroll_email_allowedmessage.txt create mode 100644 lms/templates/emails/unenroll_email_enrolledmessage.txt create mode 100644 lms/templates/emails/unenroll_email_subject.txt diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 4fea30a5c5..8481f3a707 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -152,3 +152,5 @@ Common: Updated CodeJail. Common: Allow setting of authentication session cookie name. +LMS: Option to email students when enroll/un-enroll them. + diff --git a/common/djangoapps/terrain/factories.py b/common/djangoapps/terrain/factories.py index decce42368..2ed78aaa9f 100644 --- a/common/djangoapps/terrain/factories.py +++ b/common/djangoapps/terrain/factories.py @@ -44,7 +44,7 @@ class GroupFactory(sf.GroupFactory): @world.absorb -class CourseEnrollmentAllowedFactory(sf.CourseEnrollmentAllowed): +class CourseEnrollmentAllowedFactory(sf.CourseEnrollmentAllowedFactory): """ Users allowed to enroll in the course outside of the usual window """ diff --git a/lms/djangoapps/instructor/tests/test_enrollment.py b/lms/djangoapps/instructor/tests/test_enrollment.py index 3ce82b700b..3b5bdc2ce9 100644 --- a/lms/djangoapps/instructor/tests/test_enrollment.py +++ b/lms/djangoapps/instructor/tests/test_enrollment.py @@ -1,177 +1,256 @@ -''' +""" Unit tests for enrollment methods in views.py -''' +""" from django.test.utils import override_settings -from django.contrib.auth.models import Group, User +from django.contrib.auth.models import User from django.core.urlresolvers import reverse from courseware.access import _course_staff_group_name from courseware.tests.tests import LoginEnrollmentTestCase, TEST_DATA_XML_MODULESTORE, get_user from xmodule.modulestore.django import modulestore +from xmodule.modulestore.tests.factories import CourseFactory +from student.tests.factories import UserFactory, CourseEnrollmentFactory, AdminFactory +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase +from courseware.tests.tests import TEST_DATA_MONGO_MODULESTORE, LoginEnrollmentTestCase from student.models import CourseEnrollment, CourseEnrollmentAllowed -from instructor.views import get_and_clean_student_list +from instructor.views import get_and_clean_student_list, send_mail_to_student +from django.core import mail + +USER_COUNT = 4 -@override_settings(MODULESTORE=TEST_DATA_XML_MODULESTORE) -class TestInstructorEnrollsStudent(LoginEnrollmentTestCase): - ''' - Check Enrollment/Unenrollment with/without auto-enrollment on activation - ''' +@override_settings(MODULESTORE=TEST_DATA_MONGO_MODULESTORE) +class TestInstructorEnrollsStudent(ModuleStoreTestCase, LoginEnrollmentTestCase): + """ + Check Enrollment/Unenrollment with/without auto-enrollment on activation and with/without email notification + """ def setUp(self): - self.full = modulestore().get_course("edX/full/6.002_Spring_2012") - self.toy = modulestore().get_course("edX/toy/2012_Fall") + instructor = AdminFactory.create() + self.client.login(username=instructor.username, password='test') - #Create instructor and student accounts - self.instructor = 'instructor1@test.com' - self.student1 = 'student1@test.com' - self.student2 = 'student2@test.com' - self.password = 'foo' - self.create_account('it1', self.instructor, self.password) - self.create_account('st1', self.student1, self.password) - self.create_account('st2', self.student2, self.password) - self.activate_user(self.instructor) - self.activate_user(self.student1) - self.activate_user(self.student2) + self.course = CourseFactory.create() - def make_instructor(course): - group_name = _course_staff_group_name(course.location) - g = Group.objects.create(name=group_name) - g.user_set.add(get_user(self.instructor)) + self.users = [ + UserFactory.create(username="student%d" % i, email="student%d@test.com" % i) + for i in xrange(USER_COUNT) + ] - make_instructor(self.toy) + for user in self.users: + CourseEnrollmentFactory.create(user=user, course_id=self.course.id) - #Enroll Students - self.logout() - self.login(self.student1, self.password) - self.enroll(self.toy) + # Empty the test outbox + mail.outbox = [] - self.logout() - self.login(self.student2, self.password) - self.enroll(self.toy) + def test_unenrollment_email_off(self): + """ + Do un-enrollment email off test + """ - #Enroll Instructor - self.logout() - self.login(self.instructor, self.password) - self.enroll(self.toy) + course = self.course - def test_unenrollment(self): - ''' - Do un-enrollment test - ''' - - course = self.toy + #Run the Un-enroll students command url = reverse('instructor_dashboard', kwargs={'course_id': course.id}) - response = self.client.post(url, {'action': 'Unenroll multiple students', 'multiple_students': 'student1@test.com, student2@test.com'}) + response = self.client.post(url, {'action': 'Unenroll multiple students', 'multiple_students': 'student0@test.com student1@test.com'}) - #Check the page output + #Check the page output + self.assertContains(response, 'student0@test.com') self.assertContains(response, 'student1@test.com') - self.assertContains(response, 'student2@test.com') self.assertContains(response, 'un-enrolled') #Check the enrollment table + user = User.objects.get(email='student0@test.com') + ce = CourseEnrollment.objects.filter(course_id=course.id, user=user) + self.assertEqual(0, len(ce)) + user = User.objects.get(email='student1@test.com') ce = CourseEnrollment.objects.filter(course_id=course.id, user=user) self.assertEqual(0, len(ce)) - user = User.objects.get(email='student2@test.com') - ce = CourseEnrollment.objects.filter(course_id=course.id, user=user) - self.assertEqual(0, len(ce)) + #Check the outbox + self.assertEqual(len(mail.outbox), 0) - def test_enrollment_new_student_autoenroll_on(self): - ''' - Do auto-enroll on test - ''' + def test_enrollment_new_student_autoenroll_on_email_off(self): + """ + Do auto-enroll on, email off test + """ + + course = self.course #Run the Enroll students command - course = self.toy url = reverse('instructor_dashboard', kwargs={'course_id': course.id}) - response = self.client.post(url, {'action': 'Enroll multiple students', 'multiple_students': 'test1_1@student.com, test1_2@student.com', 'auto_enroll': 'on'}) + response = self.client.post(url, {'action': 'Enroll multiple students', 'multiple_students': 'student1_1@test.com, student1_2@test.com', 'auto_enroll': 'on'}) #Check the page output - self.assertContains(response, 'test1_1@student.com') - self.assertContains(response, 'test1_2@student.com') + self.assertContains(response, 'student1_1@test.com') + self.assertContains(response, 'student1_2@test.com') self.assertContains(response, 'user does not exist, enrollment allowed, pending with auto enrollment on') + #Check the outbox + self.assertEqual(len(mail.outbox), 0) + #Check the enrollmentallowed db entries - cea = CourseEnrollmentAllowed.objects.filter(email='test1_1@student.com', course_id=course.id) + cea = CourseEnrollmentAllowed.objects.filter(email='student1_1@test.com', course_id=course.id) self.assertEqual(1, cea[0].auto_enroll) - cea = CourseEnrollmentAllowed.objects.filter(email='test1_2@student.com', course_id=course.id) + cea = CourseEnrollmentAllowed.objects.filter(email='student1_2@test.com', course_id=course.id) self.assertEqual(1, cea[0].auto_enroll) - #Check there is no enrollment db entry other than for the setup instructor and students + #Check there is no enrollment db entry other than for the other students ce = CourseEnrollment.objects.filter(course_id=course.id) - self.assertEqual(3, len(ce)) + self.assertEqual(4, len(ce)) #Create and activate student accounts with same email - self.student1 = 'test1_1@student.com' + self.student1 = 'student1_1@test.com' self.password = 'bar' self.create_account('s1_1', self.student1, self.password) self.activate_user(self.student1) - self.student2 = 'test1_2@student.com' + self.student2 = 'student1_2@test.com' self.create_account('s1_2', self.student2, self.password) self.activate_user(self.student2) #Check students are enrolled - user = User.objects.get(email='test1_1@student.com') + user = User.objects.get(email='student1_1@test.com') ce = CourseEnrollment.objects.filter(course_id=course.id, user=user) self.assertEqual(1, len(ce)) - user = User.objects.get(email='test1_2@student.com') + user = User.objects.get(email='student1_2@test.com') ce = CourseEnrollment.objects.filter(course_id=course.id, user=user) self.assertEqual(1, len(ce)) - def test_enrollmemt_new_student_autoenroll_off(self): - ''' - Do auto-enroll off test - ''' + def test_repeat_enroll(self): + """ + Try to enroll an already enrolled student + """ + + course = self.course + + url = reverse('instructor_dashboard', kwargs={'course_id': course.id}) + response = self.client.post(url, {'action': 'Enroll multiple students', 'multiple_students': 'student0@test.com', 'auto_enroll': 'on'}) + self.assertContains(response, 'student0@test.com') + self.assertContains(response, 'already enrolled') + + def test_enrollmemt_new_student_autoenroll_off_email_off(self): + """ + Do auto-enroll off, email off test + """ + + course = self.course #Run the Enroll students command - course = self.toy url = reverse('instructor_dashboard', kwargs={'course_id': course.id}) - response = self.client.post(url, {'action': 'Enroll multiple students', 'multiple_students': 'test2_1@student.com, test2_2@student.com'}) + response = self.client.post(url, {'action': 'Enroll multiple students', 'multiple_students': 'student2_1@test.com, student2_2@test.com'}) #Check the page output - self.assertContains(response, 'test2_1@student.com') - self.assertContains(response, 'test2_2@student.com') + self.assertContains(response, 'student2_1@test.com') + self.assertContains(response, 'student2_2@test.com') self.assertContains(response, 'user does not exist, enrollment allowed, pending with auto enrollment off') + #Check the outbox + self.assertEqual(len(mail.outbox), 0) + #Check the enrollmentallowed db entries - cea = CourseEnrollmentAllowed.objects.filter(email='test2_1@student.com', course_id=course.id) + cea = CourseEnrollmentAllowed.objects.filter(email='student2_1@test.com', course_id=course.id) self.assertEqual(0, cea[0].auto_enroll) - cea = CourseEnrollmentAllowed.objects.filter(email='test2_2@student.com', course_id=course.id) + cea = CourseEnrollmentAllowed.objects.filter(email='student2_2@test.com', course_id=course.id) self.assertEqual(0, cea[0].auto_enroll) #Check there is no enrollment db entry other than for the setup instructor and students ce = CourseEnrollment.objects.filter(course_id=course.id) - self.assertEqual(3, len(ce)) + self.assertEqual(4, len(ce)) #Create and activate student accounts with same email - self.student = 'test2_1@student.com' + self.student = 'student2_1@test.com' self.password = 'bar' self.create_account('s2_1', self.student, self.password) self.activate_user(self.student) - self.student = 'test2_2@student.com' + self.student = 'student2_2@test.com' self.create_account('s2_2', self.student, self.password) self.activate_user(self.student) #Check students are not enrolled - user = User.objects.get(email='test2_1@student.com') + user = User.objects.get(email='student2_1@test.com') ce = CourseEnrollment.objects.filter(course_id=course.id, user=user) self.assertEqual(0, len(ce)) - user = User.objects.get(email='test2_2@student.com') + user = User.objects.get(email='student2_2@test.com') ce = CourseEnrollment.objects.filter(course_id=course.id, user=user) self.assertEqual(0, len(ce)) def test_get_and_clean_student_list(self): - ''' + """ Clean user input test - ''' + """ - string = "abc@test.com, def@test.com ghi@test.com \n \n jkl@test.com " + string = "abc@test.com, def@test.com ghi@test.com \n \n jkl@test.com \n mno@test.com " cleaned_string, cleaned_string_lc = get_and_clean_student_list(string) - self.assertEqual(cleaned_string, ['abc@test.com', 'def@test.com', 'ghi@test.com', 'jkl@test.com']) + self.assertEqual(cleaned_string, ['abc@test.com', 'def@test.com', 'ghi@test.com', 'jkl@test.com', 'mno@test.com']) + + def test_enrollment_email_on(self): + """ + Do email on enroll test + """ + + course = self.course + + #Create activated, but not enrolled, user + UserFactory.create(username="student3_0", email="student3_0@test.com") + + url = reverse('instructor_dashboard', kwargs={'course_id': course.id}) + response = self.client.post(url, {'action': 'Enroll multiple students', 'multiple_students': 'student3_0@test.com, student3_1@test.com, student3_2@test.com', 'auto_enroll': 'on', 'email_students': 'on'}) + + #Check the page output + self.assertContains(response, 'student3_0@test.com') + self.assertContains(response, 'student3_1@test.com') + self.assertContains(response, 'student3_2@test.com') + self.assertContains(response, 'added, email sent') + self.assertContains(response, 'user does not exist, enrollment allowed, pending with auto enrollment on, email sent') + + #Check the outbox + self.assertEqual(len(mail.outbox), 3) + self.assertEqual(mail.outbox[0].subject, 'You have been enrolled in MITx/999/Robot_Super_Course') + + self.assertEqual(mail.outbox[1].subject, 'You have been invited to register for MITx/999/Robot_Super_Course') + self.assertEqual(mail.outbox[1].body, "Dear student,\n\nYou have been invited to join MITx/999/Robot_Super_Course at edx.org by a member of the course staff.\n\n" + + "To finish your registration, please visit https://edx.org/register and fill out the registration form.\n" + + "Once you have registered and activated your account, you will see MITx/999/Robot_Super_Course listed on your dashboard.\n\n" + + "----\nThis email was automatically sent from edx.org to student3_1@test.com") + + def test_unenrollment_email_on(self): + """ + Do email on unenroll test + """ + + course = self.course + + #Create invited, but not registered, user + cea = CourseEnrollmentAllowed(email='student4_0@test.com', course_id=course.id) + cea.save() + + url = reverse('instructor_dashboard', kwargs={'course_id': course.id}) + response = self.client.post(url, {'action': 'Unenroll multiple students', 'multiple_students': 'student4_0@test.com, student2@test.com, student3@test.com', 'email_students': 'on'}) + + #Check the page output + self.assertContains(response, 'student2@test.com') + self.assertContains(response, 'student3@test.com') + self.assertContains(response, 'un-enrolled, email sent') + + #Check the outbox + self.assertEqual(len(mail.outbox), 3) + self.assertEqual(mail.outbox[0].subject, 'You have been un-enrolled from MITx/999/Robot_Super_Course') + self.assertEqual(mail.outbox[0].body, "Dear Student,\n\nYou have been un-enrolled from course MITx/999/Robot_Super_Course by a member of the course staff. " + + "Please disregard the invitation previously sent.\n\n" + + "----\nThis email was automatically sent from edx.org to student4_0@test.com") + self.assertEqual(mail.outbox[1].subject, 'You have been un-enrolled from MITx/999/Robot_Super_Course') + + def test_send_mail_to_student(self): + """ + Do invalid mail template test + """ + + d = {'message': 'message_type_that_doesn\'t_exist'} + + send_mail_ret = send_mail_to_student('student0@test.com', d) + self.assertFalse(send_mail_ret) diff --git a/lms/djangoapps/instructor/views.py b/lms/djangoapps/instructor/views.py index e9fff63698..ea96901bca 100644 --- a/lms/djangoapps/instructor/views.py +++ b/lms/djangoapps/instructor/views.py @@ -20,6 +20,8 @@ from django.http import HttpResponse from django_future.csrf import ensure_csrf_cookie from django.views.decorators.cache import cache_control from django.core.urlresolvers import reverse +from django.core.mail import send_mail + import xmodule.graders as xmgraders from xmodule.modulestore.django import modulestore from xmodule.modulestore.exceptions import ItemNotFoundError @@ -45,6 +47,7 @@ from mitxmako.shortcuts import render_to_response from psychometrics import psychoanalyze from student.models import CourseEnrollment, CourseEnrollmentAllowed import track.views +from mitxmako.shortcuts import render_to_string log = logging.getLogger(__name__) @@ -634,13 +637,15 @@ def instructor_dashboard(request, course_id): students = request.POST.get('multiple_students', '') auto_enroll = bool(request.POST.get('auto_enroll')) - ret = _do_enroll_students(course, course_id, students, auto_enroll=auto_enroll) + email_students = bool(request.POST.get('email_students')) + ret = _do_enroll_students(course, course_id, students, auto_enroll=auto_enroll, email_students=email_students) datatable = ret['datatable'] elif action == 'Unenroll multiple students': students = request.POST.get('multiple_students', '') - ret = _do_unenroll_students(course_id, students) + email_students = bool(request.POST.get('email_students')) + ret = _do_unenroll_students(course_id, students, email_students=email_students) datatable = ret['datatable'] elif action == 'List sections available in remote gradebook': @@ -1068,9 +1073,17 @@ def grade_summary(request, course_id): #----------------------------------------------------------------------------- # enrollment -def _do_enroll_students(course, course_id, students, overload=False, auto_enroll=False): - """Do the actual work of enrolling multiple students, presented as a string - of emails separated by commas or returns""" +def _do_enroll_students(course, course_id, students, overload=False, auto_enroll=False, email_students=False): + """ + Do the actual work of enrolling multiple students, presented as a string + of emails separated by commas or returns + `course` is course object + `course_id` id of course (a `str`) + `students` string of student emails separated by commas or returns (a `str`) + `overload` un-enrolls all existing students (a `boolean`) + `auto_enroll` is user input preference (a `boolean`) + `email_students` is user input preference (a `boolean`) + """ new_students, new_students_lc = get_and_clean_student_list(students) status = dict([x, 'unprocessed'] for x in new_students) @@ -1088,12 +1101,22 @@ def _do_enroll_students(course, course_id, students, overload=False, auto_enroll status[cea.email] = 'removed from pending enrollment list' ceaset.delete() + if email_students: + registration_url = 'https://' + settings.SITE_NAME + reverse('student.views.register_user') + #Composition of email + d = {'site_name': settings.SITE_NAME, + 'registration_url': registration_url, + 'course_id': course_id, + 'auto_enroll': auto_enroll, + 'course_url': registration_url + '/courses/' + course_id, + } + for student in new_students: try: user = User.objects.get(email=student) except User.DoesNotExist: - #User not signed up yet, put in pending enrollment allowed table + #Student not signed up yet, put in pending enrollment allowed table cea = CourseEnrollmentAllowed.objects.filter(email=student, course_id=course_id) #If enrollmentallowed already exists, update auto_enroll flag to however it was set in UI @@ -1104,18 +1127,42 @@ def _do_enroll_students(course, course_id, students, overload=False, auto_enroll status[student] = 'user does not exist, enrollment already allowed, pending with auto enrollment ' \ + ('on' if auto_enroll else 'off') continue + + #EnrollmentAllowed doesn't exist so create it cea = CourseEnrollmentAllowed(email=student, course_id=course_id, auto_enroll=auto_enroll) cea.save() - status[student] = 'user does not exist, enrollment allowed, pending with auto enrollment ' + ('on' if auto_enroll else 'off') + + status[student] = 'user does not exist, enrollment allowed, pending with auto enrollment ' \ + + ('on' if auto_enroll else 'off') + + if email_students: + #User is allowed to enroll but has not signed up yet + d['email_address'] = student + d['message'] = 'allowed_enroll' + send_mail_ret = send_mail_to_student(student, d) + status[student] += (', email sent' if send_mail_ret else '') continue + #Student has already registered if CourseEnrollment.objects.filter(user=user, course_id=course_id): status[student] = 'already enrolled' continue + try: + #Not enrolled yet ce = CourseEnrollment(user=user, course_id=course_id) ce.save() status[student] = 'added' + + if email_students: + #User enrolled for first time, populate dict with user specific info + d['email_address'] = student + d['first_name'] = user.first_name + d['last_name'] = user.last_name + d['message'] = 'enrolled_enroll' + send_mail_ret = send_mail_to_student(student, d) + status[student] += (', email sent' if send_mail_ret else '') + except: status[student] = 'rejected' @@ -1133,13 +1180,23 @@ def _do_enroll_students(course, course_id, students, overload=False, auto_enroll #Unenrollment -def _do_unenroll_students(course_id, students): - """Do the actual work of un-enrolling multiple students, presented as a string - of emails separated by commas or returns""" +def _do_unenroll_students(course_id, students, email_students=False): + """ + Do the actual work of un-enrolling multiple students, presented as a string + of emails separated by commas or returns + `course_id` is id of course (a `str`) + `students` is string of student emails separated by commas or returns (a `str`) + `email_students` is user input preference (a `boolean`) + """ old_students, _ = get_and_clean_student_list(students) status = dict([x, 'unprocessed'] for x in old_students) + if email_students: + #Composition of email + d = {'site_name': settings.SITE_NAME, + 'course_id': course_id} + for student in old_students: isok = False @@ -1153,6 +1210,14 @@ def _do_unenroll_students(course_id, students): try: user = User.objects.get(email=student) except User.DoesNotExist: + + if isok and email_students: + #User was allowed to join but had not signed up yet + d['email_address'] = student + d['message'] = 'allowed_unenroll' + send_mail_ret = send_mail_to_student(student, d) + status[student] += (', email sent' if send_mail_ret else '') + continue ce = CourseEnrollment.objects.filter(user=user, course_id=course_id) @@ -1161,6 +1226,15 @@ def _do_unenroll_students(course_id, students): try: ce[0].delete() status[student] = "un-enrolled" + if email_students: + #User was enrolled + d['email_address'] = student + d['first_name'] = user.first_name + d['last_name'] = user.last_name + d['message'] = 'enrolled_unenroll' + send_mail_ret = send_mail_to_student(student, d) + status[student] += (', email sent' if send_mail_ret else '') + except Exception: if not isok: status[student] = "Error! Failed to un-enroll" @@ -1173,13 +1247,48 @@ def _do_unenroll_students(course_id, students): return data +def send_mail_to_student(student, param_dict): + """ + Construct the email using templates and then send it. + `student` is the student's email address (a `str`), + + `param_dict` is a `dict` with keys [ + `site_name`: name given to edX instance (a `str`) + `registration_url`: url for registration (a `str`) + `course_id`: id of course (a `str`) + `auto_enroll`: user input option (a `str`) + `course_url`: url of course (a `str`) + `email_address`: email of student (a `str`) + `first_name`: student first name (a `str`) + `last_name`: student last name (a `str`) + `message`: type of email to send and template to use (a `str`) + ] + Returns a boolean indicating whether the email was sent successfully. + """ + + EMAIL_TEMPLATE_DICT = {'allowed_enroll': ('emails/enroll_email_allowedsubject.txt', 'emails/enroll_email_allowedmessage.txt'), + 'enrolled_enroll': ('emails/enroll_email_enrolledsubject.txt', 'emails/enroll_email_enrolledmessage.txt'), + 'allowed_unenroll': ('emails/unenroll_email_subject.txt', 'emails/unenroll_email_allowedmessage.txt'), + 'enrolled_unenroll': ('emails/unenroll_email_subject.txt', 'emails/unenroll_email_enrolledmessage.txt')} + + subject_template, message_template = EMAIL_TEMPLATE_DICT.get(param_dict['message'], (None, None)) + if subject_template is not None and message_template is not None: + subject = render_to_string(subject_template, param_dict) + message = render_to_string(message_template, param_dict) + + # Email subject *must not* contain newlines + subject = ''.join(subject.splitlines()) + send_mail(subject, message, settings.DEFAULT_FROM_EMAIL, [student], fail_silently=False) + return True + else: + return False + + def get_and_clean_student_list(students): """ Separate out individual student email from the comma, or space separated string. - - In: - students: string coming from the input text area - Return: + `students` is string of student emails separated by commas or returns (a `str`) + Returns: students: list of cleaned student emails students_lc: list of lower case cleaned student emails """ diff --git a/lms/templates/courseware/instructor_dashboard.html b/lms/templates/courseware/instructor_dashboard.html index d541962906..bc49cda427 100644 --- a/lms/templates/courseware/instructor_dashboard.html +++ b/lms/templates/courseware/instructor_dashboard.html @@ -382,6 +382,8 @@ function goto( mode)

Enroll or un-enroll one or many students: enter emails, separated by new lines or commas;

+ Notify students by email +

Auto-enroll students when they activate

diff --git a/lms/templates/emails/enroll_email_allowedmessage.txt b/lms/templates/emails/enroll_email_allowedmessage.txt new file mode 100644 index 0000000000..eab347166e --- /dev/null +++ b/lms/templates/emails/enroll_email_allowedmessage.txt @@ -0,0 +1,13 @@ +Dear student, + +You have been invited to join ${course_id} at ${site_name} by a member of the course staff. + +To finish your registration, please visit ${registration_url} and fill out the registration form. +% if auto_enroll: +Once you have registered and activated your account, you will see ${course_id} listed on your dashboard. +% else: +Once you have registered and activated your account, visit ${course_url} to join the course. +% endif + +---- +This email was automatically sent from ${site_name} to ${email_address} \ No newline at end of file diff --git a/lms/templates/emails/enroll_email_allowedsubject.txt b/lms/templates/emails/enroll_email_allowedsubject.txt new file mode 100644 index 0000000000..41da60d1db --- /dev/null +++ b/lms/templates/emails/enroll_email_allowedsubject.txt @@ -0,0 +1 @@ +You have been invited to register for ${course_id} \ No newline at end of file diff --git a/lms/templates/emails/enroll_email_enrolledmessage.txt b/lms/templates/emails/enroll_email_enrolledmessage.txt new file mode 100644 index 0000000000..8e8f24efed --- /dev/null +++ b/lms/templates/emails/enroll_email_enrolledmessage.txt @@ -0,0 +1,8 @@ +Dear ${first_name} ${last_name} + +You have been enrolled in ${course_id} at ${site_name} by a member of the course staff. The course should now appear on your ${site_name} dashboard. + +To start accessing course materials, please visit ${course_url} + +---- +This email was automatically sent from ${site_name} to ${first_name} ${last_name} \ No newline at end of file diff --git a/lms/templates/emails/enroll_email_enrolledsubject.txt b/lms/templates/emails/enroll_email_enrolledsubject.txt new file mode 100644 index 0000000000..db897a3299 --- /dev/null +++ b/lms/templates/emails/enroll_email_enrolledsubject.txt @@ -0,0 +1 @@ +You have been enrolled in ${course_id} \ No newline at end of file diff --git a/lms/templates/emails/unenroll_email_allowedmessage.txt b/lms/templates/emails/unenroll_email_allowedmessage.txt new file mode 100644 index 0000000000..9bd0bd3cfd --- /dev/null +++ b/lms/templates/emails/unenroll_email_allowedmessage.txt @@ -0,0 +1,6 @@ +Dear Student, + +You have been un-enrolled from course ${course_id} by a member of the course staff. Please disregard the invitation previously sent. + +---- +This email was automatically sent from ${site_name} to ${email_address} \ No newline at end of file diff --git a/lms/templates/emails/unenroll_email_enrolledmessage.txt b/lms/templates/emails/unenroll_email_enrolledmessage.txt new file mode 100644 index 0000000000..8a7f9f996e --- /dev/null +++ b/lms/templates/emails/unenroll_email_enrolledmessage.txt @@ -0,0 +1,8 @@ +Dear ${first_name} ${last_name} + +You have been un-enrolled in ${course_id} at ${site_name} by a member of the course staff. The course will no longer appear on your ${site_name} dashboard. + +Your other courses have not been affected. + +---- +This email was automatically sent from ${site_name} to ${first_name} ${last_name} \ No newline at end of file diff --git a/lms/templates/emails/unenroll_email_subject.txt b/lms/templates/emails/unenroll_email_subject.txt new file mode 100644 index 0000000000..f79218ff22 --- /dev/null +++ b/lms/templates/emails/unenroll_email_subject.txt @@ -0,0 +1 @@ +You have been un-enrolled from ${course_id} \ No newline at end of file From c98a77565fef43ebc92c5d58d614cacb6d17c8f2 Mon Sep 17 00:00:00 2001 From: Jay Zoldak Date: Thu, 27 Jun 2013 10:52:26 -0400 Subject: [PATCH 55/55] Make the UrlResetMixin load the urlconf after resetting it, and fix the comment client test that was leaving ENABLE_DISCUSSION_SERVICE at True --- common/djangoapps/util/testing.py | 5 ++++- lms/djangoapps/django_comment_client/base/tests.py | 12 ++++-------- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/common/djangoapps/util/testing.py b/common/djangoapps/util/testing.py index d33f1c8f8b..062b04c8a0 100644 --- a/common/djangoapps/util/testing.py +++ b/common/djangoapps/util/testing.py @@ -1,7 +1,7 @@ import sys from django.conf import settings -from django.core.urlresolvers import clear_url_caches +from django.core.urlresolvers import clear_url_caches, resolve class UrlResetMixin(object): @@ -27,6 +27,9 @@ class UrlResetMixin(object): reload(sys.modules[urlconf]) clear_url_caches() + # Resolve a URL so that the new urlconf gets loaded + resolve('/') + def setUp(self): """Reset django default urlconf before tests and after tests""" super(UrlResetMixin, self).setUp() diff --git a/lms/djangoapps/django_comment_client/base/tests.py b/lms/djangoapps/django_comment_client/base/tests.py index aa5b657bd6..434d4d616b 100644 --- a/lms/djangoapps/django_comment_client/base/tests.py +++ b/lms/djangoapps/django_comment_client/base/tests.py @@ -1,6 +1,5 @@ import logging -from django.conf import settings from django.test.utils import override_settings from django.test.client import Client from django.contrib.auth.models import User @@ -21,16 +20,13 @@ log = logging.getLogger(__name__) @override_settings(MODULESTORE=TEST_DATA_MONGO_MODULESTORE) @patch('comment_client.utils.requests.request') class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase): + + @patch.dict("django.conf.settings.MITX_FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) def setUp(self): - # This feature affects the contents of urls.py, so we change - # it before the call to super.setUp() which reloads urls.py (because + # Patching the ENABLE_DISCUSSION_SERVICE value affects the contents of urls.py, + # so we need to call super.setUp() which reloads urls.py (because # of the UrlResetMixin) - - # This setting is cleaned up at the end of the test by @override_settings, which - # restores all of the old settings - settings.MITX_FEATURES['ENABLE_DISCUSSION_SERVICE'] = True - super(ViewsTestCase, self).setUp() # create a course