diff --git a/lms/djangoapps/grades/rest_api/v1/gradebook_views.py b/lms/djangoapps/grades/rest_api/v1/gradebook_views.py index 778f8a2e26..5d466b6078 100644 --- a/lms/djangoapps/grades/rest_api/v1/gradebook_views.py +++ b/lms/djangoapps/grades/rest_api/v1/gradebook_views.py @@ -21,7 +21,7 @@ from rest_framework.response import Response from rest_framework.views import APIView from common.djangoapps.student.auth import has_course_author_access -from common.djangoapps.student.models import CourseEnrollment +from common.djangoapps.student.models import CourseEnrollment, CourseAccessRole from common.djangoapps.student.roles import BulkRoleCache from common.djangoapps.track.event_transaction_utils import ( create_new_event_transaction_id, @@ -618,7 +618,19 @@ class GradebookView(GradeViewMixin, PaginatedAPIView): q_object |= Q(course_grade_in_range=True) q_objects.append(q_object) - + if request.GET.get('excluded_course_roles'): + excluded_course_roles = request.GET.getlist('excluded_course_roles') + course_access_role_filters = dict( + user=OuterRef('user'), + course_id=course_key, + ) + if 'all' not in excluded_course_roles: + course_access_role_filters['role__in'] = excluded_course_roles + annotations['has_excluded_role'] = Exists( + CourseAccessRole.objects.filter(**course_access_role_filters) + ) + # TODO: In django 3.0+, we can directly filter on this 'exists' rather than annotating + q_objects.append(Q(has_excluded_role=False)) entries = [] related_models = ['user'] users = self._paginate_users(course_key, q_objects, related_models, annotations=annotations) diff --git a/lms/djangoapps/grades/rest_api/v1/tests/test_gradebook_views.py b/lms/djangoapps/grades/rest_api/v1/tests/test_gradebook_views.py index 3aceb2878e..0983107e98 100644 --- a/lms/djangoapps/grades/rest_api/v1/tests/test_gradebook_views.py +++ b/lms/djangoapps/grades/rest_api/v1/tests/test_gradebook_views.py @@ -5,6 +5,7 @@ Tests for the course grading API view import json from collections import OrderedDict, namedtuple +from contextlib import contextmanager from datetime import datetime from unittest.mock import MagicMock, patch @@ -19,6 +20,13 @@ from rest_framework import status from rest_framework.test import APITestCase from common.djangoapps.course_modes.models import CourseMode +from common.djangoapps.student.roles import ( + CourseBetaTesterRole, + CourseCcxCoachRole, + CourseDataResearcherRole, + CourseInstructorRole, + CourseStaffRole +) from common.djangoapps.student.tests.factories import CourseEnrollmentFactory, UserFactory from lms.djangoapps.certificates.models import CertificateStatuses, GeneratedCertificate from lms.djangoapps.courseware.tests.factories import InstructorFactory, StaffFactory @@ -1280,6 +1288,142 @@ class GradebookViewTest(GradebookViewTestBase): assert actual_data['total_users_count'] == num_enrollments assert actual_data['filtered_users_count'] == num_enrollments + @contextmanager + def _mock_all_course_grade_reads(self, percent=0.9): + """ + A context manager for mocking CourseGradeFactory.read and returning the same grade for all learners. + """ + # pylint: disable=unused-argument + def fake_course_grade_read(*args, **kwargs): + return self.mock_course_grade(kwargs['user'], passed=True, percent=percent) + + with patch('lms.djangoapps.grades.course_grade_factory.CourseGradeFactory.read') as mock_read: + mock_read.side_effect = fake_course_grade_read + yield + + def _assert_usernames(self, response, expected_usernames): + """ Helper method to assert that the expected users were returned from the endpoint """ + assert status.HTTP_200_OK == response.status_code + response_data = dict(response.data) + actual_usernames = [row['username'] for row in response_data['results']] + assert set(actual_usernames) == set(expected_usernames) + + def test_users_with_course_roles(self): + """ Test that a staff member erolled in the course will be included in grade results. """ + # This function creates and enrolls a course staff (not global staff) user + staff_user = self.login_course_staff() + with override_waffle_flag(self.waffle_flag, active=True): + with self._mock_all_course_grade_reads(): + response = self.client.get(self.get_url(course_key=self.course.id)) + course_students = [ + self.student.username, + self.other_student.username, + self.program_student.username, + self.program_masters_student.username + ] + self._assert_usernames( + response, + course_students + [staff_user.username] + ) + + @ddt.data( + None, + [], + ['all'], + [CourseInstructorRole.ROLE, CourseBetaTesterRole.ROLE, CourseCcxCoachRole.ROLE], + [CourseInstructorRole.ROLE, 'all'], + [CourseBetaTesterRole.ROLE, 'nonexistant-role'], + [ + CourseInstructorRole.ROLE, + CourseStaffRole.ROLE, + CourseBetaTesterRole.ROLE, + CourseCcxCoachRole.ROLE, + CourseDataResearcherRole.ROLE + ], + ) + def test_filter_course_roles(self, excluded_course_roles): + """ Test that excluded_course_roles=all filters out any user with a course role """ + # Create test users, enroll them in the course, and give them roles. + role_user_usernames = dict() + course_roles_to_create = [ + CourseInstructorRole, + CourseStaffRole, + CourseBetaTesterRole, + CourseCcxCoachRole, + CourseDataResearcherRole, + ] + for role in course_roles_to_create: + user = UserFactory.create(username="test_filter_course_roles__" + role.ROLE) + role(self.course.id).add_users(user) + self._create_user_enrollments(user) + role_user_usernames[role.ROLE] = user.username + + # This will create global staff and not enroll them in the course + self.login_staff() + with self._mock_all_course_grade_reads(): + with override_waffle_flag(self.waffle_flag, active=True): + response = self.client.get( + self.get_url(course_key=self.course.id), + {'excluded_course_roles': excluded_course_roles} if excluded_course_roles is not None else {} + ) + + expected_usernames = [ + self.student.username, + self.other_student.username, + self.program_student.username, + self.program_masters_student.username, + ] + if not excluded_course_roles: + # Don't filter out any course roles + expected_usernames += list(role_user_usernames.values()) + elif 'all' in excluded_course_roles: + # Filter out every course role + pass + else: + # Filter out some number of course roles + for role, username in role_user_usernames.items(): + if role not in excluded_course_roles: + expected_usernames.append(username) + + self._assert_usernames(response, expected_usernames) + + @ddt.data(False, True) + def test_exclude_course_roles_for_another_course(self, other_student_role_in_self_course): + """ + Test for filtering errors when users have roles in other courses. + """ + # Conditionally make other_student a beta tester (arbitrary role) in self.course + if other_student_role_in_self_course: + CourseBetaTesterRole(self.course.id).add_users(self.other_student) + + # Create another course, enroll other_student, and make other_student course staff in other course + another_course = CourseFactory.create(display_name='another-course', run='run-1') + CourseEnrollmentFactory( + course_id=another_course.id, + user=self.other_student, + ) + CourseStaffRole(another_course.id).add_users(self.other_student) + + # Query the gradebook view for self.course, excluding staff. + # other_student is staff in another-course, not self.course, so + # they should still be included. + self.login_staff() + with self._mock_all_course_grade_reads(): + with override_waffle_flag(self.waffle_flag, active=True): + response = self.client.get( + self.get_url(course_key=self.course.id), + {'excluded_course_roles': CourseStaffRole.ROLE} + ) + self._assert_usernames( + response, + [ + self.student.username, + self.other_student.username, + self.program_student.username, + self.program_masters_student.username, + ] + ) + @ddt.ddt class GradebookBulkUpdateViewTest(GradebookViewTestBase):