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

@@ -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

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

app/helpers/tab_nav_helper.rb

@@ -6,11 +6,22 @@ module TabNavHelper
     [govuk_link_to(link_text, user), "<span class=\"govuk-visually-hidden\">User </span><span class=\"govuk-!-font-weight-regular app-!-colour-muted\">#{user.email}</span>"].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), "<span class=\"govuk-visually-hidden\">Location </span><span class=\"govuk-!-font-weight-regular app-!-colour-muted\">#{location.name}</span>"].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)

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| %>
+  <div class="govuk-grid-row">
+    <div class="govuk-grid-column-two-thirds">
+      <%= 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" %>
+
+      <div class="govuk-button-group">
+        <%= f.govuk_submit "Save and continue" %>
+
+        <%= govuk_link_to "Skip for now", "/schemes/#{@scheme.id}/check-answers#locations" %>
+      </div>
+    </div>
+  </div>
+<% end %>

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("<span>#{location.type_of_unit}</span>")) %>
         <% 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 %>

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("<span>#{location.type_of_unit}</span>")) %>
               <% 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 %>

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"

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

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 = "<a class=\"govuk-link\" rel=\"nofollow\" data-method=\"patch\" href=\"/schemes/#{scheme.id}/locations/#{location.id}/edit\">#{location.postcode}</a>\n<span class=\"govuk-visually-hidden\">Location </span><span class=\"govuk-!-font-weight-regular app-!-colour-muted\">#{location.name}</span>"
-      expect(location_cell(location, link)).to match(expected_html)
+      expect(location_cell_postcode(location, link)).to match(expected_html)
     end
   end
 

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
 

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: {})

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