From 408e988fbc59fd1e648cf46db4b4e7022d3cd37c Mon Sep 17 00:00:00 2001 From: Bridger Maxwell Date: Fri, 3 Aug 2012 11:41:06 -0400 Subject: [PATCH 001/163] Started working on fast_grade method. It skips loading sections that a student doesn't have any answers for. --- lms/djangoapps/courseware/grades.py | 106 ++++++++++++++++++++++++++-- lms/djangoapps/courseware/views.py | 13 ++++ lms/urls.py | 4 ++ 3 files changed, 116 insertions(+), 7 deletions(-) diff --git a/lms/djangoapps/courseware/grades.py b/lms/djangoapps/courseware/grades.py index 5a817e3d6c..ca8620c1f6 100644 --- a/lms/djangoapps/courseware/grades.py +++ b/lms/djangoapps/courseware/grades.py @@ -3,6 +3,7 @@ import logging from django.conf import settings +from module_render import get_module from xmodule import graders from xmodule.graders import Score from models import StudentModule @@ -10,6 +11,103 @@ from models import StudentModule _log = logging.getLogger("mitx.courseware") + + + +def get_graded_sections(course_descriptor): + """ + Arguments: + course_descriptor: a CourseDescriptor object for the course to be graded + + Returns: + A dictionary keyed by section-type. The values are arrays of dictionaries containing + "section_descriptor" : The section descriptor + "xmoduledescriptors" : An array of xmoduledescriptors that could possibly be in the section, for any student + """ + + graded_sections = {} + for c in course_descriptor.get_children(): + sections = [] + for s in c.get_children(): + if s.metadata.get('graded', False): + + xmoduledescriptors = [] + for module in yield_descriptor_descendents(s): + xmoduledescriptors.append(module) + + section_description = { 'section_descriptor' : s, 'xmoduledescriptors' : xmoduledescriptors} + + section_format = s.metadata.get('format', "") + graded_sections[ section_format ] = graded_sections.get( section_format, [] ) + [section_description] + + return graded_sections + +def yield_descriptor_descendents(module_descriptor): + for child in module_descriptor.get_children(): + yield child + for module_descriptor in yield_descriptor_descendents(child): + yield module_descriptor + +def yield_module_descendents(module): + for child in module.get_display_items(): + yield child + for module in yield_module_descendents(child): + yield module + +def fast_grade(student, request, course_graded_sections, grader, student_module_cache): + totaled_scores = {} + for section_format, sections in course_graded_sections.iteritems(): + format_scores = [] + for section in sections: + section_descriptor = section['section_descriptor'] + section_name = section_descriptor.metadata.get('display_name') + + should_grade_section = False + # If we haven't seen a single problem in the section, we don't have to grade it at all! We can assume 0% + for moduledescriptor in section['xmoduledescriptors']: + if student_module_cache.lookup(moduledescriptor.category, moduledescriptor.location.url() ): + should_grade_section = True + break + + if should_grade_section: + scores = [] + (section_module, _, _, _) = get_module(student, request, section_descriptor.location, student_module_cache) + + for module in yield_descriptor_descendents(section_module): + (correct, total) = get_score(student, module, student_module_cache) + graded = module.metadata.get("graded", False) + + if correct is None and total is None: + continue + + if settings.GENERATE_PROFILE_SCORES: + if total > 1: + correct = random.randrange(max(total - 2, 1), total + 1) + else: + correct = total + + if not total > 0: + #We simply cannot grade a problem that is 12/0, because we might need it as a percentage + graded = False + + scores.append(Score(correct, total, graded, module.metadata.get('display_name'))) + + section_total, graded_total = graders.aggregate_scores(scores, section_name) + else: + section_total = Score(0.0, 1.0, False, section_name) + graded_possible = 1.0 #if s.metadata.get("graded", False) else 0.0 + graded_total = Score(0.0, graded_possible, True, section_name) + + #Add the graded total to totaled_scores + if graded_total.possible > 0: + format_scores.append(graded_total) + + totaled_scores[section_format] = format_scores + + grade_summary = grade_summary = grader.grade(totaled_scores) + return grade_summary + + def grade_sheet(student, course, grader, student_module_cache): """ This pulls a summary of all problems in the course. It returns a dictionary with two datastructures: @@ -30,15 +128,9 @@ def grade_sheet(student, course, grader, student_module_cache): for c in course.get_children(): sections = [] for s in c.get_children(): - def yield_descendents(module): - yield module - for child in module.get_display_items(): - for module in yield_descendents(child): - yield module - graded = s.metadata.get('graded', False) scores = [] - for module in yield_descendents(s): + for module in yield_module_descendents(s): (correct, total) = get_score(student, module, student_module_cache) if correct is None and total is None: diff --git a/lms/djangoapps/courseware/views.py b/lms/djangoapps/courseware/views.py index 7281ab01ad..a89a4bf34f 100644 --- a/lms/djangoapps/courseware/views.py +++ b/lms/djangoapps/courseware/views.py @@ -35,6 +35,19 @@ log = logging.getLogger("mitx.courseware") template_imports = {'urllib': urllib} +def test_grading(request, course_id): + course = check_course(course_id) + + sections = grades.get_graded_sections(course) + + student_module_cache = StudentModuleCache(request.user, course) + + grade_result = grades.fast_grade(request.user, request, sections, course.grader, student_module_cache) + + return HttpResponse("" + json.dumps(grade_result) + "") + + + def user_groups(user): if not user.is_authenticated(): return [] diff --git a/lms/urls.py b/lms/urls.py index 1c4a065e2b..8d7460ddc2 100644 --- a/lms/urls.py +++ b/lms/urls.py @@ -118,6 +118,10 @@ if settings.COURSEWARE_ENABLED: url(r'^courses/(?P[^/]+/[^/]+/[^/]+)/about$', 'courseware.views.course_about', name="about_course"), + # TODO: Important. Remove this after testing! + url(r'^courses/(?P[^/]+/[^/]+/[^/]+)/grade$', + 'courseware.views.test_grading', name="info"), + #Inside the course url(r'^courses/(?P[^/]+/[^/]+/[^/]+)/info$', 'courseware.views.course_info', name="info"), From 854508b0067f9617096bbddf6f3d81ebefe1d22f Mon Sep 17 00:00:00 2001 From: Bridger Maxwell Date: Fri, 3 Aug 2012 14:30:45 -0400 Subject: [PATCH 002/163] Removed the unnecessary creation of StudentModules when calling module_render's get_module. --- lms/djangoapps/courseware/grades.py | 33 +++--- .../management/commands/check_course.py | 2 +- lms/djangoapps/courseware/module_render.py | 108 ++++++++++++------ lms/djangoapps/courseware/views.py | 6 +- 4 files changed, 90 insertions(+), 59 deletions(-) diff --git a/lms/djangoapps/courseware/grades.py b/lms/djangoapps/courseware/grades.py index ca8620c1f6..e522468f45 100644 --- a/lms/djangoapps/courseware/grades.py +++ b/lms/djangoapps/courseware/grades.py @@ -3,17 +3,13 @@ import logging from django.conf import settings -from module_render import get_module +from module_render import get_module, get_instance_module from xmodule import graders from xmodule.graders import Score from models import StudentModule _log = logging.getLogger("mitx.courseware") - - - - def get_graded_sections(course_descriptor): """ Arguments: @@ -71,7 +67,7 @@ def fast_grade(student, request, course_graded_sections, grader, student_module_ if should_grade_section: scores = [] - (section_module, _, _, _) = get_module(student, request, section_descriptor.location, student_module_cache) + section_module = get_module(student, request, section_descriptor.location, student_module_cache) for module in yield_descriptor_descendents(section_module): (correct, total) = get_score(student, module, student_module_cache) @@ -175,7 +171,7 @@ def grade_sheet(student, course, grader, student_module_cache): 'grade_summary': grade_summary} -def get_score(user, problem, cache): +def get_score(user, problem, student_module_cache): """ Return the score for a user on a problem @@ -186,17 +182,18 @@ def get_score(user, problem, cache): correct = 0.0 # If the ID is not in the cache, add the item - instance_module = cache.lookup(problem.category, problem.id) - if instance_module is None: - instance_module = StudentModule(module_type=problem.category, - module_state_key=problem.id, - student=user, - state=None, - grade=0, - max_grade=problem.max_score(), - done='i') - cache.append(instance_module) - instance_module.save() + instance_module = get_instance_module(user, problem, student_module_cache) + # instance_module = student_module_cache.lookup(problem.category, problem.id) + # if instance_module is None: + # instance_module = StudentModule(module_type=problem.category, + # module_state_key=problem.id, + # student=user, + # state=None, + # grade=0, + # max_grade=problem.max_score(), + # done='i') + # cache.append(instance_module) + # instance_module.save() # If this problem is ungraded/ungradable, bail if instance_module.max_grade is None: diff --git a/lms/djangoapps/courseware/management/commands/check_course.py b/lms/djangoapps/courseware/management/commands/check_course.py index 6ccd6d5fe7..bf54147513 100644 --- a/lms/djangoapps/courseware/management/commands/check_course.py +++ b/lms/djangoapps/courseware/management/commands/check_course.py @@ -79,7 +79,7 @@ class Command(BaseCommand): # TODO (cpennington): Get coursename in a legitimate way course_location = 'i4x://edx/6002xs12/course/6.002_Spring_2012' student_module_cache = StudentModuleCache(sample_user, modulestore().get_item(course_location)) - (course, _, _, _) = get_module(sample_user, None, course_location, student_module_cache) + course = get_module(sample_user, None, course_location, student_module_cache) to_run = [ #TODO (vshnayder) : make check_rendering work (use module_render.py), diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index bf7d1f4c51..021f2b2ed8 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/lms/djangoapps/courseware/module_render.py @@ -45,9 +45,9 @@ def toc_for_course(user, request, course, active_chapter, active_section): chapters with name 'hidden' are skipped. ''' - + student_module_cache = StudentModuleCache(user, course, depth=2) - (course, _, _, _) = get_module(user, request, course.location, student_module_cache) + course = get_module(user, request, course.location, student_module_cache) chapters = list() for chapter in course.get_display_items(): @@ -114,25 +114,26 @@ def get_module(user, request, location, student_module_cache, position=None): - position : extra information from URL for user-specified position within module - Returns: - - a tuple (xmodule instance, instance_module, shared_module, module category). - instance_module is a StudentModule specific to this module for this student, - or None if this is an anonymous user - shared_module is a StudentModule specific to all modules with the same - 'shared_state_key' attribute, or None if the module does not elect to - share state + Returns: xmodule instance + ''' descriptor = modulestore().get_item(location) - - instance_module = student_module_cache.lookup(descriptor.category, - descriptor.location.url()) - - shared_state_key = getattr(descriptor, 'shared_state_key', None) - if shared_state_key is not None: - shared_module = student_module_cache.lookup(descriptor.category, - shared_state_key) + + #TODO Only check the cache if this module can possibly have state + if user.is_authenticated(): + instance_module = student_module_cache.lookup(descriptor.category, + descriptor.location.url()) + + shared_state_key = getattr(descriptor, 'shared_state_key', None) + if shared_state_key is not None: + shared_module = student_module_cache.lookup(descriptor.category, + shared_state_key) + else: + shared_module = None else: + instance_module = None shared_module = None + instance_state = instance_module.state if instance_module is not None else None shared_state = shared_module.state if shared_module is not None else None @@ -144,9 +145,8 @@ def get_module(user, request, location, student_module_cache, position=None): str(user.id) + '/' + descriptor.location.url() + '/') def _get_module(location): - (module, _, _, _) = get_module(user, request, location, + return get_module(user, request, location, student_module_cache, position) - return module # TODO (cpennington): When modules are shared between courses, the static # prefix is going to have to be specific to the module, not the directory @@ -177,30 +177,59 @@ def get_module(user, request, location, student_module_cache, position=None): if settings.MITX_FEATURES.get('DISPLAY_HISTOGRAMS_TO_STAFF') and user.is_staff: module.get_html = add_histogram(module.get_html) - # If StudentModule for this instance wasn't already in the database, - # and this isn't a guest user, create it. + return module + +def get_instance_module(user, module, student_module_cache): + """ + Returns instance_module is a StudentModule specific to this module for this student, + or None if this is an anonymous user + """ if user.is_authenticated(): + instance_module = student_module_cache.lookup(module.category, + module.location.url()) + if not instance_module: instance_module = StudentModule( student=user, - module_type=descriptor.category, + module_type=module.category, module_state_key=module.id, state=module.get_instance_state(), max_grade=module.max_score()) instance_module.save() - # Add to cache. The caller and the system context have references - # to it, so the change persists past the return student_module_cache.append(instance_module) - if not shared_module and shared_state_key is not None: - shared_module = StudentModule( - student=user, - module_type=descriptor.category, - module_state_key=shared_state_key, - state=module.get_shared_state()) - shared_module.save() - student_module_cache.append(shared_module) - - return (module, instance_module, shared_module, descriptor.category) + + return instance_module + else: + return None + +def get_shared_instance_module(user, module, student_module_cache): + """ + Return shared_module is a StudentModule specific to all modules with the same + 'shared_state_key' attribute, or None if the module does not elect to + share state + """ + if user.is_authenticated(): + # To get the shared_state_key, we need to descriptor + descriptor = modulestore().get_item(module.location) + + shared_state_key = getattr(module, 'shared_state_key', None) + if shared_state_key is not None: + shared_module = student_module_cache.lookup(module.category, + shared_state_key) + if not shared_module: + shared_module = StudentModule( + student=user, + module_type=descriptor.category, + module_state_key=shared_state_key, + state=module.get_shared_state()) + shared_module.save() + student_module_cache.append(shared_module) + else: + shared_module = None + + return shared_module + else: + return None # TODO: TEMPORARY BYPASS OF AUTH! @@ -218,7 +247,9 @@ def xqueue_callback(request, userid, id, dispatch): user = User.objects.get(id=userid) student_module_cache = StudentModuleCache(user, modulestore().get_item(id)) - instance, instance_module, shared_module, module_type = get_module(request.user, request, id, student_module_cache) + instance = get_module(request.user, request, id, student_module_cache) + + instance_module = get_instance_module(request.user, instance, student_module_cache) if instance_module is None: log.debug("Couldn't find module '%s' for user '%s'", @@ -264,8 +295,11 @@ def modx_dispatch(request, dispatch=None, id=None): # ''' (fix emacs broken parsing) student_module_cache = StudentModuleCache(request.user, modulestore().get_item(id)) - instance, instance_module, shared_module, module_type = get_module(request.user, request, id, student_module_cache) - + instance = get_module(request.user, request, id, student_module_cache) + + instance_module = get_instance_module(request.user, instance, student_module_cache) + shared_module = get_shared_instance_module(request.user, instance, student_module_cache) + # Don't track state for anonymous users (who don't have student modules) if instance_module is not None: oldgrade = instance_module.grade diff --git a/lms/djangoapps/courseware/views.py b/lms/djangoapps/courseware/views.py index a89a4bf34f..423d436087 100644 --- a/lms/djangoapps/courseware/views.py +++ b/lms/djangoapps/courseware/views.py @@ -93,7 +93,7 @@ def gradebook(request, course_id): for student in student_objects: student_module_cache = StudentModuleCache(student, course) - course, _, _, _ = get_module(request.user, request, course.location, student_module_cache) + course = get_module(request.user, request, course.location, student_module_cache) student_info.append({ 'username': student.username, 'id': student.id, @@ -122,7 +122,7 @@ def profile(request, course_id, student_id=None): user_info = UserProfile.objects.get(user=student) student_module_cache = StudentModuleCache(request.user, course) - course_module, _, _, _ = get_module(request.user, request, course.location, student_module_cache) + course_module = get_module(request.user, request, course.location, student_module_cache) context = {'name': user_info.name, 'username': student.username, @@ -213,7 +213,7 @@ def index(request, course_id, chapter=None, section=None, if section_descriptor is not None: student_module_cache = StudentModuleCache(request.user, section_descriptor) - module, _, _, _ = get_module(request.user, request, + module = get_module(request.user, request, section_descriptor.location, student_module_cache) context['content'] = module.get_html() From 694520ebb2c258b225b1d1e18fddf265039a1541 Mon Sep 17 00:00:00 2001 From: Bridger Maxwell Date: Fri, 3 Aug 2012 17:00:04 -0400 Subject: [PATCH 003/163] StudentModuleCache now allows a list of descriptors. This speeds up grading significantly. --- common/lib/xmodule/xmodule/x_module.py | 10 ++-- lms/djangoapps/courseware/grades.py | 6 ++- lms/djangoapps/courseware/models.py | 67 ++++++++++++++++++-------- lms/djangoapps/courseware/views.py | 5 +- 4 files changed, 58 insertions(+), 30 deletions(-) diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index 3406bcb99c..30522b871c 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -501,11 +501,11 @@ class XModuleDescriptor(Plugin, HTMLSnippet): all(getattr(self, attr, None) == getattr(other, attr, None) for attr in self.equality_attributes)) - if not eq: - for attr in self.equality_attributes: - print(getattr(self, attr, None), - getattr(other, attr, None), - getattr(self, attr, None) == getattr(other, attr, None)) + # if not eq: + # for attr in self.equality_attributes: + # print(getattr(self, attr, None), + # getattr(other, attr, None), + # getattr(self, attr, None) == getattr(other, attr, None)) return eq diff --git a/lms/djangoapps/courseware/grades.py b/lms/djangoapps/courseware/grades.py index e522468f45..7cb928e648 100644 --- a/lms/djangoapps/courseware/grades.py +++ b/lms/djangoapps/courseware/grades.py @@ -20,6 +20,7 @@ def get_graded_sections(course_descriptor): "section_descriptor" : The section descriptor "xmoduledescriptors" : An array of xmoduledescriptors that could possibly be in the section, for any student """ + all_descriptors = [] graded_sections = {} for c in course_descriptor.get_children(): @@ -35,8 +36,11 @@ def get_graded_sections(course_descriptor): section_format = s.metadata.get('format', "") graded_sections[ section_format ] = graded_sections.get( section_format, [] ) + [section_description] + + all_descriptors.extend(xmoduledescriptors) + all_descriptors.append(s) - return graded_sections + return graded_sections, all_descriptors def yield_descriptor_descendents(module_descriptor): for child in module_descriptor.get_children(): diff --git a/lms/djangoapps/courseware/models.py b/lms/djangoapps/courseware/models.py index 3b0ca7fdcf..79617e428d 100644 --- a/lms/djangoapps/courseware/models.py +++ b/lms/djangoapps/courseware/models.py @@ -67,17 +67,30 @@ class StudentModuleCache(object): """ A cache of StudentModules for a specific student """ - def __init__(self, user, descriptor, depth=None): + def __init__(self, user, descriptor=None, depth=None, descriptors=None, descriptor_filter=lambda descriptor: True): ''' Find any StudentModule objects that are needed by any child modules of the - supplied descriptor. Avoids making multiple queries to the database - - descriptor: An XModuleDescriptor - depth is the number of levels of descendent modules to load StudentModules for, in addition to - the supplied descriptor. If depth is None, load all descendent StudentModules + supplied descriptor, or caches only the StudentModule objects specifically + for every descriptor in descriptors. Avoids making multiple queries to the + database. + + There are two ways to init: + descriptor: An XModuleDescriptor + depth is the number of levels of descendent modules to load StudentModules for, in addition to + the supplied descriptor. If depth is None, load all descendent StudentModules + OR + descriptors: An array of XModuleDescriptors. + + descriptor_filter is a function that accepts a descriptor and return wether the StudentModule + should be cached ''' if user.is_authenticated(): - module_ids = self._get_module_state_keys(descriptor, depth) + if not (descriptor == None) != (descriptors == None): #An xor on the descriptor and descriptors parameters. + raise ValueError("Either the descriptor or descriptors must be supplied to StudentModuleCache.") + + if descriptor != None: + descriptors = self._get_child_descriptors(descriptor, depth) + module_ids = self._get_module_state_keys(descriptors, descriptor_filter) # This works around a limitation in sqlite3 on the number of parameters # that can be put into a single query @@ -91,27 +104,39 @@ class StudentModuleCache(object): else: self.cache = [] - - def _get_module_state_keys(self, descriptor, depth): - ''' - Get a list of the state_keys needed for StudentModules - required for this module descriptor - + + def _get_child_descriptors(self, descriptor, depth): + """ descriptor: An XModuleDescriptor depth is the number of levels of descendent modules to load StudentModules for, in addition to the supplied descriptor. If depth is None, load all descendent StudentModules - ''' - keys = [descriptor.location.url()] - - shared_state_key = getattr(descriptor, 'shared_state_key', None) - if shared_state_key is not None: - keys.append(shared_state_key) - + """ + descriptors = [descriptor] + if depth is None or depth > 0: new_depth = depth - 1 if depth is not None else depth for child in descriptor.get_children(): - keys.extend(self._get_module_state_keys(child, new_depth)) + descriptors.extend(self._get_child_descriptors(child, new_depth)) + + return descriptors + + def _get_module_state_keys(self, descriptors, descriptor_filter): + ''' + Get a list of the state_keys needed for StudentModules + required for this module descriptor + + descriptor_filter is a function that accepts a descriptor and return wether the StudentModule + should be cached + ''' + keys = [] + for descriptor in descriptors: + if descriptor_filter(descriptor): + keys.append(descriptor.location.url()) + + shared_state_key = getattr(descriptor, 'shared_state_key', None) + if shared_state_key is not None: + keys.append(shared_state_key) return keys diff --git a/lms/djangoapps/courseware/views.py b/lms/djangoapps/courseware/views.py index 423d436087..c8939c0b75 100644 --- a/lms/djangoapps/courseware/views.py +++ b/lms/djangoapps/courseware/views.py @@ -38,9 +38,8 @@ template_imports = {'urllib': urllib} def test_grading(request, course_id): course = check_course(course_id) - sections = grades.get_graded_sections(course) - - student_module_cache = StudentModuleCache(request.user, course) + sections, all_descriptors = grades.get_graded_sections(course) + student_module_cache = StudentModuleCache(request.user, descriptors=all_descriptors) grade_result = grades.fast_grade(request.user, request, sections, course.grader, student_module_cache) From 6d650d18251bc0bedb902fcc8404548585f54eb3 Mon Sep 17 00:00:00 2001 From: Bridger Maxwell Date: Fri, 3 Aug 2012 17:55:25 -0400 Subject: [PATCH 004/163] Got the gradebook working again with the new fast grading method. --- lms/djangoapps/certificates/views.py | 2 ++ lms/djangoapps/courseware/grades.py | 42 ++++++++-------------------- lms/djangoapps/courseware/views.py | 24 ++++++++++------ lms/envs/common.py | 2 +- lms/envs/devplus.py | 4 ++- lms/templates/gradebook.html | 6 ++-- lms/urls.py | 5 +++- 7 files changed, 40 insertions(+), 45 deletions(-) diff --git a/lms/djangoapps/certificates/views.py b/lms/djangoapps/certificates/views.py index 6341133c52..deb78fe3c7 100644 --- a/lms/djangoapps/certificates/views.py +++ b/lms/djangoapps/certificates/views.py @@ -52,6 +52,7 @@ def certificate_request(request): return return_error(survey_response['error']) grade = None + # TODO: (bridger) Update this to use the faster grade instead of grade_sheet student_gradesheet = grades.grade_sheet(request.user) grade = student_gradesheet['grade'] @@ -65,6 +66,7 @@ def certificate_request(request): else: #This is not a POST, we should render the page with the form + # TODO: (bridger) Update this to use the faster grade instead of grade_sheet grade_sheet = grades.grade_sheet(request.user) certificate_state = certificate_state_for_student(request.user, grade_sheet['grade']) diff --git a/lms/djangoapps/courseware/grades.py b/lms/djangoapps/courseware/grades.py index 7cb928e648..d31239eb4f 100644 --- a/lms/djangoapps/courseware/grades.py +++ b/lms/djangoapps/courseware/grades.py @@ -30,6 +30,7 @@ def get_graded_sections(course_descriptor): xmoduledescriptors = [] for module in yield_descriptor_descendents(s): + # TODO: Only include modules that have a score here xmoduledescriptors.append(module) section_description = { 'section_descriptor' : s, 'xmoduledescriptors' : xmoduledescriptors} @@ -73,19 +74,20 @@ def fast_grade(student, request, course_graded_sections, grader, student_module_ scores = [] section_module = get_module(student, request, section_descriptor.location, student_module_cache) - for module in yield_descriptor_descendents(section_module): + # TODO: We may be able to speed this up by only getting a list of children IDs from section_module + # Then, we may not need to instatiate any problems if they are already in the database + for module in yield_module_descendents(section_module): (correct, total) = get_score(student, module, student_module_cache) - graded = module.metadata.get("graded", False) - if correct is None and total is None: continue - + if settings.GENERATE_PROFILE_SCORES: if total > 1: correct = random.randrange(max(total - 2, 1), total + 1) else: correct = total - + + graded = module.metadata.get("graded", False) if not total > 0: #We simply cannot grade a problem that is 12/0, because we might need it as a percentage graded = False @@ -110,20 +112,18 @@ def fast_grade(student, request, course_graded_sections, grader, student_module_ def grade_sheet(student, course, grader, student_module_cache): """ - This pulls a summary of all problems in the course. It returns a dictionary with two datastructures: + This pulls a summary of all problems in the course. + Returns - courseware_summary is a summary of all sections with problems in the course. It is organized as an array of chapters, each containing an array of sections, each containing an array of scores. This contains information for graded and ungraded problems, and is good for displaying a course summary with due dates, etc. - - - grade_summary is the output from the course grader. More information on the format is in the docstring for CourseGrader. - + Arguments: student: A User object for the student to grade course: An XModule containing the course to grade student_module_cache: A StudentModuleCache initialized with all instance_modules for the student """ - totaled_scores = {} chapters = [] for c in course.get_children(): sections = [] @@ -132,30 +132,13 @@ def grade_sheet(student, course, grader, student_module_cache): scores = [] for module in yield_module_descendents(s): (correct, total) = get_score(student, module, student_module_cache) - if correct is None and total is None: continue - if settings.GENERATE_PROFILE_SCORES: - if total > 1: - correct = random.randrange(max(total - 2, 1), total + 1) - else: - correct = total - - if not total > 0: - #We simply cannot grade a problem that is 12/0, because we might need it as a percentage - graded = False - scores.append(Score(correct, total, graded, module.metadata.get('display_name'))) section_total, graded_total = graders.aggregate_scores(scores, s.metadata.get('display_name')) - #Add the graded total to totaled_scores format = s.metadata.get('format', "") - if format and graded_total.possible > 0: - format_scores = totaled_scores.get(format, []) - format_scores.append(graded_total) - totaled_scores[format] = format_scores - sections.append({ 'section': s.metadata.get('display_name'), 'scores': scores, @@ -169,10 +152,7 @@ def grade_sheet(student, course, grader, student_module_cache): 'chapter': c.metadata.get('display_name'), 'sections': sections}) - grade_summary = grader.grade(totaled_scores) - - return {'courseware_summary': chapters, - 'grade_summary': grade_summary} + return chapters def get_score(user, problem, student_module_cache): diff --git a/lms/djangoapps/courseware/views.py b/lms/djangoapps/courseware/views.py index c8939c0b75..fa8245212e 100644 --- a/lms/djangoapps/courseware/views.py +++ b/lms/djangoapps/courseware/views.py @@ -86,18 +86,20 @@ def gradebook(request, course_id): if 'course_admin' not in user_groups(request.user): raise Http404 course = check_course(course_id) - + + sections, all_descriptors = grades.get_graded_sections(course) + student_objects = User.objects.all()[:100] student_info = [] - + for student in student_objects: - student_module_cache = StudentModuleCache(student, course) - course = get_module(request.user, request, course.location, student_module_cache) + student_module_cache = StudentModuleCache(student, descriptors=all_descriptors) + student_info.append({ 'username': student.username, 'id': student.id, 'email': student.email, - 'grade_info': grades.grade_sheet(student, course, student_module_cache), + 'grade_summary': grades.fast_grade(student, request, sections, course.grader, student_module_cache), 'realname': UserProfile.objects.get(user=student).name }) @@ -123,16 +125,21 @@ def profile(request, course_id, student_id=None): student_module_cache = StudentModuleCache(request.user, course) course_module = get_module(request.user, request, course.location, student_module_cache) + courseware_summary = grades.grade_sheet(student, course_module, course.grader, student_module_cache) + sections, _ = grades.get_graded_sections(course) + grade_summary = grades.fast_grade(request.user, request, sections, course.grader, student_module_cache) + context = {'name': user_info.name, 'username': student.username, 'location': user_info.location, 'language': user_info.language, 'email': student.email, 'course': course, - 'format_url_params': format_url_params, - 'csrf': csrf(request)['csrf_token'] + 'csrf': csrf(request)['csrf_token'], + 'courseware_summary' : courseware_summary, + 'grade_summary' : grade_summary } - context.update(grades.grade_sheet(student, course_module, course.grader, student_module_cache)) + context.update() return render_to_response('profile.html', context) @@ -157,6 +164,7 @@ def render_accordion(request, course, chapter, section): ('toc', toc), ('course_name', course.title), ('course_id', course.id), + #TODO: Do we need format_url_params anymore? What is a better way to create the reversed links? ('format_url_params', format_url_params), ('csrf', csrf(request)['csrf_token'])] + template_imports.items()) return render_to_string('accordion.html', context) diff --git a/lms/envs/common.py b/lms/envs/common.py index 95daa913e8..c5e653ac0e 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -41,7 +41,7 @@ PERFSTATS = False MITX_FEATURES = { 'SAMPLE' : False, 'USE_DJANGO_PIPELINE' : True, - 'DISPLAY_HISTOGRAMS_TO_STAFF' : True, + 'DISPLAY_HISTOGRAMS_TO_STAFF' : False, 'REROUTE_ACTIVATION_EMAIL' : False, # nonempty string = address for all activation emails 'DEBUG_LEVEL' : 0, # 0 = lowest level, least verbose, 255 = max level, most verbose diff --git a/lms/envs/devplus.py b/lms/envs/devplus.py index b15322c2c7..bb4524a1ab 100644 --- a/lms/envs/devplus.py +++ b/lms/envs/devplus.py @@ -65,5 +65,7 @@ DEBUG_TOOLBAR_PANELS = ( # Django=1.3.1/1.4 where requests to views get duplicated (your method gets # hit twice). So you can uncomment when you need to diagnose performance # problems, but you shouldn't leave it on. -# 'debug_toolbar.panels.profiling.ProfilingDebugPanel', + 'debug_toolbar.panels.profiling.ProfilingDebugPanel', ) + +#PIPELINE = True diff --git a/lms/templates/gradebook.html b/lms/templates/gradebook.html index bcc0456711..70f23d5a02 100644 --- a/lms/templates/gradebook.html +++ b/lms/templates/gradebook.html @@ -28,7 +28,7 @@ %if len(students) > 0: <% - templateSummary = students[0]['grade_info']['grade_summary'] + templateSummary = students[0]['grade_summary'] %> @@ -58,10 +58,10 @@ %for student in students: - %for section in student['grade_info']['grade_summary']['section_breakdown']: + %for section in student['grade_summary']['section_breakdown']: ${percent_data( section['percent'] )} %endfor - + %endfor
${student['username']}${percent_data( student['grade_info']['grade_summary']['percent'])}${percent_data( student['grade_summary']['percent'])}
diff --git a/lms/urls.py b/lms/urls.py index 8d7460ddc2..10e9e750c9 100644 --- a/lms/urls.py +++ b/lms/urls.py @@ -105,7 +105,6 @@ if settings.COURSEWARE_ENABLED: # TODO: These views need to be updated before they work # url(r'^calculate$', 'util.views.calculate'), - # url(r'^gradebook$', 'courseware.views.gradebook'), # TODO: We should probably remove the circuit package. I believe it was only used in the old way of saving wiki circuits for the wiki # url(r'^edit_circuit/(?P[^/]*)$', 'circuit.views.edit_circuit'), # url(r'^save_circuit/(?P[^/]*)$', 'circuit.views.save_circuit'), @@ -139,6 +138,10 @@ if settings.COURSEWARE_ENABLED: 'courseware.views.profile', name="profile"), url(r'^courses/(?P[^/]+/[^/]+/[^/]+)/profile/(?P[^/]*)/$', 'courseware.views.profile'), + + # For the instructor + url(r'^courses/(?P[^/]+/[^/]+/[^/]+)/gradebook$', + 'courseware.views.gradebook'), ) # Multicourse wiki From 8059008bdee5a3c3f47924212005209096638c26 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Mon, 6 Aug 2012 10:13:12 -0400 Subject: [PATCH 005/163] Switch from exception logging middleware to a django signal handler, so that we no longer swallow Http404 exceptions and turn them into 500 errors --- cms/envs/common.py | 1 + common/djangoapps/util/middleware.py | 16 ---------------- lms/envs/common.py | 4 +++- 3 files changed, 4 insertions(+), 17 deletions(-) delete mode 100644 common/djangoapps/util/middleware.py diff --git a/cms/envs/common.py b/cms/envs/common.py index 896c4515a2..5ed4dcfff1 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -116,6 +116,7 @@ MIDDLEWARE_CLASSES = ( ) ############################ SIGNAL HANDLERS ################################ +# This is imported to register the exception signal handling that logs exceptions import monitoring.exceptions # noqa ############################ DJANGO_BUILTINS ################################ diff --git a/common/djangoapps/util/middleware.py b/common/djangoapps/util/middleware.py deleted file mode 100644 index eeffa2668c..0000000000 --- a/common/djangoapps/util/middleware.py +++ /dev/null @@ -1,16 +0,0 @@ -import logging - -from django.conf import settings -from django.http import HttpResponseServerError - -log = logging.getLogger("mitx") - -class ExceptionLoggingMiddleware(object): - """Just here to log unchecked exceptions that go all the way up the Django - stack""" - - if not settings.TEMPLATE_DEBUG: - def process_exception(self, request, exception): - log.exception(exception) - return HttpResponseServerError("Server Error - Please try again later.") - diff --git a/lms/envs/common.py b/lms/envs/common.py index 8475cfa9a9..2d631407a7 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -151,6 +151,9 @@ MODULESTORE = { } } +############################ SIGNAL HANDLERS ################################ +# This is imported to register the exception signal handling that logs exceptions +import monitoring.exceptions # noqa ############################### DJANGO BUILT-INS ############################### # Change DEBUG/TEMPLATE_DEBUG in your environment settings files, not here @@ -262,7 +265,6 @@ TEMPLATE_LOADERS = ( ) MIDDLEWARE_CLASSES = ( - 'util.middleware.ExceptionLoggingMiddleware', 'django.middleware.common.CommonMiddleware', 'django.contrib.sessions.middleware.SessionMiddleware', 'django.middleware.csrf.CsrfViewMiddleware', From 7f5246f65323f79336be33992f76044b594a61b2 Mon Sep 17 00:00:00 2001 From: Bridger Maxwell Date: Mon, 6 Aug 2012 10:17:53 -0400 Subject: [PATCH 006/163] Removed test view for grading. --- lms/djangoapps/courseware/views.py | 14 +------------- lms/urls.py | 4 ---- 2 files changed, 1 insertion(+), 17 deletions(-) diff --git a/lms/djangoapps/courseware/views.py b/lms/djangoapps/courseware/views.py index fa8245212e..4489b08ca0 100644 --- a/lms/djangoapps/courseware/views.py +++ b/lms/djangoapps/courseware/views.py @@ -34,19 +34,6 @@ log = logging.getLogger("mitx.courseware") template_imports = {'urllib': urllib} - -def test_grading(request, course_id): - course = check_course(course_id) - - sections, all_descriptors = grades.get_graded_sections(course) - student_module_cache = StudentModuleCache(request.user, descriptors=all_descriptors) - - grade_result = grades.fast_grade(request.user, request, sections, course.grader, student_module_cache) - - return HttpResponse("" + json.dumps(grade_result) + "") - - - def user_groups(user): if not user.is_authenticated(): return [] @@ -92,6 +79,7 @@ def gradebook(request, course_id): student_objects = User.objects.all()[:100] student_info = [] + #TODO: Only select students who are in the course for student in student_objects: student_module_cache = StudentModuleCache(student, descriptors=all_descriptors) diff --git a/lms/urls.py b/lms/urls.py index 10e9e750c9..d93eb03873 100644 --- a/lms/urls.py +++ b/lms/urls.py @@ -116,10 +116,6 @@ if settings.COURSEWARE_ENABLED: #About the course url(r'^courses/(?P[^/]+/[^/]+/[^/]+)/about$', 'courseware.views.course_about', name="about_course"), - - # TODO: Important. Remove this after testing! - url(r'^courses/(?P[^/]+/[^/]+/[^/]+)/grade$', - 'courseware.views.test_grading', name="info"), #Inside the course url(r'^courses/(?P[^/]+/[^/]+/[^/]+)/info$', From cbab4f1006e77c95b1ce14cb32e680d8d8820fba Mon Sep 17 00:00:00 2001 From: Bridger Maxwell Date: Mon, 6 Aug 2012 11:32:26 -0400 Subject: [PATCH 007/163] Fixed some imports on gradebook.html --- lms/templates/gradebook.html | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lms/templates/gradebook.html b/lms/templates/gradebook.html index 70f23d5a02..d4c5461916 100644 --- a/lms/templates/gradebook.html +++ b/lms/templates/gradebook.html @@ -8,6 +8,7 @@ <%block name="headextra"> + <%static:css group='course'/> @@ -54,7 +54,7 @@ data_class = "grade_" + letter_grade %> - ${ "{0:.0%}".format( percentage ) } + ${ "{0:.0%}".format( percentage ) } %for student in students: From ceeea66417832a21d1a7fa17e9e1ed8d4ee87d89 Mon Sep 17 00:00:00 2001 From: kimth Date: Tue, 7 Aug 2012 09:24:17 -0400 Subject: [PATCH 028/163] Fix overrzealous file_to_filename conversion and fix CapaProblem unit tests --- common/lib/capa/capa/util.py | 18 +++++++++++++----- common/lib/xmodule/xmodule/tests/__init__.py | 2 +- common/lib/xmodule/xmodule/x_module.py | 7 +++++-- 3 files changed, 19 insertions(+), 8 deletions(-) diff --git a/common/lib/capa/capa/util.py b/common/lib/capa/capa/util.py index 211ada4d62..98c250297d 100644 --- a/common/lib/capa/capa/util.py +++ b/common/lib/capa/capa/util.py @@ -39,10 +39,18 @@ def convert_files_to_filenames(answers): ''' new_answers = dict() for answer_id in answers.keys(): - # TODO This should be done more cleanly; however, this fixes bugs - # that were introduced with this function. - if isinstance(answers[answer_id], list): - new_answers[answer_id] = answers[answer_id] + if is_uploaded_file(answers[answer_id]): + new_answers[answer_id] = answers[answer_id].name else: - new_answers[answer_id] = unicode(answers[answer_id]) + new_answers[answer_id] = answers[answer_id] return new_answers + +def is_uploaded_file(file_to_test): + ''' + Duck typing to check if 'file_to_test' is a File object + ''' + is_file = True + for method in ['read', 'name']: + if not hasattr(file_to_test, method): + is_file = False + return is_file diff --git a/common/lib/xmodule/xmodule/tests/__init__.py b/common/lib/xmodule/xmodule/tests/__init__.py index 7b1641956c..072abb4908 100644 --- a/common/lib/xmodule/xmodule/tests/__init__.py +++ b/common/lib/xmodule/xmodule/tests/__init__.py @@ -31,7 +31,7 @@ i4xs = ModuleSystem( user=Mock(), filestore=fs.osfs.OSFS(os.path.dirname(os.path.realpath(__file__))), debug=True, - xqueue=None, # TODO FIXME + xqueue=None, is_staff=False ) diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index f013aac785..9b005d1dae 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -643,7 +643,7 @@ class ModuleSystem(object): user=None, filestore=None, debug=False, - xqueue = None, + xqueue=None, is_staff=False): ''' Create a closure around the system environment. @@ -678,7 +678,10 @@ class ModuleSystem(object): TODO (vshnayder): this will need to change once we have real user roles. ''' self.ajax_url = ajax_url - self.xqueue = xqueue + if xqueue is None: + self.xqueue = {'interface':None, 'callback_url':'/', 'default_queuename':'null'} + else: + self.xqueue = xqueue self.track_function = track_function self.filestore = filestore self.get_module = get_module From cb2fbfa54c5b16696859dd84d33226deb9887f9c Mon Sep 17 00:00:00 2001 From: kimth Date: Tue, 7 Aug 2012 09:38:36 -0400 Subject: [PATCH 029/163] Added test for answers file-to-filename conversion --- common/lib/xmodule/xmodule/tests/__init__.py | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/common/lib/xmodule/xmodule/tests/__init__.py b/common/lib/xmodule/xmodule/tests/__init__.py index 072abb4908..4c0b6fc9b3 100644 --- a/common/lib/xmodule/xmodule/tests/__init__.py +++ b/common/lib/xmodule/xmodule/tests/__init__.py @@ -15,6 +15,7 @@ import xmodule import capa.calc as calc import capa.capa_problem as lcp from capa.correctmap import CorrectMap +from capa.util import convert_files_to_filenames from xmodule import graders, x_module from xmodule.x_module import ModuleSystem from xmodule.graders import Score, aggregate_scores @@ -278,7 +279,6 @@ class StringResponseWithHintTest(unittest.TestCase): class CodeResponseTest(unittest.TestCase): ''' Test CodeResponse - ''' def test_update_score(self): problem_file = os.path.dirname(__file__) + "/test_files/coderesponse.xml" @@ -327,7 +327,18 @@ class CodeResponseTest(unittest.TestCase): self.assertFalse(test_lcp.correct_map.is_queued(answer_ids[j])) # Should be dequeued, message delivered else: self.assertTrue(test_lcp.correct_map.is_queued(answer_ids[j])) # Should be queued, message undelivered - + + def test_convert_files_to_filenames(self): + problem_file = os.path.dirname(__file__) + "/test_files/coderesponse.xml" + fp = open(problem_file) + answers_with_file = {'1_2_1': 'String-based answer', + '1_3_1': ['answer1', 'answer2', 'answer3'], + '1_4_1': fp} + answers_converted = convert_files_to_filenames(answers_with_file) + self.assertEquals(answers_converted['1_2_1'], 'String-based answer') + self.assertEquals(answers_converted['1_3_1'], ['answer1', 'answer2', 'answer3']) + self.assertEquals(answers_converted['1_4_1'], fp.name) + class ChoiceResponseTest(unittest.TestCase): From 4b2091b17326f4a7be669c6723d5b603e95a7baf Mon Sep 17 00:00:00 2001 From: ichuang Date: Tue, 7 Aug 2012 10:09:55 -0400 Subject: [PATCH 030/163] fix typo in registration button jquery --- lms/templates/portal/course_about.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lms/templates/portal/course_about.html b/lms/templates/portal/course_about.html index a3bf8dd755..36ec33607f 100644 --- a/lms/templates/portal/course_about.html +++ b/lms/templates/portal/course_about.html @@ -20,7 +20,7 @@ if(json.success) { location.href="${reverse('dashboard')}"; }else{ - $('#register_message).html("

" + json.error + "

") + $('#register_message').html("

" + json.error + "

"); } }); })(this) From d09e2261f3ba92e5a248de466c7c4ef5aa230be7 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Fri, 3 Aug 2012 14:51:19 -0400 Subject: [PATCH 031/163] New file structure--everything in own file * needed for CMS performance (can now save just an item, not whole tree) * remove split_to_file methods * simplified AttrMap logic * write each descriptor to a separate file * detect format on import and adjust appropriately. * update tests --- common/lib/xmodule/xmodule/capa_module.py | 5 - common/lib/xmodule/xmodule/html_module.py | 8 +- common/lib/xmodule/xmodule/seq_module.py | 13 -- .../lib/xmodule/xmodule/tests/test_import.py | 9 +- common/lib/xmodule/xmodule/xml_module.py | 166 +++++++++--------- 5 files changed, 93 insertions(+), 108 deletions(-) diff --git a/common/lib/xmodule/xmodule/capa_module.py b/common/lib/xmodule/xmodule/capa_module.py index a384edf234..833713fcde 100644 --- a/common/lib/xmodule/xmodule/capa_module.py +++ b/common/lib/xmodule/xmodule/capa_module.py @@ -572,8 +572,3 @@ class CapaDescriptor(RawDescriptor): 'problems/' + path[8:], path[8:], ] - - @classmethod - def split_to_file(cls, xml_object): - '''Problems always written in their own files''' - return True diff --git a/common/lib/xmodule/xmodule/html_module.py b/common/lib/xmodule/xmodule/html_module.py index 260b84278b..de45af081a 100644 --- a/common/lib/xmodule/xmodule/html_module.py +++ b/common/lib/xmodule/xmodule/html_module.py @@ -109,17 +109,11 @@ class HtmlDescriptor(XmlDescriptor, EditingDescriptor): # add more info and re-raise raise Exception(msg), None, sys.exc_info()[2] - @classmethod - def split_to_file(cls, xml_object): - '''Never include inline html''' - return True - - # TODO (vshnayder): make export put things in the right places. def definition_to_xml(self, resource_fs): '''If the contents are valid xml, write them to filename.xml. Otherwise, - write just the tag to filename.xml, and the html + write just to filename.xml, and the html string to filename.html. ''' try: diff --git a/common/lib/xmodule/xmodule/seq_module.py b/common/lib/xmodule/xmodule/seq_module.py index 5f7f41bf8d..e742a58471 100644 --- a/common/lib/xmodule/xmodule/seq_module.py +++ b/common/lib/xmodule/xmodule/seq_module.py @@ -122,16 +122,3 @@ class SequenceDescriptor(MakoModuleDescriptor, XmlDescriptor): etree.fromstring(child.export_to_xml(resource_fs))) return xml_object - @classmethod - def split_to_file(cls, xml_object): - # Note: if we end up needing subclasses, can port this logic there. - yes = ('chapter',) - no = ('course',) - - if xml_object.tag in yes: - return True - elif xml_object.tag in no: - return False - - # otherwise maybe--delegate to superclass. - return XmlDescriptor.split_to_file(xml_object) diff --git a/common/lib/xmodule/xmodule/tests/test_import.py b/common/lib/xmodule/xmodule/tests/test_import.py index 0d3e2260fb..2599a74079 100644 --- a/common/lib/xmodule/xmodule/tests/test_import.py +++ b/common/lib/xmodule/xmodule/tests/test_import.py @@ -134,7 +134,8 @@ class ImportTestCase(unittest.TestCase): resource_fs = MemoryFS() exported_xml = descriptor.export_to_xml(resource_fs) print "Exported xml:", exported_xml - root = etree.fromstring(exported_xml) - chapter_tag = root[0] - self.assertEqual(chapter_tag.tag, 'chapter') - self.assertFalse('graceperiod' in chapter_tag.attrib) + # hardcode path to child + with resource_fs.open('chapter/ch.xml') as f: + chapter_xml = etree.fromstring(f.read()) + self.assertEqual(chapter_xml.tag, 'chapter') + self.assertFalse('graceperiod' in chapter_xml.attrib) diff --git a/common/lib/xmodule/xmodule/xml_module.py b/common/lib/xmodule/xmodule/xml_module.py index 7a12ed869d..fd48439e46 100644 --- a/common/lib/xmodule/xmodule/xml_module.py +++ b/common/lib/xmodule/xmodule/xml_module.py @@ -11,22 +11,21 @@ import sys log = logging.getLogger(__name__) -_AttrMapBase = namedtuple('_AttrMap', 'metadata_key to_metadata from_metadata') +_AttrMapBase = namedtuple('_AttrMap', 'from_xml to_xml') class AttrMap(_AttrMapBase): """ - A class that specifies a metadata_key, and two functions: + A class that specifies two functions: - to_metadata: convert value from the xml representation into + from_xml: convert value from the xml representation into an internal python representation - from_metadata: convert the internal python representation into + to_xml: convert the internal python representation into the value to store in the xml. """ - def __new__(_cls, metadata_key, - to_metadata=lambda x: x, - from_metadata=lambda x: x): - return _AttrMapBase.__new__(_cls, metadata_key, to_metadata, from_metadata) + def __new__(_cls, from_xml=lambda x: x, + to_xml=lambda x: x): + return _AttrMapBase.__new__(_cls, from_xml, to_xml) class XmlDescriptor(XModuleDescriptor): @@ -39,6 +38,9 @@ class XmlDescriptor(XModuleDescriptor): # The attributes will be removed from the definition xml passed # to definition_from_xml, and from the xml returned by definition_to_xml + + # Note -- url_name isn't in this list because it's handled specially on + # import and export. metadata_attributes = ('format', 'graceperiod', 'showanswer', 'rerandomize', 'start', 'due', 'graded', 'display_name', 'url_name', 'hide_from_toc', 'ispublic', # if True, then course is listed for all users; see @@ -50,8 +52,7 @@ class XmlDescriptor(XModuleDescriptor): # to import and export them xml_attribute_map = { # type conversion: want True/False in python, "true"/"false" in xml - 'graded': AttrMap('graded', - lambda val: val == 'true', + 'graded': AttrMap(lambda val: val == 'true', lambda val: str(val).lower()), } @@ -101,6 +102,24 @@ class XmlDescriptor(XModuleDescriptor): """ return etree.parse(file_object).getroot() + @classmethod + def load_file(cls, filepath, fs, location): + ''' + Open the specified file in fs, and call cls.file_to_xml on it, + returning the lxml object. + + Add details and reraise on error. + ''' + try: + with fs.open(filepath) as file: + return cls.file_to_xml(file) + except Exception as err: + # Add info about where we are, but keep the traceback + msg = 'Unable to load file contents at path %s for item %s: %s ' % ( + filepath, location.url(), str(err)) + raise Exception, msg, sys.exc_info()[2] + + @classmethod def load_definition(cls, xml_object, system, location): '''Load a descriptor definition from the specified xml_object. @@ -128,14 +147,7 @@ class XmlDescriptor(XModuleDescriptor): filepath = candidate break - try: - with system.resources_fs.open(filepath) as file: - definition_xml = cls.file_to_xml(file) - except Exception: - msg = 'Unable to load file contents at path %s for item %s' % ( - filepath, location.url()) - # Add info about where we are, but keep the traceback - raise Exception, msg, sys.exc_info()[2] + definition_xml = cls.load_file(filepath, system.resources_fs, location) cls.clean_metadata_from_xml(definition_xml) definition = cls.definition_from_xml(definition_xml, system) @@ -146,6 +158,24 @@ class XmlDescriptor(XModuleDescriptor): return definition + @classmethod + def load_metadata(cls, xml_object): + """ + Read the metadata attributes from this xml_object. + + Returns a dictionary {key: value}. + """ + metadata = {} + for attr in cls.metadata_attributes: + val = xml_object.get(attr) + if val is not None: + # VS[compat]. Remove after all key translations done + attr = cls._translate(attr) + + attr_map = cls.xml_attribute_map.get(attr, AttrMap()) + metadata[attr] = attr_map.from_xml(val) + return metadata + @classmethod def from_xml(cls, xml_data, system, org=None, course=None): @@ -161,25 +191,20 @@ class XmlDescriptor(XModuleDescriptor): """ xml_object = etree.fromstring(xml_data) # VS[compat] -- just have the url_name lookup once translation is done - slug = xml_object.get('url_name', xml_object.get('slug')) - location = Location('i4x', org, course, xml_object.tag, slug) + url_name = xml_object.get('url_name', xml_object.get('slug')) + location = Location('i4x', org, course, xml_object.tag, url_name) - def load_metadata(): - metadata = {} - for attr in cls.metadata_attributes: - val = xml_object.get(attr) - if val is not None: - # VS[compat]. Remove after all key translations done - attr = cls._translate(attr) + # VS[compat] -- detect new-style each-in-a-file mode + if len(xml_object.attrib.keys()) == 1 and len(xml_object) == 0: + # new style: this is just a pointer. + # read the actual defition file--named using url_name + filepath = cls._format_filepath(xml_object.tag, url_name) + definition_xml = cls.load_file(filepath, system.resources_fs, location) + else: + definition_xml = xml_object - attr_map = cls.xml_attribute_map.get(attr, AttrMap(attr)) - metadata[attr_map.metadata_key] = attr_map.to_metadata(val) - return metadata - - definition = cls.load_definition(xml_object, system, location) - metadata = load_metadata() - # VS[compat] -- just have the url_name lookup once translation is done - slug = xml_object.get('url_name', xml_object.get('slug')) + definition = cls.load_definition(definition_xml, system, location) + metadata = cls.load_metadata(definition_xml) return cls( system, definition, @@ -193,20 +218,6 @@ class XmlDescriptor(XModuleDescriptor): name=name, ext=cls.filename_extension) - @classmethod - def split_to_file(cls, xml_object): - ''' - Decide whether to write this object to a separate file or not. - - xml_object: an xml definition of an instance of cls. - - This default implementation will split if this has more than 7 - descendant tags. - - Can be overridden by subclasses. - ''' - return len(list(xml_object.iter())) > 7 - def export_to_xml(self, resource_fs): """ Returns an xml string representing this module, and all modules @@ -227,42 +238,39 @@ class XmlDescriptor(XModuleDescriptor): xml_object = self.definition_to_xml(resource_fs) self.__class__.clean_metadata_from_xml(xml_object) - # Set the tag first, so it's right if writing to a file + # Set the tag so we get the file path right xml_object.tag = self.category - # Write it to a file if necessary - if self.split_to_file(xml_object): - # Put this object in its own file - filepath = self.__class__._format_filepath(self.category, self.url_name) - resource_fs.makedir(os.path.dirname(filepath), allow_recreate=True) - with resource_fs.open(filepath, 'w') as file: - file.write(etree.tostring(xml_object, pretty_print=True)) - # ...and remove all of its children here - for child in xml_object: - xml_object.remove(child) - # also need to remove the text of this object. - xml_object.text = '' - # and the tail for good measure... - xml_object.tail = '' + 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.xml_attribute_map.get(attr, AttrMap()) + return attr_map.to_xml(self.own_metadata[attr]) - - xml_object.set('filename', self.url_name) - - # Add the metadata - xml_object.set('url_name', self.url_name) - for attr in self.metadata_attributes: - attr_map = self.xml_attribute_map.get(attr, AttrMap(attr)) - metadata_key = attr_map.metadata_key - - if (metadata_key not in self.metadata or - metadata_key in self._inherited_metadata): + # Add the non-inherited metadata + for attr in self.own_metadata: + if attr not in self.metadata_attributes: + log.warning("Unexpected metadata '{attr}' on element '{url}'." + " Not exporting.".format(attr=attr, + url=self.location.url())) continue - val = attr_map.from_metadata(self.metadata[metadata_key]) - xml_object.set(attr, val) + xml_object.set(attr, val_for_xml(attr)) - # Now we just have to make it beautiful - return etree.tostring(xml_object, pretty_print=True) + + # Write the actual contents to a file + filepath = self.__class__._format_filepath(self.category, self.url_name) + resource_fs.makedir(os.path.dirname(filepath), allow_recreate=True) + + with resource_fs.open(filepath, 'w') as file: + file.write(etree.tostring(xml_object, pretty_print=True)) + + # And return just a pointer with the category and filename. + record_object = etree.Element(self.category) + record_object.set('url_name', self.url_name) + + return etree.tostring(record_object, pretty_print=True) def definition_to_xml(self, resource_fs): """ From b285f50d92f02b197f16c0ee448ae26436c73397 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Fri, 3 Aug 2012 15:28:48 -0400 Subject: [PATCH 032/163] Make unknown metadata persist accross import-export * +improve test. --- .../lib/xmodule/xmodule/tests/test_import.py | 40 ++++++++++++++----- common/lib/xmodule/xmodule/xml_module.py | 17 ++++---- 2 files changed, 39 insertions(+), 18 deletions(-) diff --git a/common/lib/xmodule/xmodule/tests/test_import.py b/common/lib/xmodule/xmodule/tests/test_import.py index 2599a74079..45a142b8fe 100644 --- a/common/lib/xmodule/xmodule/tests/test_import.py +++ b/common/lib/xmodule/xmodule/tests/test_import.py @@ -111,30 +111,50 @@ class ImportTestCase(unittest.TestCase): xml_out = etree.fromstring(xml_str_out) self.assertEqual(xml_out.tag, 'sequential') - def test_metadata_inherit(self): - """Make sure metadata inherits properly""" + def test_metadata_import_export(self): + """Two checks: + - unknown metadata is preserved across import-export + - inherited metadata doesn't leak to children. + """ system = self.get_system() v = "1 hour" - start_xml = ''' - - Two houses, ... - '''.format(grace=v) + start_xml = ''' + + + Two houses, ... + + '''.format(grace=v) descriptor = XModuleDescriptor.load_from_xml(start_xml, system, 'org', 'course') - print "Errors: {0}".format(system.errorlog.errors) print descriptor, descriptor.metadata self.assertEqual(descriptor.metadata['graceperiod'], v) + self.assertEqual(descriptor.metadata['unicorn'], 'purple') - # Check that the child inherits correctly + # Check that the child inherits graceperiod correctly child = descriptor.get_children()[0] self.assertEqual(child.metadata['graceperiod'], v) - # Now export and see if the chapter tag has a graceperiod attribute + # check that the child does _not_ inherit any unicorns + self.assertTrue('unicorn' not in child.metadata) + + # Now export and check things resource_fs = MemoryFS() exported_xml = descriptor.export_to_xml(resource_fs) print "Exported xml:", exported_xml - # hardcode path to child + + # Does the course still have unicorns? + # hardcoded path to course + with resource_fs.open('course/test1.xml') as f: + course_xml = etree.fromstring(f.read()) + + self.assertEqual(course_xml.attrib['unicorn'], 'purple') + + # did we successfully strip the url_name from the definition contents? + self.assertTrue('url_name' not in course_xml.attrib) + + # Does the chapter tag now have a graceperiod attribute? + # hardcoded path to child with resource_fs.open('chapter/ch.xml') as f: chapter_xml = etree.fromstring(f.read()) self.assertEqual(chapter_xml.tag, 'chapter') diff --git a/common/lib/xmodule/xmodule/xml_module.py b/common/lib/xmodule/xmodule/xml_module.py index fd48439e46..907fc302a3 100644 --- a/common/lib/xmodule/xmodule/xml_module.py +++ b/common/lib/xmodule/xmodule/xml_module.py @@ -47,6 +47,10 @@ class XmlDescriptor(XModuleDescriptor): # VS[compat] Remove once unused. 'name', 'slug') + # VS[compat] -- remove once everything is in the CMS + # We don't want url_name in the metadata--it's in the location, so avoid + # confusion and duplication. + metadata_to_strip = ('url_name', ) # A dictionary mapping xml attribute names AttrMaps that describe how # to import and export them @@ -166,12 +170,16 @@ class XmlDescriptor(XModuleDescriptor): Returns a dictionary {key: value}. """ metadata = {} - for attr in cls.metadata_attributes: + 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) + if attr in cls.metadata_to_strip: + # don't load these + continue + attr_map = cls.xml_attribute_map.get(attr, AttrMap()) metadata[attr] = attr_map.from_xml(val) return metadata @@ -250,15 +258,8 @@ class XmlDescriptor(XModuleDescriptor): # Add the non-inherited metadata for attr in self.own_metadata: - if attr not in self.metadata_attributes: - log.warning("Unexpected metadata '{attr}' on element '{url}'." - " Not exporting.".format(attr=attr, - url=self.location.url())) - continue - xml_object.set(attr, val_for_xml(attr)) - # Write the actual contents to a file filepath = self.__class__._format_filepath(self.category, self.url_name) resource_fs.makedir(os.path.dirname(filepath), allow_recreate=True) From 6ed905275522272c2988211201913177d33bd9ad Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Fri, 3 Aug 2012 18:16:54 -0400 Subject: [PATCH 033/163] minor cleanups --- common/lib/xmodule/xmodule/abtest_module.py | 8 ++++++-- common/lib/xmodule/xmodule/course_module.py | 2 +- common/lib/xmodule/xmodule/modulestore/xml.py | 9 +++++++-- common/lib/xmodule/xmodule/x_module.py | 7 ++++--- 4 files changed, 18 insertions(+), 8 deletions(-) diff --git a/common/lib/xmodule/xmodule/abtest_module.py b/common/lib/xmodule/xmodule/abtest_module.py index 8d6604b06b..9d4744e2c9 100644 --- a/common/lib/xmodule/xmodule/abtest_module.py +++ b/common/lib/xmodule/xmodule/abtest_module.py @@ -103,7 +103,9 @@ class ABTestDescriptor(RawDescriptor, XmlDescriptor): experiment = xml_object.get('experiment') if experiment is None: - raise InvalidDefinitionError("ABTests must specify an experiment. Not found in:\n{xml}".format(xml=etree.tostring(xml_object, pretty_print=True))) + raise InvalidDefinitionError( + "ABTests must specify an experiment. Not found in:\n{xml}" + .format(xml=etree.tostring(xml_object, pretty_print=True))) definition = { 'data': { @@ -127,7 +129,9 @@ class ABTestDescriptor(RawDescriptor, XmlDescriptor): definition['data']['group_content'][name] = child_content_urls definition['children'].extend(child_content_urls) - default_portion = 1 - sum(portion for (name, portion) in definition['data']['group_portions'].items()) + default_portion = 1 - sum( + portion for (name, portion) in definition['data']['group_portions'].items()) + if default_portion < 0: raise InvalidDefinitionError("ABTest portions must add up to less than or equal to 1") diff --git a/common/lib/xmodule/xmodule/course_module.py b/common/lib/xmodule/xmodule/course_module.py index 7f4c204f45..40294fa786 100644 --- a/common/lib/xmodule/xmodule/course_module.py +++ b/common/lib/xmodule/xmodule/course_module.py @@ -37,7 +37,7 @@ class CourseDescriptor(SequenceDescriptor): def has_started(self): return time.gmtime() > self.start - + @property def grader(self): return self.__grading_policy['GRADER'] diff --git a/common/lib/xmodule/xmodule/modulestore/xml.py b/common/lib/xmodule/xmodule/modulestore/xml.py index 46fcf19469..f9093f355a 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml.py +++ b/common/lib/xmodule/xmodule/modulestore/xml.py @@ -188,21 +188,26 @@ class XMLModuleStore(ModuleStoreBase): course_file = StringIO(clean_out_mako_templating(course_file.read())) course_data = etree.parse(course_file).getroot() + org = course_data.get('org') if org is None: - log.error("No 'org' attribute set for course in {dir}. " + msg = ("No 'org' attribute set for course in {dir}. " "Using default 'edx'".format(dir=course_dir)) + log.error(msg) + tracker(msg) org = 'edx' course = course_data.get('course') if course is None: - log.error("No 'course' attribute set for course in {dir}." + msg = ("No 'course' attribute set for course in {dir}." " Using default '{default}'".format( dir=course_dir, default=course_dir )) + log.error(msg) + tracker(msg) course = course_dir system = ImportSystem(self, org, course, course_dir, tracker) diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index 9b005d1dae..818e0cd8fd 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -6,6 +6,7 @@ from fs.errors import ResourceNotFoundError from functools import partial from lxml import etree from lxml.etree import XMLSyntaxError +from pprint import pprint from xmodule.modulestore import Location from xmodule.errortracker import exc_info_to_str @@ -550,9 +551,9 @@ class XModuleDescriptor(Plugin, HTMLSnippet): if not eq: for attr in self.equality_attributes: - print(getattr(self, attr, None), - getattr(other, attr, None), - getattr(self, attr, None) == getattr(other, attr, None)) + pprint((getattr(self, attr, None), + getattr(other, attr, None), + getattr(self, attr, None) == getattr(other, attr, None))) return eq From b091dcabe0f682695b8cbcf6030051c4cee0c8c6 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Fri, 3 Aug 2012 18:20:06 -0400 Subject: [PATCH 034/163] metadata and file format cleanups * course.xml is special--has org and course attributes in addition to url_name * strip data_dir from metadata on export * more asserts * work on roundtrip import-export test --- common/lib/xmodule/xmodule/course_module.py | 1 - .../lib/xmodule/xmodule/tests/test_export.py | 68 ++++++++++++++++--- .../lib/xmodule/xmodule/tests/test_import.py | 27 ++++++-- common/lib/xmodule/xmodule/xml_module.py | 55 ++++++++++++--- 4 files changed, 124 insertions(+), 27 deletions(-) diff --git a/common/lib/xmodule/xmodule/course_module.py b/common/lib/xmodule/xmodule/course_module.py index 40294fa786..41ed5ab89a 100644 --- a/common/lib/xmodule/xmodule/course_module.py +++ b/common/lib/xmodule/xmodule/course_module.py @@ -13,7 +13,6 @@ log = logging.getLogger(__name__) class CourseDescriptor(SequenceDescriptor): module_class = SequenceModule - metadata_attributes = SequenceDescriptor.metadata_attributes + ('org', 'course') def __init__(self, system, definition=None, **kwargs): super(CourseDescriptor, self).__init__(system, definition, **kwargs) diff --git a/common/lib/xmodule/xmodule/tests/test_export.py b/common/lib/xmodule/xmodule/tests/test_export.py index eacf8352be..e1e48cc796 100644 --- a/common/lib/xmodule/xmodule/tests/test_export.py +++ b/common/lib/xmodule/xmodule/tests/test_export.py @@ -1,24 +1,72 @@ -from xmodule.modulestore.xml import XMLModuleStore -from nose.tools import assert_equals -from nose import SkipTest -from tempfile import mkdtemp from fs.osfs import OSFS +from nose.tools import assert_equals, assert_true +from nose import SkipTest +from path import path +from tempfile import mkdtemp + +from xmodule.modulestore.xml import XMLModuleStore + +# from ~/mitx_all/mitx/common/lib/xmodule/xmodule/tests/ +# to ~/mitx_all/mitx/common/test +TEST_DIR = path(__file__).abspath().dirname() +for i in range(4): + TEST_DIR = TEST_DIR.dirname() +TEST_DIR = TEST_DIR / 'test' + +DATA_DIR = TEST_DIR / 'data' -def check_export_roundtrip(data_dir): +def strip_metadata(descriptor, key): + """ + HACK: data_dir metadata tags break equality because they aren't real metadata + remove them. + + Recursively strips same tag from all children. + """ + print "strip {key} from {desc}".format(key=key, desc=descriptor.location.url()) + descriptor.metadata.pop(key, None) + for d in descriptor.get_children(): + strip_metadata(d, key) + +def check_gone(descriptor, key): + '''Make sure that the metadata of this descriptor or any + descendants does not include key''' + assert_true(key not in descriptor.metadata) + for d in descriptor.get_children(): + check_gone(d, key) + +def check_export_roundtrip(data_dir, course_dir): print "Starting import" - initial_import = XMLModuleStore('org', 'course', data_dir, eager=True) - initial_course = initial_import.course + initial_import = XMLModuleStore(data_dir, eager=True, course_dirs=[course_dir]) + + courses = initial_import.get_courses() + assert_equals(len(courses), 1) + initial_course = courses[0] print "Starting export" export_dir = mkdtemp() + print "export_dir: {0}".format(export_dir) fs = OSFS(export_dir) - xml = initial_course.export_to_xml(fs) - with fs.open('course.xml', 'w') as course_xml: + export_course_dir = 'export' + export_fs = fs.makeopendir(export_course_dir) + + xml = initial_course.export_to_xml(export_fs) + with export_fs.open('course.xml', 'w') as course_xml: course_xml.write(xml) print "Starting second import" - second_import = XMLModuleStore('org', 'course', export_dir, eager=True) + second_import = XMLModuleStore(export_dir, eager=True, + course_dirs=[export_course_dir]) + + courses2 = second_import.get_courses() + assert_equals(len(courses2), 1) + exported_course = courses2[0] + + print "Checking course equality" + strip_metadata(initial_course, 'data_dir') + strip_metadata(exported_course, 'data_dir') + + assert_equals(initial_course, exported_course) print "Checking key equality" assert_equals(initial_import.modules.keys(), second_import.modules.keys()) diff --git a/common/lib/xmodule/xmodule/tests/test_import.py b/common/lib/xmodule/xmodule/tests/test_import.py index 45a142b8fe..d2080e1bf7 100644 --- a/common/lib/xmodule/xmodule/tests/test_import.py +++ b/common/lib/xmodule/xmodule/tests/test_import.py @@ -5,6 +5,7 @@ from fs.memoryfs import MemoryFS from lxml import etree from xmodule.x_module import XMLParsingSystem, XModuleDescriptor +from xmodule.xml_module import is_pointer_tag from xmodule.errortracker import make_error_tracker from xmodule.modulestore import Location from xmodule.modulestore.exceptions import ItemNotFoundError @@ -117,15 +118,19 @@ class ImportTestCase(unittest.TestCase): - inherited metadata doesn't leak to children. """ system = self.get_system() - v = "1 hour" + v = '1 hour' + org = 'foo' + course = 'bbhh' + url_name = 'test1' start_xml = ''' - + Two houses, ... - '''.format(grace=v) + '''.format(grace=v, org=org, course=course, url_name=url_name) descriptor = XModuleDescriptor.load_from_xml(start_xml, system, - 'org', 'course') + org, course) print descriptor, descriptor.metadata self.assertEqual(descriptor.metadata['graceperiod'], v) @@ -141,15 +146,25 @@ class ImportTestCase(unittest.TestCase): # Now export and check things resource_fs = MemoryFS() exported_xml = descriptor.export_to_xml(resource_fs) + + # Check that the exported xml is just a pointer print "Exported xml:", exported_xml + pointer = etree.fromstring(exported_xml) + self.assertTrue(is_pointer_tag(pointer)) + # but it's a special case course pointer + self.assertEqual(pointer.attrib['course'], course) + self.assertEqual(pointer.attrib['org'], org) # Does the course still have unicorns? - # hardcoded path to course - with resource_fs.open('course/test1.xml') as f: + with resource_fs.open('course/{url_name}.xml'.format(url_name=url_name)) as f: course_xml = etree.fromstring(f.read()) self.assertEqual(course_xml.attrib['unicorn'], 'purple') + # the course and org tags should be _only_ in the pointer + self.assertTrue('course' not in course_xml.attrib) + self.assertTrue('org' not in course_xml.attrib) + # did we successfully strip the url_name from the definition contents? self.assertTrue('url_name' not in course_xml.attrib) diff --git a/common/lib/xmodule/xmodule/xml_module.py b/common/lib/xmodule/xmodule/xml_module.py index 907fc302a3..cb0d05a83e 100644 --- a/common/lib/xmodule/xmodule/xml_module.py +++ b/common/lib/xmodule/xmodule/xml_module.py @@ -11,6 +11,29 @@ import sys log = logging.getLogger(__name__) + +def is_pointer_tag(xml_obj): + """ + Check if xml_obj is a pointer tag: . + No children, one attribute named url_name. + + Special case for course roots: the pointer is + + + xml_obj: an etree Element + + Returns a bool. + """ + if xml_obj.tag != "course": + expected_attr = set(['url_name']) + else: + expected_attr = set(['url_name', 'course', 'org']) + + actual_attr = set(xml_obj.attrib.keys()) + return len(xml_obj) == 0 and actual_attr == expected_attr + + + _AttrMapBase = namedtuple('_AttrMap', 'from_xml to_xml') class AttrMap(_AttrMapBase): @@ -41,16 +64,19 @@ class XmlDescriptor(XModuleDescriptor): # Note -- url_name isn't in this list because it's handled specially on # import and export. + + # TODO (vshnayder): Do we need a list of metadata we actually + # understand? And if we do, is this the place? + # Related: What's the right behavior for clean_metadata? metadata_attributes = ('format', 'graceperiod', 'showanswer', 'rerandomize', 'start', 'due', 'graded', 'display_name', 'url_name', 'hide_from_toc', 'ispublic', # if True, then course is listed for all users; see # VS[compat] Remove once unused. 'name', 'slug') - # VS[compat] -- remove once everything is in the CMS - # We don't want url_name in the metadata--it's in the location, so avoid - # confusion and duplication. - metadata_to_strip = ('url_name', ) + metadata_to_strip = ('data_dir', + # VS[compat] -- remove the below attrs once everything is in the CMS + 'course', 'org', 'url_name') # A dictionary mapping xml attribute names AttrMaps that describe how # to import and export them @@ -130,6 +156,8 @@ class XmlDescriptor(XModuleDescriptor): Subclasses should not need to override this except in special cases (e.g. html module)''' + # VS[compat] -- the filename tag should go away once everything is + # converted. (note: make sure html files still work once this goes away) filename = xml_object.get('filename') if filename is None: definition_xml = copy.deepcopy(xml_object) @@ -198,13 +226,13 @@ class XmlDescriptor(XModuleDescriptor): url identifiers """ xml_object = etree.fromstring(xml_data) - # VS[compat] -- just have the url_name lookup once translation is done + # VS[compat] -- just have the url_name lookup, once translation is done url_name = xml_object.get('url_name', xml_object.get('slug')) location = Location('i4x', org, course, xml_object.tag, url_name) # VS[compat] -- detect new-style each-in-a-file mode - if len(xml_object.attrib.keys()) == 1 and len(xml_object) == 0: - # new style: this is just a pointer. + if is_pointer_tag(xml_object): + # new style: # read the actual defition file--named using url_name filepath = cls._format_filepath(xml_object.tag, url_name) definition_xml = cls.load_file(filepath, system.resources_fs, location) @@ -258,12 +286,13 @@ class XmlDescriptor(XModuleDescriptor): # Add the non-inherited metadata for attr in self.own_metadata: - xml_object.set(attr, val_for_xml(attr)) + # don't want e.g. data_dir + if attr not in self.metadata_to_strip: + xml_object.set(attr, val_for_xml(attr)) - # Write the actual contents to a file + # Write the definition to a file filepath = self.__class__._format_filepath(self.category, self.url_name) resource_fs.makedir(os.path.dirname(filepath), allow_recreate=True) - with resource_fs.open(filepath, 'w') as file: file.write(etree.tostring(xml_object, pretty_print=True)) @@ -271,6 +300,12 @@ class XmlDescriptor(XModuleDescriptor): record_object = etree.Element(self.category) record_object.set('url_name', self.url_name) + # Special case for course pointers: + if self.category == 'course': + # add org and course attributes on the pointer tag + record_object.set('org', self.location.org) + record_object.set('course', self.location.course) + return etree.tostring(record_object, pretty_print=True) def definition_to_xml(self, resource_fs): From e6e290f525123cadcffd3d89660571eba47f0f5f Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Fri, 3 Aug 2012 19:03:19 -0400 Subject: [PATCH 035/163] Make initial import-export tests pass. TODO: * need unique slugs for errors so they don't overwrite each other on export. - try to preserve origin slug. If not possible, generate random one. * figure out what metadata to strip. e.g. ({'data': '

Finger Exercise 1...'}, {'data': '

Finger Exercise 1...'}, False) - where did points and type come from? Do we want them there? * separate broken and non-broken test courses --- common/lib/xmodule/xmodule/html_module.py | 11 +- .../lib/xmodule/xmodule/tests/test_export.py | 114 ++++++++++-------- .../lib/xmodule/xmodule/tests/test_import.py | 2 - common/lib/xmodule/xmodule/xml_module.py | 2 +- common/test/data/simple/course.xml | 2 +- .../{problems => problem}/L1_Problem_1.xml | 0 .../{problems => problem}/ps01-simple.xml | 0 7 files changed, 72 insertions(+), 59 deletions(-) rename common/test/data/simple/{problems => problem}/L1_Problem_1.xml (100%) rename common/test/data/simple/{problems => problem}/ps01-simple.xml (100%) diff --git a/common/lib/xmodule/xmodule/html_module.py b/common/lib/xmodule/xmodule/html_module.py index de45af081a..89dbfb5fc0 100644 --- a/common/lib/xmodule/xmodule/html_module.py +++ b/common/lib/xmodule/xmodule/html_module.py @@ -36,7 +36,6 @@ class HtmlDescriptor(XmlDescriptor, EditingDescriptor): # are being edited in the cms @classmethod def backcompat_paths(cls, path): - origpath = path if path.endswith('.html.xml'): path = path[:-9] + '.html' #backcompat--look for html instead of xml candidates = [] @@ -45,9 +44,11 @@ class HtmlDescriptor(XmlDescriptor, EditingDescriptor): _, _, path = path.partition(os.sep) # also look for .html versions instead of .xml - if origpath.endswith('.xml'): - candidates.append(origpath[:-4] + '.html') - return candidates + nc = [] + for candidate in candidates: + if candidate.endswith('.xml'): + nc.append(candidate[:-4] + '.html') + return candidates + nc # NOTE: html descriptors are special. We do not want to parse and # export them ourselves, because that can break things (e.g. lxml @@ -80,7 +81,7 @@ class HtmlDescriptor(XmlDescriptor, EditingDescriptor): # online and has imported all current (fall 2012) courses from xml if not system.resources_fs.exists(filepath): candidates = cls.backcompat_paths(filepath) - #log.debug("candidates = {0}".format(candidates)) + log.debug("candidates = {0}".format(candidates)) for candidate in candidates: if system.resources_fs.exists(candidate): filepath = candidate diff --git a/common/lib/xmodule/xmodule/tests/test_export.py b/common/lib/xmodule/xmodule/tests/test_export.py index e1e48cc796..7358d89dde 100644 --- a/common/lib/xmodule/xmodule/tests/test_export.py +++ b/common/lib/xmodule/xmodule/tests/test_export.py @@ -1,6 +1,7 @@ +import unittest + from fs.osfs import OSFS from nose.tools import assert_equals, assert_true -from nose import SkipTest from path import path from tempfile import mkdtemp @@ -18,10 +19,7 @@ DATA_DIR = TEST_DIR / 'data' def strip_metadata(descriptor, key): """ - HACK: data_dir metadata tags break equality because they aren't real metadata - remove them. - - Recursively strips same tag from all children. + Recursively strips tag from all children. """ print "strip {key} from {desc}".format(key=key, desc=descriptor.location.url()) descriptor.metadata.pop(key, None) @@ -35,50 +33,66 @@ def check_gone(descriptor, key): for d in descriptor.get_children(): check_gone(d, key) -def check_export_roundtrip(data_dir, course_dir): - print "Starting import" - initial_import = XMLModuleStore(data_dir, eager=True, course_dirs=[course_dir]) - - courses = initial_import.get_courses() - assert_equals(len(courses), 1) - initial_course = courses[0] - - print "Starting export" - export_dir = mkdtemp() - print "export_dir: {0}".format(export_dir) - fs = OSFS(export_dir) - export_course_dir = 'export' - export_fs = fs.makeopendir(export_course_dir) - - xml = initial_course.export_to_xml(export_fs) - with export_fs.open('course.xml', 'w') as course_xml: - course_xml.write(xml) - - print "Starting second import" - second_import = XMLModuleStore(export_dir, eager=True, - course_dirs=[export_course_dir]) - - courses2 = second_import.get_courses() - assert_equals(len(courses2), 1) - exported_course = courses2[0] - - print "Checking course equality" - strip_metadata(initial_course, 'data_dir') - strip_metadata(exported_course, 'data_dir') - - assert_equals(initial_course, exported_course) - - print "Checking key equality" - assert_equals(initial_import.modules.keys(), second_import.modules.keys()) - - print "Checking module equality" - for location in initial_import.modules.keys(): - print "Checking", location - assert_equals(initial_import.modules[location], second_import.modules[location]) -def test_toy_roundtrip(): - dir = "" - # TODO: add paths and make this run. - raise SkipTest() - check_export_roundtrip(dir) +class RoundTripTestCase(unittest.TestCase): + '''Check that our test courses roundtrip properly''' + def check_export_roundtrip(self, data_dir, course_dir): + print "Starting import" + initial_import = XMLModuleStore(data_dir, eager=True, course_dirs=[course_dir]) + + courses = initial_import.get_courses() + self.assertEquals(len(courses), 1) + initial_course = courses[0] + + print "Starting export" + export_dir = mkdtemp() + print "export_dir: {0}".format(export_dir) + fs = OSFS(export_dir) + export_course_dir = 'export' + export_fs = fs.makeopendir(export_course_dir) + + xml = initial_course.export_to_xml(export_fs) + with export_fs.open('course.xml', 'w') as course_xml: + course_xml.write(xml) + + print "Starting second import" + second_import = XMLModuleStore(export_dir, eager=True, + course_dirs=[export_course_dir]) + + courses2 = second_import.get_courses() + self.assertEquals(len(courses2), 1) + exported_course = courses2[0] + + print "Checking course equality" + # HACK: data_dir metadata tags break equality because they + # aren't real metadata, and depend on paths. Remove them. + strip_metadata(initial_course, 'data_dir') + strip_metadata(exported_course, 'data_dir') + + self.assertEquals(initial_course, exported_course) + + print "Checking key equality" + self.assertEquals(sorted(initial_import.modules.keys()), + sorted(second_import.modules.keys())) + + print "Checking module equality" + for location in initial_import.modules.keys(): + print "Checking", location + if location.category == 'html': + print ("Skipping html modules--they can't import in" + " final form without writing files...") + continue + self.assertEquals(initial_import.modules[location], + second_import.modules[location]) + + + def setUp(self): + self.maxDiff = None + + def test_toy_roundtrip(self): + self.check_export_roundtrip(DATA_DIR, "toy") + + + def test_full_roundtrip(self): + self.check_export_roundtrip(DATA_DIR, "full") diff --git a/common/lib/xmodule/xmodule/tests/test_import.py b/common/lib/xmodule/xmodule/tests/test_import.py index d2080e1bf7..af7464cfdc 100644 --- a/common/lib/xmodule/xmodule/tests/test_import.py +++ b/common/lib/xmodule/xmodule/tests/test_import.py @@ -51,8 +51,6 @@ class DummySystem(XMLParsingSystem): class ImportTestCase(unittest.TestCase): '''Make sure module imports work properly, including for malformed inputs''' - - @staticmethod def get_system(): '''Get a dummy system''' diff --git a/common/lib/xmodule/xmodule/xml_module.py b/common/lib/xmodule/xmodule/xml_module.py index cb0d05a83e..1318270def 100644 --- a/common/lib/xmodule/xmodule/xml_module.py +++ b/common/lib/xmodule/xmodule/xml_module.py @@ -76,7 +76,7 @@ class XmlDescriptor(XModuleDescriptor): metadata_to_strip = ('data_dir', # VS[compat] -- remove the below attrs once everything is in the CMS - 'course', 'org', 'url_name') + 'course', 'org', 'url_name', 'filename') # A dictionary mapping xml attribute names AttrMaps that describe how # to import and export them diff --git a/common/test/data/simple/course.xml b/common/test/data/simple/course.xml index b92158cdb7..0ecc153724 100644 --- a/common/test/data/simple/course.xml +++ b/common/test/data/simple/course.xml @@ -2,7 +2,7 @@