fix conversation count bug, closes #5428

simplified the logic a bit, added specs. note that CPs are never auto-
marked-as-read any more for the sender (though newly created CPs are now
"read" by default, until the first message is added by someone other
than the user)

Change-Id: I3d113fe6135d1f8c6f07b52a245a6de7d25b7f9b
Reviewed-on: https://gerrit.instructure.com/5337
Reviewed-by: Jacob Fugal <jacob@instructure.com>
Tested-by: Hudson <hudson@instructure.com>

Author: jenseng

app/models/conversation.rb

@@ -54,9 +54,9 @@ def self.initiate(user_ids, private)
         user_ids.each do |user_id|
           participant = conversation.conversation_participants.build
           participant.user_id = user_id
+          participant.workflow_state = 'read'
           participant.save!
         end
-        User.update_all('unread_conversations_count = unread_conversations_count + 1', :id => user_ids)
       end
       conversation
     end
@@ -70,13 +70,15 @@ def add_participants(current_user, user_ids)
       user_ids -= conversation_participants.map(&:user_id)
       next if user_ids.empty?
 
-      User.update_all('unread_conversations_count = unread_conversations_count + 1', :id => user_ids)
-
       last_message_at = conversation_messages.human.first.created_at
+      raise "can't add participants if there are no messages" unless last_message_at
       num_messages = conversation_messages.human.size
+
+      User.update_all('unread_conversations_count = unread_conversations_count + 1', :id => user_ids)
       user_ids.each do |user_id|
         participant = conversation_participants.build
         participant.user_id = user_id
+        participant.workflow_state = 'unread'
         participant.last_message_at = last_message_at
         participant.message_count = num_messages
         participant.save!
@@ -154,16 +156,10 @@ def add_message(current_user, body, options = {})
           ["(last_message_at IS NULL OR subscribed) AND user_id <> ?", current_user.id]
         )
 
-        # for the sender, update the timestamps
-        # marking it as read is a ui concern, unless it's the first message (e.g. we might be sending from outside the inbox)
+        # for the sender, update the timestamps (marking it as read is always a ui concern)
         if (sender_conversation = conversation_participants.find_by_user_id(current_user.id)) && (options[:update_for_sender] || sender_conversation.last_message_at)
-          mark_as_read = options[:update_for_sender] && sender_conversation.last_message_at.nil?
-          if mark_as_read && sender_conversation.unread? && sender_conversation.last_message_at 
-            User.update_all 'unread_conversations_count = unread_conversations_count - 1', :id => current_user.id
-          end
           sender_conversation.last_message_at = Time.now.utc
           sender_conversation.last_authored_at = Time.now.utc
-          sender_conversation.workflow_state = 'read' if mark_as_read
           sender_conversation.save
         end
 

app/models/conversation_participant.rb

@@ -142,15 +142,18 @@ def remove_messages(*to_delete)
       messages.clear if messages.all?(&:generated?)
     end
     update_cached_data
+    save
   end
 
   def subscribed=(value)
     super
-    if subscribed?
-      self.workflow_state = :unread
-      update_cached_data(false)
-    else
-      mark_as_read
+    if subscribed_changed?
+      if subscribed?
+        update_cached_data(false)
+        self.workflow_state = 'unread' if last_message_at_changed? && last_message_at > last_message_at_was
+      else
+        self.workflow_state = 'read' if unread?
+      end
     end
     subscribed?
   end
@@ -200,7 +203,6 @@ def update_cached_data(recalculate_count=true)
       self.has_media_objects = false
       self.label = nil
     end
-    save
   end
 
   def update_unread_count

db/migrate/20110830152834_conversations_count_fix.rb

@@ -0,0 +1,19 @@
+class ConversationsCountFix < ActiveRecord::Migration
+  def self.up
+    execute "UPDATE conversation_participants SET workflow_state = 'read' WHERE workflow_state = 'unread' AND last_message_at IS NULL"
+
+    execute <<-SQL
+    UPDATE users
+    SET unread_conversations_count = (
+      SELECT COUNT(*)
+      FROM conversation_participants
+      WHERE workflow_state = 'unread'
+        AND last_message_at IS NOT NULL
+        AND user_id = users.id
+    )
+    SQL
+  end
+
+  def self.down
+  end
+end

spec/controllers/conversations_controller_spec.rb

@@ -160,7 +160,7 @@ def conversation(num_other_users = 1)
 
       post 'workflow_event', :conversation_id => @conversation.conversation_id, :event => "mark_as_unread"
       response.should be_success
-      @conversation.unread?.should be_true
+      @conversation.reload.unread?.should be_true
     end
   end
 

spec/models/conversation_spec.rb

@@ -99,10 +99,11 @@
   end
 
   context "unread counts" do
-    it "should increment when creating a conversation" do
+    it "should increment for recipients when sending the first message in a conversation" do
       sender = user
       recipient = user
       root_convo = Conversation.initiate([sender.id, recipient.id], false)
+      ConversationParticipant.unread.size.should eql 0 # only once the first message is added
       root_convo.add_message(sender, 'test')
       sender.reload.unread_conversations_count.should eql 0
       sender.conversations.unread.size.should eql 0
@@ -117,7 +118,7 @@
       unsubscribed_guy = user
       root_convo = Conversation.initiate([sender.id, unread_guy.id, subscribed_guy.id, unsubscribed_guy.id], false)
       root_convo.add_message(sender, 'test')
-      
+
       unread_guy.reload.unread_conversations_count.should eql 1
       unread_guy.conversations.unread.size.should eql 1
       subscribed_guy.conversations.first.mark_as_read
@@ -142,7 +143,7 @@
       unread_guy = user
       root_convo = Conversation.initiate([sender.id, unread_guy.id], false)
       root_convo.add_message(sender, 'test')
-      
+
       unread_guy.reload.unread_conversations_count.should eql 1
       unread_guy.conversations.unread.size.should eql 1
       unread_guy.conversations.first.remove_messages(:all)
@@ -155,7 +156,7 @@
       unread_guy = user
       root_convo = Conversation.initiate([sender.id, unread_guy.id], false)
       root_convo.add_message(sender, 'test')
-      
+
       unread_guy.reload.unread_conversations_count.should eql 1
       unread_guy.conversations.unread.size.should eql 1
       unread_guy.conversations.first.mark_as_read
@@ -169,7 +170,7 @@
       root_convo = Conversation.initiate([sender.id, unread_guy.id], false)
       root_convo.add_message(sender, 'test')
       unread_guy.conversations.first.mark_as_read
-      
+
       unread_guy.reload.unread_conversations_count.should eql 0
       unread_guy.conversations.unread.size.should eql 0
       unread_guy.conversations.first.mark_as_unread
@@ -178,6 +179,81 @@
     end
   end
 
+  context "subscription" do
+    it "should mark-as-read when unsubscribing iff it was unread" do
+      sender = user
+      subscription_guy = user
+      archive_guy = user
+      root_convo = Conversation.initiate([sender.id, archive_guy.id, subscription_guy.id], false)
+      root_convo.add_message(sender, 'test')
+
+      subscription_guy.reload.unread_conversations_count.should eql 1
+      subscription_guy.conversations.unread.size.should eql 1
+
+      subscription_guy.conversations.first.update_attributes(:subscribed => false)
+      subscription_guy.reload.unread_conversations_count.should eql 0
+      subscription_guy.conversations.unread.size.should eql 0
+
+      archive_guy.conversations.first.archive!
+      archive_guy.conversations.first.update_attributes(:subscribed => false)
+      archive_guy.conversations.archived.size.should eql 1
+    end
+
+    it "should mark-as-unread when re-subscribing iff there are newer messages" do
+      sender = user
+      flip_flopper_guy = user
+      subscription_guy = user
+      archive_guy = user
+      root_convo = Conversation.initiate([sender.id, flip_flopper_guy.id, archive_guy.id, subscription_guy.id], false)
+      root_convo.add_message(sender, 'test')
+
+      flip_flopper_guy.conversations.first.update_attributes(:subscribed => false)
+      flip_flopper_guy.reload.unread_conversations_count.should eql 0
+      flip_flopper_guy.conversations.unread.size.should eql 0
+      # no new messages in the interim, he should stay "marked-as-read"
+      flip_flopper_guy.conversations.first.update_attributes(:subscribed => true)
+      flip_flopper_guy.reload.unread_conversations_count.should eql 0
+      flip_flopper_guy.conversations.unread.size.should eql 0
+
+      subscription_guy.conversations.first.update_attributes(:subscribed => false)
+      archive_guy.conversations.first.archive!
+      archive_guy.conversations.first.update_attributes(:subscribed => false)
+
+      message = root_convo.add_message(sender, 'you wish you were subscribed!')
+      message.update_attribute(:created_at, Time.now.utc + 1.minute)
+      last_message_at = message.reload.created_at
+
+      subscription_guy.conversations.first.update_attributes(:subscribed => true)
+      archive_guy.conversations.first.update_attributes(:subscribed => true)
+
+      subscription_guy.reload.unread_conversations_count.should eql 1
+      subscription_guy.conversations.unread.size.should eql 1
+      subscription_guy.conversations.first.last_message_at.should eql last_message_at
+
+      archive_guy.reload.unread_conversations_count.should eql 1
+      archive_guy.conversations.unread.size.should eql 1
+      subscription_guy.conversations.first.last_message_at.should eql last_message_at
+    end
+
+    it "should not toggle read/unread until the subscription change is saved" do
+      sender = user
+      subscription_guy = user
+      root_convo = Conversation.initiate([sender.id, user.id, subscription_guy.id], false)
+      root_convo.add_message(sender, 'test')
+
+      subscription_guy.reload.unread_conversations_count.should eql 1
+      subscription_guy.conversations.unread.size.should eql 1
+
+      subscription_guy.conversations.first.subscribed = false
+      subscription_guy.reload.unread_conversations_count.should eql 1
+      subscription_guy.conversations.unread.size.should eql 1
+
+      subscription_guy.conversations.first.subscribed = true
+      subscription_guy.reload.unread_conversations_count.should eql 1
+      subscription_guy.conversations.unread.size.should eql 1
+    end
+  end
+
   context "adding messages" do
     it "should deliver the message to all participants" do
       sender = user
@@ -195,14 +271,30 @@
       end
     end
 
-    it "should not auto mark it as read for the sender" do
+    it "should never change the workflow_state for the sender" do
       sender = user
-      recipients = 5.times.map{ user }
-      Conversation.initiate([sender.id] + recipients.map(&:id), false).add_message(sender, 'test')
+      Conversation.initiate([sender.id, user.id], true).add_message(sender, 'test')
       convo = sender.conversations.first
       convo.mark_as_unread!
       convo.add_message('another test')
       convo.reload.unread?.should be_true
+
+      convo.archive!
+      convo.add_message('one more test')
+      convo.reload.archived?.should be_true
+
+      convo.unarchive!
+      convo.mark_as_unread!
+      convo.add_message('and another test', :update_for_sender => true) # overrides subscribed-ness and updates timestamps
+      convo.reload.unread?.should be_true
+
+      convo.archive!
+      convo.add_message('last one', :update_for_sender => true)
+      convo.reload.archived?.should be_true
+
+      convo.remove_messages(:all)
+      convo.add_message('for reals', :update_for_sender => true)
+      convo.reload.archived?.should be_true
     end
 
     it "should deliver the message to unsubscribed participants but not alert them" do
@@ -217,7 +309,8 @@
       rconvo.unread?.should be_false
 
       convo = sender.conversations.first
-      convo.add_message('another test')
+      message = convo.add_message('another test')
+      message.update_attribute(:created_at, Time.now.utc + 1.minute)
 
       rconvo.reload.unread?.should be_false
       rconvo.update_attributes(:subscribed => true)