User imports (#276)

* Data import refactor
* Complete user import and corrects enums

app/models/case_log.rb

@@ -259,7 +259,7 @@ private
     end
     self.hhmemb = other_hhmemb + 1 if other_hhmemb.present?
     self.renttype = RENT_TYPE_MAPPING[rent_type]
-    self.lettype = "#{renttype} #{needstype} #{owning_organisation['Org type']}" if renttype.present? && needstype.present? && owning_organisation["Org type"].present?
+    self.lettype = "#{renttype} #{needstype} #{owning_organisation[:provider_type]}" if renttype.present? && needstype.present? && owning_organisation[:provider_type].present?
     self.totchild = get_totchild
     self.totelder = get_totelder
     self.totadult = get_totadult

app/models/constants/organisation.rb

@@ -1,6 +1,6 @@
 module Constants::Organisation
-  ORG_TYPE = {
-    "LA" => 1,
-    "PRP" => 2,
+  PROVIDER_TYPE = {
+    LA: 1,
+    PRP: 2,
   }.freeze
 end

app/models/constants/user.rb

@@ -1,7 +1,7 @@
 module Constants::User
   ROLES = {
-    "data_accessor" => 0,
-    "data_provider" => 1,
-    "data_coordinator" => 2,
+    data_accessor: 0,
+    data_provider: 1,
+    data_coordinator: 2,
   }.freeze
 end

app/models/organisation.rb

@@ -4,7 +4,7 @@ class Organisation < ApplicationRecord
   has_many :managed_case_logs, class_name: "CaseLog", foreign_key: "managing_organisation_id"
 
   include Constants::Organisation
-  enum "Org type": ORG_TYPE, _suffix: true
+  enum provider_type: PROVIDER_TYPE
 
   def case_logs
     CaseLog.for_organisation(self)

app/services/import_service.rb

@@ -1,56 +0,0 @@
-class ImportService
-  def initialize(storage_service, logger = Rails.logger)
-    @storage_service = storage_service
-    @logger = logger
-  end
-
-  def update_organisations(folder)
-    filenames = @storage_service.list_files(folder)
-    filenames.each do |filename|
-      file_io = @storage_service.get_file_io(filename)
-      create_organisation(file_io)
-    end
-  end
-
-private
-
-  def create_organisation(file_io)
-    doc = Nokogiri::XML(file_io)
-    name = field_value(doc, "name")
-    old_visible_id = field_value(doc, "visible-id")
-    begin
-      Organisation.create!(
-        name: name,
-        providertype: field_value(doc, "institution-type"),
-        phone: field_value(doc, "telephone-number"),
-        holds_own_stock: to_boolean(field_value(doc, "holds-stock")),
-        active: to_boolean(field_value(doc, "active")),
-        old_association_type: field_value(doc, "old-association-type"),
-        software_supplier_id: field_value(doc, "software-supplier-id"),
-        housing_management_system: field_value(doc, "housing-management-system"),
-        choice_based_lettings: to_boolean(field_value(doc, "choice-based-lettings")),
-        common_housing_register: to_boolean(field_value(doc, "common-housing-register")),
-        choice_allocation_policy: to_boolean(field_value(doc, "choice-allocation-policy")),
-        cbl_proportion_percentage: field_value(doc, "cbl-proportion-percentage"),
-        enter_affordable_logs: to_boolean(field_value(doc, "enter-affordable-logs")),
-        owns_affordable_logs: to_boolean(field_value(doc, "owns-affordable-rent")),
-        housing_registration_no: field_value(doc, "housing-registration-no"),
-        general_needs_units: field_value(doc, "general-needs-units"),
-        supported_housing_units: field_value(doc, "supported-housing-units"),
-        unspecified_units: field_value(doc, "unspecified-units"),
-        old_org_id: field_value(doc, "id"),
-        old_visible_id: old_visible_id,
-      )
-    rescue ActiveRecord::RecordNotUnique
-      @logger.warn("Organisation #{name} is already present with old visible ID #{old_visible_id}, skipping.")
-    end
-  end
-
-  def field_value(doc, field)
-    doc.at_xpath("//institution:#{field}")&.text
-  end
-
-  def to_boolean(input_string)
-    input_string == "true"
-  end
-end

app/services/imports/import_service.rb

@@ -0,0 +1,27 @@
+module Imports
+  class ImportService
+  private
+
+    def initialize(storage_service, logger = Rails.logger)
+      @storage_service = storage_service
+      @logger = logger
+    end
+
+    def import_from(folder, create_method)
+      filenames = @storage_service.list_files(folder)
+      filenames.each do |filename|
+        file_io = @storage_service.get_file_io(filename)
+        xml_document = Nokogiri::XML(file_io)
+        send(create_method, xml_document)
+      end
+    end
+
+    def field_value(xml_document, namespace, field)
+      xml_document.at_xpath("//#{namespace}:#{field}")&.text
+    end
+
+    def to_boolean(input_string)
+      input_string == "true"
+    end
+  end
+end

app/services/imports/organisation_import_service.rb

@@ -0,0 +1,46 @@
+module Imports
+  class OrganisationImportService < ImportService
+    def create_organisations(folder)
+      import_from(folder, :create_organisation)
+    end
+
+  private
+
+    PROVIDER_TYPE = {
+      "HOUSING-ASSOCIATION" => Organisation.provider_types[:PRP],
+    }.freeze
+
+    def create_organisation(xml_document)
+      Organisation.create!(
+        name: organisation_field_value(xml_document, "name"),
+        provider_type: PROVIDER_TYPE[organisation_field_value(xml_document, "institution-type")],
+        phone: organisation_field_value(xml_document, "telephone-number"),
+        holds_own_stock: to_boolean(organisation_field_value(xml_document, "holds-stock")),
+        active: to_boolean(organisation_field_value(xml_document, "active")),
+        old_association_type: organisation_field_value(xml_document, "old-association-type"),
+        software_supplier_id: organisation_field_value(xml_document, "software-supplier-id"),
+        housing_management_system: organisation_field_value(xml_document, "housing-management-system"),
+        choice_based_lettings: to_boolean(organisation_field_value(xml_document, "choice-based-lettings")),
+        common_housing_register: to_boolean(organisation_field_value(xml_document, "common-housing-register")),
+        choice_allocation_policy: to_boolean(organisation_field_value(xml_document, "choice-allocation-policy")),
+        cbl_proportion_percentage: organisation_field_value(xml_document, "cbl-proportion-percentage"),
+        enter_affordable_logs: to_boolean(organisation_field_value(xml_document, "enter-affordable-logs")),
+        owns_affordable_logs: to_boolean(organisation_field_value(xml_document, "owns-affordable-rent")),
+        housing_registration_no: organisation_field_value(xml_document, "housing-registration-no"),
+        general_needs_units: organisation_field_value(xml_document, "general-needs-units"),
+        supported_housing_units: organisation_field_value(xml_document, "supported-housing-units"),
+        unspecified_units: organisation_field_value(xml_document, "unspecified-units"),
+        old_org_id: organisation_field_value(xml_document, "id"),
+        old_visible_id: organisation_field_value(xml_document, "visible-id"),
+      )
+    rescue ActiveRecord::RecordNotUnique
+      name = organisation_field_value(xml_document, "name")
+      old_visible_id = organisation_field_value(xml_document, "visible-id")
+      @logger.warn("Organisation #{name} is already present with old visible ID #{old_visible_id}, skipping.")
+    end
+
+    def organisation_field_value(xml_document, field)
+      field_value(xml_document, "institution", field)
+    end
+  end
+end

app/services/imports/user_import_service.rb

@@ -0,0 +1,32 @@
+module Imports
+  class UserImportService < ImportService
+    def create_users(folder)
+      import_from(folder, :create_user)
+    end
+
+  private
+
+    PROVIDER_TYPE = {
+      "Data Provider" => User.roles[:data_provider],
+    }.freeze
+
+    def create_user(xml_document)
+      organisation = Organisation.find_by(old_org_id: user_field_value(xml_document, "institution"))
+      User.create!(
+        email: user_field_value(xml_document, "user-name"),
+        name: user_field_value(xml_document, "full-name"),
+        password: Devise.friendly_token,
+        phone: user_field_value(xml_document, "telephone-no"),
+        old_user_id: user_field_value(xml_document, "id"),
+        organisation: organisation,
+        role: PROVIDER_TYPE[user_field_value(xml_document, "user-type")],
+      )
+    rescue ActiveRecord::RecordNotUnique
+      @logger.warn("User #{name} with old user id #{old_user_id} is already present, skipping.")
+    end
+
+    def user_field_value(xml_document, field)
+      field_value(xml_document, "user", field)
+    end
+  end
+end

app/services/storage_service.rb

@@ -3,7 +3,7 @@ class StorageService
 
   def initialize(paas_config_service, paas_instance_name)
     @paas_config_service = paas_config_service
-    @paas_instance_name = paas_instance_name.to_sym
+    @paas_instance_name = (paas_instance_name || "").to_sym
     @configuration = create_configuration
     @client = create_client
   end

app/views/users/new.html.erb

@@ -27,7 +27,7 @@
         value: @resource.email
       %>
 
-      <%= roles = User::ROLES.map { |key, value| OpenStruct.new(id:key, name: key.humanize) }
+      <%= roles = User::ROLES.map { |key, _| OpenStruct.new(id: key, name: key.to_s.humanize) }
         f.govuk_collection_radio_buttons :role, roles, :id, :name, legend: { text: "Role", size: "m" }
       %>
 

db/migrate/20220204134000_additional_user_fields.rb

@@ -0,0 +1,13 @@
+class AdditionalUserFields < ActiveRecord::Migration[7.0]
+  def up
+    change_table :users, bulk: true do |t|
+      t.column :old_user_id, :string
+    end
+  end
+
+  def down
+    change_table :users, bulk: true do |t|
+      t.remove :old_user_id
+    end
+  end
+end

db/migrate/202202071123100_additional_user_fields2.rb

@@ -0,0 +1,13 @@
+class AdditionalUserFields2 < ActiveRecord::Migration[7.0]
+  def up
+    change_table :users, bulk: true do |t|
+      t.column :phone, :string
+    end
+  end
+
+  def down
+    change_table :users, bulk: true do |t|
+      t.remove :phone
+    end
+  end
+end

db/migrate/20220207120200_rename_provider_type.rb

@@ -0,0 +1,9 @@
+class RenameProviderType < ActiveRecord::Migration[7.0]
+  def up
+    rename_column :organisations, :providertype, :provider_type
+  end
+
+  def down
+    rename_column :organisations, :provider_type, :providertype
+  end
+end

db/schema.rb

@@ -10,8 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 2022_02_03_145845) do
-
+ActiveRecord::Schema.define(version: 2022_02_07_1123100) do
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
 
@@ -201,7 +200,7 @@ ActiveRecord::Schema.define(version: 2022_02_03_145845) do
   create_table "organisations", force: :cascade do |t|
     t.string "name"
     t.string "phone"
-    t.integer "providertype"
+    t.integer "provider_type"
     t.string "address_line1"
     t.string "address_line2"
     t.string "postcode"
@@ -246,6 +245,8 @@ ActiveRecord::Schema.define(version: 2022_02_03_145845) do
     t.string "current_sign_in_ip"
     t.string "last_sign_in_ip"
     t.integer "role"
+    t.string "old_user_id"
+    t.string "phone"
     t.index ["email"], name: "index_users_on_email", unique: true
     t.index ["organisation_id"], name: "index_users_on_organisation_id"
     t.index ["reset_password_token"], name: "index_users_on_reset_password_token", unique: true

lib/tasks/data_import.rake

@@ -0,0 +1,21 @@
+require "nokogiri"
+
+namespace :core do
+  desc "Import data XMLs from Softwire system"
+  task :data_import, %i[type path] => :environment do |_task, args|
+    type = args[:type]
+    path = args[:path]
+    raise "Usage: rake core:data_import['data_type', 'path/to/xml_files']" if path.blank? || type.blank?
+
+    storage_service = StorageService.new(PaasConfigurationService.new, ENV["IMPORT_PAAS_INSTANCE"])
+
+    case type
+    when "organisation"
+      Imports::OrganisationImportService.new(storage_service).create_organisations(path)
+    when "user"
+      Imports::UserImportService.new(storage_service).create_users(path)
+    else
+      raise "Type #{type} is not supported by data_import"
+    end
+  end
+end

lib/tasks/data_import/organisations.rake

@@ -1,13 +0,0 @@
-require "nokogiri"
-
-namespace :data_import do
-  desc "Import Organisation XMLs from Softwire system"
-
-  # rake data_import:organisations['path/to/xml_files']
-  task :organisations, %i[path] => :environment do |_task, args|
-    directory = args.path
-    storage_service = StorageService.new(PaasConfigurationService.new, ENV["IMPORT_PAAS_INSTANCE"])
-    import_service = ImportService.new(storage_service)
-    import_service.update_organisations(directory)
-  end
-end

spec/controllers/admin/organisations_controller_spec.rb

@@ -37,7 +37,7 @@ describe Admin::OrganisationsController, type: :controller do
 
     it "creates a new admin users" do
       expect(page).to have_field("organisation_name")
-      expect(page).to have_field("organisation_providertype")
+      expect(page).to have_field("organisation_provider_type")
       expect(page).to have_field("organisation_phone")
     end
   end

spec/fixtures/softwire_imports/users/fc7625a02b24ae16162aa63ae7cb33feeec0c373.xml

@@ -0,0 +1,13 @@
+<user:user xmlns:user="dclg:user">
+  <user:id>fc7625a02b24ae16162aa63ae7cb33feeec0c373</user:id>
+  <user:password>xxx</user:password>
+  <user:full-name>John Doe</user:full-name>
+  <user:user-name>john.doe@gov.uk</user:user-name>
+  <user:institution>7c5bd5fb549c09a2c55d7cb90d7ba84927e64618</user:institution>
+  <user:email>john.doe@gov.uk</user:email>
+  <user:user-type>Data Provider</user:user-type>
+  <user:active>true</user:active>
+  <user:deleted>false</user:deleted>
+  <user:contact-priority-id>None</user:contact-priority-id>
+  <user:telephone-no>02012345678</user:telephone-no>
+</user:user>

spec/lib/tasks/data_import/organisations_spec.rb

@@ -1,32 +0,0 @@
-require "rails_helper"
-require "rake"
-
-describe "rake data_import:organisations", type: :task do
-  subject(:task) { Rake::Task["data_import:organisations"] }
-
-  let(:fixture_path) { "spec/fixtures/softwire_imports/organisations" }
-  let(:instance_name) { "my_instance" }
-  let(:storage_service) { instance_double(StorageService) }
-  let(:paas_config_service) { instance_double(PaasConfigurationService) }
-  let(:import_service) { instance_double(ImportService) }
-
-  before do
-    allow(StorageService).to receive(:new).and_return(storage_service)
-    allow(PaasConfigurationService).to receive(:new).and_return(paas_config_service)
-    allow(ImportService).to receive(:new).and_return(import_service)
-    allow(ENV).to receive(:[])
-    allow(ENV).to receive(:[]).with("IMPORT_PAAS_INSTANCE").and_return(instance_name)
-
-    Rake.application.rake_require("tasks/data_import/organisations")
-    Rake::Task.define_task(:environment)
-    task.reenable
-  end
-
-  it "creates an organisation from the given XML file" do
-    expect(StorageService).to receive(:new).with(paas_config_service, instance_name)
-    expect(ImportService).to receive(:new).with(storage_service)
-    expect(import_service).to receive(:update_organisations).with(fixture_path)
-
-    task.invoke(fixture_path)
-  end
-end

spec/lib/tasks/data_import_spec.rb

@@ -0,0 +1,48 @@
+require "rails_helper"
+require "rake"
+
+describe "rake core:data_import", type: :task do
+  subject(:task) { Rake::Task["core:data_import"] }
+
+  let(:fixture_path) { "spec/fixtures/softwire_imports/organisations" }
+  let(:instance_name) { "my_instance" }
+  let(:organisation_type) { "organisation" }
+
+  let(:storage_service) { instance_double(StorageService) }
+  let(:paas_config_service) { instance_double(PaasConfigurationService) }
+  let(:import_service) { instance_double(Imports::OrganisationImportService) }
+
+  before do
+    Rake.application.rake_require("tasks/data_import")
+    Rake::Task.define_task(:environment)
+    task.reenable
+
+    allow(StorageService).to receive(:new).and_return(storage_service)
+    allow(PaasConfigurationService).to receive(:new).and_return(paas_config_service)
+    allow(Imports::OrganisationImportService).to receive(:new).and_return(import_service)
+    allow(ENV).to receive(:[])
+    allow(ENV).to receive(:[]).with("IMPORT_PAAS_INSTANCE").and_return(instance_name)
+  end
+
+  context "when importing organisation data" do
+    it "creates an organisation from the given XML file" do
+      expect(StorageService).to receive(:new).with(paas_config_service, instance_name)
+      expect(Imports::OrganisationImportService).to receive(:new).with(storage_service)
+      expect(import_service).to receive(:create_organisations).with(fixture_path)
+
+      task.invoke(organisation_type, fixture_path)
+    end
+  end
+
+  it "raises an exception if no parameters are provided" do
+    expect { task.invoke }.to raise_error(/Usage/)
+  end
+
+  it "raises an exception if a single parameter is provided" do
+    expect { task.invoke("one_parameter") }.to raise_error(/Usage/)
+  end
+
+  it "raises an exception if the type is not supported" do
+    expect { task.invoke("unknown_type", "my_path") }.to raise_error(/Type unknown_type is not supported/)
+  end
+end

spec/models/case_log_spec.rb

@@ -962,7 +962,7 @@ RSpec.describe CaseLog do
 
   describe "derived variables" do
     require "date"
-    let(:organisation) { FactoryBot.create(:organisation, "Org type": "PRP") }
+    let(:organisation) { FactoryBot.create(:organisation, provider_type: "PRP") }
     let!(:case_log) do
       described_class.create({
         managing_organisation: organisation,

spec/models/organisation_spec.rb

@@ -11,7 +11,7 @@ RSpec.describe Organisation, type: :model do
     let(:organisation) { user.organisation }
 
     it "has expected fields" do
-      expect(organisation.attribute_names).to include("name", "phone", "Org type")
+      expect(organisation.attribute_names).to include("name", "phone", "provider_type")
     end
 
     it "has users" do

spec/services/imports/organisation_import_service_spec.rb

@@ -1,17 +1,17 @@
 require "rails_helper"
 
-RSpec.describe ImportService do
+RSpec.describe Imports::OrganisationImportService do
   let(:storage_service) { instance_double(StorageService) }
   let(:logger) { instance_double(Rails::Rack::Logger) }
   let(:folder_name) { "organisations" }
   let(:filenames) { %w[my_folder/my_file1.xml my_folder/my_file2.xml] }
   let(:fixture_directory) { "spec/fixtures/softwire_imports/organisations" }
 
-  def create_organisation_file(fixture_directory, visible_id, name = "my_organisation")
+  def create_organisation_file(fixture_directory, visible_id, name = nil)
     file = File.open("#{fixture_directory}/7c5bd5fb549c09a2c55d7cb90d7ba84927e64618.xml")
     doc =  Nokogiri::XML(file)
-    doc.at_xpath("//institution:visible-id").content = visible_id
-    doc.at_xpath("//institution:name").content = name
+    doc.at_xpath("//institution:visible-id").content = visible_id if visible_id
+    doc.at_xpath("//institution:name").content = name if name
     StringIO.new(doc.to_xml)
   end
 
@@ -29,12 +29,39 @@ RSpec.describe ImportService do
                                    .and_return(create_organisation_file(fixture_directory, 2))
     end
 
-    it "successfully create a new organisation if it does not exist" do
+    it "successfully create an organisation with the expected data" do
+      import_service.create_organisations(folder_name)
+
+      organisation = Organisation.find_by(old_visible_id: 1)
+      expect(organisation.name).to eq("HA Ltd")
+      expect(organisation.provider_type).to eq("PRP")
+      expect(organisation.phone).to eq("xxxxxxxx")
+      expect(organisation.holds_own_stock).to be_truthy
+      expect(organisation.active).to be_truthy
+      # expect(organisation.old_association_type).to eq() string VS integer
+      # expect(organisation.software_supplier_id).to eq() boolean VS string
+      expect(organisation.housing_management_system).to eq("") # Need examples
+      expect(organisation.choice_based_lettings).to be_falsey
+      expect(organisation.common_housing_register).to be_falsey
+      expect(organisation.choice_allocation_policy).to be_falsey
+      expect(organisation.cbl_proportion_percentage).to be_nil # Need example
+      expect(organisation.enter_affordable_logs).to be_truthy
+      expect(organisation.owns_affordable_logs).to be_truthy # owns_affordable_rent
+      expect(organisation.housing_registration_no).to eq("LH9999")
+      expect(organisation.general_needs_units).to eq(1104)
+      expect(organisation.supported_housing_units).to eq(217)
+      expect(organisation.unspecified_units).to eq(0)
+      expect(organisation.unspecified_units).to eq(0)
+      expect(organisation.old_org_id).to eq("7c5bd5fb549c09z2c55d9cb90d7ba84927e64618")
+      expect(organisation.old_visible_id).to eq(1)
+    end
+
+    it "successfully create multiple organisations" do
       expect(storage_service).to receive(:list_files).with(folder_name)
       expect(storage_service).to receive(:get_file_io).with(filenames[0]).ordered
       expect(storage_service).to receive(:get_file_io).with(filenames[1]).ordered
 
-      expect { import_service.update_organisations(folder_name) }.to change(Organisation, :count).by(2)
+      expect { import_service.create_organisations(folder_name) }.to change(Organisation, :count).by(2)
       expect(Organisation).to exist(old_visible_id: 1)
       expect(Organisation).to exist(old_visible_id: 2)
     end
@@ -56,10 +83,10 @@ RSpec.describe ImportService do
       expect(storage_service).to receive(:get_file_io).with(filenames[0]).twice
       expect(logger).to receive(:warn).once
 
-      expect { import_service.update_organisations(folder_name) }.to change(Organisation, :count).by(1)
-      expect { import_service.update_organisations(folder_name) }.to change(Organisation, :count).by(0)
+      expect { import_service.create_organisations(folder_name) }.to change(Organisation, :count).by(1)
+      expect { import_service.create_organisations(folder_name) }.to change(Organisation, :count).by(0)
 
-      expect(Organisation).to exist(old_visible_id: 1, name: "my_organisation")
+      expect(Organisation).to exist(old_visible_id: 1, name: "HA Ltd")
     end
   end
 end

spec/services/imports/user_import_service_spec.rb

@@ -0,0 +1,37 @@
+require "rails_helper"
+
+RSpec.describe Imports::UserImportService do
+  let(:fixture_directory) { "spec/fixtures/softwire_imports/users" }
+  let(:user_file) { File.open("#{fixture_directory}/fc7625a02b24ae16162aa63ae7cb33feeec0c373.xml") }
+  let(:storage_service) { instance_double(StorageService) }
+
+  context "when importing users" do
+    subject(:import_service) { described_class.new(storage_service) }
+
+    before do
+      allow(storage_service).to receive(:list_files)
+                                  .and_return(["user_directory/fc7625a02b24ae16162aa63ae7cb33feeec0c373.xml"])
+      allow(storage_service).to receive(:get_file_io)
+                                  .with("user_directory/fc7625a02b24ae16162aa63ae7cb33feeec0c373.xml")
+                                  .and_return(user_file)
+    end
+
+    it "successfully create a user with the expected data" do
+      FactoryBot.create(:organisation, old_org_id: "7c5bd5fb549c09a2c55d7cb90d7ba84927e64618")
+      import_service.create_users("user_directory")
+
+      user = User.find_by(old_user_id: "fc7625a02b24ae16162aa63ae7cb33feeec0c373")
+      expect(user.name).to eq("John Doe")
+      expect(user.email).to eq("john.doe@gov.uk")
+      expect(user.encrypted_password).not_to be_nil
+      expect(user.phone).to eq("02012345678")
+      expect(user).to be_data_provider
+      expect(user.organisation.old_org_id).to eq("7c5bd5fb549c09a2c55d7cb90d7ba84927e64618")
+    end
+
+    it "refuses to create a user belonging to a non existing organisation" do
+      expect { import_service.create_users("user_directory") }
+        .to raise_error(ActiveRecord::RecordInvalid, /Organisation must exist/)
+    end
+  end
+end