From 910be64aa20d8d76bbf9b3289d11e47a00ea0cf3 Mon Sep 17 00:00:00 2001 From: Ibrahim Awwal Date: Fri, 14 Sep 2012 02:13:53 -0700 Subject: [PATCH 1/5] 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)); From ac9dd7bd1ef8ef136854fdd083b9270d7dc70758 Mon Sep 17 00:00:00 2001 From: Ibrahim Awwal Date: Fri, 14 Sep 2012 02:23:39 -0700 Subject: [PATCH 2/5] Fix hiding the more pages button. --- .../src/discussion/views/discussion_thread_list_view.coffee | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) 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 8a1a16beef..a38e74410b 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 @@ -18,6 +18,7 @@ if Backbone? @collection.on "reset", (discussion) => board = $(".current-board").html() @displayedCollection.current_page = discussion.current_page + @displayedCollection.pages = discussion.pages @displayedCollection.reset discussion.models # TODO: filter correctly # target = _.filter($("a.topic:contains('#{board}')"), (el) -> el.innerText == "General" || el.innerHTML == "General") @@ -121,9 +122,6 @@ if Backbone? @$(".more-pages").html('
    ') @$(".more-pages").addClass("loading") @collection.retrieveAnotherPage(@current_search, @discussionIds, @sortBy) - if not @collection.hasMorePages() - $(".more-pages").hide() - renderThread: (thread) => content = $(_.template($("#thread-list-item-template").html())(thread.toJSON())) From 2d58f8dea135ffba4145dac2b61b1fe6710d7daf Mon Sep 17 00:00:00 2001 From: Ibrahim Awwal Date: Fri, 14 Sep 2012 02:37:46 -0700 Subject: [PATCH 3/5] Fix load more pages button on single thread. --- lms/djangoapps/django_comment_client/forum/views.py | 3 ++- lms/templates/discussion/single_thread.html | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/lms/djangoapps/django_comment_client/forum/views.py b/lms/djangoapps/django_comment_client/forum/views.py index b8991a86d6..5eb9582f90 100644 --- a/lms/djangoapps/django_comment_client/forum/views.py +++ b/lms/djangoapps/django_comment_client/forum/views.py @@ -79,7 +79,7 @@ def get_threads(request, course_id, discussion_id=None, per_page=THREADS_PER_PAG query_params = merge_dict(default_query_params, 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 @@ -248,6 +248,7 @@ def single_thread(request, course_id, discussion_id, thread_id): 'threads': saxutils.escape(json.dumps(threads), escapedict), 'category_map': category_map, 'roles': saxutils.escape(json.dumps(utils.get_role_ids(course_id)), escapedict), + 'thread_pages': query_params['num_pages'], } return render_to_response('discussion/single_thread.html', context) diff --git a/lms/templates/discussion/single_thread.html b/lms/templates/discussion/single_thread.html index 0cc1f28b24..9ec05b1534 100644 --- a/lms/templates/discussion/single_thread.html +++ b/lms/templates/discussion/single_thread.html @@ -24,7 +24,7 @@ <%include file="_new_post.html" /> -
    +
    From 6057904d5182ad3c93d5c54fadd460738b02d4a1 Mon Sep 17 00:00:00 2001 From: Ibrahim Awwal Date: Fri, 14 Sep 2012 02:40:15 -0700 Subject: [PATCH 4/5] Fix sidebar height growing forever when loading more pages, and also jumping to top when loading more pages. --- .../src/discussion/views/discussion_thread_list_view.coffee | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) 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 a38e74410b..6f1ee52c08 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 @@ -112,13 +112,15 @@ if Backbone? @$(".post-list").html(rendered.html()) @renderMorePages() + @updateSidebar() @trigger "threads:rendered" renderMorePages: -> if @displayedCollection.hasMorePages() @$(".post-list").append("
  • Load more
    • ") - loadMorePages: -> + loadMorePages: (event) -> + event.preventDefault() @$(".more-pages").html('
    ') @$(".more-pages").addClass("loading") @collection.retrieveAnotherPage(@current_search, @discussionIds, @sortBy) From 154cb843dd2b101ed937f659b76ff271d5c9e8ea Mon Sep 17 00:00:00 2001 From: Ibrahim Awwal Date: Fri, 14 Sep 2012 02:48:49 -0700 Subject: [PATCH 5/5] Bump up threads per page again, because I don't think showing 5 threads at a time is particularly useful. --- lms/djangoapps/django_comment_client/forum/views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lms/djangoapps/django_comment_client/forum/views.py b/lms/djangoapps/django_comment_client/forum/views.py index 5eb9582f90..4708037fe7 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 = 5 +THREADS_PER_PAGE = 20 INLINE_THREADS_PER_PAGE = 5 PAGES_NEARBY_DELTA = 2 escapedict = {'"': '"'}