From a162140492d256be7cde5a53cb24ba221bc5cf5b Mon Sep 17 00:00:00 2001 From: Michael Terry Date: Fri, 11 Mar 2022 09:01:13 -0500 Subject: [PATCH] fix: have progress MFE API recalculate visible course grade Previously, the course grade returned would be the stored grade, which is calculated for all content, not just the visible grades. (Some grades are not yet released to the learner.) This fix recalculates the overall grade before sending to the MFE, so that it doesn't have to recompute it itself (without all the particular logic that the platform uses when grading). AA-1217 --- .../progress/tests/test_views.py | 74 +++++++++++++------ .../course_home_api/progress/views.py | 3 + .../tests/test_submitting_problems.py | 2 +- lms/djangoapps/grades/course_grade.py | 34 ++++++--- lms/djangoapps/grades/course_grade_factory.py | 1 - .../instructor_task/tests/test_integration.py | 4 +- 6 files changed, 81 insertions(+), 37 deletions(-) diff --git a/lms/djangoapps/course_home_api/progress/tests/test_views.py b/lms/djangoapps/course_home_api/progress/tests/test_views.py index 7c0198e407..4c330c7cc3 100644 --- a/lms/djangoapps/course_home_api/progress/tests/test_views.py +++ b/lms/djangoapps/course_home_api/progress/tests/test_views.py @@ -3,15 +3,16 @@ Tests for Progress Tab API in the Course Home API """ from datetime import datetime, timedelta +from unittest.mock import patch + import dateutil import ddt from django.conf import settings - -from pytz import UTC -from unittest.mock import patch # lint-amnesty, pylint: disable=wrong-import-order from django.urls import reverse from django.utils.timezone import now from edx_toggles.toggles.testutils import override_waffle_flag +from pytz import UTC +from xmodule.modulestore.tests.factories import ItemFactory from common.djangoapps.course_modes.models import CourseMode from common.djangoapps.student.models import CourseEnrollment @@ -19,22 +20,25 @@ from common.djangoapps.student.tests.factories import UserFactory from lms.djangoapps.course_home_api.tests.utils import BaseCourseHomeTests from lms.djangoapps.course_home_api.models import DisableProgressPageStackedConfig from lms.djangoapps.course_home_api.toggles import COURSE_HOME_MICROFRONTEND_PROGRESS_TAB +from lms.djangoapps.grades.api import CourseGradeFactory from lms.djangoapps.grades.config.tests.utils import persistent_grades_feature_flags from lms.djangoapps.grades.constants import GradeOverrideFeatureEnum from lms.djangoapps.grades.models import ( PersistentSubsectionGrade, PersistentSubsectionGradeOverride ) +from lms.djangoapps.grades.tests.utils import answer_problem from lms.djangoapps.verify_student.models import ManualVerification from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from openedx.core.djangoapps.course_date_signals.utils import MIN_DURATION +from openedx.core.djangolib.testing.utils import get_mock_request from openedx.features.content_type_gating.helpers import CONTENT_GATING_PARTITION_ID, CONTENT_TYPE_GATE_GROUP_IDS from openedx.features.content_type_gating.models import ContentTypeGatingConfig from openedx.features.course_duration_limits.models import CourseDurationLimitConfig -from xmodule.modulestore.tests.factories import ItemFactory # lint-amnesty, pylint: disable=wrong-import-order @ddt.ddt +@override_waffle_flag(COURSE_HOME_MICROFRONTEND_PROGRESS_TAB, active=True) class ProgressTabTestViews(BaseCourseHomeTests): """ Tests for the Progress Tab API @@ -43,7 +47,14 @@ class ProgressTabTestViews(BaseCourseHomeTests): super().setUp() self.url = reverse('course-home:progress-tab', args=[self.course.id]) - @override_waffle_flag(COURSE_HOME_MICROFRONTEND_PROGRESS_TAB, active=True) + def add_subsection_with_problem(self, **kwargs): + """Makes a chapter -> problem chain, and sets up the subsection as requested, returning the problem""" + chapter = ItemFactory(parent=self.course, category='chapter') + subsection = ItemFactory(parent=chapter, category='sequential', graded=True, **kwargs) + vertical = ItemFactory(parent=subsection, category='vertical', graded=True) + problem = ItemFactory(parent=vertical, category='problem', graded=True) + return problem + @ddt.data(CourseMode.AUDIT, CourseMode.VERIFIED) def test_get_authenticated_enrolled_user(self, enrollment_mode): CourseEnrollment.enroll(self.user, self.course.id, enrollment_mode) @@ -64,7 +75,6 @@ class ProgressTabTestViews(BaseCourseHomeTests): elif enrollment_mode == CourseMode.AUDIT: assert response.data['certificate_data']['cert_status'] == 'audit_passing' - @override_waffle_flag(COURSE_HOME_MICROFRONTEND_PROGRESS_TAB, active=True) @ddt.data(True, False) def test_get_authenticated_user_not_enrolled(self, has_previously_enrolled): if has_previously_enrolled: @@ -74,13 +84,11 @@ class ProgressTabTestViews(BaseCourseHomeTests): response = self.client.get(self.url) assert response.status_code == 401 - @override_waffle_flag(COURSE_HOME_MICROFRONTEND_PROGRESS_TAB, active=True) def test_get_unauthenticated_user(self): self.client.logout() response = self.client.get(self.url) assert response.status_code == 401 - @override_waffle_flag(COURSE_HOME_MICROFRONTEND_PROGRESS_TAB, active=True) def test_get_unknown_course(self): url = reverse('course-home:progress-tab', args=['course-v1:unknown+course+2T2020']) response = self.client.get(url) @@ -93,7 +101,6 @@ class ProgressTabTestViews(BaseCourseHomeTests): response = self.client.get(self.url) assert response.status_code == 404 - @override_waffle_flag(COURSE_HOME_MICROFRONTEND_PROGRESS_TAB, active=True) def test_masquerade(self): # Enroll a verified user verified_user = UserFactory(is_staff=False) @@ -112,7 +119,6 @@ class ProgressTabTestViews(BaseCourseHomeTests): assert self.client.get(self.url).data.get('enrollment_mode') == 'verified' @patch.dict('django.conf.settings.FEATURES', {'DISABLE_START_DATES': False}) - @override_waffle_flag(COURSE_HOME_MICROFRONTEND_PROGRESS_TAB, active=True) def test_has_scheduled_content_data(self): CourseEnrollment.enroll(self.user, self.course.id) future = now() + timedelta(days=30) @@ -121,7 +127,6 @@ class ProgressTabTestViews(BaseCourseHomeTests): assert response.status_code == 200 assert response.json()['has_scheduled_content'] - @override_waffle_flag(COURSE_HOME_MICROFRONTEND_PROGRESS_TAB, active=True) def test_end(self): CourseEnrollment.enroll(self.user, self.course.id) future = now() + timedelta(days=30) @@ -132,7 +137,6 @@ class ProgressTabTestViews(BaseCourseHomeTests): end = dateutil.parser.parse(response.json()['end']).replace(tzinfo=UTC) assert end.date() == future.date() - @override_waffle_flag(COURSE_HOME_MICROFRONTEND_PROGRESS_TAB, active=True) def test_user_has_passing_grade(self): CourseEnrollment.enroll(self.user, self.course.id) self.course.grade_cutoffs = {'Pass': 0} @@ -141,7 +145,6 @@ class ProgressTabTestViews(BaseCourseHomeTests): assert response.status_code == 200 assert response.json()['user_has_passing_grade'] - @override_waffle_flag(COURSE_HOME_MICROFRONTEND_PROGRESS_TAB, active=True) def test_verified_mode(self): enrollment = CourseEnrollment.enroll(self.user, self.course.id) CourseDurationLimitConfig.objects.create(enabled=True, enabled_as_of=datetime(2018, 1, 1)) @@ -152,7 +155,6 @@ class ProgressTabTestViews(BaseCourseHomeTests): 'currency': 'USD', 'currency_symbol': '$', 'price': 149, 'sku': 'ABCD1234', 'upgrade_url': '/dashboard'} - @override_waffle_flag(COURSE_HOME_MICROFRONTEND_PROGRESS_TAB, active=True) def test_page_respects_stacked_config(self): CourseEnrollment.enroll(self.user, self.course.id) course_overview = CourseOverview.get_from_id(self.course.id) @@ -165,7 +167,6 @@ class ProgressTabTestViews(BaseCourseHomeTests): response = self.client.get(self.url) assert response.status_code == 404 - @override_waffle_flag(COURSE_HOME_MICROFRONTEND_PROGRESS_TAB, active=True) def test_learner_has_access(self): chapter = ItemFactory(parent=self.course, category='chapter') gated = ItemFactory(parent=chapter, category='sequential') @@ -192,7 +193,6 @@ class ProgressTabTestViews(BaseCourseHomeTests): assert not gated_score['learner_has_access'] assert ungated_score['learner_has_access'] - @override_waffle_flag(COURSE_HOME_MICROFRONTEND_PROGRESS_TAB, active=True) @patch.dict(settings.FEATURES, {'ASSUME_ZERO_GRADE_IF_ABSENT_FOR_ALL_TESTS': False}) def test_override_is_visible(self): with persistent_grades_feature_flags(global_flag=True): @@ -216,7 +216,7 @@ class ProgressTabTestViews(BaseCourseHomeTests): created_grade = PersistentSubsectionGrade.update_or_create_grade(**params) proctoring_failure_comment = "Failed Test Proctoring" - override = PersistentSubsectionGradeOverride.update_or_create_override( + PersistentSubsectionGradeOverride.update_or_create_override( requesting_user=self.staff_user, subsection_grade_model=created_grade, earned_all_override=0.0, @@ -236,7 +236,6 @@ class ProgressTabTestViews(BaseCourseHomeTests): assert override_entry['system'] == GradeOverrideFeatureEnum.proctoring assert override_entry['reason'] == proctoring_failure_comment - @override_waffle_flag(COURSE_HOME_MICROFRONTEND_PROGRESS_TAB, active=True) def test_view_other_students_progress_page(self): # Test the ability to view progress pages of other students by changing the url CourseEnrollment.enroll(self.user, self.course.id) @@ -261,13 +260,10 @@ class ProgressTabTestViews(BaseCourseHomeTests): response = self.client.get(self.url) assert response.data['username'] == other_user.username - @override_waffle_flag(COURSE_HOME_MICROFRONTEND_PROGRESS_TAB, active=True) def test_url_hidden_if_subsection_hide_after_due(self): chapter = ItemFactory(parent=self.course, category='chapter') yesterday = now() - timedelta(days=1) - hide_after_due_subsection = ItemFactory( - parent=chapter, category='sequential', hide_after_due=True, due=yesterday - ) + ItemFactory(parent=chapter, category='sequential', hide_after_due=True, due=yesterday) CourseEnrollment.enroll(self.user, self.course.id) @@ -279,3 +275,37 @@ class ProgressTabTestViews(BaseCourseHomeTests): hide_after_due_subsection = sections[1]['subsections'][0] assert regular_subsection['url'] is not None assert hide_after_due_subsection['url'] is None + + @ddt.data( + (True, 0.7), # midterm and final are visible to staff + (False, 0.3), # just the midterm is visible to learners + ) + @ddt.unpack + def test_course_grade_considers_subsection_grade_visibility(self, is_staff, expected_percent): + """ + Verify that the grade & is_passing info we send out is for visible grades only. + + Assumes that grading policy is the default one (search for DEFAULT_GRADING_POLICY). + """ + if is_staff: + self.switch_to_staff() + CourseEnrollment.enroll(self.user, self.course.id) + + tomorrow = now() + timedelta(days=1) + with self.store.bulk_operations(self.course.id): + never = self.add_subsection_with_problem(format='Homework', show_correctness='never') + always = self.add_subsection_with_problem(format='Midterm Exam', show_correctness='always') + past_due = self.add_subsection_with_problem(format='Final Exam', show_correctness='past_due', due=tomorrow) + + answer_problem(self.course, get_mock_request(self.user), never) + answer_problem(self.course, get_mock_request(self.user), always) + answer_problem(self.course, get_mock_request(self.user), past_due) + + # First, confirm the grade in the database - it should never change based on user state. + # This is midterm and final and a single problem added together. + assert CourseGradeFactory().read(self.user, self.course).percent == 0.72 + + response = self.client.get(self.url) + assert response.status_code == 200 + assert response.data['course_grade']['percent'] == expected_percent + assert response.data['course_grade']['is_passing'] == (expected_percent >= 0.5) diff --git a/lms/djangoapps/course_home_api/progress/views.py b/lms/djangoapps/course_home_api/progress/views.py index 70afa42f7b..4f9731b9b6 100644 --- a/lms/djangoapps/course_home_api/progress/views.py +++ b/lms/djangoapps/course_home_api/progress/views.py @@ -204,6 +204,9 @@ class ProgressTabView(RetrieveAPIView): collected_block_structure = get_block_structure_manager(course_key).get_collected() course_grade = CourseGradeFactory().read(student, collected_block_structure=collected_block_structure) + # recalculate course grade from visible grades (stored grade was calculated over all grades, visible or not) + course_grade.update(visible_grades_only=True, has_staff_access=is_staff) + # Get has_scheduled_content data transformers = BlockStructureTransformers() transformers += [start_date.StartDateTransformer(), ContentTypeGateTransformer()] diff --git a/lms/djangoapps/courseware/tests/test_submitting_problems.py b/lms/djangoapps/courseware/tests/test_submitting_problems.py index 959458afce..fa9f381160 100644 --- a/lms/djangoapps/courseware/tests/test_submitting_problems.py +++ b/lms/djangoapps/courseware/tests/test_submitting_problems.py @@ -282,7 +282,7 @@ class TestSubmittingProblems(ModuleStoreTestCase, LoginEnrollmentTestCase, Probl Returns list of scores: [, , ..., ] """ return [ - s.graded_total.earned for s in self.get_course_grade().graded_subsections_by_format['Homework'].values() + s.graded_total.earned for s in self.get_course_grade().graded_subsections_by_format()['Homework'].values() ] def hw_grade(self, hw_url_name): diff --git a/lms/djangoapps/grades/course_grade.py b/lms/djangoapps/grades/course_grade.py index d354fde821..551e5bafbe 100644 --- a/lms/djangoapps/grades/course_grade.py +++ b/lms/djangoapps/grades/course_grade.py @@ -41,6 +41,18 @@ class CourseGradeBase: self.passed, ) + def update(self, visible_grades_only=False, has_staff_access=False): + """ + Recalculates the grade for the course, with the given parameters. + + Also updates subsection grades if self.force_update_subsections is true. + + Arguments: + visible_grades_only: Only considers grades the user can see (via show_correctness subsection field) + has_staff_access: Used to help determine which grades are visible (if visible_grades_only=True) + """ + return self + @property def attempted(self): """ @@ -71,8 +83,7 @@ class CourseGradeBase: ) return self._get_subsection_grade(subsection) - @lazy - def graded_subsections_by_format(self): + def graded_subsections_by_format(self, visible_grades_only=False, has_staff_access=False): """ Returns grades for the subsections in the course in a dict keyed by subsection format types. @@ -80,7 +91,8 @@ class CourseGradeBase: subsections_by_format = defaultdict(OrderedDict) for chapter in self.chapter_grades.values(): for subsection_grade in chapter['sections']: - if subsection_grade.graded: + is_visible = not visible_grades_only or subsection_grade.show_grades(has_staff_access) + if subsection_grade.graded and is_visible: graded_total = subsection_grade.graded_total if graded_total.possible > 0: subsections_by_format[subsection_grade.format][subsection_grade.location] = subsection_grade @@ -156,14 +168,14 @@ class CourseGradeBase: possible += child_possible return earned, possible - @lazy - def grader_result(self): + def grader_result(self, visible_grades_only=False, has_staff_access=False): """ Returns the result from the course grader. """ course = self._prep_course_for_grading(self.course_data.course) return course.grader.grade( - self.graded_subsections_by_format, + self.graded_subsections_by_format(visible_grades_only=visible_grades_only, + has_staff_access=has_staff_access), generate_random_scores=settings.GENERATE_PROFILE_SCORES, ) @@ -174,7 +186,7 @@ class CourseGradeBase: DEPRECATED: To be removed as part of TNL-5291. """ # TODO(TNL-5291) Remove usages of this deprecated property. - grade_summary = self.grader_result + grade_summary = self.grader_result() grade_summary['percent'] = self.percent grade_summary['grade'] = self.letter_grade return grade_summary @@ -259,11 +271,10 @@ class CourseGrade(CourseGradeBase): super().__init__(user, course_data, *args, **kwargs) self._subsection_grade_factory = SubsectionGradeFactory(user, course_data=course_data) - def update(self): + def update(self, visible_grades_only=False, has_staff_access=False): """ Updates the grade for the course. Also updates subsection grades - if self.force_update_subsections is true, via the lazy call - to self.grader_result. + if self.force_update_subsections is true, via the call to self.grader_result. """ # TODO update this code to be more functional and readable. # Currently, it is hard to follow since there are plenty of @@ -271,7 +282,8 @@ class CourseGrade(CourseGradeBase): # can be passed through and not confusingly stored and used # at a later time. grade_cutoffs = self.course_data.course.grade_cutoffs - self.percent = self._compute_percent(self.grader_result) + grader_result = self.grader_result(visible_grades_only=visible_grades_only, has_staff_access=has_staff_access) + self.percent = self._compute_percent(grader_result) self.letter_grade = self._compute_letter_grade(grade_cutoffs, self.percent) self.passed = self._compute_passed(grade_cutoffs, self.percent) return self diff --git a/lms/djangoapps/grades/course_grade_factory.py b/lms/djangoapps/grades/course_grade_factory.py index 3ca0199f5d..1c64f97589 100644 --- a/lms/djangoapps/grades/course_grade_factory.py +++ b/lms/djangoapps/grades/course_grade_factory.py @@ -103,7 +103,6 @@ class CourseGradeFactory: course_data = CourseData( user=None, course=course, collected_block_structure=collected_block_structure, course_key=course_key, ) - stats_tags = [f'action:{course_data.course_key}'] # lint-amnesty, pylint: disable=unused-variable for user in users: yield self._iter_grade_result(user, course_data, force_update) diff --git a/lms/djangoapps/instructor_task/tests/test_integration.py b/lms/djangoapps/instructor_task/tests/test_integration.py index d10433fa82..c170b74f6e 100644 --- a/lms/djangoapps/instructor_task/tests/test_integration.py +++ b/lms/djangoapps/instructor_task/tests/test_integration.py @@ -134,8 +134,8 @@ class TestRescoringTask(TestIntegrationTask): expected_subsection_grade = expected_score course_grade = CourseGradeFactory().read(user, self.course) - assert course_grade.graded_subsections_by_format['Homework'][self.problem_section.location].graded_total.earned\ - == expected_subsection_grade + grade = course_grade.graded_subsections_by_format()['Homework'][self.problem_section.location].graded_total + assert grade.earned == expected_subsection_grade def submit_rescore_all_student_answers(self, instructor, problem_url_name, only_if_higher=False): """Submits the particular problem for rescoring"""