- <%= "#{school.location_name} is a school for courses run by #{@provider.provider_name}." %> -
-- To remove <%= school.location_name %>, you must first remove the school from those courses. -
+ <% if school_removal.only_school? %> ++ <%= "#{school.location_name} is the only school for #{@provider.provider_name}." %> +
++ To remove it, you must first add another school. +
+ <% else %> ++ <%= "#{school.location_name} is a school for courses run by #{@provider.provider_name}." %> +
++ To remove <%= school.location_name %>, you must first remove the school from those courses. +
+ <% end %> <% end %><%= govuk_link_to("Cancel", publish_provider_recruitment_cycle_school_path(@provider.provider_code, school.recruitment_cycle.year, school.uuid)) %> diff --git a/app/views/support/providers/schools/delete.html.erb b/app/views/support/providers/schools/delete.html.erb index c0b0b6890c..dbac942cce 100644 --- a/app/views/support/providers/schools/delete.html.erb +++ b/app/views/support/providers/schools/delete.html.erb @@ -14,6 +14,13 @@ delete_support_recruitment_cycle_provider_school_path(@provider.recruitment_cycle_year, @provider, @site.uuid), method: :delete, class: "govuk-button--warning" %> + <% elsif school_removal.only_school? %> +
+ <%= "#{@site.location_name} is the only school for #{@provider.provider_name}." %> +
++ To remove it, you must first add another school. +
<% else %><%= "#{@site.location_name} is a school for courses run by #{@provider.provider_name}." %> diff --git a/config/locales/en/publish/providers/schools.yml b/config/locales/en/publish/providers/schools.yml index a368841292..63511c53d9 100644 --- a/config/locales/en/publish/providers/schools.yml +++ b/config/locales/en/publish/providers/schools.yml @@ -17,3 +17,4 @@ en: remove: Remove school destroy: cannot_remove_school: This school could not be removed because it is used by a course + cannot_remove_only_school: This school could not be removed because it is your only school diff --git a/config/locales/en/support.yml b/config/locales/en/support.yml index b794a23932..a09304442e 100644 --- a/config/locales/en/support.yml +++ b/config/locales/en/support.yml @@ -14,6 +14,7 @@ en: search: # file destroy: cannot_remove_school: This school could not be removed because it is used by a course + cannot_remove_only_school: This school could not be removed because it is the provider’s only school users: first_name: "First name" last_name: "Last name" diff --git a/spec/requests/publish/providers/schools_spec.rb b/spec/requests/publish/providers/schools_spec.rb index 3c4bcf2569..03ef646947 100644 --- a/spec/requests/publish/providers/schools_spec.rb +++ b/spec/requests/publish/providers/schools_spec.rb @@ -171,6 +171,7 @@ def login_provider_user(provider) let(:recruitment_cycle) { find_or_create(:recruitment_cycle, year: remodel_cycle_year + 1) } let(:provider) { create(:provider, recruitment_cycle:) } let!(:provider_school) { create(:provider_school, provider:, gias_school:) } + let!(:other_provider_school) { create(:provider_school, provider:) } before { login_provider_user(provider) } @@ -182,6 +183,19 @@ def login_provider_user(provider) expect(response.body).to include("St Joseph") expect(response.body).to include("Catholic Primary School") end + + context "when it is the provider's only school" do + let!(:other_provider_school) { nil } + + it "explains that the provider would be left without a school" do + get delete_publish_provider_recruitment_cycle_school_path(provider.provider_code, recruitment_cycle.year, provider_school.uuid) + + expect(response).to have_http_status(:ok) + expect(response.body).to include("You cannot remove this school") + expect(response.body).to include("is the only school for #{provider.provider_name}") + expect(response.body).to include("To remove it, you must first add another school.") + end + end end describe "DELETE /publish/organisations/:provider_code/:recruitment_cycle_year/schools/:uuid" do @@ -190,6 +204,7 @@ def login_provider_user(provider) let!(:provider_school) do create(:provider_school, provider:, gias_school:, site_code: site.code, uuid: site.uuid) end + let!(:other_provider_school) { create(:provider_school, provider:) } let!(:exempt_course) { create(:course, :with_salary, provider:, publish_without_schools_allowed: true) } before { login_provider_user(provider) } @@ -225,7 +240,17 @@ def remove_school expect { remove_school }.not_to(change { exempt_course.reload.changed_at }) - expect(provider.schools.count).to eq(1) + expect(provider.schools).to contain_exactly(provider_school, other_provider_school) + expect(flash[:warning]).to eq("This school could not be removed because it is used by a course") + end + + it "does not remove the provider's last school" do + other_provider_school.destroy! + + expect { remove_school }.not_to(change { provider.schools.count }) + + expect(response).to redirect_to(delete_publish_provider_recruitment_cycle_school_path(provider.provider_code, provider.recruitment_cycle.year, provider_school.uuid)) + expect(flash[:warning]).to eq("This school could not be removed because it is your only school") end end end diff --git a/spec/requests/support/providers/schools_spec.rb b/spec/requests/support/providers/schools_spec.rb index ecc1c91bad..a4503dca15 100644 --- a/spec/requests/support/providers/schools_spec.rb +++ b/spec/requests/support/providers/schools_spec.rb @@ -20,6 +20,7 @@ let!(:provider_school) do create(:provider_school, provider:, gias_school:, site_code: site.code, uuid: site.uuid) end + let!(:other_provider_school) { create(:provider_school, provider:) } let!(:exempt_course) { create(:course, :with_salary, provider:, publish_without_schools_allowed: true) } def remove_school @@ -53,7 +54,17 @@ def remove_school expect { remove_school }.not_to(change { exempt_course.reload.changed_at }) - expect(provider.schools.count).to eq(1) + expect(provider.schools).to contain_exactly(provider_school, other_provider_school) + expect(flash[:warning]).to eq("This school could not be removed because it is used by a course") + end + + it "does not remove the provider's last school" do + other_provider_school.destroy! + + expect { remove_school }.not_to(change { provider.schools.count }) + + expect(response).to redirect_to(delete_support_recruitment_cycle_provider_school_path(recruitment_cycle.year, provider, provider_school.uuid)) + expect(flash[:warning]).to eq("This school could not be removed because it is the provider’s only school") end end end diff --git a/spec/services/provider_schools/removal_spec.rb b/spec/services/provider_schools/removal_spec.rb index 8bac70f0dd..44aad92a9e 100644 --- a/spec/services/provider_schools/removal_spec.rb +++ b/spec/services/provider_schools/removal_spec.rb @@ -3,25 +3,31 @@ require "rails_helper" RSpec.describe ProviderSchools::Removal do - let(:gias_school) { create(:gias_school, urn: "123456") } - let(:site_uuid) { Faker::Internet.uuid } + subject(:removal) { described_class.new(provider:, uuid: provider_school.uuid) } - describe "#call" do - let(:provider) { create(:provider) } - let!(:site) { create(:site, provider:, urn: gias_school.urn, code: "A", uuid: site_uuid) } - let!(:provider_school) { create(:provider_school, provider:, gias_school:, site_code: site.code, uuid: site_uuid) } + let(:provider) { create(:provider) } + + # The pair written together when a school is added: the provider school and + # the legacy site it shares a uuid, site code and urn with. + let!(:site) { create(:site, provider:) } + let!(:provider_school) { create(:provider_school, :for_site, site:) } + + # A second school, so removal is never blocked by the last-school guard. It + # needs no legacy site of its own: the guard only counts provider schools. + let!(:other_provider_school) { create(:provider_school, provider:) } + describe "#call" do it "removes both the legacy site and provider school" do - expect(described_class.new(provider:, uuid: site_uuid).call).to be(true) + expect(removal.call).to be(true) expect(Site.where(id: site.id)).to be_empty expect(Provider::School.where(id: provider_school.id)).to be_empty end it "does not remove either record when the provider school is attached to a course school" do - create(:course_school, course: create(:course, provider:), provider_school:, gias_school:) + create(:course_school, course: create(:course, provider:), provider_school:, gias_school: provider_school.gias_school) - expect(described_class.new(provider:, uuid: site_uuid).call).to be(false) + expect(removal.call).to be(false) expect(Site.where(id: site.id)).to contain_exactly(site) expect(Provider::School.where(id: provider_school.id)).to contain_exactly(provider_school) @@ -30,29 +36,34 @@ it "does not remove a legacy site that has no provider school" do provider_school.destroy! - expect { - described_class.new(provider:, uuid: site_uuid).call - }.to raise_error(ActiveRecord::RecordNotFound) + expect { described_class.new(provider:, uuid: site.uuid).call }.to raise_error(ActiveRecord::RecordNotFound) expect(Site.where(id: site.id)).to contain_exactly(site) end it "removes a provider school without requiring a legacy site" do - provider_school_without_site = create(:provider_school, provider:) - - expect(described_class.new(provider:, uuid: provider_school_without_site.uuid).call).to be(true) + expect(described_class.new(provider:, uuid: other_provider_school.uuid).call).to be(true) - expect(Provider::School.where(id: provider_school_without_site.id)).to be_empty + expect(Provider::School.where(id: other_provider_school.id)).to be_empty end it "does not remove a provider school belonging to another provider" do - other_provider_school = create(:provider_school, uuid: Faker::Internet.uuid) + someone_elses_school = create(:provider_school) - expect { - described_class.new(provider:, uuid: other_provider_school.uuid).call - }.to raise_error(ActiveRecord::RecordNotFound) + expect { described_class.new(provider:, uuid: someone_elses_school.uuid).call }.to raise_error(ActiveRecord::RecordNotFound) - expect(Provider::School.where(id: other_provider_school.id)).to contain_exactly(other_provider_school) + expect(Provider::School.where(id: someone_elses_school.id)).to contain_exactly(someone_elses_school) + end + + context "when it is the provider's only school" do + let!(:other_provider_school) { nil } + + it "does not remove either record" do + expect(removal.call).to be(false) + + expect(Site.where(id: site.id)).to contain_exactly(site) + expect(Provider::School.where(id: provider_school.id)).to contain_exactly(provider_school) + end end # Removal deliberately does not branch on the recruitment cycle: the same @@ -61,7 +72,7 @@ let(:provider) { create(:provider, :next_recruitment_cycle) } it "removes both the legacy site and provider school" do - expect(described_class.new(provider:, uuid: site_uuid).call).to be(true) + expect(removal.call).to be(true) expect(Site.where(id: site.id)).to be_empty expect(Provider::School.where(id: provider_school.id)).to be_empty diff --git a/spec/system/publish/providers/schools/delete_school_spec.rb b/spec/system/publish/providers/schools/delete_school_spec.rb index 209aef3b2c..62bd68e75b 100644 --- a/spec/system/publish/providers/schools/delete_school_spec.rb +++ b/spec/system/publish/providers/schools/delete_school_spec.rb @@ -7,9 +7,9 @@ include ProviderSchoolHelper scenario "with no associated courses" do - given_i_am_authenticated_as_a_provider_user + given_i_am_authenticated_as_a_provider_user_with_two_schools when_i_visit_the_schools_page - then_i_see_a_list_of_schools + then_i_see_both_schools_listed when_i_visit_the_publish_school_show_page and_i_click_remove_school_link then_i_am_on_the_school_delete_page @@ -23,9 +23,9 @@ end scenario "with associated course" do - given_i_am_authenticated_as_a_provider_user + given_i_am_authenticated_as_a_provider_user_with_two_schools when_i_visit_the_schools_page - then_i_see_a_list_of_schools + then_i_see_both_schools_listed given_there_is_an_associated_course when_i_visit_the_publish_school_show_page and_i_click_remove_school_link @@ -34,9 +34,9 @@ end scenario "when the school becomes associated with a course before removal" do - given_i_am_authenticated_as_a_provider_user + given_i_am_authenticated_as_a_provider_user_with_two_schools when_i_visit_the_schools_page - then_i_see_a_list_of_schools + then_i_see_both_schools_listed when_i_visit_the_publish_school_show_page and_i_click_remove_school_link then_i_am_on_the_school_delete_page @@ -48,9 +48,9 @@ end scenario "with discarded associated course" do - given_i_am_authenticated_as_a_provider_user + given_i_am_authenticated_as_a_provider_user_with_two_schools when_i_visit_the_schools_page - then_i_see_a_list_of_schools + then_i_see_both_schools_listed given_there_is_an_associated_course and_i_delete_the_course when_i_visit_the_publish_school_show_page @@ -63,9 +63,20 @@ and_the_school_is_deleted end + scenario "when it is the provider's only school" do + given_i_am_authenticated_as_a_provider_user + when_i_visit_the_schools_page + then_i_see_a_list_of_schools + when_i_visit_the_publish_school_show_page + and_i_click_remove_school_link + then_i_am_on_the_school_delete_page + and_i_am_told_it_is_the_only_school + end + scenario "after the schools remodel cycle without a legacy site" do given_i_am_authenticated_as_a_provider_user_after_the_schools_remodel_cycle and_there_is_a_provider_school_without_a_legacy_site + and_the_future_provider_has_a_second_school when_i_visit_the_schools_page_after_the_schools_remodel_cycle then_i_see_the_provider_school_listed_with_its_uuid @@ -77,6 +88,26 @@ and_the_provider_school_is_deleted end + def given_i_am_authenticated_as_a_provider_user_with_two_schools + given_i_am_authenticated_as_a_provider_user + second_school + end + + def second_school + @second_school ||= create( + :provider_school, + provider:, + gias_school: create(:gias_school, name: "Second School"), + site_code: "Z", + ) + end + + def then_i_see_both_schools_listed + expect(publish_schools_index_page.schools.size).to eq(2) + expect(publish_schools_index_page).to have_text(provider_school.location_name) + expect(publish_schools_index_page).to have_text(second_school.location_name) + end + def given_i_am_authenticated_as_a_provider_user_after_the_schools_remodel_cycle given_i_am_authenticated(user: create(:user, providers: [future_provider])) end @@ -85,6 +116,15 @@ def and_there_is_a_provider_school_without_a_legacy_site expect(Site.find_by_uuid(future_provider_school.uuid)).to be_nil end + def and_the_future_provider_has_a_second_school + create( + :provider_school, + provider: future_provider, + gias_school: create(:gias_school, name: "Second Future School"), + site_code: "Z", + ) + end + def when_i_visit_the_schools_page_after_the_schools_remodel_cycle publish_schools_index_page.load( provider_code: future_provider.provider_code, @@ -93,14 +133,19 @@ def when_i_visit_the_schools_page_after_the_schools_remodel_cycle end def then_i_see_the_provider_school_listed_with_its_uuid - expect(publish_schools_index_page.schools.first.name).to have_text(future_gias_school.name) - expect(publish_schools_index_page.schools.first.code).to have_text("- (dash)") - expect(publish_schools_index_page.schools.first.urn).to have_text(future_gias_school.urn) - expect(publish_schools_index_page.schools.first.edit_link[:href]).to include(future_provider_school.uuid) + row = school_row(future_gias_school.name) + + expect(row.code).to have_text("- (dash)") + expect(row.urn).to have_text(future_gias_school.urn) + expect(row.edit_link[:href]).to include(future_provider_school.uuid) end def when_i_click_the_provider_school - publish_schools_index_page.schools.first.edit_link.click + school_row(future_gias_school.name).edit_link.click + end + + def school_row(name) + publish_schools_index_page.schools.find { |school| school.name.text.include?(name) } end def then_i_see_the_provider_school_details @@ -146,6 +191,10 @@ def future_gias_school ) end + def provider_school + @provider_school ||= provider.schools.find_by!(uuid: site.uuid) + end + def when_i_visit_the_publish_school_show_page publish_school_show_page.load(provider_code: provider.provider_code, recruitment_cycle_year: provider.recruitment_cycle_year, school_id: site.uuid) end @@ -168,11 +217,13 @@ def and_i_click_remove_school_button alias_method :when_i_click_remove_school_button, :and_i_click_remove_school_button def and_the_school_is_deleted - expect(provider.sites.count).to eq 0 + expect(provider.sites.reload).to be_empty + expect(provider.schools.reload).to contain_exactly(second_school) end def and_the_school_is_not_deleted - expect(provider.sites.count).to eq 1 + expect(provider.sites.reload.count).to eq 1 + expect(provider.schools.reload).to contain_exactly(provider_school, second_school) end def given_there_is_an_associated_course @@ -180,14 +231,23 @@ def given_there_is_an_associated_course create( :course_school, course: @course, - provider_school: provider.schools.first, - gias_school: provider.schools.first.gias_school, - site_code: provider.schools.first.site_code, + provider_school:, + gias_school: provider_school.gias_school, + site_code: provider_school.site_code, ) end def and_i_cannot_delete_the_school expect(publish_school_delete_page).to have_text("You cannot remove this school") + expect(page).to have_content("#{provider_school.location_name} is a school for courses run by #{provider.provider_name}.") + expect(page).to have_content("To remove #{provider_school.location_name}, you must first remove the school from those courses.") + expect(publish_school_delete_page).not_to have_remove_school_button + end + + def and_i_am_told_it_is_the_only_school + expect(publish_school_delete_page).to have_text("You cannot remove this school") + expect(page).to have_content("#{provider_school.location_name} is the only school for #{provider.provider_name}.") + expect(page).to have_content("To remove it, you must first add another school.") expect(publish_school_delete_page).not_to have_remove_school_button end diff --git a/spec/system/support/providers/schools/delete_school_spec.rb b/spec/system/support/providers/schools/delete_school_spec.rb index 99c07f4856..e16428802a 100644 --- a/spec/system/support/providers/schools/delete_school_spec.rb +++ b/spec/system/support/providers/schools/delete_school_spec.rb @@ -7,6 +7,7 @@ scenario do given_i_am_authenticated_as_an_admin_user and_there_is_a_provider_site + and_the_provider_has_a_second_school and_i_visit_the_support_provider_school_show_page when_i_click_remove_school_link then_i_am_on_the_school_delete_page @@ -22,6 +23,7 @@ scenario "when the school becomes associated with a course before removal" do given_i_am_authenticated_as_an_admin_user and_there_is_a_provider_site + and_the_provider_has_a_second_school and_i_visit_the_support_provider_school_show_page when_i_click_remove_school_link then_i_am_on_the_school_delete_page @@ -32,9 +34,19 @@ and_the_school_is_not_deleted end + scenario "when it is the provider's only school" do + given_i_am_authenticated_as_an_admin_user + and_there_is_a_provider_site + and_i_visit_the_support_provider_school_show_page + when_i_click_remove_school_link + then_i_am_on_the_school_delete_page + and_i_am_told_it_is_the_only_school + end + scenario "after the schools remodel cycle without a legacy site" do given_i_am_authenticated_as_an_admin_user and_there_is_a_provider_school_after_the_schools_remodel_cycle_without_a_legacy_site + and_the_future_provider_has_a_second_school when_i_visit_the_support_provider_schools_index_page_after_the_schools_remodel_cycle then_i_see_the_provider_school_listed_with_its_uuid @@ -51,6 +63,20 @@ def and_there_is_a_provider_school_after_the_schools_remodel_cycle_without_a_leg expect(Site.find_by_uuid(future_provider_school.uuid)).to be_nil end + def and_the_provider_has_a_second_school + create(:provider_school, provider: @provider, gias_school: create(:gias_school, name: "Second School"), site_code: "Z") + end + + def and_the_future_provider_has_a_second_school + create(:provider_school, provider: future_provider, gias_school: create(:gias_school, name: "Second Future School"), site_code: "Z") + end + + def and_i_am_told_it_is_the_only_school + expect(page).to have_content("#{@provider_school.location_name} is the only school for School of Cats.") + expect(page).to have_content("To remove it, you must first add another school.") + expect(support_provider_school_delete_page).not_to have_remove_school_button + end + def when_i_visit_the_support_provider_schools_index_page_after_the_schools_remodel_cycle support_provider_schools_index_page.load( recruitment_cycle_year: future_recruitment_cycle.year,