From eeadf0ba87daaf70299847d8f178dddd78fedf56 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Mon, 20 Aug 2012 19:28:07 -0400 Subject: [PATCH 1/5] Let policies be stored in policies/{url_name}/policy.json * still backcompat with old mode --- common/lib/xmodule/xmodule/modulestore/xml.py | 9 +++++++-- .../{TT_2012_Fall.json => TT_2012_Fall/policy.json} | 0 2 files changed, 7 insertions(+), 2 deletions(-) rename common/test/data/two_toys/policies/{TT_2012_Fall.json => TT_2012_Fall/policy.json} (100%) diff --git a/common/lib/xmodule/xmodule/modulestore/xml.py b/common/lib/xmodule/xmodule/modulestore/xml.py index e0fecb243d..769d110518 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml.py +++ b/common/lib/xmodule/xmodule/modulestore/xml.py @@ -243,8 +243,13 @@ class XMLModuleStore(ModuleStoreBase): url_name = course_data.get('url_name', course_data.get('slug')) if url_name: - policy_path = self.data_dir / course_dir / 'policies' / '{0}.json'.format(url_name) - policy = self.load_policy(policy_path, tracker) + old_policy_path = self.data_dir / course_dir / 'policies' / url_name / 'policy.json' + policy = self.load_policy(old_policy_path, tracker) + + # VS[compat]: remove once courses use the policy dirs. + if policy == {}: + old_policy_path = self.data_dir / course_dir / 'policies' / '{0}.json'.format(url_name) + policy = self.load_policy(old_policy_path, tracker) else: policy = {} # VS[compat] : 'name' is deprecated, but support it for now... diff --git a/common/test/data/two_toys/policies/TT_2012_Fall.json b/common/test/data/two_toys/policies/TT_2012_Fall/policy.json similarity index 100% rename from common/test/data/two_toys/policies/TT_2012_Fall.json rename to common/test/data/two_toys/policies/TT_2012_Fall/policy.json From f866854411496b08f004199dbe68642a5be6ccbb Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Mon, 20 Aug 2012 20:00:32 -0400 Subject: [PATCH 2/5] Load grading policy from policy/{url_name}/grading_policy.json * with backcompat location /grading_policy.json --- common/lib/xmodule/xmodule/course_module.py | 33 +++++++-------- common/lib/xmodule/xmodule/graders.py | 16 ++++---- common/lib/xmodule/xmodule/modulestore/xml.py | 41 +++++++++++++++++-- .../lib/xmodule/xmodule/tests/test_import.py | 3 ++ .../policies/TT_2012_Fall/grading_policy.json | 35 ++++++++++++++++ 5 files changed, 98 insertions(+), 30 deletions(-) create mode 100644 common/test/data/two_toys/policies/TT_2012_Fall/grading_policy.json diff --git a/common/lib/xmodule/xmodule/course_module.py b/common/lib/xmodule/xmodule/course_module.py index a9f29b9a13..fb0721454c 100644 --- a/common/lib/xmodule/xmodule/course_module.py +++ b/common/lib/xmodule/xmodule/course_module.py @@ -18,7 +18,7 @@ class CourseDescriptor(SequenceDescriptor): class Textbook: def __init__(self, title, book_url): self.title = title - self.book_url = book_url + self.book_url = book_url self.table_of_contents = self._get_toc_from_s3() @classmethod @@ -30,11 +30,11 @@ class CourseDescriptor(SequenceDescriptor): return self.table_of_contents def _get_toc_from_s3(self): - ''' + """ Accesses the textbook's table of contents (default name "toc.xml") at the URL self.book_url Returns XML tree representation of the table of contents - ''' + """ toc_url = self.book_url + 'toc.xml' # Get the table of contents from S3 @@ -72,6 +72,15 @@ class CourseDescriptor(SequenceDescriptor): self.enrollment_start = self._try_parse_time("enrollment_start") self.enrollment_end = self._try_parse_time("enrollment_end") + # NOTE: relies on the modulestore to call set_grading_policy() right after + # init. (Modulestore is in charge of figuring out where to load the policy from) + + + def set_grading_policy(self, policy_str): + """Parse the policy specified in policy_str, and save it""" + self._grading_policy = load_grading_policy(policy_str) + + @classmethod def definition_from_xml(cls, xml_object, system): textbooks = [] @@ -87,25 +96,11 @@ class CourseDescriptor(SequenceDescriptor): @property def grader(self): - return self.__grading_policy['GRADER'] + return self._grading_policy['GRADER'] @property def grade_cutoffs(self): - return self.__grading_policy['GRADE_CUTOFFS'] - - @lazyproperty - def __grading_policy(self): - policy_string = "" - - try: - with self.system.resources_fs.open("grading_policy.json") as grading_policy_file: - policy_string = grading_policy_file.read() - except (IOError, ResourceNotFoundError): - log.warning("Unable to load course settings file from grading_policy.json in course " + self.id) - - grading_policy = load_grading_policy(policy_string) - - return grading_policy + return self._grading_policy['GRADE_CUTOFFS'] @lazyproperty def grading_context(self): diff --git a/common/lib/xmodule/xmodule/graders.py b/common/lib/xmodule/xmodule/graders.py index fca862aa9f..82dc37bf57 100644 --- a/common/lib/xmodule/xmodule/graders.py +++ b/common/lib/xmodule/xmodule/graders.py @@ -14,11 +14,11 @@ def load_grading_policy(course_policy_string): """ This loads a grading policy from a string (usually read from a file), which can be a JSON object or an empty string. - + The JSON object can have the keys GRADER and GRADE_CUTOFFS. If either is missing, it reverts to the default. """ - + default_policy_string = """ { "GRADER" : [ @@ -56,7 +56,7 @@ def load_grading_policy(course_policy_string): } } """ - + # Load the global settings as a dictionary grading_policy = json.loads(default_policy_string) @@ -64,15 +64,15 @@ def load_grading_policy(course_policy_string): course_policy = {} if course_policy_string: course_policy = json.loads(course_policy_string) - + # Override any global settings with the course settings grading_policy.update(course_policy) # Here is where we should parse any configurations, so that we can fail early grading_policy['GRADER'] = grader_from_conf(grading_policy['GRADER']) - + return grading_policy - + def aggregate_scores(scores, section_name="summary"): """ @@ -130,7 +130,9 @@ def grader_from_conf(conf): raise ValueError("Configuration has no appropriate grader class.") except (TypeError, ValueError) as error: - errorString = "Unable to parse grader configuration:\n " + str(subgraderconf) + "\n Error was:\n " + str(error) + errorString = ("Unable to parse grader configuration:\n " + + str(subgraderconf) + + "\n Error was:\n " + str(error)) log.critical(errorString) raise ValueError(errorString) diff --git a/common/lib/xmodule/xmodule/modulestore/xml.py b/common/lib/xmodule/xmodule/modulestore/xml.py index 769d110518..30610479b9 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml.py +++ b/common/lib/xmodule/xmodule/modulestore/xml.py @@ -4,16 +4,17 @@ import os import re from collections import defaultdict +from cStringIO import StringIO from fs.osfs import OSFS from importlib import import_module from lxml import etree from lxml.html import HtmlComment from path import path + from xmodule.errortracker import ErrorLog, make_error_tracker -from xmodule.x_module import XModuleDescriptor, XMLParsingSystem from xmodule.course_module import CourseDescriptor from xmodule.mako_module import MakoDescriptorSystem -from cStringIO import StringIO +from xmodule.x_module import XModuleDescriptor, XMLParsingSystem from . import ModuleStoreBase, Location from .exceptions import ItemNotFoundError @@ -202,6 +203,27 @@ class XMLModuleStore(ModuleStoreBase): return {} + def read_grading_policy(self, paths, tracker): + """Load a grading policy from the specified paths, in order, if it exists.""" + # Default to a blank policy + policy_str = "" + + for policy_path in paths: + if not os.path.exists(policy_path): + continue + try: + with open(policy_path) as grading_policy_file: + policy_str = grading_policy_file.read() + # if we successfully read the file, stop looking at backups + break + except (IOError): + msg = "Unable to load course settings file from '{0}'".format(policy_path) + tracker(msg) + log.warning(msg) + + return policy_str + + def load_course(self, course_dir, tracker): """ Load a course into this module store @@ -242,9 +264,11 @@ class XMLModuleStore(ModuleStoreBase): course = course_dir url_name = course_data.get('url_name', course_data.get('slug')) + policy_dir = None if url_name: - old_policy_path = self.data_dir / course_dir / 'policies' / url_name / 'policy.json' - policy = self.load_policy(old_policy_path, tracker) + policy_dir = self.data_dir / course_dir / 'policies' / url_name + policy_path = policy_dir / 'policy.json' + policy = self.load_policy(policy_path, tracker) # VS[compat]: remove once courses use the policy dirs. if policy == {}: @@ -273,6 +297,15 @@ class XMLModuleStore(ModuleStoreBase): # after we have the course descriptor. XModuleDescriptor.compute_inherited_metadata(course_descriptor) + # Try to load grading policy + paths = [self.data_dir / 'grading_policy.json'] + if policy_dir: + paths = [policy_dir / 'grading_policy.json'] + paths + + policy_str = self.read_grading_policy(paths, tracker) + course_descriptor.set_grading_policy(policy_str) + + log.debug('========> Done with course import from {0}'.format(course_dir)) return course_descriptor diff --git a/common/lib/xmodule/xmodule/tests/test_import.py b/common/lib/xmodule/xmodule/tests/test_import.py index 34e4767a62..3454366c1a 100644 --- a/common/lib/xmodule/xmodule/tests/test_import.py +++ b/common/lib/xmodule/xmodule/tests/test_import.py @@ -233,6 +233,9 @@ class ImportTestCase(unittest.TestCase): self.assertEqual(toy_ch.display_name, "Overview") self.assertEqual(two_toys_ch.display_name, "Two Toy Overview") + # Also check that the grading policy loaded + self.assertEqual(two_toys.grade_cutoffs['C'], 0.5999) + def test_definition_loading(self): """When two courses share the same org and course name and diff --git a/common/test/data/two_toys/policies/TT_2012_Fall/grading_policy.json b/common/test/data/two_toys/policies/TT_2012_Fall/grading_policy.json new file mode 100644 index 0000000000..13942c1715 --- /dev/null +++ b/common/test/data/two_toys/policies/TT_2012_Fall/grading_policy.json @@ -0,0 +1,35 @@ +{ + "GRADER" : [ + { + "type" : "Homework", + "min_count" : 12, + "drop_count" : 2, + "short_label" : "HW", + "weight" : 0.15 + }, + { + "type" : "Lab", + "min_count" : 12, + "drop_count" : 2, + "category" : "Labs", + "weight" : 0.15 + }, + { + "type" : "Midterm", + "name" : "Midterm Exam", + "short_label" : "Midterm", + "weight" : 0.3 + }, + { + "type" : "Final", + "name" : "Final Exam", + "short_label" : "Final", + "weight" : 0.4 + } + ], + "GRADE_CUTOFFS" : { + "A" : 0.87, + "B" : 0.7, + "C" : 0.5999 + } +} From b97a2af2a2e5d0fbcb6ffd04e007076e08dba4b5 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Tue, 21 Aug 2012 11:09:58 -0400 Subject: [PATCH 3/5] Show policy, grading policy, and top-level errors * Save errors for courses that failed to load in modulestore * global staff can see course errors on their dashboard * put policy loading errors into the error trackers * add has_access(user, 'global', 'staff'), which is equiv to user.is_staff for now --- common/djangoapps/student/views.py | 14 +++++++- common/lib/xmodule/xmodule/course_module.py | 9 ++++- common/lib/xmodule/xmodule/graders.py | 11 +++--- common/lib/xmodule/xmodule/modulestore/xml.py | 36 +++++++++++++------ lms/djangoapps/courseware/access.py | 36 +++++++++++++++++++ lms/djangoapps/courseware/courses.py | 1 - lms/templates/dashboard.html | 15 ++++++++ 7 files changed, 104 insertions(+), 18 deletions(-) diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index cab57e6819..f36936318e 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -140,7 +140,19 @@ def dashboard(request): if not user.is_active: message = render_to_string('registration/activate_account_notice.html', {'email': user.email}) - context = {'courses': courses, 'message': message} + + # Global staff can see what courses errored on their dashboard + staff_access = False + if has_access(user, 'global', 'staff'): + # Show any courses that errored on load + staff_access = True + errored_courses = modulestore().get_errored_courses() + + context = {'courses': courses, + 'message': message, + 'staff_access': staff_access, + 'errored_courses': errored_courses,} + return render_to_response('dashboard.html', context) diff --git a/common/lib/xmodule/xmodule/course_module.py b/common/lib/xmodule/xmodule/course_module.py index fb0721454c..5cc4a09165 100644 --- a/common/lib/xmodule/xmodule/course_module.py +++ b/common/lib/xmodule/xmodule/course_module.py @@ -78,7 +78,14 @@ class CourseDescriptor(SequenceDescriptor): def set_grading_policy(self, policy_str): """Parse the policy specified in policy_str, and save it""" - self._grading_policy = load_grading_policy(policy_str) + try: + self._grading_policy = load_grading_policy(policy_str) + except: + self.system.error_tracker("Failed to load grading policy") + # Setting this to an empty dictionary will lead to errors when + # grading needs to happen, but should allow course staff to see + # the error log. + self._grading_policy = {} @classmethod diff --git a/common/lib/xmodule/xmodule/graders.py b/common/lib/xmodule/xmodule/graders.py index 82dc37bf57..3f0bb63186 100644 --- a/common/lib/xmodule/xmodule/graders.py +++ b/common/lib/xmodule/xmodule/graders.py @@ -1,6 +1,7 @@ import abc import json import logging +import sys from collections import namedtuple @@ -130,11 +131,11 @@ def grader_from_conf(conf): raise ValueError("Configuration has no appropriate grader class.") except (TypeError, ValueError) as error: - errorString = ("Unable to parse grader configuration:\n " + - str(subgraderconf) + - "\n Error was:\n " + str(error)) - log.critical(errorString) - raise ValueError(errorString) + # Add info and re-raise + msg = ("Unable to parse grader configuration:\n " + + str(subgraderconf) + + "\n Error was:\n " + str(error)) + raise ValueError(msg), None, sys.exc_info()[2] return WeightedSubsectionsGrader(subgraders) diff --git a/common/lib/xmodule/xmodule/modulestore/xml.py b/common/lib/xmodule/xmodule/modulestore/xml.py index 30610479b9..3eca72987e 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml.py +++ b/common/lib/xmodule/xmodule/modulestore/xml.py @@ -135,12 +135,12 @@ class XMLModuleStore(ModuleStoreBase): self.data_dir = path(data_dir) self.modules = defaultdict(dict) # course_id -> dict(location -> XModuleDescriptor) self.courses = {} # course_dir -> XModuleDescriptor for the course + self.errored_courses = {} # course_dir -> errorlog, for dirs that failed to load if default_class is None: self.default_class = None else: module_path, _, class_name = default_class.rpartition('.') - #log.debug('module_path = %s' % module_path) class_ = getattr(import_module(module_path), class_name) self.default_class = class_ @@ -163,18 +163,27 @@ class XMLModuleStore(ModuleStoreBase): ''' Load a course, keeping track of errors as we go along. ''' + # Special-case code here, since we don't have a location for the + # course before it loads. + # So, make a tracker to track load-time errors, then put in the right + # place after the course loads and we have its location + errorlog = make_error_tracker() + course_descriptor = None try: - # Special-case code here, since we don't have a location for the - # course before it loads. - # So, make a tracker to track load-time errors, then put in the right - # place after the course loads and we have its location - errorlog = make_error_tracker() course_descriptor = self.load_course(course_dir, errorlog.tracker) + except Exception as e: + msg = "Failed to load course '{0}': {1}".format(course_dir, str(e)) + log.exception(msg) + errorlog.tracker(msg) + + if course_descriptor is not None: self.courses[course_dir] = course_descriptor self._location_errors[course_descriptor.location] = errorlog - except: - msg = "Failed to load course '%s'" % course_dir - log.exception(msg) + else: + # Didn't load course. Instead, save the errors elsewhere. + self.errored_courses[course_dir] = errorlog + + def __unicode__(self): ''' @@ -211,6 +220,7 @@ class XMLModuleStore(ModuleStoreBase): for policy_path in paths: if not os.path.exists(policy_path): continue + log.debug("Loading grading policy from {0}".format(policy_path)) try: with open(policy_path) as grading_policy_file: policy_str = grading_policy_file.read() @@ -298,7 +308,7 @@ class XMLModuleStore(ModuleStoreBase): XModuleDescriptor.compute_inherited_metadata(course_descriptor) # Try to load grading policy - paths = [self.data_dir / 'grading_policy.json'] + paths = [self.data_dir / course_dir / 'grading_policy.json'] if policy_dir: paths = [policy_dir / 'grading_policy.json'] + paths @@ -356,6 +366,12 @@ class XMLModuleStore(ModuleStoreBase): """ return self.courses.values() + def get_errored_courses(self): + """ + Return a dictionary of course_dir -> [(msg, exception_str)], for each + course_dir where course loading failed. + """ + return dict( (k, self.errored_courses[k].errors) for k in self.errored_courses) def create_item(self, location): raise NotImplementedError("XMLModuleStores are read-only") diff --git a/lms/djangoapps/courseware/access.py b/lms/djangoapps/courseware/access.py index eaf70d7814..281580cf33 100644 --- a/lms/djangoapps/courseware/access.py +++ b/lms/djangoapps/courseware/access.py @@ -63,6 +63,9 @@ def has_access(user, obj, action): if isinstance(obj, Location): return _has_access_location(user, obj, action) + if isinstance(obj, basestring): + return _has_access_string(user, obj, action) + # Passing an unknown object here is a coding error, so rather than # returning a default, complain. raise TypeError("Unknown object type in has_access(): '{0}'" @@ -238,6 +241,30 @@ def _has_access_location(user, location, action): return _dispatch(checkers, action, user, location) +def _has_access_string(user, perm, action): + """ + Check if user has certain special access, specified as string. Valid strings: + + 'global' + + Valid actions: + + 'staff' -- global staff access. + """ + + def check_staff(): + if perm != 'global': + debug("Deny: invalid permission '%s'", perm) + return False + return _has_global_staff_access(user) + + checkers = { + 'staff': check_staff + } + + return _dispatch(checkers, action, user, perm) + + ##### Internal helper methods below def _dispatch(table, action, user, obj): @@ -266,6 +293,15 @@ def _course_staff_group_name(location): """ return 'staff_%s' % Location(location).course +def _has_global_staff_access(user): + if user.is_staff: + debug("Allow: user.is_staff") + return True + else: + debug("Deny: not user.is_staff") + return False + + def _has_staff_access_to_location(user, location): ''' Returns True if the given user has staff access to a location. For now this diff --git a/lms/djangoapps/courseware/courses.py b/lms/djangoapps/courseware/courses.py index b4407b7f93..30ca38728a 100644 --- a/lms/djangoapps/courseware/courses.py +++ b/lms/djangoapps/courseware/courses.py @@ -30,7 +30,6 @@ def get_course_by_id(course_id): raise Http404("Course not found.") - def get_course_with_access(user, course_id, action): """ Given a course_id, look up the corresponding course descriptor, diff --git a/lms/templates/dashboard.html b/lms/templates/dashboard.html index cfe8a0953c..770e849841 100644 --- a/lms/templates/dashboard.html +++ b/lms/templates/dashboard.html @@ -107,6 +107,21 @@ % endif + % if staff_access and len(errored_courses) > 0: +
+

Course-loading errors

+ + % for course_dir, errors in errored_courses.items(): +

${course_dir | h}

+
    + % for (msg, err) in errors: +
  • ${msg} + 
    • ${err}
    +
    • + % endfor +
+ % endfor + % endif From ccf2cff2bafb18dcfe6d5caeab8bd78cf6816af3 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Tue, 21 Aug 2012 13:04:33 -0400 Subject: [PATCH 4/5] bugfix: set errored_courses in default case --- common/djangoapps/student/views.py | 1 + 1 file changed, 1 insertion(+) diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index f36936318e..66a860ab15 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -143,6 +143,7 @@ def dashboard(request): # Global staff can see what courses errored on their dashboard staff_access = False + errored_courses = [] if has_access(user, 'global', 'staff'): # Show any courses that errored on load staff_access = True From 2f9b2f19fc80bd276d976e21f886312a0828cc23 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Tue, 21 Aug 2012 15:01:10 -0400 Subject: [PATCH 5/5] s/[]/{}/ --- common/djangoapps/student/views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index 66a860ab15..0069935b0b 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -143,7 +143,7 @@ def dashboard(request): # Global staff can see what courses errored on their dashboard staff_access = False - errored_courses = [] + errored_courses = {} if has_access(user, 'global', 'staff'): # Show any courses that errored on load staff_access = True