From 07ca8a98a27ea8f42f0e4334563ff0750a957408 Mon Sep 17 00:00:00 2001 From: Thomas Leese Date: Wed, 30 Aug 2023 11:44:28 +0100 Subject: [PATCH 1/5] Refactor support console policies This changes the support console to use policies as they're expected to be used rather than having a single policy for everything in the support console. It allows us to test individual policies and have a clear understanding of each controller method and how it corresponds to each policy. --- .../support_interface/base_controller.rb | 5 - .../support_interface/countries_controller.rb | 13 +- .../english_language_providers_controller.rb | 11 +- .../feature_flags_controller.rb | 5 + .../support_interface/regions_controller.rb | 13 +- .../support_interface/staff_controller.rb | 11 +- .../support_interface/country_policy.rb | 15 ++ .../english_language_provider_policy.rb | 11 ++ .../support_interface/feature_flag_policy.rb | 15 ++ .../support_interface/region_policy.rb | 11 ++ .../support_interface/staff_policy.rb | 11 ++ app/policies/support_policy.rb | 32 ----- config/feature_flags.yml | 2 +- .../application_form_policy_spec.rb | 6 +- .../assessment_policy_spec.rb | 6 +- .../work_history_policy_spec.rb | 6 +- spec/policies/assessor_policy_spec.rb | 6 +- spec/policies/note_policy_spec.rb | 74 +++++----- .../support_interface/country_policy_spec.rb | 27 ++++ .../english_language_provider_policy_spec.rb | 22 +++ .../feature_flag_policy_spec.rb | 22 +++ .../support_interface/region_policy_spec.rb | 22 +++ .../support_interface/staff_policy_spec.rb | 22 +++ spec/policies/support_policy_spec.rb | 132 ------------------ spec/support/shared_examples/policy.rb | 29 ++++ 25 files changed, 285 insertions(+), 244 deletions(-) create mode 100644 app/controllers/support_interface/feature_flags_controller.rb create mode 100644 app/policies/support_interface/country_policy.rb create mode 100644 app/policies/support_interface/english_language_provider_policy.rb create mode 100644 app/policies/support_interface/feature_flag_policy.rb create mode 100644 app/policies/support_interface/region_policy.rb create mode 100644 app/policies/support_interface/staff_policy.rb delete mode 100644 app/policies/support_policy.rb create mode 100644 spec/policies/support_interface/country_policy_spec.rb create mode 100644 spec/policies/support_interface/english_language_provider_policy_spec.rb create mode 100644 spec/policies/support_interface/feature_flag_policy_spec.rb create mode 100644 spec/policies/support_interface/region_policy_spec.rb create mode 100644 spec/policies/support_interface/staff_policy_spec.rb delete mode 100644 spec/policies/support_policy_spec.rb create mode 100644 spec/support/shared_examples/policy.rb diff --git a/app/controllers/support_interface/base_controller.rb b/app/controllers/support_interface/base_controller.rb index 433c76b493..3b2cba2eba 100644 --- a/app/controllers/support_interface/base_controller.rb +++ b/app/controllers/support_interface/base_controller.rb @@ -4,10 +4,5 @@ class SupportInterface::BaseController < ApplicationController include SupportCurrentNamespace include StaffAuthenticatable - before_action :authorize_support after_action :verify_authorized - - def authorize_support - authorize :support - end end diff --git a/app/controllers/support_interface/countries_controller.rb b/app/controllers/support_interface/countries_controller.rb index 8cc3c5a953..2a8a6cca2d 100644 --- a/app/controllers/support_interface/countries_controller.rb +++ b/app/controllers/support_interface/countries_controller.rb @@ -1,20 +1,19 @@ # frozen_string_literal: true class SupportInterface::CountriesController < SupportInterface::BaseController - skip_before_action :authorize_support, only: :confirm_edit - before_action :load_country_and_edit_actions, only: %w[confirm_edit update] + before_action :load_country, except: :index + before_action :load_edit_actions, only: %w[confirm_edit update] def index + authorize [:support_interface, Country] @countries = Country.includes(:regions).order(:code) end def edit - @country = Country.includes(:regions).find(params[:id]) @all_regions = @country.regions.map(&:name).join("\n") end def confirm_edit - authorize :support, :edit? @country.assign_attributes(country_params) end @@ -43,8 +42,12 @@ def update private - def load_country_and_edit_actions + def load_country @country = Country.includes(:regions).find(params[:id]) + authorize [:support_interface, @country] + end + + def load_edit_actions @all_regions = (params.dig(:country, :all_regions) || "").chomp @diff_actions = calculate_diff_actions end diff --git a/app/controllers/support_interface/english_language_providers_controller.rb b/app/controllers/support_interface/english_language_providers_controller.rb index efd0a59a62..d16b484b20 100644 --- a/app/controllers/support_interface/english_language_providers_controller.rb +++ b/app/controllers/support_interface/english_language_providers_controller.rb @@ -1,17 +1,17 @@ # frozen_string_literal: true class SupportInterface::EnglishLanguageProvidersController < SupportInterface::BaseController + before_action :load_english_language_provider, except: :index + def index + authorize [:support_interface, EnglishLanguageProvider] @english_language_providers = EnglishLanguageProvider.order(:created_at) end def edit - @english_language_provider = EnglishLanguageProvider.find(params[:id]) end def update - @english_language_provider = EnglishLanguageProvider.find(params[:id]) - if @english_language_provider.update(english_language_provider_params) flash[ :success @@ -25,6 +25,11 @@ def update private + def load_english_language_provider + @english_language_provider = EnglishLanguageProvider.find(params[:id]) + authorize [:support_interface, @english_language_provider] + end + def english_language_provider_params params.require(:english_language_provider).permit( :name, diff --git a/app/controllers/support_interface/feature_flags_controller.rb b/app/controllers/support_interface/feature_flags_controller.rb new file mode 100644 index 0000000000..62707711fc --- /dev/null +++ b/app/controllers/support_interface/feature_flags_controller.rb @@ -0,0 +1,5 @@ +# frozen_string_literal: true + +class SupportInterface::FeatureFlagsController < SupportInterface::BaseController + before_action { authorize %i[support_interface feature_flag] } +end diff --git a/app/controllers/support_interface/regions_controller.rb b/app/controllers/support_interface/regions_controller.rb index 3c90e769f4..9ff59be106 100644 --- a/app/controllers/support_interface/regions_controller.rb +++ b/app/controllers/support_interface/regions_controller.rb @@ -1,15 +1,12 @@ # frozen_string_literal: true class SupportInterface::RegionsController < SupportInterface::BaseController - skip_before_action :authorize_support, only: :preview + before_action :load_region def edit - @region = Region.find(params[:id]) end def update - @region = Region.find(params[:id]) - if @region.update(region_params) flash[ :success @@ -26,13 +23,15 @@ def update end def preview - authorize :support, :show? - - @region = Region.find(params[:id]) end private + def load_region + @region = Region.find(params[:id]) + authorize [:support_interface, @region] + end + def region_params params.require(:region).permit( :application_form_skip_work_history, diff --git a/app/controllers/support_interface/staff_controller.rb b/app/controllers/support_interface/staff_controller.rb index a052786402..3d1a7e4967 100644 --- a/app/controllers/support_interface/staff_controller.rb +++ b/app/controllers/support_interface/staff_controller.rb @@ -1,17 +1,17 @@ # frozen_string_literal: true class SupportInterface::StaffController < SupportInterface::BaseController + before_action :load_staff, except: :index + def index + authorize [:support_interface, Staff] @staff = Staff.order(:name) end def edit - @staff = Staff.find(params[:id]) end def update - @staff = Staff.find(params[:id]) - if @staff.update(staff_params) redirect_to %i[support_interface staff index] else @@ -21,6 +21,11 @@ def update private + def load_staff + @staff = Staff.find(params[:id]) + authorize [:support_interface, @staff] + end + def staff_params params.require(:staff).permit( :award_decline_permission, diff --git a/app/policies/support_interface/country_policy.rb b/app/policies/support_interface/country_policy.rb new file mode 100644 index 0000000000..a5c58754f8 --- /dev/null +++ b/app/policies/support_interface/country_policy.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +class SupportInterface::CountryPolicy < ApplicationPolicy + def index? + user.support_console_permission? + end + + def confirm_edit? + user.support_console_permission? + end + + def update? + user.support_console_permission? + end +end diff --git a/app/policies/support_interface/english_language_provider_policy.rb b/app/policies/support_interface/english_language_provider_policy.rb new file mode 100644 index 0000000000..c59fa5f9e8 --- /dev/null +++ b/app/policies/support_interface/english_language_provider_policy.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +class SupportInterface::EnglishLanguageProviderPolicy < ApplicationPolicy + def index? + user.support_console_permission? + end + + def update? + user.support_console_permission? + end +end diff --git a/app/policies/support_interface/feature_flag_policy.rb b/app/policies/support_interface/feature_flag_policy.rb new file mode 100644 index 0000000000..8beff1aa21 --- /dev/null +++ b/app/policies/support_interface/feature_flag_policy.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +class SupportInterface::FeatureFlagPolicy < ApplicationPolicy + def index? + user.support_console_permission? + end + + def activate? + user.support_console_permission? + end + + def deactivate? + user.support_console_permission? + end +end diff --git a/app/policies/support_interface/region_policy.rb b/app/policies/support_interface/region_policy.rb new file mode 100644 index 0000000000..7e75cc2399 --- /dev/null +++ b/app/policies/support_interface/region_policy.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +class SupportInterface::RegionPolicy < ApplicationPolicy + def update? + user.support_console_permission? + end + + def preview? + user.support_console_permission? + end +end diff --git a/app/policies/support_interface/staff_policy.rb b/app/policies/support_interface/staff_policy.rb new file mode 100644 index 0000000000..6ed7df7392 --- /dev/null +++ b/app/policies/support_interface/staff_policy.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +class SupportInterface::StaffPolicy < ApplicationPolicy + def index? + user.support_console_permission? + end + + def update? + user.support_console_permission? + end +end diff --git a/app/policies/support_policy.rb b/app/policies/support_policy.rb deleted file mode 100644 index 926181fc73..0000000000 --- a/app/policies/support_policy.rb +++ /dev/null @@ -1,32 +0,0 @@ -# frozen_string_literal: true - -class SupportPolicy < ApplicationPolicy - def index? - user.support_console_permission? - end - - def show? - user.support_console_permission? - end - - def create? - user.support_console_permission? - end - - def update? - user.support_console_permission? - end - - def destroy? - user.support_console_permission? - end - - # Feature flags - def activate? - user.support_console_permission? - end - - def deactivate? - user.support_console_permission? - end -end diff --git a/config/feature_flags.yml b/config/feature_flags.yml index 143949fe73..f862f65e6a 100644 --- a/config/feature_flags.yml +++ b/config/feature_flags.yml @@ -34,4 +34,4 @@ feature_flags: Allow starting an application on this service directly after completing an eligibility check. -parent_controller: "SupportInterface::BaseController" +parent_controller: SupportInterface::FeatureFlagsController diff --git a/spec/policies/assessor_interface/application_form_policy_spec.rb b/spec/policies/assessor_interface/application_form_policy_spec.rb index 1d003a44a5..9b0fcd67a2 100644 --- a/spec/policies/assessor_interface/application_form_policy_spec.rb +++ b/spec/policies/assessor_interface/application_form_policy_spec.rb @@ -3,15 +3,13 @@ require "rails_helper" RSpec.describe AssessorInterface::ApplicationFormPolicy do + it_behaves_like "a policy" + let(:user) { nil } let(:record) { nil } subject(:policy) { described_class.new(user, record) } - it "must have a user" do - expect { policy }.to raise_error(Pundit::NotAuthorizedError) - end - describe "#index?" do subject(:index?) { policy.index? } diff --git a/spec/policies/assessor_interface/assessment_policy_spec.rb b/spec/policies/assessor_interface/assessment_policy_spec.rb index d92d705f6d..c9794eec1f 100644 --- a/spec/policies/assessor_interface/assessment_policy_spec.rb +++ b/spec/policies/assessor_interface/assessment_policy_spec.rb @@ -3,15 +3,13 @@ require "rails_helper" RSpec.describe AssessorInterface::AssessmentPolicy do + it_behaves_like "a policy" + let(:user) { nil } let(:record) { nil } subject(:policy) { described_class.new(user, record) } - it "must have a user" do - expect { policy }.to raise_error(Pundit::NotAuthorizedError) - end - describe "#index?" do subject(:index?) { policy.index? } diff --git a/spec/policies/assessor_interface/work_history_policy_spec.rb b/spec/policies/assessor_interface/work_history_policy_spec.rb index 24efdb4106..94a9aced25 100644 --- a/spec/policies/assessor_interface/work_history_policy_spec.rb +++ b/spec/policies/assessor_interface/work_history_policy_spec.rb @@ -3,15 +3,13 @@ require "rails_helper" RSpec.describe AssessorInterface::WorkHistoryPolicy do + it_behaves_like "a policy" + let(:user) { nil } let(:record) { nil } subject(:policy) { described_class.new(user, record) } - it "must have a user" do - expect { policy }.to raise_error(Pundit::NotAuthorizedError) - end - describe "#index?" do subject(:index?) { policy.index? } diff --git a/spec/policies/assessor_policy_spec.rb b/spec/policies/assessor_policy_spec.rb index 36d8809b12..a57392fc0a 100644 --- a/spec/policies/assessor_policy_spec.rb +++ b/spec/policies/assessor_policy_spec.rb @@ -3,15 +3,13 @@ require "rails_helper" RSpec.describe AssessorPolicy do + it_behaves_like "a policy" + let(:user) { nil } let(:record) { nil } subject(:policy) { described_class.new(user, record) } - it "must have a user" do - expect { policy }.to raise_error(Pundit::NotAuthorizedError) - end - describe "#index?" do subject(:index?) { policy.index? } diff --git a/spec/policies/note_policy_spec.rb b/spec/policies/note_policy_spec.rb index fdbbf641ca..ba808c3c5c 100644 --- a/spec/policies/note_policy_spec.rb +++ b/spec/policies/note_policy_spec.rb @@ -3,51 +3,45 @@ require "rails_helper" RSpec.describe NotePolicy do - let(:user) { nil } + it_behaves_like "a policy" + + let(:user) { create(:staff, :confirmed) } let(:record) { nil } subject(:policy) { described_class.new(user, record) } - it "must have a user" do - expect { policy }.to raise_error(Pundit::NotAuthorizedError) + describe "#index?" do + subject(:index?) { policy.index? } + it { is_expected.to be true } + end + + describe "#show?" do + subject(:show?) { policy.show? } + it { is_expected.to be true } + end + + describe "#create?" do + subject(:create?) { policy.create? } + it { is_expected.to be true } + end + + describe "#new?" do + subject(:new?) { policy.new? } + it { is_expected.to be true } + end + + describe "#update?" do + subject(:update?) { policy.update? } + it { is_expected.to be true } + end + + describe "#edit?" do + subject(:edit?) { policy.edit? } + it { is_expected.to be true } end - context "with a user" do - let(:user) { create(:staff, :confirmed) } - - describe "#index?" do - subject(:index?) { policy.index? } - it { is_expected.to be true } - end - - describe "#show?" do - subject(:show?) { policy.show? } - it { is_expected.to be true } - end - - describe "#create?" do - subject(:create?) { policy.create? } - it { is_expected.to be true } - end - - describe "#new?" do - subject(:new?) { policy.new? } - it { is_expected.to be true } - end - - describe "#update?" do - subject(:update?) { policy.update? } - it { is_expected.to be true } - end - - describe "#edit?" do - subject(:edit?) { policy.edit? } - it { is_expected.to be true } - end - - describe "#destroy?" do - subject(:destroy?) { policy.destroy? } - it { is_expected.to be false } - end + describe "#destroy?" do + subject(:destroy?) { policy.destroy? } + it { is_expected.to be false } end end diff --git a/spec/policies/support_interface/country_policy_spec.rb b/spec/policies/support_interface/country_policy_spec.rb new file mode 100644 index 0000000000..b9bceda0d1 --- /dev/null +++ b/spec/policies/support_interface/country_policy_spec.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +require "rails_helper" + +RSpec.describe SupportInterface::CountryPolicy do + it_behaves_like "a policy" + + describe "#index?" do + subject(:index?) { described_class.new(user, nil).index? } + it_behaves_like "a policy method requiring the support console permission" + end + + describe "#edit?" do + subject(:edit?) { described_class.new(user, nil).edit? } + it_behaves_like "a policy method requiring the support console permission" + end + + describe "#confirm_edit?" do + subject(:confirm_edit?) { described_class.new(user, nil).confirm_edit? } + it_behaves_like "a policy method requiring the support console permission" + end + + describe "#update?" do + subject(:update?) { described_class.new(user, nil).update? } + it_behaves_like "a policy method requiring the support console permission" + end +end diff --git a/spec/policies/support_interface/english_language_provider_policy_spec.rb b/spec/policies/support_interface/english_language_provider_policy_spec.rb new file mode 100644 index 0000000000..7880656fc0 --- /dev/null +++ b/spec/policies/support_interface/english_language_provider_policy_spec.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +require "rails_helper" + +RSpec.describe SupportInterface::EnglishLanguageProviderPolicy do + it_behaves_like "a policy" + + describe "#index?" do + subject(:index?) { described_class.new(user, nil).index? } + it_behaves_like "a policy method requiring the support console permission" + end + + describe "#edit?" do + subject(:edit?) { described_class.new(user, nil).edit? } + it_behaves_like "a policy method requiring the support console permission" + end + + describe "#update?" do + subject(:update?) { described_class.new(user, nil).update? } + it_behaves_like "a policy method requiring the support console permission" + end +end diff --git a/spec/policies/support_interface/feature_flag_policy_spec.rb b/spec/policies/support_interface/feature_flag_policy_spec.rb new file mode 100644 index 0000000000..d303b74d39 --- /dev/null +++ b/spec/policies/support_interface/feature_flag_policy_spec.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +require "rails_helper" + +RSpec.describe SupportInterface::FeatureFlagPolicy do + it_behaves_like "a policy" + + describe "#index?" do + subject(:index?) { described_class.new(user, nil).index? } + it_behaves_like "a policy method requiring the support console permission" + end + + describe "#activate?" do + subject(:activate?) { described_class.new(user, nil).activate? } + it_behaves_like "a policy method requiring the support console permission" + end + + describe "#deactivate?" do + subject(:deactivate?) { described_class.new(user, nil).deactivate? } + it_behaves_like "a policy method requiring the support console permission" + end +end diff --git a/spec/policies/support_interface/region_policy_spec.rb b/spec/policies/support_interface/region_policy_spec.rb new file mode 100644 index 0000000000..659c699c0e --- /dev/null +++ b/spec/policies/support_interface/region_policy_spec.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +require "rails_helper" + +RSpec.describe SupportInterface::RegionPolicy do + it_behaves_like "a policy" + + describe "#edit?" do + subject(:edit?) { described_class.new(user, nil).edit? } + it_behaves_like "a policy method requiring the support console permission" + end + + describe "#update?" do + subject(:update?) { described_class.new(user, nil).update? } + it_behaves_like "a policy method requiring the support console permission" + end + + describe "#preview?" do + subject(:preview?) { described_class.new(user, nil).preview? } + it_behaves_like "a policy method requiring the support console permission" + end +end diff --git a/spec/policies/support_interface/staff_policy_spec.rb b/spec/policies/support_interface/staff_policy_spec.rb new file mode 100644 index 0000000000..b47ce84237 --- /dev/null +++ b/spec/policies/support_interface/staff_policy_spec.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +require "rails_helper" + +RSpec.describe SupportInterface::StaffPolicy do + it_behaves_like "a policy" + + describe "#index?" do + subject(:index?) { described_class.new(user, nil).index? } + it_behaves_like "a policy method requiring the support console permission" + end + + describe "#edit?" do + subject(:edit?) { described_class.new(user, nil).edit? } + it_behaves_like "a policy method requiring the support console permission" + end + + describe "#update?" do + subject(:update?) { described_class.new(user, nil).update? } + it_behaves_like "a policy method requiring the support console permission" + end +end diff --git a/spec/policies/support_policy_spec.rb b/spec/policies/support_policy_spec.rb deleted file mode 100644 index 4efdea59aa..0000000000 --- a/spec/policies/support_policy_spec.rb +++ /dev/null @@ -1,132 +0,0 @@ -# frozen_string_literal: true - -require "rails_helper" - -RSpec.describe SupportPolicy do - let(:user) { nil } - let(:record) { nil } - - subject(:policy) { described_class.new(user, record) } - - it "must have a user" do - expect { policy }.to raise_error(Pundit::NotAuthorizedError) - end - - context "with a user" do - let(:user) { create(:staff, :confirmed) } - - it { is_expected.to_not be_nil } - end - - describe "#index?" do - subject(:index?) { policy.index? } - - context "without permission" do - let(:user) { create(:staff, :confirmed) } - it { is_expected.to be false } - end - - context "with permission" do - let(:user) do - create(:staff, :confirmed, :with_support_console_permission) - end - it { is_expected.to be true } - end - end - - describe "#show?" do - subject(:show?) { policy.show? } - - context "without permission" do - let(:user) { create(:staff, :confirmed) } - it { is_expected.to be false } - end - - context "with permission" do - let(:user) do - create(:staff, :confirmed, :with_support_console_permission) - end - it { is_expected.to be true } - end - end - - describe "#create?" do - subject(:create?) { policy.create? } - - context "without permission" do - let(:user) { create(:staff, :confirmed) } - it { is_expected.to be false } - end - - context "with permission" do - let(:user) do - create(:staff, :confirmed, :with_support_console_permission) - end - it { is_expected.to be true } - end - end - - describe "#new?" do - subject(:new?) { policy.new? } - - context "without permission" do - let(:user) { create(:staff, :confirmed) } - it { is_expected.to be false } - end - - context "with permission" do - let(:user) do - create(:staff, :confirmed, :with_support_console_permission) - end - it { is_expected.to be true } - end - end - - describe "#update?" do - subject(:update?) { policy.update? } - - context "without permission" do - let(:user) { create(:staff, :confirmed) } - it { is_expected.to be false } - end - - context "with permission" do - let(:user) do - create(:staff, :confirmed, :with_support_console_permission) - end - it { is_expected.to be true } - end - end - - describe "#edit?" do - subject(:edit?) { policy.edit? } - - context "without permission" do - let(:user) { create(:staff, :confirmed) } - it { is_expected.to be false } - end - - context "with permission" do - let(:user) do - create(:staff, :confirmed, :with_support_console_permission) - end - it { is_expected.to be true } - end - end - - describe "#destroy?" do - subject(:destroy?) { policy.destroy? } - - context "without permission" do - let(:user) { create(:staff, :confirmed) } - it { is_expected.to be false } - end - - context "with permission" do - let(:user) do - create(:staff, :confirmed, :with_support_console_permission) - end - it { is_expected.to be true } - end - end -end diff --git a/spec/support/shared_examples/policy.rb b/spec/support/shared_examples/policy.rb new file mode 100644 index 0000000000..11ee1f793d --- /dev/null +++ b/spec/support/shared_examples/policy.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +RSpec.shared_examples "a policy" do + let(:user) { nil } + + subject(:policy) { described_class.new(user, nil) } + + it "must have a user" do + expect { policy }.to raise_error(Pundit::NotAuthorizedError) + end + + context "with a user" do + let(:user) { create(:staff, :confirmed) } + + it { is_expected.to_not be_nil } + end +end + +RSpec.shared_examples "a policy method requiring the support console permission" do + context "without permission" do + let(:user) { create(:staff) } + it { is_expected.to be false } + end + + context "with permission" do + let(:user) { create(:staff, :with_support_console_permission) } + it { is_expected.to be true } + end +end From 28a0621e2a10363e39338596ed545d8d65839e43 Mon Sep 17 00:00:00 2001 From: Thomas Leese Date: Wed, 30 Aug 2023 13:18:56 +0100 Subject: [PATCH 2/5] Allow previewing region changes before saving This changes how the support console works for regions by storing any changes in the session, allowing them to be previewed before they're saved to the database. --- .../support_interface/regions_controller.rb | 20 ++++++++--------- .../support_interface/regions/edit.html.erb | 5 ++--- .../regions/preview.html.erb | 22 ++++++++++++++++++- .../support_interface/countries_spec.rb | 21 +++++++----------- 4 files changed, 40 insertions(+), 28 deletions(-) diff --git a/app/controllers/support_interface/regions_controller.rb b/app/controllers/support_interface/regions_controller.rb index 9ff59be106..7a958aa1f3 100644 --- a/app/controllers/support_interface/regions_controller.rb +++ b/app/controllers/support_interface/regions_controller.rb @@ -7,22 +7,20 @@ def edit end def update - if @region.update(region_params) - flash[ - :success - ] = "Successfully updated #{CountryName.from_region(@region)}" - - if params[:preview] == "preview" - redirect_to preview_support_interface_region_path(@region) - else - redirect_to support_interface_countries_path - end - else + @region.assign_attributes(region_params) + if @region.invalid? render :edit, status: :unprocessable_entity + elsif ActiveModel::Type::Boolean.new.cast(params[:preview]) + session[:region] = region_params + redirect_to [:preview, :support_interface, @region] + else + @region.save! + redirect_to %i[support_interface countries] end end def preview + @region.assign_attributes(session[:region]) end private diff --git a/app/views/support_interface/regions/edit.html.erb b/app/views/support_interface/regions/edit.html.erb index fd82ff57fe..4a0c2cd6f6 100644 --- a/app/views/support_interface/regions/edit.html.erb +++ b/app/views/support_interface/regions/edit.html.erb @@ -23,8 +23,7 @@ <%= f.govuk_check_box :teaching_authority_provides_written_statement, 1, 0, multiple: false, link_errors: true, small: true, label: { text: "Will the teaching authority only send the letter of professional standing (LOPS) directly to the TRA?" } %>
- <%= f.govuk_submit "Save", prevent_double_click: false do %> - <%= f.govuk_submit "Save and preview", prevent_double_click: false, name: "preview", value: "preview" %> - <%= govuk_button_link_to "Ignore changes and preview", preview_support_interface_region_path(@region), secondary: true %> + <%= f.govuk_submit "Preview", name: "preview", value: "true" do %> + <%= f.govuk_submit "Save", name: "preview", value: "false", secondary: true %> <% end %> <% end %> diff --git a/app/views/support_interface/regions/preview.html.erb b/app/views/support_interface/regions/preview.html.erb index d3f0eec241..438839160a 100644 --- a/app/views/support_interface/regions/preview.html.erb +++ b/app/views/support_interface/regions/preview.html.erb @@ -10,4 +10,24 @@
-

<%= govuk_link_to "Back to edit #{CountryName.from_region(@region)}", edit_support_interface_region_path(@region) %>

+<%= form_with model: [:support_interface, @region] do |f| %> + <%= f.hidden_field :application_form_skip_work_history, value: @region.application_form_skip_work_history %> + <%= f.hidden_field :qualifications_information, value: @region.qualifications_information %> + <%= f.hidden_field :reduced_evidence_accepted, value: @region.reduced_evidence_accepted %> + <%= f.hidden_field :requires_preliminary_check, value: @region.requires_preliminary_check %> + <%= f.hidden_field :sanction_check, value: @region.sanction_check %> + <%= f.hidden_field :status_check, value: @region.status_check %> + <%= f.hidden_field :teaching_authority_address, value: @region.teaching_authority_address %> + <%= f.hidden_field :teaching_authority_certificate, value: @region.teaching_authority_certificate %> + <%= f.hidden_field :teaching_authority_emails_string, value: @region.teaching_authority_emails_string %> + <%= f.hidden_field :teaching_authority_name, value: @region.teaching_authority_name %> + <%= f.hidden_field :teaching_authority_online_checker_url, value: @region.teaching_authority_online_checker_url %> + <%= f.hidden_field :teaching_authority_other, value: @region.teaching_authority_other %> + <%= f.hidden_field :teaching_authority_provides_written_statement, value: @region.teaching_authority_provides_written_statement %> + <%= f.hidden_field :teaching_authority_requires_submission_email, value: @region.teaching_authority_requires_submission_email %> + <%= f.hidden_field :teaching_authority_sanction_information, value: @region.teaching_authority_sanction_information%> + <%= f.hidden_field :teaching_authority_status_information, value: @region.teaching_authority_status_information%> + <%= f.hidden_field :teaching_authority_websites_string, value: @region.teaching_authority_websites_string %> + <%= f.hidden_field :written_statement_optional, value: @region.written_statement_optional %> + <%= f.govuk_submit "Save", name: "preview", value: "false" %> +<% end %> diff --git a/spec/system/support_interface/countries_spec.rb b/spec/system/support_interface/countries_spec.rb index cb5c4682e9..dfd24f52a1 100644 --- a/spec/system/support_interface/countries_spec.rb +++ b/spec/system/support_interface/countries_spec.rb @@ -31,11 +31,10 @@ when_i_fill_qualifications_information when_i_check_requires_preliminary_check when_i_fill_regions - and_i_save + and_i_click_save then_i_see_country_contact_preview then_i_see_region_changes_confirmation - and_i_confirm - and_i_see_a_success_banner + and_i_click_confirm when_i_click_on_a_region then_i_see_a_region @@ -55,9 +54,9 @@ when_i_fill_qualifications_information when_i_check_written_statement_optional when_i_check_requires_preliminary_check - and_i_save_and_preview + and_i_click_preview then_i_see_the_preview - and_i_see_a_success_banner + and_i_click_save when_i_visit_the_countries_page then_i_see_the_countries_page @@ -238,19 +237,15 @@ def when_i_check_requires_preliminary_check check "country-requires-preliminary-check-1-field", visible: false end - def and_i_save + def and_i_click_save click_button "Save", visible: false end - def and_i_save_and_preview - click_button "Save and preview", visible: false + def and_i_click_preview + click_button "Preview", visible: false end - def and_i_confirm + def and_i_click_confirm click_button "Confirm", visible: false end - - def and_i_see_a_success_banner - expect(page).to have_content "Successfully updated" - end end From ddc56c2fb5a2c2efd261a30e50d749172451fb00 Mon Sep 17 00:00:00 2001 From: Thomas Leese Date: Wed, 30 Aug 2023 14:12:48 +0100 Subject: [PATCH 3/5] Add SupportInteface::CountryForm This refactors the country controller of the support interface to use a form to perform the database changes, this adds better seperation of concerns and makes it easier to implement the preview feature. --- .../support_interface/countries_controller.rb | 146 +++++++++--------- app/forms/support_interface/country_form.rb | 95 ++++++++++++ .../forms/_changes_to_eligible_page.html.erb | 7 +- .../support_interface/forms/_regions.html.erb | 10 -- .../countries/confirm_edit.html.erb | 37 +++-- .../support_interface/countries/edit.html.erb | 14 +- .../support_interface/countries_spec.rb | 34 ++-- 7 files changed, 221 insertions(+), 122 deletions(-) create mode 100644 app/forms/support_interface/country_form.rb delete mode 100644 app/views/shared/support_interface/forms/_regions.html.erb diff --git a/app/controllers/support_interface/countries_controller.rb b/app/controllers/support_interface/countries_controller.rb index 2a8a6cca2d..a5da330b88 100644 --- a/app/controllers/support_interface/countries_controller.rb +++ b/app/controllers/support_interface/countries_controller.rb @@ -1,86 +1,86 @@ # frozen_string_literal: true -class SupportInterface::CountriesController < SupportInterface::BaseController - before_action :load_country, except: :index - before_action :load_edit_actions, only: %w[confirm_edit update] - - def index - authorize [:support_interface, Country] - @countries = Country.includes(:regions).order(:code) - end - - def edit - @all_regions = @country.regions.map(&:name).join("\n") - end +module SupportInterface + class CountriesController < BaseController + def index + authorize [:support_interface, Country] + @countries = Country.includes(:regions).order(:code) + end - def confirm_edit - @country.assign_attributes(country_params) - end + def edit + @form = + CountryForm.new( + country:, + eligibility_enabled: country.eligibility_enabled, + eligibility_skip_questions: country.eligibility_skip_questions, + has_regions: country.regions.count > 1, + qualifications_information: country.qualifications_information, + region_names: country.regions.pluck(:name).join("\n"), + requires_preliminary_check: country.requires_preliminary_check, + teaching_authority_address: country.teaching_authority_address, + teaching_authority_certificate: + country.teaching_authority_certificate, + teaching_authority_emails_string: + country.teaching_authority_emails_string, + teaching_authority_name: country.teaching_authority_name, + teaching_authority_online_checker_url: + country.teaching_authority_online_checker_url, + teaching_authority_other: country.teaching_authority_other, + teaching_authority_sanction_information: + country.teaching_authority_sanction_information, + teaching_authority_status_information: + country.teaching_authority_status_information, + teaching_authority_websites_string: + country.teaching_authority_websites_string, + ) + end - def update - if @country.update(country_params) - @diff_actions.each do |action| - case action[:action] - when :create - @country.regions.create!(name: action[:name]) - when :delete - region = @country.regions.find_by!(name: action[:name]) - region.eligibility_checks.delete_all - region.destroy! - end + def confirm_edit + @form = CountryForm.new(country_params.merge(country:)) + unless @form.assign_country_attributes + render :edit, status: :unprocessable_entity end - - flash[ - :success - ] = "Successfully updated #{CountryName.from_country(@country)}" - - redirect_to support_interface_countries_path - else - render :edit, status: :unprocessable_entity end - end - private + def update + @form = CountryForm.new(country_params.merge(country:)) - def load_country - @country = Country.includes(:regions).find(params[:id]) - authorize [:support_interface, @country] - end - - def load_edit_actions - @all_regions = (params.dig(:country, :all_regions) || "").chomp - @diff_actions = calculate_diff_actions - end - - def calculate_diff_actions - current_region_names = @country.regions.map(&:name) - new_region_names = @all_regions.split("\n").map(&:chomp) - new_region_names = [""] if new_region_names.empty? - - regions_to_delete = current_region_names - new_region_names - regions_to_create = new_region_names - current_region_names + if @form.save + redirect_to %i[support_interface countries] + else + render :edit, status: :unprocessable_entity + end + end - delete_actions = regions_to_delete.map { |name| { action: :delete, name: } } - create_actions = regions_to_create.map { |name| { action: :create, name: } } + private - (delete_actions + create_actions).sort_by { |action| action[:name] } - end + def country + @country ||= + begin + country = Country.includes(:regions).find(params[:id]) + authorize [:support_interface, country] + country + end + end - def country_params - params.require(:country).permit( - :eligibility_enabled, - :eligibility_skip_questions, - :qualifications_information, - :requires_preliminary_check, - :teaching_authority_name, - :teaching_authority_address, - :teaching_authority_emails_string, - :teaching_authority_websites_string, - :teaching_authority_certificate, - :teaching_authority_other, - :teaching_authority_sanction_information, - :teaching_authority_status_information, - :teaching_authority_online_checker_url, - ) + def country_params + params.require(:support_interface_country_form).permit( + :has_regions, + :region_names, + :eligibility_enabled, + :eligibility_skip_questions, + :qualifications_information, + :requires_preliminary_check, + :teaching_authority_name, + :teaching_authority_address, + :teaching_authority_emails_string, + :teaching_authority_websites_string, + :teaching_authority_certificate, + :teaching_authority_other, + :teaching_authority_sanction_information, + :teaching_authority_status_information, + :teaching_authority_online_checker_url, + ) + end end end diff --git a/app/forms/support_interface/country_form.rb b/app/forms/support_interface/country_form.rb new file mode 100644 index 0000000000..a47a73d076 --- /dev/null +++ b/app/forms/support_interface/country_form.rb @@ -0,0 +1,95 @@ +# frozen_string_literal: true + +class SupportInterface::CountryForm + include ActiveModel::Model + include ActiveModel::Attributes + + attr_accessor :country + validates :country, presence: true + + attribute :eligibility_enabled, :boolean + attribute :eligibility_skip_questions, :boolean + attribute :has_regions, :boolean + attribute :qualifications_information, :string + attribute :region_names, :string + attribute :requires_preliminary_check, :boolean + attribute :teaching_authority_address, :string + attribute :teaching_authority_certificate, :string + attribute :teaching_authority_emails_string, :string + attribute :teaching_authority_name, :string + attribute :teaching_authority_online_checker_url, :string + attribute :teaching_authority_other, :string + attribute :teaching_authority_sanction_information, :string + attribute :teaching_authority_status_information, :string + attribute :teaching_authority_websites_string, :string + + validates :eligibility_enabled, inclusion: { in: [true, false] } + validates :eligibility_skip_questions, inclusion: { in: [true, false] } + validates :has_regions, inclusion: { in: [true, false] } + validates :region_names, presence: true, if: :has_regions + validates :requires_preliminary_check, inclusion: { in: [true, false] } + + def save + return false unless assign_country_attributes + + ActiveRecord::Base.transaction do + country.save! + + diff_actions.each do |action| + case action[:action] + when :create + @country.regions.create!(name: action[:name]) + when :delete + region = @country.regions.find_by!(name: action[:name]) + region.eligibility_checks.delete_all + region.destroy! + end + end + end + + true + end + + def assign_country_attributes + return false if invalid? + + country.assign_attributes( + eligibility_enabled:, + eligibility_skip_questions:, + qualifications_information:, + requires_preliminary_check:, + teaching_authority_address:, + teaching_authority_certificate:, + teaching_authority_emails_string:, + teaching_authority_name:, + teaching_authority_online_checker_url:, + teaching_authority_other:, + teaching_authority_sanction_information:, + teaching_authority_status_information:, + teaching_authority_websites_string:, + ) + + true + end + + def diff_actions + @diff_actions ||= + begin + current_region_names = country.regions.map(&:name) + new_region_names = + has_regions ? region_names.split("\n").map(&:chomp) : [] + + new_region_names = [""] if new_region_names.empty? + + regions_to_delete = current_region_names - new_region_names + regions_to_create = new_region_names - current_region_names + + delete_actions = + regions_to_delete.map { |name| { action: :delete, name: } } + create_actions = + regions_to_create.map { |name| { action: :create, name: } } + + (delete_actions + create_actions).sort_by { |action| action[:name] } + end + end +end diff --git a/app/views/shared/support_interface/forms/_changes_to_eligible_page.html.erb b/app/views/shared/support_interface/forms/_changes_to_eligible_page.html.erb index 6fc65e1e6e..cba0d43f3a 100644 --- a/app/views/shared/support_interface/forms/_changes_to_eligible_page.html.erb +++ b/app/views/shared/support_interface/forms/_changes_to_eligible_page.html.erb @@ -1,8 +1,9 @@ -

Content changes to the eligible page (eligibility checker)

+

Content changes to the eligible page (eligibility checker)

-

Add additional information for sections:

+

Add additional information for sections:

-

‘Proof that you’re recognised as a teacher’
(Example: ‘TRCN will charge you a fee of ₦60,000 for providing the Letter of Professional standing. They’ll also need your QTS application reference number, as well as copies of your teaching documents.’)

+

Proof that you’re recognised as a teacher

+

Example: ‘TRCN will charge you a fee of ₦60,000 for providing the Letter of Professional standing. They’ll also need your QTS application reference number, as well as copies of your teaching documents.’

<%= f.govuk_text_area :teaching_authority_sanction_information, label: { text: 'Sanction Information' }, rows: 5 %> <%= f.govuk_text_area :teaching_authority_status_information, label: { text: 'Status Information' }, rows: 5 %> diff --git a/app/views/shared/support_interface/forms/_regions.html.erb b/app/views/shared/support_interface/forms/_regions.html.erb deleted file mode 100644 index d9b22abc6d..0000000000 --- a/app/views/shared/support_interface/forms/_regions.html.erb +++ /dev/null @@ -1,10 +0,0 @@ -<%= f.govuk_radio_buttons_fieldset(:has_regions, legend: { size: 'm', text: 'Regions' }, hint: { text: 'Does this country have regions within it that have a different teaching authority or service journey?' }) do %> - <%= f.govuk_radio_button :has_regions, 'no', label: { text: 'No' }, link_errors: true %> - <%= f.govuk_radio_button :has_regions, 'yes', label: { text: 'Yes' } do %> - <%= f.govuk_text_area :all_regions, - value: @all_regions, - label: nil, - hint: { text: "Add regions below, one region per line. (This will allow you to customise the service for each region.) " }, - rows: 20 %> - <% end %> -<% end %> diff --git a/app/views/support_interface/countries/confirm_edit.html.erb b/app/views/support_interface/countries/confirm_edit.html.erb index 920bc1d818..f26869020a 100644 --- a/app/views/support_interface/countries/confirm_edit.html.erb +++ b/app/views/support_interface/countries/confirm_edit.html.erb @@ -32,9 +32,7 @@

<% end %> - - -<% if @diff_actions.present? %> +<% if (diff_actions = @form.diff_actions).present? %>

Regions

@@ -46,7 +44,7 @@
    - <% @diff_actions.each do |action| %> + <% diff_actions.each do |action| %>
  • <% if action[:action] == :create %> <%= govuk_tag(text: "Create", colour: "green") %> @@ -64,20 +62,21 @@
<% end %> -<%= form_with model: [:support_interface, @country] do |f| %> - <%= f.hidden_field :all_regions, value: @all_regions %> - <%= f.hidden_field :eligibility_enabled, value: @country.eligibility_enabled %> - <%= f.hidden_field :eligibility_skip_questions, value: @country.eligibility_skip_questions %> - <%= f.hidden_field :qualifications_information, value: @country.qualifications_information %> - <%= f.hidden_field :requires_preliminary_check, value: @country.requires_preliminary_check %> - <%= f.hidden_field :teaching_authority_name, value: @country.teaching_authority_name %> - <%= f.hidden_field :teaching_authority_address, value: @country.teaching_authority_address %> - <%= f.hidden_field :teaching_authority_emails_string, value: @country.teaching_authority_emails_string %> - <%= f.hidden_field :teaching_authority_websites_string, value: @country.teaching_authority_websites_string %> - <%= f.hidden_field :teaching_authority_certificate, value: @country.teaching_authority_certificate %> - <%= f.hidden_field :teaching_authority_sanction_information, value: @country.teaching_authority_sanction_information%> - <%= f.hidden_field :teaching_authority_status_information, value: @country.teaching_authority_status_information%> - <%= f.hidden_field :teaching_authority_other, value: @country.teaching_authority_other %> - <%= f.hidden_field :teaching_authority_online_checker_url, value: @country.teaching_authority_online_checker_url %> +<%= form_with model: @form, url: [:support_interface, @country], method: :put do |f| %> + <%= f.hidden_field :eligibility_enabled, value: @form.eligibility_enabled %> + <%= f.hidden_field :eligibility_skip_questions, value: @form.eligibility_skip_questions %> + <%= f.hidden_field :has_regions, value: @form.has_regions %> + <%= f.hidden_field :qualifications_information, value: @form.qualifications_information %> + <%= f.hidden_field :region_names, value: @form.region_names %> + <%= f.hidden_field :requires_preliminary_check, value: @form.requires_preliminary_check %> + <%= f.hidden_field :teaching_authority_address, value: @form.teaching_authority_address %> + <%= f.hidden_field :teaching_authority_certificate, value: @form.teaching_authority_certificate %> + <%= f.hidden_field :teaching_authority_emails_string, value: @form.teaching_authority_emails_string %> + <%= f.hidden_field :teaching_authority_name, value: @form.teaching_authority_name %> + <%= f.hidden_field :teaching_authority_online_checker_url, value: @form.teaching_authority_online_checker_url %> + <%= f.hidden_field :teaching_authority_other, value: @form.teaching_authority_other %> + <%= f.hidden_field :teaching_authority_sanction_information, value: @form.teaching_authority_sanction_information%> + <%= f.hidden_field :teaching_authority_status_information, value: @form.teaching_authority_status_information%> + <%= f.hidden_field :teaching_authority_websites_string, value: @form.teaching_authority_websites_string %> <%= f.govuk_submit "Confirm save", prevent_double_click: false %> <% end %> diff --git a/app/views/support_interface/countries/edit.html.erb b/app/views/support_interface/countries/edit.html.erb index f8a026b74b..b13fa04212 100644 --- a/app/views/support_interface/countries/edit.html.erb +++ b/app/views/support_interface/countries/edit.html.erb @@ -2,17 +2,23 @@

<%= CountryName.from_country(@country) %>

-<%= form_with model: @country, url: confirm_edit_support_interface_country_path(@country), method: "POST" do |f| %> +<%= form_with model: @form, url: [:confirm_edit, :support_interface, @country], method: "POST" do |f| %> <%= f.govuk_error_summary %> - - <%= render "shared/support_interface/forms/regions", f: %> + + <%= f.govuk_radio_buttons_fieldset :has_regions, legend: { size: "m", text: "Regions" }, hint: { text: "Does this country have regions within it that have a different teaching authority or service journey?" } do %> + <%= f.govuk_radio_button :has_regions, :false, label: { text: "No" }, link_errors: true %> + <%= f.govuk_radio_button :has_regions, :true, label: { text: "Yes" } do %> + <%= f.govuk_text_area :region_names, label: nil, hint: { text: "Add regions below, one region per line. (This will allow you to customise the service for each region.) " }, rows: 10 %> + <% end %> + <% end %> + <%= render "shared/support_interface/forms/application_form", f: %> <%= render "shared/support_interface/forms/preliminary_check_form_fields", f: %> <%= render "shared/support_interface/forms/eligibility_checker", f: %> <%= render "shared/support_interface/forms/teaching_authority_form_fields", f: %> <%= render "shared/support_interface/forms/changes_to_eligible_page", f: %> - <%= f.govuk_text_area :qualifications_information, label: { text: '‘Proof of qualifications’' }, hint: { text: "‘(Example: ‘We cannot accept the National Certificate in Education (NCE) or the Teachers Certificate Grade II from Nigeria, as these do not meet the required level.’)’)" }, rows: 5 %> + <%= f.govuk_text_area :qualifications_information, label: { text: 'Proof of qualifications' }, hint: { text: "Example: ‘We cannot accept the National Certificate in Education (NCE) or the Teachers Certificate Grade II from Nigeria, as these do not meet the required level.’" }, rows: 5 %> <%= f.govuk_submit "Save", prevent_double_click: false %> <% end %> diff --git a/spec/system/support_interface/countries_spec.rb b/spec/system/support_interface/countries_spec.rb index dfd24f52a1..45d419cf87 100644 --- a/spec/system/support_interface/countries_spec.rb +++ b/spec/system/support_interface/countries_spec.rb @@ -140,8 +140,10 @@ def then_i_see_the_preview end def when_i_fill_regions - choose "country-has-regions-yes-field", visible: :all - fill_in "country-all-regions-field", with: "California" + choose "support-interface-country-form-has-regions-true-field", + visible: :all + fill_in "support-interface-country-form-region-names-field", + with: "California" end def when_i_select_sanction_check @@ -155,46 +157,51 @@ def when_i_select_status_check def when_i_fill_teaching_authority_name fill_in "region-teaching-authority-name-field", with: "Name" rescue Capybara::ElementNotFound - fill_in "country-teaching-authority-name-field", with: "Name" + fill_in "support-interface-country-form-teaching-authority-name-field", + with: "Name" end def when_i_fill_teaching_authority_address fill_in "region-teaching-authority-address-field", with: "Address" rescue Capybara::ElementNotFound - fill_in "country-teaching-authority-address-field", with: "Address" + fill_in "support-interface-country-form-teaching-authority-address-field", + with: "Address" end def when_i_fill_teaching_authority_emails fill_in "region-teaching-authority-emails-string-field", with: "Email address" rescue Capybara::ElementNotFound - fill_in "country-teaching-authority-emails-string-field", + fill_in "support-interface-country-form-teaching-authority-emails-string-field", with: "Email address" end def when_i_fill_teaching_authority_websites fill_in "region-teaching-authority-websites-string-field", with: "Website" rescue Capybara::ElementNotFound - fill_in "country-teaching-authority-websites-string-field", with: "Website" + fill_in "support-interface-country-form-teaching-authority-websites-string-field", + with: "Website" end def when_i_fill_teaching_authority_other fill_in "region-teaching-authority-other-field", with: "Other" rescue Capybara::ElementNotFound - fill_in "country-teaching-authority-other-field", with: "Other" + fill_in "support-interface-country-form-teaching-authority-other-field", + with: "Other" end def when_i_fill_teaching_authority_certificate fill_in "region-teaching-authority-certificate-field", with: "Certificate" rescue Capybara::ElementNotFound - fill_in "country-teaching-authority-certificate-field", with: "Certificate" + fill_in "support-interface-country-form-teaching-authority-certificate-field", + with: "Certificate" end def when_i_fill_teaching_authority_online_checker_url fill_in "region-teaching-authority-online-checker-url-field", with: "https://www.example.com/checks" rescue Capybara::ElementNotFound - fill_in "country-teaching-authority-online-checker-url-field", + fill_in "support-interface-country-form-teaching-authority-online-checker-url-field", with: "https://www.example.com/checks" end @@ -202,7 +209,7 @@ def when_i_fill_teaching_authority_sanction_information fill_in "region-teaching-authority-sanction-information-field", with: "Sanction information" rescue Capybara::ElementNotFound - fill_in "country-teaching-authority-sanction-information-field", + fill_in "support-interface-country-form-teaching-authority-sanction-information-field", with: "Sanction information" end @@ -210,7 +217,7 @@ def when_i_fill_teaching_authority_status_information fill_in "region-teaching-authority-status-information-field", with: "Status information" rescue Capybara::ElementNotFound - fill_in "country-teaching-authority-status-information-field", + fill_in "support-interface-country-form-teaching-authority-status-information-field", with: "Status information" end @@ -223,7 +230,7 @@ def when_i_fill_qualifications_information fill_in "region-qualifications-information-field", with: "Qualifications information" rescue Capybara::ElementNotFound - fill_in "country-qualifications-information-field", + fill_in "support-interface-country-form-qualifications-information-field", with: "Qualifications information" end @@ -234,7 +241,8 @@ def when_i_check_written_statement_optional def when_i_check_requires_preliminary_check check "region-requires-preliminary-check-1-field", visible: false rescue Capybara::ElementNotFound - check "country-requires-preliminary-check-1-field", visible: false + check "support-interface-country-form-requires-preliminary-check-1-field", + visible: false end def and_i_click_save From 92530c9daad47f59708a1033caa883efb6d492d7 Mon Sep 17 00:00:00 2001 From: Thomas Leese Date: Wed, 30 Aug 2023 14:27:09 +0100 Subject: [PATCH 4/5] Fix incorrect country fields This fixes the eligibility enabled and requires preliminary check fields to ensure they are working correctly. --- .../forms/_application_form.html.erb | 9 --------- .../forms/_preliminary_check_form_fields.html.erb | 2 -- .../forms/_requires_preliminary_check.html.erb | 8 ++++++++ .../support_interface/countries/edit.html.erb | 15 +++++++++++++-- app/views/support_interface/regions/edit.html.erb | 3 ++- spec/system/support_interface/countries_spec.rb | 6 +++--- 6 files changed, 26 insertions(+), 17 deletions(-) delete mode 100644 app/views/shared/support_interface/forms/_application_form.html.erb delete mode 100644 app/views/shared/support_interface/forms/_preliminary_check_form_fields.html.erb create mode 100644 app/views/shared/support_interface/forms/_requires_preliminary_check.html.erb diff --git a/app/views/shared/support_interface/forms/_application_form.html.erb b/app/views/shared/support_interface/forms/_application_form.html.erb deleted file mode 100644 index c91c841920..0000000000 --- a/app/views/shared/support_interface/forms/_application_form.html.erb +++ /dev/null @@ -1,9 +0,0 @@ -<%= f.govuk_collection_radio_buttons :eligibility_enabled, -[ -OpenStruct.new(value: :false, label: "Yes"), -OpenStruct.new(value: :true, label: "No") -], -:value, -:label, -legend: { text: "Application form" }, -hint: { text: "Do we accept applications from this country?"} %> diff --git a/app/views/shared/support_interface/forms/_preliminary_check_form_fields.html.erb b/app/views/shared/support_interface/forms/_preliminary_check_form_fields.html.erb deleted file mode 100644 index bd8a0ea7f2..0000000000 --- a/app/views/shared/support_interface/forms/_preliminary_check_form_fields.html.erb +++ /dev/null @@ -1,2 +0,0 @@ -<%= f.govuk_check_box :requires_preliminary_check, 1, 0, multiple: false, - link_errors: true, small: true, label: { text: "Will applications be subject to a preliminary check?" }%> diff --git a/app/views/shared/support_interface/forms/_requires_preliminary_check.html.erb b/app/views/shared/support_interface/forms/_requires_preliminary_check.html.erb new file mode 100644 index 0000000000..7387527a5e --- /dev/null +++ b/app/views/shared/support_interface/forms/_requires_preliminary_check.html.erb @@ -0,0 +1,8 @@ +<%= f.govuk_collection_radio_buttons :requires_preliminary_check, + [ + OpenStruct.new(value: :true, label: "Yes"), + OpenStruct.new(value: :false, label: "No") + ], + :value, + :label, + legend: { text: "Will applications be subject to a preliminary check?", size: "s" } %> diff --git a/app/views/support_interface/countries/edit.html.erb b/app/views/support_interface/countries/edit.html.erb index b13fa04212..fe2a0d2978 100644 --- a/app/views/support_interface/countries/edit.html.erb +++ b/app/views/support_interface/countries/edit.html.erb @@ -12,8 +12,19 @@ <% end %> <% end %> - <%= render "shared/support_interface/forms/application_form", f: %> - <%= render "shared/support_interface/forms/preliminary_check_form_fields", f: %> + <%= f.govuk_fieldset legend: { text: "Application form" } do %> + <%= f.govuk_collection_radio_buttons :eligibility_enabled, + [ + OpenStruct.new(value: :true, label: "Yes"), + OpenStruct.new(value: :false, label: "No") + ], + :value, + :label, + legend: { text: "Do we accept applications from this country?", size: "s" } %> + + <%= render "shared/support_interface/forms/requires_preliminary_check", f: %> + <% end %> + <%= render "shared/support_interface/forms/eligibility_checker", f: %> <%= render "shared/support_interface/forms/teaching_authority_form_fields", f: %> <%= render "shared/support_interface/forms/changes_to_eligible_page", f: %> diff --git a/app/views/support_interface/regions/edit.html.erb b/app/views/support_interface/regions/edit.html.erb index 4a0c2cd6f6..ee8729434f 100644 --- a/app/views/support_interface/regions/edit.html.erb +++ b/app/views/support_interface/regions/edit.html.erb @@ -6,7 +6,8 @@ <%= f.govuk_error_summary %> <%= f.govuk_check_box :application_form_skip_work_history, 1, 0, multiple: false, link_errors: true, small: true, label: { text: "Skip work history" } %> - <%= render "shared/support_interface/forms/preliminary_check_form_fields", f: %> + + <%= render "shared/support_interface/forms/requires_preliminary_check", f: %> <%= render "shared/support_interface/forms/status_sanctions_checks", f: %> <%= render "shared/support_interface/forms/teaching_authority_form_fields", f: %> diff --git a/spec/system/support_interface/countries_spec.rb b/spec/system/support_interface/countries_spec.rb index 45d419cf87..393d684c21 100644 --- a/spec/system/support_interface/countries_spec.rb +++ b/spec/system/support_interface/countries_spec.rb @@ -239,10 +239,10 @@ def when_i_check_written_statement_optional end def when_i_check_requires_preliminary_check - check "region-requires-preliminary-check-1-field", visible: false + choose "region-requires-preliminary-check-true-field", visible: false rescue Capybara::ElementNotFound - check "support-interface-country-form-requires-preliminary-check-1-field", - visible: false + choose "support-interface-country-form-requires-preliminary-check-true-field", + visible: false end def and_i_click_save From 34e473141dccd5d131dda440080122668eee64d8 Mon Sep 17 00:00:00 2001 From: Thomas Leese Date: Wed, 30 Aug 2023 14:32:37 +0100 Subject: [PATCH 5/5] Allow previewing country changes before saving This changes how the support console works for countries by storing any changes in the session, allowing them to be previewed before they're saved to the database. --- .../support_interface/countries_controller.rb | 21 +++---- app/forms/support_interface/country_form.rb | 9 +-- .../support_interface/country_policy.rb | 4 +- .../support_interface/countries/edit.html.erb | 6 +- ...confirm_edit.html.erb => preview.html.erb} | 59 ++++++++++--------- config/routes.rb | 2 +- .../support_interface/country_policy_spec.rb | 8 +-- .../support_interface/countries_spec.rb | 8 +-- 8 files changed, 56 insertions(+), 61 deletions(-) rename app/views/support_interface/countries/{confirm_edit.html.erb => preview.html.erb} (90%) diff --git a/app/controllers/support_interface/countries_controller.rb b/app/controllers/support_interface/countries_controller.rb index a5da330b88..99400b70c9 100644 --- a/app/controllers/support_interface/countries_controller.rb +++ b/app/controllers/support_interface/countries_controller.rb @@ -35,21 +35,22 @@ def edit ) end - def confirm_edit + def update @form = CountryForm.new(country_params.merge(country:)) - unless @form.assign_country_attributes + if @form.invalid? render :edit, status: :unprocessable_entity + elsif ActiveModel::Type::Boolean.new.cast(params[:preview]) + session[:country] = country_params + redirect_to [:preview, :support_interface, country] + else + @form.save! + redirect_to %i[support_interface countries] end end - def update - @form = CountryForm.new(country_params.merge(country:)) - - if @form.save - redirect_to %i[support_interface countries] - else - render :edit, status: :unprocessable_entity - end + def preview + @form = CountryForm.new(session[:country].merge(country:)) + @form.assign_country_attributes end private diff --git a/app/forms/support_interface/country_form.rb b/app/forms/support_interface/country_form.rb index a47a73d076..1d75ccf9cc 100644 --- a/app/forms/support_interface/country_form.rb +++ b/app/forms/support_interface/country_form.rb @@ -29,10 +29,9 @@ class SupportInterface::CountryForm validates :region_names, presence: true, if: :has_regions validates :requires_preliminary_check, inclusion: { in: [true, false] } - def save - return false unless assign_country_attributes - + def save! ActiveRecord::Base.transaction do + assign_country_attributes country.save! diff_actions.each do |action| @@ -51,8 +50,6 @@ def save end def assign_country_attributes - return false if invalid? - country.assign_attributes( eligibility_enabled:, eligibility_skip_questions:, @@ -68,8 +65,6 @@ def assign_country_attributes teaching_authority_status_information:, teaching_authority_websites_string:, ) - - true end def diff_actions diff --git a/app/policies/support_interface/country_policy.rb b/app/policies/support_interface/country_policy.rb index a5c58754f8..4db01f7f42 100644 --- a/app/policies/support_interface/country_policy.rb +++ b/app/policies/support_interface/country_policy.rb @@ -5,11 +5,11 @@ def index? user.support_console_permission? end - def confirm_edit? + def update? user.support_console_permission? end - def update? + def preview? user.support_console_permission? end end diff --git a/app/views/support_interface/countries/edit.html.erb b/app/views/support_interface/countries/edit.html.erb index fe2a0d2978..615b97f190 100644 --- a/app/views/support_interface/countries/edit.html.erb +++ b/app/views/support_interface/countries/edit.html.erb @@ -2,7 +2,7 @@

<%= CountryName.from_country(@country) %>

-<%= form_with model: @form, url: [:confirm_edit, :support_interface, @country], method: "POST" do |f| %> +<%= form_with model: @form, url: [:support_interface, @country], method: :put do |f| %> <%= f.govuk_error_summary %> <%= f.govuk_radio_buttons_fieldset :has_regions, legend: { size: "m", text: "Regions" }, hint: { text: "Does this country have regions within it that have a different teaching authority or service journey?" } do %> @@ -31,5 +31,7 @@ <%= f.govuk_text_area :qualifications_information, label: { text: 'Proof of qualifications' }, hint: { text: "Example: ‘We cannot accept the National Certificate in Education (NCE) or the Teachers Certificate Grade II from Nigeria, as these do not meet the required level.’" }, rows: 5 %> - <%= f.govuk_submit "Save", prevent_double_click: false %> + <%= f.govuk_submit "Preview", name: "preview", value: "true" do %> + <%= f.govuk_submit "Save", name: "preview", value: "false", secondary: true %> + <% end %> <% end %> diff --git a/app/views/support_interface/countries/confirm_edit.html.erb b/app/views/support_interface/countries/preview.html.erb similarity index 90% rename from app/views/support_interface/countries/confirm_edit.html.erb rename to app/views/support_interface/countries/preview.html.erb index f26869020a..b239e33689 100644 --- a/app/views/support_interface/countries/confirm_edit.html.erb +++ b/app/views/support_interface/countries/preview.html.erb @@ -4,34 +4,6 @@

Review and confirm the following changes:

-<% if @country.changes.keys.select { |k| k.starts_with?("teaching_authority_") }.any? %> -

Teaching authority

- - <%= render "shared/teaching_authority_contactable", contactable: @country %> - - <% if @country.teaching_authority_other.present? %> - <%= raw GovukMarkdown.render(@country.teaching_authority_other) %> - <% end %> - -

<%= @country.teaching_authority_certificate %>

- -

<%= @country.teaching_authority_online_checker_url %>

-<% end %> - -<% if @country.qualifications_information_changed? %> -

Qualifications information

- - <%= raw GovukMarkdown.render(@country.qualifications_information) %> -<% end %> - -<% if @country.requires_preliminary_check_changed? %> -

Preliminary check

- -

- <%= @country.requires_preliminary_check ? "Required" : "Not required" %> -

-<% end %> - <% if (diff_actions = @form.diff_actions).present? %>

Regions

@@ -62,6 +34,35 @@ <% end %> +<% if @country.eligibility_enabled_changed? %> +

Do we accept applications from this country?

+

<%= govuk_boolean_tag(@country.eligibility_enabled) %>

+<% end %> + +<% if @country.requires_preliminary_check_changed? %> +

Preliminary check

+

<%= govuk_boolean_tag(@country.requires_preliminary_check) %>

+<% end %> + +<% if @country.changes.keys.select { |k| k.starts_with?("teaching_authority_") }.any? %> +

Teaching authority

+ + <%= render "shared/teaching_authority_contactable", contactable: @country %> + + <% if @country.teaching_authority_other.present? %> + <%= raw GovukMarkdown.render(@country.teaching_authority_other) %> + <% end %> + +

<%= @country.teaching_authority_certificate %>

+ +

<%= @country.teaching_authority_online_checker_url %>

+<% end %> + +<% if @country.qualifications_information_changed? %> +

Qualifications information

+ <%= raw GovukMarkdown.render(@country.qualifications_information) %> +<% end %> + <%= form_with model: @form, url: [:support_interface, @country], method: :put do |f| %> <%= f.hidden_field :eligibility_enabled, value: @form.eligibility_enabled %> <%= f.hidden_field :eligibility_skip_questions, value: @form.eligibility_skip_questions %> @@ -78,5 +79,5 @@ <%= f.hidden_field :teaching_authority_sanction_information, value: @form.teaching_authority_sanction_information%> <%= f.hidden_field :teaching_authority_status_information, value: @form.teaching_authority_status_information%> <%= f.hidden_field :teaching_authority_websites_string, value: @form.teaching_authority_websites_string %> - <%= f.govuk_submit "Confirm save", prevent_double_click: false %> + <%= f.govuk_submit "Save", name: "preview", value: "false" %> <% end %> diff --git a/config/routes.rb b/config/routes.rb index babd055c44..0f4ba1bd7d 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -178,7 +178,7 @@ mount FeatureFlags::Engine, at: "features" resources :countries, only: %i[index edit update] do - post "confirm_edit", on: :member + get "preview", on: :member end resources :english_language_providers, only: %i[index edit update] diff --git a/spec/policies/support_interface/country_policy_spec.rb b/spec/policies/support_interface/country_policy_spec.rb index b9bceda0d1..6fc5b38894 100644 --- a/spec/policies/support_interface/country_policy_spec.rb +++ b/spec/policies/support_interface/country_policy_spec.rb @@ -15,13 +15,13 @@ it_behaves_like "a policy method requiring the support console permission" end - describe "#confirm_edit?" do - subject(:confirm_edit?) { described_class.new(user, nil).confirm_edit? } + describe "#update?" do + subject(:update?) { described_class.new(user, nil).update? } it_behaves_like "a policy method requiring the support console permission" end - describe "#update?" do - subject(:update?) { described_class.new(user, nil).update? } + describe "#preview?" do + subject(:preview?) { described_class.new(user, nil).preview? } it_behaves_like "a policy method requiring the support console permission" end end diff --git a/spec/system/support_interface/countries_spec.rb b/spec/system/support_interface/countries_spec.rb index 393d684c21..061ee2f1e9 100644 --- a/spec/system/support_interface/countries_spec.rb +++ b/spec/system/support_interface/countries_spec.rb @@ -31,10 +31,10 @@ when_i_fill_qualifications_information when_i_check_requires_preliminary_check when_i_fill_regions - and_i_click_save + and_i_click_preview then_i_see_country_contact_preview then_i_see_region_changes_confirmation - and_i_click_confirm + and_i_click_save when_i_click_on_a_region then_i_see_a_region @@ -252,8 +252,4 @@ def and_i_click_save def and_i_click_preview click_button "Preview", visible: false end - - def and_i_click_confirm - click_button "Confirm", visible: false - end end