Merge pull request #1655 from edx/adam/more-granular-grading-transactions

Adam/more granular grading transactions

lms/djangoapps/courseware/grades.py

@@ -4,9 +4,11 @@ from __future__ import division
 import random
 import logging
 
+from contextlib import contextmanager
 from collections import defaultdict
 from django.conf import settings
 from django.contrib.auth.models import User
+from django.db import transaction
 
 from courseware.model_data import FieldDataCache, DjangoKeyValueStore
 from xblock.fields import Scope
@@ -121,8 +123,20 @@ def answer_distributions(request, course):
     return counts
 
 
-def grade(student, request, course, field_data_cache=None, keep_raw_scores=False):
+@transaction.commit_manually
+def grade(student, request, course, keep_raw_scores=False):
     """
+    Wraps "_grade" with the manual_transaction context manager just in case
+    there are unanticipated errors.
+    """
+    with manual_transaction():
+        return _grade(student, request, course, keep_raw_scores)
+
+
+def _grade(student, request, course, keep_raw_scores):
+    """
+    Unwrapped version of "grade"
+
     This grades a student as quickly as possible. It returns the
     output from the course grader, augmented with the final letter
     grade. The keys in the output are:
@@ -132,20 +146,17 @@ def grade(student, request, course, field_data_cache=None, keep_raw_scores=False
     - grade : A final letter grade.
     - percent : The final percent for the class (rounded up).
     - section_breakdown : A breakdown of each section that makes
-        up the grade. (For display)
+      up the grade. (For display)
     - grade_breakdown : A breakdown of the major components that
-        make up the final grade. (For display)
+      make up the final grade. (For display)
-    - keep_raw_scores : if True, then value for key 'raw_scores' contains scores for every graded module
+    - keep_raw_scores : if True, then value for key 'raw_scores' contains scores
+      for every graded module
 
     More information on the format is in the docstring for CourseGrader.
     """
-
     grading_context = course.grading_context
     raw_scores = []
 
-    if field_data_cache is None:
-        field_data_cache = FieldDataCache(grading_context['all_descriptors'], course.id, student)
-
     totaled_scores = {}
     # This next complicated loop is just to collect the totaled_scores, which is
     # passed to the grader
@@ -155,26 +166,22 @@ def grade(student, request, course, field_data_cache=None, keep_raw_scores=False
             section_descriptor = section['section_descriptor']
             section_name = section_descriptor.display_name_with_default
 
-            should_grade_section = False
+            # some problems have state that is updated independently of interaction
+            # with the LMS, so they need to always be scored. (E.g. foldit.)
+            should_grade_section = any(
+                descriptor.always_recalculate_grades for descriptor in section['xmoduledescriptors']
+            )
+
             # 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 not should_grade_section:
-                # some problems have state that is updated independently of interaction
+                with manual_transaction():
-                # with the LMS, so they need to always be scored. (E.g. foldit.)
+                    should_grade_section = StudentModule.objects.filter(
-                if moduledescriptor.always_recalculate_grades:
+                        student=student,
-                    should_grade_section = True
+                        module_type='problem',
-                    break
+                        module_state_key__in=[
-
+                            descriptor.location for descriptor in section['xmoduledescriptors']
-                # Create a fake key to pull out a StudentModule object from the FieldDataCache
+                        ]
-
+                    ).exists()
-                key = DjangoKeyValueStore.Key(
-                    Scope.user_state,
-                    student.id,
-                    moduledescriptor.location,
-                    None
-                )
-                if field_data_cache.find(key):
-                    should_grade_section = True
-                    break
 
             if should_grade_section:
                 scores = []
@@ -183,11 +190,13 @@ def grade(student, request, course, field_data_cache=None, keep_raw_scores=False
                     '''creates an XModule instance given a descriptor'''
                     # TODO: We need the request to pass into here. If we could forego that, our arguments
                     # would be simpler
+                    with manual_transaction():
+                        field_data_cache = FieldDataCache([descriptor], course.id, student)
                     return get_module_for_descriptor(student, request, descriptor, field_data_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, field_data_cache)
+                    (correct, total) = get_score(course.id, student, module_descriptor, create_module)
                     if correct is None and total is None:
                         continue
 
@@ -256,11 +265,23 @@ def grade_for_percentage(grade_cutoffs, percentage):
     return letter_grade
 
 
+@transaction.commit_manually
+def progress_summary(student, request, course):
+    """
+    Wraps "_progress_summary" with the manual_transaction context manager just
+    in case there are unanticipated errors.
+    """
+    with manual_transaction():
+        return _progress_summary(student, request, course)
+
+
 # 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, field_data_cache):
+def _progress_summary(student, request, course):
     """
+    Unwrapped version of "progress_summary".
+
     This pulls a summary of all problems in the course.
 
     Returns
@@ -273,20 +294,21 @@ def progress_summary(student, request, course, field_data_cache):
     Arguments:
         student: A User object for the student to grade
         course: A Descriptor containing the course to grade
-        field_data_cache: A FieldDataCache 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.
 
     """
-
+    with manual_transaction():
-    # TODO: We need the request to pass into here. If we could forego that, our arguments
+        field_data_cache = FieldDataCache.cache_for_descriptor_descendents(
-    # would be simpler
+            course.id, student, course, depth=None
-    course_module = get_module_for_descriptor(student, request, course, field_data_cache, course.id)
+        )
-    if not course_module:
+        # TODO: We need the request to pass into here. If we could
-        # This student must not have access to the course.
+        # forego that, our arguments would be simpler
-        return None
+        course_module = get_module_for_descriptor(student, request, course, field_data_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)
@@ -296,50 +318,52 @@ def progress_summary(student, request, course, field_data_cache):
             continue
 
         sections = []
+
         for section_module in chapter_module.get_display_items():
             # Skip if the section is hidden
-            if section_module.hide_from_toc:
+            with manual_transaction():
-                continue
+                if section_module.hide_from_toc:
-
-            # Same for sections
-            graded = section_module.graded
-            scores = []
-
-            module_creator = section_module.xmodule_runtime.get_module
-
-            for module_descriptor in yield_dynamic_descriptor_descendents(section_module, module_creator):
-
-                course_id = course.id
-                (correct, total) = get_score(course_id, student, module_descriptor, module_creator, field_data_cache)
-                if correct is None and total is None:
                     continue
 
-                scores.append(Score(correct, total, graded, module_descriptor.display_name_with_default))
+                graded = section_module.graded
+                scores = []
 
-            scores.reverse()
+                module_creator = section_module.xmodule_runtime.get_module
-            section_total, _ = graders.aggregate_scores(
-                scores, section_module.display_name_with_default)
 
-            module_format = section_module.format if section_module.format is not None else ''
+                for module_descriptor in yield_dynamic_descriptor_descendents(section_module, module_creator):
-            sections.append({
-                'display_name': section_module.display_name_with_default,
-                'url_name': section_module.url_name,
-                'scores': scores,
-                'section_total': section_total,
-                'format': module_format,
-                'due': section_module.due,
-                'graded': graded,
-            })
 
-        chapters.append({'course': course.display_name_with_default,
+                    course_id = course.id
-                         'display_name': chapter_module.display_name_with_default,
+                    (correct, total) = get_score(course_id, student, module_descriptor, module_creator)
-                         'url_name': chapter_module.url_name,
+                    if correct is None and total is None:
-                         'sections': sections})
+                        continue
+
+                    scores.append(Score(correct, total, graded, module_descriptor.display_name_with_default))
+
+                scores.reverse()
+                section_total, _ = graders.aggregate_scores(
+                    scores, section_module.display_name_with_default)
+
+                module_format = section_module.format if section_module.format is not None else ''
+                sections.append({
+                    'display_name': section_module.display_name_with_default,
+                    'url_name': section_module.url_name,
+                    'scores': scores,
+                    'section_total': section_total,
+                    'format': module_format,
+                    'due': section_module.due,
+                    'graded': graded,
+                })
+
+        chapters.append({
+            'course': course.display_name_with_default,
+            'display_name': chapter_module.display_name_with_default,
+            'url_name': chapter_module.url_name,
+            'sections': sections
+        })
 
     return chapters
 
-
+def get_score(course_id, user, problem_descriptor, module_creator):
-def get_score(course_id, user, problem_descriptor, module_creator, field_data_cache):
     """
     Return the score for a user on a problem, as a tuple (correct, total).
     e.g. (5,7) if you got 5 out of 7 points.
@@ -372,15 +396,15 @@ def get_score(course_id, user, problem_descriptor, module_creator, field_data_ca
         # These are not problems, and do not have a score
         return (None, None)
 
-    # Create a fake KeyValueStore key to pull out the StudentModule
+    try:
-    key = DjangoKeyValueStore.Key(
+        student_module = StudentModule.objects.get(
-        Scope.user_state,
+            student=user,
-        user.id,
+            course_id=course_id,
-        problem_descriptor.location,
+            module_type='problem',
-        None
+            module_state_key=problem_descriptor.location
-    )
+        )
-
+    except StudentModule.DoesNotExist:
-    student_module = field_data_cache.find(key)
+        student_module = None
 
     if student_module is not None and student_module.max_grade is not None:
         correct = student_module.grade if student_module.grade is not None else 0
@@ -411,3 +435,16 @@ def get_score(course_id, user, problem_descriptor, module_creator, field_data_ca
         total = weight
 
     return (correct, total)
+
+
+@contextmanager
+def manual_transaction():
+    """A context manager for managing manual transactions"""
+    try:
+        yield
+    except Exception:
+        transaction.rollback()
+        log.exception('Due to an error, this transaction has been rolled back')
+        raise
+    else:
+        transaction.commit()

lms/djangoapps/courseware/tests/test_submitting_problems.py

@@ -236,14 +236,11 @@ class TestCourseGrader(TestSubmittingProblems):
             make up the final grade. (For display)
         """
 
-        field_data_cache = FieldDataCache.cache_for_descriptor_descendents(
+        fake_request = self.factory.get(
-            self.course.id, self.student_user, self.course)
+            reverse('progress', kwargs={'course_id': self.course.id})
+        )
 
-        fake_request = self.factory.get(reverse('progress',
+        return grades.grade(self.student_user, fake_request, self.course)
-                                        kwargs={'course_id': self.course.id}))
-
-        return grades.grade(self.student_user, fake_request,
-                            self.course, field_data_cache)
 
     def get_progress_summary(self):
         """
@@ -257,16 +254,13 @@ class TestCourseGrader(TestSubmittingProblems):
         etc.
         """
 
-        field_data_cache = FieldDataCache.cache_for_descriptor_descendents(
+        fake_request = self.factory.get(
-            self.course.id, self.student_user, self.course)
+            reverse('progress', kwargs={'course_id': self.course.id})
+        )
 
-        fake_request = self.factory.get(reverse('progress',
+        progress_summary = grades.progress_summary(
-                                        kwargs={'course_id': self.course.id}))
+            self.student_user, fake_request, self.course
-
+        )
-        progress_summary = grades.progress_summary(self.student_user,
-                                                   fake_request,
-                                                   self.course,
-                                                   field_data_cache)
         return progress_summary
 
     def check_grade_percent(self, percent):

lms/djangoapps/courseware/views.py

@@ -14,6 +14,7 @@ from django.shortcuts import redirect
 from mitxmako.shortcuts import render_to_response, render_to_string
 from django_future.csrf import ensure_csrf_cookie
 from django.views.decorators.cache import cache_control
+from django.db import transaction
 from markupsafe import escape
 
 from courseware import grades
@@ -643,8 +644,9 @@ def mktg_course_about(request, course_id):
     except (ValueError, Http404) as e:
         # if a course does not exist yet, display a coming
         # soon button
-        return render_to_response('courseware/mktg_coming_soon.html',
+        return render_to_response(
-                                  {'course_id': course_id})
+            'courseware/mktg_coming_soon.html', {'course_id': course_id}
+        )
 
     registered = registered_for_course(course, request.user)
 
@@ -659,21 +661,36 @@ def mktg_course_about(request, course_id):
                             settings.MITX_FEATURES.get('ENABLE_LMS_MIGRATION'))
     course_modes = CourseMode.modes_for_course(course.id)
 
-    return render_to_response('courseware/mktg_course_about.html',
+    return render_to_response(
-                              {
+        'courseware/mktg_course_about.html',
-                                  'course': course,
+        {
-                                  'registered': registered,
+            'course': course,
-                                  'allow_registration': allow_registration,
+            'registered': registered,
-                                  'course_target': course_target,
+            'allow_registration': allow_registration,
-                                  'show_courseware_link': show_courseware_link,
+            'course_target': course_target,
-                                  'course_modes': course_modes,
+            'show_courseware_link': show_courseware_link,
-                              })
+            'course_modes': course_modes,
+        }
+    )
 
 
 @login_required
 @cache_control(no_cache=True, no_store=True, must_revalidate=True)
+@transaction.commit_manually
 def progress(request, course_id, student_id=None):
-    """ User progress. We show the grade bar and every problem score.
+    """
+    Wraps "_progress" with the manual_transaction context manager just in case
+    there are unanticipated errors.
+    """
+    with grades.manual_transaction():
+        return _progress(request, course_id, student_id)
+
+
+def _progress(request, course_id, student_id):
+    """
+    Unwrapped version of "progress".
+
+    User progress. We show the grade bar and every problem score.
 
     Course staff are allowed to see the progress of students in their class.
     """
@@ -689,33 +706,33 @@ def progress(request, course_id, student_id=None):
             raise Http404
         student = User.objects.get(id=int(student_id))
 
-    # NOTE: To make sure impersonation by instructor works, use
+        # NOTE: To make sure impersonation by instructor works, use
-    # student instead of request.user in the rest of the function.
+        # student instead of request.user in the rest of the function.
 
-    # The pre-fetching of groups is done to make auth checks not require an
+        # The pre-fetching of groups is done to make auth checks not require an
-    # additional DB lookup (this kills the Progress page in particular).
+        # additional DB lookup (this kills the Progress page in particular).
     student = User.objects.prefetch_related("groups").get(id=student.id)
 
-    field_data_cache = FieldDataCache.cache_for_descriptor_descendents(
+    courseware_summary = grades.progress_summary(student, request, course)
-        course_id, student, course, depth=None)
 
-    courseware_summary = grades.progress_summary(student, request, course,
+    grade_summary = grades.grade(student, request, course)
-                                                 field_data_cache)
-    grade_summary = grades.grade(student, request, course, field_data_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,
+    context = {
-               'courseware_summary': courseware_summary,
+        'course': course,
-               'grade_summary': grade_summary,
+        'courseware_summary': courseware_summary,
-               'staff_access': staff_access,
+        'grade_summary': grade_summary,
-               'student': student,
+        'staff_access': staff_access,
-               }
+        'student': student,
-    context.update()
+    }
 
-    return render_to_response('courseware/progress.html', context)
+    with grades.manual_transaction():
+        response = render_to_response('courseware/progress.html', context)
+
+    return response
 
 
 @login_required