From 30f62203e2e1a8b5d3a1d2a4372f3d3d047978eb Mon Sep 17 00:00:00 2001 From: Brian Jacobel Date: Fri, 6 Jan 2017 13:34:04 -0500 Subject: [PATCH 1/2] Hide read state on profile pages. Add tests for read state generally --- .../views/discussion_thread_list_view.js | 13 +++- .../view/discussion_thread_list_view_spec.js | 69 ++++++++++++++++--- .../js/views/discussion_user_profile_view.js | 5 +- 3 files changed, 75 insertions(+), 12 deletions(-) diff --git a/common/static/common/js/discussion/views/discussion_thread_list_view.js b/common/static/common/js/discussion/views/discussion_thread_list_view.js index ef9cf793a2..e34025c5a3 100644 --- a/common/static/common/js/discussion/views/discussion_thread_list_view.js +++ b/common/static/common/js/discussion/views/discussion_thread_list_view.js @@ -93,6 +93,7 @@ this.courseSettings = options.courseSettings; this.hideRefineBar = options.hideRefineBar; this.supportsActiveThread = options.supportsActiveThread; + this.profilePage = options.profilePage || false; this.displayedCollection = new Discussion(this.collection.models, { pages: this.collection.pages }); @@ -335,7 +336,14 @@ DiscussionThreadListView.prototype.renderThread = function(thread) { var threadCommentCount = thread.get('comments_count'), threadUnreadCommentCount = thread.get('unread_comments_count'), - neverRead = !thread.get('read') && threadUnreadCommentCount === threadCommentCount, + // @TODO: On the profile page, thread read state for the viewing user is not accessible via the API. + // In this case, neverRead is set to false regardless of read state returned by the API. + // Fix this when the Discussions API can support this query. + neverRead = ( + !thread.get('read') && + threadUnreadCommentCount === threadCommentCount && + !this.profilePage + ), threadPreview = this.containsMarkup(thread.get('body')) ? '' : thread.get('body'), context = _.extend( { @@ -344,7 +352,8 @@ threadPreview: threadPreview, showThreadPreview: this.showThreadPreview }, - thread.toJSON() + thread.toJSON(), + this.profilePage ? {unread_comments_count: 0} : {} // See comment above about profile page ); return $(this.threadListItemTemplate(context).toString()); }; diff --git a/common/static/common/js/spec/discussion/view/discussion_thread_list_view_spec.js b/common/static/common/js/spec/discussion/view/discussion_thread_list_view_spec.js index 50f5048ab8..c2aa40328e 100644 --- a/common/static/common/js/spec/discussion/view/discussion_thread_list_view_spec.js +++ b/common/static/common/js/spec/discussion/view/discussion_thread_list_view_spec.js @@ -169,6 +169,7 @@ }); return this.view.render(); }); + setupAjax = function(callback) { return $.ajax.and.callFake(function(params) { if (callback) { @@ -185,19 +186,27 @@ }; }); }; + renderSingleThreadWithProps = function(props) { return makeView(new Discussion([new Thread(DiscussionViewSpecHelper.makeThreadWithProps(props))])).render(); }; - makeView = function(discussion) { - return new DiscussionThreadListView({ - el: $('#fixture-element'), - collection: discussion, - showThreadPreview: true, - courseSettings: new DiscussionCourseSettings({ - is_cohorted: true - }) - }); + + makeView = function(discussion, props) { + return new DiscussionThreadListView( + _.extend( + { + el: $('#fixture-element'), + collection: discussion, + showThreadPreview: true, + courseSettings: new DiscussionCourseSettings({ + is_cohorted: true + }) + }, + props + ) + ); }; + expectFilter = function(filterVal) { return $.ajax.and.callFake(function(params) { _.each(['unread', 'unanswered', 'flagged'], function(paramName) { @@ -681,5 +690,47 @@ expect(view.$el.find('.thread-preview-body').length).toEqual(0); }); }); + + describe('read/unread state', function() { + it('adds never-read class to unread threads', function() { + var unreads = this.threads.filter(function(thread) { + return !thread.read && thread.unread_comments_count === thread.comments_count; + }).length; + + this.view = makeView(new Discussion(this.threads)); + this.view.render(); + expect(this.view.$('.never-read').length).toEqual(unreads); + }); + + it('shows a "x new" message for threads that are read, but have unread comments', function() { + var unreadThread = this.threads.filter(function(thread) { + return thread.read && thread.unread_comments_count !== thread.comments_count; + })[0], + newCommentsOnUnreadThread = unreadThread.unread_comments_count; + + this.view = makeView(new Discussion(this.threads)); + this.view.render(); + expect( + this.view.$('.forum-nav-thread-unread-comments-count') + .first() + .text() + .trim() + ).toEqual(newCommentsOnUnreadThread + ' new'); + }); + + it('should display every thread as read if profilePage is passed to the constructor', function() { + // @TODO: This is temporary, see comment in DiscussionThreadListView.prototype.renderThread + this.view = makeView(new Discussion(this.threads), {profilePage: true}); + this.view.render(); + expect(this.view.$('.never-read').length).toEqual(0); + }); + + it('does not show the "x new" indicator for any thread if profilePage is passed', function() { + // @TODO: This is temporary, see comment in DiscussionThreadListView.prototype.renderThread + this.view = makeView(new Discussion(this.threads), {profilePage: true}); + this.view.render(); + expect(this.view.$('.forum-nav-thread-unread-comments-count').length).toEqual(0); + }); + }); }); }).call(this); diff --git a/lms/djangoapps/discussion/static/discussion/js/views/discussion_user_profile_view.js b/lms/djangoapps/discussion/static/discussion/js/views/discussion_user_profile_view.js index 7c3e199ecd..f9c7de0175 100644 --- a/lms/djangoapps/discussion/static/discussion/js/views/discussion_user_profile_view.js +++ b/lms/djangoapps/discussion/static/discussion/js/views/discussion_user_profile_view.js @@ -39,7 +39,10 @@ collection: this.discussion, el: this.$('.inline-threads'), courseSettings: this.courseSettings, - hideRefineBar: true // TODO: re-enable the search/filter bar when it works correctly + hideRefineBar: true, // TODO: re-enable the search/filter bar when it works correctly + // TODO: remove. Used temporarily to disable read state on profile page. See comment in + // discussion_thread_list_view.js / DiscussionThreadListView.prototype.renderThread + profilePage: true }).render(); this.discussionThreadListView.on('thread:selected', _.bind(this.navigateToThread, this)); From f58076e726429915731b72081a59bf415c7ac1a5 Mon Sep 17 00:00:00 2001 From: Brian Jacobel Date: Mon, 9 Jan 2017 10:25:38 -0500 Subject: [PATCH 2/2] Don't hijack existing context to do this --- .../views/discussion_thread_list_view.js | 17 +++++------------ .../view/discussion_thread_list_view_spec.js | 10 ++++------ .../discussion/thread-list-item.underscore | 4 ++-- .../js/views/discussion_user_profile_view.js | 6 +++--- 4 files changed, 14 insertions(+), 23 deletions(-) diff --git a/common/static/common/js/discussion/views/discussion_thread_list_view.js b/common/static/common/js/discussion/views/discussion_thread_list_view.js index e34025c5a3..ac151c9493 100644 --- a/common/static/common/js/discussion/views/discussion_thread_list_view.js +++ b/common/static/common/js/discussion/views/discussion_thread_list_view.js @@ -93,7 +93,7 @@ this.courseSettings = options.courseSettings; this.hideRefineBar = options.hideRefineBar; this.supportsActiveThread = options.supportsActiveThread; - this.profilePage = options.profilePage || false; + this.hideReadState = options.hideReadState || false; this.displayedCollection = new Discussion(this.collection.models, { pages: this.collection.pages }); @@ -336,24 +336,17 @@ DiscussionThreadListView.prototype.renderThread = function(thread) { var threadCommentCount = thread.get('comments_count'), threadUnreadCommentCount = thread.get('unread_comments_count'), - // @TODO: On the profile page, thread read state for the viewing user is not accessible via the API. - // In this case, neverRead is set to false regardless of read state returned by the API. - // Fix this when the Discussions API can support this query. - neverRead = ( - !thread.get('read') && - threadUnreadCommentCount === threadCommentCount && - !this.profilePage - ), + neverRead = !thread.get('read') && threadUnreadCommentCount === threadCommentCount, threadPreview = this.containsMarkup(thread.get('body')) ? '' : thread.get('body'), context = _.extend( { neverRead: neverRead, threadUrl: thread.urlFor('retrieve'), threadPreview: threadPreview, - showThreadPreview: this.showThreadPreview + showThreadPreview: this.showThreadPreview, + hideReadState: this.hideReadState }, - thread.toJSON(), - this.profilePage ? {unread_comments_count: 0} : {} // See comment above about profile page + thread.toJSON() ); return $(this.threadListItemTemplate(context).toString()); }; diff --git a/common/static/common/js/spec/discussion/view/discussion_thread_list_view_spec.js b/common/static/common/js/spec/discussion/view/discussion_thread_list_view_spec.js index c2aa40328e..713ffa801a 100644 --- a/common/static/common/js/spec/discussion/view/discussion_thread_list_view_spec.js +++ b/common/static/common/js/spec/discussion/view/discussion_thread_list_view_spec.js @@ -718,16 +718,14 @@ ).toEqual(newCommentsOnUnreadThread + ' new'); }); - it('should display every thread as read if profilePage is passed to the constructor', function() { - // @TODO: This is temporary, see comment in DiscussionThreadListView.prototype.renderThread - this.view = makeView(new Discussion(this.threads), {profilePage: true}); + it('should display every thread as read if hideReadState: true is passed to the constructor', function() { + this.view = makeView(new Discussion(this.threads), {hideReadState: true}); this.view.render(); expect(this.view.$('.never-read').length).toEqual(0); }); - it('does not show the "x new" indicator for any thread if profilePage is passed', function() { - // @TODO: This is temporary, see comment in DiscussionThreadListView.prototype.renderThread - this.view = makeView(new Discussion(this.threads), {profilePage: true}); + it('does not show the "x new" indicator for any thread if hideReadState: true is passed', function() { + this.view = makeView(new Discussion(this.threads), {hideReadState: true}); this.view.render(); expect(this.view.$('.forum-nav-thread-unread-comments-count').length).toEqual(0); }); diff --git a/common/static/common/templates/discussion/thread-list-item.underscore b/common/static/common/templates/discussion/thread-list-item.underscore index d57aec8630..6aa6e4070b 100644 --- a/common/static/common/templates/discussion/thread-list-item.underscore +++ b/common/static/common/templates/discussion/thread-list-item.underscore @@ -1,4 +1,4 @@ -
  • +
  • <% @@ -75,7 +75,7 @@ %> - <% if (!neverRead && unread_comments_count > 0) { %> + <% if (!hideReadState && !neverRead && unread_comments_count > 0) { %> <%- StringUtils.interpolate( diff --git a/lms/djangoapps/discussion/static/discussion/js/views/discussion_user_profile_view.js b/lms/djangoapps/discussion/static/discussion/js/views/discussion_user_profile_view.js index f9c7de0175..b8148b5f55 100644 --- a/lms/djangoapps/discussion/static/discussion/js/views/discussion_user_profile_view.js +++ b/lms/djangoapps/discussion/static/discussion/js/views/discussion_user_profile_view.js @@ -40,9 +40,9 @@ el: this.$('.inline-threads'), courseSettings: this.courseSettings, hideRefineBar: true, // TODO: re-enable the search/filter bar when it works correctly - // TODO: remove. Used temporarily to disable read state on profile page. See comment in - // discussion_thread_list_view.js / DiscussionThreadListView.prototype.renderThread - profilePage: true + // @TODO: On the profile page, thread read state for the viewing user is not accessible via API. + // Fix this when the Discussions API can support this query. Until then, hide read state. + hideReadState: true }).render(); this.discussionThreadListView.on('thread:selected', _.bind(this.navigateToThread, this));