From fba6f2ebb27e05f9df4fbf51e97377cda6fa7f49 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 24 Jan 2014 13:14:08 -0500 Subject: [PATCH] Allow LMS to do forum thread response pagination The relevant parameters and data are merely passed through between the front end and comments service. --- .../django_comment_client/forum/tests.py | 153 +++++++++++++++--- .../django_comment_client/forum/views.py | 10 +- lms/djangoapps/django_comment_client/utils.py | 2 +- lms/lib/comment_client/thread.py | 5 +- 4 files changed, 144 insertions(+), 26 deletions(-) diff --git a/lms/djangoapps/django_comment_client/forum/tests.py b/lms/djangoapps/django_comment_client/forum/tests.py index fa3d9c96b3..4425c3e23b 100644 --- a/lms/djangoapps/django_comment_client/forum/tests.py +++ b/lms/djangoapps/django_comment_client/forum/tests.py @@ -11,7 +11,7 @@ from django_comment_client.forum import views from courseware.tests.modulestore_config import TEST_DATA_MIXED_MODULESTORE from nose.tools import assert_true # pylint: disable=E0611 -from mock import patch, Mock +from mock import patch, Mock, ANY import logging @@ -85,6 +85,26 @@ class ViewsExceptionTestCase(UrlResetMixin, ModuleStoreTestCase): self.assertEqual(self.response.status_code, 404) +def make_mock_thread_data(text, thread_id, include_children): + thread_data = { + "id": thread_id, + "type": "thread", + "title": text, + "body": text, + "commentable_id": "dummy_commentable_id", + "resp_total": 42, + "resp_skip": 25, + "resp_limit": 5, + } + if include_children: + thread_data["children"] = [{ + "id": "dummy_comment_id", + "type": "comment", + "body": text, + }] + return thread_data + + def make_mock_request_impl(text, thread_id=None): def mock_request_impl(*args, **kwargs): url = args[1] @@ -92,30 +112,13 @@ def make_mock_request_impl(text, thread_id=None): return Mock( status_code=200, text=json.dumps({ - "collection": [{ - "id": "dummy_thread_id", - "type": "thread", - "commentable_id": "dummy_commentable_id", - "title": text, - "body": text, - }] + "collection": [make_mock_thread_data(text, "dummy_thread_id", False)] }) ) elif thread_id and url.endswith(thread_id): return Mock( status_code=200, - text=json.dumps({ - "id": thread_id, - "type": "thread", - "title": text, - "body": text, - "commentable_id": "dummy_commentable_id", - "children": [{ - "id": "dummy_comment_id", - "type": "comment", - "body": text, - }], - }) + text=json.dumps(make_mock_thread_data(text, thread_id, True)) ) else: # user query return Mock( @@ -129,6 +132,116 @@ def make_mock_request_impl(text, thread_id=None): return mock_request_impl +class StringEndsWithMatcher(object): + def __init__(self, suffix): + self.suffix = suffix + + def __eq__(self, other): + return other.endswith(self.suffix) + + +class PartialDictMatcher(object): + def __init__(self, expected_values): + self.expected_values = expected_values + + def __eq__(self, other): + return all([ + key in other and other[key] == value + for key, value in self.expected_values.iteritems() + ]) + + +@override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE) +@patch('requests.request') +class SingleThreadTestCase(ModuleStoreTestCase): + def setUp(self): + self.course = CourseFactory.create() + self.student = UserFactory.create() + CourseEnrollmentFactory.create(user=self.student, course_id=self.course.id) + + def test_ajax(self, mock_request): + text = "dummy content" + thread_id = "test_thread_id" + mock_request.side_effect = make_mock_request_impl(text, thread_id) + + request = RequestFactory().get( + "dummy_url", + HTTP_X_REQUESTED_WITH="XMLHttpRequest" + ) + request.user = self.student + response = views.single_thread( + request, + self.course.id, + "dummy_discussion_id", + "test_thread_id" + ) + + self.assertEquals(response.status_code, 200) + response_data = json.loads(response.content) + self.assertEquals( + response_data["content"], + make_mock_thread_data(text, thread_id, True) + ) + mock_request.assert_called_with( + "get", + StringEndsWithMatcher(thread_id), # url + data=None, + params=PartialDictMatcher({"mark_as_read": True, "user_id": 1, "recursive": True}), + headers=ANY, + timeout=ANY + ) + + def test_skip_limit(self, mock_request): + text = "dummy content" + thread_id = "test_thread_id" + response_skip = "45" + response_limit = "15" + mock_request.side_effect = make_mock_request_impl(text, thread_id) + + request = RequestFactory().get( + "dummy_url", + {"resp_skip": response_skip, "resp_limit": response_limit}, + HTTP_X_REQUESTED_WITH="XMLHttpRequest" + ) + request.user = self.student + response = views.single_thread( + request, + self.course.id, + "dummy_discussion_id", + "test_thread_id" + ) + self.assertEquals(response.status_code, 200) + response_data = json.loads(response.content) + self.assertEquals( + response_data["content"], + make_mock_thread_data(text, thread_id, True) + ) + mock_request.assert_called_with( + "get", + StringEndsWithMatcher(thread_id), # url + data=None, + params=PartialDictMatcher({ + "mark_as_read": True, + "user_id": 1, + "recursive": True, + "resp_skip": response_skip, + "resp_limit": response_limit, + }), + headers=ANY, + timeout=ANY + ) + + def test_post(self, mock_request): + request = RequestFactory().post("dummy_url") + response = views.single_thread( + request, + self.course.id, + "dummy_discussion_id", + "dummy_thread_id" + ) + self.assertEquals(response.status_code, 405) + + @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE) class InlineDiscussionUnicodeTestCase(ModuleStoreTestCase, UnicodeTestMixin): def setUp(self): diff --git a/lms/djangoapps/django_comment_client/forum/views.py b/lms/djangoapps/django_comment_client/forum/views.py index d6a84566e2..c328b0e141 100644 --- a/lms/djangoapps/django_comment_client/forum/views.py +++ b/lms/djangoapps/django_comment_client/forum/views.py @@ -6,6 +6,7 @@ from django.contrib.auth.decorators import login_required from django.http import Http404 from django.core.context_processors import csrf from django.contrib.auth.models import User +from django.views.decorators.http import require_GET import newrelic.agent from edxmako.shortcuts import render_to_response @@ -229,6 +230,7 @@ def forum_form_discussion(request, course_id): return render_to_response('discussion/index.html', context) +@require_GET @login_required def single_thread(request, course_id, discussion_id, thread_id): nr_transaction = newrelic.agent.current_transaction() @@ -237,12 +239,16 @@ def single_thread(request, course_id, discussion_id, thread_id): cc_user = cc.User.from_django_user(request.user) user_info = cc_user.to_dict() - thread = cc.Thread.find(thread_id).retrieve(recursive=True, user_id=request.user.id) + thread = cc.Thread.find(thread_id).retrieve( + recursive=True, + user_id=request.user.id, + response_skip=request.GET.get("resp_skip"), + response_limit=request.GET.get("resp_limit") + ) if request.is_ajax(): with newrelic.agent.FunctionTrace(nr_transaction, "get_annotated_content_infos"): annotated_content_info = utils.get_annotated_content_infos(course_id, thread, request.user, user_info=user_info) - context = {'thread': thread.to_dict(), 'course_id': course_id} content = utils.safe_content(thread.to_dict()) with newrelic.agent.FunctionTrace(nr_transaction, "add_courseware_context"): add_courseware_context([content], course) diff --git a/lms/djangoapps/django_comment_client/utils.py b/lms/djangoapps/django_comment_client/utils.py index 4aefbc67af..8446a4ae32 100644 --- a/lms/djangoapps/django_comment_client/utils.py +++ b/lms/djangoapps/django_comment_client/utils.py @@ -362,7 +362,7 @@ def safe_content(content): 'at_position_list', 'children', 'highlighted_title', 'highlighted_body', 'courseware_title', 'courseware_url', 'unread_comments_count', 'read', 'group_id', 'group_name', 'group_string', 'pinned', 'abuse_flaggers', - 'stats' + 'stats', 'resp_skip', 'resp_limit', 'resp_total', ] diff --git a/lms/lib/comment_client/thread.py b/lms/lib/comment_client/thread.py index c44ae9b5dd..768cc3aa1b 100644 --- a/lms/lib/comment_client/thread.py +++ b/lms/lib/comment_client/thread.py @@ -74,10 +74,9 @@ class Thread(models.Model): 'recursive': kwargs.get('recursive'), 'user_id': kwargs.get('user_id'), 'mark_as_read': kwargs.get('mark_as_read', True), + 'resp_skip': kwargs.get('response_skip'), + 'resp_limit': kwargs.get('response_limit'), } - - # user_id may be none, in which case it shouldn't be part of the - # request. request_params = strip_none(request_params) response = perform_request('get', url, request_params)