Merge pull request #2377 from edx/gprice/forum-thread-pagination

Add pagination of responses to forum threads
2014-02-03 08:55:20 -08:00
parent 830ad942ff 062025eeec
commit 4e1c595472
8 changed files with 349 additions and 41 deletions
--- a/common/static/coffee/spec/discussion/view/discussion_thread_view_spec.coffee
+++ b/common/static/coffee/spec/discussion/view/discussion_thread_view_spec.coffee
@@ -0,0 +1,88 @@
+describe "DiscussionThreadView", ->
+    beforeEach ->
+        setFixtures(
+            """
+            <script type="text/template" id="thread-template">
+                <article class="discussion-article">
+                    <div class="response-count"/> 
+                    <ol class="responses"/>
+                    <div class="response-pagination"/>
+                </article>
+            </script>
+            <div class="thread-fixture"/>
+            """
+        )
+
+        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")
--- 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($("<span>").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 = $("<button>").addClass("load-response-button").html(
+            _.escape(buttonText)
+          )
+          loadMoreButton.click((event) => @loadResponses(responseLimit, loadMoreButton))
+          responsePagination.append(loadMoreButton)

    renderResponse: (response) =>
        response.set('thread', @model)
--- 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):
--- 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)
--- a/lms/djangoapps/django_comment_client/utils.py
+++ b/lms/djangoapps/django_comment_client/utils.py
@@ -361,7 +361,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',

    ]

--- 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)
--- a/lms/static/sass/_discussion.scss
+++ b/lms/static/sass/_discussion.scss
@@ -1406,7 +1406,8 @@ body.discussion {
  }

  .discussion-post {
-    padding: $baseline*2 $baseline*2 $baseline/2 $baseline*2;
+    padding: $baseline*2 $baseline*2 $baseline $baseline*2;
+    box-shadow: 0 1px 3px $shadow;

    > header .vote-btn {
      position: relative;
@@ -1813,7 +1814,7 @@ body.discussion {


  .discussion-reply-new {
-    padding: 0px 30px $baseline;
+    padding: 0.5*$baseline 30px $baseline;
    @include clearfix;
    @include transition(opacity .2s linear 0s);

@@ -2572,3 +2573,32 @@ display:none;
  color: #333;
  font-style: italic;
 }
+
+.response-count {
+  margin-top: $baseline;
+  padding: 0px 3*$baseline;
+}
+
+.response-pagination {
+  padding: 0px 1.5*$baseline;
+
+  .response-display-count {
+    display: block;
+    padding: 0.5*$baseline 1.5*$baseline;
+  }
+
+  .load-response-button {
+    display: block;
+    @include white-button;
+    font: normal 1em/1.6em $sans-serif;
+    position: relative;
+    padding: 0px 1.5*$baseline;
+    margin: $baseline/2 0px;
+    border: 1px solid #b2b2b2;
+    box-shadow: 0 1px 3px rgba(0, 0, 0, .15);
+    font-size: 13px;
+    text-align: left;
+    @include animation(fadeIn .3s);
+    width: 100%;
+  }
+}
--- a/lms/templates/discussion/_underscore_templates.html
+++ b/lms/templates/discussion/_underscore_templates.html
@@ -5,15 +5,15 @@
 <script type="text/template" id="thread-template">
    <article class="discussion-article" data-id="${'<%- id %>'}">
        <div class="thread-content-wrapper"></div>
+        <div class="response-count"/>
        <div class="add-response">
            <button class="button add-response-btn">
                <i class="icon icon-reply"></i>
                <span class="add-response-btn-text">${_('Add A Response')}</span>
            </button>
        </div>
-        <ol class="responses">
-            <li class="loading"><div class="loading-animation"><span class="sr">${_('Loading content')}</span></div></li>
-        </ol>
+        <ol class="responses"/>
+        <div class="response-pagination"/>
        <div class="post-status-closed bottom-post-status" style="display: none">
          ${_("This thread is closed.")}
        </div>