From 59c5baeb1ba72cfb0cfcd9139510feb264140f6b Mon Sep 17 00:00:00 2001 From: Usman Khalid Date: Thu, 19 Sep 2013 20:50:05 +0500 Subject: [PATCH 01/54] Disables the previous/next arrows on pdf books on the first/last pages respectively --- common/static/js/pdfviewer.js | 6 ++++++ lms/static/sass/course/_textbook.scss | 8 ++++---- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/common/static/js/pdfviewer.js b/common/static/js/pdfviewer.js index 258541e0ad..889c1e7d71 100644 --- a/common/static/js/pdfviewer.js +++ b/common/static/js/pdfviewer.js @@ -200,6 +200,12 @@ PDFJS.disableWorker = true; document.getElementById('numPages').textContent = 'of ' + pdfDocument.numPages; $("#pageNumber").max = pdfDocument.numPages; $("#pageNumber").val(pageNum); + + // Enable/disable the previous/next buttons + if (pageNum > 1) $("#previous").addClass("enabled"); + else $("#previous").removeClass("enabled"); + if (pageNum < pdfDocument.numPages) $("#next").addClass("enabled"); + else $("#next").removeClass("enabled"); } // Go to previous page diff --git a/lms/static/sass/course/_textbook.scss b/lms/static/sass/course/_textbook.scss index 72d73bdb78..f839c0dd10 100644 --- a/lms/static/sass/course/_textbook.scss +++ b/lms/static/sass/course/_textbook.scss @@ -160,11 +160,11 @@ div.book-wrapper { @include transition(none); vertical-align: middle; width: 100%; + } - &:hover { - opacity: 1.0; - filter: alpha(opacity=100); - } + a.enabled:hover { + opacity: 1.0; + filter: alpha(opacity=100); } &.last { From 93a9e28fc7e5e30bfaa928f2b7a5cb74953b1bfd Mon Sep 17 00:00:00 2001 From: e0d Date: Mon, 16 Sep 2013 09:11:30 -0400 Subject: [PATCH 02/54] Upgrading to 1.4.8 --- requirements/edx/base.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 62515f7445..bcfc01ab66 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -29,7 +29,7 @@ django-storages==1.1.5 django-threaded-multihost==1.4-1 django-method-override==0.1.0 djangorestframework==2.3.5 -django==1.4.5 +django==1.4.8 feedparser==5.1.3 fs==0.4.0 GitPython==0.3.2.RC1 From d2cd8a6a58a40e8c55da06f68dc06972e529dd7e Mon Sep 17 00:00:00 2001 From: Usman Khalid Date: Fri, 20 Sep 2013 15:59:34 +0500 Subject: [PATCH 03/54] Added setting to disable find courses links --- lms/envs/common.py | 3 +++ lms/templates/dashboard.html | 10 +++++++--- lms/templates/navigation.html | 8 +++++--- 3 files changed, 15 insertions(+), 6 deletions(-) diff --git a/lms/envs/common.py b/lms/envs/common.py index 3fa1918769..b09af34b38 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -103,6 +103,9 @@ MITX_FEATURES = { # with Shib. Feature was requested by Stanford's office of general counsel 'SHIB_DISABLE_TOS': False, + # Can be turned off if all courses are invite-only. Effects views and templates. + 'ENABLE_STUDENT_SELF_ENROLLMENT': True, + # Enables ability to restrict enrollment in specific courses by the user account login method 'RESTRICT_ENROLL_BY_REG_METHOD': False, diff --git a/lms/templates/dashboard.html b/lms/templates/dashboard.html index 0a5892e3ea..3e851ecbc6 100644 --- a/lms/templates/dashboard.html +++ b/lms/templates/dashboard.html @@ -332,10 +332,14 @@ % else:
-

${_("Looks like you haven't registered for any courses yet.")}

- + % if settings.MITX_FEATURES.get('ENABLE_STUDENT_SELF_ENROLLMENT'): +

${_("Looks like you haven't registered for any courses yet.")}

+ ${_("Find courses now!")} - + + % else: +

${_("Looks like you haven't joined any courses yet.")}

+ %endif
% endif diff --git a/lms/templates/navigation.html b/lms/templates/navigation.html index 1dd5aa5229..d44cd0e1b2 100644 --- a/lms/templates/navigation.html +++ b/lms/templates/navigation.html @@ -57,9 +57,11 @@ site_status_msg = get_site_status_msg(course_id)
    <%block name="navigation_global_links_authenticated"> -
  1. - ${_('Find Courses')} -
    2. + % if settings.MITX_FEATURES.get('ENABLE_STUDENT_SELF_ENROLLMENT'): +
  2. + ${_('Find Courses')} +
    3. + % endif
    From 3ff203d11f3847621b175e9174b565fac083f4d1 Mon Sep 17 00:00:00 2001 From: Usman Khalid Date: Mon, 23 Sep 2013 15:04:11 +0000 Subject: [PATCH 04/54] Renamed setting ENABLE_STUDENT_SELF_ENROLLMENT to COURSES_ARE_BROWSABLE --- lms/envs/common.py | 4 ++-- lms/templates/dashboard.html | 2 +- lms/templates/index.html | 20 +++++++++++--------- lms/templates/navigation.html | 2 +- 4 files changed, 15 insertions(+), 13 deletions(-) diff --git a/lms/envs/common.py b/lms/envs/common.py index b09af34b38..ba060f5ab1 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -103,8 +103,8 @@ MITX_FEATURES = { # with Shib. Feature was requested by Stanford's office of general counsel 'SHIB_DISABLE_TOS': False, - # Can be turned off if all courses are invite-only. Effects views and templates. - 'ENABLE_STUDENT_SELF_ENROLLMENT': True, + # Can be turned off if course lists need to be hidden. Effects views and templates. + 'COURSES_ARE_BROWSABLE': True, # Enables ability to restrict enrollment in specific courses by the user account login method 'RESTRICT_ENROLL_BY_REG_METHOD': False, diff --git a/lms/templates/dashboard.html b/lms/templates/dashboard.html index 3e851ecbc6..bca2bf8c38 100644 --- a/lms/templates/dashboard.html +++ b/lms/templates/dashboard.html @@ -332,7 +332,7 @@ % else:
    - % if settings.MITX_FEATURES.get('ENABLE_STUDENT_SELF_ENROLLMENT'): + % if settings.MITX_FEATURES.get('COURSES_ARE_BROWSABLE'):

    ${_("Looks like you haven't registered for any courses yet.")}

    ${_("Find courses now!")} diff --git a/lms/templates/index.html b/lms/templates/index.html index 0fecd24e84..83024e01aa 100644 --- a/lms/templates/index.html +++ b/lms/templates/index.html @@ -165,15 +165,17 @@
    % endif -
    -
      - %for course in courses: -
    • - <%include file="course.html" args="course=course" /> -
      • - %endfor -
    -
    + % if settings.MITX_FEATURES.get('COURSES_ARE_BROWSABLE'): +
    +
      + %for course in courses: +
    • + <%include file="course.html" args="course=course" /> +
      • + %endfor +
    +
    + % endif diff --git a/lms/templates/navigation.html b/lms/templates/navigation.html index d44cd0e1b2..680838a719 100644 --- a/lms/templates/navigation.html +++ b/lms/templates/navigation.html @@ -57,7 +57,7 @@ site_status_msg = get_site_status_msg(course_id)
      <%block name="navigation_global_links_authenticated"> - % if settings.MITX_FEATURES.get('ENABLE_STUDENT_SELF_ENROLLMENT'): + % if settings.MITX_FEATURES.get('COURSES_ARE_BROWSABLE'):
    1. ${_('Find Courses')}
      2. From b2af990d3cf0d4b9a0b074de28c87328ced891d6 Mon Sep 17 00:00:00 2001 From: Usman Khalid Date: Mon, 23 Sep 2013 15:19:32 +0000 Subject: [PATCH 05/54] If setting COURSES_ARE_BROWSABLE is off return 404 on /courses --- lms/djangoapps/branding/views.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lms/djangoapps/branding/views.py b/lms/djangoapps/branding/views.py index 6a8ef8056b..cdf7aa5eda 100644 --- a/lms/djangoapps/branding/views.py +++ b/lms/djangoapps/branding/views.py @@ -1,5 +1,6 @@ from django.conf import settings from django.core.urlresolvers import reverse +from django.http import Http404 from django.shortcuts import redirect from django_future.csrf import ensure_csrf_cookie from mitxmako.shortcuts import render_to_response @@ -48,6 +49,9 @@ def courses(request): if settings.MITX_FEATURES.get('ENABLE_MKTG_SITE', False): return redirect(marketing_link('COURSES'), permanent=True) + if not settings.MITX_FEATURES.get('COURSES_ARE_BROWSABLE'): + raise Http404 + university = branding.get_university(request.META.get('HTTP_HOST')) if university == 'edge': return render_to_response('university_profile/edge.html', {}) From 231b130911264bf27f5f042a5ff5d3d72d539420 Mon Sep 17 00:00:00 2001 From: Usman Khalid Date: Mon, 23 Sep 2013 15:30:00 +0000 Subject: [PATCH 06/54] Removed hardcoded case for edge for /courses --- lms/djangoapps/branding/views.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/lms/djangoapps/branding/views.py b/lms/djangoapps/branding/views.py index cdf7aa5eda..3ce8dbd401 100644 --- a/lms/djangoapps/branding/views.py +++ b/lms/djangoapps/branding/views.py @@ -52,10 +52,6 @@ def courses(request): if not settings.MITX_FEATURES.get('COURSES_ARE_BROWSABLE'): raise Http404 - university = branding.get_university(request.META.get('HTTP_HOST')) - if university == 'edge': - return render_to_response('university_profile/edge.html', {}) - # we do not expect this case to be reached in cases where - # marketing and edge are enabled + # marketing is enabled or the courses are not browsable return courseware.views.courses(request) From ecddd932caba633ea1c8f04d797f8ced80ae13ff Mon Sep 17 00:00:00 2001 From: Julian Arni Date: Thu, 19 Sep 2013 11:35:10 -0400 Subject: [PATCH 07/54] Adding Pillow jpeg and tiff system libs --- requirements/system/mac_os_x/brew-formulas.txt | 2 ++ requirements/system/ubuntu/apt-packages.txt | 2 ++ 2 files changed, 4 insertions(+) diff --git a/requirements/system/mac_os_x/brew-formulas.txt b/requirements/system/mac_os_x/brew-formulas.txt index 5bc35aeb5f..22558042c4 100644 --- a/requirements/system/mac_os_x/brew-formulas.txt +++ b/requirements/system/mac_os_x/brew-formulas.txt @@ -11,3 +11,5 @@ mysql geos mongodb lynx +libjpeg +libtiff diff --git a/requirements/system/ubuntu/apt-packages.txt b/requirements/system/ubuntu/apt-packages.txt index 0e9687b19a..6f130107da 100644 --- a/requirements/system/ubuntu/apt-packages.txt +++ b/requirements/system/ubuntu/apt-packages.txt @@ -16,6 +16,8 @@ gfortran libfreetype6-dev libpng12-dev libjpeg-dev +libtiff4-dev +zlib1g-dev libxml2-dev libxslt-dev yui-compressor From d0f0e5927243974646fac2bd9dea7ad37641527c Mon Sep 17 00:00:00 2001 From: Usman Khalid Date: Wed, 25 Sep 2013 22:52:51 +0500 Subject: [PATCH 08/54] Fixed js coding style for pdf arrows toggle --- common/static/js/pdfviewer.js | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/common/static/js/pdfviewer.js b/common/static/js/pdfviewer.js index 889c1e7d71..ef30f180ae 100644 --- a/common/static/js/pdfviewer.js +++ b/common/static/js/pdfviewer.js @@ -202,10 +202,16 @@ PDFJS.disableWorker = true; $("#pageNumber").val(pageNum); // Enable/disable the previous/next buttons - if (pageNum > 1) $("#previous").addClass("enabled"); - else $("#previous").removeClass("enabled"); - if (pageNum < pdfDocument.numPages) $("#next").addClass("enabled"); - else $("#next").removeClass("enabled"); + if (pageNum > 1) { + $("#previous").addClass("enabled"); + } else { + $("#previous").removeClass("enabled"); + } + if (pageNum < pdfDocument.numPages) { + $("#next").addClass("enabled"); + } else { + $("#next").removeClass("enabled"); + } } // Go to previous page From 724fc5adbd68739d3726c84258a608c7a2f84206 Mon Sep 17 00:00:00 2001 From: Usman Khalid Date: Thu, 26 Sep 2013 15:04:43 +0500 Subject: [PATCH 09/54] Updated copy in dashboard --- lms/templates/dashboard.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lms/templates/dashboard.html b/lms/templates/dashboard.html index bca2bf8c38..121cc42791 100644 --- a/lms/templates/dashboard.html +++ b/lms/templates/dashboard.html @@ -338,7 +338,7 @@ ${_("Find courses now!")} % else: -

      ${_("Looks like you haven't joined any courses yet.")}

      +

      ${_("Looks like you haven't been enrolled in any courses yet.")}

      %endif % endif From 94c74b5c80a818df0453e87f42693f03a6819c20 Mon Sep 17 00:00:00 2001 From: Jay Zoldak Date: Thu, 26 Sep 2013 16:30:35 -0400 Subject: [PATCH 10/54] update discussion forum doc --- docs/internal/discussion.md | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/docs/internal/discussion.md b/docs/internal/discussion.md index 9885cfabf6..e2956bb165 100644 --- a/docs/internal/discussion.md +++ b/docs/internal/discussion.md @@ -22,11 +22,11 @@ For debugging, it's often more convenient to have elasticsearch running in a ter ## Setting up the discussion service -First, make sure that you have access to the [github repository](https://github.com/rll/cs_comments_service). If this were not the case, send an email to dementrock@gmail.com. +You can retrieve the source code from the [github repository](https://github.com/edx/cs_comments_service). -First go into the mitx_all directory. Then type +First go into the edx_all directory. Then type - git clone git@github.com:rll/cs_comments_service.git + git clone https://github.com/edx/cs_comments_service.git cd cs_comments_service/ If you see a prompt asking "Do you wish to trust this .rvmrc file?", type "y" @@ -52,6 +52,13 @@ It's done! Launch the app now: ruby app.rb +## Integrating with the edx platform + +The API key must match on both sides. It is configured here: +* edx-platform: COMMENTS_SERVICE_KEY in your dev.py file (dev environment) or ENV_TOKENS (prod environment) +* cs_comments_service: api_key in the application.yml file (dev environment) or ENV variable (prod environment) + + ## Running the delayed job worker In the discussion service, notifications are handled asynchronously using a third party gem called delayed_job. If you want to test this functionality, run the following command in a separate tab: From ebcb134c2ea36f9bc02dc964951f8b8c267adc7f Mon Sep 17 00:00:00 2001 From: jmclaus Date: Fri, 27 Sep 2013 15:50:24 +0200 Subject: [PATCH 11/54] First caption does not display outline on initial mouseover. --- common/lib/xmodule/xmodule/js/src/video/09_video_caption.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/common/lib/xmodule/xmodule/js/src/video/09_video_caption.js b/common/lib/xmodule/xmodule/js/src/video/09_video_caption.js index 3dc675b4c2..2c14d74244 100644 --- a/common/lib/xmodule/xmodule/js/src/video/09_video_caption.js +++ b/common/lib/xmodule/xmodule/js/src/video/09_video_caption.js @@ -345,8 +345,8 @@ function () { // Keeps track of where the focus is situated in the array of captions. // Used to implement the automatic scrolling behavior and decide if the // outline around a caption has to be hidden or shown on a mouseenter or - // mouseleave. - this.videoCaption.currentCaptionIndex = 0; + // mouseleave. Initially, no caption has the focus, set the index to -1. + this.videoCaption.currentCaptionIndex = -1; // Used to track if the focus is coming from a click or tabbing. This // has to be known to decide if, when a caption gets the focus, an // outline has to be drawn (tabbing) or not (mouse click). From 21a9446b9936431cf0483f07d9572a476667f395 Mon Sep 17 00:00:00 2001 From: Shiwen Cheng Date: Wed, 25 Sep 2013 11:33:48 +0800 Subject: [PATCH 12/54] fix the bug that the first caption and the second one will become current at the same time when displaying right captions --- .../lib/xmodule/xmodule/js/src/video/09_video_caption.js | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/common/lib/xmodule/xmodule/js/src/video/09_video_caption.js b/common/lib/xmodule/xmodule/js/src/video/09_video_caption.js index 3dc675b4c2..446111ffee 100644 --- a/common/lib/xmodule/xmodule/js/src/video/09_video_caption.js +++ b/common/lib/xmodule/xmodule/js/src/video/09_video_caption.js @@ -453,6 +453,9 @@ function () { min = 0; max = this.videoCaption.start.length - 1; + if (time < this.videoCaption.start[min]) { + return -1; + } while (min < max) { index = Math.ceil((max + min) / 2); @@ -507,10 +510,11 @@ function () { newIndex = this.videoCaption.search(time); if ( - newIndex !== void 0 && + typeof newIndex !== 'undefined' && + newIndex !== -1 && this.videoCaption.currentIndex !== newIndex ) { - if (this.videoCaption.currentIndex) { + if (typeof this.videoCaption.currentIndex !== 'undefined') { this.videoCaption.subtitlesEl .find('li.current') .removeClass('current'); From 77f3e4f96479b92b5de6556e622e3e1e7f2b5c99 Mon Sep 17 00:00:00 2001 From: Shiwen Cheng Date: Fri, 27 Sep 2013 18:43:39 +0800 Subject: [PATCH 13/54] fix the bug: when click a caption, the previous one will be in bold --- common/lib/xmodule/xmodule/js/src/video/09_video_caption.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/common/lib/xmodule/xmodule/js/src/video/09_video_caption.js b/common/lib/xmodule/xmodule/js/src/video/09_video_caption.js index 446111ffee..ff5455c70c 100644 --- a/common/lib/xmodule/xmodule/js/src/video/09_video_caption.js +++ b/common/lib/xmodule/xmodule/js/src/video/09_video_caption.js @@ -500,11 +500,11 @@ function () { // Total play time changes with speed change. Also there is // a 250 ms delay we have to take into account. time = Math.round( - Time.convert(time, this.speed, '1.0') * 1000 + 250 + Time.convert(time, this.speed, '1.0') * 1000 + 150 ); } else { // Total play time remains constant when speed changes. - time = Math.round(parseInt(time, 10) * 1000); + time = Math.round(parseInt(time, 10) * 1000 + 150); } newIndex = this.videoCaption.search(time); From 522cb8b48c15abd82512f38cd97fda3bffe20e3f Mon Sep 17 00:00:00 2001 From: Shiwen Cheng Date: Fri, 27 Sep 2013 21:52:54 +0800 Subject: [PATCH 14/54] fix the video caption lag issue --- common/lib/xmodule/xmodule/js/src/video/09_video_caption.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/common/lib/xmodule/xmodule/js/src/video/09_video_caption.js b/common/lib/xmodule/xmodule/js/src/video/09_video_caption.js index ff5455c70c..a78e08d648 100644 --- a/common/lib/xmodule/xmodule/js/src/video/09_video_caption.js +++ b/common/lib/xmodule/xmodule/js/src/video/09_video_caption.js @@ -500,11 +500,11 @@ function () { // Total play time changes with speed change. Also there is // a 250 ms delay we have to take into account. time = Math.round( - Time.convert(time, this.speed, '1.0') * 1000 + 150 + Time.convert(time, this.speed, '1.0') * 1000 + 100 ); } else { // Total play time remains constant when speed changes. - time = Math.round(parseInt(time, 10) * 1000 + 150); + time = Math.round(time * 1000 + 100); } newIndex = this.videoCaption.search(time); From 813795ddf4bbac0c22108ab84b92b130ba186810 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Mon, 16 Sep 2013 11:17:44 -0400 Subject: [PATCH 15/54] Add a management command to dump xml courses This command loads the courses with the XMLModuleStore, and then exports the fields contents as json in a diff-friendly way. This can then be used to validate against regressions in the xml parsing code. --- lms/djangoapps/debug/management/__init__.py | 0 .../debug/management/commands/__init__.py | 0 .../management/commands/dump_xml_courses.py | 59 +++++++++++++++++++ 3 files changed, 59 insertions(+) create mode 100644 lms/djangoapps/debug/management/__init__.py create mode 100644 lms/djangoapps/debug/management/commands/__init__.py create mode 100644 lms/djangoapps/debug/management/commands/dump_xml_courses.py diff --git a/lms/djangoapps/debug/management/__init__.py b/lms/djangoapps/debug/management/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/lms/djangoapps/debug/management/commands/__init__.py b/lms/djangoapps/debug/management/commands/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/lms/djangoapps/debug/management/commands/dump_xml_courses.py b/lms/djangoapps/debug/management/commands/dump_xml_courses.py new file mode 100644 index 0000000000..f0834282b7 --- /dev/null +++ b/lms/djangoapps/debug/management/commands/dump_xml_courses.py @@ -0,0 +1,59 @@ +""" +Export all xml courses in a diffable format. + +This command loads all of the xml courses in the configured DATA_DIR. +For each of the courses, it loops through all of the modules, and dumps +each as a separate output file containing the json representation +of each of its fields (including those fields that are set as default values). +""" + +from __future__ import print_function + +import json +from path import path + +from django.core.management.base import BaseCommand, CommandError +from django.conf import settings + +from xmodule.modulestore.xml import XMLModuleStore + + +class Command(BaseCommand): + """ + Django management command to export diffable representations of all xml courses + """ + help = '''Dump the in-memory representation of all xml courses in a diff-able format''' + args = '' + + def handle(self, *args, **options): + if len(args) != 1: + raise CommandError('Must called with arguments: {}'.format(self.args)) + + xml_module_store = XMLModuleStore( + data_dir=settings.DATA_DIR, + default_class='xmodule.hidden_module.HiddenDescriptor', + load_error_modules=True, + xblock_mixins=settings.XBLOCK_MIXINS, + ) + + export_dir = path(args[0]) + + for course_id, course_modules in xml_module_store.modules.iteritems(): + course_path = course_id.replace('/', '_') + for location, descriptor in course_modules.iteritems(): + location_path = location.url().replace('/', '_') + data = {} + for field_name, field in descriptor.fields.iteritems(): + try: + data[field_name] = field.read_json(descriptor) + except Exception as exc: # pylint: disable=broad-except + data[field_name] = { + '$type': str(type(exc)), + '$value': descriptor._field_data.get(descriptor, field_name) # pylint: disable=protected-access + } + + outdir = export_dir / course_path + outdir.makedirs_p() + with open(outdir / location_path + '.json', 'w') as outfile: + json.dump(data, outfile, sort_keys=True, indent=4) + print('', file=outfile) From f500b72290e3edf1e3c60b87bced55316d1466ee Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 17 Sep 2013 11:55:34 -0400 Subject: [PATCH 16/54] Make sure that we have the right set of fields available during xml parsing We had a bug where mixins weren't being applied before `load_from_xml` was called. This meant that not all of the fields were being loaded correctly. To fix it, we used the mixoligist from the runtime to apply the mixins earlier in the process. However, that caused the mixins to be applied twice. The included fixes to xblock resolved the multiply-applied mixins, and the fixes to the parsing code make it simpler to understand, and add some unit tests of the parsing to boot. --- .../lib/xmodule/xmodule/tests/test_import.py | 1 - .../xmodule/xmodule/tests/test_xml_module.py | 97 ++++++++++++- .../lib/xmodule/xmodule/tests/xml/__init__.py | 46 +++++++ .../xmodule/xmodule/tests/xml/factories.py | 128 ++++++++++++++++++ .../xmodule/tests/xml/test_inheritance.py | 29 ++++ .../xmodule/xmodule/tests/xml/test_policy.py | 30 ++++ common/lib/xmodule/xmodule/video_module.py | 35 ++--- common/lib/xmodule/xmodule/xml_module.py | 77 +++-------- lms/djangoapps/courseware/module_render.py | 2 +- pylintrc | 8 +- requirements/edx/base.txt | 2 +- requirements/edx/github.txt | 4 +- 12 files changed, 368 insertions(+), 91 deletions(-) create mode 100644 common/lib/xmodule/xmodule/tests/xml/__init__.py create mode 100644 common/lib/xmodule/xmodule/tests/xml/factories.py create mode 100644 common/lib/xmodule/xmodule/tests/xml/test_inheritance.py create mode 100644 common/lib/xmodule/xmodule/tests/xml/test_policy.py diff --git a/common/lib/xmodule/xmodule/tests/test_import.py b/common/lib/xmodule/xmodule/tests/test_import.py index cd7749dc09..19232e383f 100644 --- a/common/lib/xmodule/xmodule/tests/test_import.py +++ b/common/lib/xmodule/xmodule/tests/test_import.py @@ -91,7 +91,6 @@ class ImportTestCase(BaseCourseTestCase): self.assertNotEqual(descriptor1.location, descriptor2.location) - @unittest.skip('Temporarily disabled') def test_reimport(self): '''Make sure an already-exported error xml tag loads properly''' diff --git a/common/lib/xmodule/xmodule/tests/test_xml_module.py b/common/lib/xmodule/xmodule/tests/test_xml_module.py index 1515c76237..9d11b2b168 100644 --- a/common/lib/xmodule/xmodule/tests/test_xml_module.py +++ b/common/lib/xmodule/xmodule/tests/test_xml_module.py @@ -1,17 +1,25 @@ # disable missing docstring #pylint: disable=C0111 -from xmodule.x_module import XModuleFields -from xblock.fields import Scope, String, Dict, Boolean, Integer, Float, Any, List -from xblock.field_data import DictFieldData -from xmodule.fields import Date, Timedelta -from xmodule.xml_module import XmlDescriptor, serialize_field, deserialize_field import unittest -from nose.tools import assert_equals # pylint: disable=E0611 + from mock import Mock -from xmodule.modulestore.inheritance import InheritanceKeyValueStore, InheritanceMixin +from nose.tools import assert_equals, assert_not_equals, assert_true, assert_false, assert_in, assert_not_in # pylint: disable=E0611 + +from xblock.field_data import DictFieldData +from xblock.fields import Scope, String, Dict, Boolean, Integer, Float, Any, List from xblock.runtime import DbModel + +from xmodule.fields import Date, Timedelta +from xmodule.modulestore.inheritance import InheritanceKeyValueStore, InheritanceMixin +from xmodule.x_module import XModuleFields +from xmodule.xml_module import XmlDescriptor, serialize_field, deserialize_field +from xmodule.course_module import CourseDescriptor +from xmodule.seq_module import SequenceDescriptor + from xmodule.tests import get_test_descriptor_system +from xmodule.tests.xml import XModuleXmlImportTest +from xmodule.tests.xml.factories import CourseFactory, SequenceFactory, ProblemFactory class CrazyJsonString(String): @@ -379,3 +387,78 @@ class TestDeserializeTimedelta(TestDeserialize): self.assertDeserializeEqual('1 day 12 hours 59 minutes 59 seconds', '"1 day 12 hours 59 minutes 59 seconds"') self.assertDeserializeNonString() + + +class TestXmlAttributes(XModuleXmlImportTest): + + def test_unknown_attribute(self): + assert_false(hasattr(CourseDescriptor, 'unknown_attr')) + course = self.process_xml(CourseFactory.build(unknown_attr='value')) + assert_false(hasattr(course, 'unknown_attr')) + assert_equals('value', course.xml_attributes['unknown_attr']) + + def test_known_attribute(self): + assert_true(hasattr(CourseDescriptor, 'show_chat')) + course = self.process_xml(CourseFactory.build(show_chat='true')) + assert_true(course.show_chat) + assert_not_in('show_chat', course.xml_attributes) + + def test_rerandomize_in_policy(self): + # Rerandomize isn't a basic attribute of Sequence + assert_false(hasattr(SequenceDescriptor, 'rerandomize')) + + root = SequenceFactory.build(policy={'rerandomize': 'never'}) + ProblemFactory.build(parent=root) + + seq = self.process_xml(root) + + # Rerandomize is added to the constructed sequence via the InheritanceMixin + assert_equals('never', seq.rerandomize) + + # Rerandomize is a known value coming from policy, and shouldn't appear + # in xml_attributes + assert_not_in('rerandomize', seq.xml_attributes) + + def test_attempts_in_policy(self): + # attempts isn't a basic attribute of Sequence + assert_false(hasattr(SequenceDescriptor, 'attempts')) + + root = SequenceFactory.build(policy={'attempts': '1'}) + ProblemFactory.build(parent=root) + + seq = self.process_xml(root) + + # attempts isn't added to the constructed sequence, because + # it's not in the InheritanceMixin + assert_false(hasattr(seq, 'attempts')) + + # attempts is an unknown attribute, so we should include it + # in xml_attributes so that it gets written out (despite the misleading + # name) + assert_in('attempts', seq.xml_attributes) + + def test_inheritable_attribute(self): + # days_early_for_beta isn't a basic attribute of Sequence + assert_false(hasattr(SequenceDescriptor, 'days_early_for_beta')) + + # days_early_for_beta is added by InheritanceMixin + assert_true(hasattr(InheritanceMixin, 'days_early_for_beta')) + + root = SequenceFactory.build(policy={'days_early_for_beta': '2'}) + ProblemFactory.build(parent=root) + + # InheritanceMixin will be used when processing the XML + assert_in(InheritanceMixin, root.xblock_mixins) + + seq = self.process_xml(root) + + assert_equals(seq.unmixed_class, SequenceDescriptor) + assert_not_equals(type(seq), SequenceDescriptor) + + # days_early_for_beta is added to the constructed sequence, because + # it's in the InheritanceMixin + assert_equals(2, seq.days_early_for_beta) + + # days_early_for_beta is a known attribute, so we shouldn't include it + # in xml_attributes + assert_not_in('days_early_for_beta', seq.xml_attributes) diff --git a/common/lib/xmodule/xmodule/tests/xml/__init__.py b/common/lib/xmodule/xmodule/tests/xml/__init__.py new file mode 100644 index 0000000000..32cefadca7 --- /dev/null +++ b/common/lib/xmodule/xmodule/tests/xml/__init__.py @@ -0,0 +1,46 @@ +""" +Xml parsing tests for XModules +""" +import pprint +from mock import Mock + +from xmodule.x_module import XMLParsingSystem, XModuleDescriptor +from xmodule.mako_module import MakoDescriptorSystem + + +class InMemorySystem(XMLParsingSystem, MakoDescriptorSystem): # pylint: disable=abstract-method + """ + The simplest possible XMLParsingSystem + """ + def __init__(self, xml_import_data): + self.org = xml_import_data.org + self.course = xml_import_data.course + self.default_class = xml_import_data.default_class + self._descriptors = {} + super(InMemorySystem, self).__init__( + policy=xml_import_data.policy, + process_xml=self.process_xml, + load_item=self.load_item, + error_tracker=Mock(), + resources_fs=xml_import_data.filesystem, + mixins=xml_import_data.xblock_mixins, + render_template=lambda template, context: pprint.pformat((template, context)) + ) + + def process_xml(self, xml): # pylint: disable=method-hidden + """Parse `xml` as an XModuleDescriptor, and add it to `self._descriptors`""" + descriptor = XModuleDescriptor.load_from_xml(xml, self, self.org, self.course, self.default_class) + self._descriptors[descriptor.location.url()] = descriptor + return descriptor + + def load_item(self, location): # pylint: disable=method-hidden + """Return the descriptor loaded for `location`""" + return self._descriptors[location] + + +class XModuleXmlImportTest(object): + """Base class for tests that use basic `XModuleDescriptor.load_from_xml` xml parsing""" + def process_xml(self, xml_import_data): + """Use the `xml_import_data` to import an :class:`XModuleDescriptor` from xml""" + system = InMemorySystem(xml_import_data) + return system.process_xml(xml_import_data.xml_string) diff --git a/common/lib/xmodule/xmodule/tests/xml/factories.py b/common/lib/xmodule/xmodule/tests/xml/factories.py new file mode 100644 index 0000000000..168dcd579b --- /dev/null +++ b/common/lib/xmodule/xmodule/tests/xml/factories.py @@ -0,0 +1,128 @@ +""" +Factories for generating edXML for testing XModule import +""" + +import inspect + +from fs.memoryfs import MemoryFS +from factory import Factory, lazy_attribute, post_generation, Sequence +from lxml import etree + +from xmodule.modulestore.inheritance import InheritanceMixin + + +class XmlImportData(object): + """ + Class to capture all of the data needed to actually run an XML import, + so that the Factories have something to generate + """ + def __init__(self, xml_node, xml=None, org=None, course=None, + default_class=None, policy=None, + filesystem=None, parent=None, + xblock_mixins=()): + + self._xml_node = xml_node + self._xml_string = xml + self.org = org + self.course = course + self.default_class = default_class + self.filesystem = filesystem + self.xblock_mixins = xblock_mixins + self.parent = parent + + if policy is None: + self.policy = {} + else: + self.policy = policy + + @property + def xml_string(self): + """Return the stringified version of the generated xml""" + if self._xml_string is not None: + return self._xml_string + + return etree.tostring(self._xml_node) + + def __repr__(self): + return u"XmlImportData{!r}".format(( + self._xml_node, self._xml_string, self.org, + self.course, self.default_class, self.policy, + self.filesystem, self.parent, self.xblock_mixins + )) + + +# Extract all argument names used to construct XmlImportData objects, +# so that the factory doesn't treat them as XML attributes +XML_IMPORT_ARGS = inspect.getargspec(XmlImportData.__init__).args + + +class XmlImportFactory(Factory): + """ + Factory for generating XmlImportData's, which can hold all the data needed + to run an XModule XML import + """ + FACTORY_FOR = XmlImportData + + filesystem = MemoryFS() + xblock_mixins = (InheritanceMixin,) + url_name = Sequence(str) + attribs = {} + policy = {} + tag = 'unknown' + + @classmethod + def _adjust_kwargs(cls, **kwargs): + """ + Adjust the kwargs to be passed to the generated class. + + Any kwargs that match :fun:`XmlImportData.__init__` will be passed + through. Any other unknown `kwargs` will be treated as XML attributes + + :param tag: xml tag for the generated :class:`Element` node + :param text: (Optional) specifies the text of the generated :class:`Element`. + :param policy: (Optional) specifies data for the policy json file for this node + :type policy: dict + :param attribs: (Optional) specify attributes for the XML node + :type attribs: dict + """ + tag = kwargs.pop('tag', 'unknown') + kwargs['policy'] = {'{tag}/{url_name}'.format(tag=tag, url_name=kwargs['url_name']): kwargs['policy']} + + kwargs['xml_node'].text = kwargs.pop('text', None) + + kwargs['xml_node'].attrib.update(kwargs.pop('attribs', {})) + for key in kwargs.keys(): + if key not in XML_IMPORT_ARGS: + kwargs['xml_node'].set(key, kwargs.pop(key)) + + return kwargs + + @lazy_attribute + def xml_node(self): + """An :class:`xml.etree.Element`""" + return etree.Element(self.tag) + + @post_generation + def parent(self, _create, extracted, **_): + """Hook to merge this xml into a parent xml node""" + if extracted is None: + return + + extracted._xml_node.append(self._xml_node) # pylint: disable=no-member, protected-access + extracted.policy.update(self.policy) + + +class CourseFactory(XmlImportFactory): + """Factory for nodes""" + tag = 'course' + + +class SequenceFactory(XmlImportFactory): + """Factory for nodes""" + tag = 'sequential' + + +class ProblemFactory(XmlImportFactory): + """Factory for nodes""" + tag = 'problem' + text = '

      Empty Problem!

      ' diff --git a/common/lib/xmodule/xmodule/tests/xml/test_inheritance.py b/common/lib/xmodule/xmodule/tests/xml/test_inheritance.py new file mode 100644 index 0000000000..dc27f0c792 --- /dev/null +++ b/common/lib/xmodule/xmodule/tests/xml/test_inheritance.py @@ -0,0 +1,29 @@ +""" +Test that inherited fields work correctly when parsing XML +""" +from nose.tools import assert_equals # pylint: disable=no-name-in-module + +from xmodule.tests.xml import XModuleXmlImportTest +from xmodule.tests.xml.factories import CourseFactory, SequenceFactory, ProblemFactory + + +class TestInheritedFieldParsing(XModuleXmlImportTest): + """ + Test that inherited fields work correctly when parsing XML + + """ + def test_null_string(self): + # Test that the string inherited fields are passed through 'deserialize_field', + # which converts the string "null" to the python value None + root = CourseFactory.build(days_early_for_beta="null") + sequence = SequenceFactory.build(parent=root) + ProblemFactory.build(parent=sequence) + + course = self.process_xml(root) + assert_equals(None, course.days_early_for_beta) + + sequence = course.get_children()[0] + assert_equals(None, sequence.days_early_for_beta) + + problem = sequence.get_children()[0] + assert_equals(None, problem.days_early_for_beta) diff --git a/common/lib/xmodule/xmodule/tests/xml/test_policy.py b/common/lib/xmodule/xmodule/tests/xml/test_policy.py new file mode 100644 index 0000000000..2e702cc82d --- /dev/null +++ b/common/lib/xmodule/xmodule/tests/xml/test_policy.py @@ -0,0 +1,30 @@ +""" +Tests that policy json files import correctly when loading XML +""" + +from nose.tools import assert_equals, assert_raises # pylint: disable=no-name-in-module + +from xmodule.tests.xml.factories import CourseFactory +from xmodule.tests.xml import XModuleXmlImportTest + + +class TestPolicy(XModuleXmlImportTest): + """ + Tests that policy json files import correctly when loading xml + """ + def test_no_attribute_mapping(self): + # Policy files are json, and thus the values aren't passed through 'deserialize_field' + # Therefor, the string 'null' is passed unchanged to the Float field, which will trigger + # a ValueError + with assert_raises(ValueError): + course = self.process_xml(CourseFactory.build(policy={'days_early_for_beta': 'null'})) + + # Trigger the exception by looking at the imported data + course.days_early_for_beta # pylint: disable=pointless-statement + + def test_course_policy(self): + course = self.process_xml(CourseFactory.build(policy={'days_early_for_beta': None})) + assert_equals(None, course.days_early_for_beta) + + course = self.process_xml(CourseFactory.build(policy={'days_early_for_beta': 9})) + assert_equals(9, course.days_early_for_beta) diff --git a/common/lib/xmodule/xmodule/video_module.py b/common/lib/xmodule/xmodule/video_module.py index d97d5aa144..febfe5961f 100644 --- a/common/lib/xmodule/xmodule/video_module.py +++ b/common/lib/xmodule/xmodule/video_module.py @@ -24,7 +24,7 @@ from django.conf import settings from xmodule.x_module import XModule from xmodule.editing_module import TabsEditingDescriptor from xmodule.raw_module import EmptyDataRawDescriptor -from xmodule.xml_module import is_pointer_tag, name_to_pathname +from xmodule.xml_module import is_pointer_tag, name_to_pathname, deserialize_field from xmodule.modulestore import Location from xblock.fields import Scope, String, Boolean, Float, List, Integer, ScopeIds @@ -217,7 +217,7 @@ class VideoDescriptor(VideoFields, TabsEditingDescriptor, EmptyDataRawDescriptor # For backwards compatibility -- if we've got XML data, parse # it out and set the metadata fields if self.data: - field_data = VideoDescriptor._parse_video_xml(self.data) + field_data = self._parse_video_xml(self.data) self._field_data.set_many(self, field_data) del self.data @@ -241,7 +241,7 @@ class VideoDescriptor(VideoFields, TabsEditingDescriptor, EmptyDataRawDescriptor if is_pointer_tag(xml_object): filepath = cls._format_filepath(xml_object.tag, name_to_pathname(url_name)) xml_data = etree.tostring(cls.load_file(filepath, system.resources_fs, location)) - field_data = VideoDescriptor._parse_video_xml(xml_data) + field_data = cls._parse_video_xml(xml_data) field_data['location'] = location kvs = InheritanceKeyValueStore(initial_values=field_data) field_data = DbModel(kvs) @@ -292,8 +292,8 @@ class VideoDescriptor(VideoFields, TabsEditingDescriptor, EmptyDataRawDescriptor xml.append(ele) return xml - @staticmethod - def _parse_youtube(data): + @classmethod + def _parse_youtube(cls, data): """ Parses a string of Youtube IDs such as "1.0:AXdE34_U,1.5:VO3SxfeD" into a dictionary. Necessary for backwards compatibility with @@ -310,14 +310,14 @@ class VideoDescriptor(VideoFields, TabsEditingDescriptor, EmptyDataRawDescriptor # Handle the fact that youtube IDs got double-quoted for a period of time. # Note: we pass in "VideoFields.youtube_id_1_0" so we deserialize as a String-- # it doesn't matter what the actual speed is for the purposes of deserializing. - youtube_id = VideoDescriptor._deserialize(VideoFields.youtube_id_1_0.name, pieces[1]) + youtube_id = deserialize_field(cls.youtube_id_1_0, pieces[1]) ret[speed] = youtube_id except (ValueError, IndexError): log.warning('Invalid YouTube ID: %s' % video) return ret - @staticmethod - def _parse_video_xml(xml_data): + @classmethod + def _parse_video_xml(cls, xml_data): """ Parse video fields out of xml_data. The fields are set if they are present in the XML. @@ -326,8 +326,8 @@ class VideoDescriptor(VideoFields, TabsEditingDescriptor, EmptyDataRawDescriptor field_data = {} conversions = { - 'start_time': VideoDescriptor._parse_time, - 'end_time': VideoDescriptor._parse_time + 'start_time': cls._parse_time, + 'end_time': cls._parse_time } # Convert between key names for certain attributes -- @@ -349,10 +349,10 @@ class VideoDescriptor(VideoFields, TabsEditingDescriptor, EmptyDataRawDescriptor for attr, value in xml.items(): if attr in compat_keys: attr = compat_keys[attr] - if attr in VideoDescriptor.metadata_to_strip + ('url_name', 'name'): + if attr in cls.metadata_to_strip + ('url_name', 'name'): continue if attr == 'youtube': - speeds = VideoDescriptor._parse_youtube(value) + speeds = cls._parse_youtube(value) for speed, youtube_id in speeds.items(): # should have made these youtube_id_1_00 for # cleanliness, but hindsight doesn't need glasses @@ -367,20 +367,13 @@ class VideoDescriptor(VideoFields, TabsEditingDescriptor, EmptyDataRawDescriptor else: # We export values with json.dumps (well, except for Strings, but # for about a month we did it for Strings also). - value = VideoDescriptor._deserialize(attr, value) + value = deserialize_field(cls.fields[attr], value) field_data[attr] = value return field_data @classmethod - def _deserialize(cls, attr, value): - """ - Handles deserializing values that may have been encoded with json.dumps. - """ - return cls.get_map_for_field(attr).from_xml(value) - - @staticmethod - def _parse_time(str_time): + def _parse_time(cls, str_time): """Converts s in '12:34:45' format to seconds. If s is None, returns empty string""" if not str_time: diff --git a/common/lib/xmodule/xmodule/xml_module.py b/common/lib/xmodule/xmodule/xml_module.py index 5fa9ff3260..83bb6faffd 100644 --- a/common/lib/xmodule/xmodule/xml_module.py +++ b/common/lib/xmodule/xmodule/xml_module.py @@ -62,23 +62,6 @@ def get_metadata_from_xml(xml_object, remove=True): xml_object.remove(meta) return dmdata -_AttrMapBase = namedtuple('_AttrMap', 'from_xml to_xml') - - -class AttrMap(_AttrMapBase): - """ - A class that specifies two functions: - - from_xml: convert value from the xml representation into - an internal python representation - - to_xml: convert the internal python representation into - the value to store in the xml. - """ - def __new__(_cls, from_xml=lambda x: x, - to_xml=lambda x: x): - return _AttrMapBase.__new__(_cls, from_xml, to_xml) - def serialize_field(value): """ @@ -166,20 +149,6 @@ class XmlDescriptor(XModuleDescriptor): metadata_to_export_to_policy = ('discussion_topics', 'checklists') - @classmethod - def get_map_for_field(cls, attr): - """ - Returns a serialize/deserialize AttrMap for the given field of a class. - - Searches through fields defined by cls to find one named attr. - """ - if attr in cls.fields: - from_xml = lambda val: deserialize_field(cls.fields[attr], val) - to_xml = lambda val: serialize_field(val) - return AttrMap(from_xml, to_xml) - else: - return AttrMap() - @classmethod def definition_from_xml(cls, xml_object, system): """ @@ -274,19 +243,19 @@ class XmlDescriptor(XModuleDescriptor): Returns a dictionary {key: value}. """ - metadata = {} - for attr in xml_object.attrib: - val = xml_object.get(attr) - if val is not None: - # VS[compat]. Remove after all key translations done - attr = cls._translate(attr) + metadata = {'xml_attributes': {}} + for attr, val in xml_object.attrib.iteritems(): + # VS[compat]. Remove after all key translations done + attr = cls._translate(attr) - if attr in cls.metadata_to_strip: - # don't load these - continue + if attr in cls.metadata_to_strip: + # don't load these + continue - attr_map = cls.get_map_for_field(attr) - metadata[attr] = attr_map.from_xml(val) + if attr not in cls.fields: + metadata['xml_attributes'][attr] = val + else: + metadata[attr] = deserialize_field(cls.fields[attr], val) return metadata @classmethod @@ -295,9 +264,14 @@ class XmlDescriptor(XModuleDescriptor): Add the keys in policy to metadata, after processing them through the attrmap. Updates the metadata dict in place. """ - for attr in policy: - attr_map = cls.get_map_for_field(attr) - metadata[cls._translate(attr)] = attr_map.from_xml(policy[attr]) + for attr, value in policy.iteritems(): + attr = cls._translate(attr) + if attr not in cls.fields: + # Store unknown attributes coming from policy.json + # in such a way that they will export to xml unchanged + metadata['xml_attributes'][attr] = value + else: + metadata[attr] = value @classmethod def from_xml(cls, xml_data, system, org=None, course=None): @@ -357,11 +331,7 @@ class XmlDescriptor(XModuleDescriptor): field_data.update(definition) field_data['children'] = children - field_data['xml_attributes'] = {} field_data['xml_attributes']['filename'] = definition.get('filename', ['', None]) # for git link - for key, value in metadata.items(): - if key not in cls.fields: - field_data['xml_attributes'][key] = value field_data['location'] = location field_data['category'] = xml_object.tag kvs = InheritanceKeyValueStore(initial_values=field_data) @@ -415,18 +385,11 @@ class XmlDescriptor(XModuleDescriptor): # Set the tag so we get the file path right xml_object.tag = self.category - def val_for_xml(attr): - """Get the value for this attribute that we want to store. - (Possible format conversion through an AttrMap). - """ - attr_map = self.get_map_for_field(attr) - return attr_map.to_xml(self._field_data.get(self, attr)) - # Add the non-inherited metadata for attr in sorted(own_metadata(self)): # don't want e.g. data_dir if attr not in self.metadata_to_strip and attr not in self.metadata_to_export_to_policy: - val = val_for_xml(attr) + val = serialize_field(self._field_data.get(self, attr)) try: xml_object.set(attr, val) except Exception, e: diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index acdd4f0aa0..f86bef29ad 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/lms/djangoapps/courseware/module_render.py @@ -306,7 +306,7 @@ def get_module_for_descriptor_internal(user, descriptor, field_data_cache, cours # Construct the key for the module key = KeyValueStore.Key( scope=Scope.user_state, - student_id=user.id, + user_id=user.id, block_scope_id=descriptor.location, field_name='grade' ) diff --git a/pylintrc b/pylintrc index 9525f04362..a3c84c1555 100644 --- a/pylintrc +++ b/pylintrc @@ -89,6 +89,9 @@ evaluation=10.0 - ((float(5 * error + warning + refactor + convention) / stateme # evaluation report (RP0004). comment=no +# Display symbolic names of messages in reports +symbols=yes + [TYPECHECK] @@ -120,7 +123,10 @@ generated-members= content, status_code, # For factory_boy factories - create + create, + build, +# For xblocks + fields, [BASIC] diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index addef7b629..52fe3ce44f 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -47,7 +47,7 @@ Pillow==1.7.8 pip>=1.4 polib==1.0.3 pycrypto>=2.6 -pygments==1.5 +pygments==1.6 pygraphviz==1.1 pymongo==2.4.1 pyparsing==1.5.6 diff --git a/requirements/edx/github.txt b/requirements/edx/github.txt index add9bda1d1..e0c88b217e 100644 --- a/requirements/edx/github.txt +++ b/requirements/edx/github.txt @@ -14,8 +14,8 @@ -e git+https://github.com/eventbrite/zendesk.git@d53fe0e81b623f084e91776bcf6369f8b7b63879#egg=zendesk # Our libraries: --e git+https://github.com/edx/XBlock.git@aa0d60627#egg=XBlock +-e git+https://github.com/edx/XBlock.git@a8de02c0#egg=XBlock -e git+https://github.com/edx/codejail.git@0a1b468#egg=codejail --e git+https://github.com/edx/diff-cover.git@v0.2.3#egg=diff_cover +-e git+https://github.com/edx/diff-cover.git@v0.2.4#egg=diff_cover -e git+https://github.com/edx/js-test-tool.git@v0.0.7#egg=js_test_tool -e git+https://github.com/edx/django-waffle.git@823a102e48#egg=django-waffle From 36fc1a62434a28cba8556dbed54ac28261b696c5 Mon Sep 17 00:00:00 2001 From: Valera Rozuvan Date: Fri, 27 Sep 2013 18:43:58 +0300 Subject: [PATCH 17/54] Fix for PR 1119. A Jasmine test was falling. It went unnoticed. The PR was merged. This is a fix for it. --- common/lib/xmodule/xmodule/js/spec/video/video_caption_spec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/lib/xmodule/xmodule/js/spec/video/video_caption_spec.js b/common/lib/xmodule/xmodule/js/spec/video/video_caption_spec.js index a94a35e124..0f729da62d 100644 --- a/common/lib/xmodule/xmodule/js/spec/video/video_caption_spec.js +++ b/common/lib/xmodule/xmodule/js/spec/video/video_caption_spec.js @@ -261,7 +261,7 @@ describe('search', function() { it('return a correct caption index', function() { - expect(videoCaption.search(0)).toEqual(0); + expect(videoCaption.search(0)).toEqual(-1); expect(videoCaption.search(3120)).toEqual(1); expect(videoCaption.search(6270)).toEqual(2); expect(videoCaption.search(8490)).toEqual(2); From 5ca778ef0508047fad5db1de0753120d0b62ba4f Mon Sep 17 00:00:00 2001 From: Usman Khalid Date: Fri, 27 Sep 2013 21:06:34 +0500 Subject: [PATCH 18/54] Display:none arrows in disabled state --- common/static/js/pdfviewer.js | 12 ++++++------ lms/static/sass/course/_textbook.scss | 13 ++++++++----- 2 files changed, 14 insertions(+), 11 deletions(-) mode change 100644 => 100755 common/static/js/pdfviewer.js mode change 100644 => 100755 lms/static/sass/course/_textbook.scss diff --git a/common/static/js/pdfviewer.js b/common/static/js/pdfviewer.js old mode 100644 new mode 100755 index ef30f180ae..a76e2f397c --- a/common/static/js/pdfviewer.js +++ b/common/static/js/pdfviewer.js @@ -202,15 +202,15 @@ PDFJS.disableWorker = true; $("#pageNumber").val(pageNum); // Enable/disable the previous/next buttons - if (pageNum > 1) { - $("#previous").addClass("enabled"); + if (pageNum <= 1) { + $("#previous").addClass("is-disabled"); } else { - $("#previous").removeClass("enabled"); + $("#previous").removeClass("is-disabled"); } - if (pageNum < pdfDocument.numPages) { - $("#next").addClass("enabled"); + if (pageNum >= pdfDocument.numPages) { + $("#next").addClass("is-disabled"); } else { - $("#next").removeClass("enabled"); + $("#next").removeClass("is-disabled"); } } diff --git a/lms/static/sass/course/_textbook.scss b/lms/static/sass/course/_textbook.scss old mode 100644 new mode 100755 index f839c0dd10..d3632be2c5 --- a/lms/static/sass/course/_textbook.scss +++ b/lms/static/sass/course/_textbook.scss @@ -160,13 +160,16 @@ div.book-wrapper { @include transition(none); vertical-align: middle; width: 100%; - } - a.enabled:hover { - opacity: 1.0; - filter: alpha(opacity=100); - } + &:hover { + opacity: 1.0; + } + &.is-disabled { + display:none; + } + } + &.last { left: 0; From 59403644ae38e9e54d8099b2e1f1a2651e6639e6 Mon Sep 17 00:00:00 2001 From: Usman Khalid Date: Fri, 27 Sep 2013 21:35:16 +0500 Subject: [PATCH 19/54] Moved jquery event handling to parent since hidden elements do not get events attached --- common/static/js/pdfviewer.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/common/static/js/pdfviewer.js b/common/static/js/pdfviewer.js index a76e2f397c..3424a09892 100755 --- a/common/static/js/pdfviewer.js +++ b/common/static/js/pdfviewer.js @@ -327,11 +327,11 @@ PDFJS.disableWorker = true; loadUrl(chapterUrl, pageVal); } - $("#previous").click(function(event) { + $(".last").on('click','#previous',function(){ prevPage(); }); - $("#next").click(function(event) { + $(".next").on('click','#next',function(){ nextPage(); }); From 07f94d46a65e9573d748f05f157c6a04fba84569 Mon Sep 17 00:00:00 2001 From: Brian Wilson Date: Fri, 27 Sep 2013 12:43:27 -0400 Subject: [PATCH 20/54] Move Pearson courseware imports to function level. --- common/djangoapps/external_auth/views.py | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/common/djangoapps/external_auth/views.py b/common/djangoapps/external_auth/views.py index 4d0b7eeb2b..5872955780 100644 --- a/common/djangoapps/external_auth/views.py +++ b/common/djangoapps/external_auth/views.py @@ -12,7 +12,7 @@ from external_auth.models import ExternalAuthMap from external_auth.djangostore import DjangoOpenIDStore from django.conf import settings -from django.contrib.auth import REDIRECT_FIELD_NAME, authenticate, login, logout +from django.contrib.auth import REDIRECT_FIELD_NAME, authenticate, login from django.contrib.auth.models import User from django.core.urlresolvers import reverse from django.core.validators import validate_email @@ -45,9 +45,6 @@ from openid.extensions import ax, sreg from ratelimitbackend.exceptions import RateLimitException import student.views -# Required for Pearson -from courseware.views import get_module_for_descriptor, jump_to -from courseware.model_data import FieldDataCache from xmodule.modulestore.django import modulestore from xmodule.course_module import CourseDescriptor from xmodule.modulestore import Location @@ -238,6 +235,7 @@ def _flatten_to_ascii(txt): else: return unicode(unicodedata.normalize('NFKD', txt).encode('ASCII', 'ignore')) + @ensure_csrf_cookie def _signup(request, eamap): """ @@ -896,12 +894,17 @@ def test_center_login(request): ''' Log in students taking exams via Pearson Takes a POST request that contains the following keys: - - code - a security code provided by Pearson + - code - a security code provided by Pearson - clientCandidateID - registrationID - exitURL - the url that we redirect to once we're done - vueExamSeriesCode - a code that indicates the exam that we're using ''' + # Imports from lms/djangoapps/courseware -- these should not be + # in a common djangoapps. + from courseware.views import get_module_for_descriptor, jump_to + from courseware.model_data import FieldDataCache + # errors are returned by navigating to the error_url, adding a query parameter named "code" # which contains the error code describing the exceptional condition. def makeErrorURL(error_url, error_code): From d77491e46a11b913a31616b4f9adc035c53cc3c6 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Fri, 6 Sep 2013 14:21:30 -0400 Subject: [PATCH 21/54] Pull XModule attributes out into a mixin that can be applied to xblocks --- cms/envs/common.py | 3 +- .../xmodule/modulestore/split_mongo/split.py | 2 - .../tests/test_split_modulestore.py | 3 +- common/lib/xmodule/xmodule/tests/__init__.py | 8 +- .../lib/xmodule/xmodule/tests/test_import.py | 3 +- .../xmodule/xmodule/tests/test_xml_module.py | 6 +- common/lib/xmodule/xmodule/x_module.py | 325 ++++++++---------- lms/envs/common.py | 3 +- 8 files changed, 148 insertions(+), 205 deletions(-) diff --git a/cms/envs/common.py b/cms/envs/common.py index 7f2559eece..13faf5520e 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -31,6 +31,7 @@ from path import path from lms.xblock.mixin import LmsBlockMixin from cms.xmodule_namespace import CmsBlockMixin from xmodule.modulestore.inheritance import InheritanceMixin +from xmodule.x_module import XModuleMixin ############################ FEATURE CONFIGURATION ############################# @@ -168,7 +169,7 @@ MIDDLEWARE_CLASSES = ( # This should be moved into an XBlock Runtime/Application object # once the responsibility of XBlock creation is moved out of modulestore - cpennington -XBLOCK_MIXINS = (LmsBlockMixin, CmsBlockMixin, InheritanceMixin) +XBLOCK_MIXINS = (LmsBlockMixin, CmsBlockMixin, InheritanceMixin, XModuleMixin) ############################ SIGNAL HANDLERS ################################ diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index d85092b363..60bfa66b6a 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py @@ -43,8 +43,6 @@ log = logging.getLogger(__name__) #============================================================================== - - class SplitMongoModuleStore(ModuleStoreBase): """ A Mongodb backed ModuleStore supporting versions, inheritance, diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py index 0f1b65d0a3..be5c61a143 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py @@ -15,6 +15,7 @@ from xmodule.modulestore.exceptions import InsufficientSpecificationError, ItemN DuplicateItemError from xmodule.modulestore.locator import CourseLocator, BlockUsageLocator, VersionTree, DescriptionLocator from xmodule.modulestore.inheritance import InheritanceMixin +from xmodule.x_module import XModuleMixin from pytz import UTC from path import path import re @@ -34,7 +35,7 @@ class SplitModuleTest(unittest.TestCase): 'db': 'test_xmodule', 'collection': 'modulestore{0}'.format(uuid.uuid4().hex), 'fs_root': '', - 'xblock_mixins': (InheritanceMixin,) + 'xblock_mixins': (InheritanceMixin, XModuleMixin) } MODULESTORE = { diff --git a/common/lib/xmodule/xmodule/tests/__init__.py b/common/lib/xmodule/xmodule/tests/__init__.py index 088b2c32b6..99caec0840 100644 --- a/common/lib/xmodule/xmodule/tests/__init__.py +++ b/common/lib/xmodule/xmodule/tests/__init__.py @@ -11,15 +11,11 @@ import json import os import unittest -import fs -import fs.osfs -import numpy from mock import Mock from path import path -import calc from xblock.field_data import DictFieldData -from xmodule.x_module import ModuleSystem, XModuleDescriptor, DescriptorSystem +from xmodule.x_module import ModuleSystem, XModuleDescriptor, XModuleMixin from xmodule.modulestore.inheritance import InheritanceMixin from xmodule.mako_module import MakoDescriptorSystem @@ -81,7 +77,7 @@ def get_test_descriptor_system(): resources_fs=Mock(), error_tracker=Mock(), render_template=lambda template, context: repr(context), - mixins=(InheritanceMixin,), + mixins=(InheritanceMixin, XModuleMixin), ) diff --git a/common/lib/xmodule/xmodule/tests/test_import.py b/common/lib/xmodule/xmodule/tests/test_import.py index 19232e383f..16c82753a5 100644 --- a/common/lib/xmodule/xmodule/tests/test_import.py +++ b/common/lib/xmodule/xmodule/tests/test_import.py @@ -13,6 +13,7 @@ from xmodule.xml_module import is_pointer_tag from xmodule.modulestore import Location from xmodule.modulestore.xml import ImportSystem, XMLModuleStore from xmodule.modulestore.inheritance import compute_inherited_metadata +from xmodule.x_module import XModuleMixin from xmodule.fields import Date from xmodule.tests import DATA_DIR from xmodule.modulestore.inheritance import InheritanceMixin @@ -42,7 +43,7 @@ class DummySystem(ImportSystem): error_tracker=error_tracker, parent_tracker=parent_tracker, load_error_modules=load_error_modules, - mixins=(InheritanceMixin,) + mixins=(InheritanceMixin, XModuleMixin) ) def render_template(self, _template, _context): diff --git a/common/lib/xmodule/xmodule/tests/test_xml_module.py b/common/lib/xmodule/xmodule/tests/test_xml_module.py index 9d11b2b168..da6062f662 100644 --- a/common/lib/xmodule/xmodule/tests/test_xml_module.py +++ b/common/lib/xmodule/xmodule/tests/test_xml_module.py @@ -12,10 +12,10 @@ from xblock.runtime import DbModel from xmodule.fields import Date, Timedelta from xmodule.modulestore.inheritance import InheritanceKeyValueStore, InheritanceMixin -from xmodule.x_module import XModuleFields from xmodule.xml_module import XmlDescriptor, serialize_field, deserialize_field from xmodule.course_module import CourseDescriptor from xmodule.seq_module import SequenceDescriptor +from xmodule.x_module import XModuleMixin from xmodule.tests import get_test_descriptor_system from xmodule.tests.xml import XModuleXmlImportTest @@ -65,7 +65,7 @@ class EditableMetadataFieldsTest(unittest.TestCase): # Also tests that xml_attributes is filtered out of XmlDescriptor. self.assertEqual(1, len(editable_fields), editable_fields) self.assert_field_values( - editable_fields, 'display_name', XModuleFields.display_name, + editable_fields, 'display_name', XModuleMixin.display_name, explicitly_set=False, value=None, default_value=None ) @@ -73,7 +73,7 @@ class EditableMetadataFieldsTest(unittest.TestCase): # Tests that explicitly_set is correct when a value overrides the default (not inheritable). editable_fields = self.get_xml_editable_fields(DictFieldData({'display_name': 'foo'})) self.assert_field_values( - editable_fields, 'display_name', XModuleFields.display_name, + editable_fields, 'display_name', XModuleMixin.display_name, explicitly_set=True, value='foo', default_value=None ) diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index 63a73c4530..63ff80b9d9 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -1,5 +1,4 @@ import logging -import copy import yaml import os @@ -11,7 +10,7 @@ from xmodule.modulestore import Location from xmodule.modulestore.exceptions import ItemNotFoundError, InsufficientSpecificationError, InvalidLocationError from xblock.core import XBlock -from xblock.fields import Scope, String, Integer, Float, List +from xblock.fields import Scope, Integer, Float, List, XBlockMixin, String from xblock.fragment import Fragment from xblock.runtime import Runtime from xmodule.modulestore.locator import BlockUsageLocator @@ -83,7 +82,13 @@ class HTMLSnippet(object): .format(self.__class__)) -class XModuleFields(object): +class XModuleMixin(XBlockMixin): + """ + Fields and methods used by XModules internally. + + Adding this Mixin to an :class:`XBlock` allows it to cooperate with old-style :class:`XModules` + """ + display_name = String( display_name="Display Name", help="This name appears in the horizontal navigation at the top of the page.", @@ -93,41 +98,6 @@ class XModuleFields(object): default=None ) - -class XModule(XModuleFields, HTMLSnippet, XBlock): - ''' Implements a generic learning module. - - Subclasses must at a minimum provide a definition for get_html in order - to be displayed to users. - - See the HTML module for a simple example. - ''' - - # The default implementation of get_icon_class returns the icon_class - # attribute of the class - # - # This attribute can be overridden by subclasses, and - # the function can also be overridden if the icon class depends on the data - # in the module - icon_class = 'other' - - - def __init__(self, descriptor, *args, **kwargs): - ''' - Construct a new xmodule - - runtime: An XBlock runtime allowing access to external resources - - descriptor: the XModuleDescriptor that this module is an instance of. - - field_data: A dictionary-like object that maps field names to values - for those fields. - ''' - super(XModule, self).__init__(*args, **kwargs) - self.system = self.runtime - self.descriptor = descriptor - self._loaded_children = None - @property def id(self): return self.location.url() @@ -155,31 +125,133 @@ class XModule(XModuleFields, HTMLSnippet, XBlock): @property def url_name(self): - if self.descriptor: - return self.descriptor.url_name - elif isinstance(self.location, Location): + if isinstance(self.location, Location): return self.location.name elif isinstance(self.location, BlockUsageLocator): return self.location.usage_id else: raise InsufficientSpecificationError() - @property def display_name_with_default(self): - ''' + """ Return a display name for the module: use display_name if defined in metadata, otherwise convert the url name. - ''' + """ name = self.display_name if name is None: name = self.url_name.replace('_', ' ') return name + def get_explicitly_set_fields_by_scope(self, scope=Scope.content): + """ + Get a dictionary of the fields for the given scope which are set explicitly on this xblock. (Including + any set to None.) + """ + result = {} + for field in self.fields.values(): + if (field.scope == scope and field.is_set_on(self)): + result[field.name] = field.read_json(self) + return result + + @property + def xblock_kvs(self): + """ + Use w/ caution. Really intended for use by the persistence layer. + """ + # if caller wants kvs, caller's assuming it's up to date; so, decache it + self.save() + return self._field_data._kvs # pylint: disable=protected-access + def get_children(self): - ''' + """Returns a list of XBlock instances for the children of + this module""" + + if not self.has_children: + return [] + + if getattr(self, '_child_instances', None) is None: + self._child_instances = [] # pylint: disable=attribute-defined-outside-init + for child_loc in self.children: + try: + child = self.runtime.get_block(child_loc) + except ItemNotFoundError: + log.exception('Unable to load item {loc}, skipping'.format(loc=child_loc)) + continue + self._child_instances.append(child) + + return self._child_instances + + def get_required_module_descriptors(self): + """Returns a list of XModuleDescriptor instances upon which this module depends, but are + not children of this module""" + return [] + + def get_display_items(self): + """ + Returns a list of descendent module instances that will display + immediately inside this module. + """ + items = [] + for child in self.get_children(): + items.extend(child.displayable_items()) + + return items + + def displayable_items(self): + """ + Returns list of displayable modules contained by this module. If this + module is visible, should return [self]. + """ + return [self] + + def get_child_by(self, selector): + """ + Return a child XBlock that matches the specified selector + """ + for child in self.get_children(): + if selector(child): + return child + return None + + +class XModule(XModuleMixin, HTMLSnippet, XBlock): # pylint: disable=abstract-method + """ Implements a generic learning module. + + Subclasses must at a minimum provide a definition for get_html in order + to be displayed to users. + + See the HTML module for a simple example. + """ + + # The default implementation of get_icon_class returns the icon_class + # attribute of the class + # + # This attribute can be overridden by subclasses, and + # the function can also be overridden if the icon class depends on the data + # in the module + icon_class = 'other' + + def __init__(self, descriptor, *args, **kwargs): + """ + Construct a new xmodule + + runtime: An XBlock runtime allowing access to external resources + + descriptor: the XModuleDescriptor that this module is an instance of. + + field_data: A dictionary-like object that maps field names to values + for those fields. + """ + super(XModule, self).__init__(*args, **kwargs) + self.system = self.runtime + self.descriptor = descriptor + self._loaded_children = None + + def get_children(self): + """ Return module instances for all the children of this module. - ''' + """ if self._loaded_children is None: child_descriptors = self.get_child_descriptors() @@ -200,7 +272,7 @@ class XModule(XModuleFields, HTMLSnippet, XBlock): return ''.format(self.id) def get_child_descriptors(self): - ''' + """ Returns the descriptors of the child modules Overriding this changes the behavior of get_children and @@ -211,40 +283,13 @@ class XModule(XModuleFields, HTMLSnippet, XBlock): These children will be the same children returned by the descriptor unless descriptor.has_dynamic_children() is true. - ''' + """ return self.descriptor.get_children() - def get_child_by(self, selector): - """ - Return a child XModuleDescriptor with the specified url_name, if it exists, and None otherwise. - """ - for child in self.get_children(): - if selector(child): - return child - return None - - def get_display_items(self): - ''' - Returns a list of descendent module instances that will display - immediately inside this module. - ''' - items = [] - for child in self.get_children(): - items.extend(child.displayable_items()) - - return items - - def displayable_items(self): - ''' - Returns list of displayable modules contained by this module. If this - module is visible, should return [self]. - ''' - return [self] - def get_icon_class(self): - ''' + """ Return a css class identifying this module in the context of an icon - ''' + """ return self.icon_class # Functions used in the LMS @@ -267,7 +312,7 @@ class XModule(XModuleFields, HTMLSnippet, XBlock): return None def max_score(self): - ''' Maximum score. Two notes: + """ Maximum score. Two notes: * This is generic; in abstract, a problem could be 3/5 points on one randomization, and 5/7 on another @@ -275,22 +320,22 @@ class XModule(XModuleFields, HTMLSnippet, XBlock): * In practice, this is a Very Bad Idea, and (a) will break some code in place (although that code should get fixed), and (b) break some analytics we plan to put in place. - ''' + """ return None def get_progress(self): - ''' Return a progress.Progress object that represents how far the + """ Return a progress.Progress object that represents how far the student has gone in this module. Must be implemented to get correct progress tracking behavior in nesting modules like sequence and vertical. If this module has no notion of progress, return None. - ''' + """ return None def handle_ajax(self, _dispatch, _data): - ''' dispatch is last part of the URL. - data is a dictionary-like object with the content of the request''' + """ dispatch is last part of the URL. + data is a dictionary-like object with the content of the request""" return "" @@ -381,7 +426,7 @@ class ResourceTemplates(object): return None -class XModuleDescriptor(XModuleFields, HTMLSnippet, ResourceTemplates, XBlock): +class XModuleDescriptor(XModuleMixin, HTMLSnippet, ResourceTemplates, XBlock): """ An XModuleDescriptor is a specification for an element of a course. This could be a problem, an organizational element (a group of content), or a @@ -446,86 +491,6 @@ class XModuleDescriptor(XModuleFields, HTMLSnippet, ResourceTemplates, XBlock): self.edited_by = self.edited_on = self.previous_version = self.update_version = self.definition_locator = None self._child_instances = None - @property - def id(self): - return self.location.url() - - @property - def category(self): - return self.scope_ids.block_type - - @property - def location(self): - try: - return Location(self.scope_ids.usage_id) - except InvalidLocationError: - if isinstance(self.scope_ids.usage_id, BlockUsageLocator): - return self.scope_ids.usage_id - else: - return BlockUsageLocator(self.scope_ids.usage_id) - - @location.setter - def location(self, value): - self.scope_ids = self.scope_ids._replace( - def_id=value, - usage_id=value, - ) - - @property - def url_name(self): - if isinstance(self.location, Location): - return self.location.name - elif isinstance(self.location, BlockUsageLocator): - return self.location.usage_id - else: - raise InsufficientSpecificationError() - - - @property - def display_name_with_default(self): - ''' - Return a display name for the module: use display_name if defined in - metadata, otherwise convert the url name. - ''' - name = self.display_name - if name is None: - name = self.url_name.replace('_', ' ') - return name - - def get_required_module_descriptors(self): - """Returns a list of XModuleDescritpor instances upon which this module depends, but are - not children of this module""" - return [] - - def get_children(self): - """Returns a list of XModuleDescriptor instances for the children of - this module""" - if not self.has_children: - return [] - - if self._child_instances is None: - self._child_instances = [] - for child_loc in self.children: - if isinstance(child_loc, XModuleDescriptor): - child = child_loc - else: - try: - child = self.runtime.get_block(child_loc) - except ItemNotFoundError: - log.exception('Unable to load item {loc}, skipping'.format(loc=child_loc)) - continue - self._child_instances.append(child) - - return self._child_instances - - def get_child_by(self, selector): - """ - Return a child XModuleDescriptor with the specified url_name, if it exists, and None otherwise. - """ - for child in self.get_children(): - if selector(child): - return child - return None def xmodule(self, system): """ @@ -619,15 +584,6 @@ class XModuleDescriptor(XModuleFields, HTMLSnippet, ResourceTemplates, XBlock): raise NotImplementedError( 'Modules must implement export_to_xml to enable xml export') - @property - def xblock_kvs(self): - """ - Use w/ caution. Really intended for use by the persistence layer. - """ - # if caller wants kvs, caller's assuming it's up to date; so, decache it - self.save() - return self._field_data._kvs - # =============================== BUILTIN METHODS ========================== def __eq__(self, other): return (self.scope_ids == other.scope_ids and @@ -655,17 +611,6 @@ class XModuleDescriptor(XModuleFields, HTMLSnippet, ResourceTemplates, XBlock): return [XBlock.tags, XBlock.name] - def get_explicitly_set_fields_by_scope(self, scope=Scope.content): - """ - Get a dictionary of the fields for the given scope which are set explicitly on this xblock. (Including - any set to None.) - """ - result = {} - for field in self.fields.values(): - if (field.scope == scope and field.is_set_on(self)): - result[field.name] = field.read_json(self) - return result - @property def editable_metadata_fields(self): """ @@ -825,7 +770,7 @@ class XMLParsingSystem(DescriptorSystem): class ModuleSystem(Runtime): - ''' + """ This is an abstraction such that x_modules can function independent of the courseware (e.g. import into other types of courseware, LMS, or if we want to have a sandbox server for user-contributed content) @@ -835,7 +780,7 @@ class ModuleSystem(Runtime): Note that these functions can be closures over e.g. a django request and user, or other environment-specific info. - ''' + """ def __init__( self, ajax_url, track_function, get_module, render_template, replace_urls, xmodule_field_data, user=None, filestore=None, @@ -844,7 +789,7 @@ class ModuleSystem(Runtime): open_ended_grading_interface=None, s3_interface=None, cache=None, can_execute_unsafe_code=None, replace_course_urls=None, replace_jump_to_id_urls=None, **kwargs): - ''' + """ Create a closure around the system environment. ajax_url - the url where ajax calls to the encapsulating module go. @@ -893,7 +838,7 @@ class ModuleSystem(Runtime): can_execute_unsafe_code - A function returning a boolean, whether or not to allow the execution of unsafe, unsandboxed code. - ''' + """ super(ModuleSystem, self).__init__(**kwargs) self.ajax_url = ajax_url @@ -926,11 +871,11 @@ class ModuleSystem(Runtime): self.replace_jump_to_id_urls = replace_jump_to_id_urls def get(self, attr): - ''' provide uniform access to attributes (like etree).''' + """ provide uniform access to attributes (like etree).""" return self.__dict__.get(attr) def set(self, attr, val): - '''provide uniform access to attributes (like etree)''' + """provide uniform access to attributes (like etree)""" self.__dict__[attr] = val def __repr__(self): diff --git a/lms/envs/common.py b/lms/envs/common.py index 941809b82f..3858b1cfb1 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -32,6 +32,7 @@ from .discussionsettings import * from lms.xblock.mixin import LmsBlockMixin from xmodule.modulestore.inheritance import InheritanceMixin +from xmodule.x_module import XModuleMixin ################################### FEATURES ################################### # The display name of the platform to be used in templates/emails/etc. @@ -363,7 +364,7 @@ CONTENTSTORE = None # This should be moved into an XBlock Runtime/Application object # once the responsibility of XBlock creation is moved out of modulestore - cpennington -XBLOCK_MIXINS = (LmsBlockMixin, InheritanceMixin) +XBLOCK_MIXINS = (LmsBlockMixin, InheritanceMixin, XModuleMixin) #################### Python sandbox ############################################ From d75cdda064b00924a42abab77476e2967682edb6 Mon Sep 17 00:00:00 2001 From: Giulio Gratta Date: Thu, 26 Sep 2013 11:22:16 -0700 Subject: [PATCH 22/54] An assortment of small OE changes, both for ICE and standard - Changed save OE message text - Fix ICE legend positioning and content - Add space to "offensive" checkbox - Change tracker variable in undo to correct one --- .../xmodule/xmodule/js/src/combinedopenended/display.coffee | 2 +- .../xmodule/xmodule/js/src/peergrading/track_changes.coffee | 2 +- lms/static/sass/course/_staff_grading.scss | 2 +- lms/templates/peer_grading/peer_grading_problem.html | 5 +++-- 4 files changed, 6 insertions(+), 5 deletions(-) diff --git a/common/lib/xmodule/xmodule/js/src/combinedopenended/display.coffee b/common/lib/xmodule/xmodule/js/src/combinedopenended/display.coffee index d1006ae9a9..7103d2e841 100644 --- a/common/lib/xmodule/xmodule/js/src/combinedopenended/display.coffee +++ b/common/lib/xmodule/xmodule/js/src/combinedopenended/display.coffee @@ -353,7 +353,7 @@ class @CombinedOpenEnded @save_button.attr("disabled",true) $.postWithPrefix "#{@ajax_url}/store_answer", data, (response) => if response.success - @gentle_alert("Answer saved.") + @gentle_alert("Answer saved, but not yet submitted.") else @errors_area.html(response.error) @save_button.attr("disabled",false) diff --git a/common/lib/xmodule/xmodule/js/src/peergrading/track_changes.coffee b/common/lib/xmodule/xmodule/js/src/peergrading/track_changes.coffee index ff0b4111f2..72df455b0b 100644 --- a/common/lib/xmodule/xmodule/js/src/peergrading/track_changes.coffee +++ b/common/lib/xmodule/xmodule/js/src/peergrading/track_changes.coffee @@ -56,7 +56,7 @@ class @TrackChanges key = parseInt(@attr('data-cid')) if key > keyOfLatestChange keyOfLatestChange = key - ICEtracker.rejectChange('[data-cid="'+ keyOfLatestChange + '"]') + @tracker.rejectChange('[data-cid="'+ keyOfLatestChange + '"]') stop_tracking_on_submit: () => @tracker.stopTracking() \ No newline at end of file diff --git a/lms/static/sass/course/_staff_grading.scss b/lms/static/sass/course/_staff_grading.scss index 6810fe1bff..fd48ac28dc 100644 --- a/lms/static/sass/course/_staff_grading.scss +++ b/lms/static/sass/course/_staff_grading.scss @@ -18,7 +18,7 @@ div.peer-grading{ overflow: auto; } - div.feedback-area.track-changes, p.legend { + div.feedback-area.track-changes, p.ice-legend { .ice-controls { float: right; } diff --git a/lms/templates/peer_grading/peer_grading_problem.html b/lms/templates/peer_grading/peer_grading_problem.html index be357bf054..bc0bbdfa34 100644 --- a/lms/templates/peer_grading/peer_grading_problem.html +++ b/lms/templates/peer_grading/peer_grading_problem.html @@ -55,8 +55,8 @@ ${_("This is a deletion.")}  ${_("[This is a comment.]")}  + Undo Change   Reset Changes - Undo Change

      @@ -65,8 +65,9 @@ % endif
      +
      - ${_("This submission has explicit, offensive, or (I suspect) plagiarized content: ")} + ${_("This submission has explicit, offensive, or (I suspect) plagiarized content. ")}
      From 64d4ebdaa15f2a6a4e22f5f28d0b43137883d37f Mon Sep 17 00:00:00 2001 From: Usman Khalid Date: Sat, 28 Sep 2013 00:14:07 +0500 Subject: [PATCH 23/54] Revert last commit --- common/static/js/pdfviewer.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/common/static/js/pdfviewer.js b/common/static/js/pdfviewer.js index 3424a09892..a76e2f397c 100755 --- a/common/static/js/pdfviewer.js +++ b/common/static/js/pdfviewer.js @@ -327,11 +327,11 @@ PDFJS.disableWorker = true; loadUrl(chapterUrl, pageVal); } - $(".last").on('click','#previous',function(){ + $("#previous").click(function(event) { prevPage(); }); - $(".next").on('click','#next',function(){ + $("#next").click(function(event) { nextPage(); }); From 67ccf1e3bb291be3e36772d0fa0159a71862cbdb Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 24 Sep 2013 15:10:06 -0400 Subject: [PATCH 24/54] Move nose/django arguments for tests out of rake so that raw manage.py works when running tests --- cms/envs/test.py | 10 ++++++++++ lms/envs/test.py | 13 +++++++++++++ rakelib/tests.rake | 12 ++---------- 3 files changed, 25 insertions(+), 10 deletions(-) diff --git a/cms/envs/test.py b/cms/envs/test.py index 364bee2441..f3eee45ea7 100644 --- a/cms/envs/test.py +++ b/cms/envs/test.py @@ -20,6 +20,16 @@ from warnings import filterwarnings # Nose Test Runner TEST_RUNNER = 'django_nose.NoseTestSuiteRunner' +_system = 'cms' +_report_dir = REPO_ROOT / 'reports' / _system +_report_dir.makedirs_p() + +NOSE_ARGS = [ + '--tests', PROJECT_ROOT / 'djangoapps', COMMON_ROOT / 'djangoapps', + '--id-file', REPO_ROOT / '.testids' / _system / 'noseids', + '--xunit-file', _report_dir / 'nosetests.xml', +] + TEST_ROOT = path('test_root') # Want static files in the same dir for running on jenkins. diff --git a/lms/envs/test.py b/lms/envs/test.py index 30475e26a0..f79c8c4218 100644 --- a/lms/envs/test.py +++ b/lms/envs/test.py @@ -17,6 +17,8 @@ import os from path import path from warnings import filterwarnings +os.environ['DJANGO_LIVE_TEST_SERVER_ADDRESS'] = 'localhost:8000-9000' + # can't test start dates with this True, but on the other hand, # can test everything else :) MITX_FEATURES['DISABLE_START_DATES'] = True @@ -43,6 +45,17 @@ SOUTH_TESTS_MIGRATE = False # To disable migrations and use syncdb instead # Nose Test Runner TEST_RUNNER = 'django_nose.NoseTestSuiteRunner' +_system = 'lms' + +_report_dir = REPO_ROOT / 'reports' / _system +_report_dir.makedirs_p() + +NOSE_ARGS = [ + '--tests', PROJECT_ROOT / 'djangoapps', COMMON_ROOT / 'djangoapps', + '--id-file', REPO_ROOT / '.testids' / _system / 'noseids', + '--xunit-file', _report_dir / 'nosetests.xml', +] + # Local Directories TEST_ROOT = path("test_root") # Want static files in the same dir for running on jenkins. diff --git a/rakelib/tests.rake b/rakelib/tests.rake index fb6cf575bd..ba436f3680 100644 --- a/rakelib/tests.rake +++ b/rakelib/tests.rake @@ -17,16 +17,8 @@ def run_under_coverage(cmd, root) end def run_tests(system, report_dir, test_id=nil, stop_on_failure=true) - ENV['NOSE_XUNIT_FILE'] = File.join(report_dir, "nosetests.xml") - test_id_file = File.join(test_id_dir(system), "noseids") - dirs = Dir["common/djangoapps/*"] + Dir["#{system}/djangoapps/*"] - test_id = dirs.join(' ') if test_id.nil? or test_id == '' - cmd = django_admin( - system, :test, 'test', - '--logging-clear-handlers', - '--liveserver=localhost:8000-9000', - "--id-file=#{test_id_file}", - test_id) + test_id = '' if test_id.nil? + cmd = django_admin(system, :test, 'test', test_id) test_sh(run_under_coverage(cmd, system)) end From 9a6ae8f7403bfe310e9e1334850fb3d4f9042392 Mon Sep 17 00:00:00 2001 From: Vik Paruchuri Date: Fri, 20 Sep 2013 13:53:09 -0400 Subject: [PATCH 25/54] Clean up open ended problems view and fix error. --- lms/djangoapps/open_ended_grading/tests.py | 115 +++++++++- lms/djangoapps/open_ended_grading/utils.py | 147 ++++++++++++ lms/djangoapps/open_ended_grading/views.py | 212 ++++++------------ .../open_ended_problems.html | 56 +++-- 4 files changed, 350 insertions(+), 180 deletions(-) diff --git a/lms/djangoapps/open_ended_grading/tests.py b/lms/djangoapps/open_ended_grading/tests.py index 5a0d9e69e5..5224386664 100644 --- a/lms/djangoapps/open_ended_grading/tests.py +++ b/lms/djangoapps/open_ended_grading/tests.py @@ -18,8 +18,9 @@ from xmodule.modulestore.django import modulestore from xmodule.x_module import ModuleSystem from xblock.fields import ScopeIds -from open_ended_grading import staff_grading_service, views +from open_ended_grading import staff_grading_service, views, utils from courseware.access import _course_staff_group_name +from student.models import unique_id_for_user import logging @@ -46,6 +47,57 @@ class EmptyStaffGradingService(object): """ return json.dumps({'success': True, 'error': 'No problems found.'}) +def make_instructor(course, user_email): + """ + Makes a given user an instructor in a course. + """ + group_name = _course_staff_group_name(course.location) + group = Group.objects.create(name=group_name) + group.user_set.add(User.objects.get(email=user_email)) + +class StudentProblemListMockQuery(object): + """ + Mock controller query service for testing student problem list functionality. + """ + def get_grading_status_list(self, *args, **kwargs): + """ + Get a mock grading status list with locations from the open_ended test course. + @returns: json formatted grading status message. + """ + grading_status_list = json.dumps( + { + "version": 1, + "problem_list": [ + { + "problem_name": "Test1", + "grader_type": "IN", + "eta_available": True, + "state": "Finished", + "eta": 259200, + "location": "i4x://edX/open_ended/combinedopenended/SampleQuestion1Attempt" + }, + { + "problem_name": "Test2", + "grader_type": "NA", + "eta_available": True, + "state": "Waiting to be Graded", + "eta": 259200, + "location": "i4x://edX/open_ended/combinedopenended/SampleQuestion" + }, + { + "problem_name": "Test3", + "grader_type": "PE", + "eta_available": True, + "state": "Waiting to be Graded", + "eta": 259200, + "location": "i4x://edX/open_ended/combinedopenended/SampleQuestion454" + }, + ], + "success": True + } + ) + return grading_status_list + @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE) class TestStaffGradingService(ModuleStoreTestCase, LoginEnrollmentTestCase): ''' @@ -67,12 +119,7 @@ class TestStaffGradingService(ModuleStoreTestCase, LoginEnrollmentTestCase): self.course_id = "edX/toy/2012_Fall" self.toy = modulestore().get_course(self.course_id) - def make_instructor(course): - group_name = _course_staff_group_name(course.location) - group = Group.objects.create(name=group_name) - group.user_set.add(User.objects.get(email=self.instructor)) - - make_instructor(self.toy) + make_instructor(self.toy, self.instructor) self.mock_service = staff_grading_service.staff_grading_service() @@ -324,7 +371,7 @@ class TestPeerGradingService(ModuleStoreTestCase, LoginEnrollmentTestCase): @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE) -class TestPanel(ModuleStoreTestCase, LoginEnrollmentTestCase): +class TestPanel(ModuleStoreTestCase): """ Run tests on the open ended panel """ @@ -343,7 +390,15 @@ class TestPanel(ModuleStoreTestCase, LoginEnrollmentTestCase): found_module, peer_grading_module = views.find_peer_grading_module(self.course) self.assertTrue(found_module) - @patch('open_ended_grading.views.controller_qs', controller_query_service.MockControllerQueryService(settings.OPEN_ENDED_GRADING_INTERFACE, views.system)) + @patch( + 'open_ended_grading.utils.create_controller_query_service', + Mock( + return_value=controller_query_service.MockControllerQueryService( + settings.OPEN_ENDED_GRADING_INTERFACE, + utils.system + ) + ) + ) def test_problem_list(self): """ Ensure that the problem list from the grading controller server can be rendered properly locally @@ -370,4 +425,44 @@ class TestPeerGradingFound(ModuleStoreTestCase): """ found, url = views.find_peer_grading_module(self.course) - self.assertEqual(found, False) \ No newline at end of file + self.assertEqual(found, False) + +@override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE) +class TestStudentProblemList(ModuleStoreTestCase): + """ + Test if the student problem list correctly fetches and parses problems. + """ + + def setUp(self): + # Load an open ended course with several problems. + self.course_name = 'edX/open_ended/2012_Fall' + self.course = modulestore().get_course(self.course_name) + self.user = factories.UserFactory() + # Enroll our user in our course and make them an instructor. + make_instructor(self.course, self.user.email) + + @patch( + 'open_ended_grading.utils.create_controller_query_service', + Mock(return_value=StudentProblemListMockQuery()) + ) + def test_get_problem_list(self): + """ + Test to see if the StudentProblemList class can get and parse a problem list from ORA. + Mock the get_grading_status_list function using StudentProblemListMockQuery. + """ + # Initialize a StudentProblemList object. + student_problem_list = utils.StudentProblemList(self.course.id, unique_id_for_user(self.user)) + # Get the initial problem list from ORA. + success = student_problem_list.fetch_from_grading_service() + # Should be successful, and we should have three problems. See mock class for details. + self.assertTrue(success) + self.assertEqual(len(student_problem_list.problem_list), 3) + + # See if the problem locations are valid. + valid_problems = student_problem_list.add_problem_data(reverse('courses')) + # One location is invalid, so we should now have two. + self.assertEqual(len(valid_problems), 2) + # Ensure that human names are being set properly. + self.assertEqual(valid_problems[0]['grader_type_display_name'], "Instructor Assessment") + + diff --git a/lms/djangoapps/open_ended_grading/utils.py b/lms/djangoapps/open_ended_grading/utils.py index 15aaf0246f..1f860b1a6d 100644 --- a/lms/djangoapps/open_ended_grading/utils.py +++ b/lms/djangoapps/open_ended_grading/utils.py @@ -1,10 +1,58 @@ +import json + from xmodule.modulestore import search from xmodule.modulestore.django import modulestore from xmodule.modulestore.exceptions import ItemNotFoundError, NoPathToItem +from xmodule.x_module import ModuleSystem +from xmodule.open_ended_grading_classes.controller_query_service import ControllerQueryService +from xmodule.open_ended_grading_classes.grading_service_module import GradingServiceError + +from django.utils.translation import ugettext as _ +from django.conf import settings + +from mitxmako.shortcuts import render_to_string + +from xblock.field_data import DictFieldData + import logging log = logging.getLogger(__name__) +GRADER_DISPLAY_NAMES = { + 'ML': _("AI Assessment"), + 'PE': _("Peer Assessment"), + 'NA': _("Not yet available"), + 'BC': _("Automatic Checker"), + 'IN': _("Instructor Assessment"), + } + +STUDENT_ERROR_MESSAGE = _("Error occurred while contacting the grading service. Please notify course staff.") +STAFF_ERROR_MESSAGE = _("Error occurred while contacting the grading service. Please notify your edX point of contact.") + +system = ModuleSystem( + ajax_url=None, + track_function=None, + get_module=None, + render_template=render_to_string, + replace_urls=None, + xmodule_field_data=DictFieldData({}), + ) + +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 + @param problem_url_parts: Output of search.path_to_location + @param base_course_url: Base url of a given course + @return: A path to the problem + """ + problem_url = base_course_url + "/" + for i, part in enumerate(problem_url_parts): + if part is not None: + if i == 1: + problem_url += "courseware/" + problem_url += part + "/" + return problem_url + def does_location_exist(course_id, location): """ Checks to see if a valid module exists at a given location (ie has not been deleted) @@ -25,3 +73,102 @@ def does_location_exist(course_id, location): log.warn(("Got an unexpected NoPathToItem error in staff grading with a non-draft location {0}. " "Ensure that the location is valid.").format(location)) return False + +def create_controller_query_service(): + """ + Return an instance of a service that can query edX ORA. + """ + + return ControllerQueryService(settings.OPEN_ENDED_GRADING_INTERFACE, system) + +class StudentProblemList(object): + """ + Get a list of problems that the student has attempted from ORA. + Add in metadata as needed. + """ + def __init__(self, course_id, user_id): + """ + @param course_id: The id of a course object. Get using course.id. + @param user_id: The anonymous id of the user, from the unique_id_for_user function. + """ + self.course_id = course_id + self.user_id = user_id + + # We want to append this string to all of our error messages. + self.course_error_ending = _("for course {0} and student {1}.").format(self.course_id, user_id) + + # This is our generic error message. + self.error_text = STUDENT_ERROR_MESSAGE + self.success = False + + # Create a service to query edX ORA. + self.controller_qs = create_controller_query_service() + + def fetch_from_grading_service(self): + """ + Fetch a list of problems that the student has answered from ORA. + Handle various error conditions. + @return: A boolean success indicator. + """ + # In the case of multiple calls, ensure that success is false initially. + self.success = False + try: + #Get list of all open ended problems that the grading server knows about + problem_list_json = self.controller_qs.get_grading_status_list(self.course_id, self.user_id) + except GradingServiceError: + log.error("Problem contacting open ended grading service " + self.course_error_ending) + return self.success + try: + problem_list_dict = json.loads(problem_list_json) + except ValueError: + log.error("Problem with results from external grading service for open ended" + self.course_error_ending) + return self.success + + success = problem_list_dict['success'] + if 'error' in problem_list_dict: + self.error_text = problem_list_dict['error'] + return success + if 'problem_list' not in problem_list_dict: + log.error("Did not receive a problem list in ORA response" + self.course_error_ending) + return success + + self.problem_list = problem_list_dict['problem_list'] + + self.success = True + return self.success + + def add_problem_data(self, base_course_url): + """ + Add metadata to problems. + @param base_course_url: the base url for any course. Can get with reverse('course') + @return: A list of valid problems in the course and their appended data. + """ + # Our list of valid problems. + valid_problems = [] + + if not self.success or not isinstance(self.problem_list, list): + log.error("Called add_problem_data without a valid problem list" + self.course_error_ending) + return valid_problems + + # Iterate through all of our problems and add data. + for problem in self.problem_list: + try: + # Try to load the problem. + problem_url_parts = search.path_to_location(modulestore(), self.course_id, problem['location']) + except (ItemNotFoundError, NoPathToItem): + # If the problem cannot be found at the location received from the grading controller server, + # it has been deleted by the course author. We should not display it. + error_message = "Could not find module for course {0} at location {1}".format(self.course_id, + problem['location']) + log.error(error_message) + continue + + # Get the problem url in the courseware. + problem_url = generate_problem_url(problem_url_parts, base_course_url) + + # Map the grader name from ORA to a human readable version. + grader_type_display_name = GRADER_DISPLAY_NAMES.get(problem['grader_type'], "edX Assessment") + problem['actual_url'] = problem_url + problem['grader_type_display_name'] = grader_type_display_name + valid_problems.append(problem) + return valid_problems diff --git a/lms/djangoapps/open_ended_grading/views.py b/lms/djangoapps/open_ended_grading/views.py index dcc1e11730..e8002e0883 100644 --- a/lms/djangoapps/open_ended_grading/views.py +++ b/lms/djangoapps/open_ended_grading/views.py @@ -1,5 +1,3 @@ -# Grading Views - import logging from django.conf import settings @@ -7,13 +5,9 @@ from django.views.decorators.cache import cache_control from mitxmako.shortcuts import render_to_response from django.core.urlresolvers import reverse -from xblock.field_data import DictFieldData - from student.models import unique_id_for_user from courseware.courses import get_course_with_access -from xmodule.x_module import ModuleSystem -from xmodule.open_ended_grading_classes.controller_query_service import ControllerQueryService, convert_seconds_to_human_readable from xmodule.open_ended_grading_classes.grading_service_module import GradingServiceError import json from student.models import unique_id_for_user @@ -26,28 +20,21 @@ from xmodule.modulestore.exceptions import ItemNotFoundError, NoPathToItem from django.http import HttpResponse, Http404, HttpResponseRedirect from mitxmako.shortcuts import render_to_string +from django.utils.translation import ugettext as _ + +from open_ended_grading.utils import (STAFF_ERROR_MESSAGE, STUDENT_ERROR_MESSAGE, + StudentProblemList, generate_problem_url, create_controller_query_service) log = logging.getLogger(__name__) -system = ModuleSystem( - ajax_url=None, - track_function=None, - get_module=None, - render_template=render_to_string, - replace_urls=None, - xmodule_field_data=DictFieldData({}), -) - -controller_qs = ControllerQueryService(settings.OPEN_ENDED_GRADING_INTERFACE, system) - -""" -Reverses the URL from the name and the course id, and then adds a trailing slash if -it does not exist yet - -""" - - def _reverse_with_slash(url_name, course_id): + """ + Reverses the URL given the name and the course id, and then adds a trailing slash if + it does not exist yet. + @param url_name: The name of the url (eg 'staff_grading'). + @param course_id: The id of the course object (eg course.id). + @returns: The reversed url with a trailing slash. + """ ajax_url = _reverse_without_slash(url_name, course_id) if not ajax_url.endswith('/'): ajax_url += '/' @@ -60,22 +47,19 @@ def _reverse_without_slash(url_name, course_id): DESCRIPTION_DICT = { - 'Peer Grading': "View all problems that require peer assessment in this particular course.", - 'Staff Grading': "View ungraded submissions submitted by students for the open ended problems in the course.", - 'Problems you have submitted': "View open ended problems that you have previously submitted for grading.", - 'Flagged Submissions': "View submissions that have been flagged by students as inappropriate." + 'Peer Grading': _("View all problems that require peer assessment in this particular course."), + 'Staff Grading': _("View ungraded submissions submitted by students for the open ended problems in the course."), + 'Problems you have submitted': _("View open ended problems that you have previously submitted for grading."), + 'Flagged Submissions': _("View submissions that have been flagged by students as inappropriate."), } + ALERT_DICT = { - 'Peer Grading': "New submissions to grade", - 'Staff Grading': "New submissions to grade", - 'Problems you have submitted': "New grades have been returned", - 'Flagged Submissions': "Submissions have been flagged for review" + 'Peer Grading': _("New submissions to grade"), + 'Staff Grading': _("New submissions to grade"), + 'Problems you have submitted': _("New grades have been returned"), + 'Flagged Submissions': _("Submissions have been flagged for review"), } -STUDENT_ERROR_MESSAGE = "Error occurred while contacting the grading service. Please notify course staff." -STAFF_ERROR_MESSAGE = "Error occurred while contacting the grading service. Please notify the development team. If you do not have a point of contact, please email Vik at vik@edx.org" - - @cache_control(no_cache=True, no_store=True, must_revalidate=True) def staff_grading(request, course_id): """ @@ -107,8 +91,6 @@ def find_peer_grading_module(course): # Get the course id and split it. course_id_parts = course.id.split("/") - 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) @@ -123,7 +105,7 @@ def find_peer_grading_module(course): except NoPathToItem: # In the case of nopathtoitem, the peer grading module that was found is in an invalid state, and # can no longer be accessed. Log an informational message, but this will not impact normal behavior. - log.info("Invalid peer grading module location {0} in course {1}. This module may need to be removed.".format(item_location, course.id)) + log.info(u"Invalid peer grading module location {0} in course {1}. This module may need to be removed.".format(item_location, course.id)) continue problem_url = generate_problem_url(problem_url_parts, base_course_url) found_module = True @@ -143,121 +125,60 @@ 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_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)) + """) + log.exception(error_message + u"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): - """ - From a list of problem url parts generated by search.path_to_location and a base course url, generates a url to a problem - @param problem_url_parts: Output of search.path_to_location - @param base_course_url: Base url of a given course - @return: A path to the problem - """ - problem_url = base_course_url + "/" - for z in xrange(0, len(problem_url_parts)): - part = problem_url_parts[z] - if part is not None: - if z == 1: - problem_url += "courseware/" - problem_url += part + "/" - return problem_url - - @cache_control(no_cache=True, no_store=True, must_revalidate=True) def student_problem_list(request, course_id): - ''' - 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. - ''' + """ + Show a list of problems they have attempted to a student. + Fetch the list from the grading controller server and append some data. + @param request: The request object for this view. + @param course_id: The id of the course to get the problem list for. + @return: Renders an HTML problem list table. + """ + + # Load the course. Don't catch any errors here, as we want them to be loud. course = get_course_with_access(request.user, course_id, 'load') + + # The anonymous student id is needed for communication with ORA. student_id = unique_id_for_user(request.user) - - # call problem list service - success = False - error_text = "" - problem_list = [] base_course_url = reverse('courses') - list_to_remove = [] + error_text = "" - 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'] - if 'error' in problem_list_dict: - error_text = problem_list_dict['error'] - problem_list = [] - else: - problem_list = problem_list_dict['problem_list'] + student_problem_list = StudentProblemList(course_id, student_id) + # Get the problem list from ORA. + success = student_problem_list.fetch_from_grading_service() + # If we fetched the problem list properly, add in additional problem data. + if success: + # Add in links to problems. + valid_problems = student_problem_list.add_problem_data(base_course_url) + else: + # Get an error message to show to the student. + valid_problems = [] + error_text = student_problem_list.error_text - #A list of problems to remove (problems that can't be found in the course) - 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 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']) - 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}) - eta_available = problem_list[i]['eta_available'] - if isinstance(eta_available, basestring): - eta_available = (eta_available.lower() == "true") - - eta_string = "N/A" - if eta_available: - try: - eta_string = convert_seconds_to_human_readable(int(problem_list[i]['eta'])) - except: - #This is a student_facing_error - eta_string = "Error getting ETA." - problem_list[i].update({'eta_string': eta_string}) - - except GradingServiceError: - #This is a student_facing_error - error_text = STUDENT_ERROR_MESSAGE - #This is a dev facing error - log.error("Problem contacting open ended grading service.") - success = False - # catch error if if the json loads fails - except ValueError: - #This is a student facing error - error_text = STUDENT_ERROR_MESSAGE - #This is a dev_facing_error - 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', { + context = { 'course': course, 'course_id': course_id, 'ajax_url': ajax_url, 'success': success, - 'problem_list': problem_list, + 'problem_list': valid_problems, 'error_text': error_text, # Checked above - 'staff_access': False, }) + 'staff_access': False, + } + return render_to_response('open_ended_problems/open_ended_problems.html', context) @cache_control(no_cache=True, no_store=True, must_revalidate=True) def flagged_problem_list(request, course_id): @@ -273,6 +194,8 @@ def flagged_problem_list(request, course_id): problem_list = [] base_course_url = reverse('courses') + # Make a service that can query edX ORA. + controller_qs = create_controller_query_service() try: problem_list_json = controller_qs.get_flagged_problem_list(course_id) problem_list_dict = json.loads(problem_list_json) @@ -384,11 +307,12 @@ def take_action_on_flags(request, course_id): required = ['submission_id', 'action_type', 'student_id'] for key in required: if key not in request.POST: - #This is a staff_facing_error - return HttpResponse(json.dumps({'success': False, - 'error': STAFF_ERROR_MESSAGE + 'Missing key {0} from submission. Please reload and try again.'.format( - key)}), - mimetype="application/json") + error_message = u'Missing key {0} from submission. Please reload and try again.'.format(key) + response = { + 'success': False, + 'error': STAFF_ERROR_MESSAGE + error_message + } + return HttpResponse(json.dumps(response), mimetype="application/json") p = request.POST submission_id = p['submission_id'] @@ -397,12 +321,20 @@ def take_action_on_flags(request, course_id): student_id = student_id.strip(' \t\n\r') submission_id = submission_id.strip(' \t\n\r') action_type = action_type.lower().strip(' \t\n\r') + + # Make a service that can query edX ORA. + controller_qs = create_controller_query_service() try: response = controller_qs.take_action_on_flags(course_id, student_id, submission_id, action_type) return HttpResponse(response, mimetype="application/json") except GradingServiceError: - #This is a dev_facing_error log.exception( - "Error taking action on flagged peer grading submissions, submission_id: {0}, action_type: {1}, grader_id: {2}".format( - submission_id, action_type, grader_id)) - return _err_response(STAFF_ERROR_MESSAGE) + u"Error taking action on flagged peer grading submissions, " + u"submission_id: {0}, action_type: {1}, grader_id: {2}".format( + submission_id, action_type, student_id) + ) + response = { + 'success': False, + 'error': STAFF_ERROR_MESSAGE + } + return HttpResponse(json.dumps(response),mimetype="application/json") diff --git a/lms/templates/open_ended_problems/open_ended_problems.html b/lms/templates/open_ended_problems/open_ended_problems.html index aa053111d8..1c7e2657ed 100644 --- a/lms/templates/open_ended_problems/open_ended_problems.html +++ b/lms/templates/open_ended_problems/open_ended_problems.html @@ -19,36 +19,32 @@

      ${_("Instructions")}

      ${_("Here is a list of open ended problems for this course.")}

      % if success: - % if len(problem_list) == 0: -
      - ${_("You have not attempted any open ended problems yet.")} -
      - %else: - - - - - - - - %for problem in problem_list: - - - - - - - %endfor -
      ${_("Problem Name")}${_("Status")}${_("Grader Type")}${_("ETA")}
      - ${problem['problem_name']} - - ${problem['state']} - - ${problem['grader_type']} - - ${problem['eta_string']} -
      - %endif + % if len(problem_list) == 0: +
      + ${_("You have not attempted any open ended problems yet.")} +
      + %else: + + + + + + + %for problem in problem_list: + + + + + + %endfor +
      ${_("Problem Name")}${_("Status")}${_("Grader Type")}
      + ${problem['problem_name']} + + ${problem['state']} + + ${problem['grader_type_display_name']} +
      + %endif %endif From f8c727847067fe53d79c0a45c0019c1045c670e1 Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Thu, 26 Sep 2013 11:25:10 -0400 Subject: [PATCH 26/54] Follow patterns Move init to top of class Change asserts to more meaningful exception types Refactor hard-to-read fn --- .../xmodule/xmodule/modulestore/locator.py | 98 +++++++++---------- .../modulestore/tests/test_locators.py | 6 +- 2 files changed, 52 insertions(+), 52 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/locator.py b/common/lib/xmodule/xmodule/modulestore/locator.py index dfeafa5e1d..097f69c7c2 100644 --- a/common/lib/xmodule/xmodule/modulestore/locator.py +++ b/common/lib/xmodule/xmodule/modulestore/locator.py @@ -114,6 +114,30 @@ class CourseLocator(Locator): course_id = None branch = None + def __init__(self, url=None, version_guid=None, course_id=None, branch=None): + """ + Construct a CourseLocator + Caller may provide url (but no other parameters). + Caller may provide version_guid (but no other parameters). + Caller may provide course_id (optionally provide branch). + + Resulting CourseLocator will have either a version_guid property + or a course_id (with optional branch) property, or both. + + version_guid must be an instance of bson.objectid.ObjectId or None + url, course_id, and branch must be strings or None + + """ + self._validate_args(url, version_guid, course_id) + if url: + self.init_from_url(url) + if version_guid: + self.init_from_version_guid(version_guid) + if course_id or branch: + self.init_from_course_id(course_id, branch) + assert self.version_guid or self.course_id, \ + "Either version_guid or course_id should be set." + def __unicode__(self): """ Return a string representing this location. @@ -135,18 +159,13 @@ class CourseLocator(Locator): """ return 'edx://' + unicode(self) - # -- unused args which are used via inspect - # pylint: disable= W0613 - def validate_args(self, url, version_guid, course_id, branch): + def _validate_args(self, url, version_guid, course_id): """ - Validate provided arguments. + Validate provided arguments. Internal use only which is why it checks for each + arg and doesn't use keyword """ - need_oneof = set(('url', 'version_guid', 'course_id')) - args, _, _, values = inspect.getargvalues(inspect.currentframe()) - provided_args = [a for a in args if a != 'self' and values[a] is not None] - if len(need_oneof.intersection(provided_args)) == 0: - raise InsufficientSpecificationError("Must provide one of these args: %s " % - list(need_oneof)) + if not any((url, version_guid, course_id)): + raise InsufficientSpecificationError("Must provide one of url, version_guid, course_id") def is_fully_specified(self): """ @@ -154,8 +173,8 @@ class CourseLocator(Locator): are specified. This should always return True, since this should be validated in the constructor. """ - return self.version_guid is not None \ - or (self.course_id is not None and self.branch is not None) + return (self.version_guid is not None or + (self.course_id is not None and self.branch is not None)) def set_course_id(self, new): """ @@ -189,30 +208,6 @@ class CourseLocator(Locator): version_guid=self.version_guid, branch=self.branch) - def __init__(self, url=None, version_guid=None, course_id=None, branch=None): - """ - Construct a CourseLocator - Caller may provide url (but no other parameters). - Caller may provide version_guid (but no other parameters). - Caller may provide course_id (optionally provide branch). - - Resulting CourseLocator will have either a version_guid property - or a course_id (with optional branch) property, or both. - - version_guid must be an instance of bson.objectid.ObjectId or None - url, course_id, and branch must be strings or None - - """ - self.validate_args(url, version_guid, course_id, branch) - if url: - self.init_from_url(url) - if version_guid: - self.init_from_version_guid(version_guid) - if course_id or branch: - self.init_from_course_id(course_id, branch) - assert self.version_guid or self.course_id, \ - "Either version_guid or course_id should be set." - @classmethod def as_object_id(cls, value): """ @@ -233,9 +228,11 @@ class CourseLocator(Locator): """ if isinstance(url, Locator): url = url.url() - assert isinstance(url, basestring), '%s is not an instance of basestring' % url + if not isinstance(url, basestring): + raise TypeError('%s is not an instance of basestring' % url) parse = parse_url(url) - assert parse, 'Could not parse "%s" as a url' % url + if not parse: + raise ValueError('Could not parse "%s" as a url' % url) self._set_value( parse, 'version_guid', lambda (new_guid): self.set_version_guid(self.as_object_id(new_guid)) ) @@ -250,13 +247,13 @@ class CourseLocator(Locator): """ version_guid = self.as_object_id(version_guid) - assert isinstance(version_guid, ObjectId), \ - '%s is not an instance of ObjectId' % version_guid + if not isinstance(version_guid, ObjectId): + raise TypeError('%s is not an instance of ObjectId' % version_guid) self.set_version_guid(version_guid) def init_from_course_id(self, course_id, explicit_branch=None): """ - Course_id is a string like 'mit.eecs.6002x' or 'mit.eecs.6002x/branch/published'. + Course_id is a CourseLocator or a string like 'mit.eecs.6002x' or 'mit.eecs.6002x/branch/published'. Revision (optional) is a string like 'published'. It may be provided explicitly (explicit_branch) or embedded into course_id. @@ -270,10 +267,12 @@ class CourseLocator(Locator): if course_id: if isinstance(course_id, CourseLocator): course_id = course_id.course_id - assert course_id, "%s does not have a valid course_id" + if not course_id: + raise ValueError("%s does not have a valid course_id" % course_id) parse = parse_course_id(course_id) - assert parse, 'Could not parse "%s" as a course_id' % course_id + if not parse: + raise ValueError('Could not parse "%s" as a course_id' % course_id) self.set_course_id(parse['id']) rev = parse['branch'] if rev: @@ -348,7 +347,7 @@ class BlockUsageLocator(CourseLocator): url, course_id, branch, and usage_id must be strings or None """ - self.validate_args(url, version_guid, course_id, branch) + self._validate_args(url, version_guid, course_id) if url: self.init_block_ref_from_url(url) if course_id: @@ -398,7 +397,8 @@ class BlockUsageLocator(CourseLocator): self.set_usage_id(block_ref) else: parse = parse_block_ref(block_ref) - assert parse, 'Could not parse "%s" as a block_ref' % block_ref + if not parse: + raise ValueError('Could not parse "%s" as a block_ref' % block_ref) self.set_usage_id(parse['block']) def init_block_ref_from_url(self, url): @@ -461,10 +461,10 @@ class VersionTree(object): """ :param locator: must be version specific (Course has version_guid or definition had id) """ - assert isinstance(locator, Locator) and not inspect.isabstract(locator), \ - "locator must be a concrete subclass of Locator" - assert locator.version(), \ - "locator must be version specific (Course has version_guid or definition had id)" + if not isinstance(locator, Locator) and not inspect.isabstract(locator): + raise TypeError("locator {} must be a concrete subclass of Locator".format(locator)) + if not locator.version(): + raise ValueError("locator must be version specific (Course has version_guid or definition had id)") self.locator = locator if tree_dict is None: self.children = [] diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_locators.py b/common/lib/xmodule/xmodule/modulestore/tests/test_locators.py index 05f4d25283..330a2b7320 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_locators.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_locators.py @@ -91,8 +91,8 @@ class LocatorTest(TestCase): 'mit.eecs' + BRANCH_PREFIX + 'this ', 'mit.eecs' + BRANCH_PREFIX + 'th%is ', ): - self.assertRaises(AssertionError, CourseLocator, course_id=bad_id) - self.assertRaises(AssertionError, CourseLocator, url='edx://' + bad_id) + self.assertRaises(ValueError, CourseLocator, course_id=bad_id) + self.assertRaises(ValueError, CourseLocator, url='edx://' + bad_id) def test_course_constructor_bad_url(self): for bad_url in ('edx://', @@ -100,7 +100,7 @@ class LocatorTest(TestCase): 'http://mit.eecs', 'mit.eecs', 'edx//mit.eecs'): - self.assertRaises(AssertionError, CourseLocator, url=bad_url) + self.assertRaises(ValueError, CourseLocator, url=bad_url) def test_course_constructor_redundant_001(self): testurn = 'mit.eecs.6002x' From d79220122c2b5c743a49e21e60d76677f2b5e605 Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Thu, 26 Sep 2013 11:27:29 -0400 Subject: [PATCH 27/54] Change DescriptionLocator to DefinitionLocator --- common/lib/xmodule/xmodule/modulestore/locator.py | 2 +- .../modulestore/split_mongo/definition_lazy_loader.py | 4 ++-- .../lib/xmodule/xmodule/modulestore/split_mongo/split.py | 8 ++++---- .../xmodule/xmodule/modulestore/tests/test_locators.py | 6 +++--- .../xmodule/modulestore/tests/test_split_modulestore.py | 8 ++++---- 5 files changed, 14 insertions(+), 14 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/locator.py b/common/lib/xmodule/xmodule/modulestore/locator.py index 097f69c7c2..47753a4941 100644 --- a/common/lib/xmodule/xmodule/modulestore/locator.py +++ b/common/lib/xmodule/xmodule/modulestore/locator.py @@ -424,7 +424,7 @@ class BlockUsageLocator(CourseLocator): return rep + BLOCK_PREFIX + unicode(self.usage_id) -class DescriptionLocator(Locator): +class DefinitionLocator(Locator): """ Container for how to locate a description (the course-independent content). """ diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/definition_lazy_loader.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/definition_lazy_loader.py index 5ccaaa7ed3..9b2a652a95 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/definition_lazy_loader.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/definition_lazy_loader.py @@ -1,4 +1,4 @@ -from xmodule.modulestore.locator import DescriptionLocator +from xmodule.modulestore.locator import DefinitionLocator class DefinitionLazyLoader(object): @@ -15,7 +15,7 @@ class DefinitionLazyLoader(object): :param definition_locator: the id of the record in the above to fetch """ self.modulestore = modulestore - self.definition_locator = DescriptionLocator(definition_id) + self.definition_locator = DefinitionLocator(definition_id) def fetch(self): """ diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index 60bfa66b6a..cdcb55d899 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py @@ -11,7 +11,7 @@ from pytz import UTC from xmodule.errortracker import null_error_tracker from xmodule.x_module import XModuleDescriptor -from xmodule.modulestore.locator import BlockUsageLocator, DescriptionLocator, CourseLocator, VersionTree, LocalId +from xmodule.modulestore.locator import BlockUsageLocator, DefinitionLocator, CourseLocator, VersionTree, LocalId from xmodule.modulestore.exceptions import InsufficientSpecificationError, VersionConflictError, DuplicateItemError from xmodule.modulestore import inheritance, ModuleStoreBase, Location @@ -563,7 +563,7 @@ class SplitMongoModuleStore(ModuleStoreBase): } } new_id = self.definitions.insert(document) - definition_locator = DescriptionLocator(new_id) + definition_locator = DefinitionLocator(new_id) document['edit_info']['original_version'] = new_id self.definitions.update({'_id': new_id}, {'$set': {"edit_info.original_version": new_id}}) return definition_locator @@ -597,7 +597,7 @@ class SplitMongoModuleStore(ModuleStoreBase): old_definition['edit_info']['edited_on'] = datetime.datetime.now(UTC) old_definition['edit_info']['previous_version'] = definition_locator.definition_id new_id = self.definitions.insert(old_definition) - return DescriptionLocator(new_id), True + return DefinitionLocator(new_id), True else: return definition_locator, False @@ -1252,7 +1252,7 @@ class SplitMongoModuleStore(ModuleStoreBase): elif '_id' not in definition: return None else: - return DescriptionLocator(definition['_id']) + return DefinitionLocator(definition['_id']) def internal_clean_children(self, course_locator): """ diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_locators.py b/common/lib/xmodule/xmodule/modulestore/tests/test_locators.py index 330a2b7320..6ec9acfcd5 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_locators.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_locators.py @@ -4,7 +4,7 @@ Tests for xmodule.modulestore.locator. from unittest import TestCase from bson.objectid import ObjectId -from xmodule.modulestore.locator import Locator, CourseLocator, BlockUsageLocator, DescriptionLocator +from xmodule.modulestore.locator import Locator, CourseLocator, BlockUsageLocator, DefinitionLocator from xmodule.modulestore.parsers import BRANCH_PREFIX, BLOCK_PREFIX, VERSION_PREFIX, URL_VERSION_PREFIX from xmodule.modulestore.exceptions import InsufficientSpecificationError, OverSpecificationError @@ -254,11 +254,11 @@ class LocatorTest(TestCase): self.assertEqual('BlockUsageLocator("mit.eecs.6002x/branch/published/block/HW3")', repr(testobj)) def test_description_locator_url(self): - definition_locator = DescriptionLocator("chapter12345_2") + definition_locator = DefinitionLocator("chapter12345_2") self.assertEqual('edx://' + URL_VERSION_PREFIX + 'chapter12345_2', definition_locator.url()) def test_description_locator_version(self): - definition_locator = DescriptionLocator("chapter12345_2") + definition_locator = DefinitionLocator("chapter12345_2") self.assertEqual("chapter12345_2", definition_locator.version()) # ------------------------------------------------------------------ diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py index be5c61a143..2373c409de 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py @@ -13,7 +13,7 @@ from xblock.fields import Scope from xmodule.course_module import CourseDescriptor from xmodule.modulestore.exceptions import InsufficientSpecificationError, ItemNotFoundError, VersionConflictError, \ DuplicateItemError -from xmodule.modulestore.locator import CourseLocator, BlockUsageLocator, VersionTree, DescriptionLocator +from xmodule.modulestore.locator import CourseLocator, BlockUsageLocator, VersionTree, DefinitionLocator from xmodule.modulestore.inheritance import InheritanceMixin from xmodule.x_module import XModuleMixin from pytz import UTC @@ -562,7 +562,7 @@ class TestItemCrud(SplitModuleTest): new_module = modulestore().create_item( locator, category, 'user123', fields={'display_name': 'new chapter'}, - definition_locator=DescriptionLocator("chapter12345_2") + definition_locator=DefinitionLocator("chapter12345_2") ) # check that course version changed and course's previous is the other one self.assertNotEqual(new_module.location.version_guid, premod_course.location.version_guid) @@ -588,7 +588,7 @@ class TestItemCrud(SplitModuleTest): another_module = modulestore().create_item( locator, category, 'anotheruser', fields={'display_name': 'problem 2', 'data': another_payload}, - definition_locator=DescriptionLocator("problem12345_3_1"), + definition_locator=DefinitionLocator("problem12345_3_1"), ) # check that course version changed and course's previous is the other one parent = modulestore().get_item(locator) @@ -789,7 +789,7 @@ class TestItemCrud(SplitModuleTest): modulestore().create_item( locator, category, 'test_update_manifold', fields={'display_name': 'problem 2', 'data': another_payload}, - definition_locator=DescriptionLocator("problem12345_3_1"), + definition_locator=DefinitionLocator("problem12345_3_1"), ) # pylint: disable=W0212 modulestore()._clear_cache() From 19fb2017be6eddf0887b212e81b5459eee6b8a55 Mon Sep 17 00:00:00 2001 From: Vik Paruchuri Date: Fri, 27 Sep 2013 15:05:05 -0400 Subject: [PATCH 28/54] Streamline and test uploading ... remove requirement that uploads be images, can now upload any file. --- .../xmodule/combined_open_ended_module.py | 2 +- .../js/src/combinedopenended/display.coffee | 17 +- .../open_ended_image_submission.py | 270 ------------------ .../open_ended_module.py | 5 +- .../openendedchild.py | 228 +++++++++------ .../self_assessment_module.py | 5 +- .../xmodule/tests/test_combined_open_ended.py | 83 +++++- .../xmodule/tests/test_self_assessment.py | 2 +- .../xmodule/tests/test_util_open_ended.py | 58 +++- .../SampleQuestionImageUpload.xml | 24 ++ .../test/data/open_ended/course/2012_Fall.xml | 1 + 11 files changed, 315 insertions(+), 380 deletions(-) delete mode 100644 common/lib/xmodule/xmodule/open_ended_grading_classes/open_ended_image_submission.py create mode 100644 common/test/data/open_ended/combinedopenended/SampleQuestionImageUpload.xml diff --git a/common/lib/xmodule/xmodule/combined_open_ended_module.py b/common/lib/xmodule/xmodule/combined_open_ended_module.py index 5b1a57f20b..0bc79a4a1c 100644 --- a/common/lib/xmodule/xmodule/combined_open_ended_module.py +++ b/common/lib/xmodule/xmodule/combined_open_ended_module.py @@ -306,7 +306,7 @@ class CombinedOpenEndedFields(object): ) peer_grade_finished_submissions_when_none_pending = Boolean( display_name='Allow "overgrading" of peer submissions', - help=("Allow students to peer grade submissions that already have the requisite number of graders, " + help=("EXPERIMENTAL FEATURE. Allow students to peer grade submissions that already have the requisite number of graders, " "but ONLY WHEN all submissions they are eligible to grade already have enough graders. " "This is intended for use when settings for `Required Peer Grading` > `Peer Graders per Response`"), default=False, diff --git a/common/lib/xmodule/xmodule/js/src/combinedopenended/display.coffee b/common/lib/xmodule/xmodule/js/src/combinedopenended/display.coffee index 7103d2e841..030d93e9b5 100644 --- a/common/lib/xmodule/xmodule/js/src/combinedopenended/display.coffee +++ b/common/lib/xmodule/xmodule/js/src/combinedopenended/display.coffee @@ -372,7 +372,8 @@ class @CombinedOpenEnded answer_area_div = @$(@answer_area_div_sel) answer_area_div.html(response.student_response) else - @can_upload_files = pre_can_upload_files + @submit_button.show() + @submit_button.attr('disabled', false) @gentle_alert response.error confirm_save_answer: (event) => @@ -385,23 +386,27 @@ class @CombinedOpenEnded event.preventDefault() @answer_area.attr("disabled", true) max_filesize = 2*1000*1000 #2MB - pre_can_upload_files = @can_upload_files if @child_state == 'initial' files = "" + valid_files_attached = false if @can_upload_files == true files = @$(@file_upload_box_sel)[0].files[0] if files != undefined + valid_files_attached = true if files.size > max_filesize - @can_upload_files = false files = "" - else - @can_upload_files = false + # Don't submit the file in the case of it being too large, deal with the error locally. + @submit_button.show() + @submit_button.attr('disabled', false) + @gentle_alert "You are trying to upload a file that is too large for our system. Please choose a file under 2MB or paste a link to it into the answer box." + return fd = new FormData() fd.append('student_answer', @answer_area.val()) fd.append('student_file', files) - fd.append('can_upload_files', @can_upload_files) + fd.append('valid_files_attached', valid_files_attached) + that=this settings = type: "POST" data: fd diff --git a/common/lib/xmodule/xmodule/open_ended_grading_classes/open_ended_image_submission.py b/common/lib/xmodule/xmodule/open_ended_grading_classes/open_ended_image_submission.py deleted file mode 100644 index ea5c3b3527..0000000000 --- a/common/lib/xmodule/xmodule/open_ended_grading_classes/open_ended_image_submission.py +++ /dev/null @@ -1,270 +0,0 @@ -""" -This contains functions and classes used to evaluate if images are acceptable (do not show improper content, etc), and -to send them to S3. -""" - -try: - from PIL import Image - - ENABLE_PIL = True -except: - ENABLE_PIL = False - -from urlparse import urlparse -import requests -from boto.s3.connection import S3Connection -from boto.s3.key import Key -import logging - -log = logging.getLogger(__name__) - -#Domains where any image linked to can be trusted to have acceptable content. -TRUSTED_IMAGE_DOMAINS = [ - 'wikipedia', - 'edxuploads.s3.amazonaws.com', - 'wikimedia', -] - -#Suffixes that are allowed in image urls -ALLOWABLE_IMAGE_SUFFIXES = [ - 'jpg', - 'png', - 'gif', - 'jpeg' -] - -#Maximum allowed dimensions (x and y) for an uploaded image -MAX_ALLOWED_IMAGE_DIM = 2000 - -#Dimensions to which image is resized before it is evaluated for color count, etc -MAX_IMAGE_DIM = 150 - -#Maximum number of colors that should be counted in ImageProperties -MAX_COLORS_TO_COUNT = 16 - -#Maximum number of colors allowed in an uploaded image -MAX_COLORS = 400 - - -class ImageProperties(object): - """ - Class to check properties of an image and to validate if they are allowed. - """ - - def __init__(self, image_data): - """ - Initializes class variables - @param image: Image object (from PIL) - @return: None - """ - self.image = Image.open(image_data) - image_size = self.image.size - self.image_too_large = False - if image_size[0] > MAX_ALLOWED_IMAGE_DIM or image_size[1] > MAX_ALLOWED_IMAGE_DIM: - self.image_too_large = True - if image_size[0] > MAX_IMAGE_DIM or image_size[1] > MAX_IMAGE_DIM: - self.image = self.image.resize((MAX_IMAGE_DIM, MAX_IMAGE_DIM)) - self.image_size = self.image.size - - def count_colors(self): - """ - Counts the number of colors in an image, and matches them to the max allowed - @return: boolean true if color count is acceptable, false otherwise - """ - colors = self.image.getcolors(MAX_COLORS_TO_COUNT) - if colors is None: - color_count = MAX_COLORS_TO_COUNT - else: - color_count = len(colors) - - too_many_colors = (color_count <= MAX_COLORS) - return too_many_colors - - def check_if_rgb_is_skin(self, rgb): - """ - Checks if a given input rgb tuple/list is a skin tone - @param rgb: RGB tuple - @return: Boolean true false - """ - colors_okay = False - try: - r = rgb[0] - g = rgb[1] - b = rgb[2] - check_r = (r > 60) - check_g = (r * 0.4) < g < (r * 0.85) - check_b = (r * 0.2) < b < (r * 0.7) - colors_okay = check_r and check_b and check_g - except: - pass - - return colors_okay - - def get_skin_ratio(self): - """ - Gets the ratio of skin tone colors in an image - @return: True if the ratio is low enough to be acceptable, false otherwise - """ - colors = self.image.getcolors(MAX_COLORS_TO_COUNT) - is_okay = True - if colors is not None: - skin = sum([count for count, rgb in colors if self.check_if_rgb_is_skin(rgb)]) - total_colored_pixels = sum([count for count, rgb in colors]) - bad_color_val = float(skin) / total_colored_pixels - if bad_color_val > .4: - is_okay = False - - return is_okay - - def run_tests(self): - """ - Does all available checks on an image to ensure that it is okay (size, skin ratio, colors) - @return: Boolean indicating whether or not image passes all checks - """ - image_is_okay = False - try: - #image_is_okay = self.count_colors() and self.get_skin_ratio() and not self.image_too_large - image_is_okay = not self.image_too_large - except: - log.exception("Could not run image tests.") - - if not ENABLE_PIL: - image_is_okay = True - - #log.debug("Image OK: {0}".format(image_is_okay)) - - return image_is_okay - - -class URLProperties(object): - """ - Checks to see if a URL points to acceptable content. Added to check if students are submitting reasonable - links to the peer grading image functionality of the external grading service. - """ - - def __init__(self, url_string): - self.url_string = url_string - - def check_if_parses(self): - """ - Check to see if a URL parses properly - @return: success (True if parses, false if not) - """ - success = False - try: - self.parsed_url = urlparse(self.url_string) - success = True - except: - pass - - return success - - def check_suffix(self): - """ - Checks the suffix of a url to make sure that it is allowed - @return: True if suffix is okay, false if not - """ - good_suffix = False - for suffix in ALLOWABLE_IMAGE_SUFFIXES: - if self.url_string.endswith(suffix): - good_suffix = True - break - return good_suffix - - def run_tests(self): - """ - Runs all available url tests - @return: True if URL passes tests, false if not. - """ - url_is_okay = self.check_suffix() and self.check_if_parses() - return url_is_okay - - def check_domain(self): - """ - Checks to see if url is from a trusted domain - """ - success = False - for domain in TRUSTED_IMAGE_DOMAINS: - if domain in self.url_string: - success = True - return success - return success - - -def run_url_tests(url_string): - """ - Creates a URLProperties object and runs all tests - @param url_string: A URL in string format - @return: Boolean indicating whether or not URL has passed all tests - """ - url_properties = URLProperties(url_string) - return url_properties.run_tests() - - -def run_image_tests(image): - """ - Runs all available image tests - @param image: PIL Image object - @return: Boolean indicating whether or not all tests have been passed - """ - success = False - try: - image_properties = ImageProperties(image) - success = image_properties.run_tests() - except: - log.exception("Cannot run image tests in combined open ended xmodule. May be an issue with a particular image," - "or an issue with the deployment configuration of PIL/Pillow") - return success - - -def upload_to_s3(file_to_upload, keyname, s3_interface): - ''' - Upload file to S3 using provided keyname. - - Returns: - public_url: URL to access uploaded file - ''' - - #This commented out code is kept here in case we change the uploading method and require images to be - #converted before they are sent to S3. - #TODO: determine if commented code is needed and remove - #im = Image.open(file_to_upload) - #out_im = cStringIO.StringIO() - #im.save(out_im, 'PNG') - - try: - conn = S3Connection(s3_interface['access_key'], s3_interface['secret_access_key']) - bucketname = str(s3_interface['storage_bucket_name']) - bucket = conn.create_bucket(bucketname.lower()) - - k = Key(bucket) - k.key = keyname - k.set_metadata('filename', file_to_upload.name) - k.set_contents_from_file(file_to_upload) - - #This commented out code is kept here in case we change the uploading method and require images to be - #converted before they are sent to S3. - #k.set_contents_from_string(out_im.getvalue()) - #k.set_metadata("Content-Type", 'images/png') - - k.set_acl("public-read") - public_url = k.generate_url(60 * 60 * 24 * 365) # URL timeout in seconds. - - return True, public_url - except: - #This is a dev_facing_error - error_message = "Could not connect to S3 to upload peer grading image. Trying to utilize bucket: {0}".format( - bucketname.lower()) - log.error(error_message) - return False, error_message - - -def get_from_s3(s3_public_url): - """ - Gets an image from a given S3 url - @param s3_public_url: The URL where an image is located - @return: The image data - """ - r = requests.get(s3_public_url, timeout=2) - data = r.text - return data diff --git a/common/lib/xmodule/xmodule/open_ended_grading_classes/open_ended_module.py b/common/lib/xmodule/xmodule/open_ended_grading_classes/open_ended_module.py index d7fe8c0d26..2d973aa9ed 100644 --- a/common/lib/xmodule/xmodule/open_ended_grading_classes/open_ended_module.py +++ b/common/lib/xmodule/xmodule/open_ended_grading_classes/open_ended_module.py @@ -651,15 +651,12 @@ class OpenEndedModule(openendedchild.OpenEndedChild): return self.out_of_sync_error(data) # add new history element with answer and empty score and hint. - success, data = self.append_image_to_student_answer(data) + success, error_message, data = self.append_file_link_to_student_answer(data) if success: data['student_answer'] = OpenEndedModule.sanitize_html(data['student_answer']) self.new_history_entry(data['student_answer']) self.send_to_grader(data['student_answer'], system) self.change_state(self.ASSESSING) - else: - # This is a student_facing_error - error_message = "There was a problem saving the image in your submission. Please try a different image, or try pasting a link to an image into the answer box." return { 'success': success, diff --git a/common/lib/xmodule/xmodule/open_ended_grading_classes/openendedchild.py b/common/lib/xmodule/xmodule/open_ended_grading_classes/openendedchild.py index d7555ce77e..67a058f478 100644 --- a/common/lib/xmodule/xmodule/open_ended_grading_classes/openendedchild.py +++ b/common/lib/xmodule/xmodule/open_ended_grading_classes/openendedchild.py @@ -2,8 +2,6 @@ import json import logging from lxml.html.clean import Cleaner, autolink_html import re - -import open_ended_image_submission from xmodule.progress import Progress import capa.xqueue_interface as xqueue_interface from capa.util import * @@ -12,6 +10,9 @@ import controller_query_service from datetime import datetime from pytz import UTC +import requests +from boto.s3.connection import S3Connection +from boto.s3.key import Key log = logging.getLogger("mitx.courseware") @@ -24,6 +25,50 @@ MAX_ATTEMPTS = 1 # Overriden by max_score specified in xml. MAX_SCORE = 1 +FILE_NOT_FOUND_IN_RESPONSE_MESSAGE = "We could not find a file in your submission. Please try choosing a file or pasting a link to your file into the answer box." +ERROR_SAVING_FILE_MESSAGE = "We are having trouble saving your file. Please try another file or paste a link to your file into the answer box." + +def upload_to_s3(file_to_upload, keyname, s3_interface): + ''' + Upload file to S3 using provided keyname. + + Returns: + public_url: URL to access uploaded file + ''' + + conn = S3Connection(s3_interface['access_key'], s3_interface['secret_access_key']) + bucketname = str(s3_interface['storage_bucket_name']) + bucket = conn.create_bucket(bucketname.lower()) + + k = Key(bucket) + k.key = keyname + k.set_metadata('filename', file_to_upload.name) + k.set_contents_from_file(file_to_upload) + + k.set_acl("public-read") + public_url = k.generate_url(60 * 60 * 24 * 365) # URL timeout in seconds. + + return public_url + +class WhiteListCleaner(Cleaner): + """ + By default, lxml cleaner strips out all links that are not in a defined whitelist. + We want to allow all links, and rely on the peer grading flagging mechanic to catch + the "bad" ones. So, don't define a whitelist at all. + """ + def allow_embedded_url(self, el, url): + """ + Override the Cleaner allow_embedded_url method to remove the whitelist url requirement. + Ensure that any tags not in the whitelist are stripped beforehand. + """ + + # Tell cleaner to strip any element with a tag that isn't whitelisted. + if self.whitelist_tags is not None and el.tag not in self.whitelist_tags: + return False + + # Tell cleaner to allow all urls. + return True + class OpenEndedChild(object): """ @@ -70,6 +115,7 @@ class OpenEndedChild(object): except: log.error( "Could not load instance state for open ended. Setting it to nothing.: {0}".format(instance_state)) + instance_state = {} else: instance_state = {} @@ -176,11 +222,22 @@ class OpenEndedChild(object): @staticmethod def sanitize_html(answer): + """ + Take a student response and sanitize the HTML to prevent malicious script injection + or other unwanted content. + answer - any string + return - a cleaned version of the string + """ try: answer = autolink_html(answer) - cleaner = Cleaner(style=True, links=True, add_nofollow=False, page_structure=True, safe_attrs_only=True, - host_whitelist=open_ended_image_submission.TRUSTED_IMAGE_DOMAINS, - whitelist_tags=set(['embed', 'iframe', 'a', 'img', 'br'])) + cleaner = WhiteListCleaner( + style=True, + links=True, + add_nofollow=False, + page_structure=True, + safe_attrs_only=True, + whitelist_tags=('embed', 'iframe', 'a', 'img', 'br',) + ) clean_html = cleaner.clean_html(answer) clean_html = re.sub(r'

      $', '', re.sub(r'^

      ', '', clean_html)) clean_html = re.sub("\n","
      ", clean_html) @@ -351,119 +408,116 @@ class OpenEndedChild(object): correctness = 'correct' if self.is_submission_correct(score) else 'incorrect' return correctness - def upload_image_to_s3(self, image_data): + def upload_file_to_s3(self, file_data): """ - Uploads an image to S3 - Image_data: InMemoryUploadedFileObject that responds to read() and seek() - @return:Success and a URL corresponding to the uploaded object + Uploads a file to S3. + file_data: InMemoryUploadedFileObject that responds to read() and seek(). + @return: A URL corresponding to the uploaded object. """ - success = False - s3_public_url = "" - image_ok = False - try: - image_data.seek(0) - image_ok = open_ended_image_submission.run_image_tests(image_data) - except Exception: - log.exception("Could not create image and check it.") - if image_ok: - image_key = image_data.name + datetime.now(UTC).strftime( - xqueue_interface.dateformat - ) + file_key = file_data.name + datetime.now(UTC).strftime( + xqueue_interface.dateformat + ) - try: - image_data.seek(0) - success, s3_public_url = open_ended_image_submission.upload_to_s3( - image_data, image_key, self.s3_interface - ) - except Exception: - log.exception("Could not upload image to S3.") + file_data.seek(0) + s3_public_url = upload_to_s3( + file_data, file_key, self.s3_interface + ) - return success, image_ok, s3_public_url + return s3_public_url - def check_for_image_and_upload(self, data): + def check_for_file_and_upload(self, data): """ - Checks to see if an image was passed back in the AJAX query. If so, it will upload it to S3 - @param data: AJAX data - @return: Success, whether or not a file was in the data dictionary, - and the html corresponding to the uploaded image + Checks to see if a file was passed back by the student. If so, it will be uploaded to S3. + @param data: AJAX post dictionary containing keys student_file and valid_files_attached. + @return: has_file_to_upload, whether or not a file was in the data dictionary, + and image_tag, the html needed to create a link to the uploaded file. """ has_file_to_upload = False - uploaded_to_s3 = False image_tag = "" - image_ok = False - if 'can_upload_files' in data: - if data['can_upload_files'] in ['true', '1']: + + # Ensure that a valid file was uploaded. + if ('valid_files_attached' in data + and data['valid_files_attached'] in ['true', '1', True] + and data['student_file'] is not None + and len(data['student_file']) > 0): has_file_to_upload = True student_file = data['student_file'][0] - uploaded_to_s3, image_ok, s3_public_url = self.upload_image_to_s3(student_file) - if uploaded_to_s3: - image_tag = self.generate_image_tag_from_url(s3_public_url, student_file.name) - return has_file_to_upload, uploaded_to_s3, image_ok, image_tag + # Upload the file to S3 and generate html to embed a link. + s3_public_url = self.upload_file_to_s3(student_file) + image_tag = self.generate_file_link_html_from_url(s3_public_url, student_file.name) - def generate_image_tag_from_url(self, s3_public_url, image_name): + return has_file_to_upload, image_tag + + def generate_file_link_html_from_url(self, s3_public_url, file_name): """ - Makes an image tag from a given URL - @param s3_public_url: URL of the image - @param image_name: Name of the image - @return: Boolean success, updated AJAX data + Create an html link to a given URL. + @param s3_public_url: URL of the file. + @param file_name: Name of the file. + @return: Boolean success, updated AJAX data. """ - image_template = """ + image_link = """ {1} - """.format(s3_public_url, image_name) - return image_template + """.format(s3_public_url, file_name) + return image_link - def append_image_to_student_answer(self, data): + def append_file_link_to_student_answer(self, data): """ - Adds an image to a student answer after uploading it to S3 - @param data: AJAx data - @return: Boolean success, updated AJAX data + Adds a file to a student answer after uploading it to S3. + @param data: AJAX data containing keys student_answer, valid_files_attached, and student_file. + @return: Boolean success, and updated AJAX data dictionary. """ - overall_success = False + + error_message = "" + if not self.accept_file_upload: # If the question does not accept file uploads, do not do anything - return True, data + return True, error_message, data - has_file_to_upload, uploaded_to_s3, image_ok, image_tag = self.check_for_image_and_upload(data) - if uploaded_to_s3 and has_file_to_upload and image_ok: + try: + # Try to upload the file to S3. + has_file_to_upload, image_tag = self.check_for_file_and_upload(data) data['student_answer'] += image_tag - overall_success = True - elif has_file_to_upload and not uploaded_to_s3 and image_ok: + success = True + if not has_file_to_upload: + # If there is no file to upload, probably the student has embedded the link in the answer text + success, data['student_answer'] = self.check_for_url_in_text(data['student_answer']) + + # If success is False, we have not found a link, and no file was attached. + # Show error to student. + if success is False: + error_message = FILE_NOT_FOUND_IN_RESPONSE_MESSAGE + + except Exception: # In this case, an image was submitted by the student, but the image could not be uploaded to S3. Likely - # a config issue (development vs deployment). For now, just treat this as a "success" - log.exception("Student AJAX post to combined open ended xmodule indicated that it contained an image, " - "but the image was not able to be uploaded to S3. This could indicate a config" - "issue with this deployment, but it could also indicate a problem with S3 or with the" - "student image itself.") - overall_success = True - elif not has_file_to_upload: - # If there is no file to upload, probably the student has embedded the link in the answer text - success, data['student_answer'] = self.check_for_url_in_text(data['student_answer']) - overall_success = success + # a config issue (development vs deployment). + log.exception("Student AJAX post to combined open ended xmodule indicated that it contained a file, " + "but the image was not able to be uploaded to S3. This could indicate a configuration " + "issue with this deployment and the S3_INTERFACE setting.") + success = False + error_message = ERROR_SAVING_FILE_MESSAGE - # log.debug("Has file: {0} Uploaded: {1} Image Ok: {2}".format(has_file_to_upload, uploaded_to_s3, image_ok)) - - return overall_success, data + return success, error_message, data def check_for_url_in_text(self, string): """ - Checks for urls in a string - @param string: Arbitrary string - @return: Boolean success, the edited string + Checks for urls in a string. + @param string: Arbitrary string. + @return: Boolean success, and the edited string. """ - success = False - links = re.findall(r'(https?://\S+)', string) - if len(links) > 0: - for link in links: - success = open_ended_image_submission.run_url_tests(link) - if not success: - string = re.sub(link, '', string) - else: - string = re.sub(link, self.generate_image_tag_from_url(link, link), string) - success = True + has_link = False - return success, string + # Find all links in the string. + links = re.findall(r'(https?://\S+)', string) + if len(links)>0: + has_link = True + + # Autolink by wrapping links in anchor tags. + for link in links: + string = re.sub(link, self.generate_file_link_html_from_url(link, link), string) + + return has_link, string def get_eta(self): if self.controller_qs: diff --git a/common/lib/xmodule/xmodule/open_ended_grading_classes/self_assessment_module.py b/common/lib/xmodule/xmodule/open_ended_grading_classes/self_assessment_module.py index cc830f88c8..6c0d1bbf08 100644 --- a/common/lib/xmodule/xmodule/open_ended_grading_classes/self_assessment_module.py +++ b/common/lib/xmodule/xmodule/open_ended_grading_classes/self_assessment_module.py @@ -179,14 +179,11 @@ class SelfAssessmentModule(openendedchild.OpenEndedChild): error_message = "" # add new history element with answer and empty score and hint. - success, data = self.append_image_to_student_answer(data) + success, error_message, data = self.append_file_link_to_student_answer(data) if success: data['student_answer'] = SelfAssessmentModule.sanitize_html(data['student_answer']) self.new_history_entry(data['student_answer']) self.change_state(self.ASSESSING) - else: - # This is a student_facing_error - error_message = "There was a problem saving the image in your submission. Please try a different image, or try pasting a link to an image into the answer box." return { 'success': success, diff --git a/common/lib/xmodule/xmodule/tests/test_combined_open_ended.py b/common/lib/xmodule/xmodule/tests/test_combined_open_ended.py index 5d11a4924f..65fc2bb608 100644 --- a/common/lib/xmodule/xmodule/tests/test_combined_open_ended.py +++ b/common/lib/xmodule/xmodule/tests/test_combined_open_ended.py @@ -12,7 +12,7 @@ import logging import unittest from lxml import etree -from mock import Mock, MagicMock, ANY +from mock import Mock, MagicMock, ANY, patch from pytz import UTC from xmodule.open_ended_grading_classes.openendedchild import OpenEndedChild @@ -26,7 +26,7 @@ from xmodule.progress import Progress from xmodule.tests.test_util_open_ended import ( MockQueryDict, DummyModulestore, TEST_STATE_SA_IN, MOCK_INSTANCE_STATE, TEST_STATE_SA, TEST_STATE_AI, TEST_STATE_AI2, TEST_STATE_AI2_INVALID, - TEST_STATE_SINGLE, TEST_STATE_PE_SINGLE + TEST_STATE_SINGLE, TEST_STATE_PE_SINGLE, MockUploadedFile ) from xblock.field_data import DictFieldData @@ -374,7 +374,7 @@ class OpenEndedModuleTest(unittest.TestCase): # Submit a student response to the question. test_module.handle_ajax( "save_answer", - {"student_answer": submitted_response, "can_upload_files": False, "student_file": None}, + {"student_answer": submitted_response}, get_test_system() ) # Submitting an answer should clear the stored answer. @@ -753,7 +753,7 @@ class OpenEndedModuleXmlTest(unittest.TestCase, DummyModulestore): #Simulate a student saving an answer html = module.handle_ajax("get_html", {}) - module.handle_ajax("save_answer", {"student_answer": self.answer, "can_upload_files": False, "student_file": None}) + module.handle_ajax("save_answer", {"student_answer": self.answer}) html = module.handle_ajax("get_html", {}) #Mock a student submitting an assessment @@ -902,3 +902,78 @@ class OpenEndedModuleXmlAttemptTest(unittest.TestCase, DummyModulestore): #Try to reset, should fail because only 1 attempt is allowed reset_data = json.loads(module.handle_ajax("reset", {})) self.assertEqual(reset_data['success'], False) + +class OpenEndedModuleXmlImageUploadTest(unittest.TestCase, DummyModulestore): + """ + Test if student is able to upload images properly. + """ + problem_location = Location(["i4x", "edX", "open_ended", "combinedopenended", "SampleQuestionImageUpload"]) + answer_text = "Hello, this is my amazing answer." + file_text = "Hello, this is my amazing file." + file_name = "Student file 1" + answer_link = "http://www.edx.org" + autolink_tag = " + + + + Writing Applications + + + + + Language Conventions + + + + + + +

      Censorship in the Libraries

      +

      "All of us can think of a book that we hope none of our children or any other children have taken off the shelf. But if I have the right to remove that book from the shelf -- that work I abhor -- then you also have exactly the same right and so does everyone else. And then we have no books left on the shelf for any of us." --Katherine Paterson, Author

      +

      Write a persuasive essay to a newspaper reflecting your views on censorship in libraries. Do you believe that certain materials, such as books, music, movies, magazines, etc., should be removed from the shelves if they are found offensive? Support your position with convincing arguments from your own experience, observations, and/or reading.

      + + + + + \ No newline at end of file diff --git a/common/test/data/open_ended/course/2012_Fall.xml b/common/test/data/open_ended/course/2012_Fall.xml index 232de855cc..57bcc6ddb6 100644 --- a/common/test/data/open_ended/course/2012_Fall.xml +++ b/common/test/data/open_ended/course/2012_Fall.xml @@ -2,6 +2,7 @@ + From 486d32caae16ce0b077e1328aac3434f682d29be Mon Sep 17 00:00:00 2001 From: Ned Batchelder Date: Mon, 30 Sep 2013 11:09:34 -0400 Subject: [PATCH 29/54] Rearrange and comment .gitignore so I can find things. --- .gitignore | 87 ++++++++++++++++++++++++++++++++++-------------------- 1 file changed, 55 insertions(+), 32 deletions(-) diff --git a/.gitignore b/.gitignore index 72de96e0c4..939488bffe 100644 --- a/.gitignore +++ b/.gitignore @@ -1,49 +1,72 @@ -*.pyc -*~ -*.scssc -*.swp -*.orig -*.DS_Store -*.mo -:2e_* -:2e# -.AppleDouble -database.sqlite +# .gitignore for edx-platform. +# There's a lot here, please try to keep it organized. + +### Files private to developers + requirements/private.txt lms/envs/private.py cms/envs/private.py -courseware/static/js/mathjax/* -flushdb.sh -build + +### Python artifacts +*.pyc + +### Editor and IDE artifacts +*~ +*.swp +*.orig +/nbproject +.idea/ +.redcar/ + +### OS X artifacts +*.DS_Store +.AppleDouble +:2e_* +:2e# + +### Internationalization artifacts +*.mo +conf/locale/en/LC_MESSAGES/*.po +!messages.po + +### Testing artifacts +.testids/ +nosetests.xml .coverage coverage.xml cover/ -log/ +cover_html/ reports/ -/src/ -\#*\# + +### Installation artifacts *.egg-info Gemfile.lock -.env/ -conf/locale/en/LC_MESSAGES/*.po -!messages.po +.pip_download_cache/ +.prereqs_cache +.vagrant/ +node_modules + +### Static assets pipeline artifacts +*.scssc lms/static/sass/*.css lms/static/sass/application.scss lms/static/sass/course.scss cms/static/sass/*.css -lms/lib/comment_client/python -nosetests.xml -cover_html/ -.idea/ -.redcar/ + +### Logging artifacts +log/ +logs chromedriver.log -/nbproject ghostdriver.log -node_modules -.pip_download_cache/ -.prereqs_cache + +### Unknown artifacts +database.sqlite +courseware/static/js/mathjax/* +flushdb.sh +build +/src/ +\#*\# +.env/ +lms/lib/comment_client/python autodeploy.properties .ws_migrations_complete -.vagrant/ -logs -.testids/ From 0f3bb1e7cd29d63b5735f5c452b37789c9b2e641 Mon Sep 17 00:00:00 2001 From: Ned Batchelder Date: Mon, 30 Sep 2013 11:10:44 -0400 Subject: [PATCH 30/54] Ignore .noseids files wherever they appear. --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index 939488bffe..e92d49a0f2 100644 --- a/.gitignore +++ b/.gitignore @@ -31,6 +31,7 @@ conf/locale/en/LC_MESSAGES/*.po ### Testing artifacts .testids/ +.noseids nosetests.xml .coverage coverage.xml From 2e7e35407691d7e68606b3a29b4290abf45709c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Andr=C3=A9s=20Rocha?= Date: Mon, 30 Sep 2013 11:41:49 -0400 Subject: [PATCH 31/54] Fix course enrollment count in instructor dashboard Fix AN-115 --- lms/djangoapps/instructor/views/instructor_dashboard.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lms/djangoapps/instructor/views/instructor_dashboard.py b/lms/djangoapps/instructor/views/instructor_dashboard.py index 031eac266b..9634005b87 100644 --- a/lms/djangoapps/instructor/views/instructor_dashboard.py +++ b/lms/djangoapps/instructor/views/instructor_dashboard.py @@ -76,7 +76,7 @@ def _section_course_info(course_id): section_data['section_display_name'] = _('Course Info') section_data['course_id'] = course_id section_data['course_display_name'] = course.display_name - section_data['enrollment_count'] = CourseEnrollment.objects.filter(course_id=course_id).count() + section_data['enrollment_count'] = CourseEnrollment.objects.filter(course_id=course_id, is_active=1).count() section_data['has_started'] = course.has_started() section_data['has_ended'] = course.has_ended() From 06cac195f012b089d01813e6779aa5c1d0320d49 Mon Sep 17 00:00:00 2001 From: Jason Bau Date: Sat, 28 Sep 2013 00:30:47 -0700 Subject: [PATCH 32/54] make ENABLE_SHOPPING_CART truly optional course_about was throwing 500s if ENABLE_SHOPPING_CART=False --- lms/djangoapps/courseware/views.py | 3 +- lms/templates/courseware/course_about.html | 52 ++++++++++++---------- lms/templates/navigation.html | 2 +- 3 files changed, 32 insertions(+), 25 deletions(-) diff --git a/lms/djangoapps/courseware/views.py b/lms/djangoapps/courseware/views.py index 4488b510d0..c5489d3b30 100644 --- a/lms/djangoapps/courseware/views.py +++ b/lms/djangoapps/courseware/views.py @@ -609,7 +609,8 @@ def course_about(request, course_id): registration_price = 0 in_cart = False reg_then_add_to_cart_link = "" - if settings.MITX_FEATURES.get('ENABLE_PAID_COURSE_REGISTRATION'): + if (settings.MITX_FEATURES.get('ENABLE_SHOPPING_CART') and + settings.MITX_FEATURES.get('ENABLE_PAID_COURSE_REGISTRATION')): registration_price = CourseMode.min_course_price_for_currency(course_id, settings.PAID_COURSE_REGISTRATION_CURRENCY[0]) if request.user.is_authenticated(): diff --git a/lms/templates/courseware/course_about.html b/lms/templates/courseware/course_about.html index e63003b3ee..382dbf4660 100644 --- a/lms/templates/courseware/course_about.html +++ b/lms/templates/courseware/course_about.html @@ -3,8 +3,12 @@ from django.core.urlresolvers import reverse from courseware.courses import course_image_url, get_course_about_section from courseware.access import has_access + from django.conf import settings - cart_link = reverse('shoppingcart.views.show_cart') + if settings.MITX_FEATURES.get('ENABLE_SHOPPING_CART'): + cart_link = reverse('shoppingcart.views.show_cart') + else: + cart_link = "" %> <%namespace name='static' file='../static_content.html'/> @@ -26,29 +30,31 @@ $("#class_enroll_form").submit(); event.preventDefault(); }); - add_course_complete_handler = function(jqXHR, textStatus) { - if (jqXHR.status == 200) { - location.href = "${cart_link}"; - } - if (jqXHR.status == 400) { - $("#register_error") - .html(jqXHR.responseText ? jqXHR.responseText : "${_('An error occurred. Please try again later.')}") - .css("display", "block"); - } - else if (jqXHR.status == 403) { - location.href = "${reg_then_add_to_cart_link}"; - } - }; - $("#add_to_cart_post").click(function(event){ - $.ajax({ - url: "${reverse('add_course_to_cart', args=[course.id])}", - type: "POST", - /* Rant: HAD TO USE COMPLETE B/C PROMISE.DONE FOR SOME REASON DOES NOT WORK ON THIS PAGE. */ - complete: add_course_complete_handler - }) - event.preventDefault(); - }); + % if settings.MITX_FEATURES.get('ENABLE_SHOPPING_CART') and settings.MITX_FEATURES.get('ENABLE_PAID_COURSE_REGISTRATION'): + add_course_complete_handler = function(jqXHR, textStatus) { + if (jqXHR.status == 200) { + location.href = "${cart_link}"; + } + if (jqXHR.status == 400) { + $("#register_error") + .html(jqXHR.responseText ? jqXHR.responseText : "${_('An error occurred. Please try again later.')}") + .css("display", "block"); + } + else if (jqXHR.status == 403) { + location.href = "${reg_then_add_to_cart_link}"; + } + }; + $("#add_to_cart_post").click(function(event){ + $.ajax({ + url: "${reverse('add_course_to_cart', args=[course.id])}", + type: "POST", + /* Rant: HAD TO USE COMPLETE B/C PROMISE.DONE FOR SOME REASON DOES NOT WORK ON THIS PAGE. */ + complete: add_course_complete_handler + }) + event.preventDefault(); + }); + % endif ## making the conditional around this entire JS block for sanity %if settings.MITX_FEATURES.get('RESTRICT_ENROLL_BY_REG_METHOD') and course.enrollment_domain: diff --git a/lms/templates/navigation.html b/lms/templates/navigation.html index 4182696eb1..e2e1d9d664 100644 --- a/lms/templates/navigation.html +++ b/lms/templates/navigation.html @@ -84,7 +84,7 @@ site_status_msg = get_site_status_msg(course_id)
    % if settings.MITX_FEATURES.get('ENABLE_PAID_COURSE_REGISTRATION') and \ - settings.MITX_FEATURES['ENABLE_SHOPPING_CART'] and \ + settings.MITX_FEATURES.get('ENABLE_SHOPPING_CART') and \ shoppingcart.models.Order.user_cart_has_items(user):
    1. From 7d578e4f1f8e68c689e763240e5d0f277071e915 Mon Sep 17 00:00:00 2001 From: Jason Bau Date: Mon, 30 Sep 2013 11:23:40 -0700 Subject: [PATCH 33/54] Update CHANGELOG.rst --- CHANGELOG.rst | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 443e787482..83323e0de3 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. +LMS: Add PaidCourseRegistration mode, where payment is required before course registration. + LMS: Add split testing functionality for internal use. Studio and LMS: add ability to lock assets (cannot be viewed unless registered for class). From aeac8dddbb388da2cd72f026b430637d1c0a567f Mon Sep 17 00:00:00 2001 From: jmclaus Date: Thu, 26 Sep 2013 14:48:59 +0200 Subject: [PATCH 34/54] All video controls have a full outline when receiving focus --- .../xmodule/xmodule/css/video/display.scss | 72 +++++++++++++------ 1 file changed, 50 insertions(+), 22 deletions(-) diff --git a/common/lib/xmodule/xmodule/css/video/display.scss b/common/lib/xmodule/xmodule/css/video/display.scss index 1a1ca14fbd..c087d18098 100644 --- a/common/lib/xmodule/xmodule/css/video/display.scss +++ b/common/lib/xmodule/xmodule/css/video/display.scss @@ -136,7 +136,7 @@ div.video { &:focus, &:hover { background-color: lighten($pink, 10%); - outline: none; + outline: 0; } } } @@ -162,9 +162,16 @@ div.video { text-indent: -9999px; width: 14px; background: url('../images/vcr.png') 15px 15px no-repeat; - outline: 0; &:focus { + position: relative; + z-index: 10000; + outline: #fff dotted thin; + outline-offset: -2px; + background: #333; + } + + &:hover { outline: 0; } @@ -176,7 +183,7 @@ div.video { &.play { background-position: 17px -114px; - &:hover, &:focus { + &:hover { background-color: #444; } } @@ -184,7 +191,7 @@ div.video { &.pause { background-position: 16px -50px; - &:hover, &:focus { + &:hover { background-color: #444; } } @@ -203,6 +210,19 @@ div.video { div.secondary-controls { float: right; + div.speeds>a, div.volume>a, a.add-fullscreen, a.quality_control, + a.hide-subtitles { + // overflow is used to bypass Firefox CSS :focus outline bug + // http://johndoesdesign.com/blog/2012/css/firefox-and-its-css-focus-outline-bug/ + &:focus { + position: relative; + z-index: 10000; + outline: #fff dotted thin; + outline-offset: -2px; + overflow: auto; + } + } + div.speeds { float: left; position: relative; @@ -250,10 +270,15 @@ div.video { } } - outline: 0; - - &:focus { + &:hover { outline: 0; + opacity: 1.0; + background-color: #444; + } + + &:active { + opacity: 1.0; + background-color: #444; } h3 { @@ -280,11 +305,6 @@ div.video { line-height: 46px; color: #fff; } - - &:hover, &:active, &:focus { - opacity: 1.0; - background-color: #444; - } } // fix for now @@ -320,6 +340,7 @@ div.video { &:hover { background-color: #666; color: #aaa; + outline-offset: -4px; } } @@ -371,9 +392,12 @@ div.video { @include transition(none); -webkit-font-smoothing: antialiased; width: 30px; - - &:hover, &:active, &:focus { + + &:hover, &:active { background-color: #444; + color: #fff; + text-decoration: none; + outline: 0; } } @@ -433,14 +457,16 @@ div.video { text-indent: -9999px; @include transition(none); width: 30px; - - &:hover, &:active, &:focus { + + &:hover, &:active { background-color: #444; color: #fff; text-decoration: none; + outline: 0; } } + a.quality_control { background: url(../images/hd.png) center no-repeat; border-right: 1px solid #000; @@ -455,16 +481,18 @@ div.video { @include transition(none); width: 30px; - &:hover, &:focus { + &:hover { background-color: #444; color: #fff; text-decoration: none; + outline: 0; } &.active { background-color: #F44; color: #0ff; text-decoration: none; + outline: 0; } } @@ -483,10 +511,11 @@ div.video { -webkit-font-smoothing: antialiased; width: 30px; - &:hover, &:focus { + &:hover { background-color: #444; color: #fff; text-decoration: none; + outline: 0; } &.off { @@ -530,8 +559,7 @@ div.video { margin-bottom: 8px; padding: 0; line-height: lh(); - outline-width: 0px; - outline-style: none; + outline: 0; &.current { color: #333; @@ -539,8 +567,8 @@ div.video { } &.focused { - outline-width: 1px; - outline-style: dotted; + outline: #000 dotted thin; + outline-offset: -1px; } &:hover { From d7e441088b628a24cb40437832a2497f1b0d4c69 Mon Sep 17 00:00:00 2001 From: Ned Batchelder Date: Tue, 17 Sep 2013 15:10:56 -0400 Subject: [PATCH 35/54] Adjust parameters to construct_xblock_from_class to match XBlock repo. --- common/lib/xmodule/xmodule/error_module.py | 4 ++-- common/lib/xmodule/xmodule/modulestore/mongo/base.py | 7 +++---- .../modulestore/split_mongo/caching_descriptor_system.py | 2 +- common/lib/xmodule/xmodule/modulestore/xml.py | 2 +- common/lib/xmodule/xmodule/tests/test_editing_module.py | 2 +- common/lib/xmodule/xmodule/tests/test_video.py | 2 +- common/lib/xmodule/xmodule/tests/test_xblock_wrappers.py | 4 ++-- common/lib/xmodule/xmodule/tests/test_xml_module.py | 2 +- common/lib/xmodule/xmodule/video_module.py | 5 ++--- common/lib/xmodule/xmodule/x_module.py | 2 +- common/lib/xmodule/xmodule/xml_module.py | 5 ++--- 11 files changed, 17 insertions(+), 20 deletions(-) diff --git a/common/lib/xmodule/xmodule/error_module.py b/common/lib/xmodule/xmodule/error_module.py index 5c13b90ccf..7fc3747f44 100644 --- a/common/lib/xmodule/xmodule/error_module.py +++ b/common/lib/xmodule/xmodule/error_module.py @@ -105,10 +105,10 @@ class ErrorDescriptor(ErrorFields, XModuleDescriptor): }) return system.construct_xblock_from_class( cls, - field_data, # The error module doesn't use scoped data, and thus doesn't need # real scope keys - ScopeIds('error', None, location, location) + ScopeIds('error', None, location, location), + field_data, ) def get_context(self): diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/base.py b/common/lib/xmodule/xmodule/modulestore/mongo/base.py index 44865ab58e..2843287055 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/base.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/base.py @@ -193,7 +193,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem): field_data = DbModel(kvs) scope_ids = ScopeIds(None, category, location, location) - module = self.construct_xblock_from_class(class_, field_data, scope_ids) + module = self.construct_xblock_from_class(class_, scope_ids, field_data) if self.cached_metadata is not None: # parent container pointers don't differentiate between draft and non-draft # so when we do the lookup, we should do so with a non-draft location @@ -621,12 +621,11 @@ class MongoModuleStore(ModuleStoreBase): dbmodel = self._create_new_field_data(location.category, location, definition_data, metadata) xmodule = system.construct_xblock_from_class( xblock_class, - dbmodel, - # We're loading a descriptor, so student_id is meaningless # We also don't have separate notions of definition and usage ids yet, # so we use the location for both. - ScopeIds(None, location.category, location, location) + ScopeIds(None, location.category, location, location), + dbmodel, ) # decache any pending field settings from init xmodule.save() diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py index ba81a5231d..a0549074c3 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py @@ -111,8 +111,8 @@ class CachingDescriptorSystem(MakoDescriptorSystem): try: module = self.construct_xblock_from_class( class_, + ScopeIds(None, json_data.get('category'), definition_id, block_locator), field_data, - ScopeIds(None, json_data.get('category'), definition_id, block_locator) ) except Exception: log.warning("Failed to load descriptor", exc_info=True) diff --git a/common/lib/xmodule/xmodule/modulestore/xml.py b/common/lib/xmodule/xmodule/modulestore/xml.py index 93ab0497f9..49ad6f4e95 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml.py +++ b/common/lib/xmodule/xmodule/modulestore/xml.py @@ -477,11 +477,11 @@ class XMLModuleStore(ModuleStoreBase): loc = Location('i4x', course_descriptor.location.org, course_descriptor.location.course, category, slug) module = system.construct_xblock_from_class( HtmlDescriptor, - DictFieldData({'data': html, 'location': loc, 'category': category}), # We're loading a descriptor, so student_id is meaningless # We also don't have separate notions of definition and usage ids yet, # so we use the location for both ScopeIds(None, category, loc, loc), + DictFieldData({'data': html, 'location': loc, 'category': category}), ) # VS[compat]: # Hack because we need to pull in the 'display_name' for static tabs (because we need to edit them) diff --git a/common/lib/xmodule/xmodule/tests/test_editing_module.py b/common/lib/xmodule/xmodule/tests/test_editing_module.py index 2e59c545a4..36f028e8a4 100644 --- a/common/lib/xmodule/xmodule/tests/test_editing_module.py +++ b/common/lib/xmodule/xmodule/tests/test_editing_module.py @@ -46,8 +46,8 @@ class TabsEditingDescriptorTestCase(unittest.TestCase): TabsEditingDescriptor.tabs = self.tabs self.descriptor = system.construct_xblock_from_class( TabsEditingDescriptor, - field_data=DictFieldData({}), scope_ids=ScopeIds(None, None, None, None), + field_data=DictFieldData({}), ) def test_get_css(self): diff --git a/common/lib/xmodule/xmodule/tests/test_video.py b/common/lib/xmodule/xmodule/tests/test_video.py index b935acfb57..4c9972ea3a 100644 --- a/common/lib/xmodule/xmodule/tests/test_video.py +++ b/common/lib/xmodule/xmodule/tests/test_video.py @@ -133,8 +133,8 @@ class VideoDescriptorTest(unittest.TestCase): system = get_test_descriptor_system() self.descriptor = system.construct_xblock_from_class( VideoDescriptor, - field_data=DictFieldData({}), scope_ids=ScopeIds(None, None, None, None), + field_data=DictFieldData({}), ) def test_get_context(self): diff --git a/common/lib/xmodule/xmodule/tests/test_xblock_wrappers.py b/common/lib/xmodule/xmodule/tests/test_xblock_wrappers.py index 900907f9d1..ef81a9a26b 100644 --- a/common/lib/xmodule/xmodule/tests/test_xblock_wrappers.py +++ b/common/lib/xmodule/xmodule/tests/test_xblock_wrappers.py @@ -87,8 +87,8 @@ class TestXBlockWrapper(object): runtime.render_template = lambda *args, **kwargs: u'{!r}, {!r}'.format(args, kwargs) return runtime.construct_xblock_from_class( descriptor_cls, + ScopeIds(None, descriptor_cls.__name__, location, location), DictFieldData({}), - ScopeIds(None, descriptor_cls.__name__, location, location) ) def leaf_module(self, descriptor_cls): @@ -109,10 +109,10 @@ class TestXBlockWrapper(object): runtime.render_template = lambda *args, **kwargs: u'{!r}, {!r}'.format(args, kwargs) return runtime.construct_xblock_from_class( descriptor_cls, + ScopeIds(None, descriptor_cls.__name__, location, location), DictFieldData({ 'children': range(3) }), - ScopeIds(None, descriptor_cls.__name__, location, location) ) def container_module(self, descriptor_cls, depth): diff --git a/common/lib/xmodule/xmodule/tests/test_xml_module.py b/common/lib/xmodule/xmodule/tests/test_xml_module.py index da6062f662..f3f3a961f7 100644 --- a/common/lib/xmodule/xmodule/tests/test_xml_module.py +++ b/common/lib/xmodule/xmodule/tests/test_xml_module.py @@ -166,8 +166,8 @@ class EditableMetadataFieldsTest(unittest.TestCase): runtime = get_test_descriptor_system() return runtime.construct_xblock_from_class( XmlDescriptor, + scope_ids=Mock(), field_data=field_data, - scope_ids=Mock() ).editable_metadata_fields def get_descriptor(self, field_data): diff --git a/common/lib/xmodule/xmodule/video_module.py b/common/lib/xmodule/xmodule/video_module.py index febfe5961f..96d0a09498 100644 --- a/common/lib/xmodule/xmodule/video_module.py +++ b/common/lib/xmodule/xmodule/video_module.py @@ -247,12 +247,11 @@ class VideoDescriptor(VideoFields, TabsEditingDescriptor, EmptyDataRawDescriptor field_data = DbModel(kvs) video = system.construct_xblock_from_class( cls, - field_data, - # We're loading a descriptor, so student_id is meaningless # We also don't have separate notions of definition and usage ids yet, # so we use the location for both - ScopeIds(None, location.category, location, location) + ScopeIds(None, location.category, location, location), + field_data, ) return video diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index 63ff80b9d9..271ad4e101 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -502,8 +502,8 @@ class XModuleDescriptor(XModuleMixin, HTMLSnippet, ResourceTemplates, XBlock): module = system.construct_xblock_from_class( self.module_class, descriptor=self, - field_data=system.xmodule_field_data(self), scope_ids=self.scope_ids, + field_data=system.xmodule_field_data(self), ) module.save() return module diff --git a/common/lib/xmodule/xmodule/xml_module.py b/common/lib/xmodule/xmodule/xml_module.py index 83bb6faffd..742d79969b 100644 --- a/common/lib/xmodule/xmodule/xml_module.py +++ b/common/lib/xmodule/xmodule/xml_module.py @@ -339,12 +339,11 @@ class XmlDescriptor(XModuleDescriptor): return system.construct_xblock_from_class( cls, - field_data, - # We're loading a descriptor, so student_id is meaningless # We also don't have separate notions of definition and usage ids yet, # so we use the location for both - ScopeIds(None, location.category, location, location) + ScopeIds(None, location.category, location, location), + field_data, ) @classmethod From c36adf84a0b65337ff9c7cb07cbe6da1741b3037 Mon Sep 17 00:00:00 2001 From: Ned Batchelder Date: Fri, 20 Sep 2013 16:11:03 -0400 Subject: [PATCH 36/54] Minor cosmetic code fixes. --- common/lib/xmodule/xmodule/x_module.py | 12 ++++-------- common/lib/xmodule/xmodule/xml_module.py | 3 +-- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index 271ad4e101..ef43e05a26 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -555,7 +555,7 @@ class XModuleDescriptor(XModuleMixin, HTMLSnippet, ResourceTemplates, XBlock): def from_xml(cls, xml_data, system, org=None, course=None): """ Creates an instance of this descriptor from the supplied xml_data. - This may be overridden by subclasses + This may be overridden by subclasses. xml_data: A string of xml that will be translated into data and children for this module @@ -565,13 +565,12 @@ class XModuleDescriptor(XModuleMixin, HTMLSnippet, ResourceTemplates, XBlock): org and course are optional strings that will be used in the generated module's url identifiers """ - raise NotImplementedError( - 'Modules must implement from_xml to be parsable from xml') + raise NotImplementedError('Modules must implement from_xml to be parsable from xml') def export_to_xml(self, resource_fs): """ Returns an xml string representing this module, and all modules - underneath it. May also write required resources out to resource_fs + underneath it. May also write required resources out to resource_fs. Assumes that modules have single parentage (that no module appears twice in the same course), and that it is thus safe to nest modules as xml @@ -581,8 +580,7 @@ class XModuleDescriptor(XModuleMixin, HTMLSnippet, ResourceTemplates, XBlock): XModuleDescriptor using the from_xml method with the same system, org, and course """ - raise NotImplementedError( - 'Modules must implement export_to_xml to enable xml export') + raise NotImplementedError('Modules must implement export_to_xml to enable xml export') # =============================== BUILTIN METHODS ========================== def __eq__(self, other): @@ -756,8 +754,6 @@ class DescriptorSystem(Runtime): class XMLParsingSystem(DescriptorSystem): def __init__(self, process_xml, policy, **kwargs): """ - load_item, resources_fs, error_tracker: see DescriptorSystem - policy: a policy dictionary for overriding xml metadata process_xml: Takes an xml string, and returns a XModuleDescriptor diff --git a/common/lib/xmodule/xmodule/xml_module.py b/common/lib/xmodule/xmodule/xml_module.py index 742d79969b..b555d8f775 100644 --- a/common/lib/xmodule/xmodule/xml_module.py +++ b/common/lib/xmodule/xmodule/xml_module.py @@ -217,8 +217,7 @@ class XmlDescriptor(XModuleDescriptor): # give the class a chance to fix it up. The file will be written out # again in the correct format. This should go away once the CMS is # online and has imported all current (fall 2012) courses from xml - if not system.resources_fs.exists(filepath) and hasattr( - cls, 'backcompat_paths'): + if not system.resources_fs.exists(filepath) and hasattr(cls, 'backcompat_paths'): candidates = cls.backcompat_paths(filepath) for candidate in candidates: if system.resources_fs.exists(candidate): From 3a6450c3fc4f9020e5e440f963078c6d0a931fb6 Mon Sep 17 00:00:00 2001 From: Ned Batchelder Date: Fri, 13 Sep 2013 14:18:58 -0400 Subject: [PATCH 37/54] Use XModuleDescriptor.parse_xml to interpret XML. --- common/lib/xmodule/setup.py | 41 +++++++++++++++++++ common/lib/xmodule/xmodule/modulestore/xml.py | 36 +++++++++++++++- common/lib/xmodule/xmodule/x_module.py | 40 +++++++----------- 3 files changed, 90 insertions(+), 27 deletions(-) diff --git a/common/lib/xmodule/setup.py b/common/lib/xmodule/setup.py index 6a24bf8f27..13e9e2a2cc 100644 --- a/common/lib/xmodule/setup.py +++ b/common/lib/xmodule/setup.py @@ -17,6 +17,47 @@ setup( # See http://guide.python-distribute.org/creation.html#entry-points # for a description of entry_points entry_points={ + 'xblock.v1': [ + "abtest = xmodule.abtest_module:ABTestDescriptor", + "book = xmodule.backcompat_module:TranslateCustomTagDescriptor", + "chapter = xmodule.seq_module:SequenceDescriptor", + "combinedopenended = xmodule.combined_open_ended_module:CombinedOpenEndedDescriptor", + "conditional = xmodule.conditional_module:ConditionalDescriptor", + "course = xmodule.course_module:CourseDescriptor", + "customtag = xmodule.template_module:CustomTagDescriptor", + "discuss = xmodule.backcompat_module:TranslateCustomTagDescriptor", + "html = xmodule.html_module:HtmlDescriptor", + "image = xmodule.backcompat_module:TranslateCustomTagDescriptor", + "error = xmodule.error_module:ErrorDescriptor", + "peergrading = xmodule.peer_grading_module:PeerGradingDescriptor", + "poll_question = xmodule.poll_module:PollDescriptor", + "problem = xmodule.capa_module:CapaDescriptor", + "problemset = xmodule.seq_module:SequenceDescriptor", + "randomize = xmodule.randomize_module:RandomizeDescriptor", + "section = xmodule.backcompat_module:SemanticSectionDescriptor", + "sequential = xmodule.seq_module:SequenceDescriptor", + "slides = xmodule.backcompat_module:TranslateCustomTagDescriptor", + "timelimit = xmodule.timelimit_module:TimeLimitDescriptor", + "vertical = xmodule.vertical_module:VerticalDescriptor", + "video = xmodule.video_module:VideoDescriptor", + "videoalpha = xmodule.video_module:VideoDescriptor", + "videodev = xmodule.backcompat_module:TranslateCustomTagDescriptor", + "videosequence = xmodule.seq_module:SequenceDescriptor", + "discussion = xmodule.discussion_module:DiscussionDescriptor", + "course_info = xmodule.html_module:CourseInfoDescriptor", + "static_tab = xmodule.html_module:StaticTabDescriptor", + "custom_tag_template = xmodule.raw_module:RawDescriptor", + "about = xmodule.html_module:AboutDescriptor", + "wrapper = xmodule.wrapper_module:WrapperDescriptor", + "graphical_slider_tool = xmodule.gst_module:GraphicalSliderToolDescriptor", + "annotatable = xmodule.annotatable_module:AnnotatableDescriptor", + "foldit = xmodule.foldit_module:FolditDescriptor", + "word_cloud = xmodule.word_cloud_module:WordCloudDescriptor", + "hidden = xmodule.hidden_module:HiddenDescriptor", + "raw = xmodule.raw_module:RawDescriptor", + "crowdsource_hinter = xmodule.crowdsource_hinter:CrowdsourceHinterDescriptor", + "lti = xmodule.lti_module:LTIModuleDescriptor" + ], 'xmodule.v1': [ "abtest = xmodule.abtest_module:ABTestDescriptor", "book = xmodule.backcompat_module:TranslateCustomTagDescriptor", diff --git a/common/lib/xmodule/xmodule/modulestore/xml.py b/common/lib/xmodule/xmodule/modulestore/xml.py index 49ad6f4e95..3354c682c2 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml.py +++ b/common/lib/xmodule/xmodule/modulestore/xml.py @@ -20,8 +20,10 @@ from xmodule.mako_module import MakoDescriptorSystem from xmodule.x_module import XModuleDescriptor, XMLParsingSystem from xmodule.html_module import HtmlDescriptor +from xblock.core import XBlock from xblock.fields import ScopeIds from xblock.field_data import DictFieldData +from xblock.plugin import PluginMissingError from . import ModuleStoreBase, Location, XML_MODULESTORE_TYPE @@ -163,7 +165,7 @@ class ImportSystem(XMLParsingSystem, MakoDescriptorSystem): make_name_unique(xml_data) - descriptor = XModuleDescriptor.load_from_xml( + descriptor = create_block_from_xml( etree.tostring(xml_data, encoding='unicode'), self, self.org, self.course, xmlstore.default_class) except Exception as err: @@ -219,6 +221,38 @@ class ImportSystem(XMLParsingSystem, MakoDescriptorSystem): ) +def create_block_from_xml(xml_data, system, org=None, course=None, default_class=None): + """ + Create an XBlock instance from XML data. + + `xml_data' is a string containing valid xml. + + `system` is an XMLParsingSystem. + + `org` and `course` are optional strings that will be used in the generated + block's url identifiers. + + `default_class` is the class to instantiate of the XML indicates a class + that can't be loaded. + + Returns the fully instantiated XBlock. + + """ + node = etree.fromstring(xml_data) + raw_class = XBlock.load_class(node.tag, default_class) + xblock_class = system.mixologist.mix(raw_class) + + # leave next line commented out - useful for low-level debugging + # log.debug('[create_block_from_xml] tag=%s, class=%s' % (node.tag, xblock_class)) + + url_name = node.get('url_name', node.get('slug')) + location = Location('i4x', org, course, node.tag, url_name) + + scope_ids = ScopeIds(None, location.category, location, location) + xblock = xblock_class.parse_xml(node, system, scope_ids) + return xblock + + class ParentTracker(object): """A simple class to factor out the logic for tracking location parent pointers.""" def __init__(self): diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index ef43e05a26..ef37843371 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -524,32 +524,15 @@ class XModuleDescriptor(XModuleMixin, HTMLSnippet, ResourceTemplates, XBlock): return cls.metadata_translations.get(key, key) # ================================= XML PARSING ============================ - @staticmethod - def load_from_xml(xml_data, - system, - org=None, - course=None, - default_class=None): + @classmethod + def parse_xml(cls, node, runtime, keys): """ - This method instantiates the correct subclass of XModuleDescriptor based - on the contents of xml_data. - - xml_data must be a string containing valid xml - - system is an XMLParsingSystem - - org and course are optional strings that will be used in the generated - module's url identifiers + Interpret the parsed XML in `node`, creating an XModuleDescriptor. """ - class_ = system.mixologist.mix(XModuleDescriptor.load_class( - etree.fromstring(xml_data).tag, - default_class - )) - # leave next line, commented out - useful for low-level debugging - # log.debug('[XModuleDescriptor.load_from_xml] tag=%s, class_=%s' % ( - # etree.fromstring(xml_data).tag,class_)) - - return class_.from_xml(xml_data, system, org, course) + xml = etree.tostring(node) + # TODO: change from_xml to not take org and course, it can use self.system. + block = cls.from_xml(xml, runtime, runtime.org, runtime.course) + return block @classmethod def from_xml(cls, xml_data, system, org=None, course=None): @@ -713,7 +696,9 @@ class DescriptorSystem(Runtime): that you're about to re-raise---let the caller track them. """ - super(DescriptorSystem, self).__init__(**kwargs) + # Right now, usage_store is unused, and field_data is always supplanted + # with an explicit field_data during construct_xblock, so None's suffice. + super(DescriptorSystem, self).__init__(usage_store=None, field_data=None, **kwargs) self.load_item = load_item self.resources_fs = resources_fs @@ -835,7 +820,10 @@ class ModuleSystem(Runtime): not to allow the execution of unsafe, unsandboxed code. """ - super(ModuleSystem, self).__init__(**kwargs) + + # Right now, usage_store is unused, and field_data is always supplanted + # with an explicit field_data during construct_xblock, so None's suffice. + super(ModuleSystem, self).__init__(usage_store=None, field_data=None, **kwargs) self.ajax_url = ajax_url self.xqueue = xqueue From a1d464aed9fc383689981210f677af6f911a0d3f Mon Sep 17 00:00:00 2001 From: Ned Batchelder Date: Fri, 27 Sep 2013 10:04:11 -0400 Subject: [PATCH 38/54] Update XBlock requirement to new XML code. --- common/lib/xmodule/setup.py | 132 +++++++++++++----------------------- requirements/edx/github.txt | 2 +- 2 files changed, 48 insertions(+), 86 deletions(-) diff --git a/common/lib/xmodule/setup.py b/common/lib/xmodule/setup.py index 13e9e2a2cc..c99234e385 100644 --- a/common/lib/xmodule/setup.py +++ b/common/lib/xmodule/setup.py @@ -1,5 +1,47 @@ from setuptools import setup, find_packages +XMODULES = [ + "abtest = xmodule.abtest_module:ABTestDescriptor", + "book = xmodule.backcompat_module:TranslateCustomTagDescriptor", + "chapter = xmodule.seq_module:SequenceDescriptor", + "combinedopenended = xmodule.combined_open_ended_module:CombinedOpenEndedDescriptor", + "conditional = xmodule.conditional_module:ConditionalDescriptor", + "course = xmodule.course_module:CourseDescriptor", + "customtag = xmodule.template_module:CustomTagDescriptor", + "discuss = xmodule.backcompat_module:TranslateCustomTagDescriptor", + "html = xmodule.html_module:HtmlDescriptor", + "image = xmodule.backcompat_module:TranslateCustomTagDescriptor", + "error = xmodule.error_module:ErrorDescriptor", + "peergrading = xmodule.peer_grading_module:PeerGradingDescriptor", + "poll_question = xmodule.poll_module:PollDescriptor", + "problem = xmodule.capa_module:CapaDescriptor", + "problemset = xmodule.seq_module:SequenceDescriptor", + "randomize = xmodule.randomize_module:RandomizeDescriptor", + "section = xmodule.backcompat_module:SemanticSectionDescriptor", + "sequential = xmodule.seq_module:SequenceDescriptor", + "slides = xmodule.backcompat_module:TranslateCustomTagDescriptor", + "timelimit = xmodule.timelimit_module:TimeLimitDescriptor", + "vertical = xmodule.vertical_module:VerticalDescriptor", + "video = xmodule.video_module:VideoDescriptor", + "videoalpha = xmodule.video_module:VideoDescriptor", + "videodev = xmodule.backcompat_module:TranslateCustomTagDescriptor", + "videosequence = xmodule.seq_module:SequenceDescriptor", + "discussion = xmodule.discussion_module:DiscussionDescriptor", + "course_info = xmodule.html_module:CourseInfoDescriptor", + "static_tab = xmodule.html_module:StaticTabDescriptor", + "custom_tag_template = xmodule.raw_module:RawDescriptor", + "about = xmodule.html_module:AboutDescriptor", + "wrapper = xmodule.wrapper_module:WrapperDescriptor", + "graphical_slider_tool = xmodule.gst_module:GraphicalSliderToolDescriptor", + "annotatable = xmodule.annotatable_module:AnnotatableDescriptor", + "foldit = xmodule.foldit_module:FolditDescriptor", + "word_cloud = xmodule.word_cloud_module:WordCloudDescriptor", + "hidden = xmodule.hidden_module:HiddenDescriptor", + "raw = xmodule.raw_module:RawDescriptor", + "crowdsource_hinter = xmodule.crowdsource_hinter:CrowdsourceHinterDescriptor", + "lti = xmodule.lti_module:LTIModuleDescriptor", +] + setup( name="XModule", version="0.1", @@ -11,96 +53,16 @@ setup( 'path.py', ], package_data={ - 'xmodule': ['js/module/*'] + 'xmodule': ['js/module/*'], }, # See http://guide.python-distribute.org/creation.html#entry-points # for a description of entry_points entry_points={ - 'xblock.v1': [ - "abtest = xmodule.abtest_module:ABTestDescriptor", - "book = xmodule.backcompat_module:TranslateCustomTagDescriptor", - "chapter = xmodule.seq_module:SequenceDescriptor", - "combinedopenended = xmodule.combined_open_ended_module:CombinedOpenEndedDescriptor", - "conditional = xmodule.conditional_module:ConditionalDescriptor", - "course = xmodule.course_module:CourseDescriptor", - "customtag = xmodule.template_module:CustomTagDescriptor", - "discuss = xmodule.backcompat_module:TranslateCustomTagDescriptor", - "html = xmodule.html_module:HtmlDescriptor", - "image = xmodule.backcompat_module:TranslateCustomTagDescriptor", - "error = xmodule.error_module:ErrorDescriptor", - "peergrading = xmodule.peer_grading_module:PeerGradingDescriptor", - "poll_question = xmodule.poll_module:PollDescriptor", - "problem = xmodule.capa_module:CapaDescriptor", - "problemset = xmodule.seq_module:SequenceDescriptor", - "randomize = xmodule.randomize_module:RandomizeDescriptor", - "section = xmodule.backcompat_module:SemanticSectionDescriptor", - "sequential = xmodule.seq_module:SequenceDescriptor", - "slides = xmodule.backcompat_module:TranslateCustomTagDescriptor", - "timelimit = xmodule.timelimit_module:TimeLimitDescriptor", - "vertical = xmodule.vertical_module:VerticalDescriptor", - "video = xmodule.video_module:VideoDescriptor", - "videoalpha = xmodule.video_module:VideoDescriptor", - "videodev = xmodule.backcompat_module:TranslateCustomTagDescriptor", - "videosequence = xmodule.seq_module:SequenceDescriptor", - "discussion = xmodule.discussion_module:DiscussionDescriptor", - "course_info = xmodule.html_module:CourseInfoDescriptor", - "static_tab = xmodule.html_module:StaticTabDescriptor", - "custom_tag_template = xmodule.raw_module:RawDescriptor", - "about = xmodule.html_module:AboutDescriptor", - "wrapper = xmodule.wrapper_module:WrapperDescriptor", - "graphical_slider_tool = xmodule.gst_module:GraphicalSliderToolDescriptor", - "annotatable = xmodule.annotatable_module:AnnotatableDescriptor", - "foldit = xmodule.foldit_module:FolditDescriptor", - "word_cloud = xmodule.word_cloud_module:WordCloudDescriptor", - "hidden = xmodule.hidden_module:HiddenDescriptor", - "raw = xmodule.raw_module:RawDescriptor", - "crowdsource_hinter = xmodule.crowdsource_hinter:CrowdsourceHinterDescriptor", - "lti = xmodule.lti_module:LTIModuleDescriptor" - ], - 'xmodule.v1': [ - "abtest = xmodule.abtest_module:ABTestDescriptor", - "book = xmodule.backcompat_module:TranslateCustomTagDescriptor", - "chapter = xmodule.seq_module:SequenceDescriptor", - "combinedopenended = xmodule.combined_open_ended_module:CombinedOpenEndedDescriptor", - "conditional = xmodule.conditional_module:ConditionalDescriptor", - "course = xmodule.course_module:CourseDescriptor", - "customtag = xmodule.template_module:CustomTagDescriptor", - "discuss = xmodule.backcompat_module:TranslateCustomTagDescriptor", - "html = xmodule.html_module:HtmlDescriptor", - "image = xmodule.backcompat_module:TranslateCustomTagDescriptor", - "error = xmodule.error_module:ErrorDescriptor", - "peergrading = xmodule.peer_grading_module:PeerGradingDescriptor", - "poll_question = xmodule.poll_module:PollDescriptor", - "problem = xmodule.capa_module:CapaDescriptor", - "problemset = xmodule.seq_module:SequenceDescriptor", - "randomize = xmodule.randomize_module:RandomizeDescriptor", - "section = xmodule.backcompat_module:SemanticSectionDescriptor", - "sequential = xmodule.seq_module:SequenceDescriptor", - "slides = xmodule.backcompat_module:TranslateCustomTagDescriptor", - "timelimit = xmodule.timelimit_module:TimeLimitDescriptor", - "vertical = xmodule.vertical_module:VerticalDescriptor", - "video = xmodule.video_module:VideoDescriptor", - "videoalpha = xmodule.video_module:VideoDescriptor", - "videodev = xmodule.backcompat_module:TranslateCustomTagDescriptor", - "videosequence = xmodule.seq_module:SequenceDescriptor", - "discussion = xmodule.discussion_module:DiscussionDescriptor", - "course_info = xmodule.html_module:CourseInfoDescriptor", - "static_tab = xmodule.html_module:StaticTabDescriptor", - "custom_tag_template = xmodule.raw_module:RawDescriptor", - "about = xmodule.html_module:AboutDescriptor", - "wrapper = xmodule.wrapper_module:WrapperDescriptor", - "graphical_slider_tool = xmodule.gst_module:GraphicalSliderToolDescriptor", - "annotatable = xmodule.annotatable_module:AnnotatableDescriptor", - "foldit = xmodule.foldit_module:FolditDescriptor", - "word_cloud = xmodule.word_cloud_module:WordCloudDescriptor", - "hidden = xmodule.hidden_module:HiddenDescriptor", - "raw = xmodule.raw_module:RawDescriptor", - "crowdsource_hinter = xmodule.crowdsource_hinter:CrowdsourceHinterDescriptor", - "lti = xmodule.lti_module:LTIModuleDescriptor" - ], + 'xblock.v1': XMODULES, + 'xmodule.v1': XMODULES, 'console_scripts': [ 'xmodule_assets = xmodule.static_content:main', - ] - } + ], + }, ) diff --git a/requirements/edx/github.txt b/requirements/edx/github.txt index e0c88b217e..3a95566b2b 100644 --- a/requirements/edx/github.txt +++ b/requirements/edx/github.txt @@ -14,7 +14,7 @@ -e git+https://github.com/eventbrite/zendesk.git@d53fe0e81b623f084e91776bcf6369f8b7b63879#egg=zendesk # Our libraries: --e git+https://github.com/edx/XBlock.git@a8de02c0#egg=XBlock +-e git+https://github.com/edx/XBlock.git@8a66ca3#egg=XBlock -e git+https://github.com/edx/codejail.git@0a1b468#egg=codejail -e git+https://github.com/edx/diff-cover.git@v0.2.4#egg=diff_cover -e git+https://github.com/edx/js-test-tool.git@v0.0.7#egg=js_test_tool From 718328e4e63b90944f643c04c78168a268841822 Mon Sep 17 00:00:00 2001 From: Ned Batchelder Date: Mon, 30 Sep 2013 10:34:36 -0400 Subject: [PATCH 39/54] Get rid of all mention of XModuleDescriptor. --- common/lib/xmodule/xmodule/modulestore/xml.py | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/xml.py b/common/lib/xmodule/xmodule/modulestore/xml.py index 3354c682c2..74c375a545 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml.py +++ b/common/lib/xmodule/xmodule/modulestore/xml.py @@ -17,13 +17,12 @@ from xmodule.error_module import ErrorDescriptor from xmodule.errortracker import make_error_tracker, exc_info_to_str from xmodule.course_module import CourseDescriptor from xmodule.mako_module import MakoDescriptorSystem -from xmodule.x_module import XModuleDescriptor, XMLParsingSystem +from xmodule.x_module import XMLParsingSystem from xmodule.html_module import HtmlDescriptor from xblock.core import XBlock from xblock.fields import ScopeIds from xblock.field_data import DictFieldData -from xblock.plugin import PluginMissingError from . import ModuleStoreBase, Location, XML_MODULESTORE_TYPE @@ -65,7 +64,7 @@ class ImportSystem(XMLParsingSystem, MakoDescriptorSystem): self.load_error_modules = load_error_modules def process_xml(xml): - """Takes an xml string, and returns a XModuleDescriptor created from + """Takes an xml string, and returns a XBlock created from that xml. """ @@ -312,8 +311,8 @@ class XMLModuleStore(ModuleStoreBase): super(XMLModuleStore, self).__init__(**kwargs) self.data_dir = path(data_dir) - self.modules = defaultdict(dict) # course_id -> dict(location -> XModuleDescriptor) - self.courses = {} # course_dir -> XModuleDescriptor for the course + self.modules = defaultdict(dict) # course_id -> dict(location -> XBlock) + self.courses = {} # course_dir -> XBlock for the course self.errored_courses = {} # course_dir -> errorlog, for dirs that failed to load self.load_error_modules = load_error_modules @@ -534,7 +533,7 @@ class XMLModuleStore(ModuleStoreBase): def get_instance(self, course_id, location, depth=0): """ - Returns an XModuleDescriptor instance for the item at + Returns an XBlock instance for the item at location, with the policy for course_id. (In case two xml dirs have different content at the same location, return the one for this course_id.) @@ -562,7 +561,7 @@ class XMLModuleStore(ModuleStoreBase): def get_item(self, location, depth=0): """ - Returns an XModuleDescriptor instance for the item at location. + Returns an XBlock instance for the item at location. If any segment of the location is None except revision, raises xmodule.modulestore.exceptions.InsufficientSpecificationError From cdb80bb8eda37776aa9423462fc368df6fde49e1 Mon Sep 17 00:00:00 2001 From: Ned Batchelder Date: Mon, 30 Sep 2013 12:03:38 -0400 Subject: [PATCH 40/54] Fix the new tests to use the new way to make XBlocks. --- common/lib/xmodule/xmodule/tests/xml/__init__.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/common/lib/xmodule/xmodule/tests/xml/__init__.py b/common/lib/xmodule/xmodule/tests/xml/__init__.py index 32cefadca7..93aa582cde 100644 --- a/common/lib/xmodule/xmodule/tests/xml/__init__.py +++ b/common/lib/xmodule/xmodule/tests/xml/__init__.py @@ -4,8 +4,9 @@ Xml parsing tests for XModules import pprint from mock import Mock -from xmodule.x_module import XMLParsingSystem, XModuleDescriptor +from xmodule.x_module import XMLParsingSystem from xmodule.mako_module import MakoDescriptorSystem +from xmodule.modulestore.xml import create_block_from_xml class InMemorySystem(XMLParsingSystem, MakoDescriptorSystem): # pylint: disable=abstract-method @@ -28,8 +29,8 @@ class InMemorySystem(XMLParsingSystem, MakoDescriptorSystem): # pylint: disable ) def process_xml(self, xml): # pylint: disable=method-hidden - """Parse `xml` as an XModuleDescriptor, and add it to `self._descriptors`""" - descriptor = XModuleDescriptor.load_from_xml(xml, self, self.org, self.course, self.default_class) + """Parse `xml` as an XBlock, and add it to `self._descriptors`""" + descriptor = create_block_from_xml(xml, self, self.org, self.course, self.default_class) self._descriptors[descriptor.location.url()] = descriptor return descriptor @@ -39,8 +40,8 @@ class InMemorySystem(XMLParsingSystem, MakoDescriptorSystem): # pylint: disable class XModuleXmlImportTest(object): - """Base class for tests that use basic `XModuleDescriptor.load_from_xml` xml parsing""" + """Base class for tests that use basic XML parsing""" def process_xml(self, xml_import_data): - """Use the `xml_import_data` to import an :class:`XModuleDescriptor` from xml""" + """Use the `xml_import_data` to import an :class:`XBlock` from XML.""" system = InMemorySystem(xml_import_data) return system.process_xml(xml_import_data.xml_string) From 0ed1ee917ed14fb7678df1cef1a9833d285077b8 Mon Sep 17 00:00:00 2001 From: Nick Parlante Date: Thu, 26 Sep 2013 16:34:43 -0700 Subject: [PATCH 41/54] Create edit_course_tabs management command Just works on the tabs list in mongo, nothing deeper than that. Added implementation functions in tabs.py, providing a first step for some future GUI editor. --- CHANGELOG.rst | 3 + .../management/commands/edit_course_tabs.py | 88 +++++++++++++++++++ .../contentstore/tests/test_tabs.py | 41 +++++++++ cms/djangoapps/contentstore/views/tabs.py | 44 +++++++++- 4 files changed, 175 insertions(+), 1 deletion(-) create mode 100644 cms/djangoapps/contentstore/management/commands/edit_course_tabs.py create mode 100644 cms/djangoapps/contentstore/tests/test_tabs.py diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 443e787482..dbe74f3059 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -7,6 +7,9 @@ the top. Include a label indicating the component affected. LMS: Add split testing functionality for internal use. +CMS: Add edit_course_tabs management command, providing a primitive +editing capability for a course's list of tabs. + Studio and LMS: add ability to lock assets (cannot be viewed unless registered for class). LMS: Improved accessibility of parts of forum navigation sidebar. diff --git a/cms/djangoapps/contentstore/management/commands/edit_course_tabs.py b/cms/djangoapps/contentstore/management/commands/edit_course_tabs.py new file mode 100644 index 0000000000..d9c73e42fa --- /dev/null +++ b/cms/djangoapps/contentstore/management/commands/edit_course_tabs.py @@ -0,0 +1,88 @@ +### +### Script for editing the course's tabs +### + +# +# Run it this way: +# ./manage.py cms --settings dev edit_course_tabs --course Stanford/CS99/2013_spring +# Or via rake: +# rake django-admin[edit_course_tabs,cms,dev,"--course Stanford/CS99/2013_spring --delete 4"] +# +from optparse import make_option +from django.core.management.base import BaseCommand, CommandError +from .prompt import query_yes_no + +from courseware.courses import get_course_by_id + +from contentstore.views import tabs + + +def print_course(course): + "Prints out the course id and a numbered list of tabs." + print course.id + for index, item in enumerate(course.tabs): + print index + 1, '"' + item.get('type') + '"', '"' + item.get('name', '') + '"' + + +# course.tabs looks like this +# [{u'type': u'courseware'}, {u'type': u'course_info', u'name': u'Course Info'}, {u'type': u'textbooks'}, +# {u'type': u'discussion', u'name': u'Discussion'}, {u'type': u'wiki', u'name': u'Wiki'}, +# {u'type': u'progress', u'name': u'Progress'}] + + +class Command(BaseCommand): + help = """See and edit a course's tabs list. +Only supports insertion and deletion. Move and +rename etc. can be done with a delete +followed by an insert. +The tabs are numbered starting with 1. +Tabs 1 and 2 cannot be changed, and tabs of type +static_tab cannot be edited (use Studio for those). +""" + # Making these option objects separately, so can refer to their .help below + course_option = make_option('--course', + action='store', + dest='course', + default=False, + help='--course required, e.g. Stanford/CS99/2013_spring') + delete_option = make_option('--delete', + action='store_true', + dest='delete', + default=False, + help='--delete ') + insert_option = make_option('--insert', + action='store_true', + dest='insert', + default=False, + help='--insert , e.g. 2 "course_info" "Course Info"') + + option_list = BaseCommand.option_list + (course_option, delete_option, insert_option) + + def handle(self, *args, **options): + if not options['course']: + raise CommandError(Command.course_option.help) + + course = get_course_by_id(options['course']) + + print 'Warning: this command directly edits the list of course tabs in mongo.' + print 'Tabs before any changes:' + print_course(course) + + try: + if options['delete']: + if len(args) != 1: + raise CommandError(Command.delete_option.help) + num = int(args[0]) + if query_yes_no('Deleting tab {0} Confirm?'.format(num), default='no'): + tabs.primitive_delete(course, num - 1) # -1 for 0-based indexing + elif options['insert']: + if len(args) != 3: + raise CommandError(Command.insert_option.help) + num = int(args[0]) + tab_type = args[1] + name = args[2] + if query_yes_no('Inserting tab {0} "{1}" "{2}" Confirm?'.format(num, tab_type, name), default='no'): + tabs.primitive_insert(course, num - 1, tab_type, name) # -1 as above + except ValueError as e: + # Cute: translate to CommandError so the CLI error prints nicely. + raise CommandError(e) diff --git a/cms/djangoapps/contentstore/tests/test_tabs.py b/cms/djangoapps/contentstore/tests/test_tabs.py new file mode 100644 index 0000000000..f1cf8ddfa5 --- /dev/null +++ b/cms/djangoapps/contentstore/tests/test_tabs.py @@ -0,0 +1,41 @@ +""" Tests for tab functions (just primitive). """ + +from contentstore.views import tabs +from django.test import TestCase +from xmodule.modulestore.tests.factories import CourseFactory +from courseware.courses import get_course_by_id + + +class PrimitiveTabEdit(TestCase): + """Tests for the primitive tab edit data manipulations""" + + def test_delete(self): + """Test primitive tab deletion.""" + course = CourseFactory.create(org='edX', course='999') + with self.assertRaises(ValueError): + tabs.primitive_delete(course, 0) + with self.assertRaises(ValueError): + tabs.primitive_delete(course, 1) + with self.assertRaises(IndexError): + tabs.primitive_delete(course, 6) + tabs.primitive_delete(course, 2) + self.assertFalse({u'type': u'textbooks'} in course.tabs) + # Check that discussion has shifted down + self.assertEquals(course.tabs[2], {'type': 'discussion', 'name': 'Discussion'}) + + def test_insert(self): + """Test primitive tab insertion.""" + course = CourseFactory.create(org='edX', course='999') + tabs.primitive_insert(course, 2, 'atype', 'aname') + self.assertEquals(course.tabs[2], {'type': 'atype', 'name': 'aname'}) + with self.assertRaises(ValueError): + tabs.primitive_insert(course, 0, 'atype', 'aname') + with self.assertRaises(ValueError): + tabs.primitive_insert(course, 3, 'static_tab', 'aname') + + def test_save(self): + """Test course saving.""" + course = CourseFactory.create(org='edX', course='999') + tabs.primitive_insert(course, 3, 'atype', 'aname') + course2 = get_course_by_id(course.id) + self.assertEquals(course2.tabs[3], {'type': 'atype', 'name': 'aname'}) diff --git a/cms/djangoapps/contentstore/views/tabs.py b/cms/djangoapps/contentstore/views/tabs.py index f38685edfc..f897fa1378 100644 --- a/cms/djangoapps/contentstore/views/tabs.py +++ b/cms/djangoapps/contentstore/views/tabs.py @@ -9,13 +9,14 @@ from django.contrib.auth.decorators import login_required from django.core.exceptions import PermissionDenied from django_future.csrf import ensure_csrf_cookie from mitxmako.shortcuts import render_to_response - from xmodule.modulestore import Location from xmodule.modulestore.inheritance import own_metadata from xmodule.modulestore.django import modulestore + from ..utils import get_course_for_item, get_modulestore from .access import get_location_and_verify_access + __all__ = ['edit_tabs', 'reorder_static_tabs', 'static_pages'] @@ -84,6 +85,7 @@ def reorder_static_tabs(request): # MongoKeyValueStore before we update the mongo datastore. course.save() modulestore('direct').update_metadata(course.location, own_metadata(course)) + # TODO: above two lines are used for the primitive-save case. Maybe factor them out? return HttpResponse() @@ -136,3 +138,43 @@ def static_pages(request, org, course, coursename): return render_to_response('static-pages.html', { 'context_course': course, }) + + +# "primitive" tab edit functions driven by the command line. +# These should be replaced/deleted by a more capable GUI someday. +# Note that the command line UI identifies the tabs with 1-based +# indexing, but this implementation code is standard 0-based. + +def validate_args(num, tab_type): + "Throws for the disallowed cases." + if num <= 1: + raise ValueError('Tabs 1 and 2 cannot be edited') + if tab_type == 'static_tab': + raise ValueError('Tabs of type static_tab cannot be edited here (use Studio)') + + +def primitive_delete(course, num): + "Deletes the given tab number (0 based)." + tabs = course.tabs + validate_args(num, tabs[num].get('type', '')) + del tabs[num] + # Note for future implementations: if you delete a static_tab, then Chris Dodge + # points out that there's other stuff to delete beyond this element. + # This code happens to not delete static_tab so it doesn't come up. + primitive_save(course) + + +def primitive_insert(course, num, tab_type, name): + "Inserts a new tab at the given number (0 based)." + validate_args(num, tab_type) + new_tab = {u'type': unicode(tab_type), u'name': unicode(name)} + tabs = course.tabs + tabs.insert(num, new_tab) + primitive_save(course) + + +def primitive_save(course): + "Saves the course back to modulestore." + # This code copied from reorder_static_tabs above + course.save() + modulestore('direct').update_metadata(course.location, own_metadata(course)) From 0bb11335b2e8242592ef33804b8a0ffbb1a89e4e Mon Sep 17 00:00:00 2001 From: Ned Batchelder Date: Tue, 1 Oct 2013 13:53:59 -0400 Subject: [PATCH 42/54] We aren't ready to use XBlock.load_class, there are duplicate entry point names, and simply choosing the first one leads to unpredictability. --- common/lib/xmodule/xmodule/modulestore/xml.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/xml.py b/common/lib/xmodule/xmodule/modulestore/xml.py index 74c375a545..da86279b68 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml.py +++ b/common/lib/xmodule/xmodule/modulestore/xml.py @@ -17,7 +17,7 @@ from xmodule.error_module import ErrorDescriptor from xmodule.errortracker import make_error_tracker, exc_info_to_str from xmodule.course_module import CourseDescriptor from xmodule.mako_module import MakoDescriptorSystem -from xmodule.x_module import XMLParsingSystem +from xmodule.x_module import XMLParsingSystem, XModuleDescriptor from xmodule.html_module import HtmlDescriptor from xblock.core import XBlock @@ -238,7 +238,7 @@ def create_block_from_xml(xml_data, system, org=None, course=None, default_class """ node = etree.fromstring(xml_data) - raw_class = XBlock.load_class(node.tag, default_class) + raw_class = XModuleDescriptor.load_class(node.tag, default_class) xblock_class = system.mixologist.mix(raw_class) # leave next line commented out - useful for low-level debugging From b5fc5d89d3c97a5f90e86dc8d87f83c3d770c36f Mon Sep 17 00:00:00 2001 From: Will Daly Date: Tue, 1 Oct 2013 15:53:16 -0400 Subject: [PATCH 43/54] Fix run single test example --- docs/internal/testing.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/internal/testing.md b/docs/internal/testing.md index 5d6c75bddb..14cc2879b3 100644 --- a/docs/internal/testing.md +++ b/docs/internal/testing.md @@ -128,11 +128,11 @@ other module level tests include To run a single django test class: - rake test_lms[courseware.tests.tests:testViewAuth] + rake test_lms[lms/djangoapps/courseware/tests/tests.py:ActivateLoginTest] To run a single django test: - rake test_lms[courseware.tests.tests:TestViewAuth.test_dark_launch] + rake test_lms[lms/djangoapps/courseware/tests/tests.py:ActivateLoginTest.test_activate_login] To re-run all failing django tests from lms or cms: From 6e3ece92e37bdf2a56d3c62c125c583b63ec7c1f Mon Sep 17 00:00:00 2001 From: Jay Zoldak Date: Thu, 26 Sep 2013 11:51:09 -0400 Subject: [PATCH 44/54] Refactor upload acceptance tests and add locked asset tests for registered user --- .../contentstore/features/upload.feature | 85 ++++++------- .../contentstore/features/upload.py | 117 ++++++++++-------- common/djangoapps/terrain/course_helpers.py | 11 +- common/test/data/uploads/.gitignore | 3 + common/test/data/uploads/test | 1 - 5 files changed, 108 insertions(+), 109 deletions(-) create mode 100644 common/test/data/uploads/.gitignore delete mode 100644 common/test/data/uploads/test diff --git a/cms/djangoapps/contentstore/features/upload.feature b/cms/djangoapps/contentstore/features/upload.feature index e01bcf8fed..2e73c11c0c 100644 --- a/cms/djangoapps/contentstore/features/upload.feature +++ b/cms/djangoapps/contentstore/features/upload.feature @@ -5,17 +5,16 @@ Feature: CMS.Upload Files # Uploading isn't working on safari with sauce labs @skip_safari Scenario: Users can upload files - Given I have opened a new course in Studio - And I go to the files and uploads page + Given I am at the files and upload page of a Studio course When I upload the file "test" Then I should see the file "test" was uploaded And The url for the file "test" is valid + # Uploading isn't working on safari with sauce labs @skip_safari Scenario: Users can upload multiple files - Given I have opened a new course in studio - And I go to the files and uploads page - When I upload the files "test","test2" + Given I am at the files and upload page of a Studio course + When I upload the files "test,test2" Then I should see the file "test" was uploaded And I should see the file "test2" was uploaded And The url for the file "test2" is valid @@ -24,8 +23,7 @@ Feature: CMS.Upload Files # Uploading isn't working on safari with sauce labs @skip_safari Scenario: Users can update files - Given I have opened a new course in studio - And I go to the files and uploads page + Given I am at the files and upload page of a Studio course When I upload the file "test" And I upload the file "test" Then I should see only one "test" @@ -33,8 +31,7 @@ Feature: CMS.Upload Files # Uploading isn't working on safari with sauce labs @skip_safari Scenario: Users can delete uploaded files - Given I have opened a new course in studio - And I go to the files and uploads page + Given I am at the files and upload page of a Studio course When I upload the file "test" And I delete the file "test" Then I should not see the file "test" was uploaded @@ -43,16 +40,14 @@ Feature: CMS.Upload Files # Uploading isn't working on safari with sauce labs @skip_safari Scenario: Users can download files - Given I have opened a new course in studio - And I go to the files and uploads page + Given I am at the files and upload page of a Studio course When I upload the file "test" Then I can download the correct "test" file # Uploading isn't working on safari with sauce labs @skip_safari Scenario: Users can download updated files - Given I have opened a new course in studio - And I go to the files and uploads page + Given I am at the files and upload page of a Studio course When I upload the file "test" And I modify "test" And I reload the page @@ -62,57 +57,59 @@ Feature: CMS.Upload Files # Uploading isn't working on safari with sauce labs @skip_safari Scenario: Users can lock assets through asset index - Given I have opened a new course in studio - And I go to the files and uploads page - When I upload the file "test" - And I lock "test" - Then "test" is locked + Given I am at the files and upload page of a Studio course + When I upload an asset + And I lock the asset + Then the asset is locked And I see a "saving" notification And I reload the page - Then "test" is locked + Then the asset is locked # Uploading isn't working on safari with sauce labs @skip_safari Scenario: Users can unlock assets through asset index - Given I have opened a course with a locked asset "test" - And I unlock "test" - Then "test" is unlocked + Given I have created a course with a locked asset + When I unlock the asset + Then the asset is unlocked And I see a "saving" notification And I reload the page - Then "test" is unlocked + Then the asset is unlocked # Uploading isn't working on safari with sauce labs - # TODO: work with Jay -# @skip_safari -# Scenario: Locked assets can't be viewed if logged in as unregistered user -# Given I have opened a course with a locked asset "locked.html" -# Then the asset "locked.html" can be clicked from the asset index -# And the user "bob" exists -# And "bob" logs in -# Then the asset "locked.html" is protected + @skip_safari + Scenario: Locked assets can't be viewed if logged in as an unregistered user + Given I have created a course with a locked asset + And the user "bob" exists + When "bob" logs in + Then the asset is protected + + # Uploading isn't working on safari with sauce labs + @skip_safari + Scenario: Locked assets can be viewed if logged in as a registered user + Given I have created a course with a locked asset + And the user "bob" exists + And the user "bob" is enrolled in the course + When "bob" logs in + Then the asset is viewable # Uploading isn't working on safari with sauce labs @skip_safari Scenario: Locked assets can't be viewed if logged out - Given I have opened a course with a locked asset "locked.html" - # Note that logging out doesn't really matter at the moment- - # the asset will be protected because the user sent to middleware is the anonymous user. - # Need to work with Jay. - And I log out - Then the asset "locked.html" is protected + Given I have created a course with a locked asset + When I log out + Then the asset is protected # Uploading isn't working on safari with sauce labs @skip_safari Scenario: Locked assets can be viewed with is_staff account - Given I have opened a course with a locked asset "locked.html" + Given I have created a course with a locked asset And the user "staff" exists as a course is_staff - And "staff" logs in - Then the asset "locked.html" can be clicked from the asset index + When "staff" logs in + Then the asset is viewable # Uploading isn't working on safari with sauce labs @skip_safari Scenario: Unlocked assets can be viewed by anyone - Given I have opened a course with a unlocked asset "unlocked.html" - Then the asset "unlocked.html" can be clicked from the asset index - And I log out - Then the asset "unlocked.html" is viewable + Given I have created a course with a unlocked asset + When I log out + Then the asset is viewable diff --git a/cms/djangoapps/contentstore/features/upload.py b/cms/djangoapps/contentstore/features/upload.py index b94ccd114a..25e33f7e5e 100644 --- a/cms/djangoapps/contentstore/features/upload.py +++ b/cms/djangoapps/contentstore/features/upload.py @@ -2,14 +2,17 @@ #pylint: disable=W0621 from lettuce import world, step +from lettuce.django import django_url from django.conf import settings import requests import string import random import os +from django.contrib.auth.models import User +from student.models import CourseEnrollment +from splinter.request_handler.status_code import HttpResponseError from nose.tools import assert_equal, assert_not_equal # pylint: disable=E0611 - TEST_ROOT = settings.COMMON_TEST_DATA_ROOT ASSET_NAMES_CSS = 'td.name-col > span.title > a.filename' @@ -26,7 +29,10 @@ def go_to_uploads(_step): def upload_file(_step, file_name): upload_css = 'a.upload-button' world.css_click(upload_css) - #uploading the file itself + + _write_test_file(file_name, "test file") + + # uploading the file itself path = os.path.join(TEST_ROOT, 'uploads/', file_name) world.browser.execute_script("$('input.file-input').css('display', 'block')") world.browser.attach_file('file', os.path.abspath(path)) @@ -34,19 +40,20 @@ def upload_file(_step, file_name): world.css_click(close_css) -@step(u'I upload the files (".*")$') +@step(u'I upload the files "([^"]*)"$') def upload_files(_step, files_string): - # Turn files_string to a list of file names + # files_string should be comma separated with no spaces. files = files_string.split(",") - files = map(lambda x: string.strip(x, ' "\''), files) - upload_css = 'a.upload-button' world.css_click(upload_css) - #uploading the files - for f in files: - path = os.path.join(TEST_ROOT, 'uploads/', f) + + # uploading the files + for filename in files: + _write_test_file(filename, "test file") + path = os.path.join(TEST_ROOT, 'uploads/', filename) world.browser.execute_script("$('input.file-input').css('display', 'block')") world.browser.attach_file('file', os.path.abspath(path)) + close_css = 'a.close-button' world.css_click(close_css) @@ -104,13 +111,13 @@ def check_download(_step, file_name): r = get_file(file_name) downloaded_text = r.text assert cur_text == downloaded_text - #resetting the file back to its original state + # resetting the file back to its original state _write_test_file(file_name, "This is an arbitrary file for testing uploads") def _write_test_file(file_name, text): path = os.path.join(TEST_ROOT, 'uploads/', file_name) - #resetting the file back to its original state + # resetting the file back to its original state with open(os.path.abspath(path), 'w') as cur_file: cur_file.write(text) @@ -121,68 +128,68 @@ def modify_upload(_step, file_name): _write_test_file(file_name, new_text) -@step(u'I (lock|unlock) "([^"]*)"$') -def lock_unlock_file(_step, _lock_state, file_name): - index = get_index(file_name) - assert index != -1 +@step(u'I upload an asset$') +def upload_an_asset(step): + step.given('I upload the file "asset.html"') + + +@step(u'I (lock|unlock) the asset$') +def lock_unlock_file(_step, _lock_state): + index = get_index('asset.html') + assert index != -1, 'Expected to find an asset but could not.' + + # Warning: this is a misnomer, it really only toggles the + # lock state. TODO: fix it. lock_css = "input.lock-checkbox" world.css_find(lock_css)[index].click() -@step(u'Then "([^"]*)" is (locked|unlocked)$') -def verify_lock_unlock_file(_step, file_name, lock_state): - index = get_index(file_name) - assert index != -1 +@step(u'the user "([^"]*)" is enrolled in the course$') +def user_foo_is_enrolled_in_the_course(step, name): + world.create_user(name, 'test') + user = User.objects.get(username=name) + + course_id = world.scenario_dict['COURSE'].location.course_id + CourseEnrollment.enroll(user, course_id) + + +@step(u'Then the asset is (locked|unlocked)$') +def verify_lock_unlock_file(_step, lock_state): + index = get_index('asset.html') + assert index != -1, 'Expected to find an asset but could not.' lock_css = "input.lock-checkbox" checked = world.css_find(lock_css)[index]._element.get_attribute('checked') assert_equal(lock_state == "locked", bool(checked)) -@step(u'I have opened a course with a (locked|unlocked) asset "([^"]*)"$') -def open_course_with_locked(step, lock_state, file_name): +@step(u'I am at the files and upload page of a Studio course') +def at_upload_page(step): step.given('I have opened a new course in studio') step.given('I go to the files and uploads page') - _write_test_file(file_name, "test file") - step.given('I upload the file "' + file_name + '"') + + +@step(u'I have created a course with a (locked|unlocked) asset$') +def open_course_with_locked(step, lock_state): + step.given('I am at the files and upload page of a Studio course') + step.given('I upload the file "asset.html"') + if lock_state == "locked": - step.given('I lock "' + file_name + '"') + step.given('I lock the asset') step.given('I reload the page') -@step(u'Then the asset "([^"]*)" is (viewable|protected)$') -def view_asset(_step, file_name, status): - url = '/c4x/MITx/999/asset/' + file_name +@step(u'Then the asset is (viewable|protected)$') +def view_asset(_step, status): + url = django_url('/c4x/MITx/999/asset/asset.html') if status == 'viewable': - world.visit(url) - _verify_body_text() + expected_text = 'test file' else: - error_thrown = False - try: - world.visit(url) - except Exception as e: - assert e.status_code == 403 - error_thrown = True - assert error_thrown + expected_text = 'Unauthorized' - -@step(u'Then the asset "([^"]*)" can be clicked from the asset index$') -def click_asset_from_index(step, file_name): - # This is not ideal, but I'm having trouble with the middleware not having - # the same user in the request when I hit the URL directly. - course_link_css = 'a.course-link' - world.css_click(course_link_css) - step.given("I go to the files and uploads page") - index = get_index(file_name) - assert index != -1 - world.css_click('a.filename', index=index) - _verify_body_text() - - -def _verify_body_text(): - def verify_text(driver): - return world.css_text('body') == 'test file' - - world.wait_for(verify_text) + # Note that world.visit would trigger a 403 error instead of displaying "Unauthorized" + # Instead, we can drop back into the selenium driver get command. + world.browser.driver.get(url) + assert_equal(world.css_text('body'),expected_text) @step('I see a confirmation that the file was deleted$') diff --git a/common/djangoapps/terrain/course_helpers.py b/common/djangoapps/terrain/course_helpers.py index fc01d25d66..22222d30a4 100644 --- a/common/djangoapps/terrain/course_helpers.py +++ b/common/djangoapps/terrain/course_helpers.py @@ -2,17 +2,10 @@ # pylint: disable=W0621 from lettuce import world -from .factories import * -from django.conf import settings -from django.http import HttpRequest from django.contrib.auth.models import User -from django.contrib.auth import authenticate, login -from django.contrib.auth.middleware import AuthenticationMiddleware -from django.contrib.sessions.middleware import SessionMiddleware from student.models import CourseEnrollment from xmodule.modulestore.django import editable_modulestore from xmodule.contentstore.django import contentstore -from urllib import quote_plus @world.absorb @@ -22,7 +15,7 @@ def create_user(uname, password): if len(User.objects.filter(username=uname)) > 0: return - portal_user = UserFactory.build(username=uname, email=uname + '@edx.org') + portal_user = world.UserFactory.build(username=uname, email=uname + '@edx.org') portal_user.set_password(password) portal_user.save() @@ -30,7 +23,7 @@ def create_user(uname, password): registration.register(portal_user) registration.activate() - user_profile = world.UserProfileFactory(user=portal_user) + world.UserProfileFactory(user=portal_user) @world.absorb diff --git a/common/test/data/uploads/.gitignore b/common/test/data/uploads/.gitignore new file mode 100644 index 0000000000..a85ef7b7f3 --- /dev/null +++ b/common/test/data/uploads/.gitignore @@ -0,0 +1,3 @@ +test +test2 +asset.html diff --git a/common/test/data/uploads/test b/common/test/data/uploads/test deleted file mode 100644 index 588e9fb125..0000000000 --- a/common/test/data/uploads/test +++ /dev/null @@ -1 +0,0 @@ -This is an arbitrary file for testing uploads \ No newline at end of file From dfe45d5bd6bbe2ff0b8393d4d8dce7cf4a4f0375 Mon Sep 17 00:00:00 2001 From: jmclaus Date: Fri, 27 Sep 2013 19:21:43 +0200 Subject: [PATCH 45/54] First pass at ARIA. Video player buttons announce themselves as button when used with a screen reader --- common/lib/xmodule/xmodule/js/src/video/04_video_control.js | 4 ++++ .../xmodule/xmodule/js/src/video/05_video_quality_control.js | 3 +++ .../xmodule/xmodule/js/src/video/07_video_volume_control.js | 3 +++ .../xmodule/xmodule/js/src/video/08_video_speed_control.js | 3 +++ common/lib/xmodule/xmodule/js/src/video/09_video_caption.js | 3 +++ 5 files changed, 16 insertions(+) diff --git a/common/lib/xmodule/xmodule/js/src/video/04_video_control.js b/common/lib/xmodule/xmodule/js/src/video/04_video_control.js index 5cdb5c7536..1e1450dc54 100644 --- a/common/lib/xmodule/xmodule/js/src/video/04_video_control.js +++ b/common/lib/xmodule/xmodule/js/src/video/04_video_control.js @@ -63,6 +63,10 @@ function () { state.videoControl.el.addClass('html5'); state.controlHideTimeout = setTimeout(state.videoControl.hideControls, state.videoControl.fadeOutTimeout); } + // ARIA + // Let screen readers know these anchors behaves like a button. + state.videoControl.playPauseEl.attr('role', gettext('button')); + state.videoControl.fullScreenEl.attr('role', gettext('button')); } // function _bindHandlers(state) diff --git a/common/lib/xmodule/xmodule/js/src/video/05_video_quality_control.js b/common/lib/xmodule/xmodule/js/src/video/05_video_quality_control.js index cff103468d..e996ce5213 100644 --- a/common/lib/xmodule/xmodule/js/src/video/05_video_quality_control.js +++ b/common/lib/xmodule/xmodule/js/src/video/05_video_quality_control.js @@ -43,6 +43,9 @@ function () { state.videoQualityControl.el.show(); state.videoQualityControl.quality = null; + // ARIA + // Let screen readers know this anchor behaves like a button. + state.videoQualityControl.el.attr('role', gettext('button')); } // function _bindHandlers(state) diff --git a/common/lib/xmodule/xmodule/js/src/video/07_video_volume_control.js b/common/lib/xmodule/xmodule/js/src/video/07_video_volume_control.js index d8398ab530..3f31ddc75e 100644 --- a/common/lib/xmodule/xmodule/js/src/video/07_video_volume_control.js +++ b/common/lib/xmodule/xmodule/js/src/video/07_video_volume_control.js @@ -62,6 +62,9 @@ function () { }); state.videoVolumeControl.el.toggleClass('muted', state.videoVolumeControl.currentVolume === 0); + // ARIA + // Let screen readers know this anchor behaves like a button. + state.videoVolumeControl.buttonEl.attr('role', gettext('button')); } /** diff --git a/common/lib/xmodule/xmodule/js/src/video/08_video_speed_control.js b/common/lib/xmodule/xmodule/js/src/video/08_video_speed_control.js index 67f62edf95..4b9f532e43 100644 --- a/common/lib/xmodule/xmodule/js/src/video/08_video_speed_control.js +++ b/common/lib/xmodule/xmodule/js/src/video/08_video_speed_control.js @@ -79,6 +79,9 @@ function () { }); state.videoSpeedControl.setSpeed(state.speed); + // ARIA + // Let screen readers know this anchor behaves like a button. + state.videoSpeedControl.el.children('a').attr('role', gettext('button')); } /** diff --git a/common/lib/xmodule/xmodule/js/src/video/09_video_caption.js b/common/lib/xmodule/xmodule/js/src/video/09_video_caption.js index 825ed7b935..57bb2a38d1 100644 --- a/common/lib/xmodule/xmodule/js/src/video/09_video_caption.js +++ b/common/lib/xmodule/xmodule/js/src/video/09_video_caption.js @@ -105,6 +105,9 @@ function () { this.videoCaption.hideCaptions(true); this.videoCaption.hideSubtitlesEl.hide(); } + // ARIA + // Let screen readers know this anchor behaves like a button. + this.videoCaption.hideSubtitlesEl.attr('role', gettext('button')); } // function bindHandlers() From ce976a80c81099eae898b66c94f037d2f520e7b0 Mon Sep 17 00:00:00 2001 From: jmclaus Date: Sat, 28 Sep 2013 02:04:36 +0200 Subject: [PATCH 46/54] All buttons have an ARIA role, name , and state --- .../xmodule/js/src/video/04_video_control.js | 13 ++++++++++++- .../js/src/video/05_video_quality_control.js | 11 ++++++++++- .../xmodule/js/src/video/07_video_volume_control.js | 11 ++++++++++- .../xmodule/js/src/video/08_video_speed_control.js | 11 ++++++++++- .../xmodule/js/src/video/09_video_caption.js | 13 +++++++++++-- lms/templates/video.html | 2 +- 6 files changed, 54 insertions(+), 7 deletions(-) diff --git a/common/lib/xmodule/xmodule/js/src/video/04_video_control.js b/common/lib/xmodule/xmodule/js/src/video/04_video_control.js index 1e1450dc54..f31ed1caee 100644 --- a/common/lib/xmodule/xmodule/js/src/video/04_video_control.js +++ b/common/lib/xmodule/xmodule/js/src/video/04_video_control.js @@ -63,10 +63,21 @@ function () { state.videoControl.el.addClass('html5'); state.controlHideTimeout = setTimeout(state.videoControl.hideControls, state.videoControl.fadeOutTimeout); } + // ARIA - // Let screen readers know these anchors behaves like a button. + // Let screen readers know that: + + // these anchors behaves like buttons state.videoControl.playPauseEl.attr('role', gettext('button')); state.videoControl.fullScreenEl.attr('role', gettext('button')); + + // what their names are: (title attribute are set in video.html template): + // Play, Pause + // Fill browser + + // what their states are: + state.videoControl.playPauseEl.attr('aria-disabled', 'false'); + state.videoControl.fullScreenEl.attr('aria-disabled', 'false'); } // function _bindHandlers(state) diff --git a/common/lib/xmodule/xmodule/js/src/video/05_video_quality_control.js b/common/lib/xmodule/xmodule/js/src/video/05_video_quality_control.js index e996ce5213..5788cd6291 100644 --- a/common/lib/xmodule/xmodule/js/src/video/05_video_quality_control.js +++ b/common/lib/xmodule/xmodule/js/src/video/05_video_quality_control.js @@ -43,9 +43,18 @@ function () { state.videoQualityControl.el.show(); state.videoQualityControl.quality = null; + // ARIA - // Let screen readers know this anchor behaves like a button. + // Let screen readers know that: + + // this anchor behaves like a button state.videoQualityControl.el.attr('role', gettext('button')); + + // what its name is: (title attribute is set in video.html template): + // HD + + // what its state is: + state.videoQualityControl.el.attr('aria-disabled', 'false'); } // function _bindHandlers(state) diff --git a/common/lib/xmodule/xmodule/js/src/video/07_video_volume_control.js b/common/lib/xmodule/xmodule/js/src/video/07_video_volume_control.js index 3f31ddc75e..9eda8eade7 100644 --- a/common/lib/xmodule/xmodule/js/src/video/07_video_volume_control.js +++ b/common/lib/xmodule/xmodule/js/src/video/07_video_volume_control.js @@ -62,9 +62,18 @@ function () { }); state.videoVolumeControl.el.toggleClass('muted', state.videoVolumeControl.currentVolume === 0); + // ARIA - // Let screen readers know this anchor behaves like a button. + // Let screen readers know that: + + // this anchor behaves like a button state.videoVolumeControl.buttonEl.attr('role', gettext('button')); + + // what its name is: (title attribute is set in video.html template): + state.videoVolumeControl.buttonEl.attr('aria-label', gettext('Volume')); + + // what its state is: + state.videoVolumeControl.buttonEl.attr('aria-disabled', 'false'); } /** diff --git a/common/lib/xmodule/xmodule/js/src/video/08_video_speed_control.js b/common/lib/xmodule/xmodule/js/src/video/08_video_speed_control.js index 4b9f532e43..0ec9a5a96f 100644 --- a/common/lib/xmodule/xmodule/js/src/video/08_video_speed_control.js +++ b/common/lib/xmodule/xmodule/js/src/video/08_video_speed_control.js @@ -79,9 +79,18 @@ function () { }); state.videoSpeedControl.setSpeed(state.speed); + // ARIA - // Let screen readers know this anchor behaves like a button. + // Let screen readers know that: + + // this anchor behaves like a button state.videoSpeedControl.el.children('a').attr('role', gettext('button')); + + // what its name is: (title attribute is set in video.html template): + state.videoSpeedControl.el.children('a').attr('aria-label', 'Speeds'); + + // what its state is: + state.videoSpeedControl.el.children('a').attr('aria-disabled', 'false'); } /** diff --git a/common/lib/xmodule/xmodule/js/src/video/09_video_caption.js b/common/lib/xmodule/xmodule/js/src/video/09_video_caption.js index 57bb2a38d1..d606245420 100644 --- a/common/lib/xmodule/xmodule/js/src/video/09_video_caption.js +++ b/common/lib/xmodule/xmodule/js/src/video/09_video_caption.js @@ -105,9 +105,18 @@ function () { this.videoCaption.hideCaptions(true); this.videoCaption.hideSubtitlesEl.hide(); } + // ARIA - // Let screen readers know this anchor behaves like a button. - this.videoCaption.hideSubtitlesEl.attr('role', gettext('button')); + // Let screen readers know that: + + // this anchor behaves like a button + this.videoCaption.hideSubtitlesEl.attr('role', gettext('button')); + // what its name is: + // what its name is: (title attribute is set in video.html template): + // Speeds + + // what its state is: + this.videoCaption.hideSubtitlesEl.attr('aria-disabled', 'false'); } // function bindHandlers() diff --git a/lms/templates/video.html b/lms/templates/video.html index 3d0b9bd936..1974a08e6e 100644 --- a/lms/templates/video.html +++ b/lms/templates/video.html @@ -66,7 +66,7 @@ ${_('Fill browser')} ${_('HD')} - ${_('Captions')} + ${_('Turn off captions')} From a9b2f2dea0e80d48592e362a22250b45730c9b64 Mon Sep 17 00:00:00 2001 From: jmclaus Date: Sun, 29 Sep 2013 01:50:34 +0200 Subject: [PATCH 47/54] Volume slider announces its name and value to screen readers --- .../xmodule/xmodule/css/video/display.scss | 1 + .../xmodule/js/src/video/04_video_control.js | 3 ++ .../js/src/video/07_video_volume_control.js | 50 +++++++++++++++++++ 3 files changed, 54 insertions(+) diff --git a/common/lib/xmodule/xmodule/css/video/display.scss b/common/lib/xmodule/xmodule/css/video/display.scss index c087d18098..a0b514ab0f 100644 --- a/common/lib/xmodule/xmodule/css/video/display.scss +++ b/common/lib/xmodule/xmodule/css/video/display.scss @@ -109,6 +109,7 @@ div.video { -webkit-transition: -webkit-transform 0.7s ease-in-out; -moz-transition: -moz-transform 0.7s ease-in-out; -ms-transition: -ms-transform 0.7s ease-in-out; + tabindex: -1; transition: transform 0.7s ease-in-out; @include transform(scaleY(0.5) translate3d(0, 50%, 0)); diff --git a/common/lib/xmodule/xmodule/js/src/video/04_video_control.js b/common/lib/xmodule/xmodule/js/src/video/04_video_control.js index f31ed1caee..5272a3fb64 100644 --- a/common/lib/xmodule/xmodule/js/src/video/04_video_control.js +++ b/common/lib/xmodule/xmodule/js/src/video/04_video_control.js @@ -70,10 +70,13 @@ function () { // these anchors behaves like buttons state.videoControl.playPauseEl.attr('role', gettext('button')); state.videoControl.fullScreenEl.attr('role', gettext('button')); + // and this one as a slider + state.videoControl.sliderEl.find('.ui-slider-handle').attr('role', gettext('slider')); // what their names are: (title attribute are set in video.html template): // Play, Pause // Fill browser + state.videoControl.sliderEl.find('.ui-slider-handle').attr('title', gettext('video slider')); // what their states are: state.videoControl.playPauseEl.attr('aria-disabled', 'false'); diff --git a/common/lib/xmodule/xmodule/js/src/video/07_video_volume_control.js b/common/lib/xmodule/xmodule/js/src/video/07_video_volume_control.js index 9eda8eade7..d7116f24dc 100644 --- a/common/lib/xmodule/xmodule/js/src/video/07_video_volume_control.js +++ b/common/lib/xmodule/xmodule/js/src/video/07_video_volume_control.js @@ -74,6 +74,18 @@ function () { // what its state is: state.videoVolumeControl.buttonEl.attr('aria-disabled', 'false'); + + // Volume slider + state.videoVolumeControl.volumeSliderHandleEl = state.videoVolumeControl.volumeSliderEl.find('.ui-slider-handle'); + state.videoVolumeControl.volumeSliderHandleEl.attr({ + 'role': gettext('slider'), + 'title': 'volume', + 'aria-disabled': 'false', + 'aria-valuetext': getVolumeDescription(state.videoVolumeControl.slider.slider('option', 'value')), + 'aria-valuenow': state.videoVolumeControl.slider.slider('option', 'value'), + 'aria-valuemin': state.videoVolumeControl.slider.slider('option', 'min'), + 'aria-valuemax': state.videoVolumeControl.slider.slider('option', 'max') + }); } /** @@ -159,6 +171,9 @@ function () { }); this.trigger('videoPlayer.onVolumeChange', ui.value); + // ARIA + this.videoVolumeControl.volumeSliderHandleEl.attr('aria-valuenow', ui.value); + this.videoVolumeControl.volumeSliderHandleEl.attr('aria-valuetext', getVolumeDescription(ui.value)); } function toggleMute(event) { @@ -167,8 +182,43 @@ function () { if (this.videoVolumeControl.currentVolume > 0) { this.videoVolumeControl.previousVolume = this.videoVolumeControl.currentVolume; this.videoVolumeControl.slider.slider('option', 'value', 0); + // ARIA + state.videoVolumeControl.volumeSliderHandleEl.attr({ + 'aria-valuetext': getVolumeDescription(0), + 'aria-valuenow': 0 + }); } else { this.videoVolumeControl.slider.slider('option', 'value', this.videoVolumeControl.previousVolume); + // ARIA + state.videoVolumeControl.volumeSliderHandleEl.attr({ + 'aria-valuetext': getVolumeDescription(this.videoVolumeControl.previousVolume), + 'aria-valuenow': this.videoVolumeControl.previousVolume + }); + } + } + + // ARIA + function getVolumeDescription(vol) { + if (vol === 0) { + return 'silent'; + } + else if (vol <= 20) { + return 'very low'; + } + else if (vol <= 40) { + return 'low'; + } + else if (vol <= 60) { + return 'average'; + } + else if (vol <= 80) { + return 'loud'; + } + else if (vol <= 99) { + return 'very loud'; + } + else { + return 'maximum'; } } From e5895adb2b7e11de8b4ae98b4a9175eb4d7b1a3c Mon Sep 17 00:00:00 2001 From: jmclaus Date: Mon, 30 Sep 2013 14:16:26 +0200 Subject: [PATCH 48/54] Progress slider announces its nabe and value to screen readers --- .../js/src/video/06_video_progress_slider.js | 68 +++++++++++++++++++ lms/templates/video.html | 2 +- 2 files changed, 69 insertions(+), 1 deletion(-) diff --git a/common/lib/xmodule/xmodule/js/src/video/06_video_progress_slider.js b/common/lib/xmodule/xmodule/js/src/video/06_video_progress_slider.js index b45494ca34..7741f6eadb 100644 --- a/common/lib/xmodule/xmodule/js/src/video/06_video_progress_slider.js +++ b/common/lib/xmodule/xmodule/js/src/video/06_video_progress_slider.js @@ -54,6 +54,19 @@ function () { function _buildHandle(state) { state.videoProgressSlider.handle = state.videoProgressSlider.el.find('.ui-slider-handle'); + + // ARIA + // Let screen readers know that this anchor behaves like a slider, is + // named 'video position' and give its state + state.videoProgressSlider.handle.attr({ + 'role': gettext('slider'), + 'title': 'video position', + 'aria-disabled': 'false', + 'aria-valuetext': getTimeDescription(state.videoProgressSlider.slider.slider('option', 'value')) + //'aria-valuenow': state.videoProgressSlider.slider.slider('option', 'value'), + //'aria-valuemin': state.videoProgressSlider.slider.slider('option', 'min'), + //'aria-valuemax': state.videoProgressSlider.slider.slider('option', 'max') + }); } // *************************************************************** @@ -74,6 +87,10 @@ function () { this.videoProgressSlider.frozen = true; this.trigger('videoPlayer.onSlideSeek', {'type': 'onSlideSeek', 'time': ui.value}); + + // ARIA + this.videoProgressSlider.handle.attr('aria-valuetext', + getTimeDescription(this.videoPlayer.currentTime)); } function onStop(event, ui) { @@ -83,6 +100,10 @@ function () { this.trigger('videoPlayer.onSlideSeek', {'type': 'onSlideSeek', 'time': ui.value}); + // ARIA + this.videoProgressSlider.handle.attr('aria-valuetext', + getTimeDescription(this.videoPlayer.currentTime)); + setTimeout(function() { _this.videoProgressSlider.frozen = false; }, 200); @@ -99,6 +120,53 @@ function () { } } + function getTimeDescription(time) { + var seconds = Math.floor(time), + minutes = Math.floor(seconds / 60), + hours = Math.floor(minutes / 60), + hrStr, minStr, secStr; + seconds = seconds % 60; + minutes = minutes % 60; + + hrStr = hours.toString(10); + minStr = minutes.toString(10); + secStr = seconds.toString(10); + + if (hours) { + hrStr += (hours < 2 ? ' hour ' : ' hours '); + if (minutes) { + minStr += (minutes < 2 ? ' minute ' : ' minutes '); + } + else { + minStr += ' 0 minutes '; + } + if (seconds) { + secStr += (seconds < 2 ? ' second ' : ' seconds '); + } + else { + secStr += ' 0 seconds '; + } + return hrStr + minStr + secStr; + } + else if (minutes) { + minStr += (minutes < 2 ? ' minute ' : ' minutes '); + if (seconds) { + secStr += (seconds < 2 ? ' second ' : ' seconds '); + } + else { + secStr += ' 0 seconds '; + } + return minStr + secStr; + } + else if (seconds) { + secStr += (seconds < 2 ? ' second ' : ' seconds '); + return secStr; + } + else { + return '0 seconds'; + } + } + }); }(RequireJS.requirejs, RequireJS.require, RequireJS.define)); diff --git a/lms/templates/video.html b/lms/templates/video.html index 1974a08e6e..8ddaa68049 100644 --- a/lms/templates/video.html +++ b/lms/templates/video.html @@ -42,7 +42,7 @@
      -
      +
      From 9d418755a681c6e520d36a23b0bade63f9e921b1 Mon Sep 17 00:00:00 2001 From: jmclaus Date: Wed, 2 Oct 2013 16:54:48 +0200 Subject: [PATCH 54/54] Added changes made to video.html to fixtures. Addressed latest PR comments --- .../xmodule/xmodule/js/fixtures/video.html | 12 +++++------ .../xmodule/js/fixtures/video_all.html | 12 +++++------ .../js/fixtures/video_yt_multiple.html | 12 +++++------ .../js/src/video/06_video_progress_slider.js | 20 +++++++------------ .../js/src/video/07_video_volume_control.js | 20 +++++++------------ lms/templates/video.html | 4 ++-- 6 files changed, 34 insertions(+), 46 deletions(-) diff --git a/common/lib/xmodule/xmodule/js/fixtures/video.html b/common/lib/xmodule/xmodule/js/fixtures/video.html index 410b5869f0..f607430ba0 100644 --- a/common/lib/xmodule/xmodule/js/fixtures/video.html +++ b/common/lib/xmodule/xmodule/js/fixtures/video.html @@ -26,26 +26,26 @@
        -
        • +
      • 0:00 / 0:00
      - +

      Speed
        - +
        - Fill Browser - HD - Captions + Fill Browser + HD + Captions
        diff --git a/common/lib/xmodule/xmodule/js/fixtures/video_all.html b/common/lib/xmodule/xmodule/js/fixtures/video_all.html index f155905282..57052bf65d 100644 --- a/common/lib/xmodule/xmodule/js/fixtures/video_all.html +++ b/common/lib/xmodule/xmodule/js/fixtures/video_all.html @@ -29,26 +29,26 @@
          -
          • +
        • 0:00 / 0:00
        - + >

        Speed
          - +
          - Fill Browser - HD - Captions + Fill Browser + HD + Captions
          diff --git a/common/lib/xmodule/xmodule/js/fixtures/video_yt_multiple.html b/common/lib/xmodule/xmodule/js/fixtures/video_yt_multiple.html index 834de10406..c6b40cdf16 100644 --- a/common/lib/xmodule/xmodule/js/fixtures/video_yt_multiple.html +++ b/common/lib/xmodule/xmodule/js/fixtures/video_yt_multiple.html @@ -26,26 +26,26 @@
            -
            • +
          • 0:00 / 0:00
          - +

          Speed
            - +
            - Fill Browser - HD - Captions + Fill Browser + HD + Captions
            diff --git a/common/lib/xmodule/xmodule/js/src/video/06_video_progress_slider.js b/common/lib/xmodule/xmodule/js/src/video/06_video_progress_slider.js index 5750a7574e..18fa7ee3ad 100644 --- a/common/lib/xmodule/xmodule/js/src/video/06_video_progress_slider.js +++ b/common/lib/xmodule/xmodule/js/src/video/06_video_progress_slider.js @@ -138,35 +138,29 @@ function () { hrStr += (hours < 2 ? ' hour ' : ' hours '); if (minutes) { minStr += (minutes < 2 ? ' minute ' : ' minutes '); - } - else { + } else { minStr += ' 0 minutes '; } if (seconds) { secStr += (seconds < 2 ? ' second ' : ' seconds '); - } - else { + } else { secStr += ' 0 seconds '; } return hrStr + minStr + secStr; - } - else if (minutes) { + } else if (minutes) { minStr += (minutes < 2 ? ' minute ' : ' minutes '); if (seconds) { secStr += (seconds < 2 ? ' second ' : ' seconds '); - } - else { + } else { secStr += ' 0 seconds '; } return minStr + secStr; - } - else if (seconds) { + } else if (seconds) { secStr += (seconds < 2 ? ' second ' : ' seconds '); return secStr; } - else { - return '0 seconds'; - } + + return '0 seconds'; } }); diff --git a/common/lib/xmodule/xmodule/js/src/video/07_video_volume_control.js b/common/lib/xmodule/xmodule/js/src/video/07_video_volume_control.js index a9c7aef269..ea813f3912 100644 --- a/common/lib/xmodule/xmodule/js/src/video/07_video_volume_control.js +++ b/common/lib/xmodule/xmodule/js/src/video/07_video_volume_control.js @@ -216,25 +216,19 @@ function () { function getVolumeDescription(vol) { if (vol === 0) { return 'muted'; - } - else if (vol <= 20) { + } else if (vol <= 20) { return 'very low'; - } - else if (vol <= 40) { + } else if (vol <= 40) { return 'low'; - } - else if (vol <= 60) { + } else if (vol <= 60) { return 'average'; - } - else if (vol <= 80) { + } else if (vol <= 80) { return 'loud'; - } - else if (vol <= 99) { + } else if (vol <= 99) { return 'very loud'; } - else { - return 'maximum'; - } + + return 'maximum'; } }); diff --git a/lms/templates/video.html b/lms/templates/video.html index 89d376090c..caf0aaa06f 100644 --- a/lms/templates/video.html +++ b/lms/templates/video.html @@ -51,14 +51,14 @@
            - +

            ${_('Speed')}
              - +