From 9a51f7261c808be75d20590f1e8c4b4de07ea1de Mon Sep 17 00:00:00 2001 From: natdeanlewissoftwire <94526761+natdeanlewissoftwire@users.noreply.github.com> Date: Thu, 29 Sep 2022 11:12:59 +0100 Subject: [PATCH] Cldc 1390 la lookup fallback (#861) * feat: add la manual edit page * feat: make autocomplete accesible and la readable * feat: add correct skip and linking behaviour, also rename fallback page * feat: add page not found and tidy la list * feat: add redirection behaviour * feat: add coping with nil add_another_location query string * test: add postcodes mocks to return local authorities * feat: validate local authority to not be nil or "Select an option" * feat: persist add_another_location even on validation failure * feat: remove url query parsing, put validation before values added to db * refactor: spacing * tests: add new tests for edit-local-authority * tests: remove redundant bangs --- app/controllers/locations_controller.rb | 36 ++++- app/helpers/check_answers_helper.rb | 6 +- app/helpers/tab_nav_helper.rb | 13 +- .../locations/edit_local_authority.html.erb | 35 +++++ app/views/locations/index.html.erb | 4 +- app/views/schemes/check_answers.html.erb | 4 +- config/locales/en.yml | 1 + config/routes.rb | 1 + spec/helpers/tab_nav_helper_spec.rb | 2 +- spec/models/form_handler_spec.rb | 2 +- spec/request_helper.rb | 6 + spec/requests/locations_controller_spec.rb | 125 ++++++++++++++++++ 12 files changed, 222 insertions(+), 13 deletions(-) create mode 100644 app/views/locations/edit_local_authority.html.erb diff --git a/app/controllers/locations_controller.rb b/app/controllers/locations_controller.rb index b88b895fb..54e63d2cd 100644 --- a/app/controllers/locations_controller.rb +++ b/app/controllers/locations_controller.rb @@ -22,7 +22,9 @@ class LocationsController < ApplicationController if date_params_missing?(location_params) || valid_date_params?(location_params) @location = Location.new(location_params) if @location.save - if location_params[:add_another_location] == "Yes" + if @location.location_admin_district.nil? + redirect_to(location_edit_local_authority_path(id: @scheme.id, location_id: @location.id, add_another_location: location_params[:add_another_location])) + elsif location_params[:add_another_location] == "Yes" redirect_to new_location_path(@scheme) else check_answers_path = @scheme.confirmed? ? scheme_check_answers_path(@scheme, anchor: "locations") : scheme_check_answers_path(@scheme) @@ -47,16 +49,36 @@ class LocationsController < ApplicationController render_not_found and return unless @location && @scheme end + def edit_local_authority + render_not_found and return unless @location && @scheme + end + def update render_not_found and return unless @location && @scheme page = params[:location][:page] - if @location.update(location_params) + if page == "edit-local-authority" && !valid_location_admin_district?(location_params) + error_message = I18n.t("validations.location_admin_district") + @location.errors.add :location_admin_district, error_message + render :edit_local_authority, status: :unprocessable_entity + elsif @location.update(location_params) case page when "edit" - location_params[:add_another_location] == "Yes" ? redirect_to(new_location_path(@location.scheme)) : redirect_to(scheme_check_answers_path(@scheme, anchor: "locations")) + if @location.location_admin_district.nil? + redirect_to(location_edit_local_authority_path(id: @scheme.id, location_id: @location.id, add_another_location: location_params[:add_another_location])) + elsif location_params[:add_another_location] == "Yes" + redirect_to(new_location_path(@location.scheme)) + else + redirect_to(scheme_check_answers_path(@scheme, anchor: "locations")) + end when "edit-name" redirect_to(scheme_check_answers_path(@scheme, anchor: "locations")) + when "edit-local-authority" + if params[:add_another_location] == "Yes" + redirect_to(new_location_path(@location.scheme)) + else + redirect_to(scheme_check_answers_path(@scheme, anchor: "locations")) + end end else render :edit, status: :unprocessable_entity @@ -95,13 +117,13 @@ private end def authenticate_action! - if %w[new edit update create index edit_name].include?(action_name) && !((current_user.organisation == @scheme&.owning_organisation) || current_user.support?) + if %w[new edit update create index edit_name edit_local_authority].include?(action_name) && !((current_user.organisation == @scheme&.owning_organisation) || current_user.support?) render_not_found and return end end def location_params - required_params = params.require(:location).permit(:postcode, :name, :units, :type_of_unit, :add_another_location, :startdate, :mobility_type).merge(scheme_id: @scheme.id) + required_params = params.require(:location).permit(:postcode, :name, :units, :type_of_unit, :add_another_location, :startdate, :mobility_type, :location_admin_district).merge(scheme_id: @scheme.id) required_params[:postcode] = PostcodeService.clean(required_params[:postcode]) if required_params[:postcode] required_params end @@ -109,4 +131,8 @@ private def search_term params["search"] end + + def valid_location_admin_district?(location_params) + location_params["location_admin_district"] != "Select an option" + end end diff --git a/app/helpers/check_answers_helper.rb b/app/helpers/check_answers_helper.rb index 67f582c1c..b334ab073 100644 --- a/app/helpers/check_answers_helper.rb +++ b/app/helpers/check_answers_helper.rb @@ -16,7 +16,7 @@ module CheckAnswersHelper !scheme.confirmed? || editable_attributes.include?(attribute_name) end - def get_location_change_link_href(scheme, location) + def get_location_change_link_href_postcode(scheme, location) if location.confirmed? location_edit_name_path(id: scheme.id, location_id: location.id) else @@ -24,6 +24,10 @@ module CheckAnswersHelper end end + def get_location_change_link_href_location_admin_district(scheme, location) + location_edit_local_authority_path(id: scheme.id, location_id: location.id) + end + def any_questions_have_summary_card_number?(subsection, lettings_log) subsection.applicable_questions(lettings_log).map(&:check_answers_card_number).compact.length.positive? end diff --git a/app/helpers/tab_nav_helper.rb b/app/helpers/tab_nav_helper.rb index 6e482b7ff..1d2cf4063 100644 --- a/app/helpers/tab_nav_helper.rb +++ b/app/helpers/tab_nav_helper.rb @@ -6,11 +6,22 @@ module TabNavHelper [govuk_link_to(link_text, user), "User #{user.email}"].join("\n") end - def location_cell(location, link) + def location_cell_postcode(location, link) link_text = location.postcode [govuk_link_to(link_text, link, method: :patch), "Location #{location.name}"].join("\n") end + def location_cell_location_admin_district(location, link) + la = location.location_admin_district + if location.confirmed? + la + elsif la + govuk_link_to(la, link, method: :patch) + else + govuk_link_to("Select local authority", link, method: :patch) + end + end + def scheme_cell(scheme) link_text = scheme.service_name link = scheme.confirmed? ? scheme : scheme_check_answers_path(scheme) diff --git a/app/views/locations/edit_local_authority.html.erb b/app/views/locations/edit_local_authority.html.erb new file mode 100644 index 000000000..41cf74f1b --- /dev/null +++ b/app/views/locations/edit_local_authority.html.erb @@ -0,0 +1,35 @@ +<% content_for :before_content do %> + <%= govuk_back_link( + text: "Back", + href: "/schemes/#{@scheme.id}/locations", + ) %> +<% end %> + +<%= form_for(@location, method: :patch, url: location_path(location_id: @location.id, add_another_location: params[:add_another_location])) do |f| %> +
+
+ <%= f.govuk_error_summary %> + + <%= render partial: "organisations/headings", locals: { main: "What is the local authority of #{@location.postcode}?", sub: @scheme.service_name } %> + + <% la_list = FormHandler.instance.current_lettings_form.get_question("la", nil).answer_options.values %> + + <% las = la_list.map { |la| OpenStruct.new(name: la) } %> + + <%= f.govuk_collection_select :location_admin_district, + las, + :name, + :name, + label: { hidden: true }, + "data-controller": %w[conditional-filter accessible-autocomplete] %> + + <%= f.hidden_field :page, value: "edit-local-authority" %> + +
+ <%= f.govuk_submit "Save and continue" %> + + <%= govuk_link_to "Skip for now", "/schemes/#{@scheme.id}/check-answers#locations" %> +
+
+
+<% end %> diff --git a/app/views/locations/index.html.erb b/app/views/locations/index.html.erb index 9a3585cf8..6672fc661 100644 --- a/app/views/locations/index.html.erb +++ b/app/views/locations/index.html.erb @@ -52,11 +52,11 @@ <%= table.body do |body| %> <%= body.row do |row| %> <% row.cell(text: location.id) %> - <% row.cell(text: simple_format(location_cell(location, "/schemes/#{@scheme.id}/locations/#{location.id}/edit-name"), { class: "govuk-!-font-weight-bold" }, wrapper_tag: "div")) %> + <% row.cell(text: simple_format(location_cell_postcode(location, "/schemes/#{@scheme.id}/locations/#{location.id}/edit-name"), { class: "govuk-!-font-weight-bold" }, wrapper_tag: "div")) %> <% row.cell(text: location.units) %> <% row.cell(text: simple_format("#{location.type_of_unit}")) %> <% row.cell(text: location.mobility_type) %> - <% row.cell(text: location.location_admin_district) %> + <% row.cell(text: simple_format(location_cell_location_admin_district(location, "/schemes/#{@scheme.id}/locations/#{location.id}/edit-local-authority"), wrapper_tag: "div")) %> <% row.cell(text: location.startdate&.to_formatted_s(:govuk_date)) %> <% end %> <% end %> diff --git a/app/views/schemes/check_answers.html.erb b/app/views/schemes/check_answers.html.erb index f2f13c7e1..2cb154e1f 100644 --- a/app/views/schemes/check_answers.html.erb +++ b/app/views/schemes/check_answers.html.erb @@ -68,11 +68,11 @@ <%= table.body do |body| %> <%= body.row do |row| %> <% row.cell(text: location.id) %> - <% row.cell(text: simple_format(location_cell(location, get_location_change_link_href(@scheme, location)), { class: "govuk-!-font-weight-bold" }, wrapper_tag: "div")) %> + <% row.cell(text: simple_format(location_cell_postcode(location, get_location_change_link_href_postcode(@scheme, location)), { class: "govuk-!-font-weight-bold" }, wrapper_tag: "div")) %> <% row.cell(text: location.units) %> <% row.cell(text: simple_format("#{location.type_of_unit}")) %> <% row.cell(text: location.mobility_type) %> - <% row.cell(text: location.location_admin_district) %> + <% row.cell(text: simple_format(location_cell_location_admin_district(location, get_location_change_link_href_location_admin_district(@scheme, location)), wrapper_tag: "div")) %> <% row.cell(text: location.startdate&.to_formatted_s(:govuk_date)) %> <% end %> <% end %> diff --git a/config/locales/en.yml b/config/locales/en.yml index 388dff133..ef58f0cae 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -104,6 +104,7 @@ en: invalid_date: "Enter a date in the correct format, for example 31 1 2022" outside_collection_window: "Enter a date within the current collection windows" postcode: "Enter a postcode in the correct format, for example AA1 1AA" + location_admin_district: "Select a local authority" email: taken: "Enter an email address that hasn’t already been used to sign up" invalid: "Enter an email address in the correct format, like name@example.com" diff --git a/config/routes.rb b/config/routes.rb index 8bd1502b5..0be5f5b6a 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -53,6 +53,7 @@ Rails.application.routes.draw do resources :locations do get "edit-name", to: "locations#edit_name" get "edit", to: "locations#edit" + get "edit-local-authority", to: "locations#edit_local_authority" end end end diff --git a/spec/helpers/tab_nav_helper_spec.rb b/spec/helpers/tab_nav_helper_spec.rb index aa93b839d..b73200c4a 100644 --- a/spec/helpers/tab_nav_helper_spec.rb +++ b/spec/helpers/tab_nav_helper_spec.rb @@ -24,7 +24,7 @@ RSpec.describe TabNavHelper do it "returns the location link to the postcode with optional name" do link = "/schemes/#{location.scheme.id}/locations/#{location.id}/edit" expected_html = "#{location.postcode}\nLocation #{location.name}" - expect(location_cell(location, link)).to match(expected_html) + expect(location_cell_postcode(location, link)).to match(expected_html) end end diff --git a/spec/models/form_handler_spec.rb b/spec/models/form_handler_spec.rb index a9f2cb139..7703afdfc 100644 --- a/spec/models/form_handler_spec.rb +++ b/spec/models/form_handler_spec.rb @@ -91,7 +91,7 @@ RSpec.describe FormHandler do Timecop.unfreeze end - it "returns the same year as the the current start year" do + it "returns the same year as the current start year" do expect(form_handler.current_collection_start_year).to eq(2022) end diff --git a/spec/request_helper.rb b/spec/request_helper.rb index 93dc6f1dc..694a97e22 100644 --- a/spec/request_helper.rb +++ b/spec/request_helper.rb @@ -8,6 +8,12 @@ module RequestHelper WebMock.stub_request(:get, "https://api.postcodes.io/postcodes/AA11AA") .to_return(status: 200, body: "{\"status\":200,\"result\":{\"postcode\":\"AA1 1AA\",\"admin_district\":\"Westminster\",\"codes\":{\"admin_district\":\"E09000033\"}}}", headers: {}) + WebMock.stub_request(:get, "https://api.postcodes.io/postcodes/AA12AA") + .to_return(status: 200, body: "{\"status\":200,\"result\":{\"postcode\":\"AA1 2AA\",\"admin_district\":\"Westminster\",\"codes\":{\"admin_district\":\"E09000033\"}}}", headers: {}) + WebMock.stub_request(:get, "https://api.postcodes.io/postcodes/NW1L5DP") + .to_return(status: 200, body: "{\"status\":200,\"result\":{\"postcode\":\"NW1L 5DP\",\"admin_district\":\"Westminster\",\"codes\":{\"admin_district\":\"E09000033\"}}}", headers: {}) + WebMock.stub_request(:get, "https://api.postcodes.io/postcodes/ZZ11ZZ") + .to_return(status: 200, body: "{\"status\":200,\"result\":{\"postcode\":\"ZZ1 1ZZ\",\"admin_district\":\"Westminster\",\"codes\":{\"admin_district\":\"E09000033\"}}}", headers: {}) WebMock.stub_request(:post, /api.notifications.service.gov.uk\/v2\/notifications\/email/) .to_return(status: 200, body: "", headers: {}) diff --git a/spec/requests/locations_controller_spec.rb b/spec/requests/locations_controller_spec.rb index 080260128..66bd96c72 100644 --- a/spec/requests/locations_controller_spec.rb +++ b/spec/requests/locations_controller_spec.rb @@ -4,6 +4,11 @@ RSpec.describe LocationsController, type: :request do let(:page) { Capybara::Node::Simple.new(response.body) } let(:user) { FactoryBot.create(:user, :support) } let!(:scheme) { FactoryBot.create(:scheme, owning_organisation: user.organisation) } + let(:fake_2021_2022_form) { Form.new("spec/fixtures/forms/2021_2022.json") } + + before do + allow(FormHandler.instance).to receive(:current_lettings_form).and_return(fake_2021_2022_form) + end describe "#new" do context "when not signed in" do @@ -264,6 +269,29 @@ RSpec.describe LocationsController, type: :request do expect(page).to have_content("Check your answers before creating this scheme") end end + + context "when postcodes.io doesn't return a la_code" do + let(:params) do + { location: { + name: "Test", + units: "5", + type_of_unit: "Bungalow", + mobility_type: "N", + add_another_location: "No", + postcode: "AA1 4AA", + "startdate(3i)" => "", + "startdate(2i)" => "", + "startdate(1i)" => "", + } } + end + + it "redirects to la_fallback" do + post "/schemes/#{scheme.id}/locations", params: params + follow_redirect! + expect(response).to have_http_status(:ok) + expect(page).to have_content("What is the local authority of AA1 4AA?") + end + end end context "when signed in as a support user" do @@ -414,6 +442,29 @@ RSpec.describe LocationsController, type: :request do expect(page).to have_content("Check your answers before creating this scheme") end end + + context "when postcodes.io doesn't return a la_code" do + let(:params) do + { location: { + name: "Test", + units: "5", + type_of_unit: "Bungalow", + mobility_type: "N", + add_another_location: "No", + postcode: "AA1 4AA", + "startdate(3i)" => "", + "startdate(2i)" => "", + "startdate(1i)" => "", + } } + end + + it "redirects to la_fallback" do + post "/schemes/#{scheme.id}/locations", params: params + follow_redirect! + expect(response).to have_http_status(:ok) + expect(page).to have_content("What is the local authority of AA1 4AA?") + end + end end end @@ -1087,4 +1138,78 @@ RSpec.describe LocationsController, type: :request do end end end + + describe "#edit-local-authority" do + context "when not signed in" do + it "redirects to the sign in page" do + get "/schemes/1/locations/1/edit-local-authority" + expect(response).to redirect_to("/account/sign-in") + end + end + + context "when signed in as a data provider" do + let(:user) { FactoryBot.create(:user) } + + before do + sign_in user + get "/schemes/1/locations/1/edit-local-authority" + end + + it "returns 401 unauthorized" do + request + expect(response).to have_http_status(:unauthorized) + end + end + + context "when signed in as a data coordinator" do + let(:user) { FactoryBot.create(:user, :data_coordinator) } + let!(:scheme) { FactoryBot.create(:scheme, owning_organisation: user.organisation) } + let!(:location) { FactoryBot.create(:location, scheme:) } + + before do + sign_in user + get "/schemes/#{scheme.id}/locations/#{location.id}/edit-local-authority" + end + + it "returns a template for an edit-local-authority" do + expect(response).to have_http_status(:ok) + expect(page).to have_content("What is the local authority of #{location.postcode}?") + end + + context "when trying to edit location name of location that belongs to another organisation" do + let(:another_scheme) { FactoryBot.create(:scheme) } + let(:another_location) { FactoryBot.create(:location, scheme: another_scheme) } + + it "displays the new page with an error message" do + get "/schemes/#{another_scheme.id}/locations/#{another_location.id}/edit-local-authority" + expect(response).to have_http_status(:not_found) + end + end + end + + context "when signed in as a support user" do + let(:user) { FactoryBot.create(:user, :support) } + let!(:scheme) { FactoryBot.create(:scheme, owning_organisation: user.organisation) } + let!(:location) { FactoryBot.create(:location, scheme:) } + + before do + allow(user).to receive(:need_two_factor_authentication?).and_return(false) + sign_in user + get "/schemes/#{scheme.id}/locations/#{location.id}/edit-local-authority" + end + + it "returns a template for a new location" do + expect(response).to have_http_status(:ok) + expect(page).to have_content("What is the local authority of #{location.postcode}?") + end + + context "when the requested location does not exist" do + let(:location) { OpenStruct.new(id: (Location.maximum(:id) || 0) + 1) } + + it "returns not found" do + expect(response).to have_http_status(:not_found) + end + end + end + end end