test: enhance outcome testing and resolve related issues

Author: macite

app/models/learning_outcome.rb

@@ -110,28 +110,49 @@ def add_csv_row(row)
     row << [unit_code, task_abbreviation, abbreviation, short_description, full_outcome_description, linked_learning_outcomes]
   end
 
-  def self.create_from_csv(row, result)
+  def self.create_from_csv(unit, task_definition, row, result)
     unit_code = row['unit_code']
     task_abbreviation = row['task_abbreviation']
 
-    if unit_code.present?
-      unit = Unit.find_by(code: unit_code)
-      unless unit
-        result[:errors] << { row: row, message: "Unit #{unit_code} not found" }
+    # In a unit context, but no unit code
+    if unit.present?
+      if unit_code.nil?
+        result[:errors] << { row: row, message: 'Missing unit_code' }
+        return
+      elsif unit_code != unit.code
+        result[:errors] << { row: row, message: "Unit #{unit_code} does not match unit #{unit.code}" }
         return
       end
     end
 
+    # In a task context, but no task abbreviation
+    if task_definition.present?
+      if task_abbreviation.nil?
+        result[:errors] << { row: row, message: 'Missing task_abbreviation' }
+        return
+      elsif task_abbreviation != task_definition.abbreviation
+        result[:errors] << { row: row, message: "Task #{task_abbreviation} does not match task #{task_definition.abbreviation}" }
+        return
+      end
+    end
+
+    if unit_code.present? && unit.nil?
+      result[:errors] << { row: row, message: 'Unit outcomes must be uploaded to a unit' }
+      return
+    end
+
+    if task_abbreviation.present? && unit.nil?
+      result[:errors] << { row: row, message: 'Task outcomes must be uploaded to a unit' }
+      return
+    end
+
+    # Get the context for the outcome
     if task_abbreviation.present?
-      task_definition = TaskDefinition.find_by(abbreviation: task_abbreviation)
+      task_definition = unit.task_definitions.find_by(abbreviation: task_abbreviation)
       unless task_definition
         result[:errors] << { row: row, message: "Task #{task_abbreviation} not found" }
         return
       end
-      unless unit.task_definitions.include?(task_definition)
-        result[:errors] << { row: row, message: "Task #{task_abbreviation} is not linked to unit #{unit_code}" }
-        return
-      end
       context_type = 'TaskDefinition'
       context_id = task_definition.id
     elsif unit_code.present?
@@ -165,12 +186,26 @@ def self.create_from_csv(row, result)
     end
 
     linked_outcomes = row['linked_outcomes'].to_s.split(',').map(&:strip)
-    linked_outcome_ids = LearningOutcome.where(abbreviation: linked_outcomes).pluck(:id)
-    missing_links = linked_outcomes - LearningOutcome.where(abbreviation: linked_outcomes).pluck(:abbreviation)
 
-    if missing_links.any?
-      result[:errors] << { row: row, message: "Linked outcomes #{missing_links.join(', ')} not found" }
-      return
+    linked_outcome_ids = []
+
+    linked_outcomes.each do |linked_outcome|
+      # Search in the unit if present
+      found_outcome = if unit.present?
+                        unit.learning_outcomes.find_by(abbreviation: linked_outcome) || LearningOutcome.global_outcomes.find_by(abbreviation: linked_outcome)
+                      else
+                        # Search in the global outcomes
+                        LearningOutcome.global_outcomes.find_by(abbreviation: linked_outcome)
+                      end
+
+      if found_outcome.present?
+        linked_outcome_ids << found_outcome.id
+      else
+        result[:errors] << { row: row, message: "Linked outcome #{linked_outcome} not found" }
+        return
+      end
+
+      found_outcome.id
     end
 
     outcome = LearningOutcome.find_or_create_by(context_id: context_id, context_type: context_type, abbreviation: abbreviation)

app/models/learning_outcome_link.rb

@@ -18,6 +18,7 @@ class LearningOutcomeLink < ApplicationRecord
   validates :source_id, uniqueness: { scope: :target_id, message: 'Link already exists' }
 
   validate :validate_link_type
+  validate :validate_context
 
   def validate_link_type
     return if source.nil? || target.nil?
@@ -30,4 +31,12 @@ def validate_link_type
       errors.add(:link_type, 'Learning outcomes must only link to higher level outcomes')
     end
   end
+
+  def validate_context
+    return if source.nil? || target.nil?
+
+    if source.context_type == 'TaskDefinition' && target.context_type == 'Unit' && source.context.unit != target.context
+      errors.add(:link_type, 'Task learning outcomes must be linked to the same unit')
+    end
+  end
 end

app/models/task_definition.rb

@@ -282,7 +282,9 @@ def self.to_csv(task_definitions)
     end
   end
 
-  def export_learning_outcome_to_csv(include_tlos = false)
+  # Export the learning outcomes for this task definition to a CSV file
+  # @param _include_tlos [Boolean] ignored as at the task definition level already
+  def export_learning_outcome_to_csv(*)
     CSV.generate do |row|
       row << LearningOutcome.csv_header
       learning_outcomes.each do |outcome|
@@ -291,7 +293,7 @@ def export_learning_outcome_to_csv(include_tlos = false)
     end
   end
 
-  def export_feedback_chips_to_csv(include_tlos = false)
+  def export_feedback_chips_to_csv(*)
     CSV.generate do |row|
       row << Feedback::FeedbackChip.csv_header
       learning_outcomes.each do |outcome|
@@ -323,7 +325,7 @@ def import_outcomes_from_csv(file)
       next if row[0] =~ /abbreviation/
 
       begin
-        LearningOutcome.create_from_csv(row, result)
+        LearningOutcome.create_from_csv(unit, self, row, result)
       rescue StandardError => e
         result[:errors] << { row: row, message: e.message.to_s }
       end

app/models/unit.rb

@@ -319,9 +319,17 @@ def rollover(teaching_period, start_date, end_date, new_code)
       new_unit.group_sets << group_set.dup
     end
 
-    task_definition_outcome_mapping = {}
-    unit_learning_outcome_mapping = {}
-    chip_mapping = {}
+    # Old outcome to new outcome mapping
+    outcome_mapping = {}
+
+    # Duplicate unit learning outcomes - before task definitions as they are linked to them
+    learning_outcomes.each do |learning_outcome|
+      new_outcome = learning_outcome.dup
+      new_outcome.context = new_unit
+      new_outcome.save!
+      new_unit.learning_outcomes << new_outcome
+      outcome_mapping[learning_outcome] = new_outcome
+    end
 
     # Duplicate task definitions
     task_definitions.each do |td|
@@ -330,31 +338,7 @@ def rollover(teaching_period, start_date, end_date, new_code)
       td.learning_outcomes.each do |learning_outcome| # for each old task definition, duplicate the learning outcomes associated with it aswell
         new_outcome = learning_outcome.dup
         new_td.learning_outcomes << new_outcome
-        task_definition_outcome_mapping[learning_outcome.id] = new_outcome.id
-
-        learning_outcome.feedback_chips.each do |chip|
-          new_chip = chip.dup
-          new_outcome.feedback_chips << new_chip
-          new_chip.learning_outcome_id = new_outcome.id if chip.respond_to?(:learning_outcome_id)
-          new_chip.parent_chip_id = nil
-          new_chip.save!
-          chip_mapping[chip.id] = new_chip.id
-        end
-
-        learning_outcome.feedback_chips.where.not(parent_chip_id: nil).find_each do |chip|
-          child_chip = new_outcome.feedback_chips.find(chip_mapping[chip.id])
-          parent_chip = new_outcome.feedback_chips.find(chip_mapping[chip.parent_chip_id])
-          child_chip.update(parent_chip_id: parent_chip.id)
-        end
-      end
-
-      LearningOutcomeLink.where(source_id: task_definition_outcome_mapping.keys).find_each do |link|
-        new_source_id = task_definition_outcome_mapping[link.source_id]
-        new_target_id = task_definition_outcome_mapping[link.target_id]
-
-        if new_source_id && new_target_id
-          LearningOutcomeLink.create!(source_id: new_source_id, target_id: new_target_id)
-        end
+        outcome_mapping[learning_outcome] = new_outcome
       end
 
       # Update default task definition if necessary
@@ -363,52 +347,37 @@ def rollover(teaching_period, start_date, end_date, new_code)
       end
     end
 
-    # Duplicate unit learning outcomes
-    learning_outcomes.each do |learning_outcome|
-      new_outcome = learning_outcome.dup
-      new_outcome.context = new_unit
-      new_outcome.save!
-      new_unit.learning_outcomes << new_outcome
-      unit_learning_outcome_mapping[learning_outcome.id] = new_outcome.id
+    # Link outcomes
+    outcome_mapping.each do |old_outcome, new_outcome|
+      old_outcome.linked_outcomes.each do |old_linked_outcome|
+        new_target = outcome_mapping[old_linked_outcome] || old_linked_outcome
 
-      learning_outcome.feedback_chips.each do |chip|
+        if new_outcome.present? && new_target.present?
+          LearningOutcomeLink.create!(source_id: new_outcome.id, target_id: new_target.id)
+        end
+      end
+    end
+
+    # Now duplicate all feedback chips
+    chip_mapping = {}
+
+    outcome_mapping.each do |source_outcome, new_outcome|
+      source_outcome.feedback_chips.each do |chip|
         new_chip = chip.dup
         new_outcome.feedback_chips << new_chip
-        new_chip.learning_outcome_id = new_outcome.id if chip.respond_to?(:learning_outcome_id)
+        new_chip.learning_outcome_id = new_outcome.id
         new_chip.parent_chip_id = nil
         new_chip.save!
-        chip_mapping[chip.id] = new_chip.id
+        chip_mapping[chip] = new_chip
       end
 
-      learning_outcome.feedback_chips.where.not(parent_chip_id: nil).find_each do |chip|
-        child_chip = new_outcome.feedback_chips.find(chip_mapping[chip.id])
-        parent_chip = new_outcome.feedback_chips.find(chip_mapping[chip.parent_chip_id])
+      source_outcome.feedback_chips.where.not(parent_chip_id: nil).find_each do |old_chip|
+        child_chip = chip_mapping[old_chip]
+        parent_chip = chip_mapping[old_chip.parent_chip]
         child_chip.update(parent_chip_id: parent_chip.id)
       end
     end
 
-    LearningOutcomeLink.where(source_id: unit_learning_outcome_mapping.keys).find_each do |link|
-      new_source_id = unit_learning_outcome_mapping[link.source_id] || link.source_id
-      new_target_id = unit_learning_outcome_mapping[link.target_id] || link.target_id
-
-      if new_source_id && new_target_id
-        LearningOutcomeLink.create!(source_id: new_source_id, target_id: new_target_id)
-      end
-    end
-
-    chip_mapping.each do |old_id, new_id|
-      old_chip = Feedback::FeedbackChip.find(old_id)
-      if old_chip.parent_chip_id && chip_mapping[old_chip.parent_chip_id]
-        new_chip = Feedback::FeedbackChip.find(new_id)
-        new_chip.update(parent_chip_id: chip_mapping[old_chip.parent_chip_id])
-      end
-    end
-
-    # Duplicate alignments
-    task_outcome_alignments.each do |align|
-      align.duplicate_to(new_unit)
-    end
-
     new_unit
   end
 
@@ -1154,7 +1123,7 @@ def import_outcomes_from_csv(file)
       next if row[0] =~ /unit_code/
 
       begin
-        LearningOutcome.create_from_csv(row, result)
+        LearningOutcome.create_from_csv(self, nil, row, result)
       rescue Exception => e
         result[:errors] << { row: row, message: e.message.to_s }
       end

test/api/feedback/feedback_chip_api_consolidated_test.rb

@@ -91,7 +91,7 @@ def test_auth_for_create_edit_unit_feedback_chips
   def test_auth_for_create_edit_global_feedback_chips
     admin = FactoryBot.create(:user, :admin)
 
-    learning_outcome = LearningOutcome.global_outcomes.first
+    learning_outcome = FactoryBot.create(:learning_outcome, context_id: nil, context_type: nil)
 
     users_can = [
       admin

test/api/feedback/learning_outcome_api_test.rb

@@ -242,8 +242,8 @@ def test_delete_task_definition_learning_outcome
   end
 
   def test_create_learning_outcome_links
-    task_definition = FactoryBot.create(:task_definition, outcome_count: 0)
     unit = FactoryBot.create(:unit, name: 'i like units', code: 'abcde', description: 'test unit')
+    task_definition = FactoryBot.create(:task_definition, outcome_count: 0, unit: unit)
     target_learning_outcome = FactoryBot.create(:learning_outcome, context_id: unit.id, context_type: 'Unit', abbreviation: 'TGT1', short_description: 'target learning outcome', full_outcome_description: 'this outcome will be linked to source')
     target_learning_outcome2 = FactoryBot.create(:learning_outcome, context_id: unit.id, context_type: 'Unit', abbreviation: 'TGT2', short_description: 'target learning outcome 2', full_outcome_description: 'this outcome will be linked to source')
     data_to_post = {
@@ -268,8 +268,8 @@ def test_create_learning_outcome_links
   end
 
   def test_overwrite_linked_learning_outcomes
-    task_definition = FactoryBot.create(:task_definition, outcome_count: 0)
     unit = FactoryBot.create(:unit, name: 'i like units', code: 'abcde', description: 'test unit')
+    task_definition = FactoryBot.create(:task_definition, outcome_count: 0, unit: unit)
     target_learning_outcome = FactoryBot.create(:learning_outcome, context_id: unit.id, context_type: 'Unit', abbreviation: 'TGT1', short_description: 'target learning outcome', full_outcome_description: 'this outcome will be linked to source')
     target_learning_outcome2 = FactoryBot.create(:learning_outcome, context_id: unit.id, context_type: 'Unit', abbreviation: 'TGT2', short_description: 'target learning outcome 2', full_outcome_description: 'this outcome will be linked to source')
     data_to_post = {
@@ -329,7 +329,7 @@ def test_unit_rollover
     # Create a unit with learning outcomes
     global_learning_outcome = FactoryBot.create(:learning_outcome, context_id: nil, context_type: nil, abbreviation: 'nglo')
     original_unit = FactoryBot.create(:unit, name: 'rollver unit', code: 'abcde', description: 'test unit to be rolled over') # When creating unit, it generates 2 random outcomes in addition to below
-    original_task_definition = FactoryBot.create(:task_definition, unit_id: original_unit.id, outcome_count: 0)
+    original_task_definition = FactoryBot.create(:task_definition, unit: original_unit, outcome_count: 0)
     original_lo1 = FactoryBot.create(:learning_outcome, context_id: original_unit.id, context_type: 'Unit', abbreviation: 'olo1', short_description: 'sd', full_outcome_description: 'fod')
     original_lo2 = FactoryBot.create(:learning_outcome, context_id: original_unit.id, context_type: 'Unit', abbreviation: 'olo2', short_description: 'sd', full_outcome_description: 'fod')
     original_lo3 = FactoryBot.create(:learning_outcome, context_id: original_unit.id, context_type: 'Unit', abbreviation: 'olo3', short_description: 'sd', full_outcome_description: 'fod')
@@ -365,6 +365,7 @@ def test_unit_rollover
 
     new_unit = original_unit.rollover(TeachingPeriod.find(2), nil, nil, nil)
     new_td = new_unit.task_definitions.find_by(name: original_task_definition.name)
+    new_td_lo = new_td.learning_outcomes.find_by(abbreviation: td_lo.abbreviation)
 
     assert_equal 5, new_unit.learning_outcomes.count, 'New unit should have 5 learning outcomes'
     auto1, auto2, new_lo1, new_lo2, new_lo3 = new_unit.learning_outcomes.order(:id)
@@ -406,7 +407,7 @@ def test_unit_rollover
     new_link = LearningOutcomeLink.find_by(source_id: new_lo1.id, target_id: global_learning_outcome.id)
     assert_not_nil new_link, 'New learning outcome 1 should be linked to the global learning outcome'
 
-    new_link = LearningOutcomeLink.find_by(source_id: new_lo2.id, target_id: td_lo.id)
+    new_link = LearningOutcomeLink.find_by(source_id: new_td_lo.id, target_id: new_lo2.id)
     assert_not_nil new_link, 'New learning outcome 2 should be linked to the task definition learning outcome'
   end