Fix duplicate subject validation on secondary course selection

  - Move subjects duplicate validation to the model
  - Add subordinate_subject_id attribute to Course
  - When form is invalid and rerenders the selected values are still
    selected on the subordinate_subject_id select

app/controllers/publish/courses/subjects_controller.rb

@@ -91,14 +91,13 @@ def selected_master
       end
 
       def selected_subordinate
-        @selected_subordinate ||= params[:course][:subordinate_subject_id] if params[:course][:subordinate_subject_id].present?
+        @selected_subordinate ||= 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

@@ -255,10 +255,6 @@ 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

app/models/course.rb

@@ -18,6 +18,8 @@ class Course < ApplicationRecord
   has_associated_audits
   audited
 
+  attribute :subordinate_subject_id, :integer
+  validate :duplicate_subjects
   validates :course_code,
             uniqueness: { scope: :provider_id },
             presence: true,
@@ -1055,4 +1057,8 @@ def accredited_provider_exists_in_current_cycle
 
     errors.add(:accrediting_provider, :does_not_exist_in_cycle, accredited_provider_code:) unless RecruitmentCycle.current.providers.exists?(provider_code: accredited_provider_code)
   end
+
+  def duplicate_subjects
+    errors.add(:subjects, :duplicate) if subject_ids.uniq.count != subject_ids.count
+  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_subjects_service.rb

@@ -12,8 +12,6 @@ def initialize(course:, subject_ids:)
     end
 
     def call
-      course.errors.add(:subjects, :duplicate) if request_has_duplicate_subject_ids?
-
       update_subjects
 
       if course.persisted?
@@ -28,7 +26,7 @@ def call
     private
 
     def subjects
-      @subjects ||= Subject.find(@subject_ids)
+      @subjects ||= @subject_ids.map { |id| Subject.find(id) }
     end
 
     def update_subjects
@@ -47,10 +45,6 @@ def update_subjects
       end
     end
 
-    def request_has_duplicate_subject_ids?
-      subject_ids.uniq.count != subject_ids.count
-    end
-
     def update_further_education_fields
       course.funding_type = 'fee'
       course.english = 'not_required'

config/analytics_blocklist.yml

@@ -70,6 +70,7 @@
   - searchable
   :course:
   - master_subject_id
+  - subordinate_subject_id
   - campaign_name
   :gias_school:
   - searchable

config/locales/en.yml

@@ -494,7 +494,7 @@ en:
             subjects:
               blank: "^There is a problem with this course. Contact support to fix it (Error: S)"
               course_creation: "^Select a subject"
-              duplicate: "^You have already selected this subject. You can only select a subject once"
+              duplicate: "^You can only select a subject once"
             modern_languages_subjects:
               select_a_language: "^Select at least one language"
             study_mode:

spec/features/publish/courses/new_primary_subject_spec.rb

@@ -0,0 +1,63 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+feature 'selecting a subject', { can_edit_current_and_next_cycles: false } do
+  before do
+    given_i_am_authenticated_as_a_provider_user
+  end
+
+  scenario 'selecting a subject' do
+    when_i_visit_the_new_primary_course_subject_page
+    when_i_select_a_primary_subject('Primary with English')
+    and_i_click_continue
+    then_i_am_met_with_the_age_range_page
+  end
+
+  scenario 'invalid entries' do
+    when_i_visit_the_new_primary_course_subject_page
+    and_i_click_continue
+    then_i_am_met_with_errors
+  end
+
+  private
+
+  def given_i_am_authenticated_as_a_provider_user
+    @user = create(:user, :with_provider)
+    given_i_am_authenticated(user: @user)
+  end
+
+  def when_i_visit_the_new_primary_course_subject_page
+    publish_courses_new_subjects_page.load(provider_code: provider.provider_code, recruitment_cycle_year: Settings.current_recruitment_cycle_year, query: primary_subject_params)
+  end
+
+  def when_i_select_a_primary_subject(subject_type)
+    publish_courses_new_subjects_page.choose(subject_type)
+  end
+
+  def and_i_click_continue
+    publish_courses_new_subjects_page.continue.click
+  end
+
+  def provider
+    @provider ||= @user.providers.first
+  end
+
+  def then_i_am_met_with_the_age_range_page
+    expect(page).to have_current_path("/publish/organisations/#{provider.provider_code}/#{Settings.current_recruitment_cycle_year}/courses/age-range/new?#{params_with_subject}")
+    expect(page).to have_content('Age range')
+  end
+
+  def then_i_am_met_with_errors
+    expect(page).to have_content('There is a problem')
+    expect(page).to have_content('Select a subject')
+  end
+
+  def primary_subject
+    find_or_create(:primary_subject, :primary_with_english)
+  end
+
+  def params_with_subject
+    "course%5Bcampaign_name%5D=&course%5Bis_send%5D=0&course%5Blevel%5D=primary&course%5Bmaster_subject_id%5D=#{primary_subject.id}&course%5Bsubjects_ids%5D%5B%5D=#{primary_subject.id}"
+  end
+end

spec/features/publish/courses/new_secondary_subject_spec.rb

@@ -0,0 +1,123 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+feature 'selecting a subject', { can_edit_current_and_next_cycles: false } do
+  before do
+    given_i_am_authenticated_as_a_provider_user
+  end
+
+  scenario 'selecting one subject' do
+    when_i_visit_the_new_course_subject_page
+    when_i_select_one_subject(:business_studies)
+    and_i_click_continue
+    then_i_am_met_with_the_age_range_page(:business_studies)
+  end
+
+  scenario 'selecting two subjects' do
+    when_i_visit_the_new_course_subject_page
+    when_i_select_two_subjects(:business_studies, :physics)
+    and_i_click_continue
+    then_i_am_met_with_the_age_range_page(:business_studies, :physics)
+  end
+
+  scenario 'selecting secondary subject modern languages' do
+    when_i_visit_the_new_course_subject_page
+    when_i_select_one_subject(:modern_languages)
+    and_i_click_continue
+    then_i_am_met_with_the_modern_languages_page
+  end
+
+  scenario 'selecting duplicate first and second subject' do
+    when_i_visit_the_new_course_subject_page
+    when_i_select_two_subjects(:business_studies, :business_studies)
+    and_i_click_continue
+    expect(page).to have_content('You can only select a subject once')
+    expect(publish_courses_new_subjects_page.subordinate_subjects_fields.find('option[selected]')).to have_text('Business studies')
+  end
+
+  scenario 'invalid entries' do
+    when_i_visit_the_new_course_subject_page
+    and_i_click_continue
+    then_i_am_met_with_errors
+  end
+
+  private
+
+  def given_i_am_authenticated_as_a_provider_user
+    @user = create(:user, :with_provider)
+    given_i_am_authenticated(user: @user)
+  end
+
+  def when_i_visit_the_new_course_subject_page
+    publish_courses_new_subjects_page.load(provider_code: provider.provider_code, recruitment_cycle_year: Settings.current_recruitment_cycle_year, query: secondary_subject_params)
+  end
+
+  def when_i_select_one_subject(subject_type)
+    publish_courses_new_subjects_page.master_subject_fields.select(course_subject(subject_type).subject_name).click
+  end
+
+  def when_i_select_two_subjects(master, subordinate)
+    publish_courses_new_subjects_page.master_subject_fields.select(course_subject(master).subject_name).click
+    publish_courses_new_subjects_page.subordinate_subjects_fields.select(course_subject(subordinate).subject_name).click
+  end
+
+  def and_i_click_continue
+    publish_courses_new_subjects_page.continue.click
+  end
+
+  def provider
+    @provider ||= @user.providers.first
+  end
+
+  def then_i_am_met_with_the_age_range_page(master, subordinate = nil)
+    expect(page).to have_current_path("/publish/organisations/#{provider.provider_code}/#{Settings.current_recruitment_cycle_year}/courses/age-range/new?#{params_with_subject(master, subordinate)}")
+    expect(page).to have_content('Age range')
+  end
+
+  def then_i_am_met_with_the_modern_languages_page
+    expect(page).to have_current_path("/publish/organisations/#{provider.provider_code}/#{Settings.current_recruitment_cycle_year}/courses/modern-languages/new?#{params_with_subject(:modern_languages)}")
+    expect(page).to have_content('Languages')
+  end
+
+  def then_i_am_met_with_errors
+    expect(page).to have_content('There is a problem')
+    expect(page).to have_content('Select a subject')
+  end
+
+  def course_subject(subject_type)
+    case subject_type
+    when :business_studies
+      find_or_create(:secondary_subject, :business_studies)
+    when :physics
+      find_or_create(:secondary_subject, :physics)
+    when :modern_languages
+      find_or_create(:secondary_subject, :modern_languages)
+    end
+  end
+
+  def params_with_subject(master_subject, subordinate_subject = nil)
+    master_subject = course_subject(master_subject)
+    subordinate_subject = course_subject(subordinate_subject)
+    if subordinate_subject
+      [
+        'course%5Bcampaign_name%5D=',
+        'course%5Bis_send%5D=0',
+        'course%5Blevel%5D=secondary',
+        "course%5Bmaster_subject_id%5D=#{master_subject.id}",
+        "course%5Bsubjects_ids%5D%5B%5D=#{master_subject.id}",
+        "course%5Bsubjects_ids%5D%5B%5D=#{subordinate_subject.id}",
+        "course%5Bsubordinate_subject_id%5D=#{subordinate_subject.id}"
+      ].join('&')
+    else
+      [
+        'course%5Bcampaign_name%5D=',
+        'course%5Bis_send%5D=0',
+        'course%5Blevel%5D=secondary',
+        "course%5Bmaster_subject_id%5D=#{master_subject.id}",
+        "course%5Bsubjects_ids%5D%5B%5D=#{master_subject.id}",
+        'course%5Bsubordinate_subject_id%5D='
+      ].join('&')
+    end
+  end
+end