From 910be64aa20d8d76bbf9b3289d11e47a00ea0cf3 Mon Sep 17 00:00:00 2001 From: Ibrahim Awwal Date: Fri, 14 Sep 2012 02:13:53 -0700 Subject: [PATCH] Make pagination respect any filters or searches or sort keys. Searches and filters will query the comment service to get their results rather than filtering threads that have already been loaded. --- .../django_comment_client/forum/views.py | 8 +++-- .../django_comment_client/permissions.py | 8 ++--- lms/lib/comment_client/thread.py | 6 ++-- .../coffee/src/discussion/discussion.coffee | 11 +++++-- .../views/discussion_thread_list_view.coffee | 30 +++++++++++++++---- lms/static/js/jquery.tagsinput.js | 2 +- 6 files changed, 45 insertions(+), 20 deletions(-) diff --git a/lms/djangoapps/django_comment_client/forum/views.py b/lms/djangoapps/django_comment_client/forum/views.py index e11de15844..b8991a86d6 100644 --- a/lms/djangoapps/django_comment_client/forum/views.py +++ b/lms/djangoapps/django_comment_client/forum/views.py @@ -22,7 +22,7 @@ import django_comment_client.utils as utils import comment_client as cc import xml.sax.saxutils as saxutils -THREADS_PER_PAGE = 2 +THREADS_PER_PAGE = 5 INLINE_THREADS_PER_PAGE = 5 PAGES_NEARBY_DELTA = 2 escapedict = {'"': '"'} @@ -78,8 +78,8 @@ def get_threads(request, course_id, discussion_id=None, per_page=THREADS_PER_PAG user.save() query_params = merge_dict(default_query_params, - strip_none(extract(request.GET, ['page', 'sort_key', 'sort_order', 'text', 'tags']))) - + strip_none(extract(request.GET, ['page', 'sort_key', 'sort_order', 'text', 'tags', 'commentable_ids']))) + threads, page, num_pages = cc.Thread.search(query_params) query_params['page'] = page @@ -143,6 +143,8 @@ def forum_form_discussion(request, course_id): return utils.JsonResponse({ 'discussion_data': threads, # TODO: Standardize on 'discussion_data' vs 'threads' 'annotated_content_info': annotated_content_info, + 'num_pages': query_params['num_pages'], + 'page': query_params['page'], }) else: #recent_active_threads = cc.search_recent_active_threads( diff --git a/lms/djangoapps/django_comment_client/permissions.py b/lms/djangoapps/django_comment_client/permissions.py index 352d4cc187..d1275a3bcc 100644 --- a/lms/djangoapps/django_comment_client/permissions.py +++ b/lms/djangoapps/django_comment_client/permissions.py @@ -11,7 +11,7 @@ cache = cache.get_cache('default') def cached_has_permission(user, permission, course_id=None): """ Call has_permission if it's not cached. A change in a user's role or - a role's permissions will only become effective after CACHE_LIFESPAN seconds. + a role's permissions will only become effective after CACHE_LIFESPAN seconds. """ CACHE_LIFESPAN = 60 key = "permission_%d_%s_%s" % (user.id, str(course_id), permission) @@ -54,8 +54,8 @@ def check_conditions_permissions(user, permissions, course_id, **kwargs): """ Accepts a list of permissions and proceed if any of the permission is valid. Note that ["can_view", "can_edit"] will proceed if the user has either - "can_view" or "can_edit" permission. To use AND operator in between, wrap them in - a list. + "can_view" or "can_edit" permission. To use AND operator in between, wrap them in + a list. """ def test(user, per, operator="or"): @@ -87,7 +87,7 @@ VIEW_PERMISSIONS = { 'vote_for_thread' : [['vote', 'is_open']], 'undo_vote_for_thread': [['unvote', 'is_open']], 'follow_thread' : ['follow_thread'], - 'follow_commentable': ['follow_commentable'], + 'follow_commentable': ['follow_commentable'], 'follow_user' : ['follow_user'], 'unfollow_thread' : ['unfollow_thread'], 'unfollow_commentable': ['unfollow_commentable'], diff --git a/lms/lib/comment_client/thread.py b/lms/lib/comment_client/thread.py index e4ada77499..98ce4c3640 100644 --- a/lms/lib/comment_client/thread.py +++ b/lms/lib/comment_client/thread.py @@ -16,7 +16,7 @@ class Thread(models.Model): ] updatable_fields = [ - 'title', 'body', 'anonymous', 'course_id', + 'title', 'body', 'anonymous', 'course_id', 'closed', 'tags', 'user_id', 'commentable_id', ] @@ -33,7 +33,7 @@ class Thread(models.Model): 'course_id': query_params['course_id'], 'recursive': False} params = merge_dict(default_params, strip_blank(strip_none(query_params))) - if query_params.get('text') or query_params.get('tags'): + if query_params.get('text') or query_params.get('tags') or query_params.get('commentable_ids'): url = cls.url(action='search') else: url = cls.url(action='get_all', params=extract(params, 'commentable_id')) @@ -41,7 +41,7 @@ class Thread(models.Model): del params['commentable_id'] response = perform_request('get', url, params, *args, **kwargs) return response.get('collection', []), response.get('page', 1), response.get('num_pages', 1) - + @classmethod def url_for_threads(cls, params={}): if params.get('commentable_id'): diff --git a/lms/static/coffee/src/discussion/discussion.coffee b/lms/static/coffee/src/discussion/discussion.coffee index 91083c94e3..698fa1b039 100644 --- a/lms/static/coffee/src/discussion/discussion.coffee +++ b/lms/static/coffee/src/discussion/discussion.coffee @@ -18,18 +18,23 @@ if Backbone? @current_page < @pages addThread: (thread, options) -> + # TODO: Check for existing thread with same ID? options ||= {} model = new Thread thread @add model model - retrieveAnotherPage: (search_text="", commentable_id="", sort_key="")-> - # TODO: Obey dropdown filter (commentable_id) + retrieveAnotherPage: (search_text="", commentable_ids="", sort_key="")-> + # TODO: I really feel that this belongs in DiscussionThreadListView @current_page += 1 url = DiscussionUtil.urlFor 'threads' - data = { page: @current_page, text: search_text } + data = { page: @current_page } + if search_text + data['text'] = search_text if sort_key data['sort_key'] = sort_key + if commentable_ids + data['commentable_ids'] = commentable_ids DiscussionUtil.safeAjax $elem: @$el url: url diff --git a/lms/static/coffee/src/discussion/views/discussion_thread_list_view.coffee b/lms/static/coffee/src/discussion/views/discussion_thread_list_view.coffee index 90bd7c4978..8a1a16beef 100644 --- a/lms/static/coffee/src/discussion/views/discussion_thread_list_view.coffee +++ b/lms/static/coffee/src/discussion/views/discussion_thread_list_view.coffee @@ -14,6 +14,7 @@ if Backbone? @displayedCollection = new Discussion(@collection.models, pages: @collection.pages) @collection.on "change", @reloadDisplayedCollection @sortBy = "date" + @discussionIds="" @collection.on "reset", (discussion) => board = $(".current-board").html() @displayedCollection.current_page = discussion.current_page @@ -117,10 +118,12 @@ if Backbone? @$(".post-list").append("
  • Load more
    • ") loadMorePages: -> - # TODO: Obey dropdown filter @$(".more-pages").html('
    ') @$(".more-pages").addClass("loading") - @collection.retrieveAnotherPage(@current_search, "", @sortBy) + @collection.retrieveAnotherPage(@current_search, @discussionIds, @sortBy) + if not @collection.hasMorePages() + $(".more-pages").hide() + renderThread: (thread) => content = $(_.template($("#thread-list-item-template").html())(thread.toJSON())) @@ -232,14 +235,27 @@ if Backbone? @setTopic(event) @clearSearch @filterTopic, event else - @setTopic(event) + @setTopic(event) # just sets the title for the dropdown item = $(event.target).closest('li') if item.find("span.board-name").data("discussion_id") == "#all" item = item.parent() + @discussionIds = "" discussionIds = _.map item.find(".board-name[data-discussion_id]"), (board) -> $(board).data("discussion_id").id - filtered = @collection.filter (thread) => - _.include(discussionIds, thread.get('commentable_id')) - @displayedCollection.reset filtered + @retrieveDiscussions(discussionIds) + + retrieveDiscussions: (discussion_ids) -> + @discussionIds = discussion_ids.join(',') + url = DiscussionUtil.urlFor("search") + DiscussionUtil.safeAjax + data: { 'commentable_ids': @discussionIds } + url: url + type: "GET" + success: (response, textStatus) => + @collection.current_page = response.page + @collection.pages = response.num_pages + @collection.reset(response.discussion_data) + Content.loadContentInfos(response.content_info) + @displayedCollection.reset(@collection.models) sortThreads: (event) -> @$(".sort-bar a").removeClass("active") @@ -283,6 +299,8 @@ if Backbone? # TODO: Augment existing collection? @collection.reset(response.discussion_data) Content.loadContentInfos(response.content_info) + @collection.current_page = response.page + @collection.pages = response.num_pages # TODO: Perhaps reload user info so that votes can be updated. # In the future we might not load all of a user's votes at once # so this would probably be necessary anyway diff --git a/lms/static/js/jquery.tagsinput.js b/lms/static/js/jquery.tagsinput.js index 563567ac46..917eba817c 100644 --- a/lms/static/js/jquery.tagsinput.js +++ b/lms/static/js/jquery.tagsinput.js @@ -105,7 +105,7 @@ $('').text(value).append('  '), $('', { href : '#', - title : 'Removing tag', + title : 'Remove tag', text : 'x' }).click(function () { return $('#' + id).removeTag(escape(value));