Merge pull request #484 from communitiesuk/cldc-506-organisation_la_import_and_validation

CLDC-506: Some organisations only operate in specific LAs

app/controllers/organisations_controller.rb

@@ -51,8 +51,6 @@ private
   end
 
   def find_resource
-    return if current_user.support?
-
     @organisation = Organisation.find(params[:id])
   end
 end

app/helpers/details_table_helper.rb

@@ -0,0 +1,10 @@
+module DetailsTableHelper
+  def details_html(attribute)
+    if attribute[:format] == :bullet
+      list = attribute[:value].map { |la| "<li>#{la}</li>" }.join
+      simple_format(list, { class: "govuk-list govuk-list--bullet" }, wrapper_tag: "ul")
+    else
+      simple_format(attribute[:value].to_s, {}, wrapper_tag: "div")
+    end
+  end
+end

app/helpers/user_helper.rb

@@ -30,4 +30,8 @@ module UserHelper
   def can_edit_key_contact?(_user, current_user)
     current_user.data_coordinator? || current_user.support?
   end
+
+  def can_edit_org?(current_user)
+    current_user.data_coordinator? || current_user.support?
+  end
 end

app/models/form.rb

@@ -26,7 +26,7 @@ class Form
 
   def get_question(id, case_log)
     all_questions = questions.select { |q| q.id == id.to_s.underscore }
-    routed_question = all_questions.find { |q| q.page.routed_to?(case_log) }
+    routed_question = all_questions.find { |q| q.page.routed_to?(case_log) } if case_log
     routed_question || all_questions[0]
   end
 

app/models/form_handler.rb

@@ -10,6 +10,10 @@ class FormHandler
     @forms[form]
   end
 
+  def current_form
+    forms[forms.keys.max_by(&:to_i)]
+  end
+
 private
 
   def get_all_forms

app/models/local_authority.rb

@@ -0,0 +1,5 @@
+class LocalAuthority
+  def self.ons_code_mappings
+    FormHandler.instance.current_form.get_question("la", nil).answer_options
+  end
+end

app/models/organisation.rb

@@ -3,6 +3,7 @@ class Organisation < ApplicationRecord
   has_many :owned_case_logs, class_name: "CaseLog", foreign_key: "owning_organisation_id"
   has_many :managed_case_logs, class_name: "CaseLog", foreign_key: "managing_organisation_id"
   has_many :data_protection_confirmations
+  has_many :organisation_las
 
   has_paper_trail
 
@@ -35,13 +36,21 @@ class Organisation < ApplicationRecord
     !!data_protection_confirmations.order(created_at: :desc).first&.confirmed
   end
 
+  def local_authorities
+    organisation_las.pluck(:ons_code).map { |ons_code| ons_code }
+  end
+
+  def local_authority_names
+    local_authorities.map { |ons_code| LocalAuthority.ons_code_mappings[ons_code] }
+  end
+
   def display_attributes
     [
       { name: "name", value: name, editable: true },
       { name: "address", value: address_string, editable: true },
       { name: "telephone_number", value: phone, editable: true },
       { name: "type", value: "Org type", editable: false },
-      { name: "local_authorities_operated_in", value: local_authorities, editable: false },
+      { name: "local_authorities_operated_in", value: local_authority_names, editable: false, format: :bullet },
       { name: "holds_own_stock", value: holds_own_stock.to_s.humanize, editable: false },
       { name: "other_stock_owners", value: other_stock_owners, editable: false },
       { name: "managing_agents", value: managing_agents, editable: false },

app/models/organisation_la.rb

@@ -0,0 +1,3 @@
+class OrganisationLa < ApplicationRecord
+  belongs_to :organisation
+end

app/models/validations/local_authority_validations.rb

@@ -8,4 +8,58 @@ module Validations::LocalAuthorityValidations
       record.errors.add :ppostcode_full, error_message
     end
   end
+
+  def validate_la(record)
+    if record.la.present? && !LONDON_BOROUGHS.include?(record.la) && record.is_london_rent?
+      record.errors.add :la, I18n.t("validations.property.la.london_rent")
+      if record.postcode_known? && record.postcode_full.present?
+        record.errors.add :postcode_full, I18n.t("validations.property.la.london_rent_postcode")
+      end
+    end
+
+    if record.la_known? && record.la.blank?
+      record.errors.add :la, I18n.t("validations.property.la.la_known")
+    end
+
+    if record.owning_organisation && record.owning_organisation.local_authorities.present? &&
+        record.la && !record.owning_organisation.local_authorities.include?(record.la)
+      la_name = record.form.get_question("la", record).label_from_value(record.la)
+      org_name = record.owning_organisation.name
+      record.errors.add :la, I18n.t("validations.property.la.la_invalid_for_org", org_name:, la_name:)
+    end
+  end
+
+  LONDON_BOROUGHS = %w[E09000001
+                       E09000002
+                       E09000003
+                       E09000004
+                       E09000005
+                       E09000006
+                       E09000007
+                       E09000008
+                       E09000009
+                       E09000010
+                       E09000011
+                       E09000012
+                       E09000013
+                       E09000014
+                       E09000015
+                       E09000016
+                       E09000017
+                       E09000018
+                       E09000019
+                       E09000020
+                       E09000021
+                       E09000022
+                       E09000023
+                       E09000024
+                       E09000025
+                       E09000026
+                       E09000027
+                       E09000028
+                       E09000029
+                       E09000030
+                       E09000031
+                       E09000032
+                       E09000033].freeze
 end

app/models/validations/property_validations.rb

@@ -21,52 +21,6 @@ module Validations::PropertyValidations
     end
   end
 
-  LONDON_BOROUGHS = %w[E09000001
-                       E09000002
-                       E09000003
-                       E09000004
-                       E09000005
-                       E09000006
-                       E09000007
-                       E09000008
-                       E09000009
-                       E09000010
-                       E09000011
-                       E09000012
-                       E09000013
-                       E09000014
-                       E09000015
-                       E09000016
-                       E09000017
-                       E09000018
-                       E09000019
-                       E09000020
-                       E09000021
-                       E09000022
-                       E09000023
-                       E09000024
-                       E09000025
-                       E09000026
-                       E09000027
-                       E09000028
-                       E09000029
-                       E09000030
-                       E09000031
-                       E09000032
-                       E09000033].freeze
-  def validate_la(record)
-    if record.la.present? && !LONDON_BOROUGHS.include?(record.la) && record.is_london_rent?
-      record.errors.add :la, I18n.t("validations.property.la.london_rent")
-      if record.postcode_known? && record.postcode_full.present?
-        record.errors.add :postcode_full, I18n.t("validations.property.la.london_rent_postcode")
-      end
-    end
-
-    if record.la_known? && record.la.blank?
-      record.errors.add :la, I18n.t("validations.property.la.la_known")
-    end
-  end
-
   REFERRAL_INVALID_TMP = [8, 10, 12, 13, 14, 15].freeze
   def validate_rsnvac(record)
     if !record.first_time_property_let_as_social_housing? && record.has_first_let_vacancy_reason?

app/services/imports/organisation_la_import_service.rb

@@ -0,0 +1,23 @@
+module Imports
+  class OrganisationLaImportService < ImportService
+    def create_organisation_las(folder)
+      import_from(folder, :create_organisation_la)
+    end
+
+  private
+
+    def create_organisation_la(xml_document)
+      xml_document.remove_namespaces!
+      organisation = Organisation.find_by(old_org_id: record_field_value(xml_document, "InstitutionId"))
+
+      OrganisationLa.create!(
+        organisation:,
+        ons_code: record_field_value(xml_document, "ONSCode"),
+      )
+    end
+
+    def record_field_value(xml_document, field)
+      xml_document.at_xpath("//#{field}")&.text
+    end
+  end
+end

app/views/organisations/show.html.erb

@@ -9,10 +9,10 @@
     <%= govuk_summary_list do |summary_list| %>
       <% @organisation.display_attributes.each do |attr| %>
 
-        <% if current_user.data_coordinator? && attr[:editable] %>
+        <% if can_edit_org?(current_user) && attr[:editable] %>
           <%= summary_list.row do |row| %>
             <% row.key { attr[:name].to_s.humanize } %>
-            <% row.value { simple_format(attr[:value].to_s, {}, wrapper_tag: "div") } %>
+            <% row.value { details_html(attr) } %>
             <% row.action(
               visually_hidden_text: "name",
               href: edit_organisation_path,
@@ -22,7 +22,7 @@
         <% else %>
           <%= summary_list.row do |row| %>
             <% row.key { attr[:name].to_s.humanize } %>
-            <% row.value { simple_format(attr[:value].to_s, {}, wrapper_tag: "div") } %>
+            <% row.value { details_html(attr) } %>
             <% row.action %>
           <% end %>
         <% end %>

config/locales/en.yml

@@ -70,6 +70,7 @@ en:
         london_rent: "Local authority must be in London"
         london_rent_postcode: "The postcode must be a London postcode because you told us the rent type is London Affordable Rent or London Living Rent"
         la_known: "Enter name of local authority"
+        la_invalid_for_org: "%{org_name} does not operate in %{la_name}"
       rsnvac:
         first_let_not_social: "Reason for vacancy cannot be first let if unit has been previously let as social housing"
         first_let_social: "Reason for vacancy must be first let if unit has been previously let as social housing"

db/migrate/20220420165451_create_organisation_la.rb

@@ -0,0 +1,11 @@
+class CreateOrganisationLa < ActiveRecord::Migration[7.0]
+  def change
+    create_table :organisation_las do |t|
+      t.belongs_to :organisation
+      t.column :ons_code, :string
+
+      t.timestamps
+    end
+    remove_column :organisations, :local_authorities, :string
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[7.0].define(version: 2022_04_11_092231) do
+ActiveRecord::Schema[7.0].define(version: 2022_04_20_165451) do
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
 
@@ -256,6 +256,14 @@ ActiveRecord::Schema[7.0].define(version: 2022_04_11_092231) do
     t.datetime "created_at", default: -> { "CURRENT_TIMESTAMP" }
   end
 
+  create_table "organisation_las", force: :cascade do |t|
+    t.bigint "organisation_id"
+    t.string "ons_code"
+    t.datetime "created_at", null: false
+    t.datetime "updated_at", null: false
+    t.index ["organisation_id"], name: "index_organisation_las_on_organisation_id"
+  end
+
   create_table "organisations", force: :cascade do |t|
     t.string "name"
     t.string "phone"
@@ -263,7 +271,6 @@ ActiveRecord::Schema[7.0].define(version: 2022_04_11_092231) do
     t.string "address_line1"
     t.string "address_line2"
     t.string "postcode"
-    t.string "local_authorities"
     t.boolean "holds_own_stock"
     t.string "other_stock_owners"
     t.string "managing_agents"

db/seeds.rb

@@ -11,7 +11,6 @@ org = Organisation.create!(
   address_line1: "2 Marsham Street",
   address_line2: "London",
   postcode: "SW1P 4DF",
-  local_authorities: "None",
   holds_own_stock: false,
   other_stock_owners: "None",
   managing_agents: "None",

lib/tasks/data_import.rake

@@ -14,6 +14,8 @@ namespace :core do
       Imports::UserImportService.new(storage_service).create_users(path)
     when "data-protection-confirmation"
       Imports::DataProtectionConfirmationImportService.new(storage_service).create_data_protection_confirmations(path)
+    when "organisation-las"
+      Imports::OrganisationLaImportService.new(storage_service).create_organisation_las(path)
     else
       raise "Type #{type} is not supported by data_import"
     end

spec/factories/organisation.rb

@@ -8,4 +8,11 @@ FactoryBot.define do
     created_at { Time.zone.now }
     updated_at { Time.zone.now }
   end
+
+  factory :organisation_la do
+    organisation
+    ons_code { "E07000041" }
+    created_at { Time.zone.now }
+    updated_at { Time.zone.now }
+  end
 end

spec/fixtures/softwire_imports/organisation_las/00013f30e159d7f72a3abe9ea93fb5b685d311e4.xml

@@ -0,0 +1,6 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<institution xmlns="http://www.ons.gov.uk/institutions-and-ons-la">
+  <InstitutionId>44026acc7ed5c29516b26f2a5deb639e5e37966d</InstitutionId>
+  <AssociationId>7076</AssociationId>
+  <ONSCode>E07000041</ONSCode>
+</institution>

spec/helpers/details_table_helper_spec.rb

@@ -0,0 +1,31 @@
+require "rails_helper"
+
+RSpec.describe DetailsTableHelper do
+  describe "details html" do
+    let(:details) { details_html(attribute) }
+
+    context "when given a simple attribute" do
+      let(:attribute) { { name: "name", value: "Dummy org", editable: true } }
+
+      it "displays the string wrapped in a div" do
+        expect(details).to eq("<div>Dummy org</div>")
+      end
+    end
+
+    context "when given a bullet point list of attibutes" do
+      let(:list) { %w[Camden Westminster Bristol] }
+      let(:attribute) do
+        {
+          name: "local_authorities_operated_in",
+          value: list,
+          editable: false,
+          format: :bullet,
+        }
+      end
+
+      it "displays the string wrapped in an unordered list with the correct classes" do
+        expect(details).to eq("<ul class=\"govuk-list govuk-list--bullet\"><li>Camden</li><li>Westminster</li><li>Bristol</li></ul>")
+      end
+    end
+  end
+end

spec/helpers/user_helper_spec.rb

@@ -135,5 +135,29 @@ RSpec.describe UserHelper do
         end
       end
     end
+
+    context "when the user is a data provider viewing organisation details" do
+      let(:current_user) { FactoryBot.create(:user, :data_provider) }
+
+      it "does not allow changing details" do
+        expect(can_edit_org?(current_user)).to be false
+      end
+    end
+
+    context "when the user is a data coordinator viewing organisation details" do
+      let(:current_user) { FactoryBot.create(:user, :data_coordinator) }
+
+      it "does not allow changing details" do
+        expect(can_edit_org?(current_user)).to be true
+      end
+    end
+
+    context "when the user is a support user viewing organisation details" do
+      let(:current_user) { FactoryBot.create(:user, :support) }
+
+      it "does not allow changing details" do
+        expect(can_edit_org?(current_user)).to be true
+      end
+    end
   end
 end

spec/lib/tasks/data_import_spec.rb

@@ -72,6 +72,24 @@ describe "rake core:data_import", type: :task do
     end
   end
 
+  context "when importing organisation local authority data" do
+    let(:type) { "organisation-las" }
+    let(:import_service) { instance_double(Imports::OrganisationLaImportService) }
+    let(:fixture_path) { "spec/fixtures/softwire_imports/organisation_las" }
+
+    before do
+      allow(Imports::OrganisationLaImportService).to receive(:new).and_return(import_service)
+    end
+
+    it "creates an organisation la from the given XML file" do
+      expect(StorageService).to receive(:new).with(paas_config_service, instance_name)
+      expect(Imports::OrganisationLaImportService).to receive(:new).with(storage_service)
+      expect(import_service).to receive(:create_organisation_las).with(fixture_path)
+
+      task.invoke(type, fixture_path)
+    end
+  end
+
   it "raises an exception if no parameters are provided" do
     expect { task.invoke }.to raise_error(/Usage/)
   end

spec/models/form_handler_spec.rb

@@ -21,6 +21,15 @@ RSpec.describe FormHandler do
     end
   end
 
+  describe "Current form" do
+    it "returns the latest form by date" do
+      form_handler = described_class.instance
+      form = form_handler.current_form
+      expect(form).to be_a(Form)
+      expect(form.start_date.year).to eq(2022)
+    end
+  end
+
   it "loads the form once at boot time" do
     form_handler = described_class.instance
     expect(Form).not_to receive(:new).with(:any, test_form_name)

spec/models/local_authority_spec.rb

@@ -0,0 +1,16 @@
+require "rails_helper"
+
+RSpec.describe LocalAuthority, type: :model do
+  describe "ons code mapping" do
+    let(:form) { Form.new("spec/fixtures/forms/2021_2022.json", "2021_2022") }
+
+    before do
+      allow(FormHandler.instance).to receive(:current_form).and_return(form)
+    end
+
+    it "maps ONS code to local authority names" do
+      expect(described_class.ons_code_mappings).to be_a(Hash)
+      expect(described_class.ons_code_mappings["E07000178"]).to eq("Oxford")
+    end
+  end
+end

spec/models/organisation_spec.rb

@@ -29,6 +29,34 @@ RSpec.describe Organisation, type: :model do
       end
     end
 
+    context "when the organisation only operates in specific local authorities" do
+      let(:ons_code_mappings) do
+        {
+          "" => "Select an option",
+          "E07000223" => "Adur",
+          "E09000023" => "Lewisham",
+          "E08000003" => "Manchester",
+          "E07000178" => "Oxford",
+          "E07000114" => "Thanet",
+          "E09000033" => "Westminster",
+          "E06000014" => "York",
+        }
+      end
+
+      before do
+        FactoryBot.create(:organisation_la, organisation_id: organisation.id, ons_code: "E07000178")
+        allow(LocalAuthority).to receive(:ons_code_mappings).and_return(ons_code_mappings)
+      end
+
+      it "has local authorities associated" do
+        expect(organisation.local_authorities).to eq(%w[E07000178])
+      end
+
+      it "maps the ons codes to LA names for display" do
+        expect(organisation.local_authority_names).to eq(%w[Oxford])
+      end
+    end
+
     context "with case logs" do
       let(:other_organisation) { FactoryBot.create(:organisation) }
       let!(:owned_case_log) do

spec/models/validations/local_authority_validations_spec.rb

@@ -35,4 +35,82 @@ RSpec.describe Validations::LocalAuthorityValidations do
       expect(record.errors["ppostcode_full"]).to include(match I18n.t("validations.postcode"))
     end
   end
+
+  describe "#validate_la" do
+    context "when the rent type is London affordable" do
+      let(:expected_error) { I18n.t("validations.property.la.london_rent") }
+
+      it "validates that the local authority is in London" do
+        record.la = "E07000105"
+        record.rent_type = 2
+        local_auth_validator.validate_la(record)
+        expect(record.errors["la"]).to include(match(expected_error))
+        expect(record.errors["postcode_full"]).to be_empty
+      end
+
+      it "expects that the local authority is in London" do
+        record.la = "E09000033"
+        record.rent_type = 2
+        local_auth_validator.validate_la(record)
+        expect(record.errors["la"]).to be_empty
+      end
+
+      context "when the la has been derived from a known postcode" do
+        let(:expected_error) { I18n.t("validations.property.la.london_rent_postcode") }
+
+        it "also adds an error to the postcode field" do
+          record.la = "E07000105"
+          record.rent_type = 2
+          record.postcode_known = 1
+          record.postcode_full = "BN18 7TR"
+          local_auth_validator.validate_la(record)
+          expect(record.errors["postcode_full"]).to include(match(expected_error))
+        end
+      end
+    end
+
+    context "when previous la is known" do
+      it "la has to be provided" do
+        record.la_known = 1
+        local_auth_validator.validate_la(record)
+        expect(record.errors["la"])
+          .to include(match I18n.t("validations.property.la.la_known"))
+      end
+    end
+
+    context "when the organisation only operates in specific local authorities" do
+      let(:organisation) { FactoryBot.create(:organisation) }
+      let(:record) { FactoryBot.create(:case_log, owning_organisation: organisation) }
+
+      before do
+        FactoryBot.create(:organisation_la, organisation:, ons_code: "E07000178")
+        FactoryBot.create(:organisation_la, organisation:, ons_code: "E09000033")
+      end
+
+      it "validates that the local authority is one the owning organisation operates in" do
+        record.la = "E06000014"
+        local_auth_validator.validate_la(record)
+        expect(record.errors["la"])
+          .to include(match I18n.t(
+            "validations.property.la.la_invalid_for_org",
+            org_name: organisation.name,
+            la_name: "York",
+          ))
+      end
+
+      it "expects that the local authority can be one that the owning organisation operates in" do
+        record.la = "E07000178"
+        local_auth_validator.validate_la(record)
+        expect(record.errors["la"]).to be_empty
+      end
+    end
+
+    context "when the organisation has not listed specific local authorities it operates in" do
+      it "does not validate the local authority for the organisation" do
+        record.la = "E06000014"
+        local_auth_validator.validate_la(record)
+        expect(record.errors["la"]).to be_empty
+      end
+    end
+  end
 end

spec/models/validations/property_validations_spec.rb

@@ -147,49 +147,6 @@ RSpec.describe Validations::PropertyValidations do
     end
   end
 
-  describe "#validate_la" do
-    context "when the rent type is London affordable" do
-      let(:expected_error) { I18n.t("validations.property.la.london_rent") }
-
-      it "validates that the local authority is in London" do
-        record.la = "E07000105"
-        record.rent_type = 2
-        property_validator.validate_la(record)
-        expect(record.errors["la"]).to include(match(expected_error))
-        expect(record.errors["postcode_full"]).to be_empty
-      end
-
-      it "expects that the local authority is in London" do
-        record.la = "E09000033"
-        record.rent_type = 2
-        property_validator.validate_la(record)
-        expect(record.errors["la"]).to be_empty
-      end
-
-      context "when the la has been derived from a known postcode" do
-        let(:expected_error) { I18n.t("validations.property.la.london_rent_postcode") }
-
-        it "also adds an error to the postcode field" do
-          record.la = "E07000105"
-          record.rent_type = 2
-          record.postcode_known = 1
-          record.postcode_full = "BN18 7TR"
-          property_validator.validate_la(record)
-          expect(record.errors["postcode_full"]).to include(match(expected_error))
-        end
-      end
-    end
-
-    context "when previous la is known" do
-      it "la has to be provided" do
-        record.la_known = 1
-        property_validator.validate_la(record)
-        expect(record.errors["la"])
-          .to include(match I18n.t("validations.property.la.la_known"))
-      end
-    end
-  end
-
   describe "#validate_unitletas" do
     context "when the property has not been let before" do
       it "validates that no previous let type is provided" do

spec/services/imports/organisation_la_import_service_spec.rb

@@ -0,0 +1,41 @@
+require "rails_helper"
+
+RSpec.describe Imports::OrganisationLaImportService do
+  let(:fixture_directory) { "spec/fixtures/softwire_imports/organisation_las" }
+  let(:old_org_id) { "44026acc7ed5c29516b26f2a5deb639e5e37966d" }
+  let(:old_id) { "00013f30e159d7f72a3abe9ea93fb5b685d311e4" }
+  let(:import_file) { File.open("#{fixture_directory}/#{old_id}.xml") }
+  let(:storage_service) { instance_double(StorageService) }
+
+  context "when importing data protection confirmations" do
+    subject(:import_service) { described_class.new(storage_service) }
+
+    before do
+      allow(storage_service)
+        .to receive(:list_files)
+        .and_return(["organisation_la_directory/#{old_id}.xml"])
+      allow(storage_service)
+        .to receive(:get_file_io)
+        .with("organisation_la_directory/#{old_id}.xml")
+        .and_return(import_file)
+    end
+
+    context "when the organisation in the import file doesn't exist in the system" do
+      it "does not create an organisation la record" do
+        expect { import_service.create_organisation_las("organisation_la_directory") }
+          .to raise_error(ActiveRecord::RecordInvalid, /Organisation must exist/)
+      end
+    end
+
+    context "when the organisation does exist" do
+      before do
+        FactoryBot.create(:organisation, old_org_id:)
+      end
+
+      it "successfully create an organisation la record with the expected data" do
+        import_service.create_organisation_las("organisation_la_directory")
+        expect(Organisation.find_by(old_org_id:).organisation_las.pluck("ons_code")).to eq(%w[E07000041])
+      end
+    end
+  end
+end