From 9ee246e4073f2649ecdb88a58c89b6a403baabf4 Mon Sep 17 00:00:00 2001 From: e0d Date: Thu, 2 May 2013 15:50:33 -0400 Subject: [PATCH 01/33] flexible org repos parsing tested locally --- jenkins/base.sh | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/jenkins/base.sh b/jenkins/base.sh index fc2595662a..7eb4802b8f 100644 --- a/jenkins/base.sh +++ b/jenkins/base.sh @@ -1,6 +1,19 @@ +## +## requires >= 1.3.0 of the Jenkins git plugin +## function github_status { - gcli status create edx mitx $GIT_COMMIT \ + + if [[ ! ${GIT_URL} =~ git@github.com:([^/]+)/([^\.]+).git ]]; then + echo "Cannot parse Github org or repo from URL, using defaults." + ORG="edx" + REPO="mitx" + else + ORG=${BASH_REMATCH[1]} + REPO=${BASH_REMATCH[2]} + fi + + gcli status create $ORG $REPO $GIT_COMMIT \ --params=$1 \ target_url:$BUILD_URL \ description:"Build #$BUILD_NUMBER is running" \ From 3352ae7f752202fee7e94c94957f34e84dd4e838 Mon Sep 17 00:00:00 2001 From: John Jarvis Date: Thu, 9 May 2013 18:29:45 -0400 Subject: [PATCH 02/33] repo rename to edx-platform --- rakefile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rakefile b/rakefile index 32d92a0349..4d70547a51 100644 --- a/rakefile +++ b/rakefile @@ -14,8 +14,8 @@ LMS_REPORT_DIR = File.join(REPORT_DIR, "lms") # Packaging constants DEPLOY_DIR = "/opt/wwc" -PACKAGE_NAME = "mitx" -LINK_PATH = "/opt/wwc/mitx" +PACKAGE_NAME = "edx-platform" +LINK_PATH = "/opt/wwc/edx-platform" PKG_VERSION = "0.1" COMMIT = (ENV["GIT_COMMIT"] || `git rev-parse HEAD`).chomp()[0, 10] BRANCH = (ENV["GIT_BRANCH"] || `git symbolic-ref -q HEAD`).chomp().gsub('refs/heads/', '').gsub('origin/', '') From a1e6c194c6455e1895328842d39d48858356439d Mon Sep 17 00:00:00 2001 From: John Jarvis Date: Fri, 10 May 2013 10:24:32 -0400 Subject: [PATCH 03/33] removing LINK_PATH --- rakefile | 1 - 1 file changed, 1 deletion(-) diff --git a/rakefile b/rakefile index 4d70547a51..fb6eb1c6e5 100644 --- a/rakefile +++ b/rakefile @@ -15,7 +15,6 @@ LMS_REPORT_DIR = File.join(REPORT_DIR, "lms") # Packaging constants DEPLOY_DIR = "/opt/wwc" PACKAGE_NAME = "edx-platform" -LINK_PATH = "/opt/wwc/edx-platform" PKG_VERSION = "0.1" COMMIT = (ENV["GIT_COMMIT"] || `git rev-parse HEAD`).chomp()[0, 10] BRANCH = (ENV["GIT_BRANCH"] || `git symbolic-ref -q HEAD`).chomp().gsub('refs/heads/', '').gsub('origin/', '') From f970bbd121da44f6a28a4d29170ec7acc1ea49a3 Mon Sep 17 00:00:00 2001 From: Peter Baratta Date: Fri, 10 May 2013 15:21:29 -0400 Subject: [PATCH 04/33] Add tests on the problem level that show the infinite answer bug --- .../lib/capa/capa/tests/test_responsetypes.py | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/common/lib/capa/capa/tests/test_responsetypes.py b/common/lib/capa/capa/tests/test_responsetypes.py index 7a43fff4c9..f7848ca094 100644 --- a/common/lib/capa/capa/tests/test_responsetypes.py +++ b/common/lib/capa/capa/tests/test_responsetypes.py @@ -438,6 +438,36 @@ class FormulaResponseTest(ResponseTest): self.assert_grade(problem, incorrect, 'incorrect', msg="Failed on function {0}; the given, incorrect answer was {1} but graded 'correct'".format(func, incorrect)) + def test_grade_infinity(self): + # This resolves a bug where a problem with relative tolerance would + # pass with any arbitrarily large student answer. + + sample_dict = {'x' : (1,2)} + + # Test problem + problem = self.build_problem(sample_dict=sample_dict, + num_samples=10, + tolerance="1%", + answer="x") + # Expect such a large answer to be marked incorrect + input_formula = "x*1e999" + self.assert_grade(problem, input_formula, "incorrect") + + def test_grade_nan(self): + # attempt to produce a value which causes the student's answer to be + # evaluated to nan. See if this is resolved correctly. + + sample_dict = {'x' : (1,2)} + + # Test problem + problem = self.build_problem(sample_dict=sample_dict, + num_samples=10, + tolerance="1%", + answer="x") + # Expect an incorrect answer (+ nan) to be marked incorrect + input_formula = "10*x + 0*1e999" # right now this evaluates to 'nan' for a given x + self.assert_grade(problem, input_formula, "incorrect") + class StringResponseTest(ResponseTest): from response_xml_factory import StringResponseXMLFactory @@ -714,6 +744,28 @@ class NumericalResponseTest(ResponseTest): incorrect_responses = ["", "4.5", "3.5", "0"] self.assert_multiple_grade(problem, correct_responses, incorrect_responses) + def test_grade_infinity(self): + # This resolves a bug where a problem with relative tolerance would + # pass with any arbitrarily large student answer. + problem = self.build_problem(question_text="What is 2 + 2 approximately?", + explanation="The answer is 4", + answer=4, + tolerance="10%") + correct_responses = [] + incorrect_responses = ["1e999"] + self.assert_multiple_grade(problem, correct_responses, incorrect_responses) + + def test_grade_nan(self): + # attempt to produce a value which causes the student's answer to be + # evaluated to nan. See if this is resolved correctly. + problem = self.build_problem(question_text="What is 2 + 2 approximately?", + explanation="The answer is 4", + answer=4, + tolerance="10%") + correct_responses = [] + incorrect_responses = ["0*1e999"] # right now this evaluates to 'nan' for a given x + self.assert_multiple_grade(problem, correct_responses, incorrect_responses) + def test_grade_with_script(self): script_text = "computed_response = math.sqrt(4)" problem = self.build_problem(question_text="What is sqrt(4)?", From a1db394bcd4c841ea48fb3b13ac00737d26db28a Mon Sep 17 00:00:00 2001 From: Peter Baratta Date: Fri, 10 May 2013 15:22:23 -0400 Subject: [PATCH 05/33] Test for infinity in numerical and formula responses --- common/lib/capa/capa/responsetypes.py | 2 -- common/lib/capa/capa/util.py | 6 +++++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/common/lib/capa/capa/responsetypes.py b/common/lib/capa/capa/responsetypes.py index fda70c6a55..f4f5d854a9 100644 --- a/common/lib/capa/capa/responsetypes.py +++ b/common/lib/capa/capa/responsetypes.py @@ -1869,8 +1869,6 @@ class FormulaResponse(LoncapaResponse): log.debug('formularesponse: error %s in formula' % err) raise StudentInputError("Invalid input: Could not parse '%s' as a formula" % cgi.escape(given)) - if numpy.isnan(student_result) or numpy.isinf(student_result): - return "incorrect" if not compare_with_tolerance(student_result, instructor_result, self.tolerance): return "incorrect" return "correct" diff --git a/common/lib/capa/capa/util.py b/common/lib/capa/capa/util.py index 9f3e8bd3a0..c219a7b5f6 100644 --- a/common/lib/capa/capa/util.py +++ b/common/lib/capa/capa/util.py @@ -1,4 +1,5 @@ from .calc import evaluator, UndefinedVariable +from cmath import isinf #----------------------------------------------------------------------------- # @@ -20,8 +21,11 @@ def compare_with_tolerance(v1, v2, tol): tolerance = tolerance_rel * max(abs(v1), abs(v2)) else: tolerance = evaluator(dict(), dict(), tol) - return abs(v1 - v2) <= tolerance + if isinf(v1) or isinf(v2): + return v1 == v2 # because the other numerical comparison does not work with infinities + else: + return abs(v1 - v2) <= tolerance def contextualize_text(text, context): # private ''' Takes a string with variables. E.g. $a+$b. From 2b9d78dfd5d939687c5310252734b3f6feda94be Mon Sep 17 00:00:00 2001 From: Peter Baratta Date: Mon, 13 May 2013 10:18:19 -0400 Subject: [PATCH 06/33] Pep8 fixes and changes to NaN tests --- common/lib/capa/capa/responsetypes.py | 3 ++- common/lib/capa/capa/tests/test_responsetypes.py | 13 +++++++++---- common/lib/capa/capa/util.py | 7 +++++-- 3 files changed, 16 insertions(+), 7 deletions(-) diff --git a/common/lib/capa/capa/responsetypes.py b/common/lib/capa/capa/responsetypes.py index f4f5d854a9..65e903b576 100644 --- a/common/lib/capa/capa/responsetypes.py +++ b/common/lib/capa/capa/responsetypes.py @@ -2,7 +2,8 @@ # File: courseware/capa/responsetypes.py # ''' -Problem response evaluation. Handles checking of student responses, of a variety of types. +Problem response evaluation. Handles checking of student responses, +of a variety of types. Used by capa_problem.py ''' diff --git a/common/lib/capa/capa/tests/test_responsetypes.py b/common/lib/capa/capa/tests/test_responsetypes.py index f7848ca094..554df1cde6 100644 --- a/common/lib/capa/capa/tests/test_responsetypes.py +++ b/common/lib/capa/capa/tests/test_responsetypes.py @@ -442,7 +442,7 @@ class FormulaResponseTest(ResponseTest): # This resolves a bug where a problem with relative tolerance would # pass with any arbitrarily large student answer. - sample_dict = {'x' : (1,2)} + sample_dict = {'x': (1, 2)} # Test problem problem = self.build_problem(sample_dict=sample_dict, @@ -457,7 +457,7 @@ class FormulaResponseTest(ResponseTest): # attempt to produce a value which causes the student's answer to be # evaluated to nan. See if this is resolved correctly. - sample_dict = {'x' : (1,2)} + sample_dict = {'x': (1, 2)} # Test problem problem = self.build_problem(sample_dict=sample_dict, @@ -465,7 +465,11 @@ class FormulaResponseTest(ResponseTest): tolerance="1%", answer="x") # Expect an incorrect answer (+ nan) to be marked incorrect - input_formula = "10*x + 0*1e999" # right now this evaluates to 'nan' for a given x + # right now this evaluates to 'nan' for a given x (Python implementation-dependent) + input_formula = "10*x + 0*1e999" + self.assert_grade(problem, input_formula, "incorrect") + # Expect an correct answer (+ nan) to be marked incorrect + input_formula = "x + 0*1e999" self.assert_grade(problem, input_formula, "incorrect") @@ -763,7 +767,8 @@ class NumericalResponseTest(ResponseTest): answer=4, tolerance="10%") correct_responses = [] - incorrect_responses = ["0*1e999"] # right now this evaluates to 'nan' for a given x + # right now these evaluate to 'nan' + incorrect_responses = ["0*1e999", "4 + 0*1e999"] self.assert_multiple_grade(problem, correct_responses, incorrect_responses) def test_grade_with_script(self): diff --git a/common/lib/capa/capa/util.py b/common/lib/capa/capa/util.py index c219a7b5f6..4663b388c2 100644 --- a/common/lib/capa/capa/util.py +++ b/common/lib/capa/capa/util.py @@ -23,10 +23,12 @@ def compare_with_tolerance(v1, v2, tol): tolerance = evaluator(dict(), dict(), tol) if isinf(v1) or isinf(v2): - return v1 == v2 # because the other numerical comparison does not work with infinities + # because the other numerical comparison does not work with infinities + return v1 == v2 else: return abs(v1 - v2) <= tolerance + def contextualize_text(text, context): # private ''' Takes a string with variables. E.g. $a+$b. Does a substitution of those variables from the context ''' @@ -55,7 +57,8 @@ def convert_files_to_filenames(answers): new_answers = dict() for answer_id in answers.keys(): answer = answers[answer_id] - if is_list_of_files(answer): # Files are stored as a list, even if one file + # Files are stored as a list, even if one file + if is_list_of_files(answer): new_answers[answer_id] = [f.name for f in answer] else: new_answers[answer_id] = answers[answer_id] From 06efd40ba982e0b7304402db203adb4422fbf977 Mon Sep 17 00:00:00 2001 From: Peter Baratta Date: Mon, 13 May 2013 11:29:46 -0400 Subject: [PATCH 07/33] Comment capitalization --- common/lib/capa/capa/tests/test_responsetypes.py | 14 +++++++++----- common/lib/capa/capa/util.py | 4 +++- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/common/lib/capa/capa/tests/test_responsetypes.py b/common/lib/capa/capa/tests/test_responsetypes.py index 554df1cde6..da3d45ad74 100644 --- a/common/lib/capa/capa/tests/test_responsetypes.py +++ b/common/lib/capa/capa/tests/test_responsetypes.py @@ -452,9 +452,12 @@ class FormulaResponseTest(ResponseTest): # Expect such a large answer to be marked incorrect input_formula = "x*1e999" self.assert_grade(problem, input_formula, "incorrect") + # Expect such a large negative answer to be marked incorrect + input_formula = "-x*1e999" + self.assert_grade(problem, input_formula, "incorrect") def test_grade_nan(self): - # attempt to produce a value which causes the student's answer to be + # Attempt to produce a value which causes the student's answer to be # evaluated to nan. See if this is resolved correctly. sample_dict = {'x': (1, 2)} @@ -465,7 +468,7 @@ class FormulaResponseTest(ResponseTest): tolerance="1%", answer="x") # Expect an incorrect answer (+ nan) to be marked incorrect - # right now this evaluates to 'nan' for a given x (Python implementation-dependent) + # Right now this evaluates to 'nan' for a given x (Python implementation-dependent) input_formula = "10*x + 0*1e999" self.assert_grade(problem, input_formula, "incorrect") # Expect an correct answer (+ nan) to be marked incorrect @@ -756,18 +759,19 @@ class NumericalResponseTest(ResponseTest): answer=4, tolerance="10%") correct_responses = [] - incorrect_responses = ["1e999"] + incorrect_responses = ["1e999", "-1e999"] self.assert_multiple_grade(problem, correct_responses, incorrect_responses) def test_grade_nan(self): - # attempt to produce a value which causes the student's answer to be + # Attempt to produce a value which causes the student's answer to be # evaluated to nan. See if this is resolved correctly. problem = self.build_problem(question_text="What is 2 + 2 approximately?", explanation="The answer is 4", answer=4, tolerance="10%") correct_responses = [] - # right now these evaluate to 'nan' + # Right now these evaluate to `nan` + # `4 + nan` should be incorrect incorrect_responses = ["0*1e999", "4 + 0*1e999"] self.assert_multiple_grade(problem, correct_responses, incorrect_responses) diff --git a/common/lib/capa/capa/util.py b/common/lib/capa/capa/util.py index 4663b388c2..8b05ea717e 100644 --- a/common/lib/capa/capa/util.py +++ b/common/lib/capa/capa/util.py @@ -23,7 +23,9 @@ def compare_with_tolerance(v1, v2, tol): tolerance = evaluator(dict(), dict(), tol) if isinf(v1) or isinf(v2): - # because the other numerical comparison does not work with infinities + # If an input is infinite, we can end up with `abs(v1-v2)` and + # `tolerance` both equal to infinity. Then, below we would have + # `inf <= inf` which is a fail. Instead, compare directly. return v1 == v2 else: return abs(v1 - v2) <= tolerance From 7c989814988b7c0f9ce0a97fc06d66602df69836 Mon Sep 17 00:00:00 2001 From: David Baumgold Date: Mon, 13 May 2013 11:33:07 -0400 Subject: [PATCH 08/33] Install phantom-jasmine via npm, rather than git submodules MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Because removing a step from our install process is a Good Thing™ --- .gitmodules | 3 --- cms/envs/jasmine.py | 2 +- package.json | 7 +++++-- rakefile | 6 +++--- 4 files changed, 9 insertions(+), 9 deletions(-) delete mode 100644 .gitmodules diff --git a/.gitmodules b/.gitmodules deleted file mode 100644 index 253bae3686..0000000000 --- a/.gitmodules +++ /dev/null @@ -1,3 +0,0 @@ -[submodule "common/test/phantom-jasmine"] - path = common/test/phantom-jasmine - url = https://github.com/jcarver989/phantom-jasmine.git \ No newline at end of file diff --git a/cms/envs/jasmine.py b/cms/envs/jasmine.py index e046a6d37c..6c7cbcdcb0 100644 --- a/cms/envs/jasmine.py +++ b/cms/envs/jasmine.py @@ -33,7 +33,7 @@ PIPELINE_JS['spec'] = { JASMINE_TEST_DIRECTORY = PROJECT_ROOT + '/static/coffee' -STATICFILES_DIRS.append(COMMON_ROOT / 'test' / 'phantom-jasmine' / 'lib') +STATICFILES_DIRS.append(REPO_ROOT/'node_modules/phantom-jasmine/lib') # Remove the localization middleware class because it requires the test database # to be sync'd and migrated in order to run the jasmine tests interactively diff --git a/package.json b/package.json index 4ce95d04ce..0f0eb7fef3 100644 --- a/package.json +++ b/package.json @@ -1,5 +1,8 @@ { "name": "mitx", "version": "0.1.0", - "dependencies": { "coffee-script": "1.6.x"} -} \ No newline at end of file + "dependencies": { + "coffee-script": "1.6.X", + "phantom-jasmine": "0.3.X" + } +} diff --git a/rakefile b/rakefile index 32d92a0349..e8a087226d 100644 --- a/rakefile +++ b/rakefile @@ -95,7 +95,7 @@ def template_jasmine_runner(lib) if !coffee_files.empty? sh("node_modules/.bin/coffee -c #{coffee_files.join(' ')}") end - phantom_jasmine_path = File.expand_path("common/test/phantom-jasmine") + phantom_jasmine_path = File.expand_path("node_modules/phantom-jasmine") common_js_root = File.expand_path("common/static/js") common_coffee_root = File.expand_path("common/static/coffee/src") @@ -319,7 +319,7 @@ end compile_assets() phantomjs = ENV['PHANTOMJS_PATH'] || 'phantomjs' django_for_jasmine(system, false) do |jasmine_url| - sh("#{phantomjs} common/test/phantom-jasmine/lib/run_jasmine_test.coffee #{jasmine_url}") + sh("#{phantomjs} node_modules/phantom-jasmine/lib/run_jasmine_test.coffee #{jasmine_url}") end end end @@ -376,7 +376,7 @@ Dir["common/lib/*"].select{|lib| File.directory?(lib)}.each do |lib| task "phantomjs_jasmine_#{lib}" do phantomjs = ENV['PHANTOMJS_PATH'] || 'phantomjs' template_jasmine_runner(lib) do |f| - sh("#{phantomjs} common/test/phantom-jasmine/lib/run_jasmine_test.coffee #{f}") + sh("#{phantomjs} node_modules/phantom-jasmine/lib/run_jasmine_test.coffee #{f}") end end end From d80024472c9600d207d6f2cd31f3d44882ef224c Mon Sep 17 00:00:00 2001 From: Peter Baratta Date: Mon, 13 May 2013 12:40:32 -0400 Subject: [PATCH 09/33] Pylint fixes --- common/lib/capa/capa/responsetypes.py | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/common/lib/capa/capa/responsetypes.py b/common/lib/capa/capa/responsetypes.py index 65e903b576..3b17daa830 100644 --- a/common/lib/capa/capa/responsetypes.py +++ b/common/lib/capa/capa/responsetypes.py @@ -36,7 +36,7 @@ from datetime import datetime from .util import * from lxml import etree from lxml.html.soupparser import fromstring as fromstring_bs # uses Beautiful Soup!!! FIXME? -import xqueue_interface +import capa.xqueue_interface log = logging.getLogger(__name__) @@ -301,7 +301,7 @@ class LoncapaResponse(object): # response aid = self.answer_ids[-1] new_cmap.set_hint_and_mode(aid, hint_text, hintmode) - log.debug('after hint: new_cmap = %s' % new_cmap) + log.debug('after hint: new_cmap = %s', new_cmap) @abc.abstractmethod def get_score(self, student_answers): @@ -791,6 +791,10 @@ class OptionResponse(LoncapaResponse): class NumericalResponse(LoncapaResponse): + ''' + This response type expects a number or formulaic expression that evaluates + to a number (e.g. `4+5/2^2`), and accepts with a tolerance. + ''' response_tag = 'numericalresponse' hint_tag = 'numericalhint' @@ -807,12 +811,12 @@ class NumericalResponse(LoncapaResponse): '//*[@id=$id]//responseparam[@type="tolerance"]/@default', id=xml.get('id'))[0] self.tolerance = contextualize_text(self.tolerance_xml, context) - except Exception: + except IndexError: # xpath found an empty list, so (...)[0] is the error self.tolerance = '0' try: self.answer_id = xml.xpath('//*[@id=$id]//textline/@id', id=xml.get('id'))[0] - except Exception: + except IndexError: # Same as above self.answer_id = None def get_score(self, student_answers): @@ -837,7 +841,6 @@ class NumericalResponse(LoncapaResponse): except: # Use the traceback-preserving version of re-raising with a # different type - import sys type, value, traceback = sys.exc_info() raise StudentInputError, ("Could not interpret '%s' as a number" % From eb843172ff5084397f53238ef51a6a474da31046 Mon Sep 17 00:00:00 2001 From: e0d Date: Mon, 13 May 2013 14:02:41 -0400 Subject: [PATCH 10/33] Enable Delft and Georgetown to be handled by the dynamic path --- lms/urls.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lms/urls.py b/lms/urls.py index b00813a40d..d5b0e46bb9 100644 --- a/lms/urls.py +++ b/lms/urls.py @@ -61,10 +61,12 @@ urlpatterns = ('', # nopep8 url(r'^heartbeat$', include('heartbeat.urls')), + ## + ## Only universities without courses should be included here. If + ## courses exist, the dynamic profile rule below should win. + ## url(r'^(?i)university_profile/WellesleyX$', 'courseware.views.static_university_profile', name="static_university_profile", kwargs={'org_id': 'WellesleyX'}), - url(r'^(?i)university_profile/GeorgetownX$', 'courseware.views.static_university_profile', - name="static_university_profile", kwargs={'org_id': 'GeorgetownX'}), url(r'^(?i)university_profile/McGillX$', 'courseware.views.static_university_profile', name="static_university_profile", kwargs={'org_id': 'McGillX'}), url(r'^(?i)university_profile/TorontoX$', 'courseware.views.static_university_profile', @@ -73,8 +75,6 @@ urlpatterns = ('', # nopep8 name="static_university_profile", kwargs={'org_id': 'RiceX'}), url(r'^(?i)university_profile/ANUx$', 'courseware.views.static_university_profile', name="static_university_profile", kwargs={'org_id': 'ANUx'}), - url(r'^(?i)university_profile/DelftX$', 'courseware.views.static_university_profile', - name="static_university_profile", kwargs={'org_id': 'DelftX'}), url(r'^(?i)university_profile/EPFLx$', 'courseware.views.static_university_profile', name="static_university_profile", kwargs={'org_id': 'EPFLx'}), From b3cc9a117463794d379b104690f0587c040f8360 Mon Sep 17 00:00:00 2001 From: Steve Strassmann Date: Mon, 13 May 2013 14:55:41 -0400 Subject: [PATCH 11/33] pep8 cleanups --- cms/djangoapps/contentstore/views/assets.py | 34 +++++++++++---------- cms/djangoapps/contentstore/views/course.py | 33 +++++++++++++------- 2 files changed, 40 insertions(+), 27 deletions(-) diff --git a/cms/djangoapps/contentstore/views/assets.py b/cms/djangoapps/contentstore/views/assets.py index 25f7bb066d..0ae09fbd70 100644 --- a/cms/djangoapps/contentstore/views/assets.py +++ b/cms/djangoapps/contentstore/views/assets.py @@ -59,12 +59,12 @@ def asset_index(request, org, course, name): asset_display = [] for asset in assets: - id = asset['_id'] + asset_id = asset['_id'] display_info = {} display_info['displayname'] = asset['displayname'] display_info['uploadDate'] = get_default_time_display(asset['uploadDate'].timetuple()) - asset_location = StaticContent.compute_location(id['org'], id['course'], id['name']) + asset_location = StaticContent.compute_location(asset_id['org'], asset_id['course'], asset_id['name']) display_info['url'] = StaticContent.get_url_path_from_location(asset_location) # note, due to the schema change we may not have a 'thumbnail_location' in the result set @@ -149,9 +149,9 @@ def import_course(request, org, course, name): location = get_location_and_verify_access(request, org, course, name) if request.method == 'POST': - filename = request.FILES['course-data'].name + filenames = request.FILES['course-data'].name - if not filename.endswith('.tar.gz'): + if not filenames.endswith('.tar.gz'): return HttpResponse(json.dumps({'ErrMsg': 'We only support uploading a .tar.gz file.'})) data_root = path(settings.GITHUB_REPO_ROOT) @@ -161,7 +161,7 @@ def import_course(request, org, course, name): if not course_dir.isdir(): os.mkdir(course_dir) - temp_filepath = course_dir / filename + temp_filepath = course_dir / filenames logging.debug('importing course to {0}'.format(temp_filepath)) @@ -171,13 +171,13 @@ def import_course(request, org, course, name): temp_file.write(chunk) temp_file.close() - tf = tarfile.open(temp_filepath) - tf.extractall(course_dir + '/') + tar_file = tarfile.open(temp_filepath) + tar_file.extractall(course_dir + '/') # find the 'course.xml' file - for r, d, f in os.walk(course_dir): - for files in f: + for dirpath, dirnames, filenames in os.walk(course_dir): + for files in filenames: if files == 'course.xml': break if files == 'course.xml': @@ -186,12 +186,14 @@ def import_course(request, org, course, name): if files != '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(r)) + logging.debug('found course.xml at {0}'.format(dirpath)) - if r != course_dir: - for fname in os.listdir(r): - shutil.move(r / fname, course_dir) + if dirpath != course_dir: + for fname in os.listdir(dirpath): + shutil.move(dirpath / fname, course_dir) + # var module_store is unused + # pylint: disable=W0612 module_store, course_items = import_from_xml(modulestore('direct'), settings.GITHUB_REPO_ROOT, [course_subdir], load_error_modules=False, static_content_store=contentstore(), @@ -234,9 +236,9 @@ def generate_export_course(request, org, course, name): #filename = root_dir / name + '.tar.gz' logging.debug('tar file being generated at {0}'.format(export_file.name)) - tf = tarfile.open(name=export_file.name, mode='w:gz') - tf.add(root_dir / name, arcname=name) - tf.close() + tar_file = tarfile.open(name=export_file.name, mode='w:gz') + tar_file.add(root_dir / name, arcname=name) + tar_file.close() # remove temp dir shutil.rmtree(root_dir / name) diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index ecc271926a..c6fa340f67 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -1,3 +1,6 @@ +""" +Views related to operations on course objects +""" import json import time @@ -10,16 +13,17 @@ from django.core.urlresolvers import reverse from mitxmako.shortcuts import render_to_response from xmodule.modulestore.django import modulestore -from xmodule.modulestore.exceptions import ItemNotFoundError, \ - InvalidLocationError +from xmodule.modulestore.exceptions \ + import ItemNotFoundError, InvalidLocationError from xmodule.modulestore import Location -from contentstore.course_info_model import get_course_updates, \ - update_course_updates, delete_course_update -from contentstore.utils import get_lms_link_for_item, \ - add_open_ended_panel_tab, remove_open_ended_panel_tab -from models.settings.course_details import CourseDetails, \ - CourseSettingsEncoder +from contentstore.course_info_model \ + import get_course_updates, update_course_updates, delete_course_update +from contentstore.utils \ + import get_lms_link_for_item, add_open_ended_panel_tab, \ + remove_open_ended_panel_tab +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 @@ -30,7 +34,13 @@ from .requests import get_request_method from .tabs import initialize_course_tabs from .component import OPEN_ENDED_COMPONENT_TYPES, ADVANCED_COMPONENT_POLICY_KEY -# TODO: should explicitly enumerate exports with __all__ +__all__ = ['course_index', 'create_new_course', 'course_info', + 'course_info_updates', 'get_course_settings', + 'course_config_graders_page', + 'course_config_advanced_page', + 'course_settings_updates', + 'course_grader_updates', + 'course_advanced_updates'] @login_required @@ -87,8 +97,9 @@ def create_new_course(request): try: dest_location = Location('i4x', org, number, 'course', Location.clean(display_name)) - except InvalidLocationError as e: - return HttpResponse(json.dumps({'ErrMsg': "Unable to create course '" + display_name + "'.\n\n" + e.message})) + except InvalidLocationError as error: + return HttpResponse(json.dumps({'ErrMsg': "Unable to create course '" + + display_name + "'.\n\n" + error.message})) # see if the course already exists existing_course = None From 10b76220183d38de3baaf6a9faff66c5c6cbe31b Mon Sep 17 00:00:00 2001 From: Steve Strassmann Date: Mon, 13 May 2013 15:06:52 -0400 Subject: [PATCH 12/33] declare exports from course, component --- cms/djangoapps/contentstore/views/__init__.py | 8 ++------ cms/djangoapps/contentstore/views/assets.py | 6 +++--- cms/djangoapps/contentstore/views/component.py | 12 +++++++++--- 3 files changed, 14 insertions(+), 12 deletions(-) diff --git a/cms/djangoapps/contentstore/views/__init__.py b/cms/djangoapps/contentstore/views/__init__.py index 0da8a15707..8a1ebc4dff 100644 --- a/cms/djangoapps/contentstore/views/__init__.py +++ b/cms/djangoapps/contentstore/views/__init__.py @@ -1,15 +1,11 @@ # pylint: disable=W0401, W0511 -# TODO: component.py should explicitly enumerate exports with __all__ -from .component import * - -# TODO: course.py should explicitly enumerate exports with __all__ -from .course import * - # Disable warnings about import from wildcard # All files below declare exports with __all__ from .assets import * from .checklist import * +from .component import * +from .course import * from .error import * from .item import * from .preview import * diff --git a/cms/djangoapps/contentstore/views/assets.py b/cms/djangoapps/contentstore/views/assets.py index 0ae09fbd70..1e818f607a 100644 --- a/cms/djangoapps/contentstore/views/assets.py +++ b/cms/djangoapps/contentstore/views/assets.py @@ -149,9 +149,9 @@ def import_course(request, org, course, name): location = get_location_and_verify_access(request, org, course, name) if request.method == 'POST': - filenames = request.FILES['course-data'].name + filename = request.FILES['course-data'].name - if not filenames.endswith('.tar.gz'): + if not filename.endswith('.tar.gz'): return HttpResponse(json.dumps({'ErrMsg': 'We only support uploading a .tar.gz file.'})) data_root = path(settings.GITHUB_REPO_ROOT) @@ -161,7 +161,7 @@ def import_course(request, org, course, name): if not course_dir.isdir(): os.mkdir(course_dir) - temp_filepath = course_dir / filenames + temp_filepath = course_dir / filename logging.debug('importing course to {0}'.format(temp_filepath)) diff --git a/cms/djangoapps/contentstore/views/component.py b/cms/djangoapps/contentstore/views/component.py index 18253925e1..7d014448ba 100644 --- a/cms/djangoapps/contentstore/views/component.py +++ b/cms/djangoapps/contentstore/views/component.py @@ -26,9 +26,15 @@ from models.settings.course_grading import CourseGradingModel from .requests import get_request_method, _xmodule_recurse from .access import has_access -# TODO: should explicitly enumerate exports with __all__ - -# to install PIL on MacOSX: 'easy_install http://dist.repoze.org/PIL-1.1.6.tar.gz' +__all__ = ['OPEN_ENDED_COMPONENT_TYPES', + 'ADVANCED_COMPONENT_POLICY_KEY', + 'edit_subsection', + 'edit_unit', + 'assignment_type_update', + 'create_draft', + 'publish_draft', + 'unpublish_unit', + 'module_info'] log = logging.getLogger(__name__) From 7b71271bc1e06eaada644c7a156e1184737e0f11 Mon Sep 17 00:00:00 2001 From: Steve Strassmann Date: Mon, 13 May 2013 15:21:29 -0400 Subject: [PATCH 13/33] prefix unused vars with _ --- cms/djangoapps/contentstore/views/assets.py | 14 ++++++-------- cms/djangoapps/contentstore/views/preview.py | 4 ++-- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/cms/djangoapps/contentstore/views/assets.py b/cms/djangoapps/contentstore/views/assets.py index 1e818f607a..b5041d3e9f 100644 --- a/cms/djangoapps/contentstore/views/assets.py +++ b/cms/djangoapps/contentstore/views/assets.py @@ -176,7 +176,7 @@ def import_course(request, org, course, name): # find the 'course.xml' file - for dirpath, dirnames, filenames in os.walk(course_dir): + for dirpath, _dirnames, filenames in os.walk(course_dir): for files in filenames: if files == 'course.xml': break @@ -192,13 +192,11 @@ def import_course(request, org, course, name): for fname in os.listdir(dirpath): shutil.move(dirpath / fname, course_dir) - # var module_store is unused - # pylint: disable=W0612 - module_store, course_items = import_from_xml(modulestore('direct'), settings.GITHUB_REPO_ROOT, - [course_subdir], load_error_modules=False, - static_content_store=contentstore(), - target_location_namespace=Location(location), - draft_store=modulestore()) + _module_store, course_items = import_from_xml(modulestore('direct'), settings.GITHUB_REPO_ROOT, + [course_subdir], load_error_modules=False, + static_content_store=contentstore(), + target_location_namespace=Location(location), + draft_store=modulestore()) # we can blow this away when we're done importing. shutil.rmtree(course_dir) diff --git a/cms/djangoapps/contentstore/views/preview.py b/cms/djangoapps/contentstore/views/preview.py index 515ea3e837..deef6a27c9 100644 --- a/cms/djangoapps/contentstore/views/preview.py +++ b/cms/djangoapps/contentstore/views/preview.py @@ -96,7 +96,7 @@ def preview_module_system(request, preview_id, descriptor): return ModuleSystem( ajax_url=reverse('preview_dispatch', args=[preview_id, descriptor.location.url(), '']).rstrip('/'), # TODO (cpennington): Do we want to track how instructors are using the preview problems? - track_function=lambda type, event: None, + track_function=lambda event_type, event: None, filestore=descriptor.system.resources_fs, get_module=partial(get_preview_module, request, preview_id), render_template=render_from_lms, @@ -171,7 +171,7 @@ def get_module_previews(request, descriptor): descriptor: An XModuleDescriptor """ preview_html = [] - for idx, (instance_state, shared_state) in enumerate(descriptor.get_sample_state()): + for idx, (_instance_state, _shared_state) in enumerate(descriptor.get_sample_state()): module = load_preview_module(request, str(idx), descriptor) preview_html.append(module.get_html()) return preview_html From 4f2c3b78daefd6e3d1fe43b34ad9456330a90d2e Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Mon, 13 May 2013 16:27:27 -0400 Subject: [PATCH 14/33] fix order of operations to make things rotate correctly --- common/lib/xmodule/xmodule/js/src/word_cloud/word_cloud_main.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/lib/xmodule/xmodule/js/src/word_cloud/word_cloud_main.js b/common/lib/xmodule/xmodule/js/src/word_cloud/word_cloud_main.js index 5237d462d6..fe985cfb2c 100644 --- a/common/lib/xmodule/xmodule/js/src/word_cloud/word_cloud_main.js +++ b/common/lib/xmodule/xmodule/js/src/word_cloud/word_cloud_main.js @@ -165,7 +165,7 @@ define('WordCloudMain', ['logme'], function (logme) { d3.layout.cloud().size([this.width, this.height]) .words(words) .rotate(function () { - return Math.floor((Math.random() * 2) * 90); + return Math.floor((Math.random() * 2)) * 90; }) .font('Impact') .fontSize(function (d) { From 30e8e2e2ae7982b7b8f0a1ef5972e24f01b5a0f3 Mon Sep 17 00:00:00 2001 From: Jay Zoldak Date: Mon, 13 May 2013 16:30:01 -0400 Subject: [PATCH 15/33] Update repo name in jenkins test shell script. --- jenkins/test.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/jenkins/test.sh b/jenkins/test.sh index 7475076086..a90cc8e806 100755 --- a/jenkins/test.sh +++ b/jenkins/test.sh @@ -4,7 +4,7 @@ set -e set -x function github_status { - gcli status create edx mitx $GIT_COMMIT \ + gcli status create edx edx-platform $GIT_COMMIT \ --params=$1 \ target_url:$BUILD_URL \ description:"Build #$BUILD_NUMBER $2" \ From a5bb31792bf234917671a6991d2f15b106676fc3 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 13 May 2013 16:38:52 -0400 Subject: [PATCH 16/33] Add a prefix string to Zendesk errors to be more searchable in Splunk --- common/djangoapps/util/views.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/common/djangoapps/util/views.py b/common/djangoapps/util/views.py index 1065140f5d..4eae1d66e5 100644 --- a/common/djangoapps/util/views.py +++ b/common/djangoapps/util/views.py @@ -161,7 +161,7 @@ def submit_feedback_via_zendesk(request): try: ticket_id = zendesk_api.create_ticket(new_ticket) except zendesk.ZendeskError as err: - log.error("%s", str(err)) + log.error("Error creating Zendesk ticket: %s", str(err)) return HttpResponse(status=500) # Additional information is provided as a private update so the information @@ -170,7 +170,7 @@ def submit_feedback_via_zendesk(request): try: zendesk_api.update_ticket(ticket_id, ticket_update) except zendesk.ZendeskError as err: - log.error("%s", str(err)) + log.error("Error updating Zendesk ticket: %s", str(err)) # The update is not strictly necessary, so do not indicate failure to the user pass From d376cb52ca2ed019b9bb1b971c3dced79b0d7964 Mon Sep 17 00:00:00 2001 From: David Baumgold Date: Mon, 13 May 2013 17:30:26 -0400 Subject: [PATCH 17/33] Forgot to update phantom-jasmine location for LMS --- lms/envs/jasmine.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lms/envs/jasmine.py b/lms/envs/jasmine.py index f3f20e7fbc..ba4fcc5261 100644 --- a/lms/envs/jasmine.py +++ b/lms/envs/jasmine.py @@ -33,6 +33,6 @@ PIPELINE_JS['spec'] = { JASMINE_TEST_DIRECTORY = PROJECT_ROOT + '/static/coffee' -STATICFILES_DIRS.append(COMMON_ROOT / 'test' / 'phantom-jasmine' / 'lib') +STATICFILES_DIRS.append(REPO_ROOT/'node_modules/phantom-jasmine/lib') INSTALLED_APPS += ('django_jasmine', ) From bd6bb8e9c1a35baecfa5d232ea5d13d6e5afd57f Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 14 May 2013 09:17:30 -0400 Subject: [PATCH 18/33] Fix advanced modules list --- cms/djangoapps/contentstore/views/component.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cms/djangoapps/contentstore/views/component.py b/cms/djangoapps/contentstore/views/component.py index 7d014448ba..5127effae6 100644 --- a/cms/djangoapps/contentstore/views/component.py +++ b/cms/djangoapps/contentstore/views/component.py @@ -41,7 +41,7 @@ log = logging.getLogger(__name__) COMPONENT_TYPES = ['customtag', 'discussion', 'html', 'problem', 'video'] OPEN_ENDED_COMPONENT_TYPES = ["combinedopenended", "peergrading"] -ADVANCED_COMPONENT_TYPES = ['annotatable' + 'word_cloud'] + OPEN_ENDED_COMPONENT_TYPES +ADVANCED_COMPONENT_TYPES = ['annotatable', 'word_cloud'] + OPEN_ENDED_COMPONENT_TYPES ADVANCED_COMPONENT_CATEGORY = 'advanced' ADVANCED_COMPONENT_POLICY_KEY = 'advanced_modules' From b590d344f5c6a92d7a9b8a433100c407c73dc3d1 Mon Sep 17 00:00:00 2001 From: Jay Zoldak Date: Tue, 14 May 2013 10:30:50 -0400 Subject: [PATCH 19/33] Specify version of phantom jasmine to use. --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 0f0eb7fef3..7fa287018a 100644 --- a/package.json +++ b/package.json @@ -3,6 +3,6 @@ "version": "0.1.0", "dependencies": { "coffee-script": "1.6.X", - "phantom-jasmine": "0.3.X" + "phantom-jasmine": "0.1.0" } } From c8970cafee417ca59d1914efefb6987171ed4cd9 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 14 May 2013 11:29:41 -0400 Subject: [PATCH 20/33] Add comment service request time logging to Datadog --- lms/lib/comment_client/utils.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/lms/lib/comment_client/utils.py b/lms/lib/comment_client/utils.py index 53bdd462ad..fce9516739 100644 --- a/lms/lib/comment_client/utils.py +++ b/lms/lib/comment_client/utils.py @@ -1,3 +1,4 @@ +from dogapi import dog_stats_api import json import logging import requests @@ -32,10 +33,11 @@ def perform_request(method, url, data_or_params=None, *args, **kwargs): data_or_params = {} data_or_params['api_key'] = settings.API_KEY try: - if method in ['post', 'put', 'patch']: - response = requests.request(method, url, data=data_or_params, timeout=5) - else: - response = requests.request(method, url, params=data_or_params, timeout=5) + with dog_stats_api.timer('comment_client.request.time'): + if method in ['post', 'put', 'patch']: + response = requests.request(method, url, data=data_or_params, timeout=5) + else: + response = requests.request(method, url, params=data_or_params, timeout=5) except Exception as err: # remove API key if it is in the params if 'api_key' in data_or_params: From 87cc4fab5ac6ee12edcfadac94b47e7e6371dcf0 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 14 May 2013 13:20:55 -0400 Subject: [PATCH 21/33] Add tags to comment service request Datadog timer --- lms/lib/comment_client/utils.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/lms/lib/comment_client/utils.py b/lms/lib/comment_client/utils.py index fce9516739..1ce03ed3c7 100644 --- a/lms/lib/comment_client/utils.py +++ b/lms/lib/comment_client/utils.py @@ -31,9 +31,14 @@ def merge_dict(dic1, dic2): def perform_request(method, url, data_or_params=None, *args, **kwargs): if data_or_params is None: data_or_params = {} + tags = [ + "{k}:{v}".format(k=k, v=v) + for (k, v) in data_or_params.items() + [("method", method), ("url", url)] + if k != 'api_key' + ] data_or_params['api_key'] = settings.API_KEY try: - with dog_stats_api.timer('comment_client.request.time'): + with dog_stats_api.timer('comment_client.request.time', tags=tags): if method in ['post', 'put', 'patch']: response = requests.request(method, url, data=data_or_params, timeout=5) else: From e9a8f408bffc1c64e192f8d39401d9c5f9697a6b Mon Sep 17 00:00:00 2001 From: David Baumgold Date: Tue, 14 May 2013 16:43:41 -0400 Subject: [PATCH 22/33] Add {reset: true} to all collection.fetch() calls This is to fix a bug with the Backbone 1.0 upgrade; some views were listening to reset events that were not longer getting fired --- cms/static/js/views/checklists_view.js | 1 + cms/static/js/views/course_info_edit.js | 43 +++++++++++-------- cms/static/js/views/settings/advanced_view.js | 1 + cms/templates/course_info.html | 8 ++-- cms/templates/settings.html | 18 ++++---- 5 files changed, 39 insertions(+), 32 deletions(-) diff --git a/cms/static/js/views/checklists_view.js b/cms/static/js/views/checklists_view.js index 1ae29f99d5..b2727e58f7 100644 --- a/cms/static/js/views/checklists_view.js +++ b/cms/static/js/views/checklists_view.js @@ -22,6 +22,7 @@ CMS.Views.Checklists = Backbone.View.extend({ } ); }, + reset: true, error: CMS.ServerError } ); diff --git a/cms/static/js/views/course_info_edit.js b/cms/static/js/views/course_info_edit.js index 50793c5f1e..9554b0d7c3 100644 --- a/cms/static/js/views/course_info_edit.js +++ b/cms/static/js/views/course_info_edit.js @@ -160,11 +160,17 @@ CMS.Views.ClassInfoUpdateView = Backbone.View.extend({ var targetModel = this.eventModel(event); this.modelDom(event).remove(); var cacheThis = this; - targetModel.destroy({success : function (model, response) { - cacheThis.collection.fetch({success : function() {cacheThis.render();}, - error : CMS.ServerError}); - }, - error : CMS.ServerError + targetModel.destroy({ + success: function (model, response) { + cacheThis.collection.fetch({ + success: function() { + cacheThis.render(); + }, + reset: true, + error: CMS.ServerError + }); + }, + error : CMS.ServerError }); }, @@ -238,20 +244,19 @@ CMS.Views.ClassInfoHandoutsView = Backbone.View.extend({ initialize: function() { var self = this; - this.model.fetch( - { - complete: function() { - window.templateLoader.loadRemoteTemplate("course_info_handouts", - "/static/client_templates/course_info_handouts.html", - function (raw_template) { - self.template = _.template(raw_template); - self.render(); - } - ); - }, - error : CMS.ServerError - } - ); + this.model.fetch({ + complete: function() { + window.templateLoader.loadRemoteTemplate("course_info_handouts", + "/static/client_templates/course_info_handouts.html", + function (raw_template) { + self.template = _.template(raw_template); + self.render(); + } + ); + }, + reset: true, + error: CMS.ServerError + }); }, render: function () { diff --git a/cms/static/js/views/settings/advanced_view.js b/cms/static/js/views/settings/advanced_view.js index c1392831b8..bef908601b 100644 --- a/cms/static/js/views/settings/advanced_view.js +++ b/cms/static/js/views/settings/advanced_view.js @@ -155,6 +155,7 @@ CMS.Views.Settings.Advanced = CMS.Views.ValidatingView.extend({ self.model.clear({silent : true}); self.model.fetch({ success : function() { self.render(); }, + reset: true, error : CMS.ServerError }); }, diff --git a/cms/templates/course_info.html b/cms/templates/course_info.html index cbf436ecc5..55042ee843 100644 --- a/cms/templates/course_info.html +++ b/cms/templates/course_info.html @@ -16,18 +16,18 @@ - + - + - \ No newline at end of file + diff --git a/cms/templates/index.html b/cms/templates/index.html index 0f6e982b1d..007033623d 100644 --- a/cms/templates/index.html +++ b/cms/templates/index.html @@ -37,9 +37,7 @@ <%block name="content">
-
-

${_("My Courses")}

-
+

${_("My Courses")}

% if user.is_active: