Small refactorings (#721)

* location cell ref

* extracted regex into initializer

* extracted regex into initializer - part 2

* added PostcodeService to extract behavior

* rubo

* better test name

* moved .delete.upcase to the PostcodeService class

app/controllers/locations_controller.rb

@@ -70,7 +70,7 @@ private
 
   def location_params
     required_params = params.require(:location).permit(:postcode, :name, :total_units, :type_of_unit, :wheelchair_adaptation, :add_another_location).merge(scheme_id: @scheme.id)
-    required_params[:postcode] = required_params[:postcode].delete(" ").upcase.encode("ASCII", "UTF-8", invalid: :replace, undef: :replace, replace: "") if required_params[:postcode]
+    required_params[:postcode] = PostcodeService.clean(required_params[:postcode]) if required_params[:postcode]
     required_params
   end
 end

app/helpers/tab_nav_helper.rb

@@ -6,14 +6,9 @@ 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)
+  def location_cell(location, link)
     link_text = location.postcode
-    [govuk_link_to(link_text, "/schemes/#{location.scheme.id}/locations/#{location.id}/edit", method: :patch), "<span class=\"govuk-visually-hidden\">Location </span><span class=\"govuk-!-font-weight-regular app-!-colour-muted\">#{location.name}</span>"].join("\n")
+    [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 edit_location_name_cell(location)
-    link_text = location.postcode
-    [govuk_link_to(link_text, "/schemes/#{location.scheme.id}/locations/#{location.id}/edit-name", 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 scheme_cell(scheme)

app/models/case_log.rb

@@ -585,12 +585,12 @@ private
 
   def get_inferred_la(postcode)
     # Avoid network calls when postcode is invalid
-    return unless postcode.match(Validations::PropertyValidations::POSTCODE_REGEXP)
+    return unless postcode.match(POSTCODE_REGEXP)
 
     postcode_lookup = nil
     begin
       # URI encoding only supports ASCII characters
-      ascii_postcode = postcode.encode("ASCII", "UTF-8", invalid: :replace, undef: :replace, replace: "")
+      ascii_postcode = PostcodeService.clean(postcode)
       Timeout.timeout(5) { postcode_lookup = PIO.lookup(ascii_postcode) }
     rescue Timeout::Error
       Rails.logger.warn("Postcodes.io lookup timed out")

app/models/location.rb

@@ -1,5 +1,4 @@
 class Location < ApplicationRecord
-  include Validations::PropertyValidations
   validate :validate_postcode
   belongs_to :scheme
 
@@ -36,7 +35,7 @@ class Location < ApplicationRecord
 private
 
   def validate_postcode
-    if postcode.nil? || !postcode&.match(Validations::PropertyValidations::POSTCODE_REGEXP)
+    if postcode.nil? || !postcode&.match(POSTCODE_REGEXP)
       error_message = I18n.t("validations.postcode")
       errors.add :postcode, error_message
     end

app/models/validations/local_authority_validations.rb

@@ -1,6 +1,4 @@
 module Validations::LocalAuthorityValidations
-  POSTCODE_REGEXP = Validations::PropertyValidations::POSTCODE_REGEXP
-
   def validate_previous_accommodation_postcode(record)
     postcode = record.ppostcode_full
     if record.previous_postcode_known? && (postcode.blank? || !postcode.match(POSTCODE_REGEXP))

app/models/validations/property_validations.rb

@@ -1,7 +1,6 @@
 module Validations::PropertyValidations
   # Validations methods need to be called 'validate_<page_name>' to run on model save
   # or 'validate_' to run on submit as well
-  POSTCODE_REGEXP = /^(([A-Z]{1,2}[0-9][A-Z0-9]?|ASCN|STHL|TDCU|BBND|[BFS]IQQ|PCRN|TKCA) ?[0-9][A-Z]{2}|BFPO ?[0-9]{1,4}|(KY[0-9]|MSR|VG|AI)[ -]?[0-9]{4}|[A-Z]{2} ?[0-9]{2}|GE ?CX|GIR ?0A{2}|SAN ?TA1)$/i
 
   def validate_property_number_of_times_relet(record)
     return unless record.offered

app/services/imports/case_logs_import_service.rb

@@ -417,7 +417,6 @@ module Imports
       end
     end
 
-    POSTCODE_REGEXP = Validations::PropertyValidations::POSTCODE_REGEXP
     def compose_postcode(xml_doc, outcode, incode)
       outcode_value = string_or_nil(xml_doc, outcode)
       incode_value = string_or_nil(xml_doc, incode)

app/services/postcode_service.rb

@@ -0,0 +1,5 @@
+class PostcodeService
+  def self.clean(postcode)
+    postcode.encode("ASCII", "UTF-8", invalid: :replace, undef: :replace, replace: "").delete(" ").upcase
+  end
+end

app/views/locations/index.html.erb

@@ -36,7 +36,7 @@
         <%= table.body do |body| %>
           <%= body.row do |row| %>
             <% row.cell(text: location.id) %>
-            <% row.cell(text: simple_format(edit_location_name_cell(location), { class: "govuk-!-font-weight-bold" }, wrapper_tag: "div")) %>
+            <% 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: location.total_units) %>
             <% row.cell(text: simple_format("<span>#{location.type_of_unit}</span>#{location.wheelchair_adaptation == 'Yes' ? "\n<span class=\"govuk-!-font-weight-regular app-!-colour-muted\">With wheelchair adaptations</span>" : ''}")) %>
           <% end %>

app/views/schemes/check_answers.html.erb

@@ -89,7 +89,7 @@
             <%= table.body do |body| %>
               <%= body.row do |row| %>
                 <% row.cell(text: location.id) %>
-                <% row.cell(text: simple_format(location_cell(location), { class: "govuk-!-font-weight-bold" }, wrapper_tag: "div")) %>
+                <% row.cell(text: simple_format(location_cell(location, "/schemes/#{@scheme.id}/locations/#{location.id}/edit"), { class: "govuk-!-font-weight-bold" }, wrapper_tag: "div")) %>
                 <% row.cell(text: location.total_units) %>
                 <% row.cell(text: simple_format("<span>#{location.type_of_unit}</span>#{location.wheelchair_adaptation == 'Yes' ? "\n<span class=\"govuk-!-font-weight-regular app-!-colour-muted\">With wheelchair adaptations</span>" : ''}")) %>
               <% end %>

config/initializers/postcode_regex.rb

@@ -0,0 +1 @@
+POSTCODE_REGEXP = /^(([A-Z]{1,2}[0-9][A-Z0-9]?|ASCN|STHL|TDCU|BBND|[BFS]IQQ|PCRN|TKCA) ?[0-9][A-Z]{2}|BFPO ?[0-9]{1,4}|(KY[0-9]|MSR|VG|AI)[ -]?[0-9]{4}|[A-Z]{2} ?[0-9]{2}|GE ?CX|GIR ?0A{2}|SAN ?TA1)$/i

spec/helpers/tab_nav_helper_spec.rb

@@ -20,17 +20,11 @@ RSpec.describe TabNavHelper do
     end
   end
 
-  describe "#edit_location_name_cell" do
-    it "returns the location link to the postcode with optional name" do
-      expected_html = "<a class=\"govuk-link\" rel=\"nofollow\" data-method=\"patch\" href=\"/schemes/#{scheme.id}/locations/#{location.id}/edit-name\">#{location.postcode}</a>\n<span class=\"govuk-visually-hidden\">Location </span><span class=\"govuk-!-font-weight-regular app-!-colour-muted\">#{location.name}</span>"
-      expect(edit_location_name_cell(location)).to match(expected_html)
-    end
-  end
-
   describe "#location_cell" 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)).to match(expected_html)
+      expect(location_cell(location, link)).to match(expected_html)
     end
   end
 

spec/services/postcode_service_spec.rb

@@ -0,0 +1,9 @@
+require "rails_helper"
+
+describe PostcodeService do
+  let(:postcode) { "s r81LS\u00A0" }
+
+  it "returns clean postcode" do
+    expect(described_class.clean(postcode)).to eq "SR81LS"
+  end
+end