Fix duplicate subject validation on secondary course selection

app/controllers/publish/courses/applications_open_controller.rb

@@ -36,7 +36,8 @@ def actual_params
                 :skip_languages_goto_confirmation,
                 :accredited_provider_code,
                 :campaign_name,
-                :master_subject_id
+                :master_subject_id,
+                :subordinate_subject_id
               )
               .permit(
                 :start_date,

app/controllers/publish/courses/modern_languages_controller.rb

@@ -22,7 +22,7 @@ def continue
       def edit
         authorize(provider)
 
-        return if selected_non_language_subjects_ids.include? modern_languages_subject_id.to_s
+        return if selected_non_language_subjects_ids.include? modern_languages_subject_id
 
         redirect_to(
           details_publish_provider_recruitment_cycle_course_path(
@@ -67,7 +67,7 @@ def current_step
       private
 
       def updated_subject_list
-        @updated_subject_list ||= selected_language_subjects_ids.concat(selected_non_language_subjects_ids)
+        @updated_subject_list ||= selected_non_language_subjects_ids.concat(selected_language_subjects_ids)
       end
 
       def course_subjects_form
@@ -87,9 +87,7 @@ def selected_subjects(param_key)
 
         ids = params.dig(:course, param_key)&.map(&:to_i) || []
 
-        @course.edit_course_options[edit_course_options_key].filter_map do |subject|
-          subject.id.to_s if ids.include?(subject.id)
-        end
+        ids.intersection(@course.edit_course_options[edit_course_options_key].map(&:id))
       end
 
       def selected_language_subjects_ids
@@ -101,7 +99,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/controllers/publish/courses/subjects_controller.rb

@@ -18,56 +18,71 @@ def continue
 
       def update
         authorize(provider)
-        if params[:course][:master_subject_id] == SecondarySubject.physics.id.to_s
-          course.update(master_subject_id: params[:course][:master_subject_id])
-          redirect_to(
-            engineers_teach_physics_publish_provider_recruitment_cycle_course_path(
-              @course.provider_code,
-              @course.recruitment_cycle_year,
-              @course.course_code,
-              course: { master_subject_id: SecondarySubject.physics.id.to_s, subjects_ids: selected_subject_ids }
+        if validate_subject_ids
+          if params[:course][:master_subject_id] == SecondarySubject.physics.id.to_s
+            course.update(master_subject_id: params[:course][:master_subject_id])
+            redirect_to(
+              engineers_teach_physics_publish_provider_recruitment_cycle_course_path(
+                @course.provider_code,
+                @course.recruitment_cycle_year,
+                @course.course_code,
+                course: { master_subject_id: SecondarySubject.physics.id.to_s, subjects_ids: selected_subject_ids }
+              )
             )
-          )
-
-        elsif selected_subject_ids.include?(modern_languages_subject_id.to_s)
-          course.update(master_subject_id: params[:course][:master_subject_id])
-          redirect_to(
-            modern_languages_publish_provider_recruitment_cycle_course_path(
-              @course.provider_code,
-              @course.recruitment_cycle_year,
-              @course.course_code,
-              course: { subjects_ids: selected_subject_ids }
+
+          elsif selected_subject_ids.include?(modern_languages_subject_id.to_s)
+            course.update(master_subject_id: params[:course][:master_subject_id])
+            redirect_to(
+              modern_languages_publish_provider_recruitment_cycle_course_path(
+                @course.provider_code,
+                @course.recruitment_cycle_year,
+                @course.course_code,
+                course: { subjects_ids: selected_subject_ids }
+              )
             )
-          )
-
-        elsif course_subjects_form.save!
-          course_updated_message(section_key)
-          # TODO: move this to the form?
-          course.update(master_subject_id: params[:course][:master_subject_id])
-          course.update(name: course.generate_name)
-          course.update(campaign_name: nil) unless course.master_subject_id == SecondarySubject.physics.id
-
-          redirect_to(
-            details_publish_provider_recruitment_cycle_course_path(
-              @course.provider_code,
-              @course.recruitment_cycle_year,
-              @course.course_code
+
+          elsif course.errors.none? && course_subjects_form.save!
+            course_updated_message(section_key)
+            # TODO: move this to the form?
+            course.update(master_subject_id: params[:course][:master_subject_id])
+            course.update(name: course.generate_name)
+            course.update(campaign_name: nil) unless course.master_subject_id == SecondarySubject.physics.id
+
+            redirect_to(
+              details_publish_provider_recruitment_cycle_course_path(
+                @course.provider_code,
+                @course.recruitment_cycle_year,
+                @course.course_code
+              )
             )
-          )
+          end
         else
           @errors = @course.errors.messages
+          course.master_subject_id = selected_master
+          course.subordinate_subject_id = selected_subordinate
           render :edit
         end
       end
 
       private
 
+      def validate_subject_ids
+        if selected_master.blank?
+          course.errors.add(:subjects, :course_creation)
+          return false
+        elsif selected_master == selected_subordinate
+          course.errors.add(:subjects, :duplicate)
+          return false
+        end
+        true
+      end
+
       def campaign_name_check
         params[:course][:campaign_name] = '' unless @course.master_subject_id == SecondarySubject.physics.id
       end
 
       def course_subjects_form
-        @course_subjects_form ||= CourseSubjectsForm.new(@course, params: selected_subject_ids)
+        @course_subjects_form ||= CourseSubjectsForm.new(@course, params: [selected_master, selected_subordinate])
       end
 
       def modern_languages_subject_id
@@ -87,18 +102,17 @@ def error_keys
       end
 
       def selected_master
-        @selected_master ||= params[:course][:master_subject_id] if params[:course][:master_subject_id].present?
+        params[:course][:master_subject_id].presence
       end
 
       def selected_subordinate
-        @selected_subordinate ||= params[:course][:subordinate_subject_id] if params[:course][:subordinate_subject_id].present?
+        params[:course][:subordinate_subject_id].presence
       end
 
       def build_course_params
         previous_subject_selections = params[:course][:subjects_ids]
 
         params[:course][:subjects_ids] = selected_subject_ids
-        params[:course].delete(:subordinate_subject_id)
 
         build_new_course # to get languages edit_options
 

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 { |cs| LANGUAGE_SUBJECT_CODES.include?(cs.subject.subject_code) ? cs.subject.subject_name : cs.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,7 +143,7 @@ def apprenticeship
   end
 
   def sorted_subjects
-    object.subjects.map(&:subject_name).sort.join('<br>').html_safe
+    object.course_subjects.map { |cs| cs.subject.subject_name }.join('<br>').html_safe
   end
 
   def chosen_subjects
@@ -255,13 +255,9 @@ def selected_subject_ids
     selectable_subject_ids & selected_subject_ids
   end
 
-  def subordinate_subject_id
-    selected_subject_ids - [master_subject_id] if master_subject_id
-  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
 
@@ -481,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,11 +489,11 @@ def main_subject_is_modern_languages?
   end
 
   def main_subject
-    Subject.find(course.master_subject_id)
+    @main_subject ||= Subject.find(course.master_subject_id)
   end
 
   def additional_subjects
-    object.subjects.reject { |subject| subject.id == main_subject.id }
+    object.course_subjects.reject { |subject| subject.subject_id == main_subject.id }.map(&:subject)
   end
 
   def format_name(subjects)

app/forms/publish/course_subjects_form.rb

@@ -5,7 +5,7 @@ class CourseSubjectsForm < BaseCourseForm
     alias subject_ids params
 
     def initialize(model, params: {})
-      @previous_subject_names = model.subjects.map(&:subject_name)
+      @previous_subject_names = model.course_subjects.map { |cs| cs.subject.subject_name }
       @previous_course_name = model.name
       super
     end
@@ -15,7 +15,7 @@ def initialize(model, params: {})
     attr_reader :previous_subject_names, :previous_course_name
 
     def valid?
-      super && assign_subjects_service.valid?
+      super && assign_subjects_service.errors.none?
     end
 
     def compute_fields

app/models/course.rb

@@ -18,6 +18,19 @@ class Course < ApplicationRecord
   has_associated_audits
   audited
 
+  attribute :subordinate_subject_id, :integer
+  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 },
             presence: true,
@@ -566,7 +579,7 @@ def publish_enrichment(current_user)
   end
 
   def is_modern_language_course?
-    subjects.any? { |s| s == SecondarySubject.modern_languages }
+    course_subjects.any? { |cs| cs.subject == SecondarySubject.modern_languages }
   end
 
   def sites=(desired_sites)
@@ -795,15 +808,19 @@ def remove_carat_from_error_messages
   end
 
   def modern_language_subjects
-    subjects.where(type: 'ModernLanguagesSubject')
+    if persisted? && course_subjects.all?(&:persisted?)
+      subjects.where(type: 'ModernLanguagesSubject')
+    else
+      course_subjects.select { |cs| cs.subject.type == 'ModernLanguagesSubject' }.map(&:subject)
+    end
   end
 
   def master_subject_nil?
     master_subject_id.nil?
   end
 
   def has_any_modern_language_subject_type?
-    subjects.any? { |subject| subject.type == 'ModernLanguagesSubject' }
+    course_subjects.any? { |cs| cs.subject.type == 'ModernLanguagesSubject' }
   end
 
   def current_published_enrichment
@@ -990,24 +1007,24 @@ 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 }
+    course_subjects.any? { |cs| cs.subject&.id == SecondarySubject.modern_languages.id }
   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?
       errors.add(:subjects, :course_creation)
       return
     end
 
     case level
     when 'primary', 'further_education'
-      errors.add(:subjects, 'has too many subjects') if subjects.count > 1
+      errors.add(:subjects, 'has too many subjects') if course_subjects.count > 1
     when 'secondary'
-      errors.add(:subjects, 'has too many subjects') if subjects.count > 2 && !has_any_modern_language_subject_type?
+      errors.add(:subjects, 'has too many subjects') if course_subjects.count > 2 && !has_any_modern_language_subject_type?
     end
   end
 

app/policies/course_policy.rb

@@ -73,6 +73,7 @@ def permitted_new_course_attributes
       can_sponsor_skilled_worker_visa
       campaign_name
       master_subject_id
+      subordinate_subject_id
       subjects_ids
     ]
   end

app/services/courses/assign_program_type_service.rb

@@ -17,8 +17,6 @@ def execute(funding_type, course)
       when 'fee'
         course.program_type = calculate_fee_program(course)
       end
-      # NOTE: This looks like unwarranted side effects
-      course.save
     end
 
     private