From 37ec01dd8dd9e4d6f3e3fc6fc573b64d945a5fa5 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 16 May 2013 10:05:13 -0400 Subject: [PATCH 01/37] Handle ItemNotFoundError from the modulestore to avoid 500 errors --- lms/djangoapps/courseware/module_render.py | 18 ++++++++++- .../courseware/tests/test_module_render.py | 31 +++++++++++++++---- 2 files changed, 42 insertions(+), 7 deletions(-) diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index 6f05b32778..c11fcbeeba 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/lms/djangoapps/courseware/module_render.py @@ -402,6 +402,11 @@ def modx_dispatch(request, dispatch, location, course_id): through the part before the first '?'. - location -- the module location. Used to look up the XModule instance - course_id -- defines the course context for this request. + + Raises PermissionDenied if the user is not logged in. Raises Http404 if + the location and course_id do not identify a valid module, the module is + not accessible by the user, or the module raises NotFoundError. If the + module raises any other error, it will escape this function. ''' # ''' (fix emacs broken parsing) @@ -430,8 +435,19 @@ def modx_dispatch(request, dispatch, location, course_id): return HttpResponse(json.dumps({'success': file_too_big_msg})) p[fileinput_id] = inputfiles + try: + descriptor = modulestore().get_instance(course_id, location) + except ItemNotFoundError: + log.warn( + "Invalid location for course id {course_id}: {location}".format( + course_id=course_id, + location=location + ) + ) + raise Http404 + model_data_cache = ModelDataCache.cache_for_descriptor_descendents(course_id, - request.user, modulestore().get_instance(course_id, location)) + request.user, descriptor) instance = get_module(request.user, request, location, model_data_cache, course_id, grade_bucket_type='ajax') if instance is None: diff --git a/lms/djangoapps/courseware/tests/test_module_render.py b/lms/djangoapps/courseware/tests/test_module_render.py index 0e4ba8ba5e..94ab4b7e94 100644 --- a/lms/djangoapps/courseware/tests/test_module_render.py +++ b/lms/djangoapps/courseware/tests/test_module_render.py @@ -69,19 +69,38 @@ class ModuleRenderTestCase(LoginEnrollmentTestCase): json.dumps({'success': 'Submission aborted! Your file "%s" is too large (max size: %d MB)' % (inputfile.name, settings.STUDENT_FILEUPLOAD_MAX_SIZE / (1000 ** 2))})) mock_request_3 = MagicMock() - mock_request_3.POST.copy.return_value = {} + mock_request_3.POST.copy.return_value = {'position': 1} mock_request_3.FILES = False mock_request_3.user = UserFactory() inputfile_2 = Stub() inputfile_2.size = 1 inputfile_2.name = 'name' - self.assertRaises(ItemNotFoundError, render.modx_dispatch, - mock_request_3, 'dummy', self.location, 'toy') - self.assertRaises(Http404, render.modx_dispatch, mock_request_3, 'dummy', - self.location, self.course_id) - mock_request_3.POST.copy.return_value = {'position': 1} self.assertIsInstance(render.modx_dispatch(mock_request_3, 'goto_position', self.location, self.course_id), HttpResponse) + self.assertRaises( + Http404, + render.modx_dispatch, + mock_request_3, + 'goto_position', + self.location, + 'bad_course_id' + ) + self.assertRaises( + Http404, + render.modx_dispatch, + mock_request_3, + 'goto_position', + ['i4x', 'edX', 'toy', 'chapter', 'bad_location'], + self.course_id + ) + self.assertRaises( + Http404, + render.modx_dispatch, + mock_request_3, + 'bad_dispatch', + self.location, + self.course_id + ) def test_get_score_bucket(self): self.assertEquals(render.get_score_bucket(0, 10), 'incorrect') From 9834f27eb9282da3eee03fbd654bcfaf82bb509e Mon Sep 17 00:00:00 2001 From: Ned Batchelder Date: Fri, 17 May 2013 18:15:40 -0400 Subject: [PATCH 02/37] Fingerprint the filenames in site-packages to decide when to install requirements. --- rakefiles/prereqs.rake | 28 +++++++++++++++++----------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/rakefiles/prereqs.rake b/rakefiles/prereqs.rake index ef4958e9d7..75c75f319b 100644 --- a/rakefiles/prereqs.rake +++ b/rakefiles/prereqs.rake @@ -1,5 +1,5 @@ require './rakefiles/helpers.rb' - +require 'tempfile' PREREQS_MD5_DIR = ENV["PREREQ_CACHE_DIR"] || File.join(REPO_ROOT, '.prereqs_cache') @@ -28,15 +28,21 @@ end desc "Install all python prerequisites for the lms and cms" task :install_python_prereqs => "ws:migrate" do - unchanged = 'Python requirements unchanged, nothing to install' - when_changed(unchanged, 'requirements/**/*') do - ENV['PIP_DOWNLOAD_CACHE'] ||= '.pip_download_cache' - sh('pip install --exists-action w -r requirements/edx/base.txt') - sh('pip install --exists-action w -r requirements/edx/post.txt') - # Check for private-requirements.txt: used to install our libs as working dirs, - # or personal-use tools. - if File.file?("requirements/private.txt") - sh('pip install -r requirements/private.txt') + if !ENV['NO_PREREQ_INSTALL'] + Tempfile.open('pyinstalled') do |pyinstalled| + # Read the names of everything in site-packages, and include them in the fingerprint. + sh("python -c 'import os; import distutils.sysconfig as dusc; print sorted(os.listdir(dusc.get_python_lib()))' > #{pyinstalled.path}") + unchanged = 'Python requirements unchanged, nothing to install' + when_changed(unchanged, 'requirements/**/*', pyinstalled.path) do + ENV['PIP_DOWNLOAD_CACHE'] ||= '.pip_download_cache' + sh('pip install --exists-action w -r requirements/edx/base.txt') + sh('pip install --exists-action w -r requirements/edx/post.txt') + # requirements/private.txt is used to install our libs as + # working dirs, or for personal-use tools. + if File.file?("requirements/private.txt") + sh('pip install -r requirements/private.txt') + end + end end - end unless ENV['NO_PREREQ_INSTALL'] + end end From 5b5fbc6b14d624e1175812b85180e1279c71cbcf Mon Sep 17 00:00:00 2001 From: Ned Batchelder Date: Fri, 17 May 2013 18:15:40 -0400 Subject: [PATCH 03/37] Fingerprint the filenames in site-packages to decide when to install requirements. --- rakefiles/helpers.rb | 2 +- rakefiles/prereqs.rake | 24 ++++++++++++++---------- 2 files changed, 15 insertions(+), 11 deletions(-) diff --git a/rakefiles/helpers.rb b/rakefiles/helpers.rb index 4c4d400b8a..eef93538eb 100644 --- a/rakefiles/helpers.rb +++ b/rakefiles/helpers.rb @@ -16,7 +16,7 @@ end def when_changed(unchanged_message, *files) Rake::Task[PREREQS_MD5_DIR].invoke - cache_file = File.join(PREREQS_MD5_DIR, files.join('-').gsub(/\W+/, '-')) + '.md5' + cache_file = File.join(PREREQS_MD5_DIR, files[0].gsub(/\W+/, '-').sub(/-+$/, '') + '.md5' digest = Digest::MD5.new() Dir[*files].select{|file| File.file?(file)}.each do |file| digest.file(file) diff --git a/rakefiles/prereqs.rake b/rakefiles/prereqs.rake index ef4958e9d7..782bbb9d99 100644 --- a/rakefiles/prereqs.rake +++ b/rakefiles/prereqs.rake @@ -1,5 +1,5 @@ require './rakefiles/helpers.rb' - +require 'tempfile' PREREQS_MD5_DIR = ENV["PREREQ_CACHE_DIR"] || File.join(REPO_ROOT, '.prereqs_cache') @@ -28,15 +28,19 @@ end desc "Install all python prerequisites for the lms and cms" task :install_python_prereqs => "ws:migrate" do - unchanged = 'Python requirements unchanged, nothing to install' - when_changed(unchanged, 'requirements/**/*') do - ENV['PIP_DOWNLOAD_CACHE'] ||= '.pip_download_cache' - sh('pip install --exists-action w -r requirements/edx/base.txt') - sh('pip install --exists-action w -r requirements/edx/post.txt') - # Check for private-requirements.txt: used to install our libs as working dirs, - # or personal-use tools. - if File.file?("requirements/private.txt") - sh('pip install -r requirements/private.txt') + Tempfile.open('pyinstalled') do |pyinstalled| + # Read the names of everything in site-packages, and include them in the fingerprint. + sh("python -c 'import os; import distutils.sysconfig as dusc; print sorted(os.listdir(dusc.get_python_lib()))' > #{pyinstalled.path}") + unchanged = 'Python requirements unchanged, nothing to install' + when_changed(unchanged, 'requirements/**/*', pyinstalled.path) do + ENV['PIP_DOWNLOAD_CACHE'] ||= '.pip_download_cache' + sh('pip install --exists-action w -r requirements/edx/base.txt') + sh('pip install --exists-action w -r requirements/edx/post.txt') + # requirements/private.txt is used to install our libs as + # working dirs, or for personal-use tools. + if File.file?("requirements/private.txt") + sh('pip install -r requirements/private.txt') + end end end unless ENV['NO_PREREQ_INSTALL'] end From 23384ca2eb0dd23091b66c315b68d716c68d9eb2 Mon Sep 17 00:00:00 2001 From: Ned Batchelder Date: Mon, 20 May 2013 12:09:28 -0400 Subject: [PATCH 04/37] Use backticks so that the command won't echo to the user. --- rakefiles/prereqs.rake | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rakefiles/prereqs.rake b/rakefiles/prereqs.rake index 782bbb9d99..7cb509569f 100644 --- a/rakefiles/prereqs.rake +++ b/rakefiles/prereqs.rake @@ -30,7 +30,7 @@ desc "Install all python prerequisites for the lms and cms" task :install_python_prereqs => "ws:migrate" do Tempfile.open('pyinstalled') do |pyinstalled| # Read the names of everything in site-packages, and include them in the fingerprint. - sh("python -c 'import os; import distutils.sysconfig as dusc; print sorted(os.listdir(dusc.get_python_lib()))' > #{pyinstalled.path}") + `python -c 'import os; import distutils.sysconfig as dusc; print sorted(os.listdir(dusc.get_python_lib()))' > #{pyinstalled.path}` unchanged = 'Python requirements unchanged, nothing to install' when_changed(unchanged, 'requirements/**/*', pyinstalled.path) do ENV['PIP_DOWNLOAD_CACHE'] ||= '.pip_download_cache' From c947133e54d62d46d1985c61cee8f709e12f1d1c Mon Sep 17 00:00:00 2001 From: Ned Batchelder Date: Mon, 20 May 2013 14:39:25 -0400 Subject: [PATCH 05/37] A better way to compute the hash, that gets the right hash after an install actually installs something. --- rakefiles/helpers.rb | 25 ++++++++++++++++++++----- rakefiles/prereqs.rake | 28 ++++++++++++---------------- 2 files changed, 32 insertions(+), 21 deletions(-) diff --git a/rakefiles/helpers.rb b/rakefiles/helpers.rb index c023481f54..f344aa2042 100644 --- a/rakefiles/helpers.rb +++ b/rakefiles/helpers.rb @@ -14,16 +14,31 @@ def report_dir_path(dir) return File.join(REPORT_DIR, dir.to_s) end -def when_changed(unchanged_message, *files) - Rake::Task[PREREQS_MD5_DIR].invoke - cache_file = File.join(PREREQS_MD5_DIR, files[0].gsub(/\W+/, '-').sub(/-+$/, '')) + '.md5' +def compute_fingerprint(files, dirs) digest = Digest::MD5.new() + + # Digest the contents of all the files. Dir[*files].select{|file| File.file?(file)}.each do |file| digest.file(file) end - if !File.exists?(cache_file) or digest.hexdigest != File.read(cache_file) + + # Digest the names of the files in all the dirs. + dirs.each do |dir| + file_names = Dir.entries(dir).sort.join(" ") + digest.update(file_names) + end + + digest.hexdigest +end + +# Hash the contents of all the files, and the names of files in the dirs. +# Run the block if they've changed. +def when_changed(unchanged_message, files, dirs=[]) + Rake::Task[PREREQS_MD5_DIR].invoke + cache_file = File.join(PREREQS_MD5_DIR, files[0].gsub(/\W+/, '-').sub(/-+$/, '')) + '.md5' + if !File.exists?(cache_file) or compute_fingerprint(files, dirs) != File.read(cache_file) yield - File.write(cache_file, digest.hexdigest) + File.write(cache_file, compute_fingerprint(files, dirs)) elsif !unchanged_message.empty? puts unchanged_message end diff --git a/rakefiles/prereqs.rake b/rakefiles/prereqs.rake index 7cb509569f..f453372065 100644 --- a/rakefiles/prereqs.rake +++ b/rakefiles/prereqs.rake @@ -1,5 +1,4 @@ require './rakefiles/helpers.rb' -require 'tempfile' PREREQS_MD5_DIR = ENV["PREREQ_CACHE_DIR"] || File.join(REPO_ROOT, '.prereqs_cache') @@ -13,7 +12,7 @@ task :install_prereqs => [:install_node_prereqs, :install_ruby_prereqs, :install desc "Install all node prerequisites for the lms and cms" task :install_node_prereqs => "ws:migrate" do unchanged = 'Node requirements unchanged, nothing to install' - when_changed(unchanged, 'package.json') do + when_changed(unchanged, ['package.json']) do sh('npm install') end unless ENV['NO_PREREQ_INSTALL'] end @@ -21,26 +20,23 @@ end desc "Install all ruby prerequisites for the lms and cms" task :install_ruby_prereqs => "ws:migrate" do unchanged = 'Ruby requirements unchanged, nothing to install' - when_changed(unchanged, 'Gemfile') do + when_changed(unchanged, ['Gemfile']) do sh('bundle install') end unless ENV['NO_PREREQ_INSTALL'] end desc "Install all python prerequisites for the lms and cms" task :install_python_prereqs => "ws:migrate" do - Tempfile.open('pyinstalled') do |pyinstalled| - # Read the names of everything in site-packages, and include them in the fingerprint. - `python -c 'import os; import distutils.sysconfig as dusc; print sorted(os.listdir(dusc.get_python_lib()))' > #{pyinstalled.path}` - unchanged = 'Python requirements unchanged, nothing to install' - when_changed(unchanged, 'requirements/**/*', pyinstalled.path) do - ENV['PIP_DOWNLOAD_CACHE'] ||= '.pip_download_cache' - sh('pip install --exists-action w -r requirements/edx/base.txt') - sh('pip install --exists-action w -r requirements/edx/post.txt') - # requirements/private.txt is used to install our libs as - # working dirs, or for personal-use tools. - if File.file?("requirements/private.txt") - sh('pip install -r requirements/private.txt') - end + site_packages_dir = `python -c 'import os; import distutils.sysconfig as dusc; print dusc.get_python_lib()'`.chomp + unchanged = 'Python requirements unchanged, nothing to install' + when_changed(unchanged, ['requirements/**/*'], [site_packages_dir]) do + ENV['PIP_DOWNLOAD_CACHE'] ||= '.pip_download_cache' + sh('pip install --exists-action w -r requirements/edx/base.txt') + sh('pip install --exists-action w -r requirements/edx/post.txt') + # requirements/private.txt is used to install our libs as + # working dirs, or for personal-use tools. + if File.file?("requirements/private.txt") + sh('pip install -r requirements/private.txt') end end unless ENV['NO_PREREQ_INSTALL'] end From a7b02e002934a6947383e4c3f080c7d3bb75813e Mon Sep 17 00:00:00 2001 From: e0d Date: Mon, 20 May 2013 14:40:05 -0400 Subject: [PATCH 06/37] actually needs to be 255 given unicode requirements --- lms/djangoapps/notes/migrations/0001_initial.py | 4 ++-- lms/djangoapps/notes/models.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lms/djangoapps/notes/migrations/0001_initial.py b/lms/djangoapps/notes/migrations/0001_initial.py index 0372a889df..1629b2355d 100644 --- a/lms/djangoapps/notes/migrations/0001_initial.py +++ b/lms/djangoapps/notes/migrations/0001_initial.py @@ -13,7 +13,7 @@ class Migration(SchemaMigration): ('id', self.gf('django.db.models.fields.AutoField')(primary_key=True)), ('user', self.gf('django.db.models.fields.related.ForeignKey')(to=orm['auth.User'])), ('course_id', self.gf('django.db.models.fields.CharField')(max_length=255, db_index=True)), - ('uri', self.gf('django.db.models.fields.CharField')(max_length=512, db_index=True)), + ('uri', self.gf('django.db.models.fields.CharField')(max_length=255, db_index=True)), ('text', self.gf('django.db.models.fields.TextField')(default='')), ('quote', self.gf('django.db.models.fields.TextField')(default='')), ('range_start', self.gf('django.db.models.fields.CharField')(max_length=2048)), @@ -82,7 +82,7 @@ class Migration(SchemaMigration): 'tags': ('django.db.models.fields.TextField', [], {'default': "''"}), 'text': ('django.db.models.fields.TextField', [], {'default': "''"}), 'updated': ('django.db.models.fields.DateTimeField', [], {'auto_now': 'True', 'db_index': 'True', 'blank': 'True'}), - 'uri': ('django.db.models.fields.CharField', [], {'max_length': '512', 'db_index': 'True'}), + 'uri': ('django.db.models.fields.CharField', [], {'max_length': '255', 'db_index': 'True'}), 'user': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['auth.User']"}) } } diff --git a/lms/djangoapps/notes/models.py b/lms/djangoapps/notes/models.py index 4050516f13..aa2ec7a377 100644 --- a/lms/djangoapps/notes/models.py +++ b/lms/djangoapps/notes/models.py @@ -9,7 +9,7 @@ import json class Note(models.Model): user = models.ForeignKey(User, db_index=True) course_id = models.CharField(max_length=255, db_index=True) - uri = models.CharField(max_length=512, db_index=True) + uri = models.CharField(max_length=255, db_index=True) text = models.TextField(default="") quote = models.TextField(default="") range_start = models.CharField(max_length=2048) # xpath string From 31b5c4ab614a9129474173ec255b41d26959466e Mon Sep 17 00:00:00 2001 From: Brian Talbot Date: Mon, 20 May 2013 14:02:00 -0400 Subject: [PATCH 07/37] studio - adds back in older lh() Sass function/mixin and moves it to the inheritied/to-be-sunsetted mixins sheet --- cms/static/sass/_mixins-inherited.scss | 6 ++++++ common/static/sass/_mixins.scss | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/cms/static/sass/_mixins-inherited.scss b/cms/static/sass/_mixins-inherited.scss index a766919c74..c0d9df77ce 100644 --- a/cms/static/sass/_mixins-inherited.scss +++ b/cms/static/sass/_mixins-inherited.scss @@ -5,6 +5,12 @@ // talbs: we need to slowly ween ourselves off of these // ==================== + +// line-height (old way) +@function lh($amount: 1) { + @return $body-line-height * $amount; +} + // inherited - vertical and horizontal centering @mixin vertically-and-horizontally-centered ($height, $width) { left: 50%; diff --git a/common/static/sass/_mixins.scss b/common/static/sass/_mixins.scss index 6e1a34aaaa..c3a548bbf7 100644 --- a/common/static/sass/_mixins.scss +++ b/common/static/sass/_mixins.scss @@ -8,7 +8,7 @@ } // mixins - line height -@mixin lh($fontSize: auto){ +@mixin line-height($fontSize: auto){ line-height: ($fontSize*1.48) + px; line-height: (($fontSize/10)*1.48) + rem; } From 8bbbe992fe795a110d8da032d17c792013a9ad45 Mon Sep 17 00:00:00 2001 From: Brian Talbot Date: Mon, 20 May 2013 14:09:57 -0400 Subject: [PATCH 08/37] studio - references new line-height() Sass mixin when declaring typography archetypes --- cms/static/sass/elements/_typography.scss | 36 +++++++++++------------ 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/cms/static/sass/elements/_typography.scss b/cms/static/sass/elements/_typography.scss index 9e9ca69652..69e78750c0 100644 --- a/cms/static/sass/elements/_typography.scss +++ b/cms/static/sass/elements/_typography.scss @@ -11,54 +11,54 @@ .t-title1 { @extend .t-title; @include font-size(60); - @include lh(60); + @include line-height(60); } .t-title2 { @extend .t-title; @include font-size(48); - @include lh(48); + @include line-height(48); } .t-title3 { @include font-size(36); - @include lh(36); + @include line-height(36); } .t-title4 { @extend .t-title; @include font-size(24); - @include lh(24); + @include line-height(24); } .t-title5 { @extend .t-title; @include font-size(18); - @include lh(18); + @include line-height(18); } .t-title6 { @extend .t-title; @include font-size(16); - @include lh(16); + @include line-height(16); } .t-title7 { @extend .t-title; @include font-size(14); - @include lh(14); + @include line-height(14); } .t-title8 { @extend .t-title; @include font-size(12); - @include lh(12); + @include line-height(12); } .t-title9 { @extend .t-title; @include font-size(11); - @include lh(11); + @include line-height(11); } // ==================== @@ -71,31 +71,31 @@ .t-copy-base { @extend .t-copy; @include font-size(16); - @include lh(16); + @include line-height(16); } .t-copy-lead1 { @extend .t-copy; @include font-size(18); - @include lh(18); + @include line-height(18); } .t-copy-lead2 { @extend .t-copy; @include font-size(24); - @include lh(24); + @include line-height(24); } .t-copy-sub1 { @extend .t-copy; @include font-size(14); - @include lh(14); + @include line-height(14); } .t-copy-sub2 { @extend .t-copy; @include font-size(12); - @include lh(12); + @include line-height(12); } // ==================== @@ -103,22 +103,22 @@ // actions/labels .t-action1 { @include font-size(18); - @include lh(18); + @include line-height(18); } .t-action2 { @include font-size(16); - @include lh(16); + @include line-height(16); } .t-action3 { @include font-size(14); - @include lh(14); + @include line-height(14); } .t-action4 { @include font-size(12); - @include lh(12); + @include line-height(12); } From 241ef68f4faec1c86d721a2b15c5e6271a1f5c14 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 15 May 2013 09:44:47 -0400 Subject: [PATCH 09/37] Start linting envs files, add task to only pylint errors, and move pylintrc file to the correct location --- cms/envs/acceptance.py | 5 ++++ cms/envs/aws.py | 5 ++++ cms/envs/common.py | 4 ++++ cms/envs/dev.py | 4 ++++ cms/envs/dev_ike.py | 4 ++++ cms/envs/jasmine.py | 4 ++++ cms/envs/test.py | 5 ++++ lms/envs/acceptance.py | 5 ++++ lms/envs/aws.py | 5 ++++ lms/envs/cms/acceptance.py | 5 ++++ lms/envs/cms/aws.py | 4 ++++ lms/envs/cms/dev.py | 4 ++++ lms/envs/cms/preview_dev.py | 4 ++++ lms/envs/common.py | 5 ++++ lms/envs/content.py | 5 ++++ lms/envs/dev.py | 5 ++++ lms/envs/dev_edx4edx.py | 4 ++++ lms/envs/dev_ike.py | 5 ++++ lms/envs/dev_int.py | 5 ++++ lms/envs/dev_mongo.py | 5 ++++ lms/envs/devgroups/courses.py | 5 ++++ lms/envs/devgroups/h_cs50.py | 5 ++++ lms/envs/devgroups/m_6002.py | 5 ++++ lms/envs/devgroups/portal.py | 5 ++++ lms/envs/devplus.py | 5 ++++ lms/envs/discussionsettings.py | 4 ++++ lms/envs/edx4edx_aws.py | 4 ++++ lms/envs/jasmine.py | 4 ++++ lms/envs/static.py | 5 ++++ lms/envs/test.py | 5 ++++ lms/envs/test_ike.py | 5 ++++ .pylintrc => pylintrc | 0 rakefiles/quality.rake | 42 ++++++++++++++++++++++------------ 33 files changed, 172 insertions(+), 14 deletions(-) rename .pylintrc => pylintrc (100%) diff --git a/cms/envs/acceptance.py b/cms/envs/acceptance.py index f4b867d3c6..97bb0011f2 100644 --- a/cms/envs/acceptance.py +++ b/cms/envs/acceptance.py @@ -2,6 +2,11 @@ This config file extends the test environment configuration so that we can run the lettuce acceptance tests. """ + +# We intentionally define lots of variables that aren't used, and +# want to import all variables from base settings files +# pylint: disable=W0401, W0614 + from .test import * # You need to start the server in debug mode, diff --git a/cms/envs/aws.py b/cms/envs/aws.py index 3cd70826da..9fabb3b9e8 100644 --- a/cms/envs/aws.py +++ b/cms/envs/aws.py @@ -1,6 +1,11 @@ """ This is the default template for our main set of AWS servers. """ + +# We intentionally define lots of variables that aren't used, and +# want to import all variables from base settings files +# pylint: disable=W0401, W0614 + import json from .common import * diff --git a/cms/envs/common.py b/cms/envs/common.py index e150374cef..038c00ddbb 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -19,6 +19,10 @@ Longer TODO: multiple sites, but we do need a way to map their data assets. """ +# We intentionally define lots of variables that aren't used, and +# want to import all variables from base settings files +# pylint: disable=W0401, W0614 + import sys import lms.envs.common from path import path diff --git a/cms/envs/dev.py b/cms/envs/dev.py index f3080c356f..203e4bd909 100644 --- a/cms/envs/dev.py +++ b/cms/envs/dev.py @@ -1,6 +1,10 @@ """ This config file runs the simplest dev environment""" +# We intentionally define lots of variables that aren't used, and +# want to import all variables from base settings files +# pylint: disable=W0401, W0614 + from .common import * from logsettings import get_logger_config diff --git a/cms/envs/dev_ike.py b/cms/envs/dev_ike.py index 1ebf219d44..0c798b68aa 100644 --- a/cms/envs/dev_ike.py +++ b/cms/envs/dev_ike.py @@ -1,3 +1,7 @@ +# We intentionally define lots of variables that aren't used, and +# want to import all variables from base settings files +# pylint: disable=W0401, W0614 + # dev environment for ichuang/mit # FORCE_SCRIPT_NAME = '/cms' diff --git a/cms/envs/jasmine.py b/cms/envs/jasmine.py index 6c7cbcdcb0..f3a982aa43 100644 --- a/cms/envs/jasmine.py +++ b/cms/envs/jasmine.py @@ -2,6 +2,10 @@ This configuration is used for running jasmine tests """ +# We intentionally define lots of variables that aren't used, and +# want to import all variables from base settings files +# pylint: disable=W0401, W0614 + from .test import * from logsettings import get_logger_config diff --git a/cms/envs/test.py b/cms/envs/test.py index 4cb975e2fb..6d78b0d7d6 100644 --- a/cms/envs/test.py +++ b/cms/envs/test.py @@ -7,6 +7,11 @@ sessions. Assumes structure: /mitx # The location of this repo /log # Where we're going to write log files """ + +# We intentionally define lots of variables that aren't used, and +# want to import all variables from base settings files +# pylint: disable=W0401, W0614 + from .common import * import os from path import path diff --git a/lms/envs/acceptance.py b/lms/envs/acceptance.py index 611c3fdac8..3b87bb4326 100644 --- a/lms/envs/acceptance.py +++ b/lms/envs/acceptance.py @@ -2,6 +2,11 @@ This config file extends the test environment configuration so that we can run the lettuce acceptance tests. """ + +# We intentionally define lots of variables that aren't used, and +# want to import all variables from base settings files +# pylint: disable=W0401, W0614 + from .test import * # You need to start the server in debug mode, diff --git a/lms/envs/aws.py b/lms/envs/aws.py index bec2671d5e..df80d5a924 100644 --- a/lms/envs/aws.py +++ b/lms/envs/aws.py @@ -6,6 +6,11 @@ Common traits: * Use memcached, and cache-backed sessions * Use a MySQL 5.1 database """ + +# We intentionally define lots of variables that aren't used, and +# want to import all variables from base settings files +# pylint: disable=W0401, W0614 + import json from .common import * diff --git a/lms/envs/cms/acceptance.py b/lms/envs/cms/acceptance.py index e5ee2937f4..0b638dca8a 100644 --- a/lms/envs/cms/acceptance.py +++ b/lms/envs/cms/acceptance.py @@ -3,6 +3,11 @@ This config file is a copy of dev environment without the Debug Toolbar. I it suitable to run against acceptance tests. """ + +# We intentionally define lots of variables that aren't used, and +# want to import all variables from base settings files +# pylint: disable=W0401, W0614 + from .dev import * # REMOVE DEBUG TOOLBAR diff --git a/lms/envs/cms/aws.py b/lms/envs/cms/aws.py index a0e2f25d83..baeaebca1c 100644 --- a/lms/envs/cms/aws.py +++ b/lms/envs/cms/aws.py @@ -2,6 +2,10 @@ Settings for the LMS that runs alongside the CMS on AWS """ +# We intentionally define lots of variables that aren't used, and +# want to import all variables from base settings files +# pylint: disable=W0401, W0614 + from ..aws import * with open(ENV_ROOT / "cms.auth.json") as auth_file: diff --git a/lms/envs/cms/dev.py b/lms/envs/cms/dev.py index 9333b7883c..e55c6d61b5 100644 --- a/lms/envs/cms/dev.py +++ b/lms/envs/cms/dev.py @@ -2,6 +2,10 @@ Settings for the LMS that runs alongside the CMS on AWS """ +# We intentionally define lots of variables that aren't used, and +# want to import all variables from base settings files +# pylint: disable=W0401, W0614 + from ..dev import * MITX_FEATURES['AUTH_USE_MIT_CERTIFICATES'] = False diff --git a/lms/envs/cms/preview_dev.py b/lms/envs/cms/preview_dev.py index 463af34624..1cfaec6159 100644 --- a/lms/envs/cms/preview_dev.py +++ b/lms/envs/cms/preview_dev.py @@ -2,6 +2,10 @@ Settings for the LMS that runs alongside the CMS on AWS """ +# We intentionally define lots of variables that aren't used, and +# want to import all variables from base settings files +# pylint: disable=W0401, W0614 + from .dev import * MODULESTORE = { diff --git a/lms/envs/common.py b/lms/envs/common.py index a198f010c6..3c133a1e6b 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -18,6 +18,11 @@ Longer TODO: 3. We need to handle configuration for multiple courses. This could be as multiple sites, but we do need a way to map their data assets. """ + +# We intentionally define lots of variables that aren't used, and +# want to import all variables from base settings files +# pylint: disable=W0401, W0614 + import sys import os diff --git a/lms/envs/content.py b/lms/envs/content.py index f699153895..f85ae0b9cd 100644 --- a/lms/envs/content.py +++ b/lms/envs/content.py @@ -2,6 +2,11 @@ These are debug machines used for content creators, so they're kind of a cross between dev machines and AWS machines. """ + +# We intentionally define lots of variables that aren't used, and +# want to import all variables from base settings files +# pylint: disable=W0401, W0614 + from .aws import * DEBUG = True diff --git a/lms/envs/dev.py b/lms/envs/dev.py index 488110655e..9d7c0b3ac2 100644 --- a/lms/envs/dev.py +++ b/lms/envs/dev.py @@ -7,6 +7,11 @@ sessions. Assumes structure: /mitx # The location of this repo /log # Where we're going to write log files """ + +# We intentionally define lots of variables that aren't used, and +# want to import all variables from base settings files +# pylint: disable=W0401, W0614 + from .common import * from logsettings import get_logger_config diff --git a/lms/envs/dev_edx4edx.py b/lms/envs/dev_edx4edx.py index 2ebd24e68b..c90f369bc6 100644 --- a/lms/envs/dev_edx4edx.py +++ b/lms/envs/dev_edx4edx.py @@ -8,6 +8,10 @@ sessions. Assumes structure: /log # Where we're going to write log files """ +# We intentionally define lots of variables that aren't used, and +# want to import all variables from base settings files +# pylint: disable=W0401, W0614 + import socket if 'eecs1' in socket.gethostname(): diff --git a/lms/envs/dev_ike.py b/lms/envs/dev_ike.py index 639d186989..3f54b11d1e 100644 --- a/lms/envs/dev_ike.py +++ b/lms/envs/dev_ike.py @@ -7,6 +7,11 @@ sessions. Assumes structure: /mitx # The location of this repo /log # Where we're going to write log files """ + +# We intentionally define lots of variables that aren't used, and +# want to import all variables from base settings files +# pylint: disable=W0401, W0614 + from .common import * from logsettings import get_logger_config from .dev import * diff --git a/lms/envs/dev_int.py b/lms/envs/dev_int.py index 21c52c8abc..34921205a6 100644 --- a/lms/envs/dev_int.py +++ b/lms/envs/dev_int.py @@ -9,6 +9,11 @@ following domains to 127.0.0.1 in your /etc/hosts file: Note that OS X has a bug where using *.local domains is excruciatingly slow, so use *.dev domains instead for local testing. """ + +# We intentionally define lots of variables that aren't used, and +# want to import all variables from base settings files +# pylint: disable=W0401, W0614 + from .dev import * MITX_FEATURES['SUBDOMAIN_COURSE_LISTINGS'] = True diff --git a/lms/envs/dev_mongo.py b/lms/envs/dev_mongo.py index 6af0a429bb..dfbf473b45 100644 --- a/lms/envs/dev_mongo.py +++ b/lms/envs/dev_mongo.py @@ -1,6 +1,11 @@ """ This config file runs the dev environment, but with mongo as the datastore """ + +# We intentionally define lots of variables that aren't used, and +# want to import all variables from base settings files +# pylint: disable=W0401, W0614 + from .dev import * GITHUB_REPO_ROOT = ENV_ROOT / "data" diff --git a/lms/envs/devgroups/courses.py b/lms/envs/devgroups/courses.py index c44717c451..1a7ff58f08 100644 --- a/lms/envs/devgroups/courses.py +++ b/lms/envs/devgroups/courses.py @@ -1,3 +1,8 @@ + +# We intentionally define lots of variables that aren't used, and +# want to import all variables from base settings files +# pylint: disable=W0401, W0614 + from ..dev import * CLASSES_TO_DBS = { diff --git a/lms/envs/devgroups/h_cs50.py b/lms/envs/devgroups/h_cs50.py index 9643c33d35..21c959f5ce 100644 --- a/lms/envs/devgroups/h_cs50.py +++ b/lms/envs/devgroups/h_cs50.py @@ -1,3 +1,8 @@ + +# We intentionally define lots of variables that aren't used, and +# want to import all variables from base settings files +# pylint: disable=W0401, W0614 + from .courses import * DATABASES = course_db_for('HarvardX/CS50x/2012') diff --git a/lms/envs/devgroups/m_6002.py b/lms/envs/devgroups/m_6002.py index 411e2bcc3c..d3c10fcd04 100644 --- a/lms/envs/devgroups/m_6002.py +++ b/lms/envs/devgroups/m_6002.py @@ -1,3 +1,8 @@ + +# We intentionally define lots of variables that aren't used, and +# want to import all variables from base settings files +# pylint: disable=W0401, W0614 + from .courses import * DATABASES = course_db_for('MITx/6.002x/2012_Fall') diff --git a/lms/envs/devgroups/portal.py b/lms/envs/devgroups/portal.py index 35808d56fa..8e4635cc66 100644 --- a/lms/envs/devgroups/portal.py +++ b/lms/envs/devgroups/portal.py @@ -2,6 +2,11 @@ Note that for this to work at all, you must have memcached running (or you won't get shared sessions) """ + +# We intentionally define lots of variables that aren't used, and +# want to import all variables from base settings files +# pylint: disable=W0401, W0614 + from courses import * # Move this to a shared file later: diff --git a/lms/envs/devplus.py b/lms/envs/devplus.py index ea6590291c..bfd0788165 100644 --- a/lms/envs/devplus.py +++ b/lms/envs/devplus.py @@ -13,6 +13,11 @@ Dir structure: /log # Where we're going to write log files """ + +# We intentionally define lots of variables that aren't used, and +# want to import all variables from base settings files +# pylint: disable=W0401, W0614 + from .dev import * WIKI_ENABLED = True diff --git a/lms/envs/discussionsettings.py b/lms/envs/discussionsettings.py index f13680a7fe..1ac4c23af8 100644 --- a/lms/envs/discussionsettings.py +++ b/lms/envs/discussionsettings.py @@ -1 +1,5 @@ + +# We intentionally define variables that aren't used +# pylint: disable=W0614 + DISCUSSION_ALLOWED_UPLOAD_FILE_TYPES = ('.jpg', '.jpeg', '.gif', '.bmp', '.png', '.tiff') diff --git a/lms/envs/edx4edx_aws.py b/lms/envs/edx4edx_aws.py index b82048824f..247fa866bc 100644 --- a/lms/envs/edx4edx_aws.py +++ b/lms/envs/edx4edx_aws.py @@ -1,3 +1,7 @@ +# We intentionally define lots of variables that aren't used, and +# want to import all variables from base settings files +# pylint: disable=W0401, W0614 + # Settings for edx4edx production instance from .aws import * COURSE_NAME = "edx4edx" diff --git a/lms/envs/jasmine.py b/lms/envs/jasmine.py index ba4fcc5261..2c30bc7de7 100644 --- a/lms/envs/jasmine.py +++ b/lms/envs/jasmine.py @@ -2,6 +2,10 @@ This configuration is used for running jasmine tests """ +# We intentionally define lots of variables that aren't used, and +# want to import all variables from base settings files +# pylint: disable=W0401, W0614 + from .test import * from logsettings import get_logger_config diff --git a/lms/envs/static.py b/lms/envs/static.py index 23e735c747..260153e623 100644 --- a/lms/envs/static.py +++ b/lms/envs/static.py @@ -7,6 +7,11 @@ sessions. Assumes structure: /mitx # The location of this repo /log # Where we're going to write log files """ + +# We intentionally define lots of variables that aren't used, and +# want to import all variables from base settings files +# pylint: disable=W0401, W0614 + from .common import * from logsettings import get_logger_config diff --git a/lms/envs/test.py b/lms/envs/test.py index b8782ccd75..6691d50106 100644 --- a/lms/envs/test.py +++ b/lms/envs/test.py @@ -7,6 +7,11 @@ sessions. Assumes structure: /mitx # The location of this repo /log # Where we're going to write log files """ + +# We intentionally define lots of variables that aren't used, and +# want to import all variables from base settings files +# pylint: disable=W0401, W0614 + from .common import * import os from path import path diff --git a/lms/envs/test_ike.py b/lms/envs/test_ike.py index 907b7eeadf..46f7df211c 100644 --- a/lms/envs/test_ike.py +++ b/lms/envs/test_ike.py @@ -7,6 +7,11 @@ sessions. Assumes structure: /mitx # The location of this repo /log # Where we're going to write log files """ + +# We intentionally define lots of variables that aren't used, and +# want to import all variables from base settings files +# pylint: disable=W0401, W0614 + from .common import * from logsettings import get_logger_config import os diff --git a/.pylintrc b/pylintrc similarity index 100% rename from .pylintrc rename to pylintrc diff --git a/rakefiles/quality.rake b/rakefiles/quality.rake index 00ce627ac5..927f765eb5 100644 --- a/rakefiles/quality.rake +++ b/rakefiles/quality.rake @@ -1,3 +1,20 @@ +def run_pylint(system, report_dir, flags='') + apps = Dir["#{system}", "#{system}/djangoapps/*", "#{system}/lib/*"].map do |app| + File.basename(app) + end.select do |app| + app !=~ /.pyc$/ + end.map do |app| + if app =~ /.py$/ + app.gsub('.py', '') + else + app + end + end + + pythonpath_prefix = "PYTHONPATH=#{system}:#{system}/djangoapps:#{system}/lib:common/djangoapps:common/lib" + sh("#{pythonpath_prefix} pylint #{flags} -f parseable #{apps.join(' ')} | tee #{report_dir}/pylint.report") +end + [:lms, :cms, :common].each do |system| report_dir = report_dir_path(system) @@ -11,21 +28,18 @@ desc "Run pylint on all #{system} code" task "pylint_#{system}" => [report_dir, :install_python_prereqs] do - apps = Dir["#{system}/*.py", "#{system}/djangoapps/*", "#{system}/lib/*"].map do |app| - File.basename(app) - end.select do |app| - app !=~ /.pyc$/ - end.map do |app| - if app =~ /.py$/ - app.gsub('.py', '') - else - app - end - end - - pythonpath_prefix = "PYTHONPATH=#{system}:#{system}/djangoapps:#{system}/lib:common/djangoapps:common/lib" - sh("#{pythonpath_prefix} pylint --rcfile=.pylintrc -f parseable #{apps.join(' ')} | tee #{report_dir}/pylint.report") + run_pylint(system, report_dir) end + namespace "pylint_#{system}" do + desc "Run pylint checking for errors only, and aborting if there are any" + task :errors do + run_pylint(system, report_dir, '-E') + end + end + namespace :pylint do + task :errors => "pylint_#{system}:errors" + end + task :pylint => "pylint_#{system}" end \ No newline at end of file From d5d99ef60fe150199bcfee26353d4a7365662336 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 15 May 2013 10:02:08 -0400 Subject: [PATCH 10/37] Switch to official pylint 0.28 release --- requirements/edx/base.txt | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index ef0209ee03..3d8b95f8e2 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -74,6 +74,7 @@ lettuce==0.2.16 mock==0.8.0 nosexcover==1.0.7 pep8==1.4.5 +pylint==0.28 rednose==0.3 selenium==2.31.0 splinter==0.5.0 @@ -81,7 +82,3 @@ django_nose==1.1 django-jasmine==0.3.2 django_debug_toolbar django-debug-toolbar-mongo - -# Install pylint from a specific commit on trunk -# to get the fix for this issue: http://www.logilab.org/ticket/122793 -https://bitbucket.org/logilab/pylint/get/e828cb5.zip From 0b92e0b91b4f2e89f37adefdb717d727efe28f06 Mon Sep 17 00:00:00 2001 From: cahrens Date: Tue, 21 May 2013 09:49:28 -0400 Subject: [PATCH 11/37] Upgrade version of xblock. --- 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 d3f90d5abc..6b28d3edd9 100644 --- a/requirements/edx/github.txt +++ b/requirements/edx/github.txt @@ -8,5 +8,5 @@ -e git://github.com/eventbrite/zendesk.git@d53fe0e81b623f084e91776bcf6369f8b7b63879#egg=zendesk # Our libraries: --e git+https://github.com/edx/XBlock.git@483e0cb1#egg=XBlock +-e git+https://github.com/edx/XBlock.git@2144a25d#egg=XBlock -e git+https://github.com/edx/codejail.git@07494f1#egg=codejail From 3b15207bc625a75bb82a42482569daf79daaca8b Mon Sep 17 00:00:00 2001 From: cahrens Date: Tue, 21 May 2013 09:58:34 -0400 Subject: [PATCH 12/37] Use draft module store so we can test editing components (problems, html, etc.). --- cms/envs/acceptance.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cms/envs/acceptance.py b/cms/envs/acceptance.py index f4b867d3c6..0fe3d950cd 100644 --- a/cms/envs/acceptance.py +++ b/cms/envs/acceptance.py @@ -23,7 +23,7 @@ MODULESTORE_OPTIONS = { MODULESTORE = { 'default': { - 'ENGINE': 'xmodule.modulestore.mongo.MongoModuleStore', + 'ENGINE': 'xmodule.modulestore.mongo.DraftMongoModuleStore', 'OPTIONS': MODULESTORE_OPTIONS }, 'direct': { From cc7b2942eef5e68b3c7529b8c88d1a95b854a30d Mon Sep 17 00:00:00 2001 From: Nate Hardison Date: Fri, 17 May 2013 15:35:14 -0700 Subject: [PATCH 13/37] Handle Heroku's 503 maintenance mode response The LMS comment client previously would try to parse the response as JSON, choke, and return a 500 to the client. Now, the LMS client displays a message indicating that the forums are down for maintenance. --- lms/djangoapps/django_comment_client/forum/views.py | 3 +++ lms/lib/comment_client/utils.py | 7 +++++++ lms/templates/discussion/maintenance.html | 3 +++ 3 files changed, 13 insertions(+) create mode 100644 lms/templates/discussion/maintenance.html diff --git a/lms/djangoapps/django_comment_client/forum/views.py b/lms/djangoapps/django_comment_client/forum/views.py index 55797227ea..b04bd787d8 100644 --- a/lms/djangoapps/django_comment_client/forum/views.py +++ b/lms/djangoapps/django_comment_client/forum/views.py @@ -174,6 +174,9 @@ def forum_form_discussion(request, course_id): try: unsafethreads, query_params = get_threads(request, course_id) # This might process a search query threads = [utils.safe_content(thread) for thread in unsafethreads] + except (cc.utils.CommentClientMaintenanceError) as err: + log.warning("Forum is in maintenance mode") + return render_to_response('discussion/maintenance.html', {}) except (cc.utils.CommentClientError, cc.utils.CommentClientUnknownError) as err: log.error("Error loading forum discussion threads: %s" % str(err)) raise Http404 diff --git a/lms/lib/comment_client/utils.py b/lms/lib/comment_client/utils.py index 1ce03ed3c7..203e752a05 100644 --- a/lms/lib/comment_client/utils.py +++ b/lms/lib/comment_client/utils.py @@ -55,6 +55,9 @@ def perform_request(method, url, data_or_params=None, *args, **kwargs): if 200 < response.status_code < 500: raise CommentClientError(response.text) + # Heroku returns a 503 when an application is in maintenance mode + elif response.status_code == 503: + raise CommentClientMaintenanceError(response.text) elif response.status_code == 500: raise CommentClientUnknownError(response.text) else: @@ -72,5 +75,9 @@ class CommentClientError(Exception): return repr(self.message) +class CommentClientMaintenanceError(CommentClientError): + pass + + class CommentClientUnknownError(CommentClientError): pass diff --git a/lms/templates/discussion/maintenance.html b/lms/templates/discussion/maintenance.html new file mode 100644 index 0000000000..a0f57cdfb3 --- /dev/null +++ b/lms/templates/discussion/maintenance.html @@ -0,0 +1,3 @@ +<%inherit file="../main.html" /> +

We're sorry

+

The forums are currently undergoing maintenance. We'll have them back up shortly!

From cfe220a746b7faff79a4cbd92afdae7b961fd727 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 21 May 2013 13:07:04 -0400 Subject: [PATCH 14/37] Add exceptions for unused imports in settings files --- cms/envs/dev_with_worker.py | 4 ++++ lms/envs/dev_with_worker.py | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/cms/envs/dev_with_worker.py b/cms/envs/dev_with_worker.py index c5fc256ac9..078567c493 100644 --- a/cms/envs/dev_with_worker.py +++ b/cms/envs/dev_with_worker.py @@ -8,6 +8,10 @@ The worker can be executed using: django_admin.py celery worker """ +# We intentionally define lots of variables that aren't used, and +# want to import all variables from base settings files +# pylint: disable=W0401, W0614 + from dev import * ################################# CELERY ###################################### diff --git a/lms/envs/dev_with_worker.py b/lms/envs/dev_with_worker.py index c5fc256ac9..078567c493 100644 --- a/lms/envs/dev_with_worker.py +++ b/lms/envs/dev_with_worker.py @@ -8,6 +8,10 @@ The worker can be executed using: django_admin.py celery worker """ +# We intentionally define lots of variables that aren't used, and +# want to import all variables from base settings files +# pylint: disable=W0401, W0614 + from dev import * ################################# CELERY ###################################### From 0887383ca78d852cd90a023dc1e619235430ad03 Mon Sep 17 00:00:00 2001 From: Vik Paruchuri Date: Tue, 21 May 2013 14:18:00 -0400 Subject: [PATCH 15/37] Don't show peer grading button unless there is a peer grading element in the course --- lms/djangoapps/open_ended_grading/views.py | 53 ++++++++++++++-------- 1 file changed, 33 insertions(+), 20 deletions(-) diff --git a/lms/djangoapps/open_ended_grading/views.py b/lms/djangoapps/open_ended_grading/views.py index cb617d609d..1b7ae18029 100644 --- a/lms/djangoapps/open_ended_grading/views.py +++ b/lms/djangoapps/open_ended_grading/views.py @@ -89,6 +89,30 @@ def staff_grading(request, course_id): # Checked above 'staff_access': True, }) +def find_peer_grading_module(course): + #Reverse the base course url + base_course_url = reverse('courses') + found_module = False + problem_url = "" + + #Get the course id and split it + course_id_parts = course.id.split("/") + false_dict = [False, "False", "false", "FALSE"] + #TODO: This will not work with multiple runs of a course. Make it work. The last key in the Location passed + #to get_items is called revision. Is this the same as run? + #Get the peer grading modules currently in the course + items = modulestore().get_items(['i4x', None, course_id_parts[1], 'peergrading', None]) + #See if any of the modules are centralized modules (ie display info from multiple problems) + items = [i for i in items if getattr(i,"use_for_single_location", True) in false_dict] + #Get the first one + if len(items)>0: + item_location = items[0].location + #Generate a url for the first module and redirect the user to it + problem_url_parts = search.path_to_location(modulestore(), course.id, item_location) + problem_url = generate_problem_url(problem_url_parts, base_course_url) + found_module = True + + return found_module, problem_url @cache_control(no_cache=True, no_store=True, must_revalidate=True) def peer_grading(request, course_id): @@ -98,32 +122,16 @@ def peer_grading(request, course_id): #Get the current course course = get_course_with_access(request.user, course_id, 'load') - course_id_parts = course.id.split("/") - false_dict = [False, "False", "false", "FALSE"] - #Reverse the base course url - base_course_url = reverse('courses') - try: - #TODO: This will not work with multiple runs of a course. Make it work. The last key in the Location passed - #to get_items is called revision. Is this the same as run? - #Get the peer grading modules currently in the course - items = modulestore().get_items(['i4x', None, course_id_parts[1], 'peergrading', None]) - #See if any of the modules are centralized modules (ie display info from multiple problems) - items = [i for i in items if getattr(i,"use_for_single_location", True) in false_dict] - #Get the first one - item_location = items[0].location - #Generate a url for the first module and redirect the user to it - problem_url_parts = search.path_to_location(modulestore(), course.id, item_location) - problem_url = generate_problem_url(problem_url_parts, base_course_url) - - return HttpResponseRedirect(problem_url) - except: + found_module, problem_url = find_peer_grading_module(course) + if not found_module: #This is a student_facing_error error_message = "Error with initializing peer grading. Centralized module does not exist. Please contact course staff." #This is a dev_facing_error log.exception(error_message + "Current course is: {0}".format(course_id)) return HttpResponse(error_message) + return HttpResponseRedirect(problem_url) def generate_problem_url(problem_url_parts, base_course_url): """ @@ -300,7 +308,12 @@ def combined_notifications(request, course_id): 'description': description, 'alert_message': alert_message } - notification_list.append(notification_item) + if human_name == "Peer Grading": + found_module, problem_url = find_peer_grading_module(course) + if found_module: + notification_list.append(notification_item) + else: + notification_list.append(notification_item) ajax_url = _reverse_with_slash('open_ended_notifications', course_id) combined_dict = { From 956669de21553fc6938ad9feb0f80cf448f7c1c4 Mon Sep 17 00:00:00 2001 From: Vik Paruchuri Date: Tue, 21 May 2013 14:21:46 -0400 Subject: [PATCH 16/37] If location cannot be found, move on --- lms/djangoapps/open_ended_grading/views.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/lms/djangoapps/open_ended_grading/views.py b/lms/djangoapps/open_ended_grading/views.py index 1b7ae18029..baa4df2488 100644 --- a/lms/djangoapps/open_ended_grading/views.py +++ b/lms/djangoapps/open_ended_grading/views.py @@ -175,7 +175,12 @@ def student_problem_list(request, course_id): problem_list = problem_list_dict['problem_list'] for i in xrange(0, len(problem_list)): - problem_url_parts = search.path_to_location(modulestore(), course.id, problem_list[i]['location']) + try: + problem_url_parts = search.path_to_location(modulestore(), course.id, problem_list[i]['location']) + except: + error_message = "Could not find module for course {0} at location {1}".format(course.id, problem_list[i]['location']) + log.error(error_message) + continue problem_url = generate_problem_url(problem_url_parts, base_course_url) problem_list[i].update({'actual_url': problem_url}) eta_available = problem_list[i]['eta_available'] From c91fa2f45acfc83da74b3ca1f7d9e730ee16c4d4 Mon Sep 17 00:00:00 2001 From: Vik Paruchuri Date: Tue, 21 May 2013 14:23:24 -0400 Subject: [PATCH 17/37] Add in some comments --- lms/djangoapps/open_ended_grading/views.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lms/djangoapps/open_ended_grading/views.py b/lms/djangoapps/open_ended_grading/views.py index baa4df2488..30bf3ef4bd 100644 --- a/lms/djangoapps/open_ended_grading/views.py +++ b/lms/djangoapps/open_ended_grading/views.py @@ -165,6 +165,7 @@ def student_problem_list(request, course_id): base_course_url = reverse('courses') try: + #Get list of all open ended problems that the grading server knows about problem_list_json = controller_qs.get_grading_status_list(course_id, unique_id_for_user(request.user)) problem_list_dict = json.loads(problem_list_json) success = problem_list_dict['success'] @@ -176,8 +177,11 @@ def student_problem_list(request, course_id): for i in xrange(0, len(problem_list)): try: + #Try to load each problem in the courseware to get links to them problem_url_parts = search.path_to_location(modulestore(), course.id, problem_list[i]['location']) except: + #If the problem cannot be found at the location received from the grading controller server, it has been deleted by the course author. + #Continue with the rest of the location to construct the list error_message = "Could not find module for course {0} at location {1}".format(course.id, problem_list[i]['location']) log.error(error_message) continue From e59d345917f5206f2b715a67e6ce2afe28e12a8a Mon Sep 17 00:00:00 2001 From: Ned Batchelder Date: Tue, 21 May 2013 14:27:50 -0400 Subject: [PATCH 18/37] Use a different arbitrary dotted module, which Christina doesn't already have imported --- common/lib/capa/capa/safe_exec/tests/test_lazymod.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/common/lib/capa/capa/safe_exec/tests/test_lazymod.py b/common/lib/capa/capa/safe_exec/tests/test_lazymod.py index 68dcd81ea7..6a8ed5ff48 100644 --- a/common/lib/capa/capa/safe_exec/tests/test_lazymod.py +++ b/common/lib/capa/capa/safe_exec/tests/test_lazymod.py @@ -39,6 +39,9 @@ class TestLazyMod(unittest.TestCase): self.assertEqual(hsv[0], 0.25) def test_dotted(self): - self.assertNotIn("email.utils", sys.modules) - email_utils = LazyModule("email.utils") - self.assertEqual(email_utils.quote('"hi"'), r'\"hi\"') + # wsgiref is a module with submodules that is not already imported. + # Any similar module would do. This test demonstrates that the module + # is not already im + self.assertNotIn("wsgiref.util", sys.modules) + wsgiref_util = LazyModule("wsgiref.util") + self.assertEqual(wsgiref_util.guess_scheme({}), "http") From c5f9d94cc45497d49d25bc29f2b8cc8b98b4cd63 Mon Sep 17 00:00:00 2001 From: Vik Paruchuri Date: Tue, 21 May 2013 14:46:22 -0400 Subject: [PATCH 19/37] Add in comments, fix behavior --- lms/djangoapps/open_ended_grading/views.py | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/lms/djangoapps/open_ended_grading/views.py b/lms/djangoapps/open_ended_grading/views.py index 30bf3ef4bd..3ed144cc2b 100644 --- a/lms/djangoapps/open_ended_grading/views.py +++ b/lms/djangoapps/open_ended_grading/views.py @@ -90,6 +90,11 @@ def staff_grading(request, course_id): 'staff_access': True, }) def find_peer_grading_module(course): + """ + Given a course, finds the first peer grading module in it. + @param course: A course object. + @return: boolean found_module, string problem_url + """ #Reverse the base course url base_course_url = reverse('courses') found_module = False @@ -117,7 +122,7 @@ def find_peer_grading_module(course): @cache_control(no_cache=True, no_store=True, must_revalidate=True) def peer_grading(request, course_id): ''' - Show a peer grading interface + Show a peer grading interface to the student. The interface is linked to from the button. ''' #Get the current course @@ -153,7 +158,8 @@ def generate_problem_url(problem_url_parts, base_course_url): @cache_control(no_cache=True, no_store=True, must_revalidate=True) def student_problem_list(request, course_id): ''' - Show a student problem list + Show a student problem list to a student. Fetch the list from the grading controller server, get some metadata, + and then show it to the student. ''' course = get_course_with_access(request.user, course_id, 'load') student_id = unique_id_for_user(request.user) @@ -175,6 +181,8 @@ def student_problem_list(request, course_id): else: problem_list = problem_list_dict['problem_list'] + #A list of problems to remove (problems that can't be found in the course) + list_to_remove = [] for i in xrange(0, len(problem_list)): try: #Try to load each problem in the courseware to get links to them @@ -184,6 +192,8 @@ def student_problem_list(request, course_id): #Continue with the rest of the location to construct the list error_message = "Could not find module for course {0} at location {1}".format(course.id, problem_list[i]['location']) log.error(error_message) + #Mark the problem for removal from the list + list_to_remove.append(i) continue problem_url = generate_problem_url(problem_url_parts, base_course_url) problem_list[i].update({'actual_url': problem_url}) @@ -214,6 +224,8 @@ def student_problem_list(request, course_id): log.error("Problem with results from external grading service for open ended.") success = False + #Remove problems that cannot be found in the courseware from the list + problem_list = [problem_list[i] for i in xrange(0,len(problem_list)) if i not in list_to_remove] ajax_url = _reverse_with_slash('open_ended_problems', course_id) return render_to_response('open_ended_problems/open_ended_problems.html', { From 5bcae4e79f24b446b2840a141dcefb3b9e05cb3a Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 21 May 2013 14:47:35 -0400 Subject: [PATCH 20/37] Add Review Board config to repo --- .reviewboardrc | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 .reviewboardrc diff --git a/.reviewboardrc b/.reviewboardrc new file mode 100644 index 0000000000..b79235a4a4 --- /dev/null +++ b/.reviewboardrc @@ -0,0 +1,2 @@ +REVIEWBOARD_URL = "https://rbcommons.com/s/edx/" +GUESS_FIELDS = True From cf502e2349d96ef278ec316781be1342c64a3afe Mon Sep 17 00:00:00 2001 From: Arthur Barrett Date: Tue, 21 May 2013 17:49:20 -0400 Subject: [PATCH 21/37] 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 34a659ab29..89b5e8bdc7 100644 --- a/cms/djangoapps/contentstore/views/component.py +++ b/cms/djangoapps/contentstore/views/component.py @@ -42,7 +42,7 @@ COMPONENT_TYPES = ['customtag', 'discussion', 'html', 'problem', 'video'] OPEN_ENDED_COMPONENT_TYPES = ["combinedopenended", "peergrading"] NOTE_COMPONENT_TYPES = ['notes'] -ADVANCED_COMPONENT_TYPES = ['annotatable' + 'word_cloud'] + OPEN_ENDED_COMPONENT_TYPES + NOTE_COMPONENT_TYPES +ADVANCED_COMPONENT_TYPES = ['annotatable', 'word_cloud'] + OPEN_ENDED_COMPONENT_TYPES + NOTE_COMPONENT_TYPES ADVANCED_COMPONENT_CATEGORY = 'advanced' ADVANCED_COMPONENT_POLICY_KEY = 'advanced_modules' From ce005072b092ae8a8206666fddbc63c14cbe39c1 Mon Sep 17 00:00:00 2001 From: Ned Batchelder Date: Tue, 21 May 2013 17:25:37 -0400 Subject: [PATCH 22/37] Sandbox-installed packages will be really installed instead of -e installed. --- common/lib/calc/setup.py | 2 +- common/lib/chem/setup.py | 2 +- common/lib/sandbox-packages/setup.py | 2 +- requirements/edx-sandbox/post.txt | 10 +++++++--- 4 files changed, 10 insertions(+), 6 deletions(-) diff --git a/common/lib/calc/setup.py b/common/lib/calc/setup.py index f7bb1708af..cb638914f9 100644 --- a/common/lib/calc/setup.py +++ b/common/lib/calc/setup.py @@ -2,7 +2,7 @@ from setuptools import setup setup( name="calc", - version="0.1", + version="0.1.1", py_modules=["calc"], install_requires=[ "pyparsing==1.5.6", diff --git a/common/lib/chem/setup.py b/common/lib/chem/setup.py index 4f2b24ddee..642c9a4fe5 100644 --- a/common/lib/chem/setup.py +++ b/common/lib/chem/setup.py @@ -2,7 +2,7 @@ from setuptools import setup setup( name="chem", - version="0.1", + version="0.1.1", packages=["chem"], install_requires=[ "pyparsing==1.5.6", diff --git a/common/lib/sandbox-packages/setup.py b/common/lib/sandbox-packages/setup.py index 1b99118aca..96c1190e38 100644 --- a/common/lib/sandbox-packages/setup.py +++ b/common/lib/sandbox-packages/setup.py @@ -2,7 +2,7 @@ from setuptools import setup setup( name="sandbox-packages", - version="0.1", + version="0.1.1", packages=[ "verifiers", ], diff --git a/requirements/edx-sandbox/post.txt b/requirements/edx-sandbox/post.txt index f99e8a8c4b..d122795d18 100644 --- a/requirements/edx-sandbox/post.txt +++ b/requirements/edx-sandbox/post.txt @@ -1,6 +1,10 @@ # Packages to install in the Python sandbox for secured execution. scipy==0.11.0 lxml==3.0.1 --e common/lib/calc --e common/lib/chem --e common/lib/sandbox-packages + +# Install these packages from the edx-platform working tree +# NOTE: if you change code in these packages, you MUST change the version +# number in its setup.py or the code WILL NOT be installed during deploy. +common/lib/calc +common/lib/chem +common/lib/sandbox-packages From bf20760922a1aebb33243365c0861a6b155a9277 Mon Sep 17 00:00:00 2001 From: e0d Date: Wed, 22 May 2013 10:19:30 -0400 Subject: [PATCH 23/37] changes to refactor local requirements. --- requirements/edx-sandbox/local.txt | 6 ++++++ requirements/edx-sandbox/post.txt | 7 ------- 2 files changed, 6 insertions(+), 7 deletions(-) create mode 100644 requirements/edx-sandbox/local.txt diff --git a/requirements/edx-sandbox/local.txt b/requirements/edx-sandbox/local.txt new file mode 100644 index 0000000000..ba24805057 --- /dev/null +++ b/requirements/edx-sandbox/local.txt @@ -0,0 +1,6 @@ +# Install these packages from the edx-platform working tree +# NOTE: if you change code in these packages, you MUST change the version +# number in its setup.py or the code WILL NOT be installed during deploy. +common/lib/calc +common/lib/chem +common/lib/sandbox-packages diff --git a/requirements/edx-sandbox/post.txt b/requirements/edx-sandbox/post.txt index d122795d18..218fdf307e 100644 --- a/requirements/edx-sandbox/post.txt +++ b/requirements/edx-sandbox/post.txt @@ -1,10 +1,3 @@ # Packages to install in the Python sandbox for secured execution. scipy==0.11.0 lxml==3.0.1 - -# Install these packages from the edx-platform working tree -# NOTE: if you change code in these packages, you MUST change the version -# number in its setup.py or the code WILL NOT be installed during deploy. -common/lib/calc -common/lib/chem -common/lib/sandbox-packages From 21f7e222f79a50c7c8e498fcaecadf1c7780b348 Mon Sep 17 00:00:00 2001 From: Vik Paruchuri Date: Wed, 22 May 2013 10:51:11 -0400 Subject: [PATCH 24/37] Fix exception clause --- lms/djangoapps/open_ended_grading/views.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lms/djangoapps/open_ended_grading/views.py b/lms/djangoapps/open_ended_grading/views.py index 3ed144cc2b..f989d92902 100644 --- a/lms/djangoapps/open_ended_grading/views.py +++ b/lms/djangoapps/open_ended_grading/views.py @@ -21,6 +21,7 @@ import open_ended_notifications from xmodule.modulestore.django import modulestore from xmodule.modulestore import search +from xmodule.modulestore.exceptions import ItemNotFoundError from django.http import HttpResponse, Http404, HttpResponseRedirect from mitxmako.shortcuts import render_to_string @@ -187,7 +188,7 @@ def student_problem_list(request, course_id): try: #Try to load each problem in the courseware to get links to them problem_url_parts = search.path_to_location(modulestore(), course.id, problem_list[i]['location']) - except: + except ItemNotFoundError: #If the problem cannot be found at the location received from the grading controller server, it has been deleted by the course author. #Continue with the rest of the location to construct the list error_message = "Could not find module for course {0} at location {1}".format(course.id, problem_list[i]['location']) From a44eacd474ad938b5754641f3e7ca7fa7b63d6d7 Mon Sep 17 00:00:00 2001 From: Vik Paruchuri Date: Wed, 22 May 2013 10:54:52 -0400 Subject: [PATCH 25/37] Make fields stringy, remove false dict --- .../xmodule/combined_open_ended_module.py | 16 ++++++++-------- lms/djangoapps/open_ended_grading/views.py | 3 +-- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/common/lib/xmodule/xmodule/combined_open_ended_module.py b/common/lib/xmodule/xmodule/combined_open_ended_module.py index f4074283fe..a2ce4d0a65 100644 --- a/common/lib/xmodule/xmodule/combined_open_ended_module.py +++ b/common/lib/xmodule/xmodule/combined_open_ended_module.py @@ -8,7 +8,7 @@ from .x_module import XModule from xblock.core import Integer, Scope, String, Boolean, List from xmodule.open_ended_grading_classes.combined_open_ended_modulev1 import CombinedOpenEndedV1Module, CombinedOpenEndedV1Descriptor from collections import namedtuple -from .fields import Date, StringyFloat +from .fields import Date, StringyFloat, StringyInteger, StringyBoolean log = logging.getLogger("mitx.courseware") @@ -49,19 +49,19 @@ class VersionInteger(Integer): class CombinedOpenEndedFields(object): display_name = String(help="Display name for this module", default="Open Ended Grading", scope=Scope.settings) - current_task_number = Integer(help="Current task that the student is on.", default=0, scope=Scope.user_state) + current_task_number = StringyInteger(help="Current task that the student is on.", default=0, scope=Scope.user_state) task_states = List(help="List of state dictionaries of each task within this module.", scope=Scope.user_state) state = String(help="Which step within the current task that the student is on.", default="initial", scope=Scope.user_state) - student_attempts = Integer(help="Number of attempts taken by the student on this problem", default=0, + student_attempts = StringyInteger(help="Number of attempts taken by the student on this problem", default=0, scope=Scope.user_state) - ready_to_reset = Boolean(help="If the problem is ready to be reset or not.", default=False, + ready_to_reset = StringyBoolean(help="If the problem is ready to be reset or not.", default=False, scope=Scope.user_state) - attempts = Integer(help="Maximum number of attempts that a student is allowed.", default=1, scope=Scope.settings) - is_graded = Boolean(help="Whether or not the problem is graded.", default=False, scope=Scope.settings) - accept_file_upload = Boolean(help="Whether or not the problem accepts file uploads.", default=False, + attempts = StringyInteger(help="Maximum number of attempts that a student is allowed.", default=1, scope=Scope.settings) + is_graded = StringyBoolean(help="Whether or not the problem is graded.", default=False, scope=Scope.settings) + accept_file_upload = StringyBoolean(help="Whether or not the problem accepts file uploads.", default=False, scope=Scope.settings) - skip_spelling_checks = Boolean(help="Whether or not to skip initial spelling checks.", default=True, + skip_spelling_checks = StringyBoolean(help="Whether or not to skip initial spelling checks.", default=True, scope=Scope.settings) due = Date(help="Date that this problem is due by", default=None, scope=Scope.settings) graceperiod = String(help="Amount of time after the due date that submissions will be accepted", default=None, diff --git a/lms/djangoapps/open_ended_grading/views.py b/lms/djangoapps/open_ended_grading/views.py index f989d92902..e6da717067 100644 --- a/lms/djangoapps/open_ended_grading/views.py +++ b/lms/djangoapps/open_ended_grading/views.py @@ -103,13 +103,12 @@ def find_peer_grading_module(course): #Get the course id and split it course_id_parts = course.id.split("/") - false_dict = [False, "False", "false", "FALSE"] #TODO: This will not work with multiple runs of a course. Make it work. The last key in the Location passed #to get_items is called revision. Is this the same as run? #Get the peer grading modules currently in the course items = modulestore().get_items(['i4x', None, course_id_parts[1], 'peergrading', None]) #See if any of the modules are centralized modules (ie display info from multiple problems) - items = [i for i in items if getattr(i,"use_for_single_location", True) in false_dict] + items = [i for i in items if not getattr(i,"use_for_single_location", True)] #Get the first one if len(items)>0: item_location = items[0].location From 3bd04290f58a592139850cb72862cedafa6f60a7 Mon Sep 17 00:00:00 2001 From: Vik Paruchuri Date: Wed, 22 May 2013 11:58:03 -0400 Subject: [PATCH 26/37] Add tests, address review comments --- common/lib/xmodule/xmodule/modulestore/xml.py | 1 - .../controller_query_service.py | 46 +++++- lms/djangoapps/open_ended_grading/tests.py | 151 ++++++++++-------- lms/djangoapps/open_ended_grading/views.py | 39 +++-- 4 files changed, 156 insertions(+), 81 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/xml.py b/common/lib/xmodule/xmodule/modulestore/xml.py index 114a3281c6..7128c04a88 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml.py +++ b/common/lib/xmodule/xmodule/modulestore/xml.py @@ -290,7 +290,6 @@ class XMLModuleStore(ModuleStoreBase): if course_dirs is None: course_dirs = sorted([d for d in os.listdir(self.data_dir) if os.path.exists(self.data_dir / d / "course.xml")]) - for course_dir in course_dirs: self.try_load_course(course_dir) diff --git a/common/lib/xmodule/xmodule/open_ended_grading_classes/controller_query_service.py b/common/lib/xmodule/xmodule/open_ended_grading_classes/controller_query_service.py index 08f2a95387..b807e05160 100644 --- a/common/lib/xmodule/xmodule/open_ended_grading_classes/controller_query_service.py +++ b/common/lib/xmodule/xmodule/open_ended_grading_classes/controller_query_service.py @@ -6,7 +6,7 @@ log = logging.getLogger(__name__) class ControllerQueryService(GradingService): """ - Interface to staff grading backend. + Interface to controller query backend. """ def __init__(self, config, system): @@ -77,6 +77,50 @@ class ControllerQueryService(GradingService): return response +class MockControllerQueryService(object): + """ + Mock controller query service for testing + """ + + def __init__(self, config, system): + pass + + def check_if_name_is_unique(self, **params): + """ + Mock later if needed. Stub function for now. + @param params: + @return: + """ + pass + + def check_for_eta(self, **params): + """ + Mock later if needed. Stub function for now. + @param params: + @return: + """ + pass + + def check_combined_notifications(self, **params): + combined_notifications = '{"flagged_submissions_exist": false, "version": 1, "new_student_grading_to_view": false, "success": true, "staff_needs_to_grade": false, "student_needs_to_peer_grade": true, "overall_need_to_check": true}' + return combined_notifications + + def get_grading_status_list(self, **params): + grading_status_list = '{"version": 1, "problem_list": [{"problem_name": "Science Question -- Machine Assessed", "grader_type": "NA", "eta_available": true, "state": "Waiting to be Graded", "eta": 259200, "location": "i4x://MITx/oe101x/combinedopenended/Science_SA_ML"}, {"problem_name": "Humanities Question -- Peer Assessed", "grader_type": "NA", "eta_available": true, "state": "Waiting to be Graded", "eta": 259200, "location": "i4x://MITx/oe101x/combinedopenended/Humanities_SA_Peer"}], "success": true}' + return grading_status_list + + def get_flagged_problem_list(self, **params): + flagged_problem_list = '{"version": 1, "success": false, "error": "No flagged submissions exist for course: MITx/oe101x/2012_Fall"}' + return flagged_problem_list + + def take_action_on_flags(self, **params): + """ + Mock later if needed. Stub function for now. + @param params: + @return: + """ + pass + def convert_seconds_to_human_readable(seconds): if seconds < 60: human_string = "{0} seconds".format(seconds) diff --git a/lms/djangoapps/open_ended_grading/tests.py b/lms/djangoapps/open_ended_grading/tests.py index ffc02608d5..76068dea70 100644 --- a/lms/djangoapps/open_ended_grading/tests.py +++ b/lms/djangoapps/open_ended_grading/tests.py @@ -5,19 +5,20 @@ django-admin.py test --settings=lms.envs.test --pythonpath=. lms/djangoapps/open """ import json -from mock import MagicMock +from mock import MagicMock, patch, Mock from django.core.urlresolvers import reverse from django.contrib.auth.models import Group +from django.http import HttpResponse from mitxmako.shortcuts import render_to_string -from xmodule.open_ended_grading_classes import peer_grading_service +from xmodule.open_ended_grading_classes import peer_grading_service, controller_query_service from xmodule import peer_grading_module from xmodule.modulestore.django import modulestore import xmodule.modulestore.django from xmodule.x_module import ModuleSystem -from open_ended_grading import staff_grading_service +from open_ended_grading import staff_grading_service, views from courseware.access import _course_staff_group_name from courseware.tests.tests import LoginEnrollmentTestCase, TEST_DATA_XML_MODULESTORE, get_user @@ -25,10 +26,11 @@ import logging log = logging.getLogger(__name__) from django.test.utils import override_settings -from django.http import QueryDict from xmodule.tests import test_util_open_ended +from courseware.tests import factories + @override_settings(MODULESTORE=TEST_DATA_XML_MODULESTORE) class TestStaffGradingService(LoginEnrollmentTestCase): @@ -55,8 +57,8 @@ class TestStaffGradingService(LoginEnrollmentTestCase): 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)) + group = Group.objects.create(name=group_name) + group.user_set.add(get_user(self.instructor)) make_instructor(self.toy) @@ -76,30 +78,28 @@ class TestStaffGradingService(LoginEnrollmentTestCase): self.check_for_get_code(404, url) self.check_for_post_code(404, url) - def test_get_next(self): self.login(self.instructor, self.password) url = reverse('staff_grading_get_next', kwargs={'course_id': self.course_id}) data = {'location': self.location} - r = self.check_for_post_code(200, url, data) + response = self.check_for_post_code(200, url, data) - d = json.loads(r.content) + content = json.loads(response.content) - self.assertTrue(d['success']) - self.assertEquals(d['submission_id'], self.mock_service.cnt) - self.assertIsNotNone(d['submission']) - self.assertIsNotNone(d['num_graded']) - self.assertIsNotNone(d['min_for_ml']) - self.assertIsNotNone(d['num_pending']) - self.assertIsNotNone(d['prompt']) - self.assertIsNotNone(d['ml_error_info']) - self.assertIsNotNone(d['max_score']) - self.assertIsNotNone(d['rubric']) + self.assertTrue(content['success']) + self.assertEquals(content['submission_id'], self.mock_service.cnt) + self.assertIsNotNone(content['submission']) + self.assertIsNotNone(content['num_graded']) + self.assertIsNotNone(content['min_for_ml']) + self.assertIsNotNone(content['num_pending']) + self.assertIsNotNone(content['prompt']) + self.assertIsNotNone(content['ml_error_info']) + self.assertIsNotNone(content['max_score']) + self.assertIsNotNone(content['rubric']) - - def save_grade_base(self,skip=False): + def save_grade_base(self, skip=False): self.login(self.instructor, self.password) url = reverse('staff_grading_save_grade', kwargs={'course_id': self.course_id}) @@ -111,12 +111,12 @@ class TestStaffGradingService(LoginEnrollmentTestCase): 'submission_flagged': "true", 'rubric_scores[]': ['1', '2']} if skip: - data.update({'skipped' : True}) + data.update({'skipped': True}) - r = self.check_for_post_code(200, url, data) - d = json.loads(r.content) - self.assertTrue(d['success'], str(d)) - self.assertEquals(d['submission_id'], self.mock_service.cnt) + response = self.check_for_post_code(200, url, data) + content = json.loads(response.content) + self.assertTrue(content['success'], str(content)) + self.assertEquals(content['submission_id'], self.mock_service.cnt) def test_save_grade(self): self.save_grade_base(skip=False) @@ -130,11 +130,11 @@ class TestStaffGradingService(LoginEnrollmentTestCase): url = reverse('staff_grading_get_problem_list', kwargs={'course_id': self.course_id}) data = {} - r = self.check_for_post_code(200, url, data) - d = json.loads(r.content) + response = self.check_for_post_code(200, url, data) + content = json.loads(response.content) - self.assertTrue(d['success'], str(d)) - self.assertIsNotNone(d['problem_list']) + self.assertTrue(content['success'], str(content)) + self.assertIsNotNone(content['problem_list']) @override_settings(MODULESTORE=TEST_DATA_XML_MODULESTORE) @@ -181,14 +181,14 @@ class TestPeerGradingService(LoginEnrollmentTestCase): def test_get_next_submission_success(self): data = {'location': self.location} - r = self.peer_module.get_next_submission(data) - d = r + response = self.peer_module.get_next_submission(data) + content = response - self.assertTrue(d['success']) - self.assertIsNotNone(d['submission_id']) - self.assertIsNotNone(d['prompt']) - self.assertIsNotNone(d['submission_key']) - self.assertIsNotNone(d['max_score']) + self.assertTrue(content['success']) + self.assertIsNotNone(content['submission_id']) + self.assertIsNotNone(content['prompt']) + self.assertIsNotNone(content['submission_key']) + self.assertIsNotNone(content['max_score']) def test_get_next_submission_missing_location(self): data = {} @@ -216,10 +216,9 @@ class TestPeerGradingService(LoginEnrollmentTestCase): qdict.getlist = fake_get_item qdict.keys = data.keys - r = self.peer_module.save_grade(qdict) - d = r + response = self.peer_module.save_grade(qdict) - self.assertTrue(d['success']) + self.assertTrue(response['success']) def test_save_grade_missing_keys(self): data = {} @@ -229,37 +228,35 @@ class TestPeerGradingService(LoginEnrollmentTestCase): def test_is_calibrated_success(self): data = {'location': self.location} - r = self.peer_module.is_student_calibrated(data) - d = r + response = self.peer_module.is_student_calibrated(data) - self.assertTrue(d['success']) - self.assertTrue('calibrated' in d) + self.assertTrue(response['success']) + self.assertTrue('calibrated' in response) def test_is_calibrated_failure(self): data = {} - d = self.peer_module.is_student_calibrated(data) - self.assertFalse(d['success']) - self.assertFalse('calibrated' in d) + response = self.peer_module.is_student_calibrated(data) + self.assertFalse(response['success']) + self.assertFalse('calibrated' in response) def test_show_calibration_essay_success(self): data = {'location': self.location} - r = self.peer_module.show_calibration_essay(data) - d = r + response = self.peer_module.show_calibration_essay(data) - self.assertTrue(d['success']) - self.assertIsNotNone(d['submission_id']) - self.assertIsNotNone(d['prompt']) - self.assertIsNotNone(d['submission_key']) - self.assertIsNotNone(d['max_score']) + self.assertTrue(response['success']) + self.assertIsNotNone(response['submission_id']) + self.assertIsNotNone(response['prompt']) + self.assertIsNotNone(response['submission_key']) + self.assertIsNotNone(response['max_score']) def test_show_calibration_essay_missing_key(self): data = {} - d = self.peer_module.show_calibration_essay(data) + response = self.peer_module.show_calibration_essay(data) - self.assertFalse(d['success']) - self.assertEqual(d['error'], "Missing required keys: location") + self.assertFalse(response['success']) + self.assertEqual(response['error'], "Missing required keys: location") def test_save_calibration_essay_success(self): data = { @@ -281,13 +278,39 @@ class TestPeerGradingService(LoginEnrollmentTestCase): qdict.getlist = fake_get_item qdict.keys = data.keys - d = self.peer_module.save_calibration_essay(qdict) - self.assertTrue(d['success']) - self.assertTrue('actual_score' in d) + response = self.peer_module.save_calibration_essay(qdict) + self.assertTrue(response['success']) + self.assertTrue('actual_score' in response) def test_save_calibration_essay_missing_keys(self): data = {} - d = self.peer_module.save_calibration_essay(data) - self.assertFalse(d['success']) - self.assertTrue(d['error'].find('Missing required keys:') > -1) - self.assertFalse('actual_score' in d) + response = self.peer_module.save_calibration_essay(data) + self.assertFalse(response['success']) + self.assertTrue(response['error'].find('Missing required keys:') > -1) + self.assertFalse('actual_score' in response) + + +@override_settings(MODULESTORE=TEST_DATA_XML_MODULESTORE) +class TestPanel(LoginEnrollmentTestCase): + """Check the Table of Contents for a course""" + + def setUp(self): + # Toy courses should be loaded + self.course_name = 'edX/open_ended/2012_Fall' + self.course = modulestore().get_course(self.course_name) + self.user = factories.UserFactory() + + def test_open_ended_panel(self): + """ + Test to see if the peer grading module in the demo course is found + @return: + """ + found_module, peer_grading_module = views.find_peer_grading_module(self.course) + self.assertTrue(found_module) + + @patch('xmodule.open_ended_grading_classes.controller_query_service.ControllerQueryService', + controller_query_service.MockControllerQueryService) + def test_problem_list(self): + request = Mock(user=self.user) + response = views.student_problem_list(request, self.course.id) + self.assertTrue(isinstance(response, HttpResponse)) diff --git a/lms/djangoapps/open_ended_grading/views.py b/lms/djangoapps/open_ended_grading/views.py index e6da717067..eac79aa85e 100644 --- a/lms/djangoapps/open_ended_grading/views.py +++ b/lms/djangoapps/open_ended_grading/views.py @@ -31,10 +31,10 @@ log = logging.getLogger(__name__) system = ModuleSystem( ajax_url=None, track_function=None, - get_module = None, + get_module=None, render_template=render_to_string, - replace_urls = None, - xblock_model_data= {} + replace_urls=None, + xblock_model_data={} ) controller_qs = ControllerQueryService(settings.OPEN_ENDED_GRADING_INTERFACE, system) @@ -90,6 +90,7 @@ def staff_grading(request, course_id): # Checked above 'staff_access': True, }) + def find_peer_grading_module(course): """ Given a course, finds the first peer grading module in it. @@ -103,14 +104,15 @@ def find_peer_grading_module(course): #Get the course id and split it course_id_parts = course.id.split("/") - #TODO: This will not work with multiple runs of a course. Make it work. The last key in the Location passed - #to get_items is called revision. Is this the same as run? - #Get the peer grading modules currently in the course - items = modulestore().get_items(['i4x', None, course_id_parts[1], 'peergrading', None]) + log.info("COURSE ID PARTS") + log.info(course_id_parts) + #Get the peer grading modules currently in the course. Explicitly specify the course id to avoid issues with different runs. + items = modulestore().get_items(['i4x', course_id_parts[0], course_id_parts[1], 'peergrading', None], + course_id=course.id) #See if any of the modules are centralized modules (ie display info from multiple problems) - items = [i for i in items if not getattr(i,"use_for_single_location", True)] + items = [i for i in items if not getattr(i, "use_for_single_location", True)] #Get the first one - if len(items)>0: + if len(items) > 0: item_location = items[0].location #Generate a url for the first module and redirect the user to it problem_url_parts = search.path_to_location(modulestore(), course.id, item_location) @@ -119,10 +121,12 @@ def find_peer_grading_module(course): return found_module, problem_url + @cache_control(no_cache=True, no_store=True, must_revalidate=True) def peer_grading(request, course_id): ''' - Show a peer grading interface to the student. The interface is linked to from the button. + When a student clicks on the "peer grading" button in the open ended interface, link them to a peer grading + xmodule in the course. ''' #Get the current course @@ -138,6 +142,7 @@ def peer_grading(request, course_id): return HttpResponseRedirect(problem_url) + def generate_problem_url(problem_url_parts, base_course_url): """ From a list of problem url parts generated by search.path_to_location and a base course url, generates a url to a problem @@ -190,7 +195,9 @@ def student_problem_list(request, course_id): except ItemNotFoundError: #If the problem cannot be found at the location received from the grading controller server, it has been deleted by the course author. #Continue with the rest of the location to construct the list - error_message = "Could not find module for course {0} at location {1}".format(course.id, problem_list[i]['location']) + error_message = "Could not find module for course {0} at location {1}".format(course.id, + problem_list[i][ + 'location']) log.error(error_message) #Mark the problem for removal from the list list_to_remove.append(i) @@ -225,7 +232,7 @@ def student_problem_list(request, course_id): success = False #Remove problems that cannot be found in the courseware from the list - problem_list = [problem_list[i] for i in xrange(0,len(problem_list)) if i not in list_to_remove] + problem_list = [problem_list[i] for i in xrange(0, len(problem_list)) if i not in list_to_remove] ajax_url = _reverse_with_slash('open_ended_problems', course_id) return render_to_response('open_ended_problems/open_ended_problems.html', { @@ -329,6 +336,10 @@ def combined_notifications(request, course_id): 'description': description, 'alert_message': alert_message } + #The open ended panel will need to link the "peer grading" button in the panel to a peer grading + #xmodule defined in the course. This checks to see if the human name of the server notification + #that we are currently processing is "peer grading". If it is, it looks for a peer grading + #module in the course. If none exists, it removes the peer grading item from the panel. if human_name == "Peer Grading": found_module, problem_url = find_peer_grading_module(course) if found_module: @@ -345,9 +356,7 @@ def combined_notifications(request, course_id): 'ajax_url': ajax_url, } - return render_to_response('open_ended_problems/combined_notifications.html', - combined_dict - ) + return render_to_response('open_ended_problems/combined_notifications.html', combined_dict) @cache_control(no_cache=True, no_store=True, must_revalidate=True) From 96f145b8f60caf3a0a7712b1a3810231cd1d808b Mon Sep 17 00:00:00 2001 From: Vik Paruchuri Date: Wed, 22 May 2013 12:00:36 -0400 Subject: [PATCH 27/37] Update error message --- lms/djangoapps/open_ended_grading/views.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lms/djangoapps/open_ended_grading/views.py b/lms/djangoapps/open_ended_grading/views.py index eac79aa85e..2bb6f61491 100644 --- a/lms/djangoapps/open_ended_grading/views.py +++ b/lms/djangoapps/open_ended_grading/views.py @@ -135,7 +135,11 @@ def peer_grading(request, course_id): found_module, problem_url = find_peer_grading_module(course) if not found_module: #This is a student_facing_error - error_message = "Error with initializing peer grading. Centralized module does not exist. Please contact course staff." + error_message = """ + Error with initializing peer grading. + There has not been a peer grading module created in the courseware that would allow you to grade others. + Please check back later for this. + """ #This is a dev_facing_error log.exception(error_message + "Current course is: {0}".format(course_id)) return HttpResponse(error_message) From e9f05640f805c4a6650869b93c6df5e81b7720ca Mon Sep 17 00:00:00 2001 From: Vik Paruchuri Date: Wed, 22 May 2013 12:16:23 -0400 Subject: [PATCH 28/37] Update comments --- lms/djangoapps/open_ended_grading/tests.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/lms/djangoapps/open_ended_grading/tests.py b/lms/djangoapps/open_ended_grading/tests.py index 76068dea70..18ba863d69 100644 --- a/lms/djangoapps/open_ended_grading/tests.py +++ b/lms/djangoapps/open_ended_grading/tests.py @@ -292,7 +292,9 @@ class TestPeerGradingService(LoginEnrollmentTestCase): @override_settings(MODULESTORE=TEST_DATA_XML_MODULESTORE) class TestPanel(LoginEnrollmentTestCase): - """Check the Table of Contents for a course""" + """ + Run tests on the open ended panel + """ def setUp(self): # Toy courses should be loaded @@ -311,6 +313,10 @@ class TestPanel(LoginEnrollmentTestCase): @patch('xmodule.open_ended_grading_classes.controller_query_service.ControllerQueryService', controller_query_service.MockControllerQueryService) def test_problem_list(self): + """ + Ensure that the problem list from the grading controller server can be rendered properly locally + @return: + """ request = Mock(user=self.user) response = views.student_problem_list(request, self.course.id) self.assertTrue(isinstance(response, HttpResponse)) From 392de8d40e1055dccd22428f6ccac593d9c98178 Mon Sep 17 00:00:00 2001 From: Ned Batchelder Date: Wed, 22 May 2013 13:27:24 -0400 Subject: [PATCH 29/37] Check functions need access to course-local code also, so we need to save the python path from the problem setup, and use it for the check function. --- common/lib/capa/capa/capa_problem.py | 5 ++--- common/lib/capa/capa/responsetypes.py | 4 ++-- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/common/lib/capa/capa/capa_problem.py b/common/lib/capa/capa/capa_problem.py index 7ead599d67..8543e9e3e1 100644 --- a/common/lib/capa/capa/capa_problem.py +++ b/common/lib/capa/capa/capa_problem.py @@ -475,12 +475,11 @@ class LoncapaProblem(object): msg = "Error while executing script code: %s" % str(err).replace('<', '<') raise responsetypes.LoncapaProblemError(msg) - # store code source in context + # Store code source in context, along with the Python path needed to run it correctly. context['script_code'] = all_code + context['python_path'] = python_path return context - - def _extract_html(self, problemtree): # private ''' Main (private) function which converts Problem XML tree to HTML. diff --git a/common/lib/capa/capa/responsetypes.py b/common/lib/capa/capa/responsetypes.py index c7a99f1271..a166438f17 100644 --- a/common/lib/capa/capa/responsetypes.py +++ b/common/lib/capa/capa/responsetypes.py @@ -286,7 +286,7 @@ class LoncapaResponse(object): } try: - safe_exec.safe_exec(code, globals_dict) + safe_exec.safe_exec(code, globals_dict, python_path=self.context['python_path']) except Exception as err: msg = 'Error %s in evaluating hint function %s' % (err, hintfn) msg += "\nSee XML source line %s" % getattr( @@ -972,7 +972,7 @@ class CustomResponse(LoncapaResponse): 'ans': ans, } globals_dict.update(kwargs) - safe_exec.safe_exec(code, globals_dict, cache=self.system.cache) + safe_exec.safe_exec(code, globals_dict, python_path=self.context['python_path']) return globals_dict['cfn_return'] return check_function From 70c4af40f0ac692b8f66164b0e9e20725a8cfefd Mon Sep 17 00:00:00 2001 From: Jason Bau Date: Wed, 22 May 2013 10:55:21 -0700 Subject: [PATCH 30/37] add MITX Feature setting to enable django admin site --- lms/urls.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lms/urls.py b/lms/urls.py index 3b14b41bd7..d1bee076fc 100644 --- a/lms/urls.py +++ b/lms/urls.py @@ -8,7 +8,7 @@ from . import one_time_startup import django.contrib.auth.views # Uncomment the next two lines to enable the admin: -if settings.DEBUG: +if settings.DEBUG or settings.MITX_FEATURES.get('ENABLE_DJANGO_ADMIN_SITE'): admin.autodiscover() urlpatterns = ('', # nopep8 @@ -330,7 +330,7 @@ if settings.COURSEWARE_ENABLED: if settings.ENABLE_JASMINE: urlpatterns += (url(r'^_jasmine/', include('django_jasmine.urls')),) -if settings.DEBUG: +if settings.DEBUG or settings.MITX_FEATURES.get('ENABLE_DJANGO_ADMIN_SITE'): ## Jasmine and admin urlpatterns += (url(r'^admin/', include(admin.site.urls)),) From 787366fe02f5ea79ad50e1497469e34e988e50da Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 22 May 2013 13:08:07 -0400 Subject: [PATCH 31/37] Remove tags for comment client request time histogram in Datadog According to someone from Datadog, this was generating tags like "knowledgeable_ people_who_put_this_course_together._this_is_harvard._you_can_t_tell_us_there_s_ a_shortage_of_editorial_talent." They say that they can handle tens or hundreds of unique tags but not thousands. Given that we have a unique URL for each thread, we can't even use that as a tag. Thus, all tags are removed for now until we can determine whether there is a useful set of tags with small enough cardinality. In light of this, I did not investigate why the long tag mentioned above was being generated. --- lms/lib/comment_client/utils.py | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/lms/lib/comment_client/utils.py b/lms/lib/comment_client/utils.py index 203e752a05..0ff06fced7 100644 --- a/lms/lib/comment_client/utils.py +++ b/lms/lib/comment_client/utils.py @@ -31,14 +31,9 @@ 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', tags=tags): + 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: From 5fc8469a81cbabf3203bbac383d35219dd132fd0 Mon Sep 17 00:00:00 2001 From: Jason Bau Date: Wed, 22 May 2013 11:45:42 -0700 Subject: [PATCH 32/37] add example in lms/envs/common.py --- lms/envs/common.py | 1 + 1 file changed, 1 insertion(+) diff --git a/lms/envs/common.py b/lms/envs/common.py index 3c133a1e6b..efed3f7c7f 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -72,6 +72,7 @@ MITX_FEATURES = { 'ENABLE_PSYCHOMETRICS': False, # real-time psychometrics (eg item response theory analysis in instructor dashboard) + 'ENABLE_DJANGO_ADMIN_SITE': False, # set true to enable django's admin site, even on prod (e.g. for course ops) 'ENABLE_SQL_TRACKING_LOGS': False, 'ENABLE_LMS_MIGRATION': False, 'ENABLE_MANUAL_GIT_RELOAD': False, From ea3506d2b983e59afbef723bc98780907fd6cdc1 Mon Sep 17 00:00:00 2001 From: Nate Hardison Date: Fri, 17 May 2013 14:39:59 -0700 Subject: [PATCH 33/37] Add general theming capabilities This commit adds the requisite settings and startup features to enable integration of themes into the edX platform. It does not yet provide hooks in any of the templates, but it does cause the main `lms/static/sass/application.scss` file to `@import` a theme's base Sass. Template hooks will come down the road. CHANGELOG --------- Define a new `MITX_FEATURE`, `USE_CUSTOM_THEME`, that when enabled, can be used in templates to determine whether or not custom theme templates should be used instead of the defaults. Also define a new setting, `THEME_NAME`, which will be used to locate theme-specific files. Establish the convention that themes will be stored outside of the `REPO_ROOT`, inside the `ENV_ROOT`, in a directory named `themes/`. `themes/` will store the files for a particular theme. Provide a function, `enable_theme`, that modifies the template and static asset load paths appropriately to include the theme's files. Move the main LMS Sass file to a Mako template that conditionally `@import`s the theme's base Sass file when a theme is enabled. Add logic to the assets Rakefile to properly preprocess any Sass/ Mako templates before compiling them. --- .gitignore | 1 + lms/envs/aws.py | 5 ++ lms/envs/common.py | 35 +++++++++++++- ...application.scss => application.scss.mako} | 13 +++++ rakefile | 8 ++++ rakefiles/assets.rake | 47 ++++++++++++++++++- 6 files changed, 105 insertions(+), 4 deletions(-) rename lms/static/sass/{application.scss => application.scss.mako} (63%) diff --git a/.gitignore b/.gitignore index 9c82bb8ea9..05e76c4caa 100644 --- a/.gitignore +++ b/.gitignore @@ -26,6 +26,7 @@ Gemfile.lock conf/locale/en/LC_MESSAGES/*.po !messages.po lms/static/sass/*.css +lms/static/sass/application.scss cms/static/sass/*.css lms/lib/comment_client/python nosetests.xml diff --git a/lms/envs/aws.py b/lms/envs/aws.py index df80d5a924..a3d5cb653f 100644 --- a/lms/envs/aws.py +++ b/lms/envs/aws.py @@ -114,6 +114,11 @@ DEFAULT_FEEDBACK_EMAIL = ENV_TOKENS.get('DEFAULT_FEEDBACK_EMAIL', DEFAULT_FEEDBA ADMINS = ENV_TOKENS.get('ADMINS', ADMINS) SERVER_EMAIL = ENV_TOKENS.get('SERVER_EMAIL', SERVER_EMAIL) +#Theme overrides +THEME_NAME = ENV_TOKENS.get('THEME_NAME', None) +if not THEME_NAME is None: + enable_theme(THEME_NAME) + #Timezone overrides TIME_ZONE = ENV_TOKENS.get('TIME_ZONE', TIME_ZONE) diff --git a/lms/envs/common.py b/lms/envs/common.py index 3c133a1e6b..64012d1f53 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -110,6 +110,9 @@ MITX_FEATURES = { # Enable URL that shows information about the status of variuous services 'ENABLE_SERVICE_STATUS': False, + + # Toggle to indicate use of a custom theme + 'USE_CUSTOM_THEME': False } # Used for A/B testing @@ -167,12 +170,12 @@ MAKO_TEMPLATES['main'] = [PROJECT_ROOT / 'templates', # This is where Django Template lookup is defined. There are a few of these # still left lying around. -TEMPLATE_DIRS = ( +TEMPLATE_DIRS = [ PROJECT_ROOT / "templates", COMMON_ROOT / 'templates', COMMON_ROOT / 'lib' / 'capa' / 'capa' / 'templates', COMMON_ROOT / 'djangoapps' / 'pipeline_mako' / 'templates', -) +] TEMPLATE_CONTEXT_PROCESSORS = ( 'django.core.context_processors.request', @@ -714,3 +717,31 @@ MKTG_URL_LINK_MAP = { 'HONOR': 'honor', 'PRIVACY': 'privacy_edx', } + +############################### THEME ################################ +def enable_theme(theme_name): + """ + Enable the settings for a custom theme, whose files should be stored + in ENV_ROOT/themes/THEME_NAME (e.g., edx_all/themes/stanford). + + The THEME_NAME setting should be configured separately since it can't + be set here (this function closes too early). An idiom for doing this + is: + + THEME_NAME = "stanford" + enable_theme(THEME_NAME) + """ + MITX_FEATURES['USE_CUSTOM_THEME'] = True + + # Calculate the location of the theme's files + theme_root = ENV_ROOT / "themes" / theme_name + + # Include the theme's templates in the template search paths + TEMPLATE_DIRS.append(theme_root / 'templates') + MAKO_TEMPLATES['main'].append(theme_root / 'templates') + + # Namespace the theme's static files to 'themes/' to + # avoid collisions with default edX static files + STATICFILES_DIRS.append((u'themes/%s' % theme_name, + theme_root / 'static')) + diff --git a/lms/static/sass/application.scss b/lms/static/sass/application.scss.mako similarity index 63% rename from lms/static/sass/application.scss rename to lms/static/sass/application.scss.mako index 6a1ef8743e..434475c6ac 100644 --- a/lms/static/sass/application.scss +++ b/lms/static/sass/application.scss.mako @@ -36,3 +36,16 @@ @import 'news'; @import 'shame'; + +## THEMING +## ------- +## Set up this file to import an edX theme library if the environment +## indicates that a theme should be used. The assumption is that the +## theme resides outside of this main edX repository, in a directory +## called themes//, with its base Sass file in +## themes//static/sass/_.scss. That one entry +## point can be used to @import in as many other things as needed. +% if not env['THEME_NAME'] is None: + // import theme's Sass overrides + @import '${env['THEME_NAME']}' +% endif diff --git a/rakefile b/rakefile index cef93e67eb..20101a14db 100644 --- a/rakefile +++ b/rakefile @@ -1,3 +1,4 @@ +require 'json' require 'rake/clean' require './rakefiles/helpers.rb' @@ -7,6 +8,13 @@ end # Build Constants REPO_ROOT = File.dirname(__FILE__) +ENV_ROOT = File.dirname(REPO_ROOT) REPORT_DIR = File.join(REPO_ROOT, "reports") +# Environment constants +SERVICE_VARIANT = ENV['SERVICE_VARIANT'] +CONFIG_PREFIX = SERVICE_VARIANT ? SERVICE_VARIANT + "." : "" +ENV_FILE = File.join(ENV_ROOT, CONFIG_PREFIX + "env.json") +ENV_TOKENS = File.exists?(ENV_FILE) ? JSON.parse(File.read(ENV_FILE)) : {} + task :default => [:test, :pep8, :pylint] diff --git a/rakefiles/assets.rake b/rakefiles/assets.rake index 0369968fdf..ef77d4fea7 100644 --- a/rakefiles/assets.rake +++ b/rakefiles/assets.rake @@ -1,3 +1,36 @@ +# Theming constants +THEME_NAME = ENV_TOKENS['THEME_NAME'] +USE_CUSTOM_THEME = !(THEME_NAME.nil? || THEME_NAME.empty?) +if USE_CUSTOM_THEME + THEME_ROOT = File.join(ENV_ROOT, "themes", THEME_NAME) + THEME_SASS = File.join(THEME_ROOT, "static", "sass") +end + +# Run the specified file through the Mako templating engine, providing +# the ENV_TOKENS to the templating context. +def preprocess_with_mako(filename) + # simple command-line invocation of Mako engine + mako = "from mako.template import Template;" + + "print Template(filename=\"#{filename}\")" + + # Total hack. It works because a Python dict literal has + # the same format as a JSON object. + ".render(env=#{ENV_TOKENS.to_json});" + + # strip off the .mako extension + output_filename = filename.chomp(File.extname(filename)) + + # just pipe from stdout into the new file + File.open(output_filename, 'w') do |file| + file.write(`python -c '#{mako}'`) + end +end + +# Preprocess all static assets that have the .mako extension by +# running them through the Mako templating engine. Right now we +# just hardcode the asset filenames. +def preprocess_assets + preprocess_with_mako("lms/static/sass/application.scss.mako") +end def xmodule_cmd(watch=false, debug=false) xmodule_cmd = 'xmodule_assets common/static/xmodule' @@ -32,10 +65,20 @@ def coffee_cmd(watch=false, debug=false) end def sass_cmd(watch=false, debug=false) + # Make sure to preprocess templatized Sass first + preprocess_assets() + + sass_load_paths = ["./common/static/sass"] + sass_watch_paths = ["*/static"] + if USE_CUSTOM_THEME + sass_load_paths << THEME_SASS + sass_watch_paths << THEME_SASS + end + "sass #{debug ? '--debug-info' : '--style compressed'} " + - "--load-path ./common/static/sass " + + "--load-path #{sass_load_paths.join(' ')} " + "--require ./common/static/sass/bourbon/lib/bourbon.rb " + - "#{watch ? '--watch' : '--update'} */static" + "#{watch ? '--watch' : '--update'} #{sass_watch_paths.join(' ')}" end desc "Compile all assets" From 185ab4c35cac281ca1ab5fd4cbed92dc02a1e35d Mon Sep 17 00:00:00 2001 From: Nate Hardison Date: Tue, 21 May 2013 10:43:58 -0700 Subject: [PATCH 34/37] Make Mako preprocessing a Rake task Per feedback from @cpennington. This way we'll only preprocess once, even if the Sass compilation is invoked multiple times. --- rakefiles/assets.rake | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/rakefiles/assets.rake b/rakefiles/assets.rake index ef77d4fea7..b7edaa1b02 100644 --- a/rakefiles/assets.rake +++ b/rakefiles/assets.rake @@ -25,13 +25,6 @@ def preprocess_with_mako(filename) end end -# Preprocess all static assets that have the .mako extension by -# running them through the Mako templating engine. Right now we -# just hardcode the asset filenames. -def preprocess_assets - preprocess_with_mako("lms/static/sass/application.scss.mako") -end - def xmodule_cmd(watch=false, debug=false) xmodule_cmd = 'xmodule_assets common/static/xmodule' if watch @@ -65,9 +58,6 @@ def coffee_cmd(watch=false, debug=false) end def sass_cmd(watch=false, debug=false) - # Make sure to preprocess templatized Sass first - preprocess_assets() - sass_load_paths = ["./common/static/sass"] sass_watch_paths = ["*/static"] if USE_CUSTOM_THEME @@ -89,6 +79,13 @@ namespace :assets do desc "Compile all assets in debug mode" multitask :debug + desc "Preprocess all static assets that have the .mako extension" + task :preprocess do + # Run assets through the Mako templating engine. Right now we + # just hardcode the asset filenames. + preprocess_with_mako("lms/static/sass/application.scss.mako") + end + desc "Watch all assets for changes and automatically recompile" task :watch => 'assets:_watch' do puts "Press ENTER to terminate".red @@ -97,9 +94,9 @@ namespace :assets do {:xmodule => :install_python_prereqs, :coffee => :install_node_prereqs, - :sass => :install_ruby_prereqs}.each_pair do |asset_type, prereq_task| + :sass => [:install_ruby_prereqs, :preprocess]}.each_pair do |asset_type, prereq_tasks| desc "Compile all #{asset_type} assets" - task asset_type => prereq_task do + task asset_type => prereq_tasks do cmd = send(asset_type.to_s + "_cmd", watch=false, debug=false) if cmd.kind_of?(Array) cmd.each {|c| sh(c)} @@ -114,7 +111,7 @@ namespace :assets do namespace asset_type do desc "Compile all #{asset_type} assets in debug mode" - task :debug => prereq_task do + task :debug => prereq_tasks do cmd = send(asset_type.to_s + "_cmd", watch=false, debug=true) sh(cmd) end @@ -125,7 +122,7 @@ namespace :assets do $stdin.gets end - task :_watch => prereq_task do + task :_watch => prereq_tasks do cmd = send(asset_type.to_s + "_cmd", watch=true, debug=true) if cmd.kind_of?(Array) cmd.each {|c| background_process(c)} From a0b6a176f2b288b090595efde32fce58565dff9d Mon Sep 17 00:00:00 2001 From: Nate Hardison Date: Wed, 22 May 2013 14:49:22 -0700 Subject: [PATCH 35/37] Use .get instead of [] to access env hash If the `THEME_NAME` environment token isn't defined, then the application.scss.mako file will crash during preprocessing. Use .get to avoid a crash. --- lms/static/sass/application.scss.mako | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lms/static/sass/application.scss.mako b/lms/static/sass/application.scss.mako index 434475c6ac..4ec91dbf57 100644 --- a/lms/static/sass/application.scss.mako +++ b/lms/static/sass/application.scss.mako @@ -45,7 +45,7 @@ ## called themes//, with its base Sass file in ## themes//static/sass/_.scss. That one entry ## point can be used to @import in as many other things as needed. -% if not env['THEME_NAME'] is None: +% if not env.get('THEME_NAME') is None: // import theme's Sass overrides - @import '${env['THEME_NAME']}' + @import '${env.get('THEME_NAME')}' % endif From f826198214d569a0fe81aea15be96a1fb60e8373 Mon Sep 17 00:00:00 2001 From: Nate Hardison Date: Wed, 22 May 2013 19:34:14 -0700 Subject: [PATCH 36/37] Use "is not None" convention --- lms/static/sass/application.scss.mako | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lms/static/sass/application.scss.mako b/lms/static/sass/application.scss.mako index 4ec91dbf57..c310347b6f 100644 --- a/lms/static/sass/application.scss.mako +++ b/lms/static/sass/application.scss.mako @@ -45,7 +45,7 @@ ## called themes//, with its base Sass file in ## themes//static/sass/_.scss. That one entry ## point can be used to @import in as many other things as needed. -% if not env.get('THEME_NAME') is None: +% if env.get('THEME_NAME') is not None: // import theme's Sass overrides @import '${env.get('THEME_NAME')}' % endif From daa0ac2f018cbbc0d747982b754ae889f0cf4878 Mon Sep 17 00:00:00 2001 From: Nate Hardison Date: Wed, 22 May 2013 19:42:22 -0700 Subject: [PATCH 37/37] Capture Mako exit code and fail fast if necessary Capture the exit code of the Mako template engine invocation on asset preprocessing and abort from the Rake task on failure. This will prevent the LMS from continuing its attempt to start up, preventing further configuration errors. --- rakefiles/assets.rake | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/rakefiles/assets.rake b/rakefiles/assets.rake index b7edaa1b02..c0757b712b 100644 --- a/rakefiles/assets.rake +++ b/rakefiles/assets.rake @@ -19,9 +19,11 @@ def preprocess_with_mako(filename) # strip off the .mako extension output_filename = filename.chomp(File.extname(filename)) - # just pipe from stdout into the new file + # just pipe from stdout into the new file, exiting on failure File.open(output_filename, 'w') do |file| file.write(`python -c '#{mako}'`) + exit_code = $?.to_i + abort "#{mako} failed with #{exit_code}" if exit_code.to_i != 0 end end