From d0f99ec7786ca90826045ae9856aee67836595d5 Mon Sep 17 00:00:00 2001 From: Simon Chen Date: Mon, 1 Jun 2020 11:11:37 -0400 Subject: [PATCH] MST-264 Fix bug for course staff user in unrelated course (#24099) * MST-264 Fix the wrong assumption that a course_staff of a course not related to programs should be in course_staff logic --- .../rest_api/v1/tests/test_views.py | 62 ++++++++++++++++--- .../program_enrollments/rest_api/v1/views.py | 30 +++++---- 2 files changed, 72 insertions(+), 20 deletions(-) diff --git a/lms/djangoapps/program_enrollments/rest_api/v1/tests/test_views.py b/lms/djangoapps/program_enrollments/rest_api/v1/tests/test_views.py index 7bba79bde5..dace29ab72 100644 --- a/lms/djangoapps/program_enrollments/rest_api/v1/tests/test_views.py +++ b/lms/djangoapps/program_enrollments/rest_api/v1/tests/test_views.py @@ -1490,13 +1490,28 @@ class UserProgramReadOnlyAccessGetTests(EnrollmentsDataMixin, APITestCase): with mock.patch( _VIEW_PATCH_FORMAT.format('get_programs'), autospec=True, - return_value=[self.mock_program_data[0]] + side_effect=[[self.mock_program_data[0]], []] ) as mock_get_programs: response = self.client.get(reverse(self.view_name) + '?type=masters') assert status.HTTP_200_OK == response.status_code assert len(response.data) == 1 - mock_get_programs.assert_called_once_with(course=self.course_id) + mock_get_programs.assert_has_calls([ + mock.call(course=self.course_id), + mock.call(uuids=[]), + ], any_order=True) + + def _enroll_user_into_course_as_course_staff(self, user, course_key_string): + """ + This is a helper function to create a course run based on the course key string, + then enroll the user to the course run as a course staff. + """ + course_key_to_create = CourseKey.from_string(course_key_string) + CourseOverviewFactory(id=course_key_to_create) + CourseRunFactory.create(key=text_type(course_key_to_create)) + CourseEnrollmentFactory.create(course_id=course_key_to_create, user=user) + CourseStaffRole(course_key_to_create).add_users(user) + return course_key_to_create @ddt.data( ( @@ -1520,11 +1535,11 @@ class UserProgramReadOnlyAccessGetTests(EnrollmentsDataMixin, APITestCase): return program return None - other_course_key = CourseKey.from_string('course-v1:edX+ToyX+Other_Course') - CourseOverviewFactory(id=other_course_key) - CourseRunFactory.create(key=text_type(other_course_key)) - CourseEnrollmentFactory.create(course_id=other_course_key, user=self.course_staff) - CourseStaffRole(other_course_key).add_users(self.course_staff) + other_course_key = self._enroll_user_into_course_as_course_staff( + self.course_staff, + 'course-v1:edX+ToyX+Other_Course' + ) + self.client.login(username=self.course_staff.username, password=self.password) programs_to_return_first = [ @@ -1543,7 +1558,7 @@ class UserProgramReadOnlyAccessGetTests(EnrollmentsDataMixin, APITestCase): with mock.patch( _VIEW_PATCH_FORMAT.format('get_programs'), autospec=True, - side_effect=[programs_to_return_first, programs_to_return_second] + side_effect=[[], programs_to_return_first, programs_to_return_second] ) as mock_get_programs: response = self.client.get(reverse(self.view_name) + '?type=masters') @@ -1554,6 +1569,37 @@ class UserProgramReadOnlyAccessGetTests(EnrollmentsDataMixin, APITestCase): mock.call(course=other_course_key), ], any_order=True) + def test_course_staff_of_non_program_course(self): + created_course_key = self._enroll_user_into_course_as_course_staff( + self.student, + 'course-v1:edX+ToyX+Other_Course' + ) + + program_to_enroll = self.mock_program_data[0] + ProgramEnrollmentFactory.create( + program_uuid=program_to_enroll['uuid'], + curriculum_uuid=self.curriculum_uuid, + user=self.student, + status='enrolled', + external_user_key='user-{}'.format(self.student.id), + ) + + self.client.login(username=self.student.username, password=self.password) + + with mock.patch( + _VIEW_PATCH_FORMAT.format('get_programs'), + autospec=True, + side_effect=[[], [program_to_enroll]] + ) as mock_get_programs: + response = self.client.get(reverse(self.view_name)) + + assert status.HTTP_200_OK == response.status_code + assert len(response.data) == 1 + mock_get_programs.assert_has_calls([ + mock.call(course=created_course_key), + mock.call(uuids=[UUID(program_to_enroll['uuid'])]), + ]) + @mock.patch(_VIEW_PATCH_FORMAT.format('get_programs'), autospec=True, return_value=None) def test_learner_200_if_no_programs_enrolled(self, mock_get_programs): self.client.login(username=self.student.username, password=self.password) diff --git a/lms/djangoapps/program_enrollments/rest_api/v1/views.py b/lms/djangoapps/program_enrollments/rest_api/v1/views.py index 6036563ac2..05a25affec 100644 --- a/lms/djangoapps/program_enrollments/rest_api/v1/views.py +++ b/lms/djangoapps/program_enrollments/rest_api/v1/views.py @@ -709,15 +709,17 @@ class UserProgramReadOnlyAccessView(DeveloperErrorViewMixin, PaginatedAPIView): if request_user.is_staff: programs = get_programs_by_type(request.site, requested_program_type) - elif self.is_course_staff(request_user): - programs = self.get_programs_user_is_course_staff_for(request_user, requested_program_type) else: - program_enrollments = fetch_program_enrollments_by_student( - user=request.user, - program_enrollment_statuses=ProgramEnrollmentStatuses.__ACTIVE__, - ) - uuids = [enrollment.program_uuid for enrollment in program_enrollments] - programs = get_programs(uuids=uuids) or [] + program_dict = {} + # Check if the user is a course staff of any course which is a part of a program. + for staff_program in self.get_programs_user_is_course_staff_for(request_user, requested_program_type): + program_dict.setdefault(staff_program['uuid'], staff_program) + + # Now get the program enrollments for user purely as a learner add to the list + for learner_program in self._get_enrolled_programs_from_model(request_user): + program_dict.setdefault(learner_program['uuid'], learner_program) + + programs = list(program_dict.values()) programs_in_which_user_has_access = [ {'uuid': program['uuid'], 'slug': program['marketing_slug']} @@ -726,12 +728,16 @@ class UserProgramReadOnlyAccessView(DeveloperErrorViewMixin, PaginatedAPIView): return Response(programs_in_which_user_has_access, status.HTTP_200_OK) - def is_course_staff(self, user): + def _get_enrolled_programs_from_model(self, user): """ - Returns true if the user is a course_staff member of any course within a program + Return the Program Enrollments linked to the learner within the data model. """ - staff_course_keys = self.get_course_keys_user_is_staff_for(user) - return len(staff_course_keys) + program_enrollments = fetch_program_enrollments_by_student( + user=user, + program_enrollment_statuses=ProgramEnrollmentStatuses.__ACTIVE__, + ) + uuids = [enrollment.program_uuid for enrollment in program_enrollments] + return get_programs(uuids=uuids) or [] def get_course_keys_user_is_staff_for(self, user): """