From 690dd6508198852230645531e052f8f73685fffc Mon Sep 17 00:00:00 2001 From: Jon Willesen Date: Mon, 13 May 2013 16:10:11 -0600 Subject: [PATCH] add "mark all as read" command to discussion topic gear menu fixes CNVS-5714 test plan: - create a discussion topic as one user. - as another user, make a bunch of replies (more than a pages worth). - as the first user, refresh the topic and see a bunch of unread replies (some will get auto marked). - manually mark some of the read posts as unread. - choose the new "mark all as read option" from the gear menu. - blue ball should turn gray on all posts. - go to discussion index, unread counter on the topic should show no unread posts - go back to discussion, unread counter should show no unread posts. Change-Id: I5ffe8717731943349996e6f098d491aca5bc43a1 Reviewed-on: https://gerrit.instructure.com/20617 Tested-by: Jenkins Reviewed-by: Mark Ericksen QA-Review: Cam Theriault Product-Review: Jon Willesen --- app/coffeescripts/bundles/discussion.coffee | 15 +++ .../collections/EntryCollection.coffee | 4 + app/coffeescripts/models/Topic.coffee | 16 ++- .../DiscussionFilterResultsView.coffee | 5 + .../DiscussionToolbarView.coffee | 10 ++ .../views/DiscussionTopic/EntryView.coffee | 4 + .../DiscussionTopic/FilterEntryView.coffee | 14 +-- .../discussion_topics_controller.rb | 2 + app/views/discussion_topics/show.html.erb | 106 +++++++++--------- spec/selenium/discussions_spec.rb | 45 ++++++-- 10 files changed, 153 insertions(+), 68 deletions(-) diff --git a/app/coffeescripts/bundles/discussion.coffee b/app/coffeescripts/bundles/discussion.coffee index 9cbb9849e01..36a7496c1cc 100644 --- a/app/coffeescripts/bundles/discussion.coffee +++ b/app/coffeescripts/bundles/discussion.coffee @@ -74,6 +74,13 @@ require [ e = data.flattened[id] e.read_state = read_state if e + ## + # propagate mark all read/unread changes to all views + setAllReadStateAllViews = (newReadState) -> + entries.setAllReadState(newReadState) + EntryView.setAllReadState(newReadState) + filterView.setAllReadState(newReadState) + entriesView.on 'scrollAwayFromEntry', -> # prevent scroll to top for non-pushstate browsers when hash changes top = $container.scrollTop() @@ -112,6 +119,14 @@ require [ EntryView.collapseRootEntries() scrollToTop() + toolbarView.on 'markAllAsRead', -> + data.markAllAsRead() + setAllReadStateAllViews('read') + + toolbarView.on 'markAllAsUnread', -> + data.markAllAsUnread() + setAllReadStateAllViews('unread') + filterView.on 'render', -> scrollToTop() diff --git a/app/coffeescripts/collections/EntryCollection.coffee b/app/coffeescripts/collections/EntryCollection.coffee index 7ff361747e1..bf1ad7c57c6 100644 --- a/app/coffeescripts/collections/EntryCollection.coffee +++ b/app/coffeescripts/collections/EntryCollection.coffee @@ -33,6 +33,10 @@ define [ page.fullCollection = this page + setAllReadState: (newReadState) -> + @each (entry) -> + entry.set 'read_state', newReadState + ## # This could have been two or three well-named methods, but it doesn't make # a whole lot of sense to walk the tree over and over to get each piece of diff --git a/app/coffeescripts/models/Topic.coffee b/app/coffeescripts/models/Topic.coffee index 61e40e17390..befca9a2a4f 100644 --- a/app/coffeescripts/models/Topic.coffee +++ b/app/coffeescripts/models/Topic.coffee @@ -4,12 +4,14 @@ define [ 'i18n!discussions' + 'jquery' 'underscore' 'Backbone' 'compiled/util/BackoffPoller' 'compiled/arr/walk' 'compiled/arr/erase' -], (I18n, {each}, Backbone, BackoffPoller, walk, erase) -> + 'jquery.ajaxJSON' +], (I18n, $, {each}, Backbone, BackoffPoller, walk, erase) -> UNKNOWN_AUTHOR = avatar_image_url: null @@ -45,6 +47,18 @@ define [ backoffFactor: 1.6 loader.start() + markAllAsRead: -> + $.ajaxJSON ENV.DISCUSSION.MARK_ALL_READ_URL, 'PUT', forced_read_state: false + @setAllReadState('read') + + markAllAsUnread: -> + $.ajaxJSON ENV.DISCUSSION.MARK_ALL_UNREAD_URL, 'DELETE', forced_read_state: false + @setAllReadState('unread') + + setAllReadState: (newReadState) -> + each @flattened, (entry) -> + entry.read_state = newReadState + parse: (data, status, xhr) -> @data = data # build up entries in @data.entries, mainly because we don't want deleted diff --git a/app/coffeescripts/views/DiscussionTopic/DiscussionFilterResultsView.coffee b/app/coffeescripts/views/DiscussionTopic/DiscussionFilterResultsView.coffee index c9cc992088f..27417ac43d0 100644 --- a/app/coffeescripts/views/DiscussionTopic/DiscussionFilterResultsView.coffee +++ b/app/coffeescripts/views/DiscussionTopic/DiscussionFilterResultsView.coffee @@ -22,6 +22,11 @@ define [ attach: -> @model.on 'change', @renderOrTeardownResults + setAllReadState: (newReadState) -> + if @collection? + @collection.fullCollection.each (entry) -> + entry.set 'read_state', newReadState + resetCollection: (models) => collection = new EntryCollection models, perPage: 10 @collection = collection.getPageAsCollection 0 diff --git a/app/coffeescripts/views/DiscussionTopic/DiscussionToolbarView.coffee b/app/coffeescripts/views/DiscussionTopic/DiscussionToolbarView.coffee index 5e21f48993d..c63c3aaabad 100644 --- a/app/coffeescripts/views/DiscussionTopic/DiscussionToolbarView.coffee +++ b/app/coffeescripts/views/DiscussionTopic/DiscussionToolbarView.coffee @@ -19,6 +19,8 @@ define [ 'change #onlyUnread': 'toggleUnread' 'click #collapseAll': 'collapseAll' 'click #expandAll': 'expandAll' + 'click .mark_all_as_read': 'markAllAsRead' + 'click .mark_all_as_unread': 'markAllAsUnread' initialize: -> @model.on 'change', @clearInputs @@ -59,6 +61,14 @@ define [ @model.set 'collapsed', false @trigger 'expandAll' + markAllAsRead: (event) -> + event.preventDefault() + @trigger 'markAllAsRead' + + markAllAsUnread: (event) -> + event.preventDefault() + @trigger 'markAllAsUnread' + maybeDisableFields: -> @$disableWhileFiltering.attr 'disabled', @model.hasFilter() diff --git a/app/coffeescripts/views/DiscussionTopic/EntryView.coffee b/app/coffeescripts/views/DiscussionTopic/EntryView.coffee index f6487494261..548228a405b 100644 --- a/app/coffeescripts/views/DiscussionTopic/EntryView.coffee +++ b/app/coffeescripts/views/DiscussionTopic/EntryView.coffee @@ -30,6 +30,10 @@ define [ _.each @instances, (view) -> view.expand() unless view.model.get 'parent' + @setAllReadState = (newReadState) -> + _.each @instances, (view) -> + view.model.set 'read_state', newReadState + els: '.discussion_entry:first': '$entryContent' '.replies:first': '$replies' diff --git a/app/coffeescripts/views/DiscussionTopic/FilterEntryView.coffee b/app/coffeescripts/views/DiscussionTopic/FilterEntryView.coffee index b2d9ca8fcf1..e30a46c1488 100644 --- a/app/coffeescripts/views/DiscussionTopic/FilterEntryView.coffee +++ b/app/coffeescripts/views/DiscussionTopic/FilterEntryView.coffee @@ -23,7 +23,7 @@ define [ initialize: -> super - @model.on 'change:read_state', @toggleReadState + @model.on 'change:read_state', @updateReadState toJSON: -> @model.attributes @@ -33,7 +33,7 @@ define [ afterRender: -> super - @setToggleTooltip() + @updateReadState() toggleRead: (e) -> e.stopPropagation() @@ -43,12 +43,12 @@ define [ else @model.markAsRead() - toggleReadState: (model, read_state) => - @setToggleTooltip() - @$entryContent.toggleClass 'unread', read_state is 'unread' - @$entryContent.toggleClass 'read', read_state is 'read' + updateReadState: => + @updateTooltip() + @$entryContent.toggleClass 'unread', @model.get('read_state') is 'unread' + @$entryContent.toggleClass 'read', @model.get('read_state') is 'read' - setToggleTooltip: -> + updateTooltip: -> tooltip = if @model.get('read_state') is 'unread' I18n.t('mark_as_read', 'Mark as Read') else diff --git a/app/controllers/discussion_topics_controller.rb b/app/controllers/discussion_topics_controller.rb index 9c466091509..01c6d9c539d 100644 --- a/app/controllers/discussion_topics_controller.rb +++ b/app/controllers/discussion_topics_controller.rb @@ -260,6 +260,8 @@ class DiscussionTopicsController < ApplicationController :UPDATE_URL => named_context_url(@context, :api_v1_context_discussion_update_reply_url, @topic, ':id'), :MARK_READ_URL => named_context_url(@context, :api_v1_context_discussion_topic_discussion_entry_mark_read_url, @topic, ':id'), :MARK_UNREAD_URL => named_context_url(@context, :api_v1_context_discussion_topic_discussion_entry_mark_unread_url, @topic, ':id'), + :MARK_ALL_READ_URL => named_context_url(@context, :api_v1_context_discussion_topic_mark_all_read_url, @topic), + :MARK_ALL_UNREAD_URL => named_context_url(@context, :api_v1_context_discussion_topic_mark_all_unread_url, @topic), :MANUAL_MARK_AS_READ => @current_user.manual_mark_as_read?, :CURRENT_USER => user_display_json(@current_user), :INITIAL_POST_REQUIRED => @initial_post_required, diff --git a/app/views/discussion_topics/show.html.erb b/app/views/discussion_topics/show.html.erb index 2ffc2708bdc..774bab470ce 100644 --- a/app/views/discussion_topics/show.html.erb +++ b/app/views/discussion_topics/show.html.erb @@ -72,65 +72,65 @@ <% end %> - <% if can_do(@topic, @current_user, :delete) || - @presenter.can_grade?(@current_user) || - @presenter.show_peer_reviews?(@current_user) || - @presenter.should_show_rubric?(@current_user) %> -
- - -
<%= t :manage_discussion, 'Manage Discussion' %>
- - +
diff --git a/spec/selenium/discussions_spec.rb b/spec/selenium/discussions_spec.rb index d3b0c81aa61..a9bbb4ec04e 100644 --- a/spec/selenium/discussions_spec.rb +++ b/spec/selenium/discussions_spec.rb @@ -168,7 +168,7 @@ describe "discussions" do it "should validate closing the discussion for comments" do create_and_go_to_topic f("#discussion-toolbar .al-trigger").click - expect_new_page_load { f("#ui-id-3").click } + expect_new_page_load { f(".discussion_locked_toggler").click } f('.discussion-fyi').text.should == 'This topic is closed for comments' ff('.discussion-reply-label').should be_empty DiscussionTopic.last.workflow_state.should == 'locked' @@ -177,7 +177,7 @@ describe "discussions" do it "should validate reopening the discussion for comments" do create_and_go_to_topic('closed discussion', 'side_comment', true) f("#discussion-toolbar .al-trigger").click - expect_new_page_load { f("#ui-id-3").click } + expect_new_page_load { f(".discussion_locked_toggler").click } ff('.discussion-reply-label').should_not be_empty DiscussionTopic.last.workflow_state.should == 'active' end @@ -603,7 +603,7 @@ describe "discussions" do wait_for_ajaximations f("#discussion-toolbar .al-trigger").click - expect_new_page_load { f("#ui-id-3").click } + expect_new_page_load { f(".discussion_locked_toggler").click } @topic.reload @topic.delayed_post_at.should be_nil @@ -751,12 +751,16 @@ describe "discussions" do f('#new-discussion-btn').should be_nil end - it "should not show an empty gear menu to students who've created a discussion" do + it "should not show admin options in gear menu to students who've created a discussion" do @student_topic = @course.discussion_topics.create!(:user => @student, :message => 'student topic', :discussion_type => 'side_comment') @student_entry = @student_topic.discussion_entries.create!(:user => @student, :message => 'student entry') get "/courses/#{@course.id}/discussion_topics/#{@student_topic.id}" wait_for_ajax_requests - f('.headerBar .admin-links').should be_nil + f('.headerBar .admin-links').should_not be_nil + f('.mark_all_as_read').should_not be_nil + #f('.mark_all_as_unread').should_not be_nil + f('.delete_discussion').should be_nil + f('.discussion_locked_toggler').should be_nil end it "should allow students to reply to a discussion even if they cannot create a topic" do @@ -970,11 +974,14 @@ describe "discussions" do end context "marking as read" do - it "should mark things as read" do - reply_count = 2 + before do course_with_student course_with_teacher_logged_in(:course => @course) @topic = @course.discussion_topics.create!(:title => 'mark as read test', :message => 'test mark as read', :user => @student) + end + + it "should automatically mark things as read" do + reply_count = 2 reply_count.times { @topic.discussion_entries.create!(:message => 'Lorem ipsum dolor sit amet', :user => @student) } @topic.create_materialized_view @@ -1006,5 +1013,29 @@ describe "discussions" do wait_for_ajaximations ff(".discussion_entry.unread").size.should == 1 end + + it "should mark all as read" do + reply_count = 8 + (reply_count / 2).times do |n| + entry = @topic.reply_from(:user => @student, :text => "entry #{n}") + entry.reply_from(:user => @student, :text => "sub reply #{n}") + end + @topic.create_materialized_view + + # so auto mark as read won't mess up this test + @teacher.preferences[:manual_mark_as_read] = true + @teacher.save! + + go_to_topic + + ff('.discussion-entries .unread').length.should == reply_count + ff('.discussion-entries .read').length.should == 0 + + f("#discussion-toolbar .al-trigger").click + f('.mark_all_as_read').click + wait_for_ajaximations + ff('.discussion-entries .unread').length.should == 0 + ff('.discussion-entries .read').length.should == reply_count + end end end