From b356dc507f0da38e54c5f8c5a80af66e5d9f01ea Mon Sep 17 00:00:00 2001 From: Iain McNulty Date: Wed, 22 May 2024 09:44:40 +0100 Subject: [PATCH] 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 --- .../publish/courses/subjects_controller.rb | 3 +- app/decorators/course_decorator.rb | 4 - app/models/course.rb | 6 + app/policies/course_policy.rb | 1 + .../courses/assign_subjects_service.rb | 8 +- config/analytics_blocklist.yml | 1 + config/locales/en.yml | 2 +- .../courses/new_primary_subject_spec.rb | 63 +++++++++ .../courses/new_secondary_subject_spec.rb | 123 ++++++++++++++++++ .../publish/courses/new_subject_spec.rb | 94 ------------- spec/models/course_spec.rb | 21 +++ spec/policies/course_policy_spec.rb | 1 + .../courses/assign_subjects_service_spec.rb | 10 -- 13 files changed, 219 insertions(+), 118 deletions(-) create mode 100644 spec/features/publish/courses/new_primary_subject_spec.rb create mode 100644 spec/features/publish/courses/new_secondary_subject_spec.rb delete mode 100644 spec/features/publish/courses/new_subject_spec.rb diff --git a/app/controllers/publish/courses/subjects_controller.rb b/app/controllers/publish/courses/subjects_controller.rb index 59d97873ca..ac4eed415b 100644 --- a/app/controllers/publish/courses/subjects_controller.rb +++ b/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 diff --git a/app/decorators/course_decorator.rb b/app/decorators/course_decorator.rb index 8011a9153a..bda693d0d0 100644 --- a/app/decorators/course_decorator.rb +++ b/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 diff --git a/app/models/course.rb b/app/models/course.rb index dff34e4589..e189e709be 100644 --- a/app/models/course.rb +++ b/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 diff --git a/app/policies/course_policy.rb b/app/policies/course_policy.rb index 79ba950098..7954acf106 100644 --- a/app/policies/course_policy.rb +++ b/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 diff --git a/app/services/courses/assign_subjects_service.rb b/app/services/courses/assign_subjects_service.rb index 0cc51cc0aa..d3273efddc 100644 --- a/app/services/courses/assign_subjects_service.rb +++ b/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' diff --git a/config/analytics_blocklist.yml b/config/analytics_blocklist.yml index f0c5943b6a..33a7d909a1 100644 --- a/config/analytics_blocklist.yml +++ b/config/analytics_blocklist.yml @@ -70,6 +70,7 @@ - searchable :course: - master_subject_id + - subordinate_subject_id - campaign_name :gias_school: - searchable diff --git a/config/locales/en.yml b/config/locales/en.yml index 325b9617b9..becfabe413 100644 --- a/config/locales/en.yml +++ b/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: diff --git a/spec/features/publish/courses/new_primary_subject_spec.rb b/spec/features/publish/courses/new_primary_subject_spec.rb new file mode 100644 index 0000000000..9571f8bb2a --- /dev/null +++ b/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 diff --git a/spec/features/publish/courses/new_secondary_subject_spec.rb b/spec/features/publish/courses/new_secondary_subject_spec.rb new file mode 100644 index 0000000000..021f1de4b1 --- /dev/null +++ b/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 diff --git a/spec/features/publish/courses/new_subject_spec.rb b/spec/features/publish/courses/new_subject_spec.rb deleted file mode 100644 index c046bf257a..0000000000 --- a/spec/features/publish/courses/new_subject_spec.rb +++ /dev/null @@ -1,94 +0,0 @@ -# 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 primary subject' do - when_i_visit_the_new_course_subject_page(:primary) - when_i_select_a_primary_subject('Primary with English') - and_i_click_continue - then_i_am_met_with_the_age_range_page(:primary, :primary_with_english) - end - - scenario 'selecting secondary subject' do - when_i_visit_the_new_course_subject_page(:secondary) - when_i_select_a_subject(:business_studies) - and_i_click_continue - then_i_am_met_with_the_age_range_page(:secondary, :business_studies) - end - - scenario 'selecting secondary subject modern languages' do - when_i_visit_the_new_course_subject_page(:secondary) - when_i_select_a_subject(:modern_languages) - and_i_click_continue - then_i_am_met_with_the_modern_languages_page - end - - scenario 'invalid entries' do - when_i_visit_the_new_course_subject_page(%i[primary secondary].sample) - 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(level) - publish_courses_new_subjects_page.load(provider_code: provider.provider_code, recruitment_cycle_year: Settings.current_recruitment_cycle_year, query: level_params(level)) - end - - def when_i_select_a_primary_subject(subject_type) - publish_courses_new_subjects_page.choose(subject_type) - end - - def when_i_select_a_subject(subject_type) - publish_courses_new_subjects_page.master_subject_fields.select(course_subject(subject_type).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(level, subject_type) - expect(page).to have_current_path("/publish/organisations/#{provider.provider_code}/#{Settings.current_recruitment_cycle_year}/courses/age-range/new?#{params_with_subject(level, subject_type)}") - 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(:secondary, :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 :primary_with_english - find_or_create(:primary_subject, :primary_with_english) - when :business_studies - find_or_create(:secondary_subject, :business_studies) - when :modern_languages - find_or_create(:secondary_subject, :modern_languages) - end - end - - def params_with_subject(level, subject_type) - course_subject = course_subject(subject_type) - "course%5Bcampaign_name%5D=&course%5Bis_send%5D=0&course%5Blevel%5D=#{level}&course%5Bmaster_subject_id%5D=#{course_subject.id}&course%5Bsubjects_ids%5D%5B%5D=#{course_subject.id}" - end -end diff --git a/spec/models/course_spec.rb b/spec/models/course_spec.rb index 0ede34b3c6..fea7ec1cac 100644 --- a/spec/models/course_spec.rb +++ b/spec/models/course_spec.rb @@ -2677,6 +2677,27 @@ end end + context 'duplicated subject' do + let(:course) { Course.new } + let(:primary_subject) { find_or_create(:primary_subject, :primary) } + let(:subject_ids) { [primary_subject.id, primary_subject.id] } + + it 'have duplicated subject errors' do + expect(subject.errors[:subjects].first).to include('^You can only select a subject once') + end + end + + describe '#subordinate_subject_id' do + it 'has a subordinate_subject_id attribute' do + expect(course).to have_attributes({ subordinate_subject_id: nil }) + end + + it 'is assignabled' do + course.subordinate_subject_id = 123 + expect(course).to have_attributes({ subordinate_subject_id: 123 }) + end + end + describe '#age_minimum' do context 'when age_range_in_years set' do it 'returns lower age_range_in_years bound' do diff --git a/spec/policies/course_policy_spec.rb b/spec/policies/course_policy_spec.rb index e122dcecf6..17a3d14b64 100644 --- a/spec/policies/course_policy_spec.rb +++ b/spec/policies/course_policy_spec.rb @@ -85,6 +85,7 @@ can_sponsor_skilled_worker_visa campaign_name master_subject_id + subordinate_subject_id subjects_ids ] expect(subject).to match_array(expected_attributes) diff --git a/spec/services/courses/assign_subjects_service_spec.rb b/spec/services/courses/assign_subjects_service_spec.rb index 88abc66e5c..2c450837ba 100644 --- a/spec/services/courses/assign_subjects_service_spec.rb +++ b/spec/services/courses/assign_subjects_service_spec.rb @@ -10,16 +10,6 @@ ) end - context 'duplicated subject' do - let(:course) { Course.new } - let(:primary_subject) { find_or_create(:primary_subject, :primary) } - let(:subject_ids) { [primary_subject.id, primary_subject.id] } - - it 'have duplicated subject errors' do - expect(subject.errors[:subjects].first).to include('^You have already selected this subject. You can only select a subject once') - end - end - context 'no subject' do let(:course) { create(:course) } let(:subject_ids) { [] }