diff --git a/lms/djangoapps/django_comment_client/forum/tests.py b/lms/djangoapps/django_comment_client/forum/tests.py index 518cbfc8a0..9e3046759b 100644 --- a/lms/djangoapps/django_comment_client/forum/tests.py +++ b/lms/djangoapps/django_comment_client/forum/tests.py @@ -11,7 +11,6 @@ from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from django.core.urlresolvers import reverse from util.testing import UrlResetMixin from django_comment_client.tests.group_id import ( - GroupIdAssertionMixin, CohortedTopicGroupIdTestMixin, NonCohortedTopicGroupIdTestMixin ) @@ -570,7 +569,13 @@ class ForumFormDiscussionGroupIdTestCase(CohortedContentTestCase, CohortedTopicG class UserProfileDiscussionGroupIdTestCase(CohortedContentTestCase, CohortedTopicGroupIdTestMixin): cs_endpoint = "/active_threads" - def call_view(self, mock_request, commentable_id, user, group_id, pass_group_id=True, is_ajax=False): + def call_view_for_profiled_user( + self, mock_request, requesting_user, profiled_user, group_id, pass_group_id, is_ajax=False + ): + """ + Calls "user_profile" view method on behalf of "requesting_user" to get information about + the user "profiled_user". + """ kwargs = {} if group_id: kwargs['group_id'] = group_id @@ -587,12 +592,17 @@ class UserProfileDiscussionGroupIdTestCase(CohortedContentTestCase, CohortedTopi data=request_data, **headers ) - request.user = user + request.user = requesting_user mako_middleware_process_request(request) return views.user_profile( request, self.course.id.to_deprecated_string(), - user.id + profiled_user.id + ) + + def call_view(self, mock_request, _commentable_id, user, group_id, pass_group_id=True, is_ajax=False): + return self.call_view_for_profiled_user( + mock_request, user, user, group_id, pass_group_id=pass_group_id, is_ajax=is_ajax ) def test_group_info_in_html_response(self, mock_request): @@ -617,6 +627,109 @@ class UserProfileDiscussionGroupIdTestCase(CohortedContentTestCase, CohortedTopi response, lambda d: d['discussion_data'][0] ) + def _test_group_id_passed_to_user_profile( + self, mock_request, expect_group_id_in_request, requesting_user, profiled_user, group_id, pass_group_id + ): + """ + Helper method for testing whether or not group_id was passed to the user_profile request. + """ + + def get_params_from_user_info_call(for_specific_course): + """ + Returns the request parameters for the user info call with either course_id specified or not, + depending on value of 'for_specific_course'. + """ + # There will be 3 calls from user_profile. One has the cs_endpoint "active_threads", and it is already + # tested. The other 2 calls are for user info; one of those calls is for general information about the user, + # and it does not specify a course_id. The other call does specify a course_id, and if the caller did not + # have discussion moderator privileges, it should also contain a group_id. + for r_call in mock_request.call_args_list: + if not r_call[0][1].endswith(self.cs_endpoint): + params = r_call[1]["params"] + has_course_id = "course_id" in params + if (for_specific_course and has_course_id) or (not for_specific_course and not has_course_id): + return params + self.assertTrue( + False, + "Did not find appropriate user_profile call for 'for_specific_course'=" + for_specific_course + ) + + mock_request.reset_mock() + self.call_view_for_profiled_user( + mock_request, + requesting_user, + profiled_user, + group_id, + pass_group_id=pass_group_id, + is_ajax=False + ) + # Should never have a group_id if course_id was not included in the request. + params_without_course_id = get_params_from_user_info_call(False) + self.assertNotIn("group_id", params_without_course_id) + + params_with_course_id = get_params_from_user_info_call(True) + if expect_group_id_in_request: + self.assertIn("group_id", params_with_course_id) + self.assertEqual(group_id, params_with_course_id["group_id"]) + else: + self.assertNotIn("group_id", params_with_course_id) + + def test_group_id_passed_to_user_profile_student(self, mock_request): + """ + Test that the group id is always included when requesting user profile information for a particular + course if the requester does not have discussion moderation privileges. + """ + def verify_group_id_always_present(profiled_user, pass_group_id): + """ + Helper method to verify that group_id is always present for student in course + (non-privileged user). + """ + self._test_group_id_passed_to_user_profile( + mock_request, True, self.student, profiled_user, self.student_cohort.id, pass_group_id + ) + + # In all these test cases, the requesting_user is the student (non-privileged user). + # The profile returned on behalf of the student is for the profiled_user. + verify_group_id_always_present(profiled_user=self.student, pass_group_id=True) + verify_group_id_always_present(profiled_user=self.student, pass_group_id=False) + verify_group_id_always_present(profiled_user=self.moderator, pass_group_id=True) + verify_group_id_always_present(profiled_user=self.moderator, pass_group_id=False) + + def test_group_id_user_profile_moderator(self, mock_request): + """ + Test that the group id is only included when a privileged user requests user profile information for a + particular course and user if the group_id is explicitly passed in. + """ + def verify_group_id_present(profiled_user, pass_group_id, requested_cohort=self.moderator_cohort): + """ + Helper method to verify that group_id is present. + """ + self._test_group_id_passed_to_user_profile( + mock_request, True, self.moderator, profiled_user, requested_cohort.id, pass_group_id + ) + + def verify_group_id_not_present(profiled_user, pass_group_id, requested_cohort=self.moderator_cohort): + """ + Helper method to verify that group_id is not present. + """ + self._test_group_id_passed_to_user_profile( + mock_request, False, self.moderator, profiled_user, requested_cohort.id, pass_group_id + ) + + # In all these test cases, the requesting_user is the moderator (privileged user). + + # If the group_id is explicitly passed, it will be present in the request. + verify_group_id_present(profiled_user=self.student, pass_group_id=True) + verify_group_id_present(profiled_user=self.moderator, pass_group_id=True) + verify_group_id_present( + profiled_user=self.student, pass_group_id=True, requested_cohort=self.student_cohort + ) + + # If the group_id is not explicitly passed, it will not be present because the requesting_user + # has discussion moderator privileges. + verify_group_id_not_present(profiled_user=self.student, pass_group_id=False) + verify_group_id_not_present(profiled_user=self.moderator, pass_group_id=False) + @patch('lms.lib.comment_client.utils.requests.request') class FollowedThreadsDiscussionGroupIdTestCase(CohortedContentTestCase, CohortedTopicGroupIdTestMixin): diff --git a/lms/djangoapps/django_comment_client/forum/views.py b/lms/djangoapps/django_comment_client/forum/views.py index 962a7f1f0b..e17cacf886 100644 --- a/lms/djangoapps/django_comment_client/forum/views.py +++ b/lms/djangoapps/django_comment_client/forum/views.py @@ -325,8 +325,6 @@ def user_profile(request, course_id, user_id): #TODO: Allow sorting? course = get_course_with_access(request.user, 'load_forum', course_key) try: - profiled_user = cc.User(id=user_id, course_id=course_key) - query_params = { 'page': request.GET.get('page', 1), 'per_page': THREADS_PER_PAGE, # more than threads_per_page to show more activities @@ -338,6 +336,9 @@ def user_profile(request, course_id, user_id): return HttpResponseBadRequest("Invalid group_id") if group_id is not None: query_params['group_id'] = group_id + profiled_user = cc.User(id=user_id, course_id=course_key, group_id=group_id) + else: + profiled_user = cc.User(id=user_id, course_id=course_key) threads, page, num_pages = profiled_user.active_threads(query_params) query_params['page'] = page diff --git a/lms/lib/comment_client/user.py b/lms/lib/comment_client/user.py index 7eea4aa077..c4a69d7837 100644 --- a/lms/lib/comment_client/user.py +++ b/lms/lib/comment_client/user.py @@ -8,7 +8,7 @@ class User(models.Model): accessible_fields = ['username', 'follower_ids', 'upvoted_ids', 'downvoted_ids', 'id', 'external_id', 'subscribed_user_ids', 'children', 'course_id', - 'subscribed_thread_ids', 'subscribed_commentable_ids', + 'group_id', 'subscribed_thread_ids', 'subscribed_commentable_ids', 'subscribed_course_ids', 'threads_count', 'comments_count', 'default_sort_key' ] @@ -120,6 +120,8 @@ class User(models.Model): retrieve_params.update(kwargs) if self.attributes.get('course_id'): retrieve_params['course_id'] = self.course_id.to_deprecated_string() + if self.attributes.get('group_id'): + retrieve_params['group_id'] = self.group_id try: response = perform_request( 'get',