diff --git a/lms/djangoapps/django_comment_client/base/tests.py b/lms/djangoapps/django_comment_client/base/tests.py index 531d6c87e2..51a38c4a9f 100644 --- a/lms/djangoapps/django_comment_client/base/tests.py +++ b/lms/djangoapps/django_comment_client/base/tests.py @@ -12,7 +12,7 @@ from opaque_keys.edx.locations import SlashSeparatedCourseKey from courseware.tests.modulestore_config import TEST_DATA_MIXED_MODULESTORE from django_comment_client.base import views -from django_comment_client.tests.group_id import CohortedTopicGroupIdTestMixin, NonCohortedTopicGroupIdTestMixin +from django_comment_client.tests.group_id import CohortedTopicGroupIdTestMixin, NonCohortedTopicGroupIdTestMixin, GroupIdAssertionMixin from django_comment_client.tests.utils import CohortedContentTestCase from django_comment_client.tests.unicode import UnicodeTestMixin from django_comment_common.models import Role, FORUM_ROLE_STUDENT @@ -29,15 +29,16 @@ CS_PREFIX = "http://localhost:4567/api/v1" class MockRequestSetupMixin(object): - def _create_repsonse_mock(self, data): - return Mock(text=json.dumps(data), json=Mock(return_value=data))\ + def _create_response_mock(self, data): + return Mock(text=json.dumps(data), json=Mock(return_value=data)) def _set_mock_request_data(self, mock_request, data): - mock_request.return_value = self._create_repsonse_mock(data) + mock_request.return_value = self._create_response_mock(data) @patch('lms.lib.comment_client.utils.requests.request') class CreateThreadGroupIdTestCase( + MockRequestSetupMixin, CohortedContentTestCase, CohortedTopicGroupIdTestMixin, NonCohortedTopicGroupIdTestMixin @@ -45,6 +46,7 @@ class CreateThreadGroupIdTestCase( cs_endpoint = "/threads" def call_view(self, mock_request, commentable_id, user, group_id, pass_group_id=True): + self._set_mock_request_data(mock_request, {}) mock_request.return_value.status_code = 200 request_data = {"body": "body", "title": "title", "thread_type": "discussion"} if pass_group_id: @@ -59,6 +61,105 @@ class CreateThreadGroupIdTestCase( commentable_id=commentable_id ) + def test_group_info_in_response(self, mock_request): + response = self.call_view( + mock_request, + "cohorted_topic", + self.student, + None + ) + self._assert_json_response_contains_group_info(response) + + +@patch('lms.lib.comment_client.utils.requests.request') +class ThreadActionGroupIdTestCase( + MockRequestSetupMixin, + CohortedContentTestCase, + GroupIdAssertionMixin +): + def call_view( + self, + view_name, + mock_request, + user=None, + post_params=None, + view_args=None + ): + self._set_mock_request_data( + mock_request, + { + "user_id": str(self.student.id), + "group_id": self.student_cohort.id, + "closed": False, + "type": "thread" + } + ) + mock_request.return_value.status_code = 200 + request = RequestFactory().post("dummy_url", post_params or {}) + request.user = user or self.student + request.view_name = view_name + + return getattr(views, view_name)( + request, + course_id=self.course.id.to_deprecated_string(), + thread_id="dummy", + **(view_args or {}) + ) + + def test_update(self, mock_request): + response = self.call_view( + "update_thread", + mock_request, + post_params={"body": "body", "title": "title"} + ) + self._assert_json_response_contains_group_info(response) + + def test_delete(self, mock_request): + response = self.call_view("delete_thread", mock_request) + self._assert_json_response_contains_group_info(response) + + def test_vote(self, mock_request): + response = self.call_view( + "vote_for_thread", + mock_request, + view_args={"value": "up"} + ) + self._assert_json_response_contains_group_info(response) + response = self.call_view("undo_vote_for_thread", mock_request) + self._assert_json_response_contains_group_info(response) + + def test_flag(self, mock_request): + response = self.call_view("flag_abuse_for_thread", mock_request) + self._assert_json_response_contains_group_info(response) + response = self.call_view("un_flag_abuse_for_thread", mock_request) + self._assert_json_response_contains_group_info(response) + + def test_pin(self, mock_request): + response = self.call_view( + "pin_thread", + mock_request, + user=self.moderator + ) + self._assert_json_response_contains_group_info(response) + response = self.call_view( + "un_pin_thread", + mock_request, + user=self.moderator + ) + self._assert_json_response_contains_group_info(response) + + def test_openclose(self, mock_request): + response = self.call_view( + "openclose_thread", + mock_request, + user=self.moderator + ) + self._assert_json_response_contains_group_info( + response, + lambda d: d['content'] + ) + + @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE) @patch('lms.lib.comment_client.utils.requests.request') @@ -678,9 +779,9 @@ class ViewPermissionsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSet def handle_request(*args, **kwargs): url = args[1] if "/threads/" in url: - return self._create_repsonse_mock(thread_data) + return self._create_response_mock(thread_data) elif "/comments/" in url: - return self._create_repsonse_mock(comment_data) + return self._create_response_mock(comment_data) else: raise ArgumentError("Bad url to mock request") mock_request.side_effect = handle_request diff --git a/lms/djangoapps/django_comment_client/base/views.py b/lms/djangoapps/django_comment_client/base/views.py index fe95d7dc35..50fe1a24eb 100644 --- a/lms/djangoapps/django_comment_client/base/views.py +++ b/lms/djangoapps/django_comment_client/base/views.py @@ -27,8 +27,7 @@ from django_comment_client.utils import ( get_ability, JsonError, JsonResponse, - safe_content, - add_thread_group_name, + prepare_content, get_group_id_for_comments_service ) from django_comment_client.permissions import check_permissions_by_view, cached_has_permission @@ -60,7 +59,7 @@ def ajax_content_response(request, course_key, content): user_info = cc.User.from_django_user(request.user).to_dict() annotated_content_info = get_annotated_content_info(course_key, content, request.user, user_info) return JsonResponse({ - 'content': safe_content(content, course_key), + 'content': prepare_content(content, course_key), 'annotated_content_info': annotated_content_info, }) @@ -122,12 +121,11 @@ def create_thread(request, course_id, commentable_id): user = cc.User.from_django_user(request.user) user.follow(thread) data = thread.to_dict() - add_thread_group_name(data, course_key) add_courseware_context([data], course) if request.is_ajax(): return ajax_content_response(request, course_key, data) else: - return JsonResponse(safe_content(data, course_key)) + return JsonResponse(prepare_content(data, course_key)) @require_POST @@ -146,10 +144,11 @@ def update_thread(request, course_id, thread_id): thread.body = request.POST["body"] thread.title = request.POST["title"] thread.save() + if request.is_ajax(): return ajax_content_response(request, course_key, thread.to_dict()) else: - return JsonResponse(safe_content(thread.to_dict(), course_key)) + return JsonResponse(prepare_content(thread.to_dict(), course_key)) def _create_comment(request, course_key, thread_id=None, parent_id=None): @@ -190,7 +189,7 @@ def _create_comment(request, course_key, thread_id=None, parent_id=None): if request.is_ajax(): return ajax_content_response(request, course_key, comment.to_dict()) else: - return JsonResponse(safe_content(comment.to_dict(), course.id)) + return JsonResponse(prepare_content(comment.to_dict(), course.id)) @require_POST @@ -218,7 +217,8 @@ def delete_thread(request, course_id, thread_id): course_key = SlashSeparatedCourseKey.from_deprecated_string(course_id) thread = cc.Thread.find(thread_id) thread.delete() - return JsonResponse(safe_content(thread.to_dict(), course_key)) + + return JsonResponse(prepare_content(thread.to_dict(), course_key)) @require_POST @@ -238,7 +238,7 @@ def update_comment(request, course_id, comment_id): if request.is_ajax(): return ajax_content_response(request, course_key, comment.to_dict()) else: - return JsonResponse(safe_content(comment.to_dict(), course_key)) + return JsonResponse(prepare_content(comment.to_dict(), course_key)) @require_POST @@ -254,7 +254,7 @@ def endorse_comment(request, course_id, comment_id): comment.endorsed = request.POST.get('endorsed', 'false').lower() == 'true' comment.endorsement_user_id = request.user.id comment.save() - return JsonResponse(safe_content(comment.to_dict(), course_key)) + return JsonResponse(prepare_content(comment.to_dict(), course_key)) @require_POST @@ -269,10 +269,10 @@ def openclose_thread(request, course_id, thread_id): thread = cc.Thread.find(thread_id) thread.closed = request.POST.get('closed', 'false').lower() == 'true' thread.save() - thread = thread.to_dict() + return JsonResponse({ - 'content': safe_content(thread, course_key), - 'ability': get_ability(course_key, thread, request.user), + 'content': prepare_content(thread.to_dict(), course_key), + 'ability': get_ability(course_key, thread.to_dict(), request.user), }) @@ -301,7 +301,7 @@ def delete_comment(request, course_id, comment_id): course_key = SlashSeparatedCourseKey.from_deprecated_string(course_id) comment = cc.Comment.find(comment_id) comment.delete() - return JsonResponse(safe_content(comment.to_dict(), course_key)) + return JsonResponse(prepare_content(comment.to_dict(), course_key)) @require_POST @@ -315,7 +315,7 @@ def vote_for_comment(request, course_id, comment_id, value): user = cc.User.from_django_user(request.user) comment = cc.Comment.find(comment_id) user.vote(comment, value) - return JsonResponse(safe_content(comment.to_dict(), course_key)) + return JsonResponse(prepare_content(comment.to_dict(), course_key)) @require_POST @@ -330,7 +330,7 @@ def undo_vote_for_comment(request, course_id, comment_id): user = cc.User.from_django_user(request.user) comment = cc.Comment.find(comment_id) user.unvote(comment) - return JsonResponse(safe_content(comment.to_dict(), course_key)) + return JsonResponse(prepare_content(comment.to_dict(), course_key)) @require_POST @@ -345,7 +345,8 @@ def vote_for_thread(request, course_id, thread_id, value): user = cc.User.from_django_user(request.user) thread = cc.Thread.find(thread_id) user.vote(thread, value) - return JsonResponse(safe_content(thread.to_dict(), course_key)) + + return JsonResponse(prepare_content(thread.to_dict(), course_key)) @require_POST @@ -360,7 +361,8 @@ def flag_abuse_for_thread(request, course_id, thread_id): user = cc.User.from_django_user(request.user) thread = cc.Thread.find(thread_id) thread.flagAbuse(user, thread) - return JsonResponse(safe_content(thread.to_dict(), course_key)) + + return JsonResponse(prepare_content(thread.to_dict(), course_key)) @require_POST @@ -377,7 +379,8 @@ def un_flag_abuse_for_thread(request, course_id, thread_id): thread = cc.Thread.find(thread_id) remove_all = cached_has_permission(request.user, 'openclose_thread', course_key) or has_access(request.user, 'staff', course) thread.unFlagAbuse(user, thread, remove_all) - return JsonResponse(safe_content(thread.to_dict(), course_key)) + + return JsonResponse(prepare_content(thread.to_dict(), course_key)) @require_POST @@ -392,7 +395,7 @@ def flag_abuse_for_comment(request, course_id, comment_id): user = cc.User.from_django_user(request.user) comment = cc.Comment.find(comment_id) comment.flagAbuse(user, comment) - return JsonResponse(safe_content(comment.to_dict(), course_key)) + return JsonResponse(prepare_content(comment.to_dict(), course_key)) @require_POST @@ -409,7 +412,7 @@ def un_flag_abuse_for_comment(request, course_id, comment_id): remove_all = cached_has_permission(request.user, 'openclose_thread', course_key) or has_access(request.user, 'staff', course) comment = cc.Comment.find(comment_id) comment.unFlagAbuse(user, comment, remove_all) - return JsonResponse(safe_content(comment.to_dict(), course_key)) + return JsonResponse(prepare_content(comment.to_dict(), course_key)) @require_POST @@ -424,7 +427,8 @@ def undo_vote_for_thread(request, course_id, thread_id): user = cc.User.from_django_user(request.user) thread = cc.Thread.find(thread_id) user.unvote(thread) - return JsonResponse(safe_content(thread.to_dict(), course_key)) + + return JsonResponse(prepare_content(thread.to_dict(), course_key)) @require_POST @@ -439,7 +443,8 @@ def pin_thread(request, course_id, thread_id): user = cc.User.from_django_user(request.user) thread = cc.Thread.find(thread_id) thread.pin(user, thread_id) - return JsonResponse(safe_content(thread.to_dict(), course_key)) + + return JsonResponse(prepare_content(thread.to_dict(), course_key)) @require_POST @@ -454,7 +459,8 @@ def un_pin_thread(request, course_id, thread_id): user = cc.User.from_django_user(request.user) thread = cc.Thread.find(thread_id) thread.un_pin(user, thread_id) - return JsonResponse(safe_content(thread.to_dict(), course_key)) + + return JsonResponse(prepare_content(thread.to_dict(), course_key)) @require_POST diff --git a/lms/djangoapps/django_comment_client/forum/tests.py b/lms/djangoapps/django_comment_client/forum/tests.py index c11da0a0dc..6951318a1f 100644 --- a/lms/djangoapps/django_comment_client/forum/tests.py +++ b/lms/djangoapps/django_comment_client/forum/tests.py @@ -122,7 +122,7 @@ def make_mock_request_impl(text, thread_id="dummy_thread_id", group_id=None): def mock_request_impl(*args, **kwargs): url = args[1] data = None - if url.endswith("threads"): + if url.endswith("threads") or url.endswith("user_profile"): data = { "collection": [make_mock_thread_data(text, thread_id, False, group_id=group_id)] } @@ -406,15 +406,19 @@ class SingleThreadAccessTestCase(CohortedContentTestCase): class SingleThreadGroupIdTestCase(CohortedContentTestCase, CohortedTopicGroupIdTestMixin): cs_endpoint = "/threads" - def call_view(self, mock_request, commentable_id, user, group_id, pass_group_id=True): + def call_view(self, mock_request, commentable_id, user, group_id, pass_group_id=True, is_ajax=False): mock_request.side_effect = make_mock_request_impl("dummy context", group_id=self.student_cohort.id) request_data = {} if pass_group_id: request_data["group_id"] = group_id + headers = {} + if is_ajax: + headers['HTTP_X_REQUESTED_WITH'] = "XMLHttpRequest" request = RequestFactory().get( "dummy_url", - data=request_data + data=request_data, + **headers ) request.user = user mako_middleware_process_request(request) @@ -425,6 +429,28 @@ class SingleThreadGroupIdTestCase(CohortedContentTestCase, CohortedTopicGroupIdT "dummy_thread_id" ) + def test_group_info_in_html_response(self, mock_request): + response = self.call_view( + mock_request, + "cohorted_topic", + self.student, + self.student_cohort.id, + is_ajax=False + ) + self._assert_html_response_contains_group_info(response) + + def test_group_info_in_ajax_response(self, mock_request): + response = self.call_view( + mock_request, + "cohorted_topic", + self.student, + self.student_cohort.id, + is_ajax=True + ) + self._assert_json_response_contains_group_info( + response, lambda d: d['content'] + ) + @patch('lms.lib.comment_client.utils.requests.request') class InlineDiscussionGroupIdTestCase( @@ -435,7 +461,17 @@ class InlineDiscussionGroupIdTestCase( cs_endpoint = "/threads" def call_view(self, mock_request, commentable_id, user, group_id, pass_group_id=True): - mock_request.side_effect = make_mock_request_impl("dummy content") + kwargs = {} + if group_id: + # avoid causing a server error when the LMS chokes attempting + # to find a group name for the group_id, when we're testing with + # an invalid one. + try: + CourseUserGroup.objects.get(id=group_id) + kwargs['group_id'] = group_id + except CourseUserGroup.DoesNotExist: + pass + mock_request.side_effect = make_mock_request_impl("dummy content", **kwargs) request_data = {} if pass_group_id: @@ -451,20 +487,38 @@ class InlineDiscussionGroupIdTestCase( commentable_id ) + def test_group_info_in_ajax_response(self, mock_request): + response = self.call_view( + mock_request, + "cohorted_topic", + self.student, + self.student_cohort.id + ) + self._assert_json_response_contains_group_info( + response, lambda d: d['discussion_data'][0] + ) + @patch('lms.lib.comment_client.utils.requests.request') class ForumFormDiscussionGroupIdTestCase(CohortedContentTestCase, CohortedTopicGroupIdTestMixin): cs_endpoint = "/threads" - def call_view(self, mock_request, commentable_id, user, group_id, pass_group_id=True): - mock_request.side_effect = make_mock_request_impl("dummy content") + def call_view(self, mock_request, commentable_id, user, group_id, pass_group_id=True, is_ajax=False): + kwargs = {} + if group_id: + kwargs['group_id'] = group_id + mock_request.side_effect = make_mock_request_impl("dummy content", **kwargs) request_data = {} if pass_group_id: request_data["group_id"] = group_id + headers = {} + if is_ajax: + headers['HTTP_X_REQUESTED_WITH'] = "XMLHttpRequest" request = RequestFactory().get( "dummy_url", - data=request_data + data=request_data, + **headers ) request.user = user mako_middleware_process_request(request) @@ -473,20 +527,47 @@ class ForumFormDiscussionGroupIdTestCase(CohortedContentTestCase, CohortedTopicG self.course.id.to_deprecated_string() ) + def test_group_info_in_html_response(self, mock_request): + response = self.call_view( + mock_request, + "cohorted_topic", + self.student, + self.student_cohort.id + ) + self._assert_html_response_contains_group_info(response) + + def test_group_info_in_ajax_response(self, mock_request): + response = self.call_view( + mock_request, + "cohorted_topic", + self.student, + self.student_cohort.id, + is_ajax=True + ) + self._assert_json_response_contains_group_info( + response, lambda d: d['discussion_data'][0] + ) @patch('lms.lib.comment_client.utils.requests.request') class UserProfileDiscussionGroupIdTestCase(CohortedContentTestCase, CohortedTopicGroupIdTestMixin): cs_endpoint = "/active_threads" - def call_view(self, mock_request, commentable_id, user, group_id, pass_group_id=True): - mock_request.side_effect = make_mock_request_impl("dummy content") + def call_view(self, mock_request, commentable_id, user, group_id, pass_group_id=True, is_ajax=False): + kwargs = {} + if group_id: + kwargs['group_id'] = group_id + mock_request.side_effect = make_mock_request_impl("dummy content", **kwargs) request_data = {} if pass_group_id: request_data["group_id"] = group_id + headers = {} + if is_ajax: + headers['HTTP_X_REQUESTED_WITH'] = "XMLHttpRequest" request = RequestFactory().get( "dummy_url", - data=request_data + data=request_data, + **headers ) request.user = user mako_middleware_process_request(request) @@ -496,13 +577,38 @@ class UserProfileDiscussionGroupIdTestCase(CohortedContentTestCase, CohortedTopi user.id ) + def test_group_info_in_html_response(self, mock_request): + response = self.call_view( + mock_request, + "cohorted_topic", + self.student, + self.student_cohort.id, + is_ajax=False + ) + self._assert_html_response_contains_group_info(response) + + def test_group_info_in_ajax_response(self, mock_request): + response = self.call_view( + mock_request, + "cohorted_topic", + self.student, + self.student_cohort.id, + is_ajax=True + ) + self._assert_json_response_contains_group_info( + response, lambda d: d['discussion_data'][0] + ) + @patch('lms.lib.comment_client.utils.requests.request') class FollowedThreadsDiscussionGroupIdTestCase(CohortedContentTestCase, CohortedTopicGroupIdTestMixin): cs_endpoint = "/subscribed_threads" def call_view(self, mock_request, commentable_id, user, group_id, pass_group_id=True): - mock_request.side_effect = make_mock_request_impl("dummy content") + kwargs = {} + if group_id: + kwargs['group_id'] = group_id + mock_request.side_effect = make_mock_request_impl("dummy content", **kwargs) request_data = {} if pass_group_id: @@ -519,6 +625,17 @@ class FollowedThreadsDiscussionGroupIdTestCase(CohortedContentTestCase, Cohorted user.id ) + def test_group_info_in_ajax_response(self, mock_request): + response = self.call_view( + mock_request, + "cohorted_topic", + self.student, + self.student_cohort.id + ) + self._assert_json_response_contains_group_info( + response, lambda d: d['discussion_data'][0] + ) + @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE) @patch('requests.request') diff --git a/lms/djangoapps/django_comment_client/forum/views.py b/lms/djangoapps/django_comment_client/forum/views.py index ec8e090643..0f450e66ec 100644 --- a/lms/djangoapps/django_comment_client/forum/views.py +++ b/lms/djangoapps/django_comment_client/forum/views.py @@ -20,7 +20,6 @@ from django_comment_client.utils import ( extract, strip_none, add_courseware_context, - add_thread_group_name, get_group_id_for_comments_service ) import django_comment_client.utils as utils @@ -114,10 +113,7 @@ def get_threads(request, course_key, discussion_id=None, per_page=THREADS_PER_PA threads, page, num_pages, corrected_text = cc.Thread.search(query_params) - #now add the group name if the thread has a group id for thread in threads: - add_thread_group_name(thread, course_key) - #patch for backward compatibility to comments service if not 'pinned' in thread: thread['pinned'] = False @@ -135,27 +131,27 @@ def inline_discussion(request, course_id, discussion_id): Renders JSON for DiscussionModules """ nr_transaction = newrelic.agent.current_transaction() - course_id = SlashSeparatedCourseKey.from_deprecated_string(course_id) + course_key = SlashSeparatedCourseKey.from_deprecated_string(course_id) - course = get_course_with_access(request.user, 'load_forum', course_id) + course = get_course_with_access(request.user, 'load_forum', course_key) cc_user = cc.User.from_django_user(request.user) user_info = cc_user.to_dict() try: - threads, query_params = get_threads(request, course_id, discussion_id, per_page=INLINE_THREADS_PER_PAGE) + threads, query_params = get_threads(request, course_key, discussion_id, per_page=INLINE_THREADS_PER_PAGE) except ValueError: return HttpResponseBadRequest("Invalid group_id") with newrelic.agent.FunctionTrace(nr_transaction, "get_metadata_for_threads"): - annotated_content_info = utils.get_metadata_for_threads(course_id, threads, request.user, user_info) + annotated_content_info = utils.get_metadata_for_threads(course_key, threads, request.user, user_info) is_staff = cached_has_permission(request.user, 'openclose_thread', course.id) return utils.JsonResponse({ - 'discussion_data': [utils.safe_content(thread, course_id, is_staff) for thread in threads], + 'discussion_data': [utils.prepare_content(thread, course_key, is_staff) for thread in threads], 'user_info': user_info, 'annotated_content_info': annotated_content_info, 'page': query_params['page'], 'num_pages': query_params['num_pages'], - 'roles': utils.get_role_ids(course_id), + 'roles': utils.get_role_ids(course_key), 'course_settings': make_course_settings(course) }) @@ -164,19 +160,19 @@ def forum_form_discussion(request, course_id): """ Renders the main Discussion page, potentially filtered by a search query """ - course_id = SlashSeparatedCourseKey.from_deprecated_string(course_id) + course_key = SlashSeparatedCourseKey.from_deprecated_string(course_id) nr_transaction = newrelic.agent.current_transaction() - course = get_course_with_access(request.user, 'load_forum', course_id) + course = get_course_with_access(request.user, 'load_forum', course_key) course_settings = make_course_settings(course, include_category_map=True) user = cc.User.from_django_user(request.user) user_info = user.to_dict() try: - unsafethreads, query_params = get_threads(request, course_id) # This might process a search query + unsafethreads, query_params = get_threads(request, course_key) # This might process a search query is_staff = cached_has_permission(request.user, 'openclose_thread', course.id) - threads = [utils.safe_content(thread, course_id, is_staff) for thread in unsafethreads] + threads = [utils.prepare_content(thread, course_key, is_staff) for thread in unsafethreads] except cc.utils.CommentClientMaintenanceError: log.warning("Forum is in maintenance mode") return render_to_response('discussion/maintenance.html', {}) @@ -184,7 +180,7 @@ def forum_form_discussion(request, course_id): return HttpResponseBadRequest("Invalid group_id") with newrelic.agent.FunctionTrace(nr_transaction, "get_metadata_for_threads"): - annotated_content_info = utils.get_metadata_for_threads(course_id, threads, request.user, user_info) + annotated_content_info = utils.get_metadata_for_threads(course_key, threads, request.user, user_info) with newrelic.agent.FunctionTrace(nr_transaction, "add_courseware_context"): add_courseware_context(threads, course) @@ -199,7 +195,7 @@ def forum_form_discussion(request, course_id): }) else: with newrelic.agent.FunctionTrace(nr_transaction, "get_cohort_info"): - user_cohort_id = get_cohort_id(request.user, course_id) + user_cohort_id = get_cohort_id(request.user, course_key) context = { 'csrf': csrf(request)['csrf_token'], @@ -212,11 +208,11 @@ def forum_form_discussion(request, course_id): 'flag_moderator': cached_has_permission(request.user, 'openclose_thread', course.id) or has_access(request.user, 'staff', course), 'annotated_content_info': _attr_safe_json(annotated_content_info), 'course_id': course.id.to_deprecated_string(), - 'roles': _attr_safe_json(utils.get_role_ids(course_id)), - 'is_moderator': cached_has_permission(request.user, "see_all_cohorts", course_id), + 'roles': _attr_safe_json(utils.get_role_ids(course_key)), + 'is_moderator': cached_has_permission(request.user, "see_all_cohorts", course_key), 'cohorts': course_settings["cohorts"], # still needed to render _thread_list_template 'user_cohort': user_cohort_id, # read from container in NewPostView - 'is_course_cohorted': is_course_cohorted(course_id), # still needed to render _thread_list_template + 'is_course_cohorted': is_course_cohorted(course_key), # still needed to render _thread_list_template 'sort_preference': user.default_sort_key, 'category_map': course_settings["category_map"], 'course_settings': _attr_safe_json(course_settings) @@ -262,8 +258,7 @@ def single_thread(request, course_id, discussion_id, thread_id): if request.is_ajax(): with newrelic.agent.FunctionTrace(nr_transaction, "get_annotated_content_infos"): annotated_content_info = utils.get_annotated_content_infos(course_key, thread, request.user, user_info=user_info) - content = utils.safe_content(thread.to_dict(), course_key, is_staff) - add_thread_group_name(content, course_key) + content = utils.prepare_content(thread.to_dict(), course_key, is_staff) with newrelic.agent.FunctionTrace(nr_transaction, "add_courseware_context"): add_courseware_context([content], course) return utils.JsonResponse({ @@ -282,13 +277,11 @@ def single_thread(request, course_id, discussion_id, thread_id): add_courseware_context(threads, course) for thread in threads: - add_thread_group_name(thread, course_key) - #patch for backward compatibility with comments service if not "pinned" in thread: thread["pinned"] = False - threads = [utils.safe_content(thread, course_key, is_staff) for thread in threads] + threads = [utils.prepare_content(thread, course_key, is_staff) for thread in threads] with newrelic.agent.FunctionTrace(nr_transaction, "get_metadata_for_threads"): annotated_content_info = utils.get_metadata_for_threads(course_key, threads, request.user, user_info) @@ -323,13 +316,13 @@ def single_thread(request, course_id, discussion_id, thread_id): @require_GET @login_required def user_profile(request, course_id, user_id): - course_id = SlashSeparatedCourseKey.from_deprecated_string(course_id) + course_key = SlashSeparatedCourseKey.from_deprecated_string(course_id) nr_transaction = newrelic.agent.current_transaction() #TODO: Allow sorting? - course = get_course_with_access(request.user, 'load_forum', course_id) + course = get_course_with_access(request.user, 'load_forum', course_key) try: - profiled_user = cc.User(id=user_id, course_id=course_id) + profiled_user = cc.User(id=user_id, course_id=course_key) query_params = { 'page': request.GET.get('page', 1), @@ -337,7 +330,7 @@ def user_profile(request, course_id, user_id): } try: - group_id = get_group_id_for_comments_service(request, course_id) + group_id = get_group_id_for_comments_service(request, course_key) except ValueError: return HttpResponseBadRequest("Invalid group_id") if group_id is not None: @@ -349,12 +342,13 @@ def user_profile(request, course_id, user_id): user_info = cc.User.from_django_user(request.user).to_dict() with newrelic.agent.FunctionTrace(nr_transaction, "get_metadata_for_threads"): - annotated_content_info = utils.get_metadata_for_threads(course_id, threads, request.user, user_info) + annotated_content_info = utils.get_metadata_for_threads(course_key, threads, request.user, user_info) + is_staff = cached_has_permission(request.user, 'openclose_thread', course.id) + threads = [utils.prepare_content(thread, course_key, is_staff) for thread in threads] if request.is_ajax(): - is_staff = cached_has_permission(request.user, 'openclose_thread', course.id) return utils.JsonResponse({ - 'discussion_data': [utils.safe_content(thread, course_id, is_staff) for thread in threads], + 'discussion_data': threads, 'page': query_params['page'], 'num_pages': query_params['num_pages'], 'annotated_content_info': _attr_safe_json(annotated_content_info), @@ -370,7 +364,6 @@ def user_profile(request, course_id, user_id): 'annotated_content_info': _attr_safe_json(annotated_content_info), 'page': query_params['page'], 'num_pages': query_params['num_pages'], -# 'content': content, } return render_to_response('discussion/user_profile.html', context) @@ -380,12 +373,12 @@ def user_profile(request, course_id, user_id): @login_required def followed_threads(request, course_id, user_id): - course_id = SlashSeparatedCourseKey.from_deprecated_string(course_id) + course_key = SlashSeparatedCourseKey.from_deprecated_string(course_id) nr_transaction = newrelic.agent.current_transaction() - course = get_course_with_access(request.user, 'load_forum', course_id) + course = get_course_with_access(request.user, 'load_forum', course_key) try: - profiled_user = cc.User(id=user_id, course_id=course_id) + profiled_user = cc.User(id=user_id, course_id=course_key) default_query_params = { 'page': 1, @@ -412,7 +405,7 @@ def followed_threads(request, course_id, user_id): ) try: - group_id = get_group_id_for_comments_service(request, course_id) + group_id = get_group_id_for_comments_service(request, course_key) except ValueError: return HttpResponseBadRequest("Invalid group_id") if group_id is not None: @@ -424,17 +417,17 @@ def followed_threads(request, course_id, user_id): user_info = cc.User.from_django_user(request.user).to_dict() with newrelic.agent.FunctionTrace(nr_transaction, "get_metadata_for_threads"): - annotated_content_info = utils.get_metadata_for_threads(course_id, threads, request.user, user_info) + annotated_content_info = utils.get_metadata_for_threads(course_key, threads, request.user, user_info) if request.is_ajax(): is_staff = cached_has_permission(request.user, 'openclose_thread', course.id) return utils.JsonResponse({ 'annotated_content_info': annotated_content_info, - 'discussion_data': [utils.safe_content(thread, course_id, is_staff) for thread in threads], + 'discussion_data': [utils.prepare_content(thread, course_key, is_staff) for thread in threads], 'page': query_params['page'], 'num_pages': query_params['num_pages'], }) + #TODO remove non-AJAX support, it does not appear to be used and does not appear to work. else: - context = { 'course': course, 'user': request.user, diff --git a/lms/djangoapps/django_comment_client/tests/group_id.py b/lms/djangoapps/django_comment_client/tests/group_id.py index c60761074f..23f965c771 100644 --- a/lms/djangoapps/django_comment_client/tests/group_id.py +++ b/lms/djangoapps/django_comment_client/tests/group_id.py @@ -1,3 +1,8 @@ +import json +import re + +from course_groups.models import CourseUserGroup + class GroupIdAssertionMixin(object): def _data_or_params_cs_request(self, mock_request): """ @@ -17,6 +22,31 @@ class GroupIdAssertionMixin(object): self.assertTrue(mock_request.called) self.assertNotIn("group_id", self._data_or_params_cs_request(mock_request)) + def _assert_html_response_contains_group_info(self, response): + group_info = {"group_id": None, "group_name": None} + match = re.search(r'"group_id": ([\d]*)', response.content) + if match and match.group(1) != '': + group_info["group_id"] = int(match.group(1)) + match = re.search(r'"group_name": "([^&]*)"', response.content) + if match: + group_info["group_name"] = match.group(1) + self._assert_thread_contains_group_info(group_info) + + def _assert_json_response_contains_group_info(self, response, extract_thread=None): + """ + :param extract_thread: a function which accepts a dictionary (complete + json response payload) and returns another dictionary (first + occurrence of a thread model within that payload). if None is + passed, the identity function is assumed. + """ + payload = json.loads(response.content) + thread = extract_thread(payload) if extract_thread else payload + self._assert_thread_contains_group_info(thread) + + def _assert_thread_contains_group_info(self, thread): + self.assertEqual(thread['group_id'], self.student_cohort.id) + self.assertEqual(thread['group_name'], self.student_cohort.name) + class CohortedTopicGroupIdTestMixin(GroupIdAssertionMixin): """ diff --git a/lms/djangoapps/django_comment_client/utils.py b/lms/djangoapps/django_comment_client/utils.py index bff4c2de69..9894f17e90 100644 --- a/lms/djangoapps/django_comment_client/utils.py +++ b/lms/djangoapps/django_comment_client/utils.py @@ -366,7 +366,16 @@ def add_courseware_context(content_list, course): content.update({"courseware_url": url, "courseware_title": title}) -def safe_content(content, course_id, is_staff=False): +def prepare_content(content, course_key, is_staff=False): + """ + This function is used to pre-process thread and comment models in various + ways before adding them to the HTTP response. This includes fixing empty + attribute fields, enforcing author anonymity, and enriching metadata around + group ownership and response endorsement. + + @TODO: not all response pre-processing steps are currently integrated into + this function. + """ fields = [ 'id', 'title', 'body', 'course_id', 'anonymous', 'anonymous_to_peers', 'endorsed', 'parent_id', 'thread_id', 'votes', 'closed', 'created_at', @@ -407,22 +416,18 @@ def safe_content(content, course_id, is_staff=False): for child_content_key in ["children", "endorsed_responses", "non_endorsed_responses"]: if child_content_key in content: - safe_children = [ - safe_content(child, course_id, is_staff) for child in content[child_content_key] + children = [ + prepare_content(child, course_key, is_staff) for child in content[child_content_key] ] - content[child_content_key] = safe_children + content[child_content_key] = children + + # Augment the specified thread info to include the group name if a group id is present. + if content.get('group_id') is not None: + content['group_name'] = get_cohort_by_id(course_key, content.get('group_id')).name return content -def add_thread_group_name(thread_info, course_key): - """ - Augment the specified thread info to include the group name if a group id is present. - """ - if thread_info.get('group_id') is not None: - thread_info['group_name'] = get_cohort_by_id(course_key, thread_info.get('group_id')).name - - def get_group_id_for_comments_service(request, course_key, commentable_id=None): """ Given a user requesting content within a `commentable_id`, determine the