Merge pull request #13804 from edx/beryl/course_grades_glue

Beryl/course grades glue

lms/djangoapps/ccx/tests/test_field_override_performance.py

@@ -229,18 +229,18 @@ class TestFieldOverrideMongoPerformance(FieldOverridePerformanceTestCase):
         #     # of sql queries to default,
         #     # of mongo queries,
         # )
-        ('no_overrides', 1, True, False): (17, 6),
-        ('no_overrides', 2, True, False): (17, 6),
-        ('no_overrides', 3, True, False): (17, 6),
-        ('ccx', 1, True, False): (17, 6),
-        ('ccx', 2, True, False): (17, 6),
-        ('ccx', 3, True, False): (17, 6),
-        ('no_overrides', 1, False, False): (17, 6),
-        ('no_overrides', 2, False, False): (17, 6),
-        ('no_overrides', 3, False, False): (17, 6),
-        ('ccx', 1, False, False): (17, 6),
-        ('ccx', 2, False, False): (17, 6),
-        ('ccx', 3, False, False): (17, 6),
+        ('no_overrides', 1, True, False): (18, 6),
+        ('no_overrides', 2, True, False): (18, 6),
+        ('no_overrides', 3, True, False): (18, 6),
+        ('ccx', 1, True, False): (18, 6),
+        ('ccx', 2, True, False): (18, 6),
+        ('ccx', 3, True, False): (18, 6),
+        ('no_overrides', 1, False, False): (18, 6),
+        ('no_overrides', 2, False, False): (18, 6),
+        ('no_overrides', 3, False, False): (18, 6),
+        ('ccx', 1, False, False): (18, 6),
+        ('ccx', 2, False, False): (18, 6),
+        ('ccx', 3, False, False): (18, 6),
     }
 
 
@@ -252,19 +252,19 @@ class TestFieldOverrideSplitPerformance(FieldOverridePerformanceTestCase):
     __test__ = True
 
     TEST_DATA = {
-        ('no_overrides', 1, True, False): (17, 3),
-        ('no_overrides', 2, True, False): (17, 3),
-        ('no_overrides', 3, True, False): (17, 3),
-        ('ccx', 1, True, False): (17, 3),
-        ('ccx', 2, True, False): (17, 3),
-        ('ccx', 3, True, False): (17, 3),
-        ('ccx', 1, True, True): (18, 3),
-        ('ccx', 2, True, True): (18, 3),
-        ('ccx', 3, True, True): (18, 3),
-        ('no_overrides', 1, False, False): (17, 3),
-        ('no_overrides', 2, False, False): (17, 3),
-        ('no_overrides', 3, False, False): (17, 3),
-        ('ccx', 1, False, False): (17, 3),
-        ('ccx', 2, False, False): (17, 3),
-        ('ccx', 3, False, False): (17, 3),
+        ('no_overrides', 1, True, False): (18, 3),
+        ('no_overrides', 2, True, False): (18, 3),
+        ('no_overrides', 3, True, False): (18, 3),
+        ('ccx', 1, True, False): (18, 3),
+        ('ccx', 2, True, False): (18, 3),
+        ('ccx', 3, True, False): (18, 3),
+        ('ccx', 1, True, True): (19, 3),
+        ('ccx', 2, True, True): (19, 3),
+        ('ccx', 3, True, True): (19, 3),
+        ('no_overrides', 1, False, False): (18, 3),
+        ('no_overrides', 2, False, False): (18, 3),
+        ('no_overrides', 3, False, False): (18, 3),
+        ('ccx', 1, False, False): (18, 3),
+        ('ccx', 2, False, False): (18, 3),
+        ('ccx', 3, False, False): (18, 3),
     }

lms/djangoapps/courseware/tests/test_view_authentication.py

@@ -86,7 +86,7 @@ class TestViewAuth(ModuleStoreTestCase, LoginEnrollmentTestCase):
 
         # The student progress tab is not accessible to a student
         # before launch, so the instructor view-as-student feature
-        # should return a 404 as well.
+        # should return a 403.
         # TODO (vshnayder): If this is not the behavior we want, will need
         # to make access checking smarter and understand both the effective
         # user (the student), and the requesting user (the prof)
@@ -97,7 +97,7 @@ class TestViewAuth(ModuleStoreTestCase, LoginEnrollmentTestCase):
                 'student_id': self.enrolled_user.id,
             }
         )
-        self.assert_request_status_code(404, url)
+        self.assert_request_status_code(403, url)
 
         # The courseware url should redirect, not 200
         url = self._reverse_urls(['courseware'], course)[0]

lms/djangoapps/courseware/tests/test_views.py

@@ -1345,17 +1345,17 @@ class ProgressPageTests(ModuleStoreTestCase):
         """Test that query counts remain the same for self-paced and instructor-paced courses."""
         SelfPacedConfiguration(enabled=self_paced_enabled).save()
         self.setup_course(self_paced=self_paced)
-        with self.assertNumQueries(34), check_mongo_calls(4):
+        with self.assertNumQueries(35), check_mongo_calls(4):
             self._get_progress_page()
 
     def test_progress_queries(self):
         self.setup_course()
-        with self.assertNumQueries(34), check_mongo_calls(4):
+        with self.assertNumQueries(35), check_mongo_calls(4):
             self._get_progress_page()
 
         # subsequent accesses to the progress page require fewer queries.
         for _ in range(2):
-            with self.assertNumQueries(20), check_mongo_calls(4):
+            with self.assertNumQueries(21), check_mongo_calls(4):
                 self._get_progress_page()
 
     @patch(

lms/djangoapps/courseware/views/views.py

@@ -720,10 +720,6 @@ def _progress(request, course_key, student_id):
     student = User.objects.prefetch_related("groups").get(id=student.id)
 
     course_grade = CourseGradeFactory(student).create(course)
-    if not course_grade.has_access_to_course:
-        # This means the student didn't have access to the course (which the instructor requested)
-        raise Http404
-
     courseware_summary = course_grade.chapter_grades
     grade_summary = course_grade.summary
 

lms/djangoapps/grades/api/views.py

@@ -149,14 +149,6 @@ class UserGradeView(GradeViewMixin, GenericAPIView):
 
         prep_course_for_grading(course, request)
         course_grade = CourseGradeFactory(request.user).create(course)
-        if not course_grade.has_access_to_course:
-            # This means the student didn't have access to the course
-            log.info('User %s not allowed to access grade for course %s', request.user.username, username)
-            return self.make_error_response(
-                status_code=status.HTTP_403_FORBIDDEN,
-                developer_message='The user does not have access to the course.',
-                error_code='user_does_not_have_access_to_course'
-            )
 
         return Response([{
             'username': username,

lms/djangoapps/grades/new/course_grade.py

@@ -4,6 +4,7 @@ CourseGrade Class
 
 from collections import defaultdict
 from django.conf import settings
+from django.core.exceptions import PermissionDenied
 from lazy import lazy
 from logging import getLogger
 from lms.djangoapps.course_blocks.api import get_course_blocks
@@ -11,7 +12,9 @@ from lms.djangoapps.grades.config.models import PersistentGradesEnabledFlag
 from openedx.core.djangoapps.signals.signals import GRADES_UPDATED
 from xmodule import block_metadata_utils
 
+from ..models import PersistentCourseGrade
 from .subsection_grade import SubsectionGradeFactory
+from ..transformer import GradesTransformer
 
 
 log = getLogger(__name__)
@@ -24,8 +27,12 @@ class CourseGrade(object):
     def __init__(self, student, course, course_structure):
         self.student = student
         self.course = course
+        self.course_version = getattr(course, 'course_version', None)
+        self.course_edited_timestamp = getattr(course, 'subtree_edited_on', None)
         self.course_structure = course_structure
-        self.chapter_grades = []
+        self._percent = None
+        self._letter_grade = None
+        self._subsection_grade_factory = SubsectionGradeFactory(self.student, self.course, self.course_structure)
 
     @lazy
     def subsection_grade_totals_by_format(self):
@@ -70,27 +77,46 @@ class CourseGrade(object):
         self._log_event(log.warning, u"grade_value, percent: {0}, grade: {1}".format(percent, letter_grade))
         return grade_value
 
-    @property
-    def has_access_to_course(self):
+    @lazy
+    def chapter_grades(self):
         """
-        Returns whether the course structure as seen by the
-        given student is non-empty.
+        Returns a list of chapters, each containing its subsection grades,
+        display name, and url name.
         """
-        return len(self.course_structure) > 0
+        chapter_grades = []
+        for chapter_key in self.course_structure.get_children(self.course.location):
+            chapter = self.course_structure[chapter_key]
+            chapter_subsection_grades = []
+            children = self.course_structure.get_children(chapter_key)
+            for subsection_key in children:
+                chapter_subsection_grades.append(
+                    self._subsection_grade_factory.create(self.course_structure[subsection_key], read_only=True)
+                )
+
+            chapter_grades.append({
+                'display_name': block_metadata_utils.display_name_with_default_escaped(chapter),
+                'url_name': block_metadata_utils.url_name_for_block(chapter),
+                'sections': chapter_subsection_grades
+            })
+        return chapter_grades
 
     @property
     def percent(self):
         """
         Returns a rounded percent from the overall grade.
         """
-        return self._calc_percent(self.grade_value)
+        if self._percent is None:
+            self._percent = self._calc_percent(self.grade_value)
+        return self._percent
 
     @property
     def letter_grade(self):
         """
         Returns a letter representing the grade.
         """
-        return self._compute_letter_grade(self.percent)
+        if self._letter_grade is None:
+            self._letter_grade = self._compute_letter_grade(self.percent)
+        return self._letter_grade
 
     @property
     def passed(self):
@@ -107,9 +133,6 @@ class CourseGrade(object):
         Returns the grade summary as calculated by the course's grader.
         """
         grade_summary = self.grade_value
-
-        # We round the grade here, to make sure that the grade is a whole percentage and
-        # doesn't get displayed differently than it gets grades
         grade_summary['percent'] = self.percent
         grade_summary['grade'] = self.letter_grade
         grade_summary['totaled_scores'] = self.subsection_grade_totals_by_format
@@ -123,30 +146,24 @@ class CourseGrade(object):
 
         If read_only is True, doesn't save any updates to the grades.
         """
-        subsection_grade_factory = SubsectionGradeFactory(self.student, self.course, self.course_structure)
-        subsections_total = 0
-        for chapter_key in self.course_structure.get_children(self.course.location):
-            chapter = self.course_structure[chapter_key]
-            chapter_subsection_grades = []
-            children = self.course_structure.get_children(chapter_key)
-            subsections_total += len(children)
-            for subsection_key in children:
-                chapter_subsection_grades.append(
-                    subsection_grade_factory.create(self.course_structure[subsection_key], read_only=True)
-                )
-
-            self.chapter_grades.append({
-                'display_name': block_metadata_utils.display_name_with_default_escaped(chapter),
-                'url_name': block_metadata_utils.url_name_for_block(chapter),
-                'sections': chapter_subsection_grades
-            })
+        subsections_total = sum(len(chapter['sections']) for chapter in self.chapter_grades)
 
         total_graded_subsections = sum(len(x) for x in self.subsection_grade_totals_by_format.itervalues())
-        subsections_created = len(subsection_grade_factory._unsaved_subsection_grades)  # pylint: disable=protected-access
+        subsections_created = len(self._subsection_grade_factory._unsaved_subsection_grades)  # pylint: disable=protected-access
         subsections_read = subsections_total - subsections_created
         blocks_total = len(self.locations_to_scores)
         if not read_only:
-            subsection_grade_factory.bulk_create_unsaved()
+            self._subsection_grade_factory.bulk_create_unsaved()
+            grading_policy_hash = self.get_grading_policy_hash(self.course.location, self.course_structure)
+            PersistentCourseGrade.update_or_create_course_grade(
+                user_id=self.student.id,
+                course_id=self.course.id,
+                course_version=self.course_version,
+                course_edited_timestamp=self.course_edited_timestamp,
+                grading_policy_hash=grading_policy_hash,
+                percent_grade=self.percent,
+                letter_grade=self.letter_grade or "",
+            )
 
         self._signal_listeners_when_grade_computed()
         self._log_event(
@@ -184,6 +201,46 @@ class CourseGrade(object):
         return earned, possible
 
     @staticmethod
+    def get_grading_policy_hash(course_location, course_structure):
+        """
+        Gets the grading policy of the course at the given location
+        in the given course structure.
+        """
+        return course_structure.get_transformer_block_field(
+            course_location,
+            GradesTransformer,
+            'grading_policy_hash'
+        )
+
+    @classmethod
+    def load_persisted_grade(cls, user, course, course_structure):
+        """
+        Initializes a CourseGrade object, filling its members with persisted values from the database.
+
+        If the grading policy is out of date, recomputes the grade.
+
+        If no persisted values are found, returns None.
+        """
+        try:
+            persistent_grade = PersistentCourseGrade.read_course_grade(user.id, course.id)
+        except PersistentCourseGrade.DoesNotExist:
+            return None
+        course_grade = CourseGrade(user, course, course_structure)
+
+        current_grading_policy_hash = course_grade.get_grading_policy_hash(course.location, course_structure)
+        if current_grading_policy_hash != persistent_grade.grading_policy_hash:
+            return None
+        else:
+            course_grade._percent = persistent_grade.percent_grade  # pylint: disable=protected-access
+            course_grade._letter_grade = persistent_grade.letter_grade  # pylint: disable=protected-access
+            course_grade.course_version = persistent_grade.course_version
+            course_grade.course_edited_timestamp = persistent_grade.course_edited_timestamp
+
+        course_grade._log_event(log.info, u"load_persisted_grade")  # pylint: disable=protected-access
+
+        return course_grade
+
+    @staticmethod
     def _calc_percent(grade_value):
         """
         Helper for percent calculation.
@@ -253,8 +310,12 @@ class CourseGradeFactory(object):
         Returns the CourseGrade object for the given student and course.
 
         If read_only is True, doesn't save any updates to the grades.
+        Raises a PermissionDenied if the user does not have course access.
         """
         course_structure = get_course_blocks(self.student, course.location)
+        # if user does not have access to this course, throw an exception
+        if not self._user_has_access_to_course(course_structure):
+            raise PermissionDenied("User does not have access to this course")
         return (
             self._get_saved_grade(course, course_structure) or
             self._compute_and_update_grade(course, course_structure, read_only)
@@ -281,13 +342,19 @@ class CourseGradeFactory(object):
         """
         Returns the saved grade for the given course and student.
         """
-        if PersistentGradesEnabledFlag.feature_enabled(course.id):
-            # TODO LATER Retrieve the saved grade for the course, if it exists.
-            _pretend_to_save_course_grades()
+        if not PersistentGradesEnabledFlag.feature_enabled(course.id):
+            return None
 
+        return CourseGrade.load_persisted_grade(
+            self.student,
+            course,
+            course_structure
+        )
 
-def _pretend_to_save_course_grades():
-    """
-    Stub to facilitate testing feature flag until robust grade work lands.
-    """
-    pass
+    def _user_has_access_to_course(self, course_structure):
+        """
+        Given a course structure, returns whether the user
+        for whom that course structure was retrieved
+        has access to the course.
+        """
+        return len(course_structure) > 0

lms/djangoapps/grades/tests/test_new.py

@@ -48,7 +48,8 @@ class GradeTestBase(SharedModuleStoreTestCase):
             parent=cls.chapter,
             category='sequential',
             display_name="Test Sequential 1",
-            graded=True
+            graded=True,
+            format="Homework"
         )
         cls.vertical = ItemFactory.create(
             parent=cls.sequence,
@@ -100,13 +101,35 @@ class TestCourseGradeFactory(GradeTestBase):
             course_id=self.course.id,
             enabled_for_course=course_setting
         ):
-            with patch('lms.djangoapps.grades.new.course_grade._pretend_to_save_course_grades') as mock_save_grades:
+            with patch('lms.djangoapps.grades.new.course_grade.CourseGrade.load_persisted_grade') as mock_save_grades:
                 grade_factory.create(self.course)
         self.assertEqual(mock_save_grades.called, feature_flag and course_setting)
 
+    def test_course_grade_creation(self):
+        grading_policy = {
+            "GRADER": [
+                {
+                    "type": "Homework",
+                    "min_count": 1,
+                    "drop_count": 0,
+                    "short_label": "HW",
+                    "weight": 1.0,
+                },
+            ],
+            "GRADE_CUTOFFS": {
+                "Pass": 0.5,
+            },
+        }
+        self.course.set_grading_policy(grading_policy)
+        grade_factory = CourseGradeFactory(self.request.user)
+        with mock_get_score(1, 2):
+            course_grade = grade_factory.create(self.course)
+        self.assertEqual(course_grade.letter_grade, u'Pass')
+        self.assertEqual(course_grade.percent, 0.5)
+
 
 @ddt.ddt
-class TestSubsectionGradeFactory(GradeTestBase, ProblemSubmissionTestMixin):
+class TestSubsectionGradeFactory(ProblemSubmissionTestMixin, GradeTestBase):
     """
     Tests for SubsectionGradeFactory functionality.
 
@@ -456,7 +479,7 @@ class TestVariedMetadata(ProblemSubmissionTestMixin, ModuleStoreTestCase):
         self.assertEqual(score.graded_total.possible, expected_possible)
 
 
-class TestCourseGradeLogging(SharedModuleStoreTestCase):
+class TestCourseGradeLogging(ProblemSubmissionTestMixin, SharedModuleStoreTestCase):
     """
     Tests logging in the course grades module.
     Uses a larger course structure than other
@@ -504,26 +527,26 @@ class TestCourseGradeLogging(SharedModuleStoreTestCase):
             display_name='Test Vertical 3'
         )
         problem_xml = MultipleChoiceResponseXMLFactory().build_xml(
-            question_text='The correct answer is Choice 3',
+            question_text='The correct answer is Choice 2',
             choices=[False, False, True, False],
             choice_names=['choice_0', 'choice_1', 'choice_2', 'choice_3']
         )
         self.problem = ItemFactory.create(
             parent=self.vertical,
             category="problem",
-            display_name="Test Problem 1",
+            display_name="test_problem_1",
             data=problem_xml
         )
         self.problem_2 = ItemFactory.create(
             parent=self.vertical_2,
             category="problem",
-            display_name="Test Problem 2",
+            display_name="test_problem_2",
             data=problem_xml
         )
         self.problem_3 = ItemFactory.create(
             parent=self.vertical_3,
             category="problem",
-            display_name="Test Problem 3",
+            display_name="test_problem_3",
             data=problem_xml
         )
         self.request = get_request_for_user(UserFactory())
@@ -536,28 +559,14 @@ class TestCourseGradeLogging(SharedModuleStoreTestCase):
             self,
             factory,
             log_mock,
-            read_only,
-            subsections_read,
-            subsections_created,
-            blocks_accessed,
-            total_graded_subsections
+            log_statement
     ):
         """
         Creates a course grade and asserts that the associated logging
         matches the expected totals passed in to the function.
         """
         factory.create(self.course, read_only=False)
-        log_statement = u''.join((
-            u"compute_and_update, read_only: {0}, subsections read/created: {1}/{2}, blocks ",
-            u"accessed: {3}, total graded subsections: {4}"
-        )).format(
-            read_only,
-            subsections_read,
-            subsections_created,
-            blocks_accessed,
-            total_graded_subsections,
-        )
-        log_mock.warning.assert_called_with(
+        log_mock.assert_called_with(
             u"Persistent Grades: CourseGrade.{0}, course: {1}, user: {2}".format(
                 log_statement,
                 unicode(self.course.id),
@@ -574,24 +583,36 @@ class TestCourseGradeLogging(SharedModuleStoreTestCase):
             enabled_for_course=True
         ):
             with patch('lms.djangoapps.grades.new.course_grade.log') as log_mock:
-                # TODO: once merged with the "glue code" PR, update expected logging to include the relevant new info
-
                 # the course grade has not been created, so we expect each grade to be created
+                log_statement = u''.join((
+                    u"compute_and_update, read_only: {0}, subsections read/created: {1}/{2}, blocks ",
+                    u"accessed: {3}, total graded subsections: {4}"
+                )).format(False, 0, 3, 3, 2)
+                self._create_course_grade_and_check_logging(
+                    grade_factory,
+                    log_mock.warning,
+                    log_statement
+                )
+                log_mock.reset_mock()
+
+                # the course grade has been created, so we expect to read it from the db
+                log_statement = u"load_persisted_grade"
                 self._create_course_grade_and_check_logging(
                     grade_factory,
-                    log_mock,
-                    read_only=False,
-                    subsections_read=0,
-                    subsections_created=3,
-                    blocks_accessed=3,
-                    total_graded_subsections=2)
-                # the course grade has been created, so we expect each grade to be read
+                    log_mock.info,
+                    log_statement
+                )
+                log_mock.reset_mock()
+
+                # only problem submission, a subsection grade update triggers
+                # a course grade update
+                self.submit_question_answer(u'test_problem_1', {u'2_1': u'choice_choice_2'})
+                log_statement = u''.join((
+                    u"compute_and_update, read_only: {0}, subsections read/created: {1}/{2}, blocks ",
+                    u"accessed: {3}, total graded subsections: {4}"
+                )).format(False, 3, 0, 3, 2)
                 self._create_course_grade_and_check_logging(
                     grade_factory,
-                    log_mock,
-                    read_only=False,
-                    subsections_read=3,
-                    subsections_created=0,
-                    blocks_accessed=3,
-                    total_graded_subsections=2,
+                    log_mock.warning,
+                    log_statement
                 )

lms/djangoapps/grades/tests/test_tasks.py

@@ -146,7 +146,7 @@ class RecalculateSubsectionGradeTest(ModuleStoreTestCase):
         with self.store.default_store(default_store):
             self.set_up_course()
             self.assertTrue(PersistentGradesEnabledFlag.feature_enabled(self.course.id))
-            with check_mongo_calls(2) and self.assertNumQueries(21 + added_queries):
+            with check_mongo_calls(2) and self.assertNumQueries(25 + added_queries):
                 recalculate_subsection_grade.apply(args=tuple(self.score_changed_kwargs.values()))
 
     def test_single_call_to_create_block_structure(self):
@@ -170,7 +170,7 @@ class RecalculateSubsectionGradeTest(ModuleStoreTestCase):
             self.assertTrue(PersistentGradesEnabledFlag.feature_enabled(self.course.id))
             ItemFactory.create(parent=self.sequential, category='problem', display_name='problem2')
             ItemFactory.create(parent=self.sequential, category='problem', display_name='problem3')
-            with check_mongo_calls(2) and self.assertNumQueries(21 + added_queries):
+            with check_mongo_calls(2) and self.assertNumQueries(25 + added_queries):
                 recalculate_subsection_grade.apply(args=tuple(self.score_changed_kwargs.values()))
 
     @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split)
@@ -179,7 +179,7 @@ class RecalculateSubsectionGradeTest(ModuleStoreTestCase):
             self.set_up_course(enable_subsection_grades=False)
             self.assertFalse(PersistentGradesEnabledFlag.feature_enabled(self.course.id))
             additional_queries = 1 if default_store == ModuleStoreEnum.Type.mongo else 0
-            with check_mongo_calls(2) and self.assertNumQueries(12 + additional_queries):
+            with check_mongo_calls(2) and self.assertNumQueries(16 + additional_queries):
                 recalculate_subsection_grade.apply(args=tuple(self.score_changed_kwargs.values()))
 
     @skip("Pending completion of TNL-5089")

lms/djangoapps/instructor_task/tests/test_tasks_helper.py

@@ -1674,7 +1674,7 @@ class TestCertificateGeneration(InstructorTaskModuleTestCase):
             'failed': 3,
             'skipped': 2
         }
-        with self.assertNumQueries(158):
+        with self.assertNumQueries(166):
             self.assertCertificatesGenerated(task_input, expected_results)
 
         expected_results = {