fix conversation merge issue, fixes #7801

fix an issue where private conversations were improperly merged when
merging user accounts (only a problem when it was being merged into an
existing conversation on the target user). this would cause page errors
due to orphaned conversation_participants.

includes a data migration to fix affected conversations.

test plan:
1. on the old code, create three users (A, B, C)
2. start a private conversation between A and B
3. start a private conversation between A and C
4. merge user B into user C
5. confirm that users A and C get a page error when going to conversations
6. upgrade the code and run the migration
7. confirm that conversations load and the merged conversation appears
   correctly
8. on the new code, create three more users, repeat steps 2-4, and confirm
   that conversations load and the merged conversation appears correctly

Change-Id: I50785764ae4376a38824ffa099ff678686ec5023
Reviewed-on: https://gerrit.instructure.com/9847
Tested-by: Hudson <hudson@instructure.com>
Reviewed-by: Cody Cutrer <cody@instructure.com>

app/models/conversation_participant.rb

@@ -266,14 +266,16 @@ class ConversationParticipant < ActiveRecord::Base
   end
 
   def move_to_user(new_user)
-    conversation.conversation_messages.update_all(["author_id = ?", new_user.id], ["author_id = ?", user_id])
+    self.class.send :with_exclusive_scope do
-    if existing = conversation.conversation_participants.find_by_user_id(new_user.id)
+      conversation.conversation_messages.update_all(["author_id = ?", new_user.id], ["author_id = ?", user_id])
-      existing.update_attribute(:workflow_state, workflow_state) if unread? || existing.archived?
+      if existing = conversation.conversation_participants.find_by_user_id(new_user.id)
-      destroy
+        existing.update_attribute(:workflow_state, workflow_state) if unread? || existing.archived?
-    else
+        destroy
-      update_attribute :user_id, new_user.id
+      else
+        update_attribute :user_id, new_user.id
+      end
+      conversation.regenerate_private_hash! if private?
     end
-    conversation.regenerate_private_hash! if private?
   end
 
   attr_writer :last_message

db/migrate/20120404230916_fix_user_merge_conversations2.rb

@@ -0,0 +1,38 @@
+class FixUserMergeConversations2 < ActiveRecord::Migration
+  tag :postdeploy
+
+  def self.up
+    # basically we are re-running lines 408-410 and 417-419 of
+    # Conversation#merge_into (plus replicating some surrounding setup logic)
+    # for any private conversations that were merged into existing private
+    # conversations since 57d3a82.
+    # the previous merging was done incorrectly due to a scoping issue
+    
+    # there are only about 100 that need to be fixed, so we just load them all
+    convos = ConversationParticipant.find(:all, :conditions => "NOT EXISTS (SELECT 1 FROM conversations WHERE id = conversation_id)")
+    convos.group_by(&:conversation_id).each do |conversation_id, cps|
+      private_hash = Conversation.private_hash_for(cps.map(&:user_id))
+      if target = Conversation.find_by_private_hash(private_hash)
+        new_participants = target.conversation_participants.inject({}){ |h,p| h[p.user_id] = p; h }
+        cps.each do |cp|
+          if new_cp = new_participants[cp.user_id]
+            new_cp.update_attribute(:workflow_state, cp.workflow_state) if cp.unread? || new_cp.archived?
+            cp.conversation_message_participants.update_all(["conversation_participant_id = ?", new_cp.id])
+            cp.destroy
+          else
+            $stderr.puts "couldn't find a target ConversationParticipant for id #{cp.id}"
+          end
+        end
+        target.conversation_participants(true).each do |cp|
+          cp.update_cached_data! :recalculate_count => true, :set_last_message_at => false, :regenerate_tags => false
+        end
+      else
+        $stderr.puts "couldn't find a target Conversation for hash #{private_hash}"
+      end
+    end
+  end
+
+  def self.down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end

spec/migrations/fix_user_merge_conversations2.rb

@@ -0,0 +1,77 @@
+#
+# Copyright (C) 2011 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')
+require 'db/migrate/20120404230916_fix_user_merge_conversations2.rb'
+
+describe FixUserMergeConversations2 do
+  describe "up" do
+    it "should work" do
+      u1 = user
+      u2 = user
+      u3 = user
+
+      # set up borked conversation that is partially merged...
+      # conversation deleted, cp's and cmps orphaned,
+      # and cm on the target conversation
+      borked = Conversation.initiate([u1.id, u2.id], true)
+      borked_cps = borked.conversation_participants.all
+      borked_cmps = borked_cps.map(&:conversation_message_participants).flatten
+      m1 = borked.add_message(u1, "test")
+      Conversation.delete_all(:id => borked.id) # bypass callbacks
+
+      correct = Conversation.initiate([u1.id, u2.id], true)
+      m2 = correct.add_message(u1, "test2")
+      correct.conversation_participants.each { |cp| cp.update_attribute :workflow_state, 'archived'}
+
+      # put it the message on the correct conversation
+      m1.update_attribute :conversation_id, correct.id
+
+      unrelated = Conversation.initiate([u1.id, u3.id], true)
+      unrelated.add_message(u1, "test3")
+
+      FixUserMergeConversations2.up
+
+      correct.reload
+      unrelated.reload
+
+      # these are gone for reals now
+      lambda { borked.reload }.should raise_error
+      borked_cps.each { |cp| lambda { cp.reload }.should raise_error }
+
+      # these are moved to the right place
+      borked_cmps.each do |cmp|
+        lambda { cmp.reload }.should_not raise_error
+        cmp.conversation_participant.conversation.should eql correct
+      end
+      correct.conversation_participants.each do |cp|
+        cp.messages.size.should eql 2
+        cp.message_count.should eql 2
+      end
+      # got bumped out of archived by the merged/deleted ones
+      correct.conversation_participants.default.size.should eql 2
+      correct.conversation_participants.unread.size.should eql 1
+
+      # no changes here
+      unrelated.conversation_participants.each do |cp|
+        cp.messages.size.should eql 1
+        cp.message_count.should eql 1
+      end
+    end
+  end
+end

spec/models/conversation_participant_spec.rb

@@ -338,5 +338,31 @@ describe ConversationParticipant do
       @user1.reload.unread_conversations_count.should eql 0
       @user2.reload.unread_conversations_count.should eql 1
     end
+
+    it "should not be adversely affected by an outer scope" do
+      other_guy = user
+      c = @user1.initiate_conversation([other_guy.id])
+      c.add_message("hello")
+      c.update_attribute(:workflow_state, 'unread')
+      c2 = @user2.initiate_conversation([other_guy.id])
+      c2.add_message("hola")
+
+      c.reload
+      ConversationParticipant.send :with_scope, :find => {:conditions => ["user_id = ?", @user1.id]} do
+        c.move_to_user @user2
+      end
+  
+      lambda{ c.reload }.should raise_error # deleted
+      lambda{ Conversation.find(c.conversation_id) }.should raise_error # deleted
+
+      c2.reload.messages.size.should eql 2
+      c2.messages.map(&:author_id).should eql [@user2.id, @user2.id]
+      c2.message_count.should eql 2
+      c2.user_id.should eql @user2.id
+      c2.conversation.participants.size.should eql 2
+      @user1.reload.unread_conversations_count.should eql 0
+      @user2.reload.unread_conversations_count.should eql 1
+      other_guy.reload.unread_conversations_count.should eql 1
+    end
   end
 end