From fba6f2ebb27e05f9df4fbf51e97377cda6fa7f49 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 24 Jan 2014 13:14:08 -0500 Subject: [PATCH 1/2] 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) From 062025eeecc659985395211d0a9b6176e719a2b2 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 30 Jan 2014 10:20:13 -0500 Subject: [PATCH 2/2] Add pagination of repsonses to forum threads Users will first be shown the first 25 responses of a viewed thread and then have the option of loading 100 more at a time until all responses have been loaded. This mitigates the performance impact of very large threads by avoiding the need to render and transfer over the network all content of a large thread at once. It will also allow students with slower browsers to interact with large threads even if they cannot load all of the responses. --- .../view/discussion_thread_view_spec.coffee | 88 ++++++++++++++++++ .../views/discussion_thread_view.coffee | 92 +++++++++++++++++-- lms/static/sass/_discussion.scss | 34 ++++++- .../discussion/_underscore_templates.html | 6 +- 4 files changed, 205 insertions(+), 15 deletions(-) create mode 100644 common/static/coffee/spec/discussion/view/discussion_thread_view_spec.coffee diff --git a/common/static/coffee/spec/discussion/view/discussion_thread_view_spec.coffee b/common/static/coffee/spec/discussion/view/discussion_thread_view_spec.coffee new file mode 100644 index 0000000000..bd251de39f --- /dev/null +++ b/common/static/coffee/spec/discussion/view/discussion_thread_view_spec.coffee @@ -0,0 +1,88 @@ +describe "DiscussionThreadView", -> + beforeEach -> + setFixtures( + """ + +
+ """ + ) + + jasmine.Clock.useMock() + @threadData = { + id: "dummy" + } + @thread = new Thread(@threadData) + @view = new DiscussionThreadView({ model: @thread }) + @view.setElement($(".thread-fixture")) + spyOn($, "ajax") + # Avoid unnecessary boilerplate + spyOn(@view.showView, "render") + spyOn(@view, "makeWmdEditor") + spyOn(DiscussionThreadView.prototype, "renderResponse") + + describe "response count and pagination", -> + + setNextResponseContent = (content) -> + $.ajax.andCallFake( + (params) => + params.success({"content": content}) + {always: ->} + ) + + renderWithContent = (view, content) -> + setNextResponseContent(content) + view.render() + jasmine.Clock.tick(100) + + assertRenderedCorrectly = (view, countText, displayCountText, buttonText) -> + expect(view.$el.find(".response-count").text()).toEqual(countText) + if displayCountText + expect(view.$el.find(".response-display-count").text()).toEqual(displayCountText) + else + expect(view.$el.find(".response-display-count").length).toEqual(0) + if buttonText + expect(view.$el.find(".load-response-button").text()).toEqual(buttonText) + else + expect(view.$el.find(".load-response-button").length).toEqual(0) + + it "correctly render for a thread with no responses", -> + renderWithContent(@view, {resp_total: 0, children: []}) + assertRenderedCorrectly(@view, "0 responses", null, null) + + it "correctly render for a thread with one response", -> + renderWithContent(@view, {resp_total: 1, children: [{}]}) + assertRenderedCorrectly(@view, "1 response", "Showing all responses", null) + + it "correctly render for a thread with one additional page", -> + renderWithContent(@view, {resp_total: 2, children: [{}]}) + assertRenderedCorrectly(@view, "2 responses", "Showing first response", "Load all responses") + + it "correctly render for a thread with multiple additional pages", -> + renderWithContent(@view, {resp_total: 111, children: [{}, {}]}) + assertRenderedCorrectly(@view, "111 responses", "Showing first 2 responses", "Load next 100 responses") + + describe "on clicking the load more button", -> + beforeEach -> + renderWithContent(@view, {resp_total: 5, children: [{}]}) + assertRenderedCorrectly(@view, "5 responses", "Showing first response", "Load all responses") + + it "correctly re-render when all threads have loaded", -> + setNextResponseContent({resp_total: 5, children: [{}, {}, {}, {}]}) + @view.$el.find(".load-response-button").click() + assertRenderedCorrectly(@view, "5 responses", "Showing all responses", null) + + it "correctly re-render when one page remains", -> + setNextResponseContent({resp_total: 42, children: [{}, {}]}) + @view.$el.find(".load-response-button").click() + assertRenderedCorrectly(@view, "42 responses", "Showing first 3 responses", "Load all responses") + + it "correctly re-render when multiple pages remain", -> + setNextResponseContent({resp_total: 111, children: [{}, {}]}) + @view.$el.find(".load-response-button").click() + assertRenderedCorrectly(@view, "111 responses", "Showing first 3 responses", "Load next 100 responses") diff --git a/common/static/coffee/src/discussion/views/discussion_thread_view.coffee b/common/static/coffee/src/discussion/views/discussion_thread_view.coffee index e79dafacdd..95cda6b255 100644 --- a/common/static/coffee/src/discussion/views/discussion_thread_view.coffee +++ b/common/static/coffee/src/discussion/views/discussion_thread_view.coffee @@ -1,6 +1,9 @@ if Backbone? class @DiscussionThreadView extends DiscussionContentView + INITIAL_RESPONSE_PAGE_SIZE = 25 + SUBSEQUENT_RESPONSE_PAGE_SIZE = 100 + events: "click .discussion-submit-post": "submitComment" "click .add-response-btn": "scrollToAddResponse" @@ -11,6 +14,7 @@ if Backbone? initialize: -> super() @createShowView() + @responses = new Comments() renderTemplate: -> @template = _.template($("#thread-template").html()) @@ -18,7 +22,6 @@ if Backbone? render: -> @$el.html(@renderTemplate()) - @$el.find(".loading").hide() @delegateEvents() @renderShowView() @@ -27,26 +30,95 @@ if Backbone? @$("span.timeago").timeago() @makeWmdEditor "reply-body" @renderAddResponseButton() - @renderResponses() + @responses.on("add", @renderResponse) + # Without a delay, jQuery doesn't add the loading extension defined in + # utils.coffee before safeAjax is invoked, which results in an error + setTimeout( + => @loadResponses(INITIAL_RESPONSE_PAGE_SIZE, @$el.find(".responses"), true), + 100 + ) @ cleanup: -> if @responsesRequest? @responsesRequest.abort() - renderResponses: -> - setTimeout(=> - @$el.find(".loading").show() - , 200) + loadResponses: (responseLimit, elem, firstLoad) -> @responsesRequest = DiscussionUtil.safeAjax url: DiscussionUtil.urlFor('retrieve_single_thread', @model.get('commentable_id'), @model.id) + data: + resp_skip: @responses.size() + resp_limit: responseLimit if responseLimit + $elem: elem + $loading: elem + takeFocus: true + complete: => + @responseRequest = null success: (data, textStatus, xhr) => - @responsesRequest = null - @$el.find(".loading").remove() Content.loadContentInfos(data['annotated_content_info']) - comments = new Comments(data['content']['children']) - comments.each @renderResponse + @responses.add(data['content']['children']) + @renderResponseCountAndPagination(data['content']['resp_total']) @trigger "thread:responses:rendered" + error: => + if firstLoad + DiscussionUtil.discussionAlert( + gettext("Sorry"), + gettext("We had some trouble loading responses. Please reload the page.") + ) + else + DiscussionUtil.discussionAlert( + gettext("Sorry"), + gettext("We had some trouble loading more responses. Please try again.") + ) + + renderResponseCountAndPagination: (responseTotal) => + @$el.find(".response-count").html( + interpolate( + ngettext( + "%(numResponses)s response", + "%(numResponses)s responses", + responseTotal + ), + {numResponses: responseTotal}, + true + ) + ) + responsePagination = @$el.find(".response-pagination") + responsePagination.empty() + if responseTotal > 0 + responsesRemaining = responseTotal - @responses.size() + showingResponsesText = + if responsesRemaining == 0 + gettext("Showing all responses") + else + interpolate( + ngettext( + "Showing first response", + "Showing first %(numResponses)s responses", + @responses.size() + ), + {numResponses: @responses.size()}, + true + ) + responsePagination.append($("").addClass("response-display-count").html( + _.escape(showingResponsesText) + )) + if responsesRemaining > 0 + if responsesRemaining < SUBSEQUENT_RESPONSE_PAGE_SIZE + responseLimit = null + buttonText = gettext("Load all responses") + else + responseLimit = SUBSEQUENT_RESPONSE_PAGE_SIZE + buttonText = interpolate( + gettext("Load next %(numResponses)s responses"), + {numResponses: responseLimit}, + true + ) + loadMoreButton = $("