From 4afd3321ebf32089bddf7e553a818a0f31fe387e Mon Sep 17 00:00:00 2001 From: J G <7750475+moarpheus@users.noreply.github.com> Date: Mon, 11 Jul 2022 09:00:11 +0100 Subject: [PATCH] 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 | 2 +- app/helpers/tab_nav_helper.rb | 9 ++------- app/models/case_log.rb | 4 ++-- app/models/location.rb | 3 +-- app/models/validations/local_authority_validations.rb | 2 -- app/models/validations/property_validations.rb | 1 - app/services/imports/case_logs_import_service.rb | 1 - app/services/postcode_service.rb | 5 +++++ app/views/locations/index.html.erb | 2 +- app/views/schemes/check_answers.html.erb | 2 +- config/initializers/postcode_regex.rb | 1 + spec/helpers/tab_nav_helper_spec.rb | 10 ++-------- spec/services/postcode_service_spec.rb | 9 +++++++++ 13 files changed, 25 insertions(+), 26 deletions(-) create mode 100644 app/services/postcode_service.rb create mode 100644 config/initializers/postcode_regex.rb create mode 100644 spec/services/postcode_service_spec.rb diff --git a/app/controllers/locations_controller.rb b/app/controllers/locations_controller.rb index d08a14209..e900a12ff 100644 --- a/app/controllers/locations_controller.rb +++ b/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 diff --git a/app/helpers/tab_nav_helper.rb b/app/helpers/tab_nav_helper.rb index 0cba9f8e4..1f6db638e 100644 --- a/app/helpers/tab_nav_helper.rb +++ b/app/helpers/tab_nav_helper.rb @@ -6,14 +6,9 @@ module TabNavHelper [govuk_link_to(link_text, user), "User #{user.email}"].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), "Location #{location.name}"].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), "Location #{location.name}"].join("\n") + [govuk_link_to(link_text, link, method: :patch), "Location #{location.name}"].join("\n") end def scheme_cell(scheme) diff --git a/app/models/case_log.rb b/app/models/case_log.rb index 49c3a8a73..305582db2 100644 --- a/app/models/case_log.rb +++ b/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") diff --git a/app/models/location.rb b/app/models/location.rb index c8be513eb..6718c79a7 100644 --- a/app/models/location.rb +++ b/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 diff --git a/app/models/validations/local_authority_validations.rb b/app/models/validations/local_authority_validations.rb index a7b3ed30d..c895880bd 100644 --- a/app/models/validations/local_authority_validations.rb +++ b/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)) diff --git a/app/models/validations/property_validations.rb b/app/models/validations/property_validations.rb index 050cdbd37..dc14fc8fd 100644 --- a/app/models/validations/property_validations.rb +++ b/app/models/validations/property_validations.rb @@ -1,7 +1,6 @@ module Validations::PropertyValidations # Validations methods need to be called 'validate_' 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 diff --git a/app/services/imports/case_logs_import_service.rb b/app/services/imports/case_logs_import_service.rb index 936218838..2a3f129df 100644 --- a/app/services/imports/case_logs_import_service.rb +++ b/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) diff --git a/app/services/postcode_service.rb b/app/services/postcode_service.rb new file mode 100644 index 000000000..737e3fd9c --- /dev/null +++ b/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 diff --git a/app/views/locations/index.html.erb b/app/views/locations/index.html.erb index 88bff9ca4..a610fc80a 100644 --- a/app/views/locations/index.html.erb +++ b/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("#{location.type_of_unit}#{location.wheelchair_adaptation == 'Yes' ? "\nWith wheelchair adaptations" : ''}")) %> <% end %> diff --git a/app/views/schemes/check_answers.html.erb b/app/views/schemes/check_answers.html.erb index 2b521fcd6..7043fa502 100644 --- a/app/views/schemes/check_answers.html.erb +++ b/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("#{location.type_of_unit}#{location.wheelchair_adaptation == 'Yes' ? "\nWith wheelchair adaptations" : ''}")) %> <% end %> diff --git a/config/initializers/postcode_regex.rb b/config/initializers/postcode_regex.rb new file mode 100644 index 000000000..1d68e6dec --- /dev/null +++ b/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 diff --git a/spec/helpers/tab_nav_helper_spec.rb b/spec/helpers/tab_nav_helper_spec.rb index 8afc45193..aa93b839d 100644 --- a/spec/helpers/tab_nav_helper_spec.rb +++ b/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 = "#{location.postcode}\nLocation #{location.name}" - 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 = "#{location.postcode}\nLocation #{location.name}" - expect(location_cell(location)).to match(expected_html) + expect(location_cell(location, link)).to match(expected_html) end end diff --git a/spec/services/postcode_service_spec.rb b/spec/services/postcode_service_spec.rb new file mode 100644 index 000000000..ecf20fdac --- /dev/null +++ b/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