From 854508b0067f9617096bbddf6f3d81ebefe1d22f Mon Sep 17 00:00:00 2001 From: Bridger Maxwell Date: Fri, 3 Aug 2012 14:30:45 -0400 Subject: [PATCH] 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()