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"""