clean up user account associations

refs CNVS-1338

test plan:
 * run the migration on production-like data
 * it should not fail

Change-Id: Ib8527f2cbee05d5e00941286f16c245091b1a2d9
Reviewed-on: https://gerrit.instructure.com/17117
Tested-by: Jenkins <jenkins@instructure.com>
QA-Review: Clare Hetherington <clare@instructure.com>
Reviewed-by: Jacob Fugal <jacob@instructure.com>

app/models/course_section.rb

@@ -109,8 +109,6 @@ class CourseSection < ActiveRecord::Base
 
   def update_account_associations
     Course.update_account_associations([self.course, self.nonxlist_course].compact)
-    self.course.try(:update_account_associations)
-    self.nonxlist_course.try(:update_account_associations)
   end
 
   def verify_unique_sis_source_id

app/models/user.rb

@@ -415,7 +415,8 @@ class User < ActiveRecord::Base
       to_delete = []
       UserAccountAssociation.find(:all, :conditions => { :user_id => user_ids }).each do |aa|
         key = [aa.user_id, aa.account_id]
-        # duplicates
+        # duplicates. the unique index prevents these now, but this code
+        # needs to hang around for the migration itself
         if current_associations.has_key?(key)
           to_delete << aa.id
           next

app/models/user_account_association.rb

@@ -20,5 +20,7 @@ class UserAccountAssociation < ActiveRecord::Base
   belongs_to :user
   belongs_to :account
 
+  validates_presence_of :user_id, :account_id
+
   attr_accessible :account_id, :depth
 end

db/migrate/20111006220252_update_account_associations_for_users_with_duplicates.rb

@@ -1,19 +0,0 @@
-class UpdateAccountAssociationsForUsersWithDuplicates < ActiveRecord::Migration
-  def self.up
-    # Do this in batches so that we don't try to load too many users into memory at once.
-    begin
-      loop do
-        users = User.find_by_sql("select u.* from (select user_id, account_id, updated_at, count(*) from user_account_associations group by user_id, account_id, updated_at having count(*) > 1 order by updated_at desc) foo, users u where foo.user_id = u.id limit 1000")
-        break if users.length == 0
-        
-        User.update_account_associations(users)
-      end
-    rescue => e
-      puts " !! Exception encountered updating account associations for users with duplicates: #{e.inspect}"
-      puts " !! This is a non-fatal error and migrations will continue, but it should be investigated."
-    end
-  end
-
-  def self.down
-  end
-end

db/migrate/20130124203149_clean_up_user_account_associations.rb

@@ -0,0 +1,26 @@
+class CleanUpUserAccountAssociations < ActiveRecord::Migration
+  tag :predeploy
+  self.transactional = false
+
+  def self.up
+    # clean up garbage data
+    UserAccountAssociation.delete_all(:user_id => nil)
+    # we don't have any of these in production, but just in case...
+    UserAccountAssociation.delete_all(:account_id => nil)
+
+    # clean up dups by recalculating
+    user_ids = UserAccountAssociation.find(:all, :select => 'DISTINCT user_id',
+       :group => 'user_id, account_id', :having => 'COUNT(*) > 1').map(&:user_id)
+    User.update_account_associations(user_ids)
+
+    # add a unique index
+    add_index :user_account_associations, [:user_id, :account_id], :unique => true, :concurrently => true
+    # remove the non-unique index that's now covered by the unique index
+    remove_index :user_account_associations, :user_id
+  end
+
+  def self.down
+    add_index :user_account_associations, :user_id, :concurrently => true
+    remove_index :user_account_associations, [:user_id, :account_id]
+  end
+end

spec/apis/v1/conversations_api_spec.rb

@@ -39,7 +39,6 @@ describe ConversationsController, :type => :integration do
     enrollment = @course.enroll_user(u, 'StudentEnrollment', :section => section)
     enrollment.workflow_state = 'active'
     enrollment.save
-    u.associated_accounts << Account.default
     u
   end
 

spec/apis/v1/search_api_spec.rb

@@ -21,7 +21,6 @@ describe SearchController, :type => :integration do
     enrollment = @course.enroll_user(u, 'StudentEnrollment', :section => section)
     enrollment.workflow_state = 'active'
     enrollment.save
-    u.associated_accounts << Account.default
     u
   end
 
@@ -131,7 +130,6 @@ describe SearchController, :type => :integration do
         enrollment.associated_user = associated_user
         enrollment.workflow_state = 'active'
         enrollment.save
-        u.associated_accounts << Account.default
         u
       end
 

spec/controllers/conversations_controller_spec.rb

@@ -27,7 +27,6 @@ describe ConversationsController do
       enrollment = course.enroll_student(u)
       enrollment.workflow_state = 'active'
       enrollment.save
-      u.associated_accounts << Account.default
       u.id
     }
     @conversation = @user.initiate_conversation(user_ids)
@@ -135,7 +134,6 @@ describe ConversationsController do
         a = Account.default
         @student = user_with_pseudonym(:active_all => true)
         course_with_student(:active_all => true, :account => a, :user => @student)
-        @student.associated_accounts << a
         @student.initiate_conversation([user.id]).add_message('test1', :root_account_id => a.id)
         @student.initiate_conversation([user.id]).add_message('test2') # no root account, so teacher can't see it
   
@@ -303,7 +301,6 @@ describe ConversationsController do
     it "should generate a user note when requested" do
       Account.default.update_attribute :enable_user_notes, true
       course_with_teacher_logged_in(:active_all => true)
-      @teacher.associated_accounts << Account.default
       conversation
 
       post 'add_message', :conversation_id => @conversation.conversation_id, :body => "hello world"

spec/models/conversation_message_spec.rb

@@ -111,9 +111,7 @@ describe ConversationMessage do
     it "should add a user note under nominal circumstances" do
       Account.default.update_attribute :enable_user_notes, true
       course_with_teacher
-      @teacher.associated_accounts << Account.default
       student = student_in_course.user
-      student.associated_accounts << Account.default
       conversation = @teacher.initiate_conversation([student.id])
       ConversationMessage.any_instance.stubs(:current_time_from_proper_timezone).returns(Time.at(0))
       conversation.add_message("reprimanded!", :generate_user_note => true)
@@ -127,9 +125,7 @@ describe ConversationMessage do
     it "should fail if notes are disabled on the account" do
       Account.default.update_attribute :enable_user_notes, false
       course_with_teacher
-      @teacher.associated_accounts << Account.default
       student = student_in_course.user
-      student.associated_accounts << Account.default
       conversation = @teacher.initiate_conversation([student.id])
       conversation.add_message("reprimanded!", :generate_user_note => true)
       student.user_notes.size.should be(0)
@@ -138,11 +134,8 @@ describe ConversationMessage do
     it "should fail if there's more than one recipient" do
       Account.default.update_attribute :enable_user_notes, true
       course_with_teacher
-      @teacher.associated_accounts << Account.default
       student1 = student_in_course.user
-      student1.associated_accounts << Account.default
       student2 = student_in_course.user
-      student2.associated_accounts << Account.default
       conversation = @teacher.initiate_conversation([student1.id, student2.id])
       conversation.add_message("reprimanded!", :generate_user_note => true)
       student1.user_notes.size.should be(0)

spec/models/conversation_participant_spec.rb

@@ -161,7 +161,7 @@ describe ConversationParticipant do
       @admin_user = user
       @a1.add_user(@admin_user)
       @a2.add_user(@admin_user)
-      @admin_user.associated_accounts << @a3 # in the account, but not an admin
+      @a3.pseudonyms.create!(:user => @admin_user, :unique_id => 'a3') # in the account, but not an admin
 
       @target_user = user
       # visible to @user