Revert "exclude deleted discussion entries from read counts; fixes #7741"

This reverts commit 76c1086dca.

Conflicts:

	lib/data_fixup/exclude_deleted_entries_from_unread_count.rb
	spec/migrations/exclude_deleted_entries_from_counts.rb

Change-Id: Ie75780d2fdfa17896e75e80dfd18749cc8eed1c8
Reviewed-on: https://gerrit.instructure.com/9486
Tested-by: Hudson <hudson@instructure.com>
Reviewed-by: Simon Williams <simon@instructure.com>
Reviewed-by: Jon Jensen <jon@instructure.com>

app/models/discussion_entry.rb

@@ -27,7 +27,7 @@ class DiscussionEntry < ActiveRecord::Base
   has_many :discussion_subentries, :class_name => 'DiscussionEntry', :foreign_key => "parent_id", :order => :created_at
   has_many :unordered_discussion_subentries, :class_name => 'DiscussionEntry', :foreign_key => "parent_id"
   has_many :flattened_discussion_subentries, :class_name => 'DiscussionEntry', :foreign_key => "root_entry_id"
-  has_many :discussion_entry_participants
+  has_many :discussion_entry_participants, :dependent => :destroy
   belongs_to :discussion_topic, :touch => true
   # null if a root entry
   belongs_to :parent_entry, :class_name => 'DiscussionEntry', :foreign_key => :parent_id
@@ -159,7 +159,6 @@ class DiscussionEntry < ActiveRecord::Base
   alias_method :destroy!, :destroy
   def destroy
     flattened_discussion_subentries.destroy_all
-    destroy_participants
     self.workflow_state = 'deleted'
     self.deleted_at = Time.now
     save!
@@ -193,6 +192,7 @@ class DiscussionEntry < ActiveRecord::Base
   end
 
   named_scope :active, :conditions => ['discussion_entries.workflow_state != ?', 'deleted']
+  named_scope :deleted, :conditions => ['discussion_entries.workflow_state = ?', 'deleted']
 
   def user_name
     self.user.name rescue t :default_user_name, "User Name"
@@ -376,20 +376,6 @@ class DiscussionEntry < ActiveRecord::Base
     end
   end
 
-  def destroy_participants
-    transaction do
-      # this could count toward the unread count either if there is a "unread"
-      # entry participant or no entry participant, so find all that have
-      # explicitly been marked "read" and decrement for all the others
-      read_deps = DiscussionEntryParticipant.find(:all, :conditions => { :discussion_entry_id => self.id, :workflow_state => "read" })
-      read_user_ids = read_deps.map(&:user_id)
-      dtp_conditions = sanitize_sql(["discussion_topic_id = ?", self.discussion_topic_id])
-      dtp_conditions = sanitize_sql(["discussion_topic_id = ? AND user_id NOT IN (?)", self.discussion_topic_id, read_user_ids]) if read_user_ids.present?
-      DiscussionTopicParticipant.update_all("unread_entry_count = unread_entry_count - 1", dtp_conditions)
-      self.discussion_entry_participants.destroy_all
-    end
-  end
-
   attr_accessor :current_user
   def read_state(current_user = nil)
     current_user ||= self.current_user

app/models/discussion_topic.rb

@@ -221,7 +221,7 @@ class DiscussionTopic < ActiveRecord::Base
       new_count = (new_state == 'unread' ? self.default_unread_count : 0)
       self.update_or_create_participant(:current_user => current_user, :new_state => new_state, :new_count => new_count)
 
-      entry_ids = self.discussion_entries.active.map(&:id)
+      entry_ids = self.discussion_entries.map(&:id)
       if entry_ids.present?
         existing_entry_participants = DiscussionEntryParticipant.find(:all, :conditions => ["user_id = ? AND discussion_entry_id IN (?)",
                                                                       current_user.id, entry_ids])
@@ -243,7 +243,7 @@ class DiscussionTopic < ActiveRecord::Base
   end
 
   def default_unread_count
-    self.discussion_entries.active.count
+    self.discussion_entries.count
   end
 
   def unread_count(current_user = nil)

db/migrate/20120319184846_reintroduce_deleted_entries_to_unread_count.rb

@@ -0,0 +1,10 @@
+class ReintroduceDeletedEntriesToUnreadCount < ActiveRecord::Migration
+  tag :postdeploy
+
+  def self.up
+    DataFixup::ReintroduceDeletedEntriesToUnreadCount.send_later_if_production_enqueue_args(:run, :priority => Delayed::LOW_PRIORITY)
+  end
+
+  def self.down
+  end
+end

lib/data_fixup/reintroduce_deleted_entries_to_unread_count.rb

@@ -0,0 +1,23 @@
+module DataFixup::ReintroduceDeletedEntriesToUnreadCount
+  def self.run
+    # Recalculate counts to include deleted entries
+    DiscussionTopicParticipant.find_each(:include => [:discussion_topic, :user]) do |participant|
+      # since the previous code treated all deleted discussion entries as
+      # hidden and not included in unread counts, we're going to update all
+      # pre-existing deleted entries to be marked as read for all users
+      #
+      # and then the new behavior will only apply going forward
+      topic = participant.discussion_topic
+      topic.discussion_entries.deleted.each do |entry|
+        entry.update_or_create_participant(:current_user => participant.user, :new_state => 'read')
+      end
+
+      # in theory this count won't need updating, but race conditions mean it
+      # could be out of sync after the above, so we'll update it here. if it
+      # doesn't change, the participant won't get re-saved
+      read_count = topic.discussion_entry_participants.scoped(:conditions => { :user_id => participant.user_id, :workflow_state => "read" }).count
+      participant.unread_entry_count = topic.discussion_entries.count - read_count
+      participant.save
+    end
+  end
+end

spec/apis/v1/discussion_topics_api_spec.rb

@@ -884,10 +884,7 @@ describe DiscussionTopicsController, :type => :integration do
                   { :controller => "discussion_topics_api", :action => "entry_list", :format => "json", :course_id => @course.id.to_s, :topic_id => @topic.id.to_s },
                  { :ids => @sub1.id })
         json.size.should == 1
-        pending("read state should be read") do
-          json.first.delete('read_state').should == 'read'
-        end
-        json.first.should == { 'id' => @sub1.id, 'deleted' => true, 'parent_id' => @entry.id, 'updated_at' => @sub1.updated_at.as_json, 'created_at' => @sub1.created_at.as_json }
+        json.first.should == { 'id' => @sub1.id, 'deleted' => true, 'read_state' => 'read', 'parent_id' => @entry.id, 'updated_at' => @sub1.updated_at.as_json, 'created_at' => @sub1.created_at.as_json }
       end
     end
   end

spec/migrations/reintroduce_deleted_entries_to_unread_count.rb

@@ -0,0 +1,54 @@
+#
+# Copyright (C) 2012 Instructure, Inc.
+#
+# This file is part of Canvas.
+#
+# Canvas is free software: you can redistribute it and/or modify it under
+# the terms of the GNU Affero General Public License as published by the Free
+# Software Foundation, version 3 of the License.
+#
+# Canvas is distributed in the hope that it will be useful, but WITHOUT ANY
+# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR
+# A PARTICULAR PURPOSE. See the GNU Affero General Public License for more
+# details.
+#
+# You should have received a copy of the GNU Affero General Public License along
+# with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+
+require File.expand_path(File.dirname(__FILE__) + '/../spec_helper.rb')
+
+describe DataFixup::ReintroduceDeletedEntriesToUnreadCount do
+  it "should mark existing deleted entries as read" do
+    course_with_teacher(:active_user => true, :active_course => true)
+    @student1 = student_in_course(:active_all => true).user
+    @student2 = student_in_course(:active_all => true).user
+
+    @topic = @course.discussion_topics.create!(:title => "hi", :message => "there", :user => @teacher)
+    @entry1 = @topic.discussion_entries.create!(:message => "it's me", :user => @teacher)
+    @entry2 = @topic.discussion_entries.create!(:message => "it's me again", :user => @teacher)
+    @entry3 = @topic.discussion_entries.create!(:message => "one more", :user => @teacher)
+
+    @topic2 = @course.discussion_topics.create!(:title => "hi again", :message => "blah", :user => @teacher)
+    @entry2_1 = @topic2.discussion_entries.create!(:message => "yo", :user => @teacher)
+    @entry2_1.change_read_state("read", @student1)
+
+    @entry1.change_read_state("read", @student1)
+    @entry2.change_read_state("read", @student2)
+    @entry2.destroy
+
+    # run the previous migration, which deletes read status for deleted messages
+    DataFixup::ExcludeDeletedEntriesFromUnreadCount.run
+    @topic.unread_count(@student2).should == 2
+    DiscussionEntryParticipant.find_by_user_id_and_discussion_entry_id(@student2.id, @entry2.id).should be_nil
+
+    # now the new migration, which marks existing deleted entries as read for all
+    DataFixup::ReintroduceDeletedEntriesToUnreadCount.run
+    @topic.unread_count(@student2).should == 2
+    DiscussionEntryParticipant.find_by_user_id_and_discussion_entry_id(@student2.id, @entry2.id).should be_present
+    @topic.unread_count(@teacher).should == 0
+    @topic2.unread_count(@teacher).should == 0
+    @topic.unread_count(@student1).should == 1
+    @topic2.unread_count(@student1).should == 0
+  end
+end

spec/models/discussion_entry_spec.rb

@@ -273,17 +273,6 @@ describe DiscussionEntry do
       @topic.unread_count(@student).should == 2
     end
 
-    it "should decrement unread counts on destroy" do
-      @topic.unread_count(@student).should == 1
-      @entry.change_read_state("read", @student)
-      @topic.unread_count(@student).should == 0
-      @entry2 = @topic.discussion_entries.create!(:message => "entry 2", :user => @teacher)
-      @topic.unread_count(@student).should == 1
-      @entry2.destroy
-      @topic.unread_count(@student).should == 0
-      @topic.unread_count(@teacher).should == 0
-    end
-
     it "should allow a complex series of read/unread updates" do
       @s1 = @student
       student_in_course(:active_all => true); @s2 = @student
@@ -320,11 +309,9 @@ describe DiscussionEntry do
       @topic.unread_count(@s2).should == 4
       @topic.read?(@s2).should be_false
       @entry.read?(@s2).should be_false
-      @s1entry.destroy
-      @topic.unread_count(@s2).should == 3
 
       student_in_course(:active_all => true); @s4 = @student
-      @topic.unread_count(@s4).should == 3
+      @topic.unread_count(@s4).should == 4
       @topic.change_all_read_state("unread", @s4)
       @topic.read?(@s4).should be_false
       @entry.read?(@s4).should be_false
@@ -376,10 +363,7 @@ describe DiscussionEntry do
       # 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
-      pending("deleted entries marked as read") do
-        read.delete(@reply1.id).should be_present
-      end
-      read.should == [@root2, @reply2, @reply_reply1, @reply3].map(&:id)
+      read.should == [@root2, @reply1, @reply2, @reply_reply1, @reply3].map(&:id)
     end
   end
 end

spec/models/discussion_topic_spec.rb

@@ -663,13 +663,6 @@ describe DiscussionTopic do
       DiscussionTopic.expects(:unique_constraint_retry).once
       @topic.change_all_read_state("unread", @student)
     end
-
-    it "should use active entries as deafult unread count" do
-      @entry1 = @topic.discussion_entries.create!(:message => "HI 1", :user => @teacher)
-      @entry2 = @topic.discussion_entries.create!(:message => "HI 2", :user => @teacher)
-      @entry2.destroy
-      @topic.unread_count(@student).should == 1
-    end
   end
 
   context "materialized view" do