diff --git a/common/lib/xmodule/xmodule/abtest_module.py b/common/lib/xmodule/xmodule/abtest_module.py index ceca6ff9ed..8fd4810946 100644 --- a/common/lib/xmodule/xmodule/abtest_module.py +++ b/common/lib/xmodule/xmodule/abtest_module.py @@ -47,11 +47,14 @@ class ABTestModule(XModule): def get_shared_state(self): return json.dumps({'group': self.group}) - + + def get_children_locations(self): + return self.definition['data']['group_content'][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] + # 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 @@ -158,3 +161,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/capa_module.py b/common/lib/xmodule/xmodule/capa_module.py index 0d810af87a..052f4eefe7 100644 --- a/common/lib/xmodule/xmodule/capa_module.py +++ b/common/lib/xmodule/xmodule/capa_module.py @@ -125,12 +125,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': self.seed = 1 elif self.rerandomize == "per_student" and hasattr(self.system, 'id'): @@ -279,7 +273,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 @@ -659,3 +653,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 diff --git a/common/lib/xmodule/xmodule/course_module.py b/common/lib/xmodule/xmodule/course_module.py index 71c0d761ef..e7e3e4e519 100644 --- a/common/lib/xmodule/xmodule/course_module.py +++ b/common/lib/xmodule/xmodule/course_module.py @@ -189,6 +189,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/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 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/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..026d846b6c --- /dev/null +++ b/common/test/data/graded/policies/2012_Fall.json @@ -0,0 +1,50 @@ +{ + "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" + }, + + "problem/H2P1": { + "weight": 4 + }, + + "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..b046a28d3b --- /dev/null +++ b/common/test/data/graded/roots/2012_Fall.xml @@ -0,0 +1 @@ + \ No newline at end of file diff --git a/lms/djangoapps/courseware/grades.py b/lms/djangoapps/courseware/grades.py index 2243caaab6..e3e66724a5 100644 --- a/lms/djangoapps/courseware/grades.py +++ b/lms/djangoapps/courseware/grades.py @@ -27,6 +27,29 @@ 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) + 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 = [descriptor] + + 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 +151,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,20 +160,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: 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) + + 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 @@ -160,12 +179,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: @@ -214,7 +233,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, student_module_cache): """ This pulls a summary of all problems in the course. @@ -230,58 +253,76 @@ def progress_summary(student, 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. + """ + + + # 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) + 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) - for c in course.get_display_items(): + 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_display_items(): + 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 - + # Same for sections - graded = s.metadata.get('graded', False) + graded = section_module.metadata.get('graded', False) scores = [] - 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) + + 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 + (correct, total) = get_score(course_id, student, module_descriptor, module_creator, 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')) + 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 -def get_score(course_id, user, problem, 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). @@ -289,29 +330,23 @@ 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 = module_creator(problem_descriptor) + 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 @@ -319,7 +354,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)) diff --git a/lms/djangoapps/courseware/tests/tests.py b/lms/djangoapps/courseware/tests/tests.py index f3b086748d..1c4595b5dd 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,133 @@ 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 + # 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.42) + + + # Third homework + self.submit_question_answer('H3P1', ['Correct', 'Correct']) + 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 + + + # 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% + diff --git a/lms/djangoapps/courseware/views.py b/lms/djangoapps/courseware/views.py index 269977e458..eba45e373a 100644 --- a/lms/djangoapps/courseware/views.py +++ b/lms/djangoapps/courseware/views.py @@ -23,7 +23,7 @@ from courseware import grades from courseware.access import has_access from courseware.courses import (get_course_with_access, get_courses_by_university) import courseware.tabs as tabs -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 @@ -484,16 +484,14 @@ 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, - course.grader, student_module_cache) + courseware_summary = grades.progress_summary(student, request, course, + 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, diff --git a/lms/djangoapps/instructor/views.py b/lms/djangoapps/instructor/views.py index 312a46142a..0b6392a7fc 100644 --- a/lms/djangoapps/instructor/views.py +++ b/lms/djangoapps/instructor/views.py @@ -295,7 +295,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! 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']}