add forced_read_state to DiscussionEntryParticipant and APIs

Don't automatically mark a discussion entry as read if the user
manually marks the entry as unread.

fixes CNVS-5705

test plan:
 - As one user, create a discussion with lots of posts.
 - As a different user, go to the discussion, see the entries
   are automatically marked as read. Don't scroll all the way
   to the bottom of the discussion so there are still some
   unread posts.
 - Click the gray circle to manually mark some of the read
   entries as unread.
 - Refresh the page. The manually marked entries should show
   as unread and should not be automatically marked as read.
 - Scroll down. Unread entries that were not manually marked as
   unread should still be automatically marked as read.

Change-Id: I963d1dea9fc961a28a0a5c488399d4e3964c8d10
Reviewed-on: https://gerrit.instructure.com/20414
Tested-by: Jenkins <jenkins@instructure.com>
Reviewed-by: Zach Pendleton <zachp@instructure.com>
Product-Review: Marc LeGendre <marc@instructure.com>
QA-Review: Marc LeGendre <marc@instructure.com>

app/coffeescripts/models/DiscussionTopic.coffee

@@ -50,8 +50,9 @@ define [
     # see: http://<canvas>/doc/api/discussion_topics.html#method.discussion_topics_api.view
     fetchEntries: ->
       baseUrl = _.result this, 'url'
-      $.get "#{baseUrl}/view", ({unread_entries, participants, view: entries}) =>
+      $.get "#{baseUrl}/view", ({unread_entries, forced_entries, participants, view: entries}) =>
         @unreadEntries = unread_entries
+        @forcedEntries = forced_entries
         @participants.reset participants
 
         # TODO: handle nested replies and 'new_entries' here

app/coffeescripts/models/Entry.coffee

@@ -23,6 +23,7 @@ define [
       message: I18n.t('no_content', 'No Content')
       user_id: null
       read_state: 'read'
+      forced_read_state: false
       created_at: null
       updated_at: null
       deleted: false
@@ -90,6 +91,7 @@ define [
         'message'
         'user_id'
         'read_state'
+        'forced_read_state'
         'created_at'
         'updated_at'
         'deleted'

app/coffeescripts/models/Topic.coffee

@@ -23,6 +23,7 @@ define [
       entries: []
       new_entries: []
       unread_entries: []
+      forced_entries: []
 
     url: ->
       "#{@get 'root_url'}?include_new_entries=1"
@@ -77,6 +78,7 @@ define [
       parent = @flattened[entry.parent_id]
       entry.parent = parent
       entry.read_state = 'unread' if entry.id in @data.unread_entries
+      entry.forced_read_state = true if entry.id in @data.forced_entries
 
       @setEntryAuthor(entry)
 

app/controllers/discussion_topics_api_controller.rb

@@ -58,6 +58,10 @@ class DiscussionTopicsApiController < ApplicationController
   #   and avatar_url.
   # * "unread_entries": A list of entry ids that are unread by the current
   #   user. this implies that any entry not in this list is read.
+  # * "forced_entries": A list of entry ids that have forced_read_state set to
+  #   true. This flag is meant to indicate the entry's read_state has been 
+  #   manually set to 'unread' by the user, so the entry should not be
+  #   automatically marked as read.
   # * "view": A threaded view of all the entries in the discussion, containing
   #   the id, user_id, and message.
   # * "new_entries": Because this view is eventually consistent, it's possible
@@ -75,6 +79,7 @@ class DiscussionTopicsApiController < ApplicationController
   # @example_response
   #   {
   #     "unread_entries": [1,3,4],
+  #     "forced_entries": [1],
   #     "participants": [
   #       { "id": 10, "display_name": "user 1", "avatar_image_url": "https://...", "html_url": "https://..." },
   #       { "id": 11, "display_name": "user 2", "avatar_image_url": "https://...", "html_url": "https://..." }
@@ -96,10 +101,19 @@ class DiscussionTopicsApiController < ApplicationController
         user_display_json(user, @context.is_a_context? && @context)
       end
       unread_entries = entry_ids - DiscussionEntryParticipant.read_entry_ids(entry_ids, @current_user)
+      forced_entries = DiscussionEntryParticipant.forced_read_state_entry_ids(entry_ids, @current_user)
       # as an optimization, the view structure is pre-serialized as a json
       # string, so we have to do a bit of manual json building here to fit it
       # into the response.
-      render :json => %[{ "unread_entries": #{unread_entries.to_json}, "participants": #{participant_info.to_json}, "view": #{structure}, "new_entries": #{new_entries_structure} }]
+      fragments = {
+        :unread_entries => unread_entries.to_json,
+        :forced_entries => forced_entries.to_json,
+        :participants   => participant_info.to_json,
+        :view           => structure,
+        :new_entries    => new_entries_structure,
+      }
+      fragments = fragments.map { |k, v| %("#{k}": #{v}) }
+      render :json => "{ #{fragments.join(', ')} }"
     else
       render :nothing => true, :status => 503
     end
@@ -156,6 +170,8 @@ class DiscussionTopicsApiController < ApplicationController
   #
   # @response_field read_state The read state of the entry, "read" or "unread".
   #
+  # @response_field forced_read_state Whether the read_state was forced (was set manually)
+  #
   # @response_field created_at The creation time of the entry, in ISO8601
   #   format.
   #
@@ -181,6 +197,7 @@ class DiscussionTopicsApiController < ApplicationController
   #       "user_name": "nobody@example.com",
   #       "message": "Newer entry",
   #       "read_state": "read",
+  #       "forced_read_state": false,
   #       "created_at": "2011-11-03T21:33:29Z",
   #       "attachment": {
   #         "content-type": "unknown/unknown",
@@ -193,6 +210,7 @@ class DiscussionTopicsApiController < ApplicationController
   #       "user_name": "nobody@example.com",
   #       "message": "first top-level entry",
   #       "read_state": "unread",
+  #       "forced_read_state": false,
   #       "created_at": "2011-11-03T21:32:29Z",
   #       "recent_replies": [
   #         {
@@ -261,6 +279,8 @@ class DiscussionTopicsApiController < ApplicationController
   #
   # @response_field read_state The read state of the entry, "read" or "unread".
   #
+  # @response_field forced_read_state Whether the read_state was forced (was set manually)
+  #
   # @response_field created_at The creation time of the reply, in ISO8601
   #   format.
   #
@@ -271,6 +291,7 @@ class DiscussionTopicsApiController < ApplicationController
   #       "user_name": "nobody@example.com",
   #       "message": "Newer message",
   #       "read_state": "read",
+  #       "forced_read_state": false,
   #       "created_at": "2011-11-03T21:27:44Z" },
   #     {
   #       "id": 1014,
@@ -278,6 +299,7 @@ class DiscussionTopicsApiController < ApplicationController
   #       "user_name": "nobody@example.com",
   #       "message": "Older message",
   #       "read_state": "unread",
+  #       "forced_read_state": false,
   #       "created_at": "2011-11-03T21:26:44Z" } ]
   def replies
     @parent = root_entries(@topic).find(params[:entry_id])
@@ -306,6 +328,8 @@ class DiscussionTopicsApiController < ApplicationController
   #
   # @response_field read_state The read state of the entry, "read" or "unread".
   #
+  # @response_field forced_read_state Whether the read_state was forced (was set manually)
+  #
   # @response_field created_at The creation time of the reply, in ISO8601
   #   format.
   #
@@ -370,6 +394,8 @@ class DiscussionTopicsApiController < ApplicationController
   #
   # No request fields are necessary.
   #
+  # @argument forced_read_state [Optional] A boolean value to set all of the entries' forced_read_state. No change is made if this argument is not specified.
+  # 
   # On success, the response will be 204 No Content with an empty body.
   #
   # @example_request
@@ -379,10 +405,7 @@ class DiscussionTopicsApiController < ApplicationController
   #        -H "Authorization: Bearer <token>" \ 
   #        -H "Content-Length: 0"
   def mark_all_read
-    if authorized_action(@topic, @current_user, :read)
-      @topic.change_all_read_state("read", @current_user)
-      render :json => {}, :status => :no_content
-    end
+    change_topic_all_read_state('read')
   end
 
   # @API Mark all entries as unread
@@ -390,6 +413,8 @@ class DiscussionTopicsApiController < ApplicationController
   #
   # No request fields are necessary.
   #
+  # @argument forced_read_state [Optional] A boolean value to set all of the entries' forced_read_state. No change is made if this argument is not specified.
+  # 
   # On success, the response will be 204 No Content with an empty body.
   #
   # @example_request
@@ -398,10 +423,7 @@ class DiscussionTopicsApiController < ApplicationController
   #        -X DELETE \ 
   #        -H "Authorization: Bearer <token>"
   def mark_all_unread
-    if authorized_action(@topic, @current_user, :read)
-      @topic.change_all_read_state("unread", @current_user)
-      render :json => {}, :status => :no_content
-    end
+    change_topic_all_read_state('unread')
   end
 
   # @API Mark entry as read
@@ -409,6 +431,8 @@ class DiscussionTopicsApiController < ApplicationController
   #
   # No request fields are necessary.
   #
+  # @argument forced_read_state [Optional] A boolean value to set the entry's forced_read_state. No change is made if this argument is not specified.
+  #
   # On success, the response will be 204 No Content with an empty body.
   #
   # @example_request
@@ -426,6 +450,8 @@ class DiscussionTopicsApiController < ApplicationController
   #
   # No request fields are necessary.
   #
+  # @argument forced_read_state [Optional] A boolean value to set the entry's forced_read_state. No change is made if this argument is not specified.
+  #
   # On success, the response will be 204 No Content with an empty body.
   #
   # @example_request
@@ -519,10 +545,27 @@ class DiscussionTopicsApiController < ApplicationController
     end
   end
 
+  def get_forced_option()
+    opts = {}
+    opts[:forced] = value_to_boolean(params[:forced_read_state]) if params.has_key?(:forced_read_state)    
+    opts
+  end
+
+  def change_topic_all_read_state(new_state)
+    opts = get_forced_option
+
+    if authorized_action(@topic, @current_user, :read)
+      @topic.change_all_read_state(new_state, @current_user, opts)
+      render :json => {}, :status => :no_content
+    end
+  end
+
   def change_entry_read_state(new_state)
     @entry = @topic.discussion_entries.find(params[:entry_id])
+    opts = get_forced_option
+
     if authorized_action(@entry, @current_user, :read)
-      entry_participant = @entry.change_read_state(new_state, @current_user)
+      entry_participant = @entry.change_read_state(new_state, @current_user, opts)
       if entry_participant.present? && (entry_participant == true || entry_participant.errors.blank?)
         render :nothing => true, :status => :no_content
       else

app/models/discussion_entry.rb

@@ -387,7 +387,7 @@ class DiscussionEntry < ActiveRecord::Base
   def read_state(current_user = nil)
     current_user ||= self.current_user
     return "read" unless current_user # default for logged out users
-    discussion_entry_participants.find_by_user_id(current_user).try(:workflow_state) || "unread"
+    find_existing_participant(current_user).workflow_state
   end
 
   def read?(current_user = nil)
@@ -398,14 +398,25 @@ class DiscussionEntry < ActiveRecord::Base
     !read?(current_user)
   end
 
-  def change_read_state(new_state, current_user = nil)
+  # Public: Change the workflow_state of the entry for the specified user.
+  #
+  # new_state    - The new workflow_state.
+  # current_user - The User to to change state for. This function does nothing
+  #                if nil is passed. (default: self.current_user)
+  # opts         - Additional named arguments (default: {})
+  #                :forced - Also set the forced_read_state to this value.
+  #
+  # Returns nil if current_user is nil, the DiscussionEntryParticipent if the
+  # read_state was changed, or true if the read_state was not changed. If the
+  # read_state is not changed, a participant record will not be created.
+  def change_read_state(new_state, current_user = nil, opts = {})
     current_user ||= self.current_user
     return nil unless current_user
 
     if new_state != self.read_state(current_user)
-      entry_participant = self.update_or_create_participant(:current_user => current_user, :new_state => new_state)
+      entry_participant = self.update_or_create_participant(opts.merge(:current_user => current_user, :new_state => new_state))
       if entry_participant.present? && entry_participant.valid?
-        self.discussion_topic.update_or_create_participant(:current_user => current_user, :offset => (new_state == "unread" ? 1 : -1))
+        self.discussion_topic.update_or_create_participant(opts.merge(:current_user => current_user, :offset => (new_state == "unread" ? 1 : -1)))
       end
       entry_participant
     else
@@ -413,6 +424,18 @@ class DiscussionEntry < ActiveRecord::Base
     end
   end
 
+  # Public: Update and save the DiscussionEntryParticipant for a specified user,
+  # creating it if necessary. This function properly handles race conditions of
+  # calling this function simultaneously in two separate processes.
+  #
+  # opts - The options for this operation
+  #        :current_user - The User that should have its participant updated.
+  #                        (default: self.current_user)
+  #        :new_state    - The new workflow_state for the participant.
+  #        :forced       - The new forced_read_state for the participant.
+  #
+  # Returns the DiscussionEntryParticipant for the specified User, or nil if no
+  # current_user is specified.
   def update_or_create_participant(opts={})
     current_user = opts[:current_user] || self.current_user
     return nil unless current_user
@@ -423,9 +446,36 @@ class DiscussionEntry < ActiveRecord::Base
         entry_participant = self.discussion_entry_participants.where(:user_id => current_user).first
         entry_participant ||= self.discussion_entry_participants.build(:user => current_user, :workflow_state => "unread")
         entry_participant.workflow_state = opts[:new_state] if opts[:new_state]
+        entry_participant.forced_read_state = opts[:forced] if opts.has_key?(:forced)
         entry_participant.save
       end
     end
     entry_participant
   end
+
+  # Public: Find the existing DiscussionEntryParticipant, or create a default
+  # participant, for the specified user.
+  # 
+  # user - The User to lookup the participant for.
+  # 
+  # Returns the DiscussionEntryParticipant for the user, or a participant with
+  # default values set. The returned record is marked as readonly! If you need
+  # to update a participant, use the #update_or_create_participant method
+  # instead.
+  def find_existing_participant(user)
+    participant = discussion_entry_participants.where(:user_id => user).first
+    unless participant
+      # return a temporary record with default values
+      participant = discussion_entry_participants.new({
+        :workflow_state => "unread",
+        :forced_read_state => false,
+        })
+      participant.user = user
+    end
+
+    # Do not save this record. Use update_or_create_participant instead if you need to save it
+    participant.readonly!
+    participant
+  end
+
 end

app/models/discussion_entry_participant.rb

@@ -20,13 +20,19 @@ class DiscussionEntryParticipant < ActiveRecord::Base
   include Workflow
 
   # Be more restrictive if this is ever updatable from user params
-  attr_accessible :discussion_entry, :user, :workflow_state
+  attr_accessible :discussion_entry, :user, :workflow_state, :forced_read_state
 
   belongs_to :discussion_entry
   belongs_to :user
 
   def self.read_entry_ids(entry_ids, user)
-    self.connection.select_values(sanitize_sql_array ["SELECT discussion_entry_id FROM #{connection.quote_table_name(table_name)} WHERE user_id = ? AND discussion_entry_id IN (?) AND workflow_state = ?", user.id, entry_ids, 'read']).map(&:to_i)
+    self.where(:user_id => user, :discussion_entry_id => entry_ids, :workflow_state => 'read').
+      pluck(:discussion_entry_id)
+  end
+
+  def self.forced_read_state_entry_ids(entry_ids, user)
+    self.where(:user_id => user, :discussion_entry_id => entry_ids, :forced_read_state => true).
+      pluck(:discussion_entry_id)
   end
 
   workflow do

app/models/discussion_topic.rb

@@ -249,10 +249,13 @@ class DiscussionTopic < ActiveRecord::Base
     self.update_or_create_participant(:current_user => current_user, :new_state => new_state)
   end
 
-  def change_all_read_state(new_state, current_user = nil)
+  def change_all_read_state(new_state, current_user = nil, opts = {})
     current_user ||= self.current_user
     return unless current_user
 
+    update_fields = { :workflow_state => new_state }
+    update_fields[:forced_read_state] = opts[:forced] if opts.has_key?(:forced)
+
     transaction do
       self.context_module_action(current_user, :read) if new_state == 'read'
       StreamItem.update_read_state_for_asset(self, new_state, current_user.id)
@@ -265,7 +268,7 @@ class DiscussionTopic < ActiveRecord::Base
         existing_entry_participants = DiscussionEntryParticipant.where(:user_id =>current_user, :discussion_entry_id => entry_ids).
           select([:id, :discussion_entry_id]).all
         existing_ids = existing_entry_participants.map(&:id)
-        DiscussionEntryParticipant.where(:id => existing_ids).update_all(:workflow_state => new_state) if existing_ids.present?
+        DiscussionEntryParticipant.where(:id => existing_ids).update_all(update_fields) if existing_ids.present?
 
         if new_state == "read"
           new_entry_ids = entry_ids - existing_entry_participants.map(&:discussion_entry_id)
@@ -273,8 +276,7 @@ class DiscussionTopic < ActiveRecord::Base
             {
               :discussion_entry_id => entry_id,
               :user_id => current_user.id,
-              :workflow_state => new_state
-            }
+            }.merge(update_fields)
           })
         end
       end

db/migrate/20130506191104_add_forced_read_state_to_discussion_entry_participants.rb

@@ -0,0 +1,13 @@
+class AddForcedReadStateToDiscussionEntryParticipants < ActiveRecord::Migration
+  tag :predeploy
+  self.transactional = false
+
+  def self.up
+    add_column :discussion_entry_participants, :forced_read_state, :boolean
+    DiscussionEntryParticipant.reset_column_information
+  end
+
+  def self.down
+    remove_column :discussion_entry_participants, :forced_read_state
+  end
+end

lib/api/v1/discussion_topics.rb

@@ -96,7 +96,12 @@ module Api::V1::DiscussionTopics
           json[:attachments] = [json[:attachment]]
         end
       end
-      json[:read_state] = entry.read_state(user) if user
+
+      if user
+        participant = entry.find_existing_participant(user)
+        json[:read_state] = participant.workflow_state
+        json[:forced_read_state] = participant.forced_read_state?
+      end
 
       if includes.include?(:subentries) && entry.root_entry_id.nil?
         replies = entry.flattened_discussion_subentries.active.newest_first.limit(11).all

spec/apis/v1/discussion_topics_api_spec.rb

@@ -688,6 +688,7 @@ describe DiscussionTopicsController, :type => :integration do
         "user_id" => @user.id,
         "user_name" => @user.name,
         "read_state" => "read",
+        "forced_read_state" => false,
         "message" => @message,
         "created_at" => @entry.created_at.utc.iso8601,
         "updated_at" => @entry.updated_at.as_json,
@@ -1258,9 +1259,9 @@ describe DiscussionTopicsController, :type => :integration do
     end
 
     def call_mark_entry_unread(course, topic, entry)
-      raw_api_call(:delete, "/api/v1/courses/#{course.id}/discussion_topics/#{topic.id}/entries/#{entry.id}/read.json",
+      raw_api_call(:delete, "/api/v1/courses/#{course.id}/discussion_topics/#{topic.id}/entries/#{entry.id}/read.json?forced_read_state=true",
                       { :controller => 'discussion_topics_api', :action => 'mark_entry_unread', :format => 'json',
-                        :course_id => course.id.to_s, :topic_id => topic.id.to_s, :entry_id => entry.id.to_s })
+                        :course_id => course.id.to_s, :topic_id => topic.id.to_s, :entry_id => entry.id.to_s, :forced_read_state => "true" })
     end
 
     it "should set the read state for a entry" do
@@ -1268,12 +1269,20 @@ describe DiscussionTopicsController, :type => :integration do
       call_mark_entry_read(@course, @topic, @entry)
       response.status.should == '204 No Content'
       @entry.read?(@user).should be_true
+      @entry.find_existing_participant(@user).should_not be_forced_read_state
       @topic.unread_count(@user).should == 1
 
       call_mark_entry_unread(@course, @topic, @entry)
       response.status.should == '204 No Content'
       @entry.read?(@user).should be_false
+      @entry.find_existing_participant(@user).should be_forced_read_state
       @topic.unread_count(@user).should == 2
+
+      call_mark_entry_read(@course, @topic, @entry)
+      response.status.should == '204 No Content'
+      @entry.read?(@user).should be_true
+      @entry.find_existing_participant(@user).should be_forced_read_state
+      @topic.unread_count(@user).should == 1
     end
 
     it "should be idempotent for setting entry read state" do
@@ -1289,24 +1298,50 @@ describe DiscussionTopicsController, :type => :integration do
       @topic.unread_count(@user).should == 1
     end
 
-    it "should allow mark all as read/unread" do
+    def call_mark_all_as_read_state(new_state, opts = {})
+      method = new_state == 'read' ? :put : :delete
+      url = "/api/v1/courses/#{@course.id}/discussion_topics/#{@topic.id}/read_all.json"
+      expected_params = { :controller => 'discussion_topics_api', :action => "mark_all_#{new_state}", :format => 'json',
+                        :course_id => @course.id.to_s, :topic_id => @topic.id.to_s }
+      if opts.has_key?(:forced)
+        url << "?forced_read_state=#{opts[:forced]}"
+        expected_params[:forced_read_state] = opts[:forced].to_s
+      end
+      raw_api_call(method, url, expected_params)
+    end
+
+    it "should allow mark all as read without forced update" do
       student_in_course(:active_all => true)
-      raw_api_call(:put, "/api/v1/courses/#{@course.id}/discussion_topics/#{@topic.id}/read_all.json",
-                      { :controller => 'discussion_topics_api', :action => 'mark_all_read', :format => 'json',
-                        :course_id => @course.id.to_s, :topic_id => @topic.id.to_s })
+      @entry.change_read_state('read', @user, :forced => true)
+
+      call_mark_all_as_read_state('read')
       response.status.should == '204 No Content'
       @topic.reload
       @topic.read?(@user).should be_true
-      @entry.read?(@user).should be_true
-      @topic.unread_count(@user).should == 0
 
-      raw_api_call(:delete, "/api/v1/courses/#{@course.id}/discussion_topics/#{@topic.id}/read_all.json",
-                      { :controller => 'discussion_topics_api', :action => 'mark_all_unread', :format => 'json',
-                        :course_id => @course.id.to_s, :topic_id => @topic.id.to_s })
+      @entry.read?(@user).should be_true
+      @entry.find_existing_participant(@user).should be_forced_read_state
+
+      @reply.read?(@user).should be_true
+      @reply.find_existing_participant(@user).should_not be_forced_read_state
+
+      @topic.unread_count(@user).should == 0
+    end
+
+    it "should allow mark all as unread with forced update" do
+      [@topic, @entry].each { |e| e.change_read_state('read', @user) }
+
+      call_mark_all_as_read_state('unread', :forced => true)
       response.status.should == '204 No Content'
       @topic.reload
       @topic.read?(@user).should be_false
+
       @entry.read?(@user).should be_false
+      @entry.find_existing_participant(@user).should be_forced_read_state
+
+      @reply.read?(@user).should be_false
+      @reply.find_existing_participant(@user).should be_forced_read_state
+
       @topic.unread_count(@user).should == 2
     end
   end
@@ -1577,7 +1612,7 @@ describe DiscussionTopicsController, :type => :integration do
     it "should return an empty discussion view for an item" do
       json = api_call(:get, "/api/v1/collection_items/#{@item.id}/discussion_topics/self/view",
         { :collection_item_id => "#{@item.id}", :controller => "discussion_topics_api", :format => "json", :action => "view", :topic_id => "self"})
-      json.should == { "participants" => [], "unread_entries" => [], "view" => [], "new_entries" => [] }
+      json.should == { "participants" => [], "unread_entries" => [], "forced_entries" => [], "view" => [], "new_entries" => [] }
       @item.discussion_topic.should be_new_record
     end
 

spec/apis/v1/submissions_api_spec.rb

@@ -251,6 +251,7 @@ describe 'Submissions API', :type => :integration do
         'message' => 'sub 1',
         'user_id' => @student.id,
         'read_state' => 'unread',
+        'forced_read_state' => false,
         'parent_id' => e1.id,
         'created_at' => se1.created_at.as_json,
         'updated_at' => se1.updated_at.as_json,
@@ -261,6 +262,7 @@ describe 'Submissions API', :type => :integration do
         'message' => 'student 1',
         'user_id' => @student.id,
         'read_state' => 'unread',
+        'forced_read_state' => false,
         'parent_id' => nil,
         'created_at' => se2.created_at.as_json,
         'updated_at' => se2.updated_at.as_json,
@@ -317,6 +319,7 @@ describe 'Submissions API', :type => :integration do
         'user_id' => @student.id,
         'user_name' => 'User',
         'read_state' => 'unread',
+        'forced_read_state' => false,
         'parent_id' => e1.id,
         'created_at' => se1.created_at.as_json,
         'updated_at' => se1.updated_at.as_json,
@@ -327,6 +330,7 @@ describe 'Submissions API', :type => :integration do
         'user_id' => @student.id,
         'user_name' => 'User',
         'read_state' => 'unread',
+        'forced_read_state' => false,
         'parent_id' => nil,
         'created_at' => se2.created_at.as_json,
         'updated_at' => se2.updated_at.as_json,

spec/models/discussion_entry_spec.rb

@@ -411,18 +411,64 @@ describe DiscussionEntry do
     end
   end
 
-  context "DiscussionEntryParticipant.read_entry_ids" do
-    it "should return the ids of the read entries" do
+  describe "DiscussionEntryParticipant" do
+    before do
       topic_with_nested_replies
-      @root2.change_read_state('read', @teacher)
-      @reply_reply1.change_read_state('read', @teacher)
-      @reply_reply2.change_read_state('read', @teacher)
-      @reply3.change_read_state('read', @teacher)
-      # change one back to unread, it shouldn't be returned
-      @reply_reply2.change_read_state('unread', @teacher)
-      read = DiscussionEntryParticipant.read_entry_ids(@topic.discussion_entries.map(&:id), @teacher).sort
-      read.should == [@root2, @reply1, @reply2, @reply_reply1, @reply3].map(&:id)
     end
+
+    context ".read_entry_ids" do
+      it "should return the ids of the read entries" do
+        @root2.change_read_state('read', @teacher)
+        @reply_reply1.change_read_state('read', @teacher)
+        @reply_reply2.change_read_state('read', @teacher)
+        @reply3.change_read_state('read', @teacher)
+        # change one back to unread, it shouldn't be returned
+        @reply_reply2.change_read_state('unread', @teacher)
+        read = DiscussionEntryParticipant.read_entry_ids(@topic.discussion_entries.map(&:id), @teacher).sort
+        read.should == [@root2, @reply1, @reply2, @reply_reply1, @reply3].map(&:id)
+      end
+    end
+
+    context ".forced_read_state_entry_ids" do
+      it "should return the ids of entries that have been marked as force_read_state" do
+        marked_entries = [@root2, @reply_reply1, @reply_reply2, @reply3]
+        marked_entries.each do |e|
+          e.change_read_state('read', @teacher, :forced => true)
+        end
+        # change back, without :forced parameter, should stay forced
+        @reply_reply2.change_read_state('unread', @teacher)
+        # change forced to false so it shouldn't be in results
+        @reply3.change_read_state('unread', @teacher, :forced => false)
+        marked_entries -= [@reply3]
+
+        forced = DiscussionEntryParticipant.forced_read_state_entry_ids(@all_entries.map(&:id), @teacher).sort
+        forced.should == marked_entries.map(&:id).sort
+      end
+    end
+
+    context ".find_existing_participant" do
+      it "should return existing data" do
+        @root2.change_read_state('read', @teacher, :forced => true)
+        participant = @root2.find_existing_participant(@teacher)
+        participant.id.should_not be_nil
+        participant.should be_readonly
+        participant.user.should == @teacher
+        participant.discussion_entry.should == @root2
+        participant.workflow_state.should == 'read'
+        participant.forced_read_state.should be_true
+      end
+
+      it "should return default data" do
+        participant = @reply2.find_existing_participant(@student)
+        participant.id.should be_nil
+        participant.should be_readonly
+        participant.user.should == @student
+        participant.discussion_entry.should == @reply2
+        participant.workflow_state.should == 'unread'
+        participant.forced_read_state.should be_false
+      end
+    end
+
   end
 
   describe "reply_from" do

spec/models/discussion_topic_spec.rb

@@ -834,23 +834,40 @@ describe DiscussionTopic do
       @topic.unread_count(@student).should == 0
     end
 
-    it "should allow mark all as unread" do
+    it "should allow mark all as unread with forced_read_state" do
       @entry = @topic.discussion_entries.create!(:message => "Hello!", :user => @teacher)
-      @topic.change_all_read_state("unread", @teacher)
-      @topic.reload
+      @reply = @entry.reply_from(:user => @student, :text => "ohai!")
+      @reply.change_read_state('read', @teacher, :forced => false)
 
-      @topic.read?(@student).should be_false
-      @entry.read?(@student).should be_false
-      @topic.unread_count(@student).should == 1
+      @topic.change_all_read_state("unread", @teacher, :forced => true)
+      @topic.reload
+      @topic.read?(@teacher).should be_false
+
+      @entry.read?(@teacher).should be_false
+      @entry.find_existing_participant(@teacher).should be_forced_read_state
+
+      @reply.read?(@teacher).should be_false
+      @reply.find_existing_participant(@teacher).should be_forced_read_state
+
+      @topic.unread_count(@teacher).should == 2
     end
 
-    it "should allow mark all as read" do
+    it "should allow mark all as read without forced_read_state" do
       @entry = @topic.discussion_entries.create!(:message => "Hello!", :user => @teacher)
+      @reply = @entry.reply_from(:user => @student, :text => "ohai!")
+      @reply.change_read_state('unread', @student, :forced => true)
+
       @topic.change_all_read_state("read", @student)
       @topic.reload
 
       @topic.read?(@student).should be_true
+
       @entry.read?(@student).should be_true
+      @entry.find_existing_participant(@student).should_not be_forced_read_state
+
+      @reply.read?(@student).should be_true
+      @reply.find_existing_participant(@student).should be_forced_read_state
+
       @topic.unread_count(@student).should == 0
     end
 

spec/selenium/discussions_spec.rb

@@ -971,29 +971,40 @@ describe "discussions" do
 
   context "marking as read" do
     it "should mark things as read" do
-      pending "figure out delayed jobs"
-      reply_count = 3
-      course_with_teacher_logged_in
-      @topic = @course.discussion_topics.create!
-      reply_count.times { @topic.discussion_entries.create!(:message => 'Lorem ipsum dolor sit amet') }
+      reply_count = 2
+      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)
+      reply_count.times { @topic.discussion_entries.create!(:message => 'Lorem ipsum dolor sit amet', :user => @student) }
+      @topic.create_materialized_view
 
       # make sure everything looks unread
       get "/courses/#{@course.id}/discussion_topics/#{@topic.id}"
-      ff('.can_be_marked_as_read.unread').length.should == reply_count + 1
+      ff('.discussion_entry.unread').length.should == reply_count
       f('.new-and-total-badge .new-items').text.should == reply_count.to_s
 
       #wait for the discussionEntryReadMarker to run, make sure it marks everything as .just_read
-      sleep 2
-      ff('.can_be_marked_as_read.unread').should be_empty
-      ff('.can_be_marked_as_read.just_read').length.should == reply_count + 1
+      keep_trying_until { ff('.discussion_entry.unread').should be_empty }
+      ff('.discussion_entry.read').length.should == reply_count + 1 # +1 because the topic also has the .discussion_entry class
+
+      # refresh page and make sure nothing is unread and everthing is .read
+      get "/courses/#{@course.id}/discussion_topics/#{@topic.id}"
+      ff(".discussion_entry.unread").should be_empty
       f('.new-and-total-badge .new-items').text.should == ''
 
-      # refresh page and make sure nothing is unread/just_read and everthing is .read
+      # Mark one as unread manually, and create a new reply. The new reply 
+      # should be automarked as read, but the manual one should not.
+      f('.discussion-read-state-btn').click
+      wait_for_ajaximations
+      @topic.discussion_entries.create!(:message => 'new lorem ipsum', :user => @student)
+      @topic.create_materialized_view
+
       get "/courses/#{@course.id}/discussion_topics/#{@topic.id}"
-      ['unread', 'just_read'].each do |state|
-        ff(".can_be_marked_as_read.#{state}").should be_empty
-      end
-      f('.new-and-total-badge .new-items').text.should == ''
+      ff(".discussion_entry.unread").size.should == 2
+      f('.new-and-total-badge .new-items').text.should == '2'
+      keep_trying_until { ff('.discussion_entry.unread').size < 2 }
+      wait_for_ajaximations
+      ff(".discussion_entry.unread").size.should == 1
     end
   end
 end