From e76ef3aa31aaaa1f02a4f920084ce040777ec532 Mon Sep 17 00:00:00 2001 From: Will Daly Date: Tue, 18 Jun 2013 08:06:42 -0400 Subject: [PATCH 01/15] Combined video and videoalpha acceptance tests. Resolved conflict between two steps with the same name. --- .../courseware/features/video.feature | 10 ++++-- lms/djangoapps/courseware/features/video.py | 22 ++++++++++++ .../courseware/features/videoalpha.py | 36 ------------------- 3 files changed, 29 insertions(+), 39 deletions(-) delete mode 100644 lms/djangoapps/courseware/features/videoalpha.py diff --git a/lms/djangoapps/courseware/features/video.feature b/lms/djangoapps/courseware/features/video.feature index c4d96f93f7..2b8d0f013a 100644 --- a/lms/djangoapps/courseware/features/video.feature +++ b/lms/djangoapps/courseware/features/video.feature @@ -1,6 +1,10 @@ Feature: Video component As a student, I want to view course videos in LMS. - Scenario: Autoplay is enabled in LMS - Given the course has a Video component - Then when I view the video it has autoplay enabled + Scenario: Autoplay is enabled in LMS for a Video component + Given the course has a Video component + Then when I view the video it has autoplay enabled + + Scenario: Autoplay is enabled in the LMS for a VideoAlpha component + Given the course has a VideoAlpha component + Then when I view the video it has autoplay enabled diff --git a/lms/djangoapps/courseware/features/video.py b/lms/djangoapps/courseware/features/video.py index 8cef5564f3..745f0ae99a 100644 --- a/lms/djangoapps/courseware/features/video.py +++ b/lms/djangoapps/courseware/features/video.py @@ -27,8 +27,30 @@ def view_video(_step): world.browser.visit(url) +@step('the course has a VideoAlpha component') +def view_videoalpha(step): + coursename = TEST_COURSE_NAME.replace(' ', '_') + i_am_registered_for_the_course(step, coursename) + + # Make sure we have a videoalpha + add_videoalpha_to_course(coursename) + chapter_name = TEST_SECTION_NAME.replace(" ", "_") + section_name = chapter_name + url = django_url('/courses/edx/Test_Course/Test_Course/courseware/%s/%s' % + (chapter_name, section_name)) + + world.browser.visit(url) + + def add_video_to_course(course): template_name = 'i4x://edx/templates/video/default' world.ItemFactory.create(parent_location=section_location(course), template=template_name, display_name='Video') + + +def add_videoalpha_to_course(course): + template_name = 'i4x://edx/templates/videoalpha/default' + world.ItemFactory.create(parent_location=section_location(course), + template=template_name, + display_name='Video Alpha 1') diff --git a/lms/djangoapps/courseware/features/videoalpha.py b/lms/djangoapps/courseware/features/videoalpha.py deleted file mode 100644 index cabf8c681f..0000000000 --- a/lms/djangoapps/courseware/features/videoalpha.py +++ /dev/null @@ -1,36 +0,0 @@ -#pylint: disable=C0111 -#pylint: disable=W0613 -#pylint: disable=W0621 - -from lettuce import world, step -from lettuce.django import django_url -from common import TEST_COURSE_NAME, TEST_SECTION_NAME, i_am_registered_for_the_course, section_location - -############### ACTIONS #################### - - -@step('when I view the video it has autoplay enabled') -def does_autoplay(step): - assert(world.css_find('.videoalpha')[0]['data-autoplay'] == 'True') - - -@step('the course has a Video component') -def view_videoalpha(step): - coursename = TEST_COURSE_NAME.replace(' ', '_') - i_am_registered_for_the_course(step, coursename) - - # Make sure we have a videoalpha - add_videoalpha_to_course(coursename) - chapter_name = TEST_SECTION_NAME.replace(" ", "_") - section_name = chapter_name - url = django_url('/courses/edx/Test_Course/Test_Course/courseware/%s/%s' % - (chapter_name, section_name)) - - world.browser.visit(url) - - -def add_videoalpha_to_course(course): - template_name = 'i4x://edx/templates/videoalpha/default' - world.ItemFactory.create(parent_location=section_location(course), - template=template_name, - display_name='Video Alpha 1') From 3b37e0c19f13ab5dfacbdd197e6a50f7e1dc4bb0 Mon Sep 17 00:00:00 2001 From: Will Daly Date: Tue, 18 Jun 2013 08:35:56 -0400 Subject: [PATCH 02/15] Fixed incorrect videoalpha template name --- lms/djangoapps/courseware/features/video.py | 2 +- lms/djangoapps/courseware/features/videoalpha.feature | 6 ------ 2 files changed, 1 insertion(+), 7 deletions(-) delete mode 100644 lms/djangoapps/courseware/features/videoalpha.feature diff --git a/lms/djangoapps/courseware/features/video.py b/lms/djangoapps/courseware/features/video.py index 745f0ae99a..90f68c1daf 100644 --- a/lms/djangoapps/courseware/features/video.py +++ b/lms/djangoapps/courseware/features/video.py @@ -50,7 +50,7 @@ def add_video_to_course(course): def add_videoalpha_to_course(course): - template_name = 'i4x://edx/templates/videoalpha/default' + template_name = 'i4x://edx/templates/videoalpha/Video_Alpha_1' world.ItemFactory.create(parent_location=section_location(course), template=template_name, display_name='Video Alpha 1') diff --git a/lms/djangoapps/courseware/features/videoalpha.feature b/lms/djangoapps/courseware/features/videoalpha.feature deleted file mode 100644 index 2a0acb0f9b..0000000000 --- a/lms/djangoapps/courseware/features/videoalpha.feature +++ /dev/null @@ -1,6 +0,0 @@ -Feature: Video Alpha component - As a student, I want to view course videos in LMS. - - Scenario: Autoplay is enabled in LMS - Given the course has a Video component - Then when I view the video it has autoplay enabled From 6ab5bb2f205953070c9c9099352761acf5239419 Mon Sep 17 00:00:00 2001 From: Will Daly Date: Tue, 18 Jun 2013 08:47:00 -0400 Subject: [PATCH 03/15] Changed videoalpha stub to exclude video sources, but include the rest of the player. --- lms/templates/videoalpha.html | 56 +++++++++++++++++------------------ 1 file changed, 28 insertions(+), 28 deletions(-) diff --git a/lms/templates/videoalpha.html b/lms/templates/videoalpha.html index 07c7dbee27..4e136bd170 100644 --- a/lms/templates/videoalpha.html +++ b/lms/templates/videoalpha.html @@ -2,34 +2,34 @@

${display_name}

% endif -%if settings.MITX_FEATURES['STUB_VIDEO_FOR_TESTING']: -
-%else: -
-
-
-
-
-
-
-
-
-
-%endif +
+
+
+
+
+
+
+
+
+
% if sources.get('main'):
From b1c963ab5e35c7999ee43e949876000010421d39 Mon Sep 17 00:00:00 2001 From: Will Daly Date: Tue, 18 Jun 2013 09:02:21 -0400 Subject: [PATCH 04/15] Changed stubbing behavior in video templates to exclude only the sources (not the player) --- lms/templates/video.html | 48 ++++++++++++++++------------------- lms/templates/videoalpha.html | 4 +++ 2 files changed, 26 insertions(+), 26 deletions(-) diff --git a/lms/templates/video.html b/lms/templates/video.html index 267372176a..91b5f63b81 100644 --- a/lms/templates/video.html +++ b/lms/templates/video.html @@ -2,37 +2,33 @@

${display_name}

% endif -%if settings.MITX_FEATURES['STUB_VIDEO_FOR_TESTING']: -
-
-
-
-
-
    -
    • -
  • -
    0:00 / 0:00
    -
    • -
-
- Fill Browser -
-
-
-
-
-%elif settings.MITX_FEATURES.get('USE_YOUTUBE_OBJECT_API') and normal_speed_video_id: +%if settings.MITX_FEATURES.get('USE_YOUTUBE_OBJECT_API') and normal_speed_video_id: + % if not settings.MITX_FEATURES['STUB_VIDEO_FOR_TESTING']: + value="https://www.youtube.com/v/${normal_speed_video_id}?version=3&autoplay=1&rel=0"> + % endif + - + %else: -
+
diff --git a/lms/templates/videoalpha.html b/lms/templates/videoalpha.html index 4e136bd170..2bb5d817a8 100644 --- a/lms/templates/videoalpha.html +++ b/lms/templates/videoalpha.html @@ -5,7 +5,11 @@
Date: Thu, 20 Jun 2013 07:44:33 -0400 Subject: [PATCH 05/15] Updated video alpha template name to reflect recent changes in master. --- lms/djangoapps/courseware/features/video.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lms/djangoapps/courseware/features/video.py b/lms/djangoapps/courseware/features/video.py index 90f68c1daf..cd1bdcf60f 100644 --- a/lms/djangoapps/courseware/features/video.py +++ b/lms/djangoapps/courseware/features/video.py @@ -50,7 +50,7 @@ def add_video_to_course(course): def add_videoalpha_to_course(course): - template_name = 'i4x://edx/templates/videoalpha/Video_Alpha_1' + template_name = 'i4x://edx/templates/videoalpha/Video_Alpha' world.ItemFactory.create(parent_location=section_location(course), template=template_name, - display_name='Video Alpha 1') + display_name='Video Alpha') From 83af3e594f561c4f1c3c35b59f34089b2843a887 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Mon, 17 Jun 2013 13:49:40 -0400 Subject: [PATCH 06/15] Use built-in rakelibdir feature Rake by default imports all .rake files in the rakelib dir, so we can use that rather than doing our own import loop. --- rakefile | 6 +----- {rakefiles => rakelib}/assets.rake | 0 {rakefiles => rakelib}/deploy.rake | 0 {rakefiles => rakelib}/deprecated.rake | 0 {rakefiles => rakelib}/django.rake | 0 {rakefiles => rakelib}/docs.rake | 0 {rakefiles => rakelib}/helpers.rb | 0 {rakefiles => rakelib}/i18n.rake | 0 {rakefiles => rakelib}/jasmine.rake | 0 {rakefiles => rakelib}/prereqs.rake | 2 -- {rakefiles => rakelib}/quality.rake | 0 {rakefiles => rakelib}/tests.rake | 0 {rakefiles => rakelib}/workspace.rake | 0 13 files changed, 1 insertion(+), 7 deletions(-) rename {rakefiles => rakelib}/assets.rake (100%) rename {rakefiles => rakelib}/deploy.rake (100%) rename {rakefiles => rakelib}/deprecated.rake (100%) rename {rakefiles => rakelib}/django.rake (100%) rename {rakefiles => rakelib}/docs.rake (100%) rename {rakefiles => rakelib}/helpers.rb (100%) rename {rakefiles => rakelib}/i18n.rake (100%) rename {rakefiles => rakelib}/jasmine.rake (100%) rename {rakefiles => rakelib}/prereqs.rake (98%) rename {rakefiles => rakelib}/quality.rake (100%) rename {rakefiles => rakelib}/tests.rake (100%) rename {rakefiles => rakelib}/workspace.rake (100%) diff --git a/rakefile b/rakefile index 20101a14db..3fcd16f995 100644 --- a/rakefile +++ b/rakefile @@ -1,10 +1,6 @@ require 'json' require 'rake/clean' -require './rakefiles/helpers.rb' - -Dir['rakefiles/*.rake'].each do |rakefile| - import rakefile -end +require './rakelib/helpers.rb' # Build Constants REPO_ROOT = File.dirname(__FILE__) diff --git a/rakefiles/assets.rake b/rakelib/assets.rake similarity index 100% rename from rakefiles/assets.rake rename to rakelib/assets.rake diff --git a/rakefiles/deploy.rake b/rakelib/deploy.rake similarity index 100% rename from rakefiles/deploy.rake rename to rakelib/deploy.rake diff --git a/rakefiles/deprecated.rake b/rakelib/deprecated.rake similarity index 100% rename from rakefiles/deprecated.rake rename to rakelib/deprecated.rake diff --git a/rakefiles/django.rake b/rakelib/django.rake similarity index 100% rename from rakefiles/django.rake rename to rakelib/django.rake diff --git a/rakefiles/docs.rake b/rakelib/docs.rake similarity index 100% rename from rakefiles/docs.rake rename to rakelib/docs.rake diff --git a/rakefiles/helpers.rb b/rakelib/helpers.rb similarity index 100% rename from rakefiles/helpers.rb rename to rakelib/helpers.rb diff --git a/rakefiles/i18n.rake b/rakelib/i18n.rake similarity index 100% rename from rakefiles/i18n.rake rename to rakelib/i18n.rake diff --git a/rakefiles/jasmine.rake b/rakelib/jasmine.rake similarity index 100% rename from rakefiles/jasmine.rake rename to rakelib/jasmine.rake diff --git a/rakefiles/prereqs.rake b/rakelib/prereqs.rake similarity index 98% rename from rakefiles/prereqs.rake rename to rakelib/prereqs.rake index ff8b4b8784..e06d411435 100644 --- a/rakefiles/prereqs.rake +++ b/rakelib/prereqs.rake @@ -1,5 +1,3 @@ -require './rakefiles/helpers.rb' - PREREQS_MD5_DIR = ENV["PREREQ_CACHE_DIR"] || File.join(REPO_ROOT, '.prereqs_cache') CLOBBER.include(PREREQS_MD5_DIR) diff --git a/rakefiles/quality.rake b/rakelib/quality.rake similarity index 100% rename from rakefiles/quality.rake rename to rakelib/quality.rake diff --git a/rakefiles/tests.rake b/rakelib/tests.rake similarity index 100% rename from rakefiles/tests.rake rename to rakelib/tests.rake diff --git a/rakefiles/workspace.rake b/rakelib/workspace.rake similarity index 100% rename from rakefiles/workspace.rake rename to rakelib/workspace.rake From d9268acd6bb9b19175315998de623818e0a799f1 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Mon, 17 Jun 2013 13:52:00 -0400 Subject: [PATCH 07/15] Provide instructions of ruby imports fail in rake `rake install_prereqs` requires a minimal level of ruby and rake already installed. If it doesn't exist, then print out a helpful message indicating next steps. --- CHANGELOG.rst | 2 ++ rakefile | 12 +++++++++--- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 7fc07a3f19..9c7af1520b 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -5,6 +5,8 @@ These are notable changes in edx-platform. This is a rolling list of changes, in roughly chronological order, most recent first. Add your entries at or near the top. Include a label indicating the component affected. +Common: Make rake provide better error messages if packages are missing. + Common: Repairs development documentation generation by sphinx. LMS: Problem rescoring. Added options on the Grades tab of the diff --git a/rakefile b/rakefile index 3fcd16f995..96bd4c2e96 100644 --- a/rakefile +++ b/rakefile @@ -1,6 +1,12 @@ -require 'json' -require 'rake/clean' -require './rakelib/helpers.rb' +begin + require 'json' + require 'rake/clean' + require './rakelib/helpers.rb' +rescue LoadError => error + puts "Import faild (#{error})" + puts "Please run `bundle install` to bootstrap ruby dependencies" + exit 1 +end # Build Constants REPO_ROOT = File.dirname(__FILE__) From d99ad53ae90bebc0fb2c099b42a2ffa1c64ab4b8 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 19 Jun 2013 15:30:47 -0400 Subject: [PATCH 08/15] Add system and env arguments to asset tasks The preprocess task requires system and env arguments in order to correctly load up the django environment to preprocess sass files to inject themes. In order for that task to recieve the arguments, all tasks that depend on it also have to accept that same set of arguments. This will all go away once the next evolution of themes arrives, which will remove the preprocessing needed to inject theme names. --- rakelib/assets.rake | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/rakelib/assets.rake b/rakelib/assets.rake index 764d049a68..0c58047bc2 100644 --- a/rakelib/assets.rake +++ b/rakelib/assets.rake @@ -55,8 +55,9 @@ def sass_cmd(watch=false, debug=false) "#{watch ? '--watch' : '--update'} -E utf-8 #{sass_watch_paths.join(' ')}" end +# This task takes arguments purely to pass them via dependencies to the preprocess task desc "Compile all assets" -multitask :assets => 'assets:all' +task :assets, [:system, :env] => 'assets:all' namespace :assets do @@ -80,8 +81,9 @@ namespace :assets do {:xmodule => [:install_python_prereqs], :coffee => [:install_node_prereqs, :'assets:coffee:clobber'], :sass => [:install_ruby_prereqs, :preprocess]}.each_pair do |asset_type, prereq_tasks| + # This task takes arguments purely to pass them via dependencies to the preprocess task desc "Compile all #{asset_type} assets" - task asset_type => prereq_tasks do + task asset_type, [:system, :env] => prereq_tasks do |t, args| cmd = send(asset_type.to_s + "_cmd", watch=false, debug=false) if cmd.kind_of?(Array) cmd.each {|c| sh(c)} @@ -90,7 +92,8 @@ namespace :assets do end end - multitask :all => asset_type + # This task takes arguments purely to pass them via dependencies to the preprocess task + multitask :all, [:system, :env] => asset_type multitask :debug => "assets:#{asset_type}:debug" multitask :_watch => "assets:#{asset_type}:_watch" From 2e480f6404b2ae640623266a4cbd9f91dfa067c6 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Mon, 17 Jun 2013 15:00:55 -0400 Subject: [PATCH 09/15] Switch to standard coffee watcher Using `ulimit -n` to set the limit much higher than the default of 256 in Darwin seems to avoid the `EMFILE` error that was plaguing our Mac developers. --- CHANGELOG.rst | 2 ++ doc/development.md | 19 +++++++++++++++++++ rakelib/assets.rake | 26 +++++++++----------------- 3 files changed, 30 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 9c7af1520b..99accb83cb 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -5,6 +5,8 @@ These are notable changes in edx-platform. This is a rolling list of changes, in roughly chronological order, most recent first. Add your entries at or near the top. Include a label indicating the component affected. +Common: Use coffee directly when watching for coffeescript file changes. + Common: Make rake provide better error messages if packages are missing. Common: Repairs development documentation generation by sphinx. diff --git a/doc/development.md b/doc/development.md index c99e99f906..e6ab650002 100644 --- a/doc/development.md +++ b/doc/development.md @@ -63,6 +63,25 @@ To get a full list of available rake tasks, use: rake -T +### Troubleshooting + +#### Reference Error: XModule is not defined (javascript) +This means that the javascript defining an xmodule hasn't loaded correctly. There are a number +of different things that could be causing this: + +1. See `Error: watch EMFILE` + +#### Error: watch EMFILE (coffee) +When running a development server, we also start a watcher process alongside to recompile coffeescript +and sass as changes are made. On Mac OSX systems, the coffee watcher process takes more file handles +than are allowed by default. This will result in `EMFILE` errors when coffeescript is running, and +will prevent javascript from compiling, leading to the error 'XModule is not defined' + +To work around this issue, we use `Process::setrlimit` to set the number of allowed open files. +Coffee watches both directories and files, so you will need to set this fairly high (anecdotally, +8000 seems to do the trick on OSX 10.7.5, 10.8.3, and 10.8.4) + + ## Running Tests See `testing.md` for instructions on running the test suite. diff --git a/rakelib/assets.rake b/rakelib/assets.rake index 0c58047bc2..10dfb14f18 100644 --- a/rakelib/assets.rake +++ b/rakelib/assets.rake @@ -6,6 +6,8 @@ if USE_CUSTOM_THEME THEME_SASS = File.join(THEME_ROOT, "static", "sass") end +MINIMAL_DARWIN_NOFILE_LIMIT = 8000 + def xmodule_cmd(watch=false, debug=false) xmodule_cmd = 'xmodule_assets common/static/xmodule' if watch @@ -21,24 +23,14 @@ def xmodule_cmd(watch=false, debug=false) end def coffee_cmd(watch=false, debug=false) - if watch - # On OSx, coffee fails with EMFILE when - # trying to watch all of our coffee files at the same - # time. - # - # Ref: https://github.com/joyent/node/issues/2479 - # - # So, instead, we use watchmedo, which works around the problem - "watchmedo shell-command " + - "--command 'node_modules/.bin/coffee -c ${watch_src_path}' " + - "--recursive " + - "--patterns '*.coffee' " + - "--ignore-directories " + - "--wait " + - "." - else - 'node_modules/.bin/coffee --compile .' + if watch && Launchy::Application.new.host_os_family.darwin? + available_files = Process::getrlimit(:NOFILE)[0] + if available_files < MINIMAL_DARWIN_NOFILE_LIMIT + Process.setrlimit(:NOFILE, MINIMAL_DARWIN_NOFILE_LIMIT) + + end end + "node_modules/.bin/coffee --compile #{watch ? '--watch' : ''} ." end def sass_cmd(watch=false, debug=false) From f4200127bcc91fc30eca2f727a21c6a71139fbfd Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Fri, 14 Jun 2013 21:46:27 -0400 Subject: [PATCH 10/15] Make coffee watch message more informative When running under watchmedo, coffee doesn't display any useful information when it recompiles a changed file, so we make watchmedo echo that information instead. --- rakelib/assets.rake | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rakelib/assets.rake b/rakelib/assets.rake index 0c58047bc2..c80b275c27 100644 --- a/rakelib/assets.rake +++ b/rakelib/assets.rake @@ -30,7 +30,7 @@ def coffee_cmd(watch=false, debug=false) # # So, instead, we use watchmedo, which works around the problem "watchmedo shell-command " + - "--command 'node_modules/.bin/coffee -c ${watch_src_path}' " + + "--command 'echo \">>> Change detected to ${watch_src_path}\" && node_modules/.bin/coffee -c ${watch_src_path}' " + "--recursive " + "--patterns '*.coffee' " + "--ignore-directories " + From eb3e94660bf166b75a34601549b7093507c46bf7 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Fri, 14 Jun 2013 21:47:22 -0400 Subject: [PATCH 11/15] Don't delete generated files from xmodule-assets xmodule-assets creates coffeescript files in the output directories. On its next run, it used to delete the javascript files compiled from those coffee files. Now it doesn't which should make coffee have to do less work. Fixes LMS-451 --- CHANGELOG.rst | 4 ++++ common/lib/xmodule/xmodule/static_content.py | 16 ++++++++++++---- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 9c7af1520b..fbc007949c 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -5,6 +5,10 @@ These are notable changes in edx-platform. This is a rolling list of changes, in roughly chronological order, most recent first. Add your entries at or near the top. Include a label indicating the component affected. +XModule: Don't delete generated xmodule asset files when compiling (for +instance, when XModule provides a coffeescript file, don't delete +the associated javascript) + Common: Make rake provide better error messages if packages are missing. Common: Repairs development documentation generation by sphinx. diff --git a/common/lib/xmodule/xmodule/static_content.py b/common/lib/xmodule/xmodule/static_content.py index 4c4827e0aa..42fef65b11 100755 --- a/common/lib/xmodule/xmodule/static_content.py +++ b/common/lib/xmodule/xmodule/static_content.py @@ -121,15 +121,23 @@ def _write_js(output_root, classes): type=filetype) contents[filename] = fragment - _write_files(output_root, contents) + _write_files(output_root, contents, {'.coffee': '.js'}) return [output_root / filename for filename in contents.keys()] -def _write_files(output_root, contents): +def _write_files(output_root, contents, generated_suffix_map=None): _ensure_dir(output_root) - for extra_file in set(output_root.files()) - set(contents.keys()): - extra_file.remove_p() + to_delete = set(file.basename() for file in output_root.files()) - set(contents.keys()) + + if generated_suffix_map: + for output_file in contents.keys(): + for suffix, generated_suffix in generated_suffix_map.items(): + if output_file.endswith(suffix): + to_delete.discard(output_file.replace(suffix, generated_suffix)) + + for extra_file in to_delete: + (output_root / extra_file).remove_p() for filename, file_content in contents.iteritems(): (output_root / filename).write_bytes(file_content) From cd57e281f5eab2d7616e47d4d9b95adf46efda1e Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Fri, 14 Jun 2013 22:27:24 -0400 Subject: [PATCH 12/15] Only write to xmodule asset files that have changed Use md5 checksumming to verify that we only write out xmodule asset files whose contents differ from what we are about to write. This minimizes thrashing of the other watchers. Fixes LMS-452 --- CHANGELOG.rst | 2 ++ common/lib/xmodule/xmodule/static_content.py | 12 +++++++++++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index fbc007949c..d74816990b 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -5,6 +5,8 @@ These are notable changes in edx-platform. This is a rolling list of changes, in roughly chronological order, most recent first. Add your entries at or near the top. Include a label indicating the component affected. +XModule: Only write out assets files if the contents have changed. + XModule: Don't delete generated xmodule asset files when compiling (for instance, when XModule provides a coffeescript file, don't delete the associated javascript) diff --git a/common/lib/xmodule/xmodule/static_content.py b/common/lib/xmodule/xmodule/static_content.py index 42fef65b11..7662499c16 100755 --- a/common/lib/xmodule/xmodule/static_content.py +++ b/common/lib/xmodule/xmodule/static_content.py @@ -4,6 +4,7 @@ This module has utility functions for gathering up the static content that is defined by XModules and XModuleDescriptors (javascript and css) """ +import logging import hashlib import os import errno @@ -15,6 +16,9 @@ from path import path from xmodule.x_module import XModuleDescriptor +LOG = logging.getLogger(__name__) + + def write_module_styles(output_root): return _write_styles('.xmodule_display', output_root, _list_modules()) @@ -140,7 +144,13 @@ def _write_files(output_root, contents, generated_suffix_map=None): (output_root / extra_file).remove_p() for filename, file_content in contents.iteritems(): - (output_root / filename).write_bytes(file_content) + output_file = output_root / filename + + if not output_file.isfile() or output_file.read_md5() != hashlib.md5(file_content).digest(): + LOG.debug("Writing %s", output_file) + output_file.write_bytes(file_content) + else: + LOG.debug("%s unchanged, skipping", output_file) def main(): From 64912a774199e40a68cab3020d3e98ad9bb8c6e5 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Fri, 14 Jun 2013 21:17:08 -0400 Subject: [PATCH 13/15] Make assets watchers run as singletons Previously, multiple copies of the watchers started from the different shells would run simultaneously, which left the possiblity of zombie watchers, increased resource consumption, and incorrect results. This fixes that problem by only starting a watcher if that same command isn't already in the process list. Fixes LMS-499 --- CHANGELOG.rst | 3 +++ Gemfile | 1 + rakelib/assets.rake | 4 ++-- rakelib/helpers.rb | 12 ++++++++++++ 4 files changed, 18 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 9c7af1520b..18855e82ae 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -5,6 +5,9 @@ These are notable changes in edx-platform. This is a rolling list of changes, in roughly chronological order, most recent first. Add your entries at or near the top. Include a label indicating the component affected. +Common: Make asset watchers run as singletons (so they won't start if the +watcher is already running in another shell). + Common: Make rake provide better error messages if packages are missing. Common: Repairs development documentation generation by sphinx. diff --git a/Gemfile b/Gemfile index 7f7b146978..1ad685c34d 100644 --- a/Gemfile +++ b/Gemfile @@ -4,3 +4,4 @@ gem 'sass', '3.1.15' gem 'bourbon', '~> 1.3.6' gem 'colorize', '~> 0.5.8' gem 'launchy', '~> 2.1.2' +gem 'sys-proctable', '~> 0.9.3' diff --git a/rakelib/assets.rake b/rakelib/assets.rake index 0c58047bc2..c935b0d53b 100644 --- a/rakelib/assets.rake +++ b/rakelib/assets.rake @@ -114,9 +114,9 @@ namespace :assets do task :_watch => (prereq_tasks + ["assets:#{asset_type}:debug"]) do cmd = send(asset_type.to_s + "_cmd", watch=true, debug=true) if cmd.kind_of?(Array) - cmd.each {|c| background_process(c)} + cmd.each {|c| singleton_process(c)} else - background_process(cmd) + singleton_process(cmd) end end end diff --git a/rakelib/helpers.rb b/rakelib/helpers.rb index 4b10bef709..3373214a19 100644 --- a/rakelib/helpers.rb +++ b/rakelib/helpers.rb @@ -1,4 +1,6 @@ require 'digest/md5' +require 'sys/proctable' +require 'colorize' def find_executable(exec) path = %x(which #{exec}).strip @@ -84,6 +86,16 @@ def background_process(*command) end end +# Runs a command as a background process, as long as no other processes +# tagged with the same tag are running +def singleton_process(*command) + if Sys::ProcTable.ps.select {|proc| proc.cmdline.include?(command.join(' '))}.empty? + background_process(*command) + else + puts "Process '#{command.join(' ')} already running, skipping".blue + end +end + def environments(system) Dir["#{system}/envs/**/*.py"].select{|file| ! (/__init__.py$/ =~ file)}.map do |env_file| env_file.gsub("#{system}/envs/", '').gsub(/\.py/, '').gsub('/', '.') From c00721bbe6dd341590bea992e93803b98291c3e1 Mon Sep 17 00:00:00 2001 From: Felix Sun Date: Tue, 18 Jun 2013 13:10:47 -0400 Subject: [PATCH 14/15] Fixed the preferences scope of xblock. Added self to authors. Conflicts: AUTHORS CHANGELOG.rst --- AUTHORS | 1 + CHANGELOG.rst | 2 ++ lms/djangoapps/courseware/model_data.py | 2 +- lms/djangoapps/courseware/tests/factories.py | 2 +- lms/djangoapps/courseware/tests/test_model_data.py | 7 ++++++- 5 files changed, 11 insertions(+), 3 deletions(-) diff --git a/AUTHORS b/AUTHORS index 1af7349491..9bb4ede121 100644 --- a/AUTHORS +++ b/AUTHORS @@ -77,3 +77,4 @@ Slater Victoroff Peter Fogg Bethany LaPenta Renzo Lucioni +Felix Sun diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 9c405ed365..206be44c87 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -27,6 +27,8 @@ students' number of attempts to zero. Provides a list of background tasks that are currently running for the course, and an option to see a history of background tasks for a given problem. +LMS: Fixed the preferences scope for storing data in xmodules. + LMS: Forums. Added handling for case where discussion module can get `None` as value of lms.start in `lms/djangoapps/django_comment_client/utils.py` diff --git a/lms/djangoapps/courseware/model_data.py b/lms/djangoapps/courseware/model_data.py index f363546af0..790f1fd721 100644 --- a/lms/djangoapps/courseware/model_data.py +++ b/lms/djangoapps/courseware/model_data.py @@ -163,7 +163,7 @@ class ModelDataCache(object): return self._chunked_query( XModuleStudentPrefsField, 'module_type__in', - set(descriptor.location.category for descriptor in self.descriptors), + set(descriptor.module_class.__name__ for descriptor in self.descriptors), student=self.user.pk, field_name__in=set(field.name for field in fields), ) diff --git a/lms/djangoapps/courseware/tests/factories.py b/lms/djangoapps/courseware/tests/factories.py index 26df68ca7e..69f8f54eec 100644 --- a/lms/djangoapps/courseware/tests/factories.py +++ b/lms/djangoapps/courseware/tests/factories.py @@ -75,7 +75,7 @@ class StudentPrefsFactory(DjangoModelFactory): field_name = 'existing_field' value = json.dumps('old_value') student = SubFactory(UserFactory) - module_type = 'problem' + module_type = 'MockProblemModule' class StudentInfoFactory(DjangoModelFactory): diff --git a/lms/djangoapps/courseware/tests/test_model_data.py b/lms/djangoapps/courseware/tests/test_model_data.py index 9f225f73bd..e961f80939 100644 --- a/lms/djangoapps/courseware/tests/test_model_data.py +++ b/lms/djangoapps/courseware/tests/test_model_data.py @@ -29,6 +29,7 @@ def mock_descriptor(fields=[], lms_fields=[]): descriptor.location = location('def_id') descriptor.module_class.fields = fields descriptor.module_class.lms.fields = lms_fields + descriptor.module_class.__name__ = 'MockProblemModule' return descriptor location = partial(Location, 'i4x', 'edX', 'test_course', 'problem') @@ -37,7 +38,7 @@ course_id = 'edX/test_course/test' content_key = partial(LmsKeyValueStore.Key, Scope.content, None, location('def_id')) settings_key = partial(LmsKeyValueStore.Key, Scope.settings, None, location('def_id')) user_state_key = partial(LmsKeyValueStore.Key, Scope.user_state, 'user', location('def_id')) -prefs_key = partial(LmsKeyValueStore.Key, Scope.preferences, 'user', 'problem') +prefs_key = partial(LmsKeyValueStore.Key, Scope.preferences, 'user', 'MockProblemModule') user_info_key = partial(LmsKeyValueStore.Key, Scope.user_info, 'user', None) @@ -190,6 +191,10 @@ class StorageTestBase(object): self.mdc = ModelDataCache([mock_descriptor([mock_field(self.scope, 'existing_field')])], course_id, self.user) self.kvs = LmsKeyValueStore(self.desc_md, self.mdc) + def test_set_and_get_existing_field(self): + self.kvs.set(self.key_factory('existing_field'), 'test_value') + self.assertEquals('test_value', self.kvs.get(self.key_factory('existing_field'))) + def test_get_existing_field(self): "Test that getting an existing field in an existing Storage Field works" self.assertEquals('old_value', self.kvs.get(self.key_factory('existing_field'))) From 9e69586bb360db757c1d3c43bbe862cb65a6b670 Mon Sep 17 00:00:00 2001 From: cahrens Date: Thu, 20 Jun 2013 10:31:44 -0400 Subject: [PATCH 15/15] pep8 fixes. --- .../features/advanced-settings.py | 7 +++-- common/djangoapps/terrain/ui_helpers.py | 26 +++++++++---------- 2 files changed, 16 insertions(+), 17 deletions(-) diff --git a/cms/djangoapps/contentstore/features/advanced-settings.py b/cms/djangoapps/contentstore/features/advanced-settings.py index 3113603467..473fc20a68 100644 --- a/cms/djangoapps/contentstore/features/advanced-settings.py +++ b/cms/djangoapps/contentstore/features/advanced-settings.py @@ -31,11 +31,10 @@ def press_the_notification_button(step, name): # Save was clicked if either the save notification bar is gone, or we have a error notification # overlaying it (expected in the case of typing Object into display_name). - save_clicked = lambda : world.is_css_not_present('.is-shown.wrapper-notification-warning') or \ - world.is_css_present('.is-shown.wrapper-notification-error') + save_clicked = lambda: world.is_css_not_present('.is-shown.wrapper-notification-warning') or\ + world.is_css_present('.is-shown.wrapper-notification-error') - assert_true(world.css_click(css, success_condition=save_clicked), - 'The save button was not clicked after 5 attempts.') + assert_true(world.css_click(css, success_condition=save_clicked), 'Save button not clicked after 5 attempts.') @step(u'I edit the value of a policy key$') diff --git a/common/djangoapps/terrain/ui_helpers.py b/common/djangoapps/terrain/ui_helpers.py index 8e4330d940..77667f2d63 100644 --- a/common/djangoapps/terrain/ui_helpers.py +++ b/common/djangoapps/terrain/ui_helpers.py @@ -49,7 +49,7 @@ def css_has_text(css_selector, text): @world.absorb def css_find(css, wait_time=5): - def is_visible(driver): + def is_visible(_driver): return EC.visibility_of_element_located((By.CSS_SELECTOR, css,)) world.browser.is_element_present_by_css(css, wait_time=wait_time) @@ -58,7 +58,7 @@ def css_find(css, wait_time=5): @world.absorb -def css_click(css_selector, index=0, attempts=5, success_condition=lambda:True): +def css_click(css_selector, index=0, attempts=5, success_condition=lambda: True): """ Perform a click on a CSS selector, retrying if it initially fails. @@ -90,15 +90,15 @@ def css_click(css_selector, index=0, attempts=5, success_condition=lambda:True): @world.absorb -def css_click_at(css, x=10, y=10): +def css_click_at(css, x_cord=10, y_cord=10): ''' A method to click at x,y coordinates of the element rather than in the center of the element ''' - e = css_find(css).first - e.action_chains.move_to_element_with_offset(e._element, x, y) - e.action_chains.click() - e.action_chains.perform() + element = css_find(css).first + element.action_chains.move_to_element_with_offset(element._element, x_cord, y_cord) + element.action_chains.click() + element.action_chains.perform() @world.absorb @@ -143,7 +143,7 @@ def css_visible(css_selector): @world.absorb def dialogs_closed(): - def are_dialogs_closed(driver): + def are_dialogs_closed(_driver): ''' Return True when no modal dialogs are visible ''' @@ -154,12 +154,12 @@ def dialogs_closed(): @world.absorb def save_the_html(path='/tmp'): - u = world.browser.url + url = world.browser.url html = world.browser.html.encode('ascii', 'ignore') - filename = '%s.html' % quote_plus(u) - f = open('%s/%s' % (path, filename), 'w') - f.write(html) - f.close() + filename = '%s.html' % quote_plus(url) + file = open('%s/%s' % (path, filename), 'w') + file.write(html) + file.close() @world.absorb