Outcome imports can customize criterion ratings

closes OUT-396

test plan
  - enable Academic Benchmark importing
  - initiate an import without overriding any criterion ratings
  - after the import is complete, confirm that the default
    calculation method, calculation int, points possible, mastery
    points and ratings are configured for the imported outcomes
  - on the account's outcome page, import any one of the newly
    imported standards
  - on the course's outcome page, import the newly imported
    account outcome
  - create an assignment and align to the course outcome
  - as a student, submit to the assignment
  - as a teacher, grade the assignment's rubric
  - initiate another import, specifying a calculation method,
    calculation int, points possible, mastery points and ratings
  - after import is complete, confirm that the learning outcomes
    have those values set, except for the one outcome that was
    aligned, which should have the default values

Change-Id: If3a730b85548a0851b82c7da67dc4a2e8714eb74
Reviewed-on: https://gerrit.instructure.com/93948
Tested-by: Jenkins
Reviewed-by: Michael Brewer-Davis <mbd@instructure.com>
QA-Review: Alex Ortiz-Rosado <aortiz@instructure.com>
Product-Review: McCall Smith <mcsmith@instructure.com>

Author: augustocallejas

app/controllers/outcomes_import_api_controller.rb

@@ -31,13 +31,15 @@ def available
 
   def create
     return render json: { error: "must specify a guid to import" } unless params[:guid]
-    return unless valid_guid(params[:guid])
-
     begin
+      err_msg = "Invalid parameters"
+      options = valid_options(params)
+      valid_guid(params[:guid])
+
       err_msg = "Import failed to queue"
 
       # AcademicBenchmark.queue_migration_for_guid can raise Canvas::Migration::Error
-      migration = AcademicBenchmark.import(Array(params[:guid])).first
+      migration = AcademicBenchmark.import(Array(params[:guid]), options).first
 
       if migration
         render json: { migration_id: migration.id, guid: params[:guid] }
@@ -92,10 +94,54 @@ def has_api_config
   ##
   def valid_guid(guid)
     unless guid =~ /[A-F0-9]{8}-[A-F0-9]{4}-[A-F0-9]{4}-[A-F0-9]{4}-[A-F0-9]{12}/i
-      render json: { error: "GUID is invalid" }
-      return false
+      raise "GUID is invalid: #{guid}"
     end
-    true
+  end
+
+  def parse_int(value)
+    Integer value
+  rescue
+    raise "invalid value, must be integer: #{value}"
+  end
+
+  def valid_options(hash)
+    options = {}
+    if hash.key? :calculation_method
+      options[:calculation_method] = parse_calculation_method hash[:calculation_method]
+      options[:calculation_int] = parse_calculation_int options[:calculation_method], hash[:calculation_int]
+    end
+    options[:mastery_points] = parse_int hash[:mastery_points] if hash.key? :mastery_points
+    options[:points_possible] = parse_int hash[:points_possible] if hash.key? :points_possible
+    unless hash[:ratings].nil?
+      raise "ratings expected to be an array" unless hash[:ratings].is_a?(Array)
+      options[:ratings] = hash[:ratings].map {|r| parse_rating(r)}
+    end
+    options
+  end
+
+  def parse_calculation_method(value)
+    unless LearningOutcome.valid_calculation_method?(value)
+      raise "invalid calculation_method: #{value}"
+    end
+    value
+  end
+
+  def parse_calculation_int(calc_method, value)
+    int = value.nil? ? nil : parse_int(value)
+    unless LearningOutcome.valid_calculation_int?(int, calc_method)
+      raise "invalid calculation_int: #{value}"
+    end
+    int
+  end
+
+  def parse_rating(rating)
+    if rating.nil? || !rating.is_a?(Hash)
+      raise "invalid ratings value: #{rating}"
+    end
+    if rating[:description].nil? || !rating[:description].is_a?(String)
+      raise "invalid description value: #{rating[:description]}"
+    end
+    { :description => rating[:description], :points => parse_int(rating[:points]) }
   end
 
   ##

app/models/importers/learning_outcome_importer.rb

@@ -71,14 +71,19 @@ def self.import_from_migration(hash, migration, item=nil)
         item.workflow_state = 'active' if item.deleted?
         item.short_description = hash[:title]
         item.description = hash[:description]
-        item.calculation_method ||= hash[:calculation_method]
-        item.calculation_int ||= hash[:calculation_int]
+        assessed = item.assessed?
+        unless assessed
+          item.calculation_method = hash[:calculation_method] || item.calculation_method
+          item.calculation_int = hash[:calculation_int] || item.calculation_int
+        end
 
         if hash[:ratings]
           item.data = {:rubric_criterion=>{}}
-          item.data[:rubric_criterion][:ratings] = hash[:ratings] ? hash[:ratings].map(&:symbolize_keys) : []
-          item.data[:rubric_criterion][:mastery_points] = hash[:mastery_points]
-          item.data[:rubric_criterion][:points_possible] = hash[:points_possible]
+          unless assessed
+            item.data[:rubric_criterion][:ratings] = hash[:ratings] ? hash[:ratings].map(&:symbolize_keys) : []
+            item.data[:rubric_criterion][:mastery_points] = hash[:mastery_points]
+            item.data[:rubric_criterion][:points_possible] = hash[:points_possible]
+          end
           item.data[:rubric_criterion][:description] = item.short_description || item.description
         end
 

app/models/learning_outcome.rb

@@ -87,31 +87,32 @@ def infer_defaults
   end
 
   def validate_calculation_int
-    unless valid_calculation_int?(calculation_int, calculation_method)
-      if valid_calculation_ints.to_a.empty?
+    unless self.class.valid_calculation_int?(calculation_int, calculation_method)
+      valid_ints = self.class.valid_calculation_ints(self.calculation_method)
+      if valid_ints.to_a.empty?
         errors.add(:calculation_int, t(
           "A calculation value is not used with this calculation method"
         ))
       else
         errors.add(:calculation_int, t(
           "'%{calculation_int}' is not a valid value for this calculation method. The value must be between '%{valid_calculation_ints_min}' and '%{valid_calculation_ints_max}'",
           :calculation_int => calculation_int,
-          :valid_calculation_ints_min => valid_calculation_ints.min,
-          :valid_calculation_ints_max => valid_calculation_ints.max
+          :valid_calculation_ints_min => valid_ints.min,
+          :valid_calculation_ints_max => valid_ints.max
         ))
       end
     end
   end
 
-  def valid_calculation_method?(method=self.calculation_method)
+  def self.valid_calculation_method?(method)
     CALCULATION_METHODS.keys.include?(method)
   end
 
-  def valid_calculation_ints(method=self.calculation_method)
+  def self.valid_calculation_ints(method)
     VALID_CALCULATION_INTS[method]
   end
 
-  def valid_calculation_int?(int, method=self.calculation_method)
+  def self.valid_calculation_int?(int, method)
     if valid_calculation_method?(method)
       valid_ints = valid_calculation_ints(method)
       (int.nil? && valid_ints.to_a.empty?) || valid_ints.include?(int)

gems/plugins/academic_benchmark/lib/academic_benchmark.rb

@@ -16,7 +16,7 @@ def self.config
 
   class APIError < StandardError; end
 
-  def self.import(guid_or_guids)
+  def self.import(guid_or_guids, options={})
     if !AcademicBenchmark.config[:api_key] || AcademicBenchmark.config[:api_key].empty?
       raise Canvas::Migration::Error.new("Not importing academic benchmark data because no API key is set")
     end
@@ -32,11 +32,11 @@ def self.import(guid_or_guids)
     end
 
     Array(guid_or_guids).map do |guid|
-      AcademicBenchmark.queue_migration_for_guid(guid, permissionful_user).first
+      AcademicBenchmark.queue_migration_for_guid(guid, permissionful_user, options).first
     end
   end
 
-  def self.queue_migration_for_guid(guid, user)
+  def self.queue_migration_for_guid(guid, user, options={})
     unless Account.site_admin.grants_right?(user, :manage_global_outcomes)
       raise Canvas::Migration::Error.new(I18n.t('academic_benchmark.no_permissions', "User isn't allowed to edit global outcomes"))
     end
@@ -49,6 +49,7 @@ def self.queue_migration_for_guid(guid, user)
     cm.migration_settings[:no_archive_file] = true
     cm.migration_settings[:skip_import_notification] = true
     cm.migration_settings[:skip_job_progress] = true
+    cm.migration_settings[:migration_options] = options
     cm.strand = "academic_benchmark"
     cm.user = user
     cm.save!

gems/plugins/academic_benchmark/lib/academic_benchmark/converter.rb

@@ -5,6 +5,7 @@ class Converter < Canvas::Migration::Migrator
 
     def initialize(settings={})
       super(settings, "academic_benchmark")
+      @ratings_overrides = settings[:migration_options] || {}
 
       ab_settings = AcademicBenchmark.config
 
@@ -126,7 +127,7 @@ def process_leaves!(data)
 
     def process_json_data(data)
       if data = find_by_prop(data, "type", "authority")
-        outcomes = Standard.new(data).build_outcomes
+        outcomes = Standard.new(data).build_outcomes(@ratings_overrides)
         @course[:learning_outcomes] << outcomes
       else
         err_msg = I18n.t("Couldn't find an authority to update")

gems/plugins/academic_benchmark/lib/academic_benchmark/standard.rb

@@ -18,22 +18,19 @@ def initialize(data, parent=nil)
     end
   end
 
-  def build_outcomes
+  def build_outcomes(ratings={})
     hash = {:migration_id => guid, :vendor_guid => guid, :low_grade => low_grade, :high_grade => high_grade, :is_global_standard => true}
     hash[:description] = description
     if is_leaf?
       # create outcome
       hash[:type] = 'learning_outcome'
       hash[:title] = build_num_title
-      set_default_ratings(hash)
+      set_default_ratings(hash, ratings)
     else
       #create outcome group
       hash[:type] = 'learning_outcome_group'
       hash[:title] = build_title
-      hash[:outcomes] = []
-      @children.each do |chld|
-        hash[:outcomes] << chld.build_outcomes
-      end
+      hash[:outcomes] = @children.map {|chld| chld.build_outcomes(ratings)}
     end
 
     hash
@@ -139,12 +136,13 @@ def is_leaf?
     num && @children.empty?
   end
 
-  def set_default_ratings(hash)
+  def set_default_ratings(hash, overrides={})
     hash[:ratings] = [{:description => "Exceeds Expectations", :points => 5},
                       {:description => "Meets Expectations", :points => 3},
                       {:description => "Does Not Meet Expectations", :points => 0}]
     hash[:mastery_points] = 3
     hash[:points_possible] = 5
+    hash.merge!(overrides)
   end
 end
 end

gems/plugins/academic_benchmark/spec_canvas/academic_benchmark_spec.rb

@@ -13,6 +13,8 @@
     @cm.migration_settings[:migration_type] = 'academic_benchmark_importer'
     @cm.migration_settings[:import_immediately] = true
     @cm.migration_settings[:base_url] = "http://example.com/"
+    @cm.migration_settings[:migration_options] = {points_possible: 10, mastery_points: 6,
+      ratings: [{description: 'Bad', points: 0}, {description: 'Awesome', points: 10}]}
     @cm.user = @user
     @cm.save!
 
@@ -34,6 +36,13 @@
     @cm.save!
   end
 
+  def verify_rubric_criterion(outcome)
+    expect(outcome.data[:rubric_criterion][:mastery_points]).to eq 6
+    expect(outcome.data[:rubric_criterion][:points_possible]).to eq 10
+    expect(outcome.data[:rubric_criterion][:ratings]).to eq [{description: 'Bad', points: 0},
+                                                             {description: 'Awesome', points: 10}]
+  end
+
   def verify_full_import
     @root_group = LearningOutcomeGroup.global_root_outcome_group
     expect(@root_group.child_outcome_groups.count).to eq 1
@@ -66,12 +75,15 @@ def verify_full_import
     expect(f.child_outcome_links.count).to eq 3
 
     g = LearningOutcome.global.where(migration_id: "ggggggggggggggggg").first
+    verify_rubric_criterion(g)
     expect(g.short_description).to eq "K.CC.1"
     expect(g.description).to eq "Count to 100 by ones and by tens."
     g = LearningOutcome.global.where(migration_id: "hhhhhhhhhhhhhhhh").first
+    verify_rubric_criterion(g)
     expect(g.short_description).to eq "K.CC.2"
     expect(g.description).to eq "Count forward beginning from a given number within the known sequence (instead of having to begin at 1)."
     g = LearningOutcome.global.where(migration_id: "iiiiiiiiiiiiiiiii").first
+    verify_rubric_criterion(g)
     expect(g.short_description).to eq "K.CC.3"
     expect(g.description).to eq "Write numbers from 0 to 20. Represent a number of objects with a written numeral 0-20 (with 0 representing a count of no objects)."
 
@@ -95,6 +107,7 @@ def verify_full_import
     expect(l.child_outcome_links.count).to eq 1
 
     m = LearningOutcome.global.where(migration_id: "mmmmmmmmmmm").first
+    verify_rubric_criterion(g)
     expect(m.short_description).to eq "1.DD.1"
     expect(m.description).to eq "And something else"
     expect(m.title).to eq "1.DD.1"