migrate yaml in database into a psych compatible format

from now on dumps all yaml with psych and inserts a tag to
differentiate between syck and psych yaml on load

also runs through the database and converts all
incompatible yaml to psych format

after the migrations have run we will be able to switch
to psych fully and clean up the tags

test plan:
* the migrations should work

closes #CNVS-27229

Change-Id: I79ce0691dd455396ca39422051ff79b8fbaebef6
Reviewed-on: https://gerrit.instructure.com/72012
Reviewed-by: Cody Cutrer <cody@instructure.com>
QA-Review: Jeremy Putnam <jeremyp@instructure.com>
Tested-by: Jenkins
Product-Review: James Williams  <jamesw@instructure.com>

Gemfile.d/app.rb

@@ -39,7 +39,7 @@ gem 'bcrypt-ruby', '3.0.1'
 gem 'canvas_connect', '0.3.10'
   gem 'adobe_connect', '1.0.3', require: false
 gem 'canvas_webex', '0.17'
-gem 'canvas-jobs', '0.10.4'
+gem 'canvas-jobs', '0.10.5'
   gem 'rufus-scheduler', '3.1.2', require: false
 gem 'ffi', '1.1.5', require: false
 gem 'hairtrigger', '0.2.15'

app/models/external_feed.rb

@@ -103,7 +103,7 @@ class ExternalFeed < ActiveRecord::Base
       end
       if entry
         entry.update_feed_attributes(
-          :title => item.title,
+          :title => item.title.to_s,
           :message => description,
           :url => item.link
         )
@@ -115,7 +115,7 @@ class ExternalFeed < ActiveRecord::Base
       description = "<a href='#{ERB::Util.h(item.link)}'>#{ERB::Util.h(t(:original_article, "Original article"))}</a><br/><br/>"
       description += format_description(item.description || item.title)
       entry = self.external_feed_entries.new(
-        :title => item.title,
+        :title => item.title.to_s,
         :message => description,
         :source_name => feed.channel.title,
         :source_url => feed.channel.link,
@@ -136,7 +136,7 @@ class ExternalFeed < ActiveRecord::Base
       end
       if entry
         entry.update_feed_attributes(
-          :title => item.title,
+          :title => item.title.to_s,
           :message => description,
           :url => item.links.alternate.to_s,
           :author_name => author.name,
@@ -153,7 +153,7 @@ class ExternalFeed < ActiveRecord::Base
       entry = self.external_feed_entries.new(
         :title => item.title,
         :message => description,
-        :source_name => feed.title,
+        :source_name => feed.title.to_s,
         :source_url => feed.links.alternate.to_s,
         :posted_at => item.published,
         :url => item.links.alternate.to_s,

app/models/progress.rb

@@ -90,8 +90,11 @@ class Progress < ActiveRecord::Base
     end
 
     def perform
+      self.args[0] = @progress if self.args[0] == @progress # maintain the same object reference
       @progress.start
-      super.tap { @progress.complete }
+      super
+      @progress.reload
+      @progress.complete
     end
 
     def on_permanent_failure(error)

config/application.rb

@@ -159,6 +159,14 @@ module CanvasRails
       val.constantize
     end
 
+    # TODO: Use this instead of the above block when we switch to Psych
+    Psych.add_domain_type("ruby/object", "Class") do |_type, val|
+      if SafeYAML.safe_parsing && !Canvas::Migration.valid_converter_classes.include?(val)
+        raise "Cannot load class #{val} from YAML"
+      end
+      val.constantize
+    end
+
     # Extend any base classes, even gem classes
     Dir.glob("#{Rails.root}/lib/ext/**/*.rb").each { |file| require file }
 

config/initializers/active_record.rb

@@ -594,7 +594,11 @@ class ActiveRecord::Base
   def self.find_ids_in_ranges(options = {})
     batch_size = options[:batch_size].try(:to_i) || 1000
     subquery_scope = all.except(:select).select("#{quoted_table_name}.#{primary_key} as id").reorder(primary_key).limit(batch_size)
-    ids = connection.select_rows("select min(id), max(id) from (#{subquery_scope.to_sql}) as subquery").first
+    subquery_scope = subquery_scope.where("#{quoted_table_name}.#{primary_key} <= ?", options[:end_at]) if options[:end_at]
+
+    first_subquery_scope = options[:start_at] ? subquery_scope.where("#{quoted_table_name}.#{primary_key} >= ?", options[:start_at]) : subquery_scope
+    ids = connection.select_rows("select min(id), max(id) from (#{first_subquery_scope.to_sql}) as subquery").first
+
     while ids.first.present?
       ids.map!(&:to_i) if columns_hash[primary_key.to_s].type == :integer
       yield(*ids)

db/migrate/20160210153643_begin_psych_migration.rb

@@ -0,0 +1,10 @@
+class BeginPsychMigration < ActiveRecord::Migration
+  tag :postdeploy
+
+  def up
+    DataFixup::PsychMigration.run if CANVAS_RAILS4_0 || !Rails.env.test?
+  end
+
+  def down
+  end
+end

lib/canvas_yaml.rb

@@ -23,6 +23,7 @@
 # is switched to Syck. Otherwise we
 # won't have access to (safe|unsafe)_load.
 require 'yaml'
+
 require 'syck'
 YAML::ENGINE.yamler = 'syck' if defined?(YAML::ENGINE)
 
@@ -32,6 +33,7 @@ trusted_tags = SafeYAML::TRUSTED_TAGS.dup
 trusted_tags << 'tag:yaml.org,2002:merge'
 SafeYAML.send(:remove_const, :TRUSTED_TAGS)
 SafeYAML.const_set(:TRUSTED_TAGS, trusted_tags.freeze)
+
 module FixSafeYAMLNullMerge
   def merge_into_hash(hash, array)
     return unless array
@@ -65,5 +67,97 @@ SafeYAML::OPTIONS.merge!(
         tag:ruby.yaml.org,2002:object:URI::HTTPS
         tag:ruby.yaml.org,2002:object:OpenObject
         tag:yaml.org,2002:map:WeakParameters
+      ] + %w[
+        !ruby/symbol
+        !binary
+        !float
+        !float#exp
+        !float#inf
+        !str
+        !timestamp
+        !timestamp#iso8601
+        !timestamp#spaced
+        !map:HashWithIndifferentAccess
+        !map:ActiveSupport::HashWithIndifferentAccess
+        !map:WeakParameters
+        !ruby/hash:HashWithIndifferentAccess
+        !ruby/hash:ActiveSupport::HashWithIndifferentAccess
+        !ruby/hash:WeakParameters
+        !ruby/object:Class
+        !ruby/object:OpenStruct
+        !ruby/object:Scribd::Document
+        !ruby/object:Mime::Type
+        !ruby/object:URI::HTTP
+        !ruby/object:URI::HTTPS
+        !ruby/object:OpenObject
+        !ruby/object:DateTime
       ]
 )
+
+module Syckness
+  TAG = "#GETDOWNWITHTHESYCKNESS\n"
+
+  module SyckDumper
+    def dump(*args)
+      Psych.dump(*args)
+    end
+  end
+
+  module PsychDumper
+    def dump(*args)
+      yaml = super
+      yaml += TAG if yaml.is_a?(String)
+      yaml
+    end
+  end
+
+  module SafeLoader
+    require "safe_yaml/psych_resolver"
+    require "safe_yaml/safe_to_ruby_visitor"
+
+    def load(yaml, *args)
+      if yaml.is_a?(String) && yaml.end_with?(TAG)
+        SafeYAML::PsychResolver.new.resolve_node(Psych.parse(yaml))
+      else
+        super
+      end
+    end
+  end
+
+  module UnsafeLoader
+    def unsafe_load(yaml, *args)
+      if yaml.is_a?(String) && yaml.end_with?(TAG)
+        Psych.load(yaml)
+      else
+        super
+      end
+    end
+  end
+end
+
+[Object, Hash, Struct, Array, Exception, String, Symbol, Range, Regexp, Time,
+  Date, Integer, Float, Rational, Complex, TrueClass, FalseClass, NilClass].each do |klass|
+  klass.class_eval do
+    alias :to_yaml :psych_to_yaml
+  end
+end
+
+Syck.singleton_class.prepend(Syckness::SyckDumper)
+Psych.singleton_class.prepend(Syckness::PsychDumper)
+SafeYAML.singleton_class.prepend(Syckness::SafeLoader)
+YAML.singleton_class.prepend(Syckness::UnsafeLoader)
+
+SafeYAML::PsychResolver.class_eval do
+  attr_accessor :aliased_nodes
+end
+
+module MaintainAliases
+  def accept(node)
+    if node.respond_to?(:anchor) && node.anchor && @resolver.get_node_type(node) != :alias
+      @resolver.aliased_nodes[node.anchor] = node
+    end
+    super
+  end
+end
+SafeYAML::SafeToRubyVisitor.prepend(MaintainAliases)
+

lib/data_fixup/psych_migration.rb

@@ -0,0 +1,139 @@
+require "safe_yaml/psych_resolver"
+require "safe_yaml/safe_to_ruby_visitor"
+
+module DataFixup::PsychMigration
+  class << self
+    def run
+      raise "Rails 4.0 specific" unless CANVAS_RAILS4_0
+
+      columns_hash.each do |model, columns|
+        next if model.shard_category == :unsharded && Shard.current != Shard.default
+
+        if ranges = id_ranges(model)
+          ranges.each do |start_at, end_at|
+            queue_migration(model, columns, start_at, end_at)
+          end
+        else
+          queue_migration(model, columns)
+        end
+      end
+    end
+
+    def queue_migration(model, columns, start_at=nil, end_at=nil)
+      args = [model, columns, start_at, end_at]
+      if run_immediately?
+        self.migrate_yaml(nil, *args)
+      else
+        progress = Progress.create!(:context => Account.site_admin, :tag => "psych_migration")
+        progress.set_results({:model_name => model.name, :start_at => start_at, :end_at => end_at})
+        progress.process_job(self, :migrate_yaml, {:n_strand => ["psych_migration", Shard.current.database_server.id],
+          :priority => Delayed::MAX_PRIORITY, :max_attempts => 1}, *args)
+      end
+    end
+
+    def run_immediately?
+      !Rails.env.production?
+    end
+
+    def migrate_yaml(progress, model, columns, start_at, end_at)
+      total_count = 0
+      unparsable_count = 0
+      changed_count = 0
+
+      use_shard_id = (model <= Delayed::Backend::ActiveRecord::Job) && model.column_names.include?('shard_id')
+
+      model.find_ids_in_ranges(:batch_size => 50, :start_at => start_at, :end_at => end_at) do |min_id, max_id|
+        # rename the columns on the pluck so that Rails doesn't silently deserialize them for us
+        to_pluck = columns.map { |c| "#{c} AS #{c}1" }
+        to_pluck << :shard_id if use_shard_id
+
+        model.transaction do
+          rows = model.shard(Shard.current).where(model.primary_key => min_id..max_id).lock(:no_key_update).pluck(model.primary_key, *to_pluck)
+          rows.each do |row|
+            changes = {}
+
+            shard = (use_shard_id && Shard.lookup(row.pop)) || Shard.current
+            shard.activate do
+              columns.each_with_index do |column, i|
+                value = row[i + 1]
+                next if value.nil? || value.end_with?(Syckness::TAG)
+
+                obj_from_syck = begin
+                  YAML.unsafe_load(value)
+                rescue
+                  unparsable_count += 1
+                  nil
+                end
+
+                if obj_from_syck
+                  obj_from_psych = Psych.load(value) rescue nil
+                  if obj_from_syck != obj_from_psych
+                    Utf8Cleaner.recursively_strip_invalid_utf8!(obj_from_syck, true)
+                    changes[column] = YAML.dump(obj_from_syck)
+                  end
+                end
+              end
+            end
+
+            next if changes.empty?
+
+            model.where(model.primary_key => row.first).update_all(changes)
+            changed_count += 1
+          end
+
+          total_count += rows.count
+        end
+      end
+      if progress
+        progress.set_results(progress.results.merge(:successful => true, :total_count => total_count,
+          :changed_count => changed_count, :unparsable_count => unparsable_count))
+      end
+    end
+
+    def columns_hash
+      result = ActiveRecord::Base.all_models.map do |model|
+        next unless model.superclass == ActiveRecord::Base
+
+        attributes = model.serialized_attributes.select do |attr, coder|
+          coder.is_a?(ActiveRecord::Coders::YAMLColumn)
+        end
+        next if attributes.empty?
+        [model, attributes.keys]
+      end.compact.to_h
+      result[Version] = ['yaml']
+      result[Delayed::Backend::ActiveRecord::Job] = ['handler']
+      result[Delayed::Backend::ActiveRecord::Job::Failed] = ['handler']
+      result
+    end
+
+    def range_size
+      1_000_000
+    end
+
+    def id_ranges(model)
+      # try to partition off ranges of ids in the table with at most 1,000,000 ids per partition
+      return unless model.primary_key == "id"
+
+      ranges = []
+      scope = model.shard(Shard.current).where("id < ?", Shard::IDS_PER_SHARD)
+      start_id = scope.minimum(:id)
+      return unless start_id
+
+      current_min = start_id
+
+      while current_min
+        current_max = current_min + range_size - 1
+
+        next_min = scope.where("id > ?", current_max).minimum(:id)
+        if next_min
+          ranges << [current_min, current_max]
+        elsif !next_min && ranges.any?
+          ranges << [current_min, nil] # grab all the rest of the rows if we're at the end - including shadow objects
+        end
+        current_min = next_min
+      end
+
+      ranges if ranges.any?
+    end
+  end
+end

spec/lib/safe_yaml_spec.rb

@@ -19,25 +19,7 @@
 require File.expand_path(File.dirname(__FILE__) + '/../spec_helper.rb')
 
 describe "safe_yaml" do
-  it "should be used by default" do
+  let(:test_yaml) {
-    yaml = <<-YAML
---- !ruby/object:ActionController::Base
-real_format:
-YAML
-    expect { YAML.load yaml }.to raise_error
-    result = YAML.unsafe_load yaml
-    expect(result.class).to eq ActionController::Base
-  end
-
-  it "doesn't allow deserialization of arbitrary classes" do
-    expect { YAML.load(YAML.dump(ActionController::Base)) }.to raise_error
-  end
-
-  it "allows deserialization of arbitrary classes when unsafe_loading" do
-    expect(YAML.unsafe_load(YAML.dump(ActionController::Base))).to eq ActionController::Base
-  end
-
-  it "should allow some whitelisted classes" do
     yaml = <<-YAML
 ---
 hwia: !map:HashWithIndifferentAccess
@@ -92,8 +74,29 @@ verbose_symbol: !ruby/symbol blah
 oo: !ruby/object:OpenObject
   table:
     :a: 1
+    YAML
+  }
+
+  it "should be used by default" do
+    yaml = <<-YAML
+--- !ruby/object:ActionController::Base
+real_format:
 YAML
-    result = YAML.load yaml
+    expect { YAML.load yaml }.to raise_error
+    result = YAML.unsafe_load yaml
+    expect(result.class).to eq ActionController::Base
+  end
+
+  it "doesn't allow deserialization of arbitrary classes" do
+    expect { YAML.load(YAML.dump(ActionController::Base)) }.to raise_error
+  end
+
+  it "allows deserialization of arbitrary classes when unsafe_loading" do
+    expect(YAML.unsafe_load(YAML.dump(ActionController::Base))).to eq ActionController::Base
+  end
+
+  it "should allow some whitelisted classes" do
+    result = YAML.load(test_yaml)
 
     def verify(result, key, klass)
       obj = result[key]
@@ -139,4 +142,30 @@ YAML
     oo = verify(result, 'oo', OpenObject)
     expect(oo.a).to eq 1
   end
+
+  it "should allow some whitelisted classes through psych" do
+    old_result = YAML.load(test_yaml)
+    psych_yaml = YAML.dump(old_result)
+    expect(Psych.load(psych_yaml)).to eq old_result
+    expect(YAML.load(psych_yaml)).to eq old_result
+  end
+
+  it "should seamlessly dump yaml into a psych-compatible format (and be cross-compatible)" do
+    yaml = "--- \nsadness: \"\\xF0\\x9F\\x98\\x82\"\n"
+    hash = YAML.load(yaml)
+
+    psych_dump = "---\nsadness: \"\\U0001F602\"\n#{Syckness::TAG}"
+    expect(hash.to_yaml).to eq psych_dump
+    expect(YAML.dump(hash)).to eq psych_dump
+
+    expect(YAML.load(psych_dump)).to eq hash
+    expect(YAML.unsafe_load(psych_dump)).to eq hash
+  end
+
+  it "should work with aliases" do
+    hash = {:a => 1}.with_indifferent_access
+    obj = {:blah => hash, :bloop => hash}.with_indifferent_access
+    yaml = Psych.dump(obj)
+    expect(YAML.load(yaml)).to eq obj
+  end
 end

spec/migrations/fix_imported_question_media_comments_spec.rb

@@ -2,6 +2,8 @@ require File.expand_path(File.dirname(__FILE__) + '/../spec_helper.rb')
 
 describe DataFixup::FixImportedQuestionMediaComments do
   it 'should fix broken yaml in questions and quizzes' do
+    skip("Rails 4.0 specific") unless CANVAS_RAILS4_0
+
     course
     placeholder = "SOMETEXT"
     bank = @course.assessment_question_banks.create!(:title => 'bank')
@@ -25,12 +27,9 @@ describe DataFixup::FixImportedQuestionMediaComments do
     #just in case someone tries to run this spec in the past
     updated_at = ActiveRecord::Base.connection.quote(DateTime.parse('2015-10-16'))
     # deliberately create broken yaml
-    ActiveRecord::Base.connection.execute("UPDATE #{AssessmentQuestion.quoted_table_name} SET updated_at = #{updated_at},
+    AssessmentQuestion.where(:id => aq).update_all(:question_data => aq['question_data'].to_yaml.gsub(placeholder, broken_link), :updated_at => updated_at)
-      question_data = '#{aq['question_data'].to_yaml.gsub(placeholder, broken_link)}' WHERE id = #{aq.id}")
+    Quizzes::QuizQuestion.where(:id => qq).update_all(:question_data => qq['question_data'].to_yaml.gsub(placeholder, broken_link), :updated_at => updated_at)
-    ActiveRecord::Base.connection.execute("UPDATE #{Quizzes::QuizQuestion.quoted_table_name} SET updated_at = #{updated_at},
+    Quizzes::Quiz.where(:id => quiz).update_all(:quiz_data => quiz['quiz_data'].to_yaml.gsub(placeholder, broken_link), :updated_at => updated_at)
-      question_data = '#{qq['question_data'].to_yaml.gsub(placeholder, broken_link)}' WHERE id = #{qq.id}")
-    ActiveRecord::Base.connection.execute("UPDATE #{Quizzes::Quiz.quoted_table_name} SET updated_at = #{updated_at},
-      quiz_data = '#{quiz['quiz_data'].to_yaml.gsub(placeholder, broken_link)}' WHERE id = #{quiz.id}")
 
     aq = AssessmentQuestion.where(:id => aq).first
     qq = Quizzes::QuizQuestion.where(:id => qq).first

spec/migrations/psych_migration_spec.rb

@@ -0,0 +1,128 @@
+require_relative '../sharding_spec_helper'
+
+describe DataFixup::PsychMigration do
+  before :each do
+    skip("Rails 4.0 specific") unless CANVAS_RAILS4_0
+  end
+
+  let(:bad_yaml) { "--- \nsadness: \"\\xF0\\x9F\\x98\\x82\"\n"}
+  let(:fixed_yaml) { "---\nsadness: \"\\U0001F602\"\n#{Syckness::TAG}" }
+
+  it "should translate yaml in serialized columns into a psych compatible state" do
+    user
+    User.where(:id => @user).update_all(:preferences => bad_yaml)
+
+    DataFixup::PsychMigration.run
+
+    yaml = User.where(:id => @user).pluck("preferences AS p1").first
+    expect(yaml).to eq fixed_yaml
+  end
+
+  it "should fix all the columns" do
+    course
+
+    Course.where(:id => @course).update_all(:tab_configuration => bad_yaml, :settings => bad_yaml)
+
+    DataFixup::PsychMigration.run
+
+    yamls = Course.where(:id => @course).pluck("tab_configuration AS c1, settings AS c2").first
+    expect(yamls).to eq [fixed_yaml, fixed_yaml]
+  end
+
+  it "should queue a job with a progress model on production" do
+    user
+    User.where(:id => @user).update_all(:preferences => bad_yaml)
+
+    DataFixup::PsychMigration.stubs(:run_immediately?).returns(false)
+    DataFixup::PsychMigration.run
+
+    expect(User.where(:id => @user).pluck("preferences AS p1").first).to eq bad_yaml # should not have run yet
+
+    progresses = Progress.where(:tag => 'psych_migration').to_a
+    expect(progresses.map{|prog| prog.results[:model_name]}).to match_array(DataFixup::PsychMigration.columns_hash.keys.map(&:name))
+
+    progress = progresses.detect{|prog| prog.results[:model_name] == "User"}
+    expect(progress).to be_queued
+
+    run_jobs
+
+    progresses.each do |prog|
+      prog.reload
+      expect(prog).to be_completed
+    end
+
+    expect(progress.results[:successful]).to be_truthy
+    expect(progress.results[:changed_count]).to eq 1
+
+    yaml = User.where(:id => @user).pluck("preferences AS p1").first
+    expect(yaml).to eq fixed_yaml
+  end
+
+  it "should split into multiple jobs with id ranges if needed" do
+    skip "needs AR jobs" unless Delayed::Job == Delayed::Backend::ActiveRecord::Job
+
+    users = []
+    8.times do
+      user
+      users << @user
+    end
+    User.where(:id => users).update_all(:preferences => bad_yaml)
+
+    DataFixup::PsychMigration.stubs(:run_immediately?).returns(false)
+    DataFixup::PsychMigration.stubs(:range_size).returns(3)
+    DataFixup::PsychMigration.run
+
+    user_progresses = Progress.where(:tag => 'psych_migration').to_a.select{|prog| prog.results[:model_name] == "User"}
+    expect(user_progresses.count).to eq 3
+    ranges = user_progresses.map{|prog| [prog[:results][:start_at], prog[:results][:end_at]]}
+    expect(ranges).to match_array [
+      [users[0].id, users[2].id],
+      [users[3].id, users[5].id],
+      [users[6].id, nil]
+    ]
+
+    prog = user_progresses.detect{|prog| prog[:results][:start_at] == users[3].id}
+    job = Delayed::Job.where("handler LIKE ?", "%ActiveRecord:Progress #{prog.id}\n%").first
+
+    run_job(job)
+    prog.reload
+    expect(prog).to be_completed
+    expect(prog.results[:changed_count]).to eq 3
+
+    fixed_users = [users[3], users[4], users[5]]
+    expect(User.where(:id => fixed_users).pluck("preferences AS p1")).to eq ([fixed_yaml] * 3)
+    expect(User.where.not(:id => fixed_users).pluck("preferences AS p1")).to eq ([bad_yaml] * 5) # should not have run everywhere else
+
+    run_jobs
+
+    expect(User.where(:id => users).pluck("preferences AS p1")).to eq ([fixed_yaml] * 8)
+  end
+
+  context "cross-shard" do
+    specs_require_sharding
+
+    it "should deserialize job data on the correct shard" do
+      skip "needs job shard id" unless Delayed::Job == Delayed::Backend::ActiveRecord::Job && Delayed::Job.column_names.include?('shard_id')
+
+      @utf_arg = "\xF0\x9F\x98\x82"
+
+      @shard1.stubs(:delayed_jobs_shard).returns(Shard.default)
+      @shard1.activate do
+        user
+        @user.send_later(:save, @utf_arg)
+      end
+
+      job = Delayed::Job.where("handler LIKE ?", "%ActiveRecord:User #{@user.local_id}\n%").first
+      expect(job.shard_id).to eq @shard1.id
+      fixed = job.handler
+      broken = fixed.sub("\"\\U0001F602\"", "\"\\xF0\\x9F\\x98\\x82\"").sub(Syckness::TAG, "") # reconvert back into syck format for the spec
+      Delayed::Job.where(:id => job).update_all(:handler => broken)
+
+      expect(job.reload.handler).to eq broken
+
+      DataFixup::PsychMigration.run
+
+      expect(job.reload.handler).to eq fixed
+    end
+  end
+end

spec/models/active_record_base_spec.rb

@@ -384,16 +384,19 @@ describe ActiveRecord::Base do
   end
 
   describe "find_ids_in_ranges" do
+    before :once do
+      @ids = []
+      10.times { @ids << User.create!().id }
+    end
+
     it "should return ids from the table in ranges" do
-      ids = []
-      10.times { ids << User.create!().id }
       batches = []
-      User.where(id: ids).find_ids_in_ranges(:batch_size => 4) do |*found_ids|
+      User.where(id: @ids).find_ids_in_ranges(:batch_size => 4) do |*found_ids|
         batches << found_ids
       end
-      expect(batches).to eq [ [ids[0], ids[3]],
+      expect(batches).to eq [ [@ids[0], @ids[3]],
-                          [ids[4], ids[7]],
+                          [@ids[4], @ids[7]],
-                          [ids[8], ids[9]] ]
+                          [@ids[8], @ids[9]] ]
     end
 
     it "should work with scopes" do
@@ -404,6 +407,30 @@ describe ActiveRecord::Base do
         expect(found_ids).to eq [user.id, user.id]
       end
     end
+
+    it "should accept an option to start searching at a given id" do
+      batches = []
+      User.where(id: @ids).find_ids_in_ranges(:batch_size => 4, :start_at => @ids[3]) do |*found_ids|
+        batches << found_ids
+      end
+      expect(batches).to eq [ [@ids[3], @ids[6]], [@ids[7], @ids[9]] ]
+    end
+
+    it "should accept an option to end at a given id" do
+      batches = []
+      User.where(id: @ids).find_ids_in_ranges(:batch_size => 4, :end_at => @ids[5]) do |*found_ids|
+        batches << found_ids
+      end
+      expect(batches).to eq [ [@ids[0], @ids[3]], [@ids[4], @ids[5]] ]
+    end
+
+    it "should accept both options to start and end at given ids" do
+      batches = []
+      User.where(id: @ids).find_ids_in_ranges(:batch_size => 4, :start_at => @ids[2], :end_at => @ids[7]) do |*found_ids|
+        batches << found_ids
+      end
+      expect(batches).to eq [ [@ids[2], @ids[5]], [@ids[6], @ids[7]] ]
+    end
   end
 
   context "Finder tests" do

spec/models/importers/course_content_importer_spec.rb

@@ -63,35 +63,35 @@ describe Course do
       @course.reload
 
       # discussion topic tests
-      expect(@course.discussion_topics.length).to eql(3)
+      expect(@course.discussion_topics.length).to eq(3)
       migration_ids = ["1864019689002", "1865116155002", "4488523052421"].sort
       added_migration_ids = @course.discussion_topics.map(&:migration_id).uniq.sort
-      expect(added_migration_ids).to eql(migration_ids)
+      expect(added_migration_ids).to eq(migration_ids)
       topic = @course.discussion_topics.where(migration_id: "1864019689002").first
       expect(topic).not_to be_nil
-      expect(topic.title).to eql("Post here for group events, etc.")
+      expect(topic.title).to eq("Post here for group events, etc.")
       expect(topic.discussion_entries).to be_empty
       topic = @course.discussion_topics.where(migration_id: "1865116155002").first
       expect(topic).not_to be_nil
       expect(topic.assignment).not_to be_nil
 
       # quizzes
-      expect(@course.quizzes.length).to eql(1)
+      expect(@course.quizzes.length).to eq(1)
       quiz = @course.quizzes.first
       quiz.migration_id = '1865116175002'
-      expect(quiz.title).to eql("Orientation Quiz")
+      expect(quiz.title).to eq("Orientation Quiz")
 
       # wiki pages tests
       migration_ids = ["1865116206002", "1865116207002"].sort
       added_migration_ids = @course.wiki.wiki_pages.map(&:migration_id).uniq.sort
-      expect(added_migration_ids).to eql(migration_ids)
+      expect(added_migration_ids).to eq(migration_ids)
-      expect(@course.wiki.wiki_pages.length).to eql(migration_ids.length)
+      expect(@course.wiki.wiki_pages.length).to eq(migration_ids.length)
       # front page
       page = @course.wiki.front_page
       expect(page).not_to be_nil
-      expect(page.migration_id).to eql("1865116206002")
+      expect(page.migration_id).to eq("1865116206002")
       expect(page.body).not_to be_nil
-      expect(page.body.scan(/<li>/).length).to eql(4)
+      expect(page.body.scan(/<li>/).length).to eq(4)
       expect(page.body).to match(/Orientation/)
       expect(page.body).to match(/Orientation Quiz/)
       file = @course.attachments.where(migration_id: "1865116527002").first
@@ -103,11 +103,11 @@ describe Course do
       @course.reload
       expect(@course.assignments.length).to eq 4
       expect(@course.assignments.map(&:migration_id).sort).to(
-        eql(['1865116155002', '1865116014002', '4407365899221', '4469882339231'].sort))
+        eq(['1865116155002', '1865116014002', '4407365899221', '4469882339231'].sort))
       # assignment with due date
       assignment = @course.assignments.where(migration_id: "1865116014002").first
       expect(assignment).not_to be_nil
-      expect(assignment.title).to eql("Concert Review Assignment")
+      expect(assignment.title).to eq("Concert Review Assignment")
       expect(assignment.description).to match(Regexp.new("USE THE TEXT BOX!  DO NOT ATTACH YOUR ASSIGNMENT!!"))
       # The old due date (Fri Mar 27 23:55:00 -0600 2009) should have been adjusted to new time frame
       expect(assignment.due_at.year).to eq 2011
@@ -115,19 +115,19 @@ describe Course do
       # discussion topic assignment
       assignment = @course.assignments.where(migration_id: "1865116155002").first
       expect(assignment).not_to be_nil
-      expect(assignment.title).to eql("Introduce yourself!")
+      expect(assignment.title).to eq("Introduce yourself!")
-      expect(assignment.points_possible).to eql(10.0)
+      expect(assignment.points_possible).to eq(10.0)
       expect(assignment.discussion_topic).not_to be_nil
       # assignment with rubric
       assignment = @course.assignments.where(migration_id: "4469882339231").first
       expect(assignment).not_to be_nil
-      expect(assignment.title).to eql("Rubric assignment")
+      expect(assignment.title).to eq("Rubric assignment")
       expect(assignment.rubric).not_to be_nil
-      expect(assignment.rubric.migration_id).to eql("4469882249231")
+      expect(assignment.rubric.migration_id).to eq("4469882249231")
       # assignment with file
       assignment = @course.assignments.where(migration_id: "4407365899221").first
       expect(assignment).not_to be_nil
-      expect(assignment.title).to eql("new assignment")
+      expect(assignment.title).to eq("new assignment")
       file = @course.attachments.where(migration_id: "1865116527002").first
       expect(file).not_to be_nil
       expect(assignment.description).to match(Regexp.new("/files/#{file.id}/download"))
@@ -136,61 +136,61 @@ describe Course do
       expect(@course.calendar_events).to be_empty
 
       # rubrics
-      expect(@course.rubrics.length).to eql(1)
+      expect(@course.rubrics.length).to eq(1)
       rubric = @course.rubrics.first
-      expect(rubric.data.length).to eql(3)
+      expect(rubric.data.length).to eq(3)
       # Spelling
       criterion = rubric.data[0].with_indifferent_access
-      expect(criterion["description"]).to eql("Spelling")
+      expect(criterion["description"]).to eq("Spelling")
-      expect(criterion["points"]).to eql(15.0)
+      expect(criterion["points"]).to eq(15.0)
-      expect(criterion["ratings"].length).to eql(3)
+      expect(criterion["ratings"].length).to eq(3)
-      expect(criterion["ratings"][0]["points"]).to eql(15.0)
+      expect(criterion["ratings"][0]["points"]).to eq(15.0)
-      expect(criterion["ratings"][0]["description"]).to eql("Exceptional - fff")
+      expect(criterion["ratings"][0]["description"]).to eq("Exceptional - fff")
-      expect(criterion["ratings"][1]["points"]).to eql(10.0)
+      expect(criterion["ratings"][1]["points"]).to eq(10.0)
-      expect(criterion["ratings"][1]["description"]).to eql("Meet Expectations - asdf")
+      expect(criterion["ratings"][1]["description"]).to eq("Meet Expectations - asdf")
-      expect(criterion["ratings"][2]["points"]).to eql(5.0)
+      expect(criterion["ratings"][2]["points"]).to eq(5.0)
-      expect(criterion["ratings"][2]["description"]).to eql("Need Improvement - rubric entry text")
+      expect(criterion["ratings"][2]["description"]).to eq("Need Improvement - rubric entry text")
 
       # Grammar
       criterion = rubric.data[1]
-      expect(criterion["description"]).to eql("Grammar")
+      expect(criterion["description"]).to eq("Grammar")
-      expect(criterion["points"]).to eql(15.0)
+      expect(criterion["points"]).to eq(15.0)
-      expect(criterion["ratings"].length).to eql(3)
+      expect(criterion["ratings"].length).to eq(3)
-      expect(criterion["ratings"][0]["points"]).to eql(15.0)
+      expect(criterion["ratings"][0]["points"]).to eq(15.0)
-      expect(criterion["ratings"][0]["description"]).to eql("Exceptional")
+      expect(criterion["ratings"][0]["description"]).to eq("Exceptional")
-      expect(criterion["ratings"][1]["points"]).to eql(10.0)
+      expect(criterion["ratings"][1]["points"]).to eq(10.0)
-      expect(criterion["ratings"][1]["description"]).to eql("Meet Expectations")
+      expect(criterion["ratings"][1]["description"]).to eq("Meet Expectations")
-      expect(criterion["ratings"][2]["points"]).to eql(5.0)
+      expect(criterion["ratings"][2]["points"]).to eq(5.0)
-      expect(criterion["ratings"][2]["description"]).to eql("Need Improvement - you smell")
+      expect(criterion["ratings"][2]["description"]).to eq("Need Improvement - you smell")
 
       # Style
       criterion = rubric.data[2]
-      expect(criterion["description"]).to eql("Style")
+      expect(criterion["description"]).to eq("Style")
-      expect(criterion["points"]).to eql(15.0)
+      expect(criterion["points"]).to eq(15.0)
-      expect(criterion["ratings"].length).to eql(3)
+      expect(criterion["ratings"].length).to eq(3)
-      expect(criterion["ratings"][0]["points"]).to eql(15.0)
+      expect(criterion["ratings"][0]["points"]).to eq(15.0)
-      expect(criterion["ratings"][0]["description"]).to eql("Exceptional")
+      expect(criterion["ratings"][0]["description"]).to eq("Exceptional")
-      expect(criterion["ratings"][1]["points"]).to eql(10.0)
+      expect(criterion["ratings"][1]["points"]).to eq(10.0)
-      expect(criterion["ratings"][1]["description"]).to eql("Meet Expectations")
+      expect(criterion["ratings"][1]["description"]).to eq("Meet Expectations")
-      expect(criterion["ratings"][2]["points"]).to eql(5.0)
+      expect(criterion["ratings"][2]["points"]).to eq(5.0)
-      expect(criterion["ratings"][2]["description"]).to eql("Need Improvement")
+      expect(criterion["ratings"][2]["description"]).to eq("Need Improvement")
 
       # groups
-      expect(@course.groups.length).to eql(2)
+      expect(@course.groups.length).to eq(2)
 
       # files
-      expect(@course.attachments.length).to eql(4)
+      expect(@course.attachments.length).to eq(4)
       @course.attachments.each do |f|
         expect(File).to be_exist(f.full_filename)
       end
       file = @course.attachments.where(migration_id: "1865116044002").first
       expect(file).not_to be_nil
-      expect(file.filename).to eql("theatre_example.htm")
+      expect(file.filename).to eq("theatre_example.htm")
-      expect(file.folder.full_name).to eql("course files/Writing Assignments/Examples")
+      expect(file.folder.full_name).to eq("course files/Writing Assignments/Examples")
       file = @course.attachments.where(migration_id: "1864019880002").first
       expect(file).not_to be_nil
-      expect(file.filename).to eql("dropbox.zip")
+      expect(file.filename).to eq("dropbox.zip")
-      expect(file.folder.full_name).to eql("course files/Course Content/Orientation/WebCT specific and old stuff")
+      expect(file.folder.full_name).to eq("course files/Course Content/Orientation/WebCT specific and old stuff")
     end
 
     def setup_import(import_course, filename, params, copy_options={})

spec/models/quizzes/quiz_submission_spec.rb

@@ -1040,23 +1040,6 @@ describe Quizzes::QuizSubmission do
         submission.update_submission_version(vs.last, [:score])
         expect(submission.versions.map{ |s| s.model.score }).to eq [15, 25]
       end
-
-      context "when loading UTF-8 data" do
-        it "should strip bad chars" do
-          version = submission.versions.last
-
-          # inject bad byte into yaml
-          submission.submission_data = ["placeholder"]
-          submission.update_submission_version(version, [:submission_data])
-          version.yaml = version.yaml.sub("placeholder", "bad\x81byte")
-
-          # reload yaml by setting a different column
-          submission.score = 20
-          submission.update_submission_version(version, [:score])
-
-          expect(submission.versions.map{ |s| s.model.submission_data }).to eq [nil, ["badbyte"]]
-        end
-      end
     end
 
     describe "#submitted_attempts" do

spec/models/rubric_assessment_spec.rb

@@ -48,7 +48,6 @@ describe RubricAssessment do
     t = Class.new
     t.extend HtmlTextHelper
     expected = t.format_message(comment).first
-    expected.gsub!("\r", '') if CANVAS_RAILS4_0 # data has been round-tripped through YAML, and syck doesn't preserve carriage returns
     expect(@assessment.data.first[:comments_html]).to eq expected
   end