From 2376ba35eb103d2ebda2aa8868cf1199c56e0928 Mon Sep 17 00:00:00 2001 From: Joseph Kempster Date: Tue, 5 Sep 2023 12:03:05 +0100 Subject: [PATCH 01/11] Add constants for global and special redirect types We want to add a new site form. In order to do this we will need an easy way to access these values. We might want to consider refining these further into enums, as this would allow us to remove some of the methods in the site model. --- app/models/site.rb | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/app/models/site.rb b/app/models/site.rb index 2fa229459..522b0a508 100644 --- a/app/models/site.rb +++ b/app/models/site.rb @@ -2,6 +2,9 @@ require "./lib/transition/path_or_url" class Site < ApplicationRecord + GLOBAL_TYPES = %w[redirect archive].freeze + SPECIAL_REDIRECT_STRATEGY_TYPES = %w[via_aka supplier].freeze + belongs_to :organisation has_many :hosts @@ -20,7 +23,7 @@ class Site < ApplicationRecord validates :organisation, presence: true validates :homepage, presence: true, non_blank_url: true validates :abbr, uniqueness: true, presence: true, format: { with: /\A[a-zA-Z0-9_-]+\z/, message: "can only contain alphanumeric characters, underscores and dashes" } - validates :special_redirect_strategy, inclusion: { in: %w[via_aka supplier], allow_nil: true } + validates :special_redirect_strategy, inclusion: { in: SPECIAL_REDIRECT_STRATEGY_TYPES, allow_nil: true } validates :global_new_url, presence: { if: :global_redirect? } validates :global_new_url, format: { without: /\?/, From 98a59f278788262650669c4938484c3affa2ba11 Mon Sep 17 00:00:00 2001 From: Joseph Kempster Date: Tue, 5 Sep 2023 12:09:54 +0100 Subject: [PATCH 02/11] Add routes for site creation We want to add sites per organisation. This add nested routes for site creation. --- config/routes.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/config/routes.rb b/config/routes.rb index fa5ba4951..62b348eb5 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -19,7 +19,9 @@ resources :hosts, only: [:index] - resources :organisations, only: %i[show index] + resources :organisations, only: %i[show index] do + resources :sites, only: %i[new create], controller: :sites + end get "mappings/find_global", to: "mappings#find_global" get "hits", to: "hits#universal_summary" From 0250f542d8dde56b7347d0362670f4c63be4d1b9 Mon Sep 17 00:00:00 2001 From: Joseph Kempster Date: Tue, 5 Sep 2023 12:38:56 +0100 Subject: [PATCH 03/11] Only validate presence of canonical host or aka if hostname is present If hostname is not provided for the host, this validation will raise an error. Add a condition that this validation only runs when hostname is present. --- app/models/host.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/host.rb b/app/models/host.rb index 7e93a51ef..397d132c5 100644 --- a/app/models/host.rb +++ b/app/models/host.rb @@ -11,7 +11,7 @@ class Host < ApplicationRecord validates :hostname, presence: true validates :hostname, hostname: true validates :site, presence: true - validate :canonical_host_id_xor_aka_present + validate :canonical_host_id_xor_aka_present, if: -> { hostname.present? } after_update :update_hits_relations, if: :saved_change_to_site_id? From da2db05f28396742bd5dd8063358012d5b6047dd Mon Sep 17 00:00:00 2001 From: Joseph Kempster Date: Tue, 5 Sep 2023 15:53:55 +0100 Subject: [PATCH 04/11] Add site form object We are aiming to archive Transition Config which is currently used to create sites in Transition. This adds a form object for the creation of a site. It's worth noting that the process currently moves existing hosts and aka hosts to the new site if they already exist (see `lib/transition/import/site_yaml_file.rb`). We aren't yet sure whether we want to create a new UI for that functionality, or to use this form. For now, this form does not have that functionality. --- app/forms/site_form.rb | 105 ++++++++++++++++++++++++++ spec/factories/site_forms.rb | 27 +++++++ spec/forms/site_form_spec.rb | 139 +++++++++++++++++++++++++++++++++++ 3 files changed, 271 insertions(+) create mode 100644 app/forms/site_form.rb create mode 100644 spec/factories/site_forms.rb create mode 100644 spec/forms/site_form_spec.rb diff --git a/app/forms/site_form.rb b/app/forms/site_form.rb new file mode 100644 index 000000000..3797dad1d --- /dev/null +++ b/app/forms/site_form.rb @@ -0,0 +1,105 @@ +class SiteForm + include ActiveModel::Model + include ActiveModel::Attributes + + attribute :organisation_slug + attribute :abbr + attribute :tna_timestamp, :datetime + attribute :homepage + attribute :homepage_title + attribute :extra_organisations + attribute :global_type + attribute :global_new_url + attribute :homepage_furl + attribute :query_params + attribute :global_redirect_append_path, :boolean, default: false + attribute :special_redirect_strategy + + attribute :hostname + attribute :aliases + + validate :validate_children + validate :validate_aliases + validate :aliases_are_unique + + def save + return false if invalid? + + ActiveRecord::Base.transaction do + site.save! + hosts.each(&:save!) + aka_hosts.each(&:save!) + end + + site + end + + def organisations + Organisation.where.not(whitehall_slug: organisation_slug) + end + +private + + def site + @site ||= Site.new( + abbr:, + tna_timestamp:, + homepage:, + organisation: Organisation.find_by(whitehall_slug: organisation_slug), + extra_organisations: Organisation.where(title: extra_organisations), + homepage_title:, + homepage_furl:, + global_type:, + global_new_url:, + global_redirect_append_path:, + query_params:, + special_redirect_strategy:, + ) + end + + def hosts + [default_host].concat(alias_hosts) + end + + def aka_hosts + hosts.map { |host| build_aka_host(host) } + end + + def default_host + @default_host ||= Host.new(hostname:, site:) + end + + def alias_hosts + return [] if aliases.nil? + + @alias_hosts ||= aliases.split(",").map do |host| + Host.new(hostname: host, site:) + end + end + + def build_aka_host(host) + Host.new(hostname: host.aka_hostname, canonical_host: host, site:) + end + + def validate_children + [site, default_host].each do |child| + errors.merge!(child.errors) if child.invalid? + end + end + + def validate_aliases + alias_hosts.each do |host| + next if host.valid? + + host.errors.each do |error| + errors.add(:aliases, "\"#{host.hostname}\" #{error.message}") + end + end + end + + def aliases_are_unique + if alias_hosts.length != alias_hosts.map(&:hostname).uniq.length + errors.add(:aliases, "must be unique") + end + end +end diff --git a/spec/factories/site_forms.rb b/spec/factories/site_forms.rb new file mode 100644 index 000000000..4260277f5 --- /dev/null +++ b/spec/factories/site_forms.rb @@ -0,0 +1,27 @@ +FactoryBot.define do + factory :site_form do + abbr { "aaib" } + organisation_slug { "air-accidents-investigation-branch" } + homepage { "https://www.gov.uk/government/organisations/air-accidents-investigation-branch" } + tna_timestamp { "20141104112824" } + hostname { "www.aaib.gov.uk" } + homepage_title { "Air accidents investigation branch" } + + trait :with_optional_fields do + homepage_furl { "www.gov.uk/aaib" } + global_type { "redirect" } + global_new_url { "https://www.gov.uk/government/organisations/air-accidents-investigation-branch/about" } + query_params { "file" } + global_redirect_append_path { true } + special_redirect_strategy { "via_aka" } + end + + trait :with_aliases do + aliases { "aaib.gov.uk,aaib.com" } + end + + trait :with_extra_organisations do + extra_organisations { ["The adjudicator's office", "Government digital service"] } + end + end +end diff --git a/spec/forms/site_form_spec.rb b/spec/forms/site_form_spec.rb new file mode 100644 index 000000000..9b6cf88dd --- /dev/null +++ b/spec/forms/site_form_spec.rb @@ -0,0 +1,139 @@ +require "rails_helper" + +describe SiteForm do + describe "validations" do + it "surfaces errors on the site model" do + site_form = build(:site_form, tna_timestamp: nil) + + expect(site_form.valid?).to be false + expect(site_form.errors[:tna_timestamp]).to include("can't be blank") + end + + it "surfaces errors on the default host model" do + site_form = build(:site_form, hostname: nil) + + expect(site_form.valid?).to be false + expect(site_form.errors[:hostname]).to include("can't be blank") + end + + describe "#aliases" do + context "when there is a validation error on an alias host" do + it "adds the error message and problematic hostname to the errors" do + site_form = build(:site_form, aliases: "aaib.gov.uk,,aaib.com") + + expect(site_form.valid?).to be false + expect(site_form.errors[:aliases]).to include("\"\" can't be blank") # hostname is blank, hence "" + end + end + + context "when the list contains duplicates" do + it "adds the error message and problematic hostname to the errors" do + site_form = build(:site_form, aliases: "aaib.gov.uk,aaib.gov.uk") + + expect(site_form.valid?).to be false + expect(site_form.errors[:aliases]).to include("must be unique") + end + end + end + end + + describe "#organisations" do + let!(:organisation) { create(:organisation, whitehall_slug: organisation_slug) } + let!(:other_organisation) { create(:organisation, whitehall_slug: "the-adjudicator-s-office", title: "The adjudicator's office") } + let(:organisation_slug) { "air-accidents-investigation-branch" } + + it "returns all organisation except the current organisation" do + site_form = build(:site_form, organisation_slug:) + + expect(site_form.organisations.length).to be 1 + expect(site_form.organisations.last).to eq other_organisation + end + end + + describe "#save" do + let!(:organisation) { create(:organisation, whitehall_slug: organisation_slug) } + let(:organisation_slug) { "air-accidents-investigation-branch" } + + context "when invalid" do + it "returns false" do + site_form = build(:site_form, tna_timestamp: nil) + + expect(site_form.save).to be false + end + end + + it "returns an instance of the new Site" do + site_form = build(:site_form) + + site = site_form.save + + expect(site).to be_a Site + expect(site).to have_attributes( + abbr: "aaib", + organisation:, + homepage: "https://www.gov.uk/government/organisations/air-accidents-investigation-branch", + tna_timestamp: Time.strptime("20141104112824", "%Y%m%d%H%M%S"), + ) + end + + it "creates the default host and aka host" do + site_form = build(:site_form) + + site = site_form.save + + expect(site.hosts.count).to be 2 + expect(site.hosts.first.hostname).to eq "www.aaib.gov.uk" + expect(site.hosts.last.hostname).to eq "aka.aaib.gov.uk" + end + + context "with optional fields" do + it "returns an instance of the new Site" do + site_form = build(:site_form, :with_optional_fields) + + site = site_form.save + + expect(site).to be_a Site + expect(site).to have_attributes( + homepage_title: "Air accidents investigation branch", + homepage_furl: "www.gov.uk/aaib", + global_type: "redirect", + global_new_url: "https://www.gov.uk/government/organisations/air-accidents-investigation-branch/about", + global_redirect_append_path: true, + query_params: "file", + special_redirect_strategy: "via_aka", + ) + end + end + + context "with a comma-separated list of alias hosts" do + it "creates the alias hosts and aka hosts" do + site_form = build(:site_form, :with_aliases) + + site = site_form.save + + alias_site_1 = site.hosts.where(hostname: "aaib.gov.uk") + alias_site_2 = site.hosts.where(hostname: "aaib.com") + + expect(alias_site_1).to exist + expect(alias_site_2).to exist + expect(site.hosts.where(hostname: "aka-aaib.gov.uk", canonical_host: alias_site_1)).to exist + expect(site.hosts.where(hostname: "aka-aaib.com", canonical_host: alias_site_2)).to exist + end + end + + context "with extra organisations" do + let!(:extra_organisation) { create(:organisation, whitehall_slug: "the-adjudicator-s-office", title: "The adjudicator's office") } + let!(:extra_organisation_2) { create(:organisation, whitehall_slug: "government-digital-service", title: "Government digital service") } + + it "creates extra organisations" do + site_form = build(:site_form, :with_extra_organisations) + + site = site_form.save + + expect(site.extra_organisations.count).to be 2 + expect(site.extra_organisations.where(title: "The adjudicator's office")).to exist + expect(site.extra_organisations.where(title: "Government digital service")).to exist + end + end + end +end From 6e3810101958d4860f1d6a5a978d86b0b4a8afc9 Mon Sep 17 00:00:00 2001 From: Joseph Kempster Date: Thu, 7 Sep 2023 11:53:20 +0100 Subject: [PATCH 05/11] Add site creation actions and views We are aiming to archive Transition Config which is currently used to create sites in Transition. This adds the actions and views necessary for a user to add a new transition. --- app/controllers/sites_controller.rb | 44 ++++++++- app/views/organisations/show.html.erb | 2 + app/views/sites/new.html.erb | 96 +++++++++++++++++++ config/breadcrumbs.rb | 5 + config/locales/en.yml | 17 ++++ features/site.feature | 13 +++ .../site_interaction_steps.rb | 30 ++++++ spec/controllers/sites_controller_spec.rb | 12 +++ spec/factories/site_forms.rb | 2 +- spec/requests/site_creation_spec.rb | 64 +++++++++++++ 10 files changed, 281 insertions(+), 4 deletions(-) create mode 100644 app/views/sites/new.html.erb create mode 100644 spec/requests/site_creation_spec.rb diff --git a/app/controllers/sites_controller.rb b/app/controllers/sites_controller.rb index 67385105a..cd3702c3d 100644 --- a/app/controllers/sites_controller.rb +++ b/app/controllers/sites_controller.rb @@ -1,11 +1,26 @@ class SitesController < ApplicationController - before_action :find_site + before_action :find_site, only: %i[edit update show] + before_action :find_organisation, only: %i[new create] before_action :check_user_is_gds_editor, only: %i[edit update] + def new + @site_form = SiteForm.new(organisation_slug: @organisation.whitehall_slug) + end + + def create + @site_form = SiteForm.new(create_params) + + if (site = @site_form.save) + redirect_to site_path(site), flash: { success: "Transition site created" } + else + render :new + end + end + def edit; end def update - if @site.update(site_params) + if @site.update(update_params) redirect_to site_path(@site), flash: { success: "Transition date updated" } else redirect_to edit_site_path(@site), flash: { alert: "We couldn't save your change" } @@ -24,7 +39,30 @@ def find_site @site = Site.find_by!(abbr: params[:id]) end - def site_params + def find_organisation + @organisation = Organisation.find_by(whitehall_slug: params[:organisation_id]) + end + + def create_params + params.require(:site_form).permit( + :organisation_slug, + :abbr, + :tna_timestamp, + :homepage, + :homepage_title, + :global_type, + :global_new_url, + :global_redirect_append_path, + :homepage_furl, + :hostname, + :query_params, + :special_redirect_strategy, + :aliases, + extra_organisations: [], + ) + end + + def update_params params.require(:site).permit(:launch_date) end diff --git a/app/views/organisations/show.html.erb b/app/views/organisations/show.html.erb index f566e3c51..d3f34db5a 100644 --- a/app/views/organisations/show.html.erb +++ b/app/views/organisations/show.html.erb @@ -10,6 +10,8 @@ <%= render 'in_conjunction_with' %> +<%= link_to "Add a transition site", new_organisation_site_path(@organisation), class: "btn btn-default" %> + <% unless @sites.empty? %>

Sites

diff --git a/app/views/sites/new.html.erb b/app/views/sites/new.html.erb new file mode 100644 index 000000000..12eadde8f --- /dev/null +++ b/app/views/sites/new.html.erb @@ -0,0 +1,96 @@ +<% breadcrumb(:new_site, @organisation) %> + +
+

New transition site

+
+ +<%= form_for @site_form, html: { role: "form" }, url: organisation_sites_path do |form| %> + <%= render "shared/error_messages", error_messages: form.object.errors.full_messages %> + +
+
+ <%= form.hidden_field :organisation_slug, value: @organisation.whitehall_slug %> + +
+ <%= form.label :abbr %> + <%= form.text_field :abbr, class: "form-control" %> +
+ +
+ <%= form.label :tna_timestamp %> + <%= form.text_field :tna_timestamp, class: "form-control" %> +
+ +
+ <%= form.label :homepage %> + <%= form.text_field :homepage, class: "form-control" %> +
+ +
+ <%= form.label :hostname %> + <%= form.text_field :hostname, class: "form-control" %> +
+ +
+ <%= form.label :homepage_title %> + <%= form.text_field :homepage_title, class: "form-control" %> +
+ +
+ <%= form.label :extra_organisations %> + <%= form.collection_select :extra_organisations, + form.object.organisations, + :id, + :title, + {}, + { multiple: true, class: "form-control" } + %> +
+ +
+ <%= form.label :homepage_furl %> + <%= form.text_field :homepage_furl, class: "form-control" %> +
+ +
+ <%= form.label :global_type %> + <%= form.collection_radio_buttons :global_type, + Site::GLOBAL_TYPES, + :to_s, + :humanize + %> +
+ +
+ <%= form.label :global_new_url %> + <%= form.text_field :global_new_url, class: "form-control" %> +
+ +
+ <%= form.label :query_params %> + <%= form.text_field :query_params, class: "form-control" %> +
+ +
+ <%= form.label :global_redirect_append_path %> + <%= form.check_box :global_redirect_append_path %> +
+ +
+ <%= form.label :special_redirect_strategy %> + <%= form.collection_radio_buttons :special_redirect_strategy, + Site::SPECIAL_REDIRECT_STRATEGY_TYPES, + :to_s, + :humanize + %> +
+ +
+ <%= form.label :aliases %> + <%= form.text_area :aliases, class: "form-control" %> +
+ + <%= form.submit 'Save', class: 'add-top-margin btn btn-success' %> +
+
+<% end %> diff --git a/config/breadcrumbs.rb b/config/breadcrumbs.rb index 87824111c..64a987f77 100644 --- a/config/breadcrumbs.rb +++ b/config/breadcrumbs.rb @@ -7,6 +7,11 @@ parent :root end +crumb :new_site do |organisation| + link "New transition site" + parent :organisation, organisation +end + crumb :site do |site| link site.default_host.hostname, site_path(site) parent :organisation, site.organisation diff --git a/config/locales/en.yml b/config/locales/en.yml index fe6ef44e6..6593fddd7 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -16,6 +16,23 @@ en: new_url: 'The URL to redirect to' mappings_batch: paths: Old URLs + activemodel: + attributes: + site_form: + abbr: Abbreviated name + host: Host + tna_timestamp: TNA timestamp + homepage: Homepage + homepage_title: Homepage title (Optional) + global_type: Global type (Optional) + global_new_url: Global new URL (Optional) + extra_organisations: Extra organisations (Optional) + homepage_furl: Homepage full URL (Optional) + options: Options (Optional) + global_redirect_append_path: Global redirect append path (Optional) + query_params: Query params (Optional) + special_redirect_strategy: Special redirect strategy (Optional) + aliases: Aliases (Optional) mappings: success: all_created: '%{created}%{tagged_with}' diff --git a/features/site.feature b/features/site.feature index 485257052..31220f167 100644 --- a/features/site.feature +++ b/features/site.feature @@ -102,6 +102,19 @@ Scenario: Jumping to a non-existent site And I jump to the site or mapping "http://not-a-site.gov.uk" Then I should see the header "Unknown site" +Scenario: Creating a site + Given I have logged in as a GDS Editor + And there are these organisations without sites: + | whitehall_slug | title | + | ukti | UK Trade & Industry | + | go-science | Government Office for Science | + When I visit the page for the UK Trade & Industry organisation + And I click the link "Add a transition site" + Then I should be on the new transition site page for the UK Trade & Industry organisation + When I fill in the new transition site fields + And I save my changes + Then I should be redirected to the new site + Scenario: Editing a site's transition date as a GDS Editor Given I have logged in as a GDS Editor And the date is 29/11/19 diff --git a/features/step_definitions/site_interaction_steps.rb b/features/step_definitions/site_interaction_steps.rb index 5c3e09bd7..020b07645 100644 --- a/features/step_definitions/site_interaction_steps.rb +++ b/features/step_definitions/site_interaction_steps.rb @@ -1,3 +1,8 @@ +When(/^I visit the page for the (.*) organisation$/) do |organisation_title| + organisation = Organisation.find_by(title: organisation_title) + visit organisation_path(organisation) +end + When(/^I visit this site page$/) do visit site_path(@site) end @@ -9,3 +14,28 @@ select("20", from: "site_launch_date_3i") click_button "Save" end + +When(/^I fill in the new transition site fields/) do + fill_in "Abbreviated name", with: "aaib" + fill_in "TNA timestamp", with: "20141104112824" + fill_in "Homepage", with: "https://www.gov.uk/government/organisations/air-accidents-investigation-branch" + fill_in "Hostname", with: "www.aaib.gov.uk" + fill_in "Homepage title", with: "Air accidents investigation branch" + select "Government Office for Science" + fill_in "Homepage full URL", with: "www.gov.uk/aaib" + choose "Redirect" + fill_in "Global new URL", with: "https://www.gov.uk/government/organisations/air-accidents-investigation-branch/about" + fill_in "Query params", with: "file" + check "Global redirect append path" + choose "Via aka" + fill_in "Aliases", with: "aaib.gov.uk,aaib.com" +end + +Then(/^I should be on the new transition site page for the (.*) organisation$/) do |organisation_title| + organisation = Organisation.find_by(title: organisation_title) + i_should_be_on_the_path new_organisation_site_path(organisation) +end + +Then(/^I should be redirected to the new site$/) do + i_should_be_on_the_path site_path(Site.last) +end diff --git a/spec/controllers/sites_controller_spec.rb b/spec/controllers/sites_controller_spec.rb index 91731622e..8a7e429b0 100644 --- a/spec/controllers/sites_controller_spec.rb +++ b/spec/controllers/sites_controller_spec.rb @@ -4,6 +4,18 @@ let(:site) { create :site, abbr: "moj" } let(:gds_bob) { create(:gds_editor, name: "Bob Terwhilliger") } + describe "#new" do + let(:organisation) { create(:organisation) } + + before { login_as gds_bob } + + it "returns a success response" do + get :new, params: { organisation_id: organisation.whitehall_slug } + + expect(response.status).to eql(200) + end + end + describe "#edit" do context "when the user does have permission" do before do diff --git a/spec/factories/site_forms.rb b/spec/factories/site_forms.rb index 4260277f5..9f3d51bdc 100644 --- a/spec/factories/site_forms.rb +++ b/spec/factories/site_forms.rb @@ -5,9 +5,9 @@ homepage { "https://www.gov.uk/government/organisations/air-accidents-investigation-branch" } tna_timestamp { "20141104112824" } hostname { "www.aaib.gov.uk" } - homepage_title { "Air accidents investigation branch" } trait :with_optional_fields do + homepage_title { "Air accidents investigation branch" } homepage_furl { "www.gov.uk/aaib" } global_type { "redirect" } global_new_url { "https://www.gov.uk/government/organisations/air-accidents-investigation-branch/about" } diff --git a/spec/requests/site_creation_spec.rb b/spec/requests/site_creation_spec.rb new file mode 100644 index 000000000..edc1b53c5 --- /dev/null +++ b/spec/requests/site_creation_spec.rb @@ -0,0 +1,64 @@ +require "rails_helper" + +describe "Site creation" do + render_views + + let!(:gds_bob) { create(:gds_editor, name: "Bob Terwhilliger") } + let(:organisation) { create(:organisation, whitehall_slug: "air-accidents-investigation-branch") } + let(:params) { attributes_for :site_form, :with_optional_fields, :with_aliases, organisation_slug: "air-accidents-investigation-branch" } + + it "redirects to the new site path" do + post organisation_sites_path(organisation), params: { site_form: params } + + expect(response).to redirect_to(site_path(Site.last)) + end + + it "creates the new site" do + post organisation_sites_path(organisation), params: { site_form: params } + + attributes = { + abbr: "aaib", + global_new_url: "https://www.gov.uk/government/organisations/air-accidents-investigation-branch/about", + global_redirect_append_path: true, + global_type: "redirect", + homepage: "https://www.gov.uk/government/organisations/air-accidents-investigation-branch", + homepage_furl: "www.gov.uk/aaib", + homepage_title: "Air accidents investigation branch", + query_params: "file", + special_redirect_strategy: "via_aka", + tna_timestamp: Time.strptime("20141104112824", "%Y%m%d%H%M%S"), + } + + expect(Site.last.attributes.with_indifferent_access).to include attributes + expect(Site.last.organisation).to eq organisation + end + + it "creates related hosts and aka hosts" do + post organisation_sites_path(organisation), params: { site_form: params } + + alias_site_1 = Host.where(hostname: "www.aaib.gov.uk") + alias_site_2 = Host.where(hostname: "aaib.gov.uk") + alias_site_3 = Host.where(hostname: "aaib.com") + + expect(alias_site_1).to exist + expect(alias_site_2).to exist + expect(alias_site_3).to exist + expect(Host.where(hostname: "aka.aaib.gov.uk", canonical_host: alias_site_1)).to exist + expect(Host.where(hostname: "aka-aaib.gov.uk", canonical_host: alias_site_2)).to exist + expect(Host.where(hostname: "aka-aaib.com", canonical_host: alias_site_3)).to exist + end + + context "with extra organisations" do + let!(:extra_organisation) { create(:organisation, whitehall_slug: "the-adjudicator-s-office", title: "The adjudicator's office") } + let!(:extra_organisation_2) { create(:organisation, whitehall_slug: "government-digital-service", title: "Government digital service") } + let(:params) { attributes_for(:site_form, :with_extra_organisations) } + + it "creates extra organisations" do + post organisation_sites_path(organisation), params: { site_form: params } + + expect(Site.last.extra_organisations.count).to be 2 + expect(Site.last.extra_organisations.where(title: "The adjudicator's office")).to exist + expect(Site.last.extra_organisations.where(title: "Government digital service")).to exist + end + end +end From e249f33cb6c10e49b0c46a3793bb777ee89f54f4 Mon Sep 17 00:00:00 2001 From: Joseph Kempster Date: Tue, 5 Sep 2023 13:23:36 +0100 Subject: [PATCH 06/11] Validate that host hostname is lowercase Currently the site importer (`lib/transition/import/site_yaml_file.rb`) downcases any hostnames. This adds validation to the host model which will be surfaced on the new site form. --- app/models/host.rb | 7 +++++++ spec/models/host_spec.rb | 12 ++++++++++++ 2 files changed, 19 insertions(+) diff --git a/app/models/host.rb b/app/models/host.rb index 397d132c5..35cd1fcd1 100644 --- a/app/models/host.rb +++ b/app/models/host.rb @@ -12,6 +12,7 @@ class Host < ApplicationRecord validates :hostname, hostname: true validates :site, presence: true validate :canonical_host_id_xor_aka_present, if: -> { hostname.present? } + validate :hostname_is_downcased, if: -> { hostname.present? } after_update :update_hits_relations, if: :saved_change_to_site_id? @@ -71,4 +72,10 @@ def update_hits_relations hits.update_all(mapping_id: nil) Transition::Import::HitsMappingsRelations.refresh!(site) end + + def hostname_is_downcased + if hostname != hostname.downcase + errors.add(:hostname, "must be lowercase") + end + end end diff --git a/spec/models/host_spec.rb b/spec/models/host_spec.rb index f9578fea1..9482f9520 100644 --- a/spec/models/host_spec.rb +++ b/spec/models/host_spec.rb @@ -75,6 +75,18 @@ expect(host.errors_on(:hostname)).to include("is an invalid hostname") end end + + context "is downcased" do + subject(:host) { build :host, hostname: "AB.com" } + + describe "#valid?" do + subject { super().valid? } + it { is_expected.to be_falsey } + end + it "should have an error for invalid hostname" do + expect(host.errors_on(:hostname)).to include("must be lowercase") + end + end end end From cb051c71b248f58ab6d24a3511914658361e9695 Mon Sep 17 00:00:00 2001 From: Joseph Kempster Date: Tue, 5 Sep 2023 15:47:58 +0100 Subject: [PATCH 07/11] Only allow `Site managers` to create new sites This adds authorization that only people with the `Site manager` role in GOV.UK Signon can create new sites. This is so that we can safely test new features before we officially archive Transition Config. --- app/controllers/sites_controller.rb | 8 +++++ app/views/organisations/show.html.erb | 4 ++- features/site.feature | 2 +- spec/controllers/sites_controller_spec.rb | 32 ++++++++++++++++++- spec/requests/site_creation_spec.rb | 2 +- .../support/shared_examples/authentication.rb | 15 +++++++++ 6 files changed, 59 insertions(+), 4 deletions(-) diff --git a/app/controllers/sites_controller.rb b/app/controllers/sites_controller.rb index cd3702c3d..a8f64d1e1 100644 --- a/app/controllers/sites_controller.rb +++ b/app/controllers/sites_controller.rb @@ -2,6 +2,7 @@ class SitesController < ApplicationController before_action :find_site, only: %i[edit update show] before_action :find_organisation, only: %i[new create] before_action :check_user_is_gds_editor, only: %i[edit update] + before_action :check_user_is_site_manager, only: %i[new create] def new @site_form = SiteForm.new(organisation_slug: @organisation.whitehall_slug) @@ -72,4 +73,11 @@ def check_user_is_gds_editor redirect_to site_path(@site), alert: message end end + + def check_user_is_site_manager + unless current_user.site_manager? + message = "Only Site Managers can access that." + redirect_to organisation_path(@organisation), alert: message + end + end end diff --git a/app/views/organisations/show.html.erb b/app/views/organisations/show.html.erb index d3f34db5a..ef1d77b46 100644 --- a/app/views/organisations/show.html.erb +++ b/app/views/organisations/show.html.erb @@ -10,7 +10,9 @@ <%= render 'in_conjunction_with' %> -<%= link_to "Add a transition site", new_organisation_site_path(@organisation), class: "btn btn-default" %> +<% if current_user.site_manager? %> + <%= link_to "Add a transition site", new_organisation_site_path(@organisation), class: "btn btn-default" %> +<% end %> <% unless @sites.empty? %>

Sites

diff --git a/features/site.feature b/features/site.feature index 31220f167..f47bf2d07 100644 --- a/features/site.feature +++ b/features/site.feature @@ -103,7 +103,7 @@ Scenario: Jumping to a non-existent site Then I should see the header "Unknown site" Scenario: Creating a site - Given I have logged in as a GDS Editor + Given I have logged in as a Site Manager And there are these organisations without sites: | whitehall_slug | title | | ukti | UK Trade & Industry | diff --git a/spec/controllers/sites_controller_spec.rb b/spec/controllers/sites_controller_spec.rb index 8a7e429b0..85e56957b 100644 --- a/spec/controllers/sites_controller_spec.rb +++ b/spec/controllers/sites_controller_spec.rb @@ -3,17 +3,47 @@ describe SitesController do let(:site) { create :site, abbr: "moj" } let(:gds_bob) { create(:gds_editor, name: "Bob Terwhilliger") } + let(:site_manager) { create(:site_manager) } describe "#new" do let(:organisation) { create(:organisation) } - before { login_as gds_bob } + before { login_as site_manager } it "returns a success response" do get :new, params: { organisation_id: organisation.whitehall_slug } expect(response.status).to eql(200) end + + context "when the user does not have permission" do + def make_request + get :new, params: { organisation_id: organisation.whitehall_slug } + end + + it_behaves_like "disallows editing by non-Site managers" + end + end + + describe "#create" do + let(:organisation) { create(:organisation) } + let(:params) { attributes_for(:site_form) } + + before { login_as site_manager } + + it "returns a success response" do + post :create, params: { organisation_id: organisation.whitehall_slug, site_form: params } + + expect(response.status).to eql(200) + end + + context "when the user does not have permission" do + def make_request + post :create, params: { organisation_id: organisation.whitehall_slug, site_form: params } + end + + it_behaves_like "disallows editing by non-Site managers" + end end describe "#edit" do diff --git a/spec/requests/site_creation_spec.rb b/spec/requests/site_creation_spec.rb index edc1b53c5..c4e53d2c5 100644 --- a/spec/requests/site_creation_spec.rb +++ b/spec/requests/site_creation_spec.rb @@ -3,7 +3,7 @@ describe "Site creation" do render_views - let!(:gds_bob) { create(:gds_editor, name: "Bob Terwhilliger") } + let!(:site_manager) { create(:site_manager) } let(:organisation) { create(:organisation, whitehall_slug: "air-accidents-investigation-branch") } let(:params) { attributes_for :site_form, :with_optional_fields, :with_aliases, organisation_slug: "air-accidents-investigation-branch" } diff --git a/spec/support/shared_examples/authentication.rb b/spec/support/shared_examples/authentication.rb index ad2c4bb67..ceffe4067 100644 --- a/spec/support/shared_examples/authentication.rb +++ b/spec/support/shared_examples/authentication.rb @@ -28,6 +28,21 @@ end end +shared_examples "disallows editing by non-Site managers" do + before do + login_as stub_user + make_request + end + + it "redirects to the organisation page" do + expect(response).to redirect_to organisation_path(organisation) + end + + it "sets a flash message" do + expect(flash[:alert]).to eql("Only Site Managers can access that.") + end +end + shared_examples "disallows editing of a global site" do before do make_request From 7e1e35e07334809cfe3011dd7a5b361efc5e7fbd Mon Sep 17 00:00:00 2001 From: Joseph Kempster Date: Wed, 6 Sep 2023 14:23:41 +0100 Subject: [PATCH 08/11] Validate that global new URL is absent if the global type is "archive" Currently, there is validation in place to prevent the creation of sites which have a global type of "redirect" without a global new URL, but the opposite validation is missing. --- app/models/site.rb | 1 + spec/models/site_spec.rb | 9 +++++++++ 2 files changed, 10 insertions(+) diff --git a/app/models/site.rb b/app/models/site.rb index 522b0a508..0b4e24d5a 100644 --- a/app/models/site.rb +++ b/app/models/site.rb @@ -25,6 +25,7 @@ class Site < ApplicationRecord validates :abbr, uniqueness: true, presence: true, format: { with: /\A[a-zA-Z0-9_-]+\z/, message: "can only contain alphanumeric characters, underscores and dashes" } validates :special_redirect_strategy, inclusion: { in: SPECIAL_REDIRECT_STRATEGY_TYPES, allow_nil: true } validates :global_new_url, presence: { if: :global_redirect? } + validates :global_new_url, absence: { if: :global_archive? } validates :global_new_url, format: { without: /\?/, message: "cannot contain a query when the path is appended", diff --git a/spec/models/site_spec.rb b/spec/models/site_spec.rb index f610da93f..2b22f7c5f 100644 --- a/spec/models/site_spec.rb +++ b/spec/models/site_spec.rb @@ -41,6 +41,15 @@ end end + context "global archive" do + subject(:site) { build(:site, global_type: "archive", global_new_url: "http://a.com/") } + + before { expect(site).not_to be_valid } + it "should validate absence of global_new_url" do + expect(site.errors[:global_new_url]).to eq(["must be blank"]) + end + end + context "global redirect with path appended" do subject(:site) { build(:site, global_type: "redirect", global_redirect_append_path: true, global_new_url: "http://a.com/?") } From ab5026ccc3ddb754feb098b8718d669832ce95d1 Mon Sep 17 00:00:00 2001 From: Joseph Kempster Date: Thu, 7 Sep 2023 10:53:41 +0100 Subject: [PATCH 09/11] Improve appearance of radio buttons In order to style these using Bootstrap, we need to provide a block to `collection_radio_buttons`. --- app/views/sites/new.html.erb | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/app/views/sites/new.html.erb b/app/views/sites/new.html.erb index 12eadde8f..fe6f32f69 100644 --- a/app/views/sites/new.html.erb +++ b/app/views/sites/new.html.erb @@ -57,8 +57,13 @@ <%= form.collection_radio_buttons :global_type, Site::GLOBAL_TYPES, :to_s, - :humanize - %> + :humanize do |button| %> +
+ <%= button.label do %> + <%= button.radio_button + button.text %> + <% end %> +
+ <% end %>
@@ -81,8 +86,13 @@ <%= form.collection_radio_buttons :special_redirect_strategy, Site::SPECIAL_REDIRECT_STRATEGY_TYPES, :to_s, - :humanize - %> + :humanize do |button| %> +
+ <%= button.label do %> + <%= button.radio_button + button.text %> + <% end %> +
+ <% end %>
From e68936de121f2f666e3e5f6c29828dec9657bdc6 Mon Sep 17 00:00:00 2001 From: Joseph Kempster Date: Fri, 8 Sep 2023 11:36:03 +0100 Subject: [PATCH 10/11] Nilify blanks on the Site model The optional fields of the site form come through as empty strings when left blank. These are then saved in the database, when ideally these would be saved as `nil` in order to preserve the previous behaviour of adding them through Transition Config for consistency. We already have a module that handles the nilification of blanks, however we need to make a couple of changes: - We add an "except list" of attributes that we don't want to nilify, such as Booleans that should be false and not nil (false.blank? == true). - We allow blank for `special_redirect_strategy` as this will now be nilified before save. --- app/models/concerns/nilify_blanks.rb | 8 +++++++ app/models/site.rb | 8 ++++++- spec/factories/site_forms.rb | 10 ++++++++ spec/models/site_spec.rb | 34 ++++++++++++++++++++++++++++ spec/requests/site_creation_spec.rb | 24 ++++++++++++++++++++ 5 files changed, 83 insertions(+), 1 deletion(-) diff --git a/app/models/concerns/nilify_blanks.rb b/app/models/concerns/nilify_blanks.rb index 100c2a725..2630f3b42 100644 --- a/app/models/concerns/nilify_blanks.rb +++ b/app/models/concerns/nilify_blanks.rb @@ -7,7 +7,15 @@ module NilifyBlanks def nilify_blanks attributes.each do |column, value| + next if nilify_except.include?(column.to_sym) + self[column] = nil if value.blank? end end + +private + + def nilify_except + [] + end end diff --git a/app/models/site.rb b/app/models/site.rb index 0b4e24d5a..0b9edcf2d 100644 --- a/app/models/site.rb +++ b/app/models/site.rb @@ -2,6 +2,8 @@ require "./lib/transition/path_or_url" class Site < ApplicationRecord + include NilifyBlanks + GLOBAL_TYPES = %w[redirect archive].freeze SPECIAL_REDIRECT_STRATEGY_TYPES = %w[via_aka supplier].freeze @@ -23,7 +25,7 @@ class Site < ApplicationRecord validates :organisation, presence: true validates :homepage, presence: true, non_blank_url: true validates :abbr, uniqueness: true, presence: true, format: { with: /\A[a-zA-Z0-9_-]+\z/, message: "can only contain alphanumeric characters, underscores and dashes" } - validates :special_redirect_strategy, inclusion: { in: SPECIAL_REDIRECT_STRATEGY_TYPES, allow_nil: true } + validates :special_redirect_strategy, inclusion: { in: SPECIAL_REDIRECT_STRATEGY_TYPES, allow_blank: true } validates :global_new_url, presence: { if: :global_redirect? } validates :global_new_url, absence: { if: :global_archive? } validates :global_new_url, @@ -135,4 +137,8 @@ def should_remove_unused_view? def remove_all_hits_view Postgres::MaterializedView.drop(precomputed_view_name) end + + def nilify_except + %i[global_redirect_append_path query_params precompute_all_hits_view] + end end diff --git a/spec/factories/site_forms.rb b/spec/factories/site_forms.rb index 9f3d51bdc..433cf23e7 100644 --- a/spec/factories/site_forms.rb +++ b/spec/factories/site_forms.rb @@ -16,6 +16,16 @@ special_redirect_strategy { "via_aka" } end + trait :with_blank_optional_fields do + homepage_title { "" } + homepage_furl { "" } + global_type { "" } + global_new_url { "" } + query_params { "" } + global_redirect_append_path { false } + special_redirect_strategy { "" } + end + trait :with_aliases do aliases { "aaib.gov.uk,aaib.com" } end diff --git a/spec/models/site_spec.rb b/spec/models/site_spec.rb index 2b22f7c5f..2ac0fee66 100644 --- a/spec/models/site_spec.rb +++ b/spec/models/site_spec.rb @@ -143,6 +143,40 @@ end end + describe "nillifying blanks before validation" do + let(:site) { create :site, homepage_furl: "" } + + subject { site.homepage_furl } + + it { is_expected.to be_nil } + + context "attributes not nilified" do + describe "#global_redirect_append_path" do + let(:site) { create :site, global_redirect_append_path: false } + + subject { site.global_redirect_append_path } + + it { is_expected.to be false } + end + + describe "#global_redirect_append_path" do + let(:site) { create :site, query_params: "" } + + subject { site.query_params } + + it { is_expected.to eq "" } + end + + describe "#precompute_all_hits_view" do + let(:site) { create :site, precompute_all_hits_view: false } + + subject { site.precompute_all_hits_view } + + it { is_expected.to be false } + end + end + end + describe "#transition_status" do let!(:site) { create :site_without_host } diff --git a/spec/requests/site_creation_spec.rb b/spec/requests/site_creation_spec.rb index c4e53d2c5..2863d942b 100644 --- a/spec/requests/site_creation_spec.rb +++ b/spec/requests/site_creation_spec.rb @@ -61,4 +61,28 @@ expect(Site.last.extra_organisations.where(title: "Government digital service")).to exist end end + + context "with blank optional fields" do + let(:params) { attributes_for(:site_form, :with_blank_optional_fields) } + + it "nullifies blanks when creating the new site" do + post organisation_sites_path(organisation), params: { site_form: params } + + attributes = { + abbr: "aaib", + homepage: "https://www.gov.uk/government/organisations/air-accidents-investigation-branch", + tna_timestamp: Time.strptime("20141104112824", "%Y%m%d%H%M%S"), + global_redirect_append_path: false, + global_new_url: nil, + special_redirect_strategy: nil, + global_type: nil, + homepage_title: nil, + homepage_furl: nil, + query_params: "", + } + + expect(Site.last.attributes.with_indifferent_access).to include attributes + expect(Site.last.organisation).to eq organisation + end + end end From 1f8f4efe14705f113139b9ef48fedeaa89c1a0f5 Mon Sep 17 00:00:00 2001 From: Joseph Kempster Date: Fri, 8 Sep 2023 10:50:11 +0100 Subject: [PATCH 11/11] Add hints to new site form inputs This copies the guidance fro Transition Config over to the new site form. This will need later refinement. --- app/views/sites/new.html.erb | 40 ++++++++++++++++++++++++------------ config/locales/en.yml | 15 ++++++++++++++ 2 files changed, 42 insertions(+), 13 deletions(-) diff --git a/app/views/sites/new.html.erb b/app/views/sites/new.html.erb index fe6f32f69..98bb83d17 100644 --- a/app/views/sites/new.html.erb +++ b/app/views/sites/new.html.erb @@ -13,27 +13,32 @@
<%= form.label :abbr %> - <%= form.text_field :abbr, class: "form-control" %> + <%= form.text_field :abbr, class: "form-control", aria: { describedby: "abbr-help" } %> + <%= I18n.t("activemodel.hint.site_form.abbr") %>
<%= form.label :tna_timestamp %> - <%= form.text_field :tna_timestamp, class: "form-control" %> + <%= form.text_field :tna_timestamp, class: "form-control", aria: { describedby: "tna-timestamp-help" } %> + <%= I18n.t("activemodel.hint.site_form.tna_timestamp").html_safe %>
<%= form.label :homepage %> - <%= form.text_field :homepage, class: "form-control" %> + <%= form.text_field :homepage, class: "form-control", aria: { describedby: "homepage-help" } %> + <%= I18n.t("activemodel.hint.site_form.homepage") %>
<%= form.label :hostname %> - <%= form.text_field :hostname, class: "form-control" %> + <%= form.text_field :hostname, class: "form-control", aria: { describedby: "hostname-help" } %> + <%= I18n.t("activemodel.hint.site_form.hostname") %>
<%= form.label :homepage_title %> - <%= form.text_field :homepage_title, class: "form-control" %> + <%= form.text_field :homepage_title, class: "form-control", aria: { describedby: "homepage-title-help" } %> + <%= I18n.t("activemodel.hint.site_form.homepage_title") %>
@@ -43,13 +48,15 @@ :id, :title, {}, - { multiple: true, class: "form-control" } + { multiple: true, class: "form-control", aria: { describedby: "extra-organisations-help" } } %> + <%= I18n.t("activemodel.hint.site_form.extra_organisations") %>
<%= form.label :homepage_furl %> - <%= form.text_field :homepage_furl, class: "form-control" %> + <%= form.text_field :homepage_furl, class: "form-control", aria: { describedby: "homepage-furl-help" } %> + <%= I18n.t("activemodel.hint.site_form.homepage_furl") %>
@@ -57,13 +64,15 @@ <%= form.collection_radio_buttons :global_type, Site::GLOBAL_TYPES, :to_s, - :humanize do |button| %> + :humanize, + aria: { describedby: "global-type-help" } do |button| %>
<%= button.label do %> <%= button.radio_button + button.text %> <% end %>
<% end %> + <%= I18n.t("activemodel.hint.site_form.global_type") %>
@@ -73,12 +82,14 @@
<%= form.label :query_params %> - <%= form.text_field :query_params, class: "form-control" %> + <%= form.text_field :query_params, class: "form-control", aria: { describedby: "query-params-help" } %> + <%= I18n.t("activemodel.hint.site_form.query_params") %>
-
+
<%= form.label :global_redirect_append_path %> - <%= form.check_box :global_redirect_append_path %> + <%= form.check_box :global_redirect_append_path, aria: { describedby: "global-redirect-append-path-help" } %> + <%= I18n.t("activemodel.hint.site_form.global_redirect_append_path") %>
@@ -86,18 +97,21 @@ <%= form.collection_radio_buttons :special_redirect_strategy, Site::SPECIAL_REDIRECT_STRATEGY_TYPES, :to_s, - :humanize do |button| %> + :humanize, + aria: { describedby: "special-redirect-strategy-help" } do |button| %>
<%= button.label do %> <%= button.radio_button + button.text %> <% end %>
<% end %> + <%= I18n.t("activemodel.hint.site_form.special_redirect_strategy") %>
<%= form.label :aliases %> - <%= form.text_area :aliases, class: "form-control" %> + <%= form.text_area :aliases, class: "form-control", aria: { describedby: "aliases-help" } %> + <%= I18n.t("activemodel.hint.site_form.aliases") %>
<%= form.submit 'Save', class: 'add-top-margin btn btn-success' %> diff --git a/config/locales/en.yml b/config/locales/en.yml index 6593fddd7..4c2551e0c 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -33,6 +33,21 @@ en: query_params: Query params (Optional) special_redirect_strategy: Special redirect strategy (Optional) aliases: Aliases (Optional) + hint: + site_form: + abbr: This is the owning organisation abbreviation followed by the abbreviated site name, separated by an underscore. For example, the "Obesity West Midlands" site which is owned by Public Health England would be "phe_obesitywm" + host: This is the primary hostname for the site. For example, "www.example.com". + tna_timestamp: This is the last good capture from the UK Government Web Archives. For example, "20131002172858". If the site has not been crawled by the National Archives, set a stub timestamp "20201010101010". + homepage: This is the URL for the new site. + hostname: This is the primary hostname for the site. + homepage_title: This is the title for 404/410 pages, it defaults to the organisation title. It should fit into the sentence "Visit the new [title] site at [full URL or homepage]" + extra_organisations: These are the additional organisations which own this site. They are used for access control in Transition. + homepage_furl: This is the friendly URL displayed on 404/410 pages. It should redirect to the homepage. It doesn't need to include 'http' or 'https'. + global_type: This sets a global redirect or archive for all paths. If "Redirect", all site URLs will redirect to the Global new URL. If "Archive", all site URLs will show a page saying the site has been archived. + query_params: A significant querystring parameter is one which on the old website changes the content in a meaningful way - which we might therefore need to map to a different place. Query string parameters should be specified in lowercase; uppercase parameters will not be preserved during canonicalisation. Enter as a colon-separated list. + global_redirect_append_path: Should the path the user supplied be appended to the URL for the global redirect? + special_redirect_strategy: When the transition is partial, some tools or content will be left behind and managed by the previous supplier. If "Via aka", the supplier is redirecting some paths to our aka domain. If "supplier", the supplier is managing redirects to gov.uk. No traffic comes through Bouncer for this site. + aliases: This is a list of alias domains. Enter as a comma-separated list. mappings: success: all_created: '%{created}%{tagged_with}'