From 3baf92a84963029e41c8ca61a1a7d7a2c756d1fe Mon Sep 17 00:00:00 2001 From: Bridger Maxwell Date: Sat, 8 Sep 2012 00:20:47 -0400 Subject: [PATCH 01/17] Changed course grading to avoid initing capa modules. --- lms/djangoapps/courseware/grades.py | 46 ++++++++++++++--------------- 1 file changed, 23 insertions(+), 23 deletions(-) diff --git a/lms/djangoapps/courseware/grades.py b/lms/djangoapps/courseware/grades.py index 2243caaab6..cac4f7f682 100644 --- a/lms/djangoapps/courseware/grades.py +++ b/lms/djangoapps/courseware/grades.py @@ -149,8 +149,13 @@ def grade(student, request, course, student_module_cache=None, keep_raw_scores=F # 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(course.id, student, module, student_module_cache) + module_locations = section_module.definition.get('children', []) + + + for module_location in module_locations: + module_descriptor = section_module.descriptor.system.load_item(module_location) + + (correct, total) = get_score(course.id, student, module_descriptor, section_module.system, student_module_cache) if correct is None and total is None: continue @@ -160,12 +165,12 @@ def grade(student, request, course, student_module_cache=None, keep_raw_scores=F else: correct = total - graded = module.metadata.get("graded", False) + graded = module_descriptor.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 - scores.append(Score(correct, total, graded, module.metadata.get('display_name'))) + scores.append(Score(correct, total, graded, module_descriptor.metadata.get('display_name'))) section_total, graded_total = graders.aggregate_scores(scores, section_name) if keep_raw_scores: @@ -252,7 +257,8 @@ def progress_summary(student, course, grader, student_module_cache): for module in yield_module_descendents(s): # course is a module, not a descriptor... course_id = course.descriptor.id - (correct, total) = get_score(course_id, student, module, student_module_cache) + # TODO: This instantiates the problem TWICE! That is no good. + (correct, total) = get_score(course_id, student, module.descriptor, module.system, student_module_cache) if correct is None and total is None: continue @@ -281,7 +287,7 @@ def progress_summary(student, course, grader, student_module_cache): return chapters -def get_score(course_id, user, problem, student_module_cache): +def get_score(course_id, user, problem_descriptor, xmodule_system, student_module_cache): """ Return the score for a user on a problem, as a tuple (correct, total). @@ -289,26 +295,20 @@ def get_score(course_id, user, problem, student_module_cache): problem: an XModule cache: A StudentModuleCache """ - if not (problem.descriptor.stores_state and problem.descriptor.has_score): + if not (problem_descriptor.stores_state and problem_descriptor.has_score): # These are not problems, and do not have a score return (None, None) correct = 0.0 - - # If the ID is not in the cache, add the item - instance_module = get_instance_module(course_id, 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, - # course_id=????, - # 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 = student_module_cache.lookup( + course_id, problem_descriptor.category, problem_descriptor.location.url()) + + if not instance_module: + # If the problem was not in the cache, we need to instantiate the problem. + # Otherwise, the max score (cached in instance_module) won't be available + problem = xmodule_system.get_module(problem_descriptor.location) + instance_module = get_instance_module(course_id, user, problem, student_module_cache) # If this problem is ungraded/ungradable, bail if instance_module.max_grade is None: @@ -319,7 +319,7 @@ def get_score(course_id, user, problem, student_module_cache): if correct is not None and total is not None: #Now we re-weight the problem, if specified - weight = getattr(problem, 'weight', None) + weight = getattr(problem_descriptor, 'weight', None) if weight is not None: if total == 0: log.exception("Cannot reweight a problem with zero weight. Problem: " + str(instance_module)) From 87da104974a556aed86b8eb009bdb214a06298a2 Mon Sep 17 00:00:00 2001 From: Bridger Maxwell Date: Sat, 8 Sep 2012 01:09:42 -0400 Subject: [PATCH 02/17] Changes to speed up the progress summary too. --- lms/djangoapps/courseware/grades.py | 46 ++++++++++++++++++++--------- lms/djangoapps/courseware/views.py | 8 +---- 2 files changed, 33 insertions(+), 21 deletions(-) diff --git a/lms/djangoapps/courseware/grades.py b/lms/djangoapps/courseware/grades.py index cac4f7f682..5d0a1a2dec 100644 --- a/lms/djangoapps/courseware/grades.py +++ b/lms/djangoapps/courseware/grades.py @@ -146,12 +146,10 @@ def grade(student, request, course, student_module_cache=None, keep_raw_scores=F # student doesn't have access to this module, or something else # went wrong. continue - - # 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 + + #TODO: This won't get all problems, will it? They could be hidden in a sub-child. What is a better + # way of getting all of the children as descriptors? module_locations = section_module.definition.get('children', []) - - for module_location in module_locations: module_descriptor = section_module.descriptor.system.load_item(module_location) @@ -219,7 +217,11 @@ def grade_for_percentage(grade_cutoffs, percentage): return letter_grade -def progress_summary(student, course, grader, student_module_cache): + +# TODO: This method is not very good. It was written in the old course style and +# then converted over and performance is not good. Once the progress page is redesigned +# to not have the progress summary this method should be deleted (so it won't be copied). +def progress_summary(student, request, course, grader, student_module_cache): """ This pulls a summary of all problems in the course. @@ -236,34 +238,50 @@ def progress_summary(student, course, grader, student_module_cache): student_module_cache: A StudentModuleCache initialized with all instance_modules for the student """ + chapters = [] # Don't include chapters that aren't displayable (e.g. due to error) - for c in course.get_display_items(): + for c in course.get_children(): # Skip if the chapter is hidden hidden = c.metadata.get('hide_from_toc','false') if hidden.lower() == 'true': continue sections = [] - for s in c.get_display_items(): + for s in c.get_children(): # Skip if the section is hidden hidden = s.metadata.get('hide_from_toc','false') if hidden.lower() == 'true': continue + + # Now we actually instantiate the section module, to get the children + + # TODO: We need the request to pass into here. If we could forgo that, our arguments + # would be simpler + section_module = get_module(student, request, + s.location, student_module_cache, + course.id) + if section_module is None: + # student doesn't have access to this module, or something else + # went wrong. + continue + # Same for sections graded = s.metadata.get('graded', False) scores = [] - for module in yield_module_descendents(s): - # course is a module, not a descriptor... - course_id = course.descriptor.id - # TODO: This instantiates the problem TWICE! That is no good. - (correct, total) = get_score(course_id, student, module.descriptor, module.system, student_module_cache) + + module_locations = section_module.definition.get('children', []) + for module_location in module_locations: + module_descriptor = s.system.load_item(module_location) + + course_id = course.id + (correct, total) = get_score(course_id, student, module_descriptor, section_module.system, student_module_cache) if correct is None and total is None: continue scores.append(Score(correct, total, graded, - module.metadata.get('display_name'))) + module_descriptor.metadata.get('display_name'))) section_total, graded_total = graders.aggregate_scores( scores, s.metadata.get('display_name')) diff --git a/lms/djangoapps/courseware/views.py b/lms/djangoapps/courseware/views.py index 60279d34c9..5b6518992d 100644 --- a/lms/djangoapps/courseware/views.py +++ b/lms/djangoapps/courseware/views.py @@ -350,14 +350,8 @@ def progress(request, course_id, student_id=None): student_module_cache = StudentModuleCache.cache_for_descriptor_descendents( course_id, student, course) - course_module = get_module(student, request, course.location, - student_module_cache, course_id) - # The course_module should be accessible, but check anyway just in case something went wrong: - if course_module is None: - raise Http404("Course does not exist") - - courseware_summary = grades.progress_summary(student, course_module, + courseware_summary = grades.progress_summary(student, request, course, course.grader, student_module_cache) grade_summary = grades.grade(student, request, course, student_module_cache) From bdc9e55fd09a11b650917280cc21be177666ec26 Mon Sep 17 00:00:00 2001 From: Bridger Maxwell Date: Sun, 16 Sep 2012 00:36:26 -0400 Subject: [PATCH 03/17] Fixed progress_summary to use get_display_items(), which respects access control. --- lms/djangoapps/courseware/grades.py | 48 +++++++++++++---------------- 1 file changed, 21 insertions(+), 27 deletions(-) diff --git a/lms/djangoapps/courseware/grades.py b/lms/djangoapps/courseware/grades.py index 5d0a1a2dec..2bc930d81c 100644 --- a/lms/djangoapps/courseware/grades.py +++ b/lms/djangoapps/courseware/grades.py @@ -239,41 +239,35 @@ def progress_summary(student, request, course, grader, student_module_cache): instance_modules for the student """ + + # TODO: We need the request to pass into here. If we could forgo that, our arguments + # would be simpler + course_module = get_module(student, request, + course.location, student_module_cache, + course.id) + chapters = [] # Don't include chapters that aren't displayable (e.g. due to error) - for c in course.get_children(): + for chapter_module in course_module.get_display_items(): # Skip if the chapter is hidden - hidden = c.metadata.get('hide_from_toc','false') + hidden = chapter_module.metadata.get('hide_from_toc','false') if hidden.lower() == 'true': continue - + sections = [] - for s in c.get_children(): + for section_module in chapter_module.get_display_items(): # Skip if the section is hidden - hidden = s.metadata.get('hide_from_toc','false') + hidden = section_module.metadata.get('hide_from_toc','false') if hidden.lower() == 'true': continue - # Now we actually instantiate the section module, to get the children - - # TODO: We need the request to pass into here. If we could forgo that, our arguments - # would be simpler - section_module = get_module(student, request, - s.location, student_module_cache, - course.id) - if section_module is None: - # student doesn't have access to this module, or something else - # went wrong. - continue - - # Same for sections - graded = s.metadata.get('graded', False) + graded = section_module.metadata.get('graded', False) scores = [] module_locations = section_module.definition.get('children', []) for module_location in module_locations: - module_descriptor = s.system.load_item(module_location) + module_descriptor = course.system.load_item(module_location) course_id = course.id (correct, total) = get_score(course_id, student, module_descriptor, section_module.system, student_module_cache) @@ -284,22 +278,22 @@ def progress_summary(student, request, course, grader, student_module_cache): module_descriptor.metadata.get('display_name'))) section_total, graded_total = graders.aggregate_scores( - scores, s.metadata.get('display_name')) + scores, section_module.metadata.get('display_name')) - format = s.metadata.get('format', "") + format = section_module.metadata.get('format', "") sections.append({ - 'display_name': s.display_name, - 'url_name': s.url_name, + 'display_name': section_module.display_name, + 'url_name': section_module.url_name, 'scores': scores, 'section_total': section_total, 'format': format, - 'due': s.metadata.get("due", ""), + 'due': section_module.metadata.get("due", ""), 'graded': graded, }) chapters.append({'course': course.display_name, - 'display_name': c.display_name, - 'url_name': c.url_name, + 'display_name': chapter_module.display_name, + 'url_name': chapter_module.url_name, 'sections': sections}) return chapters From adc95ba14cf569e7ec35167dfbe4e814f9358e36 Mon Sep 17 00:00:00 2001 From: Bridger Maxwell Date: Sun, 16 Sep 2012 00:36:55 -0400 Subject: [PATCH 04/17] Minor change on progress page. Hiding (0/0) on sections that don't have any score. --- lms/templates/courseware/progress.html | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lms/templates/courseware/progress.html b/lms/templates/courseware/progress.html index 86649a2e91..87ac06bae6 100644 --- a/lms/templates/courseware/progress.html +++ b/lms/templates/courseware/progress.html @@ -50,7 +50,11 @@ ${progress_graph.body(grade_summary, course.grade_cutoffs, "grade-detail-graph") %>

- ${ section['display_name'] } ${"({0:.3n}/{1:.3n}) {2}".format( float(earned), float(total), percentageString )}

+ ${ section['display_name'] } + %if total > 0 or earned > 0: + ${"({0:.3n}/{1:.3n}) {2}".format( float(earned), float(total), percentageString )} + %endif +

${section['format']} From b65577f2b783374e4dade3328aec11fc6cbd5408 Mon Sep 17 00:00:00 2001 From: Bridger Maxwell Date: Tue, 18 Sep 2012 13:18:19 -0400 Subject: [PATCH 05/17] Fixed tests for fast grading. --- lms/djangoapps/courseware/grades.py | 7 +++++++ lms/djangoapps/courseware/views.py | 4 ++++ 2 files changed, 11 insertions(+) diff --git a/lms/djangoapps/courseware/grades.py b/lms/djangoapps/courseware/grades.py index 2bc930d81c..10ea81a8ee 100644 --- a/lms/djangoapps/courseware/grades.py +++ b/lms/djangoapps/courseware/grades.py @@ -237,6 +237,10 @@ def progress_summary(student, request, course, grader, student_module_cache): course: An XModule containing the course to grade student_module_cache: A StudentModuleCache initialized with all instance_modules for the student + + If the student does not have access to load the course module, this function + will return None. + """ @@ -245,6 +249,9 @@ def progress_summary(student, request, course, grader, student_module_cache): course_module = get_module(student, request, course.location, student_module_cache, course.id) + if not course_module: + # This student must not have access to the course. + return None chapters = [] # Don't include chapters that aren't displayable (e.g. due to error) diff --git a/lms/djangoapps/courseware/views.py b/lms/djangoapps/courseware/views.py index 09078cc36b..6350d70385 100644 --- a/lms/djangoapps/courseware/views.py +++ b/lms/djangoapps/courseware/views.py @@ -459,6 +459,10 @@ def progress(request, course_id, student_id=None): courseware_summary = grades.progress_summary(student, request, course, course.grader, student_module_cache) grade_summary = grades.grade(student, request, course, student_module_cache) + + if courseware_summary is None: + #This means the student didn't have access to the course (which the instructor requested) + raise Http404 context = {'course': course, 'courseware_summary': courseware_summary, From 142624a6caeb50fcf787ed8d7fa9ec6584023398 Mon Sep 17 00:00:00 2001 From: Bridger Maxwell Date: Wed, 19 Sep 2012 00:20:46 -0400 Subject: [PATCH 06/17] Added select_related to gradebook. Slight performance boost. --- lms/djangoapps/instructor/views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lms/djangoapps/instructor/views.py b/lms/djangoapps/instructor/views.py index d812791c3d..3ce975410c 100644 --- a/lms/djangoapps/instructor/views.py +++ b/lms/djangoapps/instructor/views.py @@ -294,7 +294,7 @@ def gradebook(request, course_id): """ course = get_course_with_access(request.user, course_id, 'staff') - enrolled_students = User.objects.filter(courseenrollment__course_id=course_id).order_by('username') + enrolled_students = User.objects.filter(courseenrollment__course_id=course_id).order_by('username').select_related("profile") # TODO (vshnayder): implement pagination. enrolled_students = enrolled_students[:1000] # HACK! From 28826b8a365d8939328d355ff49fa64b71351ca1 Mon Sep 17 00:00:00 2001 From: Bridger Maxwell Date: Thu, 20 Sep 2012 00:31:37 -0400 Subject: [PATCH 07/17] Added new API to xmodule(descriptor). get_children_locations and has_dynamic_children. --- common/lib/xmodule/xmodule/abtest_module.py | 12 +++++---- common/lib/xmodule/xmodule/x_module.py | 29 ++++++++++++++++++++- 2 files changed, 35 insertions(+), 6 deletions(-) diff --git a/common/lib/xmodule/xmodule/abtest_module.py b/common/lib/xmodule/xmodule/abtest_module.py index ceca6ff9ed..7080682172 100644 --- a/common/lib/xmodule/xmodule/abtest_module.py +++ b/common/lib/xmodule/xmodule/abtest_module.py @@ -47,11 +47,9 @@ class ABTestModule(XModule): def get_shared_state(self): return json.dumps({'group': self.group}) - - def displayable_items(self): - child_locations = self.definition['data']['group_content'][self.group] - children = [self.system.get_module(loc) for loc in child_locations] - return [c for c in children if c is not None] + + def get_children_locations(self): + return self.definition['data']['group_content'][self.group] # TODO (cpennington): Use Groups should be a first class object, rather than being @@ -158,3 +156,7 @@ class ABTestDescriptor(RawDescriptor, XmlDescriptor): group_elem.append(etree.fromstring(child.export_to_xml(resource_fs))) return xml_object + + + def has_dynamic_children(self): + return True diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index e21e858baa..8846926e05 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -219,13 +219,28 @@ class XModule(HTMLSnippet): Return module instances for all the children of this module. ''' if self._loaded_children is None: - child_locations = self.definition.get('children', []) + child_locations = self.get_children_locations() children = [self.system.get_module(loc) for loc in child_locations] # get_module returns None if the current user doesn't have access # to the location. self._loaded_children = [c for c in children if c is not None] return self._loaded_children + + def get_children_locations(self): + ''' + Returns the locations of each of child modules. + + Overriding this changes the behavior of get_children and + anything that uses get_children, such as get_display_items. + + This method will not instantiate the modules of the children + unless absolutely necessary, so it is cheaper to call than get_children + + These children will be the same children returned by the + descriptor unless descriptor.has_dynamic_children() is true. + ''' + return self.definition.get('children', []) def get_display_items(self): ''' @@ -489,6 +504,18 @@ class XModuleDescriptor(Plugin, HTMLSnippet): self, metadata=self.metadata ) + + + def has_dynamic_children(self): + """ + Returns True if this descriptor has dynamic children for a given + student when the module is created. + + Returns False if the children of this descriptor are the same + children that the module will return for any student. + """ + return False + # ================================= JSON PARSING =========================== @staticmethod From 234fb813c60fa88353235f20e846df82357c3888 Mon Sep 17 00:00:00 2001 From: Bridger Maxwell Date: Thu, 20 Sep 2012 01:03:43 -0400 Subject: [PATCH 08/17] Changed grades.grade to use new xmodule API. Now properly explores descriptor tree. --- lms/djangoapps/courseware/grades.py | 58 +++++++++++++++++++---------- 1 file changed, 38 insertions(+), 20 deletions(-) diff --git a/lms/djangoapps/courseware/grades.py b/lms/djangoapps/courseware/grades.py index 10ea81a8ee..60753438e4 100644 --- a/lms/djangoapps/courseware/grades.py +++ b/lms/djangoapps/courseware/grades.py @@ -27,6 +27,30 @@ def yield_module_descendents(module): stack.extend( next_module.get_display_items() ) yield next_module +def yield_dynamic_descriptor_descendents(descriptor, module_creator): + """ + This returns all of the descendants of a descriptor. If the descriptor + has dynamic children, the module will be created using module_creator + and the children (as descriptors) of that module will be returned. + """ + def get_dynamic_descriptor_children(descriptor): + if descriptor.has_dynamic_children(): + module = module_creator(descriptor) + children_locations = module.get_children_locations() + return [descriptor.system.load_item(child_location) for child_location in child_locations ] + else: + return descriptor.get_children() + + + stack = get_dynamic_descriptor_children(descriptor) + stack.reverse() + + while len(stack) > 0: + next_descriptor = stack.pop() + stack.extend( get_dynamic_descriptor_children(next_descriptor) ) + yield next_descriptor + + def yield_problems(request, course, student): """ Return an iterator over capa_modules that this student has @@ -128,7 +152,7 @@ def grade(student, request, course, student_module_cache=None, keep_raw_scores=F 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% + # 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( course.id, moduledescriptor.category, moduledescriptor.location.url()): @@ -137,23 +161,16 @@ def grade(student, request, course, student_module_cache=None, keep_raw_scores=F if should_grade_section: scores = [] - # TODO: We need the request to pass into here. If we could forgo that, our arguments - # would be simpler - section_module = get_module(student, request, - section_descriptor.location, student_module_cache, - course.id) - if section_module is None: - # student doesn't have access to this module, or something else - # went wrong. - continue - #TODO: This won't get all problems, will it? They could be hidden in a sub-child. What is a better - # way of getting all of the children as descriptors? - module_locations = section_module.definition.get('children', []) - for module_location in module_locations: - module_descriptor = section_module.descriptor.system.load_item(module_location) - - (correct, total) = get_score(course.id, student, module_descriptor, section_module.system, student_module_cache) + def create_module(descriptor): + # TODO: We need the request to pass into here. If we could forgo that, our arguments + # would be simpler + return get_module(student, request, descriptor.location, + student_module_cache, course.id) + + for module_descriptor in yield_dynamic_descriptor_descendents(section_descriptor, create_module): + + (correct, total) = get_score(course.id, student, module_descriptor, create_module, student_module_cache) if correct is None and total is None: continue @@ -277,7 +294,8 @@ def progress_summary(student, request, course, grader, student_module_cache): module_descriptor = course.system.load_item(module_location) course_id = course.id - (correct, total) = get_score(course_id, student, module_descriptor, section_module.system, student_module_cache) + module_creator = lambda decriptor : section_module.system.get_module(descriptor.location) + (correct, total) = get_score(course_id, student, module_descriptor, module_creator, student_module_cache) if correct is None and total is None: continue @@ -306,7 +324,7 @@ def progress_summary(student, request, course, grader, student_module_cache): return chapters -def get_score(course_id, user, problem_descriptor, xmodule_system, student_module_cache): +def get_score(course_id, user, problem_descriptor, module_creator, student_module_cache): """ Return the score for a user on a problem, as a tuple (correct, total). @@ -326,7 +344,7 @@ def get_score(course_id, user, problem_descriptor, xmodule_system, student_modul if not instance_module: # If the problem was not in the cache, we need to instantiate the problem. # Otherwise, the max score (cached in instance_module) won't be available - problem = xmodule_system.get_module(problem_descriptor.location) + problem = module_creator(problem_descriptor) instance_module = get_instance_module(course_id, user, problem, student_module_cache) # If this problem is ungraded/ungradable, bail From b62e13aec8d78d4518b00d3083869122f3b14965 Mon Sep 17 00:00:00 2001 From: Bridger Maxwell Date: Thu, 20 Sep 2012 01:45:06 -0400 Subject: [PATCH 09/17] Using new xmodule API in grades.py. --- common/lib/xmodule/xmodule/course_module.py | 1 + lms/djangoapps/courseware/grades.py | 19 +++++++++---------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/common/lib/xmodule/xmodule/course_module.py b/common/lib/xmodule/xmodule/course_module.py index 7aa904205d..d6387ee55d 100644 --- a/common/lib/xmodule/xmodule/course_module.py +++ b/common/lib/xmodule/xmodule/course_module.py @@ -178,6 +178,7 @@ class CourseDescriptor(SequenceDescriptor): for s in c.get_children(): if s.metadata.get('graded', False): xmoduledescriptors = list(yield_descriptor_descendents(s)) + xmoduledescriptors.append(s) # The xmoduledescriptors included here are only the ones that have scores. section_description = { 'section_descriptor' : s, 'xmoduledescriptors' : filter(lambda child: child.has_score, xmoduledescriptors) } diff --git a/lms/djangoapps/courseware/grades.py b/lms/djangoapps/courseware/grades.py index 60753438e4..90af314a5a 100644 --- a/lms/djangoapps/courseware/grades.py +++ b/lms/djangoapps/courseware/grades.py @@ -35,15 +35,15 @@ def yield_dynamic_descriptor_descendents(descriptor, module_creator): """ def get_dynamic_descriptor_children(descriptor): if descriptor.has_dynamic_children(): + print "descriptor has dynamic children" , descriptor.location module = module_creator(descriptor) - children_locations = module.get_children_locations() + child_locations = module.get_children_locations() return [descriptor.system.load_item(child_location) for child_location in child_locations ] else: return descriptor.get_children() - stack = get_dynamic_descriptor_children(descriptor) - stack.reverse() + stack = [descriptor] while len(stack) > 0: next_descriptor = stack.pop() @@ -152,7 +152,7 @@ def grade(student, request, course, student_module_cache=None, keep_raw_scores=F 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% + # 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( course.id, moduledescriptor.category, moduledescriptor.location.url()): @@ -168,8 +168,8 @@ def grade(student, request, course, student_module_cache=None, keep_raw_scores=F return get_module(student, request, descriptor.location, student_module_cache, course.id) - for module_descriptor in yield_dynamic_descriptor_descendents(section_descriptor, create_module): - + for module_descriptor in yield_dynamic_descriptor_descendents(section_descriptor, create_module): + (correct, total) = get_score(course.id, student, module_descriptor, create_module, student_module_cache) if correct is None and total is None: continue @@ -289,12 +289,11 @@ def progress_summary(student, request, course, grader, student_module_cache): graded = section_module.metadata.get('graded', False) scores = [] - module_locations = section_module.definition.get('children', []) - for module_location in module_locations: - module_descriptor = course.system.load_item(module_location) + module_creator = lambda descriptor : section_module.system.get_module(descriptor.location) + + for module_descriptor in yield_dynamic_descriptor_descendents(section_module.descriptor, module_creator): course_id = course.id - module_creator = lambda decriptor : section_module.system.get_module(descriptor.location) (correct, total) = get_score(course_id, student, module_descriptor, module_creator, student_module_cache) if correct is None and total is None: continue From 21f8fc3f9c2871e2cc4c38dba81bdf4ba6b4ee5b Mon Sep 17 00:00:00 2001 From: Bridger Maxwell Date: Thu, 20 Sep 2012 02:17:51 -0400 Subject: [PATCH 10/17] Fixed small bug. --- lms/djangoapps/courseware/grades.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lms/djangoapps/courseware/grades.py b/lms/djangoapps/courseware/grades.py index 90af314a5a..d1a397ced5 100644 --- a/lms/djangoapps/courseware/grades.py +++ b/lms/djangoapps/courseware/grades.py @@ -347,7 +347,7 @@ def get_score(course_id, user, problem_descriptor, module_creator, student_modul instance_module = get_instance_module(course_id, user, problem, student_module_cache) # If this problem is ungraded/ungradable, bail - if instance_module.max_grade is None: + if not instance_module or instance_module.max_grade is None: return (None, None) correct = instance_module.grade if instance_module.grade is not None else 0 From a43b09c0a9c49e06d8f0b4ba1f960f5e833c6795 Mon Sep 17 00:00:00 2001 From: Bridger Maxwell Date: Thu, 20 Sep 2012 13:23:47 -0400 Subject: [PATCH 11/17] Fixed new implementation of abtest's displayable_items. --- common/lib/xmodule/xmodule/abtest_module.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/common/lib/xmodule/xmodule/abtest_module.py b/common/lib/xmodule/xmodule/abtest_module.py index 7080682172..8fd4810946 100644 --- a/common/lib/xmodule/xmodule/abtest_module.py +++ b/common/lib/xmodule/xmodule/abtest_module.py @@ -50,6 +50,11 @@ class ABTestModule(XModule): def get_children_locations(self): return self.definition['data']['group_content'][self.group] + + def displayable_items(self): + # Most modules return "self" as the displayable_item. We never display ourself + # (which is why we don't implement get_html). We only display our children. + return self.get_children() # TODO (cpennington): Use Groups should be a first class object, rather than being From 9ce3b83be3ce488309f9e9839cf94dee35034427 Mon Sep 17 00:00:00 2001 From: Bridger Maxwell Date: Thu, 20 Sep 2012 13:29:16 -0400 Subject: [PATCH 12/17] Added a simple course for testing the grading functions. --- common/test/data/graded/README.md | 1 + common/test/data/graded/course.xml | 1 + common/test/data/graded/course/2012_Fall.xml | 87 +++++++++++++++++++ common/test/data/graded/graded | 1 + common/test/data/graded/grading_policy.json | 22 +++++ .../test/data/graded/policies/2012_Fall.json | 46 ++++++++++ common/test/data/graded/roots/2012_Fall.xml | 1 + 7 files changed, 159 insertions(+) create mode 100644 common/test/data/graded/README.md create mode 120000 common/test/data/graded/course.xml create mode 100644 common/test/data/graded/course/2012_Fall.xml create mode 120000 common/test/data/graded/graded create mode 100644 common/test/data/graded/grading_policy.json create mode 100644 common/test/data/graded/policies/2012_Fall.json create mode 100644 common/test/data/graded/roots/2012_Fall.xml diff --git a/common/test/data/graded/README.md b/common/test/data/graded/README.md new file mode 100644 index 0000000000..59ab392ed3 --- /dev/null +++ b/common/test/data/graded/README.md @@ -0,0 +1 @@ +This is a very very simple course, useful for initial debugging of processing code. diff --git a/common/test/data/graded/course.xml b/common/test/data/graded/course.xml new file mode 120000 index 0000000000..49041310f6 --- /dev/null +++ b/common/test/data/graded/course.xml @@ -0,0 +1 @@ +roots/2012_Fall.xml \ No newline at end of file diff --git a/common/test/data/graded/course/2012_Fall.xml b/common/test/data/graded/course/2012_Fall.xml new file mode 100644 index 0000000000..e9735f9756 --- /dev/null +++ b/common/test/data/graded/course/2012_Fall.xml @@ -0,0 +1,87 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/common/test/data/graded/graded b/common/test/data/graded/graded new file mode 120000 index 0000000000..be6adb9cd0 --- /dev/null +++ b/common/test/data/graded/graded @@ -0,0 +1 @@ +graded \ No newline at end of file diff --git a/common/test/data/graded/grading_policy.json b/common/test/data/graded/grading_policy.json new file mode 100644 index 0000000000..54a874191e --- /dev/null +++ b/common/test/data/graded/grading_policy.json @@ -0,0 +1,22 @@ +{ + "GRADER" : [ + { + "type" : "Homework", + "min_count" : 3, + "drop_count" : 1, + "short_label" : "HW", + "weight" : 0.5 + }, + { + "type" : "Final", + "name" : "Final Question", + "short_label" : "Final", + "weight" : 0.5 + } + ], + "GRADE_CUTOFFS" : { + "A" : 0.8, + "B" : 0.7, + "C" : 0.6 + } +} diff --git a/common/test/data/graded/policies/2012_Fall.json b/common/test/data/graded/policies/2012_Fall.json new file mode 100644 index 0000000000..d9734157a4 --- /dev/null +++ b/common/test/data/graded/policies/2012_Fall.json @@ -0,0 +1,46 @@ +{ + "course/2012_Fall": { + "graceperiod": "2 days 5 hours 59 minutes 59 seconds", + "start": "2010-07-17T12:00", + "display_name": "Graded Course", + "graded": "true" + }, + + + "vertical/Homework1": { + "display_name": "Homework 1", + "graded": true, + "format": "Homework" + }, + + + "videosequence/Homework2": { + "display_name": "Homework 2", + "graded": true, + "format": "Homework" + }, + + "videosequence/Homework3": { + "display_name": "Homework 3", + "graded": true, + "format": "Homework" + }, + + + "vertical/Homework1": { + "display_name": "Homework 1", + "graded": true, + "format": "Homework" + }, + + "problem/FinalQuestion": { + "display_name": "Final Question", + "graded": true, + "format": "Final" + }, + + "chapter/Overview": { + "display_name": "Overview" + } + +} diff --git a/common/test/data/graded/roots/2012_Fall.xml b/common/test/data/graded/roots/2012_Fall.xml new file mode 100644 index 0000000000..b71528809b --- /dev/null +++ b/common/test/data/graded/roots/2012_Fall.xml @@ -0,0 +1 @@ + \ No newline at end of file From 9ec38176e01639f3087eec4b85af67ef24c8e2f7 Mon Sep 17 00:00:00 2001 From: Bridger Maxwell Date: Thu, 20 Sep 2012 16:56:09 -0400 Subject: [PATCH 13/17] Small cleanup tweaks in grading. --- lms/djangoapps/courseware/grades.py | 3 +-- lms/djangoapps/courseware/views.py | 4 ++-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/lms/djangoapps/courseware/grades.py b/lms/djangoapps/courseware/grades.py index d1a397ced5..e3e66724a5 100644 --- a/lms/djangoapps/courseware/grades.py +++ b/lms/djangoapps/courseware/grades.py @@ -35,7 +35,6 @@ def yield_dynamic_descriptor_descendents(descriptor, module_creator): """ def get_dynamic_descriptor_children(descriptor): if descriptor.has_dynamic_children(): - print "descriptor has dynamic children" , descriptor.location module = module_creator(descriptor) child_locations = module.get_children_locations() return [descriptor.system.load_item(child_location) for child_location in child_locations ] @@ -238,7 +237,7 @@ def grade_for_percentage(grade_cutoffs, percentage): # TODO: This method is not very good. It was written in the old course style and # then converted over and performance is not good. Once the progress page is redesigned # to not have the progress summary this method should be deleted (so it won't be copied). -def progress_summary(student, request, course, grader, student_module_cache): +def progress_summary(student, request, course, student_module_cache): """ This pulls a summary of all problems in the course. diff --git a/lms/djangoapps/courseware/views.py b/lms/djangoapps/courseware/views.py index 6350d70385..f3e8277484 100644 --- a/lms/djangoapps/courseware/views.py +++ b/lms/djangoapps/courseware/views.py @@ -22,7 +22,7 @@ from django.views.decorators.cache import cache_control from courseware import grades from courseware.access import has_access from courseware.courses import (get_course_with_access, get_courses_by_university) -from models import StudentModuleCache +from courseware.models import StudentModuleCache from module_render import toc_for_course, get_module, get_instance_module from student.models import UserProfile @@ -457,7 +457,7 @@ def progress(request, course_id, student_id=None): course_id, student, course) courseware_summary = grades.progress_summary(student, request, course, - course.grader, student_module_cache) + student_module_cache) grade_summary = grades.grade(student, request, course, student_module_cache) if courseware_summary is None: From 92171ea5da65633fa30ea74c35ebd009bbe31e97 Mon Sep 17 00:00:00 2001 From: Bridger Maxwell Date: Thu, 20 Sep 2012 16:56:27 -0400 Subject: [PATCH 14/17] Added course grading test. --- common/test/data/graded/roots/2012_Fall.xml | 2 +- lms/djangoapps/courseware/tests/tests.py | 132 +++++++++++++++++++- 2 files changed, 132 insertions(+), 2 deletions(-) diff --git a/common/test/data/graded/roots/2012_Fall.xml b/common/test/data/graded/roots/2012_Fall.xml index b71528809b..b046a28d3b 100644 --- a/common/test/data/graded/roots/2012_Fall.xml +++ b/common/test/data/graded/roots/2012_Fall.xml @@ -1 +1 @@ - \ No newline at end of file + \ No newline at end of file diff --git a/lms/djangoapps/courseware/tests/tests.py b/lms/djangoapps/courseware/tests/tests.py index f3b086748d..c0932eee0b 100644 --- a/lms/djangoapps/courseware/tests/tests.py +++ b/lms/djangoapps/courseware/tests/tests.py @@ -10,8 +10,9 @@ from pprint import pprint from urlparse import urlsplit, urlunsplit from django.contrib.auth.models import User, Group +from django.core.handlers.wsgi import WSGIRequest from django.test import TestCase -from django.test.client import Client +from django.test.client import Client, RequestFactory from django.conf import settings from django.core.urlresolvers import reverse from mock import patch, Mock @@ -20,7 +21,9 @@ from override_settings import override_settings import xmodule.modulestore.django # Need access to internal func to put users in the right group +from courseware import grades from courseware.access import _course_staff_group_name +from courseware.models import StudentModuleCache from student.models import Registration from xmodule.modulestore.django import modulestore @@ -640,3 +643,130 @@ class RealCoursesLoadTestCase(PageLoader): # ========= TODO: check ajax interaction here too? + + +@override_settings(MODULESTORE=TEST_DATA_XML_MODULESTORE) +class TestCourseGrader(PageLoader): + """Check that a course gets graded properly""" + + # NOTE: setUpClass() runs before override_settings takes effect, so + # can't do imports there without manually hacking settings. + + def setUp(self): + xmodule.modulestore.django._MODULESTORES = {} + courses = modulestore().get_courses() + + def find_course(course_id): + """Assumes the course is present""" + return [c for c in courses if c.id==course_id][0] + + self.graded_course = find_course("edX/graded/2012_Fall") + + # create a test student + self.student = 'view@test.com' + self.password = 'foo' + self.create_account('u1', self.student, self.password) + self.activate_user(self.student) + self.enroll(self.graded_course) + + self.student_user = user(self.student) + + self.factory = RequestFactory() + + def check_grade_percent(self, percent): + + student_module_cache = StudentModuleCache.cache_for_descriptor_descendents( + self.graded_course.id, self.student_user, self.graded_course) + + fake_request = self.factory.get(reverse('progress', + kwargs={'course_id': self.graded_course.id})) + + grade_summary = grades.grade(self.student_user, fake_request, + self.graded_course, student_module_cache) + self.assertEqual(grade_summary['percent'], percent) + + def submit_question_answer(self, problem_url_name, responses): + """ + The field names of a problem are hard to determine. This method only works + for the problems used in the edX/graded course, which has fields named in the + following form: + input_i4x-edX-graded-problem-H1P3_2_1 + input_i4x-edX-graded-problem-H1P3_2_2 + """ + + + problem_location = "i4x://edX/graded/problem/{0}".format(problem_url_name) + + modx_url = reverse('modx_dispatch', + kwargs={ + 'course_id' : self.graded_course.id, + 'location' : problem_location, + 'dispatch' : 'problem_check', } + ) + + resp = self.client.post(modx_url, { + 'input_i4x-edX-graded-problem-{0}_2_1'.format(problem_url_name): responses[0], + 'input_i4x-edX-graded-problem-{0}_2_2'.format(problem_url_name): responses[1], + }) + print "modx_url" , modx_url, "responses" , responses + print "resp" , resp + + return resp + + def reset_question_answer(self, problem_url_name): + problem_location = "i4x://edX/graded/problem/{0}".format(problem_url_name) + + modx_url = reverse('modx_dispatch', + kwargs={ + 'course_id' : self.graded_course.id, + 'location' : problem_location, + 'dispatch' : 'problem_reset', } + ) + + resp = self.client.post(modx_url) + return resp + + + def test_get_graded(self): + #### Check that the grader shows we have 0% in the course + self.check_grade_percent(0) + + + #### Submit the answers to a few problems as ajax calls + + # Only get half of the first problem correct + self.submit_question_answer('H1P1', ['Correct', 'Incorrect']) + self.check_grade_percent(0.06) + + # Get both parts of the first problem correct + self.reset_question_answer('H1P1') + self.submit_question_answer('H1P1', ['Correct', 'Correct']) + self.check_grade_percent(0.13) + + # This problem is shown in an ABTest + self.submit_question_answer('H1P2', ['Correct', 'Correct']) + self.check_grade_percent(0.25) + + # This problem is hidden in an ABTest. Getting it correct doesn't change total grade + self.submit_question_answer('H1P3', ['Correct', 'Correct']) + self.check_grade_percent(0.25) + + + # On the second homework, we only answer half of the questions. + # Then it will be dropped when homework three becomes the higher percent + self.submit_question_answer('H2P1', ['Correct', 'Correct']) + self.check_grade_percent(0.38) + + + # Third homework + self.submit_question_answer('H3P1', ['Correct', 'Correct']) + self.check_grade_percent(0.38) # Score didn't change + + self.submit_question_answer('H3P2', ['Correct', 'Correct']) + self.check_grade_percent(0.5) # Now homework2 dropped. Score changes + + + # Now we answer the final question (worth half of the grade) + self.submit_question_answer('FinalQuestion', ['Correct', 'Correct']) + self.check_grade_percent(1.0) # Hooray! We got 100% + From dfa977dbfa12daeead788a36f62e34401c0149cb Mon Sep 17 00:00:00 2001 From: Bridger Maxwell Date: Thu, 20 Sep 2012 17:37:34 -0400 Subject: [PATCH 15/17] Fixed problem weighting bug. Attribute is on descriptor, not module. --- common/lib/xmodule/xmodule/capa_module.py | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/common/lib/xmodule/xmodule/capa_module.py b/common/lib/xmodule/xmodule/capa_module.py index 8bf1a56404..51ee79b380 100644 --- a/common/lib/xmodule/xmodule/capa_module.py +++ b/common/lib/xmodule/xmodule/capa_module.py @@ -124,12 +124,6 @@ class CapaModule(XModule): self.name = only_one(dom2.xpath('/problem/@name')) - weight_string = only_one(dom2.xpath('/problem/@weight')) - if weight_string: - self.weight = float(weight_string) - else: - self.weight = None - if self.rerandomize == 'never': seed = 1 elif self.rerandomize == "per_student" and hasattr(self.system, 'id'): @@ -235,7 +229,7 @@ class CapaModule(XModule): content = {'name': self.display_name, 'html': html, - 'weight': self.weight, + 'weight': self.descriptor.weight, } # We using strings as truthy values, because the terminology of the @@ -615,3 +609,12 @@ class CapaDescriptor(RawDescriptor): 'problems/' + path[8:], path[8:], ] + + def __init__(self, *args, **kwargs): + super(CapaDescriptor, self).__init__(*args, **kwargs) + + weight_string = self.metadata.get('weight', None) + if weight_string: + self.weight = float(weight_string) + else: + self.weight = None From 997c0ee1c6697be43b9d8b073990280b27dcb1ec Mon Sep 17 00:00:00 2001 From: Bridger Maxwell Date: Thu, 20 Sep 2012 17:38:07 -0400 Subject: [PATCH 16/17] Grading test now tries a weighted problem. --- common/test/data/graded/policies/2012_Fall.json | 6 +++++- lms/djangoapps/courseware/tests/tests.py | 7 +++++-- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/common/test/data/graded/policies/2012_Fall.json b/common/test/data/graded/policies/2012_Fall.json index d9734157a4..026d846b6c 100644 --- a/common/test/data/graded/policies/2012_Fall.json +++ b/common/test/data/graded/policies/2012_Fall.json @@ -19,7 +19,11 @@ "graded": true, "format": "Homework" }, - + + "problem/H2P1": { + "weight": 4 + }, + "videosequence/Homework3": { "display_name": "Homework 3", "graded": true, diff --git a/lms/djangoapps/courseware/tests/tests.py b/lms/djangoapps/courseware/tests/tests.py index c0932eee0b..1c4595b5dd 100644 --- a/lms/djangoapps/courseware/tests/tests.py +++ b/lms/djangoapps/courseware/tests/tests.py @@ -754,13 +754,16 @@ class TestCourseGrader(PageLoader): # On the second homework, we only answer half of the questions. # Then it will be dropped when homework three becomes the higher percent + # This problem is also weighted to be 4 points (instead of default of 2) + # If the problem was unweighted the percent would have been 0.38 so we + # know it works. self.submit_question_answer('H2P1', ['Correct', 'Correct']) - self.check_grade_percent(0.38) + self.check_grade_percent(0.42) # Third homework self.submit_question_answer('H3P1', ['Correct', 'Correct']) - self.check_grade_percent(0.38) # Score didn't change + self.check_grade_percent(0.42) # Score didn't change self.submit_question_answer('H3P2', ['Correct', 'Correct']) self.check_grade_percent(0.5) # Now homework2 dropped. Score changes From 188eeed3a3142f3dbec27fdfd70547609e8965e3 Mon Sep 17 00:00:00 2001 From: Bridger Maxwell Date: Thu, 20 Sep 2012 17:41:02 -0400 Subject: [PATCH 17/17] Removed accidental symlink. --- common/test/data/graded/graded | 1 - 1 file changed, 1 deletion(-) delete mode 120000 common/test/data/graded/graded diff --git a/common/test/data/graded/graded b/common/test/data/graded/graded deleted file mode 120000 index be6adb9cd0..0000000000 --- a/common/test/data/graded/graded +++ /dev/null @@ -1 +0,0 @@ -graded \ No newline at end of file