Validation that courses in family share course type

dashboard/app/models/concerns/curriculum/course_types.rb

@@ -15,11 +15,45 @@ module Curriculum::CourseTypes
     validates :participant_audience, acceptance: {accept: SharedCourseConstants::PARTICIPANT_AUDIENCE.to_h.values, message: 'must be facilitator, teacher, or student'}
 
     validate :cannot_have_same_audiences
+    validate :must_have_same_course_type_as_family
+  end
+
+  # All courses in the same family name must have the save instruction_type, instructor_audience, and participant audience
+  def must_have_same_course_type_as_family
+    all_family_courses = get_family_courses
+    return if all_family_courses.nil_or_empty?
+
+    return unless all_family_courses.length > 1
+    errors.add(:instructor_audience, 'must be the same for all courses in a family.') if all_family_courses.map(&:instructor_audience).uniq.length > 1 || all_family_courses.last&.instructor_audience != instructor_audience
+    errors.add(:participant_audience, 'must be the same for all courses in a family.') if all_family_courses.map(&:participant_audience).uniq.length > 1 || all_family_courses.last&.participant_audience != participant_audience
+    errors.add(:instruction_type, 'must be the same for all courses in a family.') if all_family_courses.map(&:instruction_type).uniq.length > 1 || all_family_courses.last&.instruction_type != instruction_type
+  end
+
+  # Get the family name for the course based on if its set on the UnitGroup or Unit
+  def get_course_family_name
+    is_a?(Script) && unit_group ? unit_group.family_name : family_name
+  end
+
+  # If course we are check is a unit_group or a unit that is in a unit_group check the family_name on the UnitGroup.
+  # If the course is a unit that is not in a unit_group check the unit for the family_name
+  def get_family_courses
+    family_name = get_course_family_name
+    return nil if family_name.nil_or_empty?
+
+    all_family_courses = nil
+
+    if is_a?(UnitGroup) || (is_a?(Script) && unit_group)
+      all_family_courses = UnitGroup.all.select {|c| c.family_name == family_name}
+    elsif is_a?(Script)
+      all_family_courses = Script.get_family_from_cache(family_name)
+    end
+
+    all_family_courses
   end
 
   # Instructor and Participant Audience can not be equal unless they are nil
   def cannot_have_same_audiences
-    errors.add(:instructor_audience, 'You cannot have the same instructor and participant audiences.') if !instructor_audience.nil? && instructor_audience == participant_audience
+    errors.add(:instructor_audience, 'should be different from participant audiences.') if !instructor_audience.nil? && instructor_audience == participant_audience
   end
 
   # Checks if a user can be the instructor for the course. universal instructors and levelbuilders

dashboard/test/controllers/scripts_controller_test.rb

@@ -901,7 +901,7 @@ class ScriptsControllerTest < ActionController::TestCase
       is_course: 'on',
       peer_reviews_to_complete: 1,
       curriculum_path: 'fake_curriculum_path',
-      family_name: 'coursea',
+      family_name: 'fake-family-z',
       version_year: '2020',
       pilot_experiment: 'fake-pilot-experiment',
       editor_experiment: 'fake-editor-experiment',

dashboard/test/factories/factories.rb

@@ -753,6 +753,7 @@
     sequence(:name) {|n| "bogus-script-#{n}"}
     published_state "beta"
     is_migrated true
+    instruction_type "teacher_led"
     participant_audience "student"
     instructor_audience "teacher"
 

dashboard/test/fixtures/test-serialize-seeding-json.script_json

@@ -12,7 +12,7 @@
     "new_name": null,
     "family_name": "test-serialize-seeding-json-family",
     "published_state": "beta",
-    "instruction_type": null,
+    "instruction_type": "teacher_led",
     "instructor_audience": "teacher",
     "participant_audience": "student",
     "seeding_key": {
@@ -1056,4 +1056,4 @@
       }
     }
   ]
-}
+}

dashboard/test/lib/services/script_seed_test.rb

@@ -83,12 +83,13 @@ class ScriptSeedTest < ActiveSupport::TestCase
       #   1 query to get all the programming environments
       #   1 query to get all the standards frameworks
       #   1 query to check for a CourseOffering. (Would be a few more if is_course was true)
+      #   1 query to check if units in family have the same course type settings
       # LevelsScriptLevels has queries which scale linearly with the number of rows.
       # As far as I know, to get rid of those queries per row, we'd need to load all Levels into memory. I think
       # this is slower for most individual Scripts, but there could be a savings when seeding multiple Scripts.
       # For now, leaving this as a potential future optimization, since it seems to be reasonably fast as is.
       # The game queries can probably be avoided with a little work, though they only apply for Blockly levels.
-      assert_queries(86) do
+      assert_queries(87) do
         ScriptSeed.seed_from_json(json)
       end
 

dashboard/test/models/concerns/curriculum/course_types_test.rb

@@ -9,33 +9,43 @@ class CourseTypesTests < ActiveSupport::TestCase
     @plc_reviewer = create :plc_reviewer
     @levelbuilder = create :levelbuilder
 
-    @unit_group = create(:unit_group, name: 'course-instructed-by-teacher', instructor_audience: SharedCourseConstants::INSTRUCTOR_AUDIENCE.teacher, participant_audience: SharedCourseConstants::PARTICIPANT_AUDIENCE.student)
+    # Unit Groups with Units
+    @unit_group = create(:unit_group, name: 'course-instructed-by-teacher', family_name: 'teacher-unit-groups')
     @unit_in_course = create(:script, name: 'unit-in-teacher-instructed-course')
     create(:unit_group_unit, script: @unit_in_course, unit_group: @unit_group, position: 1)
     @unit_in_course.reload
 
-    @course_teacher_to_students = create(:unit_group, name: 'course-teacher-to-student', instructor_audience: SharedCourseConstants::INSTRUCTOR_AUDIENCE.teacher, participant_audience: SharedCourseConstants::PARTICIPANT_AUDIENCE.student)
+    @unit_group_2 = create(:unit_group, name: 'course-instructed-by-teacher-2', family_name: 'teacher-unit-groups')
+    @unit_in_course_2 = create(:script, name: 'unit-in-teacher-instructed-course-2')
+    create(:unit_group_unit, script: @unit_in_course_2, unit_group: @unit_group_2, position: 1)
+    @unit_in_course_2.reload
+
+    # UnitGroups without Units
+    @course_teacher_to_students = create(:unit_group, name: 'course-teacher-to-student', family_name: 'teacher-unit-groups')
     @course_facilitator_to_teacher = create(:unit_group, name: 'course-facilitator-to-teacher', instructor_audience: SharedCourseConstants::INSTRUCTOR_AUDIENCE.facilitator, participant_audience: SharedCourseConstants::PARTICIPANT_AUDIENCE.teacher)
     @course_universal_instructor_to_teacher = create(:unit_group, name: 'course-universal-instructor-to-teacher', instructor_audience: SharedCourseConstants::INSTRUCTOR_AUDIENCE.universal_instructor, participant_audience: SharedCourseConstants::PARTICIPANT_AUDIENCE.teacher)
     @course_plc_reviewer_to_facilitator = create(:unit_group, name: 'course-plc-reviewer-to-facilitator', instructor_audience: SharedCourseConstants::INSTRUCTOR_AUDIENCE.plc_reviewer, participant_audience: SharedCourseConstants::PARTICIPANT_AUDIENCE.facilitator)
 
-    @unit_teacher_to_students = create(:script, name: 'unit-teacher-to-student', instructor_audience: SharedCourseConstants::INSTRUCTOR_AUDIENCE.teacher, participant_audience: SharedCourseConstants::PARTICIPANT_AUDIENCE.student)
+    # Units not in UnitGroup
+    @unit_teacher_to_students = create(:script, name: 'unit-teacher-to-student', family_name: 'teacher-units')
     @unit_facilitator_to_teacher = create(:script, name: 'unit-facilitator-to-teacher', instructor_audience: SharedCourseConstants::INSTRUCTOR_AUDIENCE.facilitator, participant_audience: SharedCourseConstants::PARTICIPANT_AUDIENCE.teacher)
     @unit_universal_instructor_to_teacher = create(:script, name: 'universal-instructor-to-teacher', instructor_audience: SharedCourseConstants::INSTRUCTOR_AUDIENCE.universal_instructor, participant_audience: SharedCourseConstants::PARTICIPANT_AUDIENCE.teacher)
     @unit_plc_reviewer_to_facilitator = create(:script, name: 'plc-reviewer-to-facilitator', instructor_audience: SharedCourseConstants::INSTRUCTOR_AUDIENCE.plc_reviewer, participant_audience: SharedCourseConstants::PARTICIPANT_AUDIENCE.facilitator)
+
+    @unit_teacher_to_students_2 = create(:script, name: 'unit-teacher-to-student-2', family_name: 'teacher-units')
   end
 
   test 'create unit_group with same audiences raises error' do
     e = assert_raises do
       create(:unit_group, name: 'same-audiences', instructor_audience: SharedCourseConstants::INSTRUCTOR_AUDIENCE.teacher, participant_audience: SharedCourseConstants::PARTICIPANT_AUDIENCE.teacher)
     end
-    assert_equal "Validation failed: Instructor audience You cannot have the same instructor and participant audiences.", e.message
+    assert_equal "Validation failed: Instructor audience should be different from participant audiences.", e.message
   end
   test 'create script with same audiences raises error' do
     e = assert_raises do
       create(:script, name: 'same-audiences', instructor_audience: SharedCourseConstants::INSTRUCTOR_AUDIENCE.teacher, participant_audience: SharedCourseConstants::PARTICIPANT_AUDIENCE.teacher)
     end
-    assert_equal "Validation failed: Instructor audience You cannot have the same instructor and participant audiences.", e.message
+    assert_equal "Validation failed: Instructor audience should be different from participant audiences.", e.message
   end
 
   test 'unit in course should check course for if it is a pl course' do
@@ -215,4 +225,68 @@ class CourseTypesTests < ActiveSupport::TestCase
     refute @course_universal_instructor_to_teacher.can_be_participant?(nil)
     refute @course_plc_reviewer_to_facilitator.can_be_participant?(nil)
   end
+
+  test 'get_course_family_name should get family name from UnitGroup if called for Unit in UnitGroup' do
+    assert_equal @unit_in_course_2.get_course_family_name, 'teacher-unit-groups'
+  end
+
+  test 'get_course_family_name should get family name from Unit if called for Unit that is not in UnitGroup' do
+    assert_equal @unit_group_2.get_course_family_name, 'teacher-unit-groups'
+  end
+
+  test 'get_course_family_name should get family name from UnitGroup if called for UnitGroup' do
+    assert_equal @unit_teacher_to_students_2.get_course_family_name, 'teacher-units'
+  end
+
+  test 'get_family_courses should get all UnitGroups with the same family name if called for Unit in UnitGroup' do
+    assert_equal @unit_in_course_2.get_family_courses.map(&:name), ['course-instructed-by-teacher', 'course-instructed-by-teacher-2', 'course-teacher-to-student']
+  end
+
+  test 'get_family_courses should get all UnitGroups with the same family name if called for UnitGroup' do
+    assert_equal @unit_group_2.get_family_courses.map(&:name), ['course-instructed-by-teacher', 'course-instructed-by-teacher-2', 'course-teacher-to-student']
+  end
+
+  test 'get_family_courses should get all Unit with the same family name if called for Unit not in UnitGroup' do
+    assert_equal @unit_teacher_to_students_2.get_family_courses.map(&:name), ['unit-teacher-to-student', 'unit-teacher-to-student-2']
+  end
+
+  test 'get_family_courses should return nil if there is no family name' do
+    unit_without_family_name = create :script, name: 'no-family-name'
+    assert_equal unit_without_family_name.get_family_courses, nil
+  end
+
+  test 'should raise error if instruction type does not match rest of course family' do
+    error = assert_raises do
+      @unit_group_2.instruction_type = SharedCourseConstants::INSTRUCTION_TYPE.self_paced
+      @unit_group_2.save!
+    end
+
+    assert_includes error.message, 'Instruction type must be the same for all courses in a family.'
+  end
+
+  test 'should raise error if instructor audience does not match rest of course family' do
+    error = assert_raises do
+      @unit_group_2.instructor_audience = SharedCourseConstants::INSTRUCTOR_AUDIENCE.facilitator
+      @unit_group_2.save!
+    end
+
+    assert_includes error.message, 'Instructor audience must be the same for all courses in a family.'
+  end
+
+  test 'should raise error if participant audience does not match rest of course family' do
+    error = assert_raises do
+      @unit_group_2.participant_audience = SharedCourseConstants::PARTICIPANT_AUDIENCE.facilitator
+      @unit_group_2.save!
+    end
+
+    assert_includes error.message, 'Participant audience must be the same for all courses in a family.'
+  end
+
+  test 'should not raise error when changing course type values for a course that is the only one in its family' do
+    solo_unit_in_family_name = create :script, name: 'solo-family-name', family_name: 'solo-family-name'
+    assert_nothing_raised do
+      solo_unit_in_family_name.participant_audience = SharedCourseConstants::PARTICIPANT_AUDIENCE.facilitator
+      solo_unit_in_family_name.save!
+    end
+  end
 end