clean up course account associations

closes CNVS-3417

test plan:
 * after the migrations are run, ensure that every section has at
   least one entry in CourseAccountAssociations in the database
 * smoke test SIS imports

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

Author: ccutrer

app/models/course.rb

@@ -1,5 +1,5 @@
 #
-# Copyright (C) 2011 - 2012 Instructure, Inc.
+# Copyright (C) 2011 - 2013 Instructure, Inc.
 #
 # This file is part of Canvas.
 #
@@ -334,16 +334,17 @@ def self.update_account_associations(courses_or_course_ids, opts = {})
         course_ids = courses.map(&:id)
       else
         course_ids = courses_or_course_ids
-        courses = Course.find(:all, :conditions => {:id => course_ids }, :include => { :course_sections => :nonxlist_course })
+        courses = Course.find(:all, :conditions => {:id => course_ids }, :include => { :course_sections => [:course, :nonxlist_course] })
       end
       course_ids_to_update_user_account_associations = []
       CourseAccountAssociation.transaction do
         current_associations = {}
         to_delete = []
         CourseAccountAssociation.find(:all, :conditions => { :course_id => course_ids }).each do |aa|
           key = [aa.course_section_id, aa.account_id]
-          # duplicates
           current_course_associations = current_associations[aa.course_id] ||= {}
+          # duplicates. the unique index prevents these now, but this code
+          # needs to hang around for the migration itself
           if current_course_associations.has_key?(key)
             to_delete << aa.id
             next
@@ -359,7 +360,7 @@ def self.update_account_associations(courses_or_course_ids, opts = {})
           (course.course_sections + [nil]).each do |section|
             next if section && !section.active?
             section.course = course if section
-            starting_account_ids = [course.account_id, section.try(:account_id), section.try(:nonxlist_course).try(:account_id)].compact.uniq
+            starting_account_ids = [course.account_id, section.try(:course).try(:account_id), section.try(:nonxlist_course).try(:account_id)].compact.uniq
 
             account_ids_with_depth = User.calculate_account_associations_from_accounts(starting_account_ids, account_chain_cache).map
 
@@ -1652,7 +1653,7 @@ def default_section(opts = {})
       section = course_sections.build
       section.default_section = true
       section.course = self
-      section.root_account = self.root_account
+      section.root_account_id = self.root_account_id
       section.save unless new_record?
     end
     section

app/models/course_section.rb

@@ -1,5 +1,5 @@
 #
-# Copyright (C) 2011 Instructure, Inc.
+# Copyright (C) 2011 - 2013 Instructure, Inc.
 #
 # This file is part of Canvas.
 #
@@ -24,7 +24,6 @@ class CourseSection < ActiveRecord::Base
   belongs_to :course
   belongs_to :nonxlist_course, :class_name => 'Course'
   belongs_to :root_account, :class_name => 'Account'
-  belongs_to :account
   has_many :enrollments, :include => :user, :conditions => ['enrollments.workflow_state != ?', 'deleted'], :dependent => :destroy
   has_many :all_enrollments, :class_name => 'Enrollment'
   has_many :students, :through => :student_enrollments, :source => :user
@@ -98,17 +97,19 @@ def touch_all_enrollments
   end
 
   def set_update_account_associations_if_changed
-    @should_update_account_associations = self.account_id_changed? || self.course_id_changed? || self.nonxlist_course_id_changed?
-    @should_update_account_associations = false if self.new_record? && self.account_id.nil?
+    @should_update_account_associations = self.course_id_changed? || self.nonxlist_course_id_changed?
     true
   end
 
   def update_account_associations_if_changed
-    send_later_if_production(:update_account_associations) if @should_update_account_associations && !Course.skip_updating_account_associations?
+    if @should_update_account_associations && !Course.skip_updating_account_associations?
+      Course.send_later_if_production(:update_account_associations,
+                                      [self.course_id, self.course_id_was, self.nonxlist_course_id, self.nonxlist_course_id_was].compact.uniq)
+    end
   end
 
   def update_account_associations
-    Course.update_account_associations([self.course, self.nonxlist_course].compact)
+    Course.update_account_associations([self.course_id, self.nonxlist_course_id].compact)
   end
 
   def verify_unique_sis_source_id
@@ -128,7 +129,6 @@ def section_code
 
   def infer_defaults
     self.root_account_id ||= (self.course.root_account_id rescue nil) || Account.default.id
-    self.assert_course unless self.course
     raise "Course required" unless self.course
     self.root_account_id = self.course.root_account_id || Account.default.id
     # This is messy, and I hate it.
@@ -160,10 +160,6 @@ def display_name
     @section_display_name ||= self.name || self.section_code
   end
 
-  def assert_course
-    self.course ||= Course.create!(:name => self.name || self.section_code, :root_account => self.root_account)
-  end
-
   def move_to_course(course, *opts)
     return self if self.course_id == course.id
     old_course = self.course

app/models/enrollment.rb

@@ -1,5 +1,5 @@
 #
-# Copyright (C) 2011 - 2012 Instructure, Inc.
+# Copyright (C) 2011 - 2013 Instructure, Inc.
 #
 # This file is part of Canvas.
 #
@@ -430,7 +430,7 @@ def associated_user_name
 
   def assert_section
     self.course_section ||= self.course.default_section if self.course
-    self.root_account_id = self.course_section.root_account_id rescue nil
+    self.root_account_id = self.course.root_account_id rescue nil
   end
 
   def infer_privileges

config/initializers/dropped_columns.rb

@@ -1,5 +1,5 @@
 #
-# Copyright (C) 2011 Instructure, Inc.
+# Copyright (C) 2011 - 2013 Instructure, Inc.
 #
 # This file is part of Canvas.
 #
@@ -28,7 +28,7 @@ class ActiveRecord::Base
     'calendar_events' => %w(calendar_event_repeat_id for_repeat_on),
     'content_tags' => %w(sequence_position context_module_association_id),
     'conversation_messages' => %w(context_message_id),
-    'course_sections' => %w(sis_cross_listed_section_id sis_cross_listed_section_sis_batch_id sticky_xlist sis_name students_can_participate_before_start_at section_organization_name long_section_code),
+    'course_sections' => %w(sis_cross_listed_section_id sis_cross_listed_section_sis_batch_id sticky_xlist sis_name students_can_participate_before_start_at section_organization_name long_section_code account_id),
     'courses' => %w(section hidden_tabs sis_name sis_course_code hashtag allow_student_assignment_edits),
     'discussion_topics' => %w(authorization_list_id),
     'enrollment_terms' => %w(sis_data sis_name),

db/migrate/20130128220410_drop_account_id_from_course_sections.rb

@@ -0,0 +1,11 @@
+class DropAccountIdFromCourseSections < ActiveRecord::Migration
+  tag :postdeploy
+
+  def self.up
+    remove_column :course_sections, :account_id
+  end
+
+  def self.down
+    add_column :course_sections, :account_id, :integer, :limit => 8
+  end
+end

db/migrate/20130128221236_add_unique_index_on_course_account_associations.rb

@@ -0,0 +1,19 @@
+class AddUniqueIndexOnCourseAccountAssociations < ActiveRecord::Migration
+  tag :predeploy
+  self.transactional = false
+
+  def self.up
+    # clean up any dups first
+    course_ids = CourseAccountAssociation.find(:all, :select => 'DISTINCT course_id',
+      :group => 'course_id, course_section_id, account_id', :having => 'COUNT(*) > 1').map(&:course_id)
+    Course.update_account_associations(course_ids)
+
+    add_index :course_account_associations, [:course_id, :course_section_id, :account_id], :unique => true, :concurrently => true, :name => 'index_caa_on_course_id_and_section_id_and_account_id'
+    remove_index :course_account_associations, :course_id
+  end
+
+  def self.down
+    add_index :course_account_associations, :course_id, :concurrently => true
+    remove_index :course_account_associations, 'index_caa_on_course_id_and_section_id_and_account_id'
+  end
+end

db/migrate/20130128221237_recalculate_course_account_associations.rb

@@ -0,0 +1,8 @@
+class RecalculateCourseAccountAssociations < ActiveRecord::Migration
+  tag :postdeploy
+
+  def self.up
+    # a bug was fixed in Course.update_account_associations; we need to recalculate them all
+    DataFixup::RecalculateCourseAccountAssociations.send_later_if_production(:run)
+  end
+end

doc/api/sis_csv.md

@@ -310,12 +310,6 @@ interface, this is called the SIS ID.</td>
 <td>date</td>
 <td>The section end date The format should be in ISO 8601: YYYY-MM-DDTHH:MM:SSZ</td>
 </tr>
-<tr>
-<td>account_id</td>
-<td>text</td>
-<td>The account identifier from accounts.csv, if none is specified the course will be attached
-to the root account</td>
-</tr>
 </table>
 
 <p>If the start_date is set, it will override the course and term start dates. If the end_date is

lib/data_fixup/recalculate_course_account_associations.rb

@@ -0,0 +1,9 @@
+module DataFixup::RecalculateCourseAccountAssociations
+  def self.run
+    Course.active.scoped(:joins => :root_account, :conditions => "accounts.workflow_state<>'deleted'").find_in_batches do |batch|
+      Course.send(:with_exclusive_scope) do
+        Course.update_account_associations(batch)
+      end
+    end
+  end
+end

lib/sis/csv/section_importer.rb

@@ -1,5 +1,5 @@
 #
-# Copyright (C) 2011 Instructure, Inc.
+# Copyright (C) 2011 - 2013 Instructure, Inc.
 #
 # This file is part of Canvas.
 #
@@ -44,7 +44,7 @@ def process(csv)
             end
 
             begin
-              importer.add_section(row['section_id'], row['course_id'], row['name'], row['status'], start_date, end_date, row['account_id'])
+              importer.add_section(row['section_id'], row['course_id'], row['name'], row['status'], start_date, end_date)
             rescue ImportError => e
               add_warning(csv, "#{e}")
             end