diff --git a/lms/djangoapps/ccx/tests/test_field_override_performance.py b/lms/djangoapps/ccx/tests/test_field_override_performance.py index 6bfff67baf..1a8fc76fe3 100644 --- a/lms/djangoapps/ccx/tests/test_field_override_performance.py +++ b/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), } diff --git a/lms/djangoapps/courseware/tests/test_view_authentication.py b/lms/djangoapps/courseware/tests/test_view_authentication.py index 48dd842a74..304ce1544a 100644 --- a/lms/djangoapps/courseware/tests/test_view_authentication.py +++ b/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] diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index b37f573417..af31a7c724 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/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( diff --git a/lms/djangoapps/courseware/views/views.py b/lms/djangoapps/courseware/views/views.py index 1b60b16ec2..662020978b 100644 --- a/lms/djangoapps/courseware/views/views.py +++ b/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 diff --git a/lms/djangoapps/grades/api/views.py b/lms/djangoapps/grades/api/views.py index b46f02ccc5..24c6892928 100644 --- a/lms/djangoapps/grades/api/views.py +++ b/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, diff --git a/lms/djangoapps/grades/new/course_grade.py b/lms/djangoapps/grades/new/course_grade.py index 04c0036533..261971f16d 100644 --- a/lms/djangoapps/grades/new/course_grade.py +++ b/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( @@ -183,6 +200,46 @@ class CourseGrade(object): possible += child_possible 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): """ @@ -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 diff --git a/lms/djangoapps/grades/tests/test_new.py b/lms/djangoapps/grades/tests/test_new.py index 9dc425bc93..b0829352cb 100644 --- a/lms/djangoapps/grades/tests/test_new.py +++ b/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, - 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 - 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 + ) + 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.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.warning, + log_statement ) diff --git a/lms/djangoapps/grades/tests/test_tasks.py b/lms/djangoapps/grades/tests/test_tasks.py index 8066c17dd9..959d1c1ff5 100644 --- a/lms/djangoapps/grades/tests/test_tasks.py +++ b/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") diff --git a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py index 74459c1ce9..0ba1320b6c 100644 --- a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py +++ b/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 = {