From 622eebc4736835a282726b98a94057f0b927aa4c Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Sun, 12 Aug 2012 16:21:38 -0400 Subject: [PATCH 1/4] Don't error on missing static files * Just log a warning and return a dummy url * May want smarter checking later (e.g. would be nice to tell staff what files are missing.) --- common/djangoapps/pipeline_mako/__init__.py | 9 ++++---- common/djangoapps/static_replace.py | 23 +++++++++++++++++++-- lms/djangoapps/courseware/courses.py | 10 ++++----- 3 files changed, 31 insertions(+), 11 deletions(-) diff --git a/common/djangoapps/pipeline_mako/__init__.py b/common/djangoapps/pipeline_mako/__init__.py index 4703b53e52..1cdc287e2e 100644 --- a/common/djangoapps/pipeline_mako/__init__.py +++ b/common/djangoapps/pipeline_mako/__init__.py @@ -1,10 +1,9 @@ -from staticfiles.storage import staticfiles_storage - from mitxmako.shortcuts import render_to_string from pipeline.conf import settings from pipeline.packager import Packager from pipeline.utils import guess_type +from static_replace import try_staticfiles_lookup def compressed_css(package_name): @@ -25,9 +24,11 @@ def compressed_css(package_name): def render_css(package, path): template_name = package.template_name or "mako/css.html" context = package.extra_context + + url = try_staticfiles_lookup(path) context.update({ 'type': guess_type(path, 'text/css'), - 'url': staticfiles_storage.url(path) + 'url': url, }) return render_to_string(template_name, context) @@ -58,7 +59,7 @@ def render_js(package, path): context = package.extra_context context.update({ 'type': guess_type(path, 'text/javascript'), - 'url': staticfiles_storage.url(path) + 'url': try_staticfiles_lookup(path) }) return render_to_string(template_name, context) diff --git a/common/djangoapps/static_replace.py b/common/djangoapps/static_replace.py index 73e473c412..ba3cfb302a 100644 --- a/common/djangoapps/static_replace.py +++ b/common/djangoapps/static_replace.py @@ -1,7 +1,25 @@ +import logging +import re + from staticfiles.storage import staticfiles_storage from staticfiles import finders from django.conf import settings -import re + +log = logging.getLogger(__name__) + +def try_staticfiles_lookup(path): + """ + Try to lookup a path in staticfiles_storage. If it fails, return + a dead link instead of raising an exception. + """ + try: + url = staticfiles_storage.url(path) + except Exception as err: + log.warning("staticfiles_storage couldn't find path {}: {}".format( + path, str(err))) + # Just return a dead link--don't kill everything. + url = "file_not_found" + return url def replace(static_url, prefix=None): @@ -22,7 +40,8 @@ def replace(static_url, prefix=None): if servable: return static_url.group(0) else: - url = staticfiles_storage.url(prefix + static_url.group('rest')) + # don't error if file can't be found + url = try_staticfiles_lookup(prefix + static_url.group('rest')) return "".join([quote, url, quote]) diff --git a/lms/djangoapps/courseware/courses.py b/lms/djangoapps/courseware/courses.py index 3e603a108d..c48b9b378a 100644 --- a/lms/djangoapps/courseware/courses.py +++ b/lms/djangoapps/courseware/courses.py @@ -10,8 +10,7 @@ from django.http import Http404 from xmodule.course_module import CourseDescriptor from xmodule.modulestore.django import modulestore from xmodule.modulestore.exceptions import ItemNotFoundError -from static_replace import replace_urls -from staticfiles.storage import staticfiles_storage +from static_replace import replace_urls, try_staticfiles_lookup log = logging.getLogger(__name__) @@ -46,9 +45,10 @@ def check_course(course_id, course_must_be_open=True, course_required=True): def course_image_url(course): - return staticfiles_storage.url(course.metadata['data_dir'] + - "/images/course_image.jpg") - + """Try to look up the image url for the course. If it's not found, + log an error and return the dead link""" + path = course.metadata['data_dir'] + "/images/course_image.jpg" + return try_staticfiles_lookup(path) def get_course_about_section(course, section_key): """ From d7f94a05b72f23b92c0174303e930fea9bf9f108 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Sun, 12 Aug 2012 16:23:31 -0400 Subject: [PATCH 2/4] Add DARK_LAUNCH functionality * pass user to check_course * if dark launch feature enabled, users with staff access to course can see courseware before start date. Students still can't. * tests. * Remaining: enrollment view has custom access control. Need to check it. --- lms/djangoapps/courseware/courses.py | 20 +++- lms/djangoapps/courseware/tests/tests.py | 143 +++++++++++++++++++++-- lms/djangoapps/courseware/views.py | 15 +-- lms/djangoapps/simplewiki/views.py | 24 ++-- lms/djangoapps/staticbook/views.py | 2 +- lms/envs/common.py | 1 + lms/urls.py | 10 +- 7 files changed, 178 insertions(+), 37 deletions(-) diff --git a/lms/djangoapps/courseware/courses.py b/lms/djangoapps/courseware/courses.py index c48b9b378a..fa9b6c8220 100644 --- a/lms/djangoapps/courseware/courses.py +++ b/lms/djangoapps/courseware/courses.py @@ -15,11 +15,11 @@ from static_replace import replace_urls, try_staticfiles_lookup log = logging.getLogger(__name__) -def check_course(course_id, course_must_be_open=True, course_required=True): +def check_course(user, course_id, course_must_be_open=True, course_required=True): """ - Given a course_id, this returns the course object. By default, - if the course is not found or the course is not open yet, this - method will raise a 404. + Given a django user and a course_id, this returns the course + object. By default, if the course is not found or the course is + not open yet, this method will raise a 404. If course_must_be_open is False, the course will be returned without a 404 even if it is not open. @@ -27,6 +27,10 @@ def check_course(course_id, course_must_be_open=True, course_required=True): If course_required is False, a course_id of None is acceptable. The course returned will be None. Even if the course is not required, if a course_id is given that does not exist a 404 will be raised. + + This behavior is modified by MITX_FEATURES['DARK_LAUNCH']: + if dark launch is enabled, course_must_be_open is ignored for + users that have staff access. """ course = None if course_required or course_id: @@ -38,7 +42,13 @@ def check_course(course_id, course_must_be_open=True, course_required=True): raise Http404("Course not found.") started = course.has_started() or settings.MITX_FEATURES['DISABLE_START_DATES'] - if course_must_be_open and not started: + + must_be_open = course_must_be_open + if (settings.MITX_FEATURES['DARK_LAUNCH'] and + has_staff_access_to_course(user, course)): + must_be_open = False + + if must_be_open and not started: raise Http404("This course has not yet started.") return course diff --git a/lms/djangoapps/courseware/tests/tests.py b/lms/djangoapps/courseware/tests/tests.py index da0688be3d..0fb5c9983e 100644 --- a/lms/djangoapps/courseware/tests/tests.py +++ b/lms/djangoapps/courseware/tests/tests.py @@ -1,11 +1,14 @@ import copy import json -from path import path import os +import sys +import time -from pprint import pprint from nose import SkipTest +from path import path +from pprint import pprint +from django.contrib.auth.models import User, Group from django.test import TestCase from django.test.client import Client from django.conf import settings @@ -13,12 +16,11 @@ from django.core.urlresolvers import reverse from mock import patch, Mock from override_settings import override_settings -from django.contrib.auth.models import User, Group +import xmodule.modulestore.django + from student.models import Registration from courseware.courses import course_staff_group_name - from xmodule.modulestore.django import modulestore -import xmodule.modulestore.django from xmodule.modulestore import Location from xmodule.modulestore.xml_importer import import_from_xml @@ -206,13 +208,14 @@ class TestCoursesLoadTestCase(PageLoader): @override_settings(MODULESTORE=TEST_DATA_MODULESTORE) -class TestInstructorAuth(PageLoader): - """Check that authentication works properly""" +class TestViewAuth(PageLoader): + """Check that view authentication works properly""" # NOTE: setUpClass() runs before override_settings takes effect, so # can't do imports there without manually hacking settings. def setUp(self): + print "sys.path: {}".format(sys.path) xmodule.modulestore.django._MODULESTORES = {} modulestore().collection.drop() import_from_xml(modulestore(), TEST_DATA_DIR, ['toy']) @@ -237,12 +240,16 @@ class TestInstructorAuth(PageLoader): # TODO (vshnayder): once we're returning 404s, get rid of this if. if code != 404: self.assertEqual(resp.status_code, code) + # And 'page not found' shouldn't be in the returned page + self.assertTrue(resp.content.lower().find('page not found') == -1) else: # look for "page not found" instead of the status code + #print resp.content self.assertTrue(resp.content.lower().find('page not found') != -1) - def test_instructor_page(self): - "Make sure only instructors can load it" + def test_instructor_pages(self): + """Make sure only instructors for the course or staff can load the instructor + dashboard, the grade views, and student profile pages""" # First, try with an enrolled student self.login(self.student, self.password) @@ -297,7 +304,125 @@ class TestInstructorAuth(PageLoader): self.check_for_get_code(200, url) + def test_dark_launch(self): + """Make sure that when dark launch is on, students can't access course + pages, but instructors can""" + # test.py turns off start dates, enable them and set them correctly. + # Because settings is global, be careful not to mess it up for other tests + # (Can't use override_settings because we're only changing part of the + # MITX_FEATURES dict) + oldDSD = settings.MITX_FEATURES['DISABLE_START_DATES'] + oldDL = settings.MITX_FEATURES['DARK_LAUNCH'] + + try: + settings.MITX_FEATURES['DISABLE_START_DATES'] = False + settings.MITX_FEATURES['DARK_LAUNCH'] = True + self._do_test_dark_launch() + finally: + settings.MITX_FEATURES['DISABLE_START_DATES'] = oldDSD + settings.MITX_FEATURES['DARK_LAUNCH'] = oldDL + + + def _do_test_dark_launch(self): + """Actually do the test, relying on settings to be right.""" + + # Make courses start in the future + tomorrow = time.time() + 24*3600 + self.toy.start = self.toy.metadata['start'] = time.gmtime(tomorrow) + self.full.start = self.full.metadata['start'] = time.gmtime(tomorrow) + + self.assertFalse(self.toy.has_started()) + self.assertFalse(self.full.has_started()) + self.assertFalse(settings.MITX_FEATURES['DISABLE_START_DATES']) + self.assertTrue(settings.MITX_FEATURES['DARK_LAUNCH']) + + def reverse_urls(names, course): + return [reverse(name, kwargs={'course_id': course.id}) for name in names] + + def dark_student_urls(course): + """ + list of urls that students should be able to see only + after launch, but staff should see before + """ + urls = reverse_urls(['info', 'book', 'courseware', 'profile'], course) + return urls + + def light_student_urls(course): + """ + list of urls that students should be able to see before + launch. + """ + urls = reverse_urls(['about_course'], course) + urls.append(reverse('courses')) + # Need separate test for change_enrollment, since it's a POST view + #urls.append(reverse('change_enrollment')) + + return urls + + def instructor_urls(course): + """list of urls that only instructors/staff should be able to see""" + urls = reverse_urls(['instructor_dashboard','gradebook','grade_summary'], + course) + urls.append(reverse('student_profile', kwargs={'course_id': course.id, + 'student_id': user(self.student).id})) + return urls + + def check_non_staff(course): + """Check that access is right for non-staff in course""" + print '=== Checking non-staff access for {}'.format(course.id) + for url in instructor_urls(course) + dark_student_urls(course): + print 'checking for 404 on {}'.format(url) + self.check_for_get_code(404, url) + + for url in light_student_urls(course): + print 'checking for 200 on {}'.format(url) + self.check_for_get_code(200, url) + + def check_staff(course): + """Check that access is right for staff in course""" + print '=== Checking staff access for {}'.format(course.id) + for url in (instructor_urls(course) + + dark_student_urls(course) + + light_student_urls(course)): + print 'checking for 200 on {}'.format(url) + self.check_for_get_code(200, url) + + # First, try with an enrolled student + print '=== Testing student access....' + self.login(self.student, self.password) + self.enroll(self.toy) + self.enroll(self.full) + + # shouldn't be able to get to anything except the light pages + check_non_staff(self.toy) + check_non_staff(self.full) + + print '=== Testing course instructor access....' + # Make the instructor staff in the toy course + group_name = course_staff_group_name(self.toy) + g = Group.objects.create(name=group_name) + g.user_set.add(user(self.instructor)) + + self.logout() + self.login(self.instructor, self.password) + # Enroll in the classes---can't see courseware otherwise. + self.enroll(self.toy) + self.enroll(self.full) + + # should now be able to get to everything for toy course + check_non_staff(self.full) + check_staff(self.toy) + + print '=== Testing staff access....' + # now also make the instructor staff + u = user(self.instructor) + u.is_staff = True + u.save() + + # and now should be able to load both + check_staff(self.toy) + check_staff(self.full) @override_settings(MODULESTORE=REAL_DATA_MODULESTORE) diff --git a/lms/djangoapps/courseware/views.py b/lms/djangoapps/courseware/views.py index 02e6d00a58..00ab45e605 100644 --- a/lms/djangoapps/courseware/views.py +++ b/lms/djangoapps/courseware/views.py @@ -110,9 +110,10 @@ def index(request, course_id, chapter=None, section=None, - HTTPresponse ''' - course = check_course(course_id) + course = check_course(request.user, course_id) registered = registered_for_course(course, request.user) if not registered: + # TODO (vshnayder): do course instructors need to be registered to see course? log.debug('User %s tried to view course %s but is not enrolled' % (request.user,course.location.url())) return redirect(reverse('about_course', args=[course.id])) @@ -203,7 +204,7 @@ def course_info(request, course_id): Assumes the course_id is in a valid format. """ - course = check_course(course_id) + course = check_course(request.user, course_id) return render_to_response('info.html', {'course': course}) @@ -220,7 +221,7 @@ def registered_for_course(course, user): @ensure_csrf_cookie @cache_if_anonymous def course_about(request, course_id): - course = check_course(course_id, course_must_be_open=False) + course = check_course(request.user, course_id, course_must_be_open=False) registered = registered_for_course(course, request.user) return render_to_response('portal/course_about.html', {'course': course, 'registered': registered}) @@ -252,7 +253,7 @@ def profile(request, course_id, student_id=None): Course staff are allowed to see the profiles of students in their class. """ - course = check_course(course_id) + course = check_course(request.user, course_id) if student_id is None or student_id == request.user.id: # always allowed to see your own profile @@ -299,7 +300,7 @@ def gradebook(request, course_id): if not has_staff_access_to_course_id(request.user, course_id): raise Http404 - course = check_course(course_id) + course = check_course(request.user, course_id) enrolled_students = User.objects.filter(courseenrollment__course_id=course_id).order_by('username') @@ -324,7 +325,7 @@ def grade_summary(request, course_id): if not has_staff_access_to_course_id(request.user, course_id): raise Http404 - course = check_course(course_id) + course = check_course(request.user, course_id) # For now, just a static page context = {'course': course } @@ -337,7 +338,7 @@ def instructor_dashboard(request, course_id): if not has_staff_access_to_course_id(request.user, course_id): raise Http404 - course = check_course(course_id) + course = check_course(request.user, course_id) # For now, just a static page context = {'course': course } diff --git a/lms/djangoapps/simplewiki/views.py b/lms/djangoapps/simplewiki/views.py index 77fadc49ba..a6bb192fd7 100644 --- a/lms/djangoapps/simplewiki/views.py +++ b/lms/djangoapps/simplewiki/views.py @@ -51,7 +51,7 @@ def update_template_dictionary(dictionary, request=None, course=None, article=No def view(request, article_path, course_id=None): - course = check_course(course_id, course_required=False) + course = check_course(request.user, course_id, course_required=False) (article, err) = get_article(request, article_path, course) if err: @@ -67,7 +67,7 @@ def view(request, article_path, course_id=None): def view_revision(request, revision_number, article_path, course_id=None): - course = check_course(course_id, course_required=False) + course = check_course(request.user, course_id, course_required=False) (article, err) = get_article(request, article_path, course) if err: @@ -91,7 +91,7 @@ def view_revision(request, revision_number, article_path, course_id=None): def root_redirect(request, course_id=None): - course = check_course(course_id, course_required=False) + course = check_course(request.user, course_id, course_required=False) #TODO: Add a default namespace to settings. namespace = course.wiki_namespace if course else "edX" @@ -109,7 +109,7 @@ def root_redirect(request, course_id=None): def create(request, article_path, course_id=None): - course = check_course(course_id, course_required=False) + course = check_course(request.user, course_id, course_required=False) article_path_components = article_path.split('/') @@ -170,7 +170,7 @@ def create(request, article_path, course_id=None): def edit(request, article_path, course_id=None): - course = check_course(course_id, course_required=False) + course = check_course(request.user, course_id, course_required=False) (article, err) = get_article(request, article_path, course) if err: @@ -218,7 +218,7 @@ def edit(request, article_path, course_id=None): def history(request, article_path, page=1, course_id=None): - course = check_course(course_id, course_required=False) + course = check_course(request.user, course_id, course_required=False) (article, err) = get_article(request, article_path, course) if err: @@ -300,7 +300,7 @@ def history(request, article_path, page=1, course_id=None): def revision_feed(request, page=1, namespace=None, course_id=None): - course = check_course(course_id, course_required=False) + course = check_course(request.user, course_id, course_required=False) page_size = 10 @@ -333,7 +333,7 @@ def revision_feed(request, page=1, namespace=None, course_id=None): def search_articles(request, namespace=None, course_id=None): - course = check_course(course_id, course_required=False) + course = check_course(request.user, course_id, course_required=False) # blampe: We should check for the presence of other popular django search # apps and use those if possible. Only fall back on this as a last resort. @@ -382,7 +382,7 @@ def search_articles(request, namespace=None, course_id=None): def search_add_related(request, course_id, slug, namespace): - course = check_course(course_id, course_required=False) + course = check_course(request.user, course_id, course_required=False) (article, err) = get_article(request, slug, namespace if namespace else course_id) if err: @@ -415,7 +415,7 @@ def search_add_related(request, course_id, slug, namespace): def add_related(request, course_id, slug, namespace): - course = check_course(course_id, course_required=False) + course = check_course(request.user, course_id, course_required=False) (article, err) = get_article(request, slug, namespace if namespace else course_id) if err: @@ -439,7 +439,7 @@ def add_related(request, course_id, slug, namespace): def remove_related(request, course_id, namespace, slug, related_id): - course = check_course(course_id, course_required=False) + course = check_course(request.user, course_id, course_required=False) (article, err) = get_article(request, slug, namespace if namespace else course_id) @@ -462,7 +462,7 @@ def remove_related(request, course_id, namespace, slug, related_id): def random_article(request, course_id=None): - course = check_course(course_id, course_required=False) + course = check_course(request.user, course_id, course_required=False) from random import randint num_arts = Article.objects.count() diff --git a/lms/djangoapps/staticbook/views.py b/lms/djangoapps/staticbook/views.py index cb15f5855e..e63619756c 100644 --- a/lms/djangoapps/staticbook/views.py +++ b/lms/djangoapps/staticbook/views.py @@ -6,7 +6,7 @@ from lxml import etree @login_required def index(request, course_id, page=0): - course = check_course(course_id) + course = check_course(request.user, course_id) raw_table_of_contents = open('lms/templates/book_toc.xml', 'r') # TODO: This will need to come from S3 table_of_contents = etree.parse(raw_table_of_contents).getroot() return render_to_response('staticbook.html', {'page': int(page), 'course': course, 'table_of_contents': table_of_contents}) diff --git a/lms/envs/common.py b/lms/envs/common.py index 487e2efe48..9772b98fc3 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -48,6 +48,7 @@ MITX_FEATURES = { ## DO NOT SET TO True IN THIS FILE ## Doing so will cause all courses to be released on production 'DISABLE_START_DATES': False, # When True, all courses will be active, regardless of start date + 'DARK_LAUNCH': False, # When True, courses will be active for staff only 'ENABLE_TEXTBOOK' : True, 'ENABLE_DISCUSSION' : True, diff --git a/lms/urls.py b/lms/urls.py index 4f1895e2c4..468ee8e62c 100644 --- a/lms/urls.py +++ b/lms/urls.py @@ -97,12 +97,16 @@ if settings.PERFSTATS: if settings.COURSEWARE_ENABLED: urlpatterns += ( + # Hook django-masquerade, allowing staff to view site as other users url(r'^masquerade/', include('masquerade.urls')), url(r'^jump_to/(?P.*)$', 'courseware.views.jump_to', name="jump_to"), - url(r'^modx/(?P.*?)/(?P[^/]*)$', 'courseware.module_render.modx_dispatch'), #reset_problem'), - url(r'^xqueue/(?P[^/]*)/(?P.*?)/(?P[^/]*)$', 'courseware.module_render.xqueue_callback'), - url(r'^change_setting$', 'student.views.change_setting'), + url(r'^modx/(?P.*?)/(?P[^/]*)$', + 'courseware.module_render.modx_dispatch', name='modx_dispatch'), + url(r'^xqueue/(?P[^/]*)/(?P.*?)/(?P[^/]*)$', + 'courseware.module_render.xqueue_callback', name='xqueue_callback'), + url(r'^change_setting$', 'student.views.change_setting', + name='change_setting'), # TODO: These views need to be updated before they work # url(r'^calculate$', 'util.views.calculate'), From e980f8b296565e0eb46ac9d883a135f1b45817c8 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Sun, 12 Aug 2012 16:59:07 -0400 Subject: [PATCH 3/4] add notes on how to run individual tests --- doc/development.md | 28 +++++++++++++++++++++++++--- 1 file changed, 25 insertions(+), 3 deletions(-) diff --git a/doc/development.md b/doc/development.md index 9d6628732a..44965cb0de 100644 --- a/doc/development.md +++ b/doc/development.md @@ -34,12 +34,34 @@ This will import all courses in your data directory into mongodb This runs all the tests (long, uses collectstatic): rake test - + +If if you aren't changing static files, can run `rake test` once, then run + + rake fasttest_{lms,cms} + xmodule can be tested independently, with this: rake test_common/lib/xmodule - + To see all available rake commands, do this: rake -T - \ No newline at end of file + +To run a single django test class: + + django-admin.py test --settings=lms.envs.test --pythonpath=. lms/djangoapps/courseware/tests/tests.py:TestViewAuth + +To run a single django test: + + django-admin.py test --settings=lms.envs.test --pythonpath=. lms/djangoapps/courseware/tests/tests.py:TestViewAuth.test_dark_launch + + +To run a single nose test file: + + nosetests common/lib/xmodule/xmodule/tests/test_stringify.py + +To run a single nose test: + + nosetests common/lib/xmodule/xmodule/tests/test_stringify.py:test_stringify + + From af7e70a9791c11c131ffde31a64c056e0a60f057 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Sun, 12 Aug 2012 17:21:40 -0400 Subject: [PATCH 4/4] Log content problems as warnings, not errors * will avoid newrelic complaining * NOTE: Is this what we want post-ship? - need some way of notifying instructors of problems --- common/djangoapps/student/views.py | 4 ++-- common/lib/capa/capa/capa_problem.py | 16 ++++++++-------- common/lib/capa/capa/inputtypes.py | 14 +++++++------- common/lib/xmodule/xmodule/capa_module.py | 2 +- common/lib/xmodule/xmodule/modulestore/xml.py | 9 +++++---- common/lib/xmodule/xmodule/x_module.py | 9 +++++---- 6 files changed, 28 insertions(+), 26 deletions(-) diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index 8093a5a51a..ab22cc0d82 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -128,7 +128,7 @@ def dashboard(request): try: courses.append(course_from_id(enrollment.course_id)) except ItemNotFoundError: - log.error("User {0} enrolled in non-existant course {1}" + log.error("User {0} enrolled in non-existent course {1}" .format(user.username, enrollment.course_id)) message = "" @@ -182,7 +182,7 @@ def change_enrollment(request): try: course = course_from_id(course_id) except ItemNotFoundError: - log.error("User {0} tried to enroll in non-existant course {1}" + log.warning("User {0} tried to enroll in non-existant course {1}" .format(user.username, enrollment.course_id)) return {'success': False, 'error': 'The course requested does not exist.'} diff --git a/common/lib/capa/capa/capa_problem.py b/common/lib/capa/capa/capa_problem.py index 92823667e7..d1798f2c67 100644 --- a/common/lib/capa/capa/capa_problem.py +++ b/common/lib/capa/capa/capa_problem.py @@ -229,14 +229,14 @@ class LoncapaProblem(object): Calls the Response for each question in this problem, to do the actual grading. ''' - + self.student_answers = convert_files_to_filenames(answers) - + oldcmap = self.correct_map # old CorrectMap newcmap = CorrectMap() # start new with empty CorrectMap # log.debug('Responders: %s' % self.responders) for responder in self.responders.values(): # Call each responsetype instance to do actual grading - if 'filesubmission' in responder.allowed_inputfields: # File objects are passed only if responsetype + if 'filesubmission' in responder.allowed_inputfields: # File objects are passed only if responsetype # explicitly allows for file submissions results = responder.evaluate_answers(answers, oldcmap) else: @@ -295,9 +295,9 @@ class LoncapaProblem(object): try: ifp = self.system.filestore.open(file) # open using ModuleSystem OSFS filestore except Exception as err: - log.error('Error %s in problem xml include: %s' % ( + log.warning('Error %s in problem xml include: %s' % ( err, etree.tostring(inc, pretty_print=True))) - log.error('Cannot find file %s in %s' % ( + log.warning('Cannot find file %s in %s' % ( file, self.system.filestore)) # if debugging, don't fail - just log error # TODO (vshnayder): need real error handling, display to users @@ -306,11 +306,11 @@ class LoncapaProblem(object): else: continue try: - incxml = etree.XML(ifp.read()) # read in and convert to XML + incxml = etree.XML(ifp.read()) # read in and convert to XML except Exception as err: - log.error('Error %s in problem xml include: %s' % ( + log.warning('Error %s in problem xml include: %s' % ( err, etree.tostring(inc, pretty_print=True))) - log.error('Cannot parse XML in %s' % (file)) + log.warning('Cannot parse XML in %s' % (file)) # if debugging, don't fail - just log error # TODO (vshnayder): same as above if not self.system.get('DEBUG'): diff --git a/common/lib/capa/capa/inputtypes.py b/common/lib/capa/capa/inputtypes.py index 0c47892598..c3acbdda91 100644 --- a/common/lib/capa/capa/inputtypes.py +++ b/common/lib/capa/capa/inputtypes.py @@ -313,13 +313,13 @@ def filesubmission(element, value, status, render_template, msg=''): if status == 'incomplete': # Flag indicating that the problem has been queued, 'msg' is length of queue status = 'queued' queue_len = msg - msg = 'Submitted to grader. (Queue length: %s)' % queue_len + msg = 'Submitted to grader. (Queue length: %s)' % queue_len - context = { 'id': eid, 'state': status, 'msg': msg, 'value': value, + context = { 'id': eid, 'state': status, 'msg': msg, 'value': value, 'queue_len': queue_len } html = render_template("filesubmission.html", context) - return etree.XML(html) + return etree.XML(html) #----------------------------------------------------------------------------- @@ -339,16 +339,16 @@ def textbox(element, value, status, render_template, msg=''): hidden = element.get('hidden', '') # if specified, then textline is hidden and id is stored in div of name given by hidden if not value: value = element.text # if no student input yet, then use the default input given by the problem - + # Check if problem has been queued queue_len = 0 if status == 'incomplete': # Flag indicating that the problem has been queued, 'msg' is length of queue - status = 'queued' + status = 'queued' queue_len = msg - msg = 'Submitted to grader. (Queue length: %s)' % queue_len + msg = 'Submitted to grader. (Queue length: %s)' % queue_len # For CodeMirror - mode = element.get('mode','python') + mode = element.get('mode','python') linenumbers = element.get('linenumbers','true') tabsize = element.get('tabsize','4') tabsize = int(tabsize) diff --git a/common/lib/xmodule/xmodule/capa_module.py b/common/lib/xmodule/xmodule/capa_module.py index eb083e97db..f12b1c2be4 100644 --- a/common/lib/xmodule/xmodule/capa_module.py +++ b/common/lib/xmodule/xmodule/capa_module.py @@ -150,7 +150,7 @@ class CapaModule(XModule): # TODO (vshnayder): do modules need error handlers too? # We shouldn't be switching on DEBUG. if self.system.DEBUG: - log.error(msg) + log.warning(msg) # TODO (vshnayder): This logic should be general, not here--and may # want to preserve the data instead of replacing it. # e.g. in the CMS diff --git a/common/lib/xmodule/xmodule/modulestore/xml.py b/common/lib/xmodule/xmodule/modulestore/xml.py index 2dc3b33323..8c4c373d4f 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml.py +++ b/common/lib/xmodule/xmodule/modulestore/xml.py @@ -50,8 +50,9 @@ class ImportSystem(XMLParsingSystem, MakoDescriptorSystem): # have been imported into the cms from xml xml = clean_out_mako_templating(xml) xml_data = etree.fromstring(xml) - except: - log.exception("Unable to parse xml: {xml}".format(xml=xml)) + except Exception as err: + log.warning("Unable to parse xml: {err}, xml: {xml}".format( + err=str(err), xml=xml)) raise # VS[compat]. Take this out once course conversion is done @@ -194,7 +195,7 @@ class XMLModuleStore(ModuleStoreBase): if org is None: msg = ("No 'org' attribute set for course in {dir}. " "Using default 'edx'".format(dir=course_dir)) - log.error(msg) + log.warning(msg) tracker(msg) org = 'edx' @@ -206,7 +207,7 @@ class XMLModuleStore(ModuleStoreBase): dir=course_dir, default=course_dir )) - log.error(msg) + log.warning(msg) tracker(msg) course = course_dir diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index 2cd51b9e6e..3b89f29989 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -522,7 +522,7 @@ class XModuleDescriptor(Plugin, HTMLSnippet): # Put import here to avoid circular import errors from xmodule.error_module import ErrorDescriptor msg = "Error loading from xml." - log.exception(msg) + log.warning(msg + " " + str(err)) system.error_tracker(msg) err_msg = msg + "\n" + exc_info_to_str(sys.exc_info()) descriptor = ErrorDescriptor.from_xml(xml_data, system, org, course, @@ -615,9 +615,10 @@ class DescriptorSystem(object): try: x = access_some_resource() check_some_format(x) - except SomeProblem: - msg = 'Grommet {0} is broken'.format(x) - log.exception(msg) # don't rely on handler to log + except SomeProblem as err: + msg = 'Grommet {0} is broken: {1}'.format(x, str(err)) + log.warning(msg) # don't rely on tracker to log + # NOTE: we generally don't want content errors logged as errors self.system.error_tracker(msg) # work around return 'Oops, couldn't load grommet'