diff --git a/lms/djangoapps/grades/api/v1/gradebook_views.py b/lms/djangoapps/grades/api/v1/gradebook_views.py index c9fb1b372f..e289a5d29f 100644 --- a/lms/djangoapps/grades/api/v1/gradebook_views.py +++ b/lms/djangoapps/grades/api/v1/gradebook_views.py @@ -26,6 +26,7 @@ from lms.djangoapps.grades.api.v1.utils import ( ) from lms.djangoapps.grades.config.waffle import WRITABLE_GRADEBOOK, waffle_flags from lms.djangoapps.grades.constants import ScoreDatabaseTableEnum +from lms.djangoapps.grades.context import graded_subsections_for_course from lms.djangoapps.grades.course_data import CourseData from lms.djangoapps.grades.course_grade_factory import CourseGradeFactory from lms.djangoapps.grades.events import SUBSECTION_GRADE_CALCULATED, subsection_grade_calculated @@ -538,19 +539,6 @@ class GradebookView(GradeViewMixin, PaginatedAPIView): return self.get_paginated_response(serializer.data) -def graded_subsections_for_course(course_structure): - """ - Given a course block structure, yields the subsections of the course that are graded. - Args: - course_structure: A course structure object. Not user-specific. - """ - for chapter_key in course_structure.get_children(course_structure.root_block_usage_key): - for subsection_key in course_structure.get_children(chapter_key): - subsection = course_structure[subsection_key] - if subsection.graded: - yield subsection - - GradebookUpdateResponseItem = namedtuple('GradebookUpdateResponseItem', ['user_id', 'usage_id', 'success', 'reason']) diff --git a/lms/djangoapps/grades/api/v1/tests/test_gradebook_views.py b/lms/djangoapps/grades/api/v1/tests/test_gradebook_views.py index a1e6525830..fcd9645c0d 100644 --- a/lms/djangoapps/grades/api/v1/tests/test_gradebook_views.py +++ b/lms/djangoapps/grades/api/v1/tests/test_gradebook_views.py @@ -293,6 +293,17 @@ class GradebookViewTestBase(GradeViewTestMixin, APITestCase): ), ], } + + # Data about graded subsections visible to staff only + # should not be exposed via the gradebook API + cls.hidden_subsection = ItemFactory.create( + parent_location=cls.chapter_1.location, + category='sequential', + graded=True, + visible_to_staff_only=True, + display_name='Hidden Section', + ) + cls.course_data = CourseData(None, course=cls.course) # we have to force the collection of course data from the block_structure API # so that CourseGrade.course_data objects can later have a non-null effective_structure @@ -479,6 +490,12 @@ class GradebookViewTest(GradebookViewTestBase): self.assertIsNone(actual_data['next']) self.assertIsNone(actual_data['previous']) self.assertEqual(expected_results, actual_data['results']) + # assert that the hidden subsection data is not represented in the response + for actual_user_data in actual_data['results']: + actual_subsection_display_names = [ + item['subsection_name'] for item in actual_user_data['section_breakdown'] + ] + self.assertNotIn(self.hidden_subsection.display_name, actual_subsection_display_names) def _assert_empty_response(self, response): """ @@ -584,6 +601,11 @@ class GradebookViewTest(GradebookViewTestBase): self.assertEqual(status.HTTP_200_OK, resp.status_code) actual_data = dict(resp.data) self.assertEqual(expected_results, actual_data) + # assert that the hidden subsection data is not represented in the response + actual_subsection_display_names = [ + item['subsection_name'] for item in actual_data['section_breakdown'] + ] + self.assertNotIn(self.hidden_subsection.display_name, actual_subsection_display_names) @ddt.data( 'login_staff', diff --git a/lms/djangoapps/grades/context.py b/lms/djangoapps/grades/context.py index 0c49f959f9..d2f211f7a5 100644 --- a/lms/djangoapps/grades/context.py +++ b/lms/djangoapps/grades/context.py @@ -17,6 +17,30 @@ def grading_context_for_course(course): return grading_context(course, course_structure) +def visible_to_staff_only(subsection): + """ + Returns True if the given subsection is visible to staff only else False + """ + try: + return subsection.transformer_data['visibility'].fields['merged_visible_to_staff_only'] + except KeyError: + return False + + +def graded_subsections_for_course(course_structure): + """ + Given a course block structure, yields the subsections of the course that are graded + and visible to non-staff users. + Args: + course_structure: A course structure object. + """ + for chapter_key in course_structure.get_children(course_structure.root_block_usage_key): + for subsection_key in course_structure.get_children(chapter_key): + subsection = course_structure[subsection_key] + if not visible_to_staff_only(subsection) and subsection.graded: + yield subsection + + def grading_context(course, course_structure): """ This returns a dictionary with keys necessary for quickly grading @@ -40,32 +64,29 @@ def grading_context(course, course_structure): count_all_graded_blocks = 0 all_graded_subsections_by_type = OrderedDict() - for chapter_key in course_structure.get_children(course_structure.root_block_usage_key): - for subsection_key in course_structure.get_children(chapter_key): - subsection = course_structure[subsection_key] - scored_descendants_of_subsection = [] - if subsection.graded: - for descendant_key in course_structure.post_order_traversal( - filter_func=possibly_scored, - start_node=subsection_key, - ): - scored_descendants_of_subsection.append( - course_structure[descendant_key], - ) + for subsection in graded_subsections_for_course(course_structure): + scored_descendants_of_subsection = [] + for descendant_key in course_structure.post_order_traversal( + filter_func=possibly_scored, + start_node=subsection.location, + ): + scored_descendants_of_subsection.append( + course_structure[descendant_key], + ) - # include only those blocks that have scores, not if they are just a parent - subsection_info = { - 'subsection_block': subsection, - 'scored_descendants': [ - child for child in scored_descendants_of_subsection - if getattr(child, 'has_score', None) - ] - } - subsection_format = getattr(subsection, 'format', '') - if subsection_format not in all_graded_subsections_by_type: - all_graded_subsections_by_type[subsection_format] = [] - all_graded_subsections_by_type[subsection_format].append(subsection_info) - count_all_graded_blocks += len(scored_descendants_of_subsection) + # include only those blocks that have scores, not if they are just a parent + subsection_info = { + 'subsection_block': subsection, + 'scored_descendants': [ + child for child in scored_descendants_of_subsection + if getattr(child, 'has_score', None) + ] + } + subsection_format = getattr(subsection, 'format', '') + if subsection_format not in all_graded_subsections_by_type: + all_graded_subsections_by_type[subsection_format] = [] + all_graded_subsections_by_type[subsection_format].append(subsection_info) + count_all_graded_blocks += len(scored_descendants_of_subsection) return { 'all_graded_subsections_by_type': all_graded_subsections_by_type, diff --git a/lms/djangoapps/instructor_task/tests/test_base.py b/lms/djangoapps/instructor_task/tests/test_base.py index cc6a284c8f..ee534f3bf9 100644 --- a/lms/djangoapps/instructor_task/tests/test_base.py +++ b/lms/djangoapps/instructor_task/tests/test_base.py @@ -360,10 +360,33 @@ class TestReportMixin(object): {key: row.get(key) for key in expected_rows[index].keys()} for index, row in enumerate(csv_rows) ] + numeric_csv_rows = [self._extract_and_round_numeric_items(row) for row in csv_rows] + numeric_expected_rows = [self._extract_and_round_numeric_items(row) for row in expected_rows] + if verify_order: self.assertEqual(csv_rows, expected_rows) + self.assertEqual(numeric_csv_rows, numeric_expected_rows) else: self.assertItemsEqual(csv_rows, expected_rows) + self.assertItemsEqual(numeric_csv_rows, numeric_expected_rows) + + @staticmethod + def _extract_and_round_numeric_items(dictionary): + """ + csv data may contain numeric values that are converted to strings, and fractional + numbers can be imprecise (e.g. 1 / 6 is sometimes '0.16666666666666666' and other times + '0.166666666667'). This function mutates the provided input (sorry) and returns + a new dictionary that contains only the numerically-valued items from it, rounded + to four decimal places. + """ + extracted = {} + for key, value in dictionary.items(): + try: + float(value) + extracted[key] = round(float(dictionary.pop(key)), 4) + except ValueError: + pass + return extracted def get_csv_row_with_headers(self): """ diff --git a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py index e7c3ae82ac..30bbb6c81c 100644 --- a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py +++ b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py @@ -7,6 +7,7 @@ Unit tests for LMS instructor-initiated background tasks helper functions. - Tests all of the existing reports. """ +from __future__ import unicode_literals import os import shutil @@ -765,7 +766,7 @@ class TestInstructorDetailedEnrollmentReport(TestReportMixin, InstructorTaskCour response = self.client.get(redeem_url) self.assertEquals(response.status_code, 200) # check button text - self.assertIn('Activate Course Enrollment', response.content) + self.assertIn('Activate Course Enrollment', response.content.decode('utf-8')) response = self.client.post(redeem_url) self.assertEquals(response.status_code, 200) @@ -799,7 +800,7 @@ class TestInstructorDetailedEnrollmentReport(TestReportMixin, InstructorTaskCour response = self.client.get(redeem_url) self.assertEquals(response.status_code, 200) # check button text - self.assertIn('Activate Course Enrollment', response.content) + self.assertIn('Activate Course Enrollment', response.content.decode('utf-8')) response = self.client.post(redeem_url) self.assertEquals(response.status_code, 200) @@ -840,7 +841,7 @@ class TestInstructorDetailedEnrollmentReport(TestReportMixin, InstructorTaskCour response = self.client.get(redeem_url) self.assertEquals(response.status_code, 200) # check button text - self.assertIn('Activate Course Enrollment', response.content) + self.assertIn('Activate Course Enrollment', response.content.decode('utf-8')) response = self.client.post(redeem_url) self.assertEquals(response.status_code, 200) @@ -1325,7 +1326,7 @@ class TestExecutiveSummaryReport(TestReportMixin, InstructorTaskCourseTestCase): response = self.client.get(redeem_url) self.assertEquals(response.status_code, 200) # check button text - self.assertIn('Activate Course Enrollment', response.content) + self.assertIn('Activate Course Enrollment', response.content.decode('utf-8')) response = self.client.post(redeem_url) self.assertEquals(response.status_code, 200) @@ -1954,7 +1955,6 @@ class TestGradeReport(TestReportMixin, InstructorTaskModuleTestCase): with patch('lms.djangoapps.instructor_task.tasks_helper.runner._get_current_task'): result = CourseGradeReport.generate(None, None, self.course.id, None, 'graded') - self.assertDictContainsSubset( {'action_name': 'graded', 'attempted': 1, 'succeeded': 1, 'failed': 0}, result, @@ -1967,10 +1967,9 @@ class TestGradeReport(TestReportMixin, InstructorTaskModuleTestCase): u'Username': self.student.username, u'Grade': '0.13', u'Homework 1: Subsection': '0.5', - u'Homework 2: Hidden': u'Not Attempted', - u'Homework 3: Unattempted': u'Not Attempted', - u'Homework 4: Empty': u'Not Attempted', - u'Homework (Avg)': '0.125', + u'Homework 2: Unattempted': 'Not Attempted', + u'Homework 3: Empty': 'Not Attempted', + u'Homework (Avg)': text_type(1.0 / 6.0), }, ], ignore_other_columns=True,