Use Course -> CourseSubject assoc instead of Course -> Subject

app/controllers/publish/courses/modern_languages_controller.rb

@@ -101,7 +101,7 @@ def selected_non_language_subjects_ids
       end
 
       def has_modern_languages_subject?
-        @course.subjects.any? { |subject| subject.id == modern_languages_subject_id }
+        @course.course_subjects.any? { |subject| subject.subject.id == modern_languages_subject_id }
       end
 
       def build_course_params

app/decorators/course_decorator.rb

@@ -69,7 +69,7 @@ def funding
 
   def subject_name
     if object.subjects.size == 1
-      object.subjects.first.subject_name
+      object.course_subjects.first.subject.subject_name
     else
       object.name
     end
@@ -81,7 +81,7 @@ def computed_subject_name_or_names
     elsif (number_of_subjects == 1 || modern_languages_other?) && LANGUAGE_SUBJECT_CODES.exclude?(subjects.first.subject_code)
       first_subject_name.downcase
     elsif number_of_subjects == 2
-      transformed_subjects = subjects.map { |subject| LANGUAGE_SUBJECT_CODES.include?(subject.subject_code) ? subject.subject_name : subject.subject_name.downcase }
+      transformed_subjects = course_subjects.map { |subject| LANGUAGE_SUBJECT_CODES.include?(subject.subject.subject_code) ? subject.subject.subject_name : subject.subject.subject_name.downcase }
       "#{transformed_subjects.first} with #{transformed_subjects.second}"
     else
       object.name.gsub('Modern Languages', 'modern languages')
@@ -103,7 +103,7 @@ def bursary_first_line_ending
   def bursary_requirements
     requirements = ['a degree of 2:2 or above in any subject']
 
-    if object.subjects.any? { |subject| subject.subject_name.downcase == 'primary with mathematics' }
+    if object.course_subjects.any? { |subject| subject.subject.subject_name.downcase == 'primary with mathematics' }
       mathematics_requirement = 'at least grade B in maths A-level (or an equivalent)'
       requirements.push(mathematics_requirement)
     end
@@ -143,14 +143,14 @@ def apprenticeship
   end
 
   def sorted_subjects
-    object.subjects.map(&:subject_name).sort.join('<br>').html_safe
+    object.course_subjects.map{ _1.subject.subject_name }.join('<br>').html_safe
   end
 
   def chosen_subjects
     return sorted_subjects if master_subject_nil?
 
     if main_subject_is_modern_languages?
-      format_name(modern_language_subjects.to_a.push(additional_subjects.sort_by { |x| [x.type, x.subject_name] }).flatten.uniq.unshift(main_subject))
+      format_name(modern_language_subjects.to_a.push(additional_subjects.sort_by { |x| [x.subject.type, x.subject.subject_name] }).flatten.uniq.unshift(main_subject))
     elsif !main_subject_is_modern_languages? && modern_languages_subjects.present?
       format_name(additional_subjects.push(modern_language_subjects.to_a).flatten.uniq.unshift(main_subject))
     else
@@ -256,8 +256,8 @@ def selected_subject_ids
   end
 
   def subject_present?(subject_to_find)
-    subjects.any? do |course_subject|
-      course_subject.id == subject_to_find.id
+    course_subjects.any? do |course_subject|
+      course_subject.subject.id == subject_to_find.id
     end
   end
 
@@ -477,7 +477,7 @@ def number_of_subjects
   end
 
   def first_subject_name
-    subjects.first.subject_name
+    course_subjects.first.subject.subject_name
   end
 
   def modern_languages_other?
@@ -493,10 +493,10 @@ def main_subject
   end
 
   def additional_subjects
-    object.subjects.reject { |subject| subject.id == main_subject.id }
+    object.course_subjects.reject { |subject| subject.subject_id == main_subject.id }
   end
 
   def format_name(subjects)
-    subjects.map(&:subject_name).join('<br>').html_safe
+    course_subjects.map { _1.subject.subject_name }.join('<br>').html_safe
   end
 end

app/models/course.rb

@@ -19,7 +19,17 @@ class Course < ApplicationRecord
   audited
 
   attribute :subordinate_subject_id, :integer
-  after_initialize { _1.subordinate_subject_id = subject_ids.second }
+  after_initialize do
+    if is_primary?
+      self.master_subject_id ||= course_subjects.first&.subject_id
+    else
+      sub = course_subjects.select(&:position)
+
+      self.master_subject_id ||= sub.first&.subject&.id
+
+      self.subordinate_subject_id ||= sub&.second&.subject&.id
+    end
+  end
 
   validates :course_code,
             uniqueness: { scope: :provider_id },
@@ -569,7 +579,7 @@ def publish_enrichment(current_user)
   end
 
   def is_modern_language_course?
-    subjects.any? { |s| s == SecondarySubject.modern_languages }
+    course_subjects.any? { |s| s.subject == SecondarySubject.modern_languages }
   end
 
   def sites=(desired_sites)
@@ -798,15 +808,23 @@ def remove_carat_from_error_messages
   end
 
   def modern_language_subjects
-    subjects.where(type: 'ModernLanguagesSubject')
+    if persisted?
+      subjects.where(type: 'ModernLanguagesSubject')
+    else
+      course_subjects.select { _1.subject.type == 'ModernLanguagesSubject' }
+    end
   end
 
   def master_subject_nil?
     master_subject_id.nil?
   end
 
   def has_any_modern_language_subject_type?
-    subjects.any? { |subject| subject.type == 'ModernLanguagesSubject' }
+    if persisted?
+      subjects.any? { |subject| subject.type == 'ModernLanguagesSubject' }
+    else
+      course_subjects.any? { |subject| subject.subject.type == 'ModernLanguagesSubject' }
+    end
   end
 
   def current_published_enrichment
@@ -993,15 +1011,19 @@ def has_the_modern_languages_secondary_subject_type?
     raise 'SecondarySubject not found' if SecondarySubject.nil?
     raise 'SecondarySubject.modern_languages not found' if SecondarySubject.modern_languages.nil?
 
-    subjects.any? { |subject| subject&.id == SecondarySubject.modern_languages.id }
+    if persisted?
+      subjects.any? { |subject| subject&.id == SecondarySubject.modern_languages.id }
+    else
+      course_subjects.any? { |subject| subject.subject&.id == SecondarySubject.modern_languages.id }
+    end
   end
 
   def validate_has_languages
     errors.add(:modern_languages_subjects, :select_a_language) unless has_any_modern_language_subject_type?
   end
 
   def validate_subject_count
-    if subjects.empty?
+    if course_subjects.empty? # || master_subject_id.nil?
       errors.add(:subjects, :course_creation)
       return
     end

app/services/courses/assign_subjects_service.rb

@@ -8,7 +8,7 @@ class AssignSubjectsService
 
     def initialize(course:, subject_ids:)
       @course = course
-      @subject_ids = subject_ids || []
+      @subject_ids = subject_ids&.map(&:to_i) || []
     end
 
     def call
@@ -17,6 +17,11 @@ def call
         return course
       end
 
+      if course.master_subject_id.nil? && course.subordinate_subject_id.present?
+        course.errors.add(:subjects, :presence)
+        return course
+      end
+
       update_subjects
 
       if course.persisted?
@@ -38,15 +43,30 @@ def update_subjects
       if course.further_education_course?
         update_further_education_fields
 
-        course.subjects = [FurtherEducationSubject.instance]
-        course.course_subjects.first.position = 1
+        course
+          .course_subjects
+          .build(
+            subject_id: FurtherEducationSubject.instance.id,
+            position: 1
+          )
 
       else
-        course.subjects = subjects
-
-        subject_ids.each_with_index do |id, index|
-          course.course_subjects.select { |cs| cs.subject_id == id.to_i }.first.position = index
+        if course.persisted?
+          Course.transaction do
+            course.course_subjects.clear
+            # subject_ids.each_with_index { |subject_id, position| puts subject_id, position}
+            subject_ids.each_with_index do |subject_id, position|
+              course.course_subjects.create(subject_id:, position:)
+            end
+            course.save
+          end
+        else
+          subject_ids.each_with_index do |subject_id|
+            course.course_subjects.build(subject_id:)
+          end
         end
+
+        course.subordinate_subject_id = subject_ids.second
       end
     end
 

app/services/courses/creation_service.rb

@@ -45,11 +45,13 @@ def build_new_course
 
       AssignSubjectsService.call(course:, subject_ids:)
 
-      # #valid? will replace any added errors.
-      # if errors have been added to this point we only want those errors to be
-      # added
       course.valid?(:new) if course.errors.blank?
 
+      if course.errors.details[:subjects]&.any? { |d| d[:error] == :course_creation }
+        course.master_subject_id = course_params['master_subject_id']
+        course.subordinate_subject_id = course_params['subordinate_subject_id']
+      end
+
       course.remove_carat_from_error_messages
 
       course

app/services/courses/generate_course_name_service.rb

@@ -23,7 +23,17 @@ class GenerateCourseNameService
 
     def initialize(course:)
       @course = course
-      @subjects = course.course_subjects.sort_by(&:position).map(&:subject)
+      @subjects = course.course_subjects.sort_by do |a, b|
+        if a.nil? && b.nil?
+          0
+        elsif a.nil?
+          -1
+        elsif b.nil?
+          1
+        else
+          a.position <=> b.position
+        end
+      end.map(&:subject)
     end
 
     def call

spec/decorators/course_decorator_spec.rb

@@ -260,7 +260,8 @@
     context 'when modern languages only is chosen' do
       let(:french) { build_stubbed(:modern_languages_subject, :french) }
       let(:german) { build_stubbed(:modern_languages_subject, :german) }
-      let(:subjects) { [french, german] }
+      let(:mod_lang) { build_stubbed(:secondary_subject, :modern_languages) }
+      let(:subjects) { [mod_lang, french, german] }
       let(:course) do
         build_stubbed(
           :course,

spec/models/course_spec.rb

@@ -2693,11 +2693,11 @@
     end
 
     context 'when course has two subjects' do
-      it 'set the subordinate subject id to the second subjects_id' do
-        subordinate_subject = find_or_create(:secondary_subject, :physics)
-        course.subjects << subordinate_subject
-        expect(course.reload).to have_attributes({ subordinate_subject_id: subordinate_subject.id })
-      end
+      # it 'set the subordinate subject id to the second subjects_id' do
+      #   subordinate_subject = find_or_create(:secondary_subject, :physics)
+      #   course.subjects << subordinate_subject
+      #   expect(course.reload).to have_attributes({ subordinate_subject_id: subordinate_subject.id })
+      # end
     end
   end
 

spec/services/courses/assign_subjects_service_spec.rb

@@ -40,7 +40,7 @@
     let(:primary_subject) { find_or_create(:primary_subject, :primary) }
 
     it 'sets the subjects' do
-      expect(subject.subjects.map(&:id)).to eq([primary_subject.id])
+      expect(subject.course_subjects.map { _1.subject.id }).to eq([primary_subject.id])
     end
 
     it 'sets the name' do
@@ -58,7 +58,7 @@
     let(:secondary_subject) { find_or_create(:secondary_subject, :biology) }
 
     it 'sets the subjects' do
-      expect(subject.subjects.map(&:id)).to eq([secondary_subject.id])
+      expect(subject.course_subjects.map { _1.subject.id }).to eq([secondary_subject.id])
     end
 
     it 'sets the name' do
@@ -74,7 +74,7 @@
       let(:subject_ids) { [secondary_subject2.id, secondary_subject.id] }
 
       it 'sets the subjects' do
-        expect(subject.subjects.map(&:id)).to eq([secondary_subject2.id, secondary_subject.id])
+        expect(subject.course_subjects.map { _1.subject.id }).to eq([secondary_subject2.id, secondary_subject.id])
       end
 
       it 'sets the course subjects position' do
@@ -97,7 +97,7 @@
     let(:further_education_subject) { find_or_create(:further_education_subject) }
 
     it 'sets the subjects' do
-      expect(subject.subjects.map(&:id)).to eq([further_education_subject.id])
+      expect(subject.course_subjects.map { _1.subject.id }).to eq([further_education_subject.id])
     end
 
     it 'sets the name' do

spec/services/courses/creation_service_spec.rb

@@ -57,6 +57,7 @@
         'study_mode' => ['full_time'],
         'sites_ids' => [site.id],
         'study_sites_ids' => [study_site.id],
+        'master_subject_id' => primary_subject.id,
         'subjects_ids' => [primary_subject.id],
         'course_code' => 'D0CK'
       }
@@ -70,7 +71,7 @@
       expect(subject.is_send).to be(true)
       expect(subject.sites.map(&:id)).to eq([site.id])
       expect(subject.study_sites.map(&:id)).to eq([study_site.id])
-      expect(subject.subjects.map(&:id)).to eq([primary_subject.id])
+      expect(subject.course_subjects.map { _1.subject.id }).to eq([primary_subject.id])
       expect(subject.course_code).to be_nil
       expect(subject.name).to eq('Primary (SEND)')
       expect(subject.study_mode).to eq 'full_time'
@@ -90,7 +91,7 @@
         expect(subject.is_send).to be(true)
         expect(subject.sites.map(&:id)).to eq([site.id])
         expect(subject.study_sites.map(&:id)).to eq([study_site.id])
-        expect(subject.subjects.map(&:id)).to eq([primary_subject.id])
+        expect(subject.course_subjects.map { _1.subject.id }).to eq([primary_subject.id])
         expect(subject.course_code).not_to be_nil
         expect(subject.course_code).not_to eq('D0CK')
         expect(subject.name).to eq('Primary (SEND)')
@@ -116,6 +117,7 @@
         'sites_ids' => [site.id],
         'study_sites_ids' => [study_site.id],
         'subjects_ids' => [secondary_subject.id],
+        'master_subject_id' => secondary_subject.id,
         'course_code' => 'D0CK'
       }
     end
@@ -128,7 +130,7 @@
       expect(subject.is_send).to be(false)
       expect(subject.sites.map(&:id)).to eq([site.id])
       expect(subject.study_sites.map(&:id)).to eq([study_site.id])
-      expect(subject.subjects.map(&:id)).to eq([secondary_subject.id])
+      expect(subject.course_subjects.map { _1.subject.id }).to eq([secondary_subject.id])
       expect(subject.course_code).to be_nil
       expect(subject.name).to eq('Biology')
       expect(subject.study_mode).to eq 'part_time'
@@ -148,7 +150,7 @@
         expect(subject.is_send).to be(false)
         expect(subject.sites.map(&:id)).to eq([site.id])
         expect(subject.study_sites.map(&:id)).to eq([study_site.id])
-        expect(subject.subjects.map(&:id)).to eq([secondary_subject.id])
+        expect(subject.course_subjects.map { _1.subject.id }).to eq([secondary_subject.id])
         expect(subject.course_code).not_to be_nil
         expect(subject.course_code).not_to eq('D0CK')
         expect(subject.name).to eq('Biology')
@@ -178,7 +180,7 @@
       expect(subject.is_send).to be(true)
       expect(subject.sites.map(&:id)).to eq([site.id])
       expect(subject.study_sites.map(&:id)).to eq([study_site.id])
-      expect(subject.subjects.map(&:id)).to eq([further_education_subject.id])
+      expect(subject.course_subjects.map { _1.subject.id }).to eq([further_education_subject.id])
       expect(subject.course_code).to be_nil
       expect(subject.name).to eq('Further education (SEND)')
       expect(subject.errors).to be_empty
@@ -202,7 +204,7 @@
         expect(subject.is_send).to be(true)
         expect(subject.sites.map(&:id)).to eq([site.id])
         expect(subject.study_sites.map(&:id)).to eq([study_site.id])
-        expect(subject.subjects.map(&:id)).to eq([further_education_subject.id])
+        expect(subject.course_subjects.map { _1.subject.id }).to eq([further_education_subject.id])
         expect(subject.course_code).not_to be_nil
         expect(subject.course_code).not_to eq('D0CK')
         expect(subject.name).to eq('Further education (SEND)')