diff --git a/app/models/conversation_participant.rb b/app/models/conversation_participant.rb index b0f2d94e95e..05f66b67fdc 100644 --- a/app/models/conversation_participant.rb +++ b/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]) - if existing = conversation.conversation_participants.find_by_user_id(new_user.id) - existing.update_attribute(:workflow_state, workflow_state) if unread? || existing.archived? - destroy - else - update_attribute :user_id, new_user.id + self.class.send :with_exclusive_scope do + conversation.conversation_messages.update_all(["author_id = ?", new_user.id], ["author_id = ?", user_id]) + if existing = conversation.conversation_participants.find_by_user_id(new_user.id) + existing.update_attribute(:workflow_state, workflow_state) if unread? || existing.archived? + destroy + 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 diff --git a/db/migrate/20120404230916_fix_user_merge_conversations2.rb b/db/migrate/20120404230916_fix_user_merge_conversations2.rb new file mode 100644 index 00000000000..312652a7184 --- /dev/null +++ b/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 diff --git a/spec/migrations/fix_user_merge_conversations2.rb b/spec/migrations/fix_user_merge_conversations2.rb new file mode 100644 index 00000000000..03bbfe74e28 --- /dev/null +++ b/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 . +# + +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 diff --git a/spec/models/conversation_participant_spec.rb b/spec/models/conversation_participant_spec.rb index 92675f808f2..ab9b1e36464 100644 --- a/spec/models/conversation_participant_spec.rb +++ b/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