From 884d5e3051399bf5dd9ef6b132badd4fdef458e2 Mon Sep 17 00:00:00 2001 From: baarkerlounger <5101747+baarkerlounger@users.noreply.github.com> Date: Thu, 25 Aug 2022 09:50:03 +0100 Subject: [PATCH 1/3] Use scheme name everywhere (#845) --- app/models/scheme.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/scheme.rb b/app/models/scheme.rb index 45f09d4a5..b52f7539c 100644 --- a/app/models/scheme.rb +++ b/app/models/scheme.rb @@ -112,7 +112,7 @@ class Scheme < ApplicationRecord def check_details_attributes [ - { name: "Service code", value: id_to_display, id: "id" }, + { name: "Scheme code", value: id_to_display, id: "id" }, { name: "Name", value: service_name, id: "service_name" }, { name: "Confidential information", value: sensitive, id: "sensitive" }, { name: "Type of scheme", value: scheme_type, id: "scheme_type" }, From b55d221cbc1c9a1dc421c8a125487a5e2cd9c537 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Meny?= Date: Thu, 25 Aug 2022 11:03:20 +0100 Subject: [PATCH 2/3] CLDC-1520: Support external S3 configuration (#835) * Move storage classes into a dedicated module * Move configuration classes into a dedicated module * Refactor configuration service * Implement configuration via env variables --- app/services/archive_storage_service.rb | 24 ---- .../configuration/configuration_service.rb | 54 ++++++++ .../env_configuration_service.rb | 37 +++++ .../paas_configuration_service.rb | 23 ++++ app/services/paas_configuration_service.rb | 62 --------- app/services/s3_storage_service.rb | 78 ----------- app/services/storage/archive_service.rb | 26 ++++ app/services/storage/s3_service.rb | 80 +++++++++++ app/services/storage/storage_service.rb | 19 +++ app/services/storage_service.rb | 17 --- config/initializers/rack_attack.rb | 5 +- lib/tasks/data_export.rake | 2 +- lib/tasks/data_import.rake | 2 +- lib/tasks/data_import_field.rake | 2 +- lib/tasks/full_import.rake | 4 +- spec/lib/tasks/data_export_spec.rb | 12 +- spec/lib/tasks/data_import_spec.rb | 22 +-- spec/lib/tasks/date_import_field_spec.rb | 14 +- spec/lib/tasks/full_import_spec.rb | 12 +- .../env_configuration_service_spec.rb | 128 ++++++++++++++++++ .../paas_configuration_service_spec.rb | 120 ++++++++-------- .../exports/case_log_export_service_spec.rb | 2 +- .../case_logs_field_import_service_spec.rb | 2 +- .../imports/case_logs_import_service_spec.rb | 2 +- ...ection_confirmation_import_service_spec.rb | 2 +- .../organisation_import_service_spec.rb | 2 +- ...isation_rent_period_import_service_spec.rb | 2 +- .../imports/scheme_import_service_spec.rb | 2 +- .../scheme_location_import_service_spec.rb | 2 +- .../imports/user_import_service_spec.rb | 2 +- .../archive_service_spec.rb} | 2 +- .../s3_service_spec.rb} | 28 ++-- 32 files changed, 497 insertions(+), 294 deletions(-) delete mode 100644 app/services/archive_storage_service.rb create mode 100644 app/services/configuration/configuration_service.rb create mode 100644 app/services/configuration/env_configuration_service.rb create mode 100644 app/services/configuration/paas_configuration_service.rb delete mode 100644 app/services/paas_configuration_service.rb delete mode 100644 app/services/s3_storage_service.rb create mode 100644 app/services/storage/archive_service.rb create mode 100644 app/services/storage/s3_service.rb create mode 100644 app/services/storage/storage_service.rb delete mode 100644 app/services/storage_service.rb create mode 100644 spec/services/configuration/env_configuration_service_spec.rb rename spec/services/{ => configuration}/paas_configuration_service_spec.rb (66%) rename spec/services/{archive_storage_service_spec.rb => storage/archive_service_spec.rb} (98%) rename spec/services/{s3_storage_service_spec.rb => storage/s3_service_spec.rb} (80%) diff --git a/app/services/archive_storage_service.rb b/app/services/archive_storage_service.rb deleted file mode 100644 index 92875d86a..000000000 --- a/app/services/archive_storage_service.rb +++ /dev/null @@ -1,24 +0,0 @@ -class ArchiveStorageService < StorageService - MAX_SIZE = 50 * (1024**2) # 50MiB - - def initialize(archive_io) - super() - @archive = Zip::File.open_buffer(archive_io) - end - - def list_files(folder) - @archive.glob(File.join(folder, "*.*")) - .map(&:name) - end - - def folder_present?(folder) - !list_files(folder).empty? - end - - def get_file_io(file_name) - entry = @archive.get_entry(file_name) - raise "File too large to be extracted" if entry.size > MAX_SIZE - - entry.get_input_stream - end -end diff --git a/app/services/configuration/configuration_service.rb b/app/services/configuration/configuration_service.rb new file mode 100644 index 000000000..07617a807 --- /dev/null +++ b/app/services/configuration/configuration_service.rb @@ -0,0 +1,54 @@ +module Configuration + class ConfigurationService + attr_reader :s3_buckets, :redis_uris + + def initialize(logger = Rails.logger) + @logger = logger + @config = read_config + @s3_buckets = read_s3_buckets + @redis_uris = read_redis_uris + end + + def s3_config_present? + config_present? && @config.key?(:"aws-s3-bucket") + end + + def redis_config_present? + config_present? && @config.key?(:redis) + end + + private + + def config_present? + raise NotImplementedError + end + + def read_config + raise NotImplementedError + end + + def read_s3_buckets + return {} unless s3_config_present? + + s3_buckets = {} + @config[:"aws-s3-bucket"].each do |bucket_config| + if bucket_config.key?(:instance_name) + s3_buckets[bucket_config[:instance_name].to_sym] = bucket_config + end + end + s3_buckets + end + + def read_redis_uris + return {} unless redis_config_present? + + redis_uris = {} + @config[:redis].each do |redis_config| + if redis_config.key?(:instance_name) + redis_uris[redis_config[:instance_name].to_sym] = redis_config.dig(:credentials, :uri) + end + end + redis_uris + end + end +end diff --git a/app/services/configuration/env_configuration_service.rb b/app/services/configuration/env_configuration_service.rb new file mode 100644 index 000000000..077f425b4 --- /dev/null +++ b/app/services/configuration/env_configuration_service.rb @@ -0,0 +1,37 @@ +module Configuration + class EnvConfigurationService < ConfigurationService + private + + def config_present? + !ENV["S3_CONFIG"].nil? || !ENV["REDIS_CONFIG"].nil? + end + + def read_config + unless config_present? + @logger.warn("Could not find S3_CONFIG or REDIS_CONFIG in the environment variables!") + return {} + end + + config = {} + assign_config(config, :"aws-s3-bucket", "S3_CONFIG") + assign_config(config, :redis, "REDIS_CONFIG") + config + end + + def assign_config(config, symbol, env_variable) + config_hash = parse_json_config(env_variable) + config[symbol] = config_hash unless config_hash.empty? + end + + def parse_json_config(env_variable_name) + if ENV[env_variable_name].present? + begin + return JSON.parse(ENV[env_variable_name], { symbolize_names: true }) + rescue StandardError + @logger.warn("Could not parse #{env_variable_name}!") + end + end + {} + end + end +end diff --git a/app/services/configuration/paas_configuration_service.rb b/app/services/configuration/paas_configuration_service.rb new file mode 100644 index 000000000..b9f2a3226 --- /dev/null +++ b/app/services/configuration/paas_configuration_service.rb @@ -0,0 +1,23 @@ +module Configuration + class PaasConfigurationService < ConfigurationService + private + + def config_present? + !ENV["VCAP_SERVICES"].nil? + end + + def read_config + unless config_present? + @logger.warn("Could not find VCAP_SERVICES in the environment variables!") + return {} + end + + begin + JSON.parse(ENV["VCAP_SERVICES"], { symbolize_names: true }) + rescue StandardError + @logger.warn("Could not parse VCAP_SERVICES!") + {} + end + end + end +end diff --git a/app/services/paas_configuration_service.rb b/app/services/paas_configuration_service.rb deleted file mode 100644 index 008b4d2ab..000000000 --- a/app/services/paas_configuration_service.rb +++ /dev/null @@ -1,62 +0,0 @@ -class PaasConfigurationService - attr_reader :s3_buckets, :redis_uris - - def initialize(logger = Rails.logger) - @logger = logger - @paas_config = read_pass_config - @s3_buckets = read_s3_buckets - @redis_uris = read_redis_uris - end - - def config_present? - !ENV["VCAP_SERVICES"].nil? - end - - def s3_config_present? - config_present? && @paas_config.key?(:"aws-s3-bucket") - end - - def redis_config_present? - config_present? && @paas_config.key?(:redis) - end - -private - - def read_pass_config - unless config_present? - @logger.warn("Could not find VCAP_SERVICES in the environment!") - return {} - end - - begin - JSON.parse(ENV["VCAP_SERVICES"], { symbolize_names: true }) - rescue StandardError - @logger.warn("Could not parse VCAP_SERVICES!") - {} - end - end - - def read_s3_buckets - return {} unless s3_config_present? - - s3_buckets = {} - @paas_config[:"aws-s3-bucket"].each do |bucket_config| - if bucket_config.key?(:instance_name) - s3_buckets[bucket_config[:instance_name].to_sym] = bucket_config - end - end - s3_buckets - end - - def read_redis_uris - return {} unless redis_config_present? - - redis_uris = {} - @paas_config[:redis].each do |redis_config| - if redis_config.key?(:instance_name) - redis_uris[redis_config[:instance_name].to_sym] = redis_config.dig(:credentials, :uri) - end - end - redis_uris - end -end diff --git a/app/services/s3_storage_service.rb b/app/services/s3_storage_service.rb deleted file mode 100644 index 15cbee3d0..000000000 --- a/app/services/s3_storage_service.rb +++ /dev/null @@ -1,78 +0,0 @@ -class S3StorageService < StorageService - attr_reader :configuration - - def initialize(paas_config_service, paas_instance_name) - super() - @paas_config_service = paas_config_service - @paas_instance_name = (paas_instance_name || "").to_sym - @configuration = create_configuration - @client = create_client - end - - def list_files(folder) - @client.list_objects_v2(bucket: @configuration.bucket_name, prefix: folder) - .flat_map { |response| response.contents.map(&:key) } - end - - def folder_present?(folder) - response = @client.list_objects_v2(bucket: @configuration.bucket_name, prefix: folder, max_keys: 1) - response.key_count == 1 - end - - def get_file_io(file_name) - @client.get_object(bucket: @configuration.bucket_name, key: file_name) - .body - end - - def write_file(file_name, data) - @client.put_object( - body: data, - bucket: @configuration.bucket_name, - key: file_name, - ) - end - -private - - def create_configuration - unless @paas_config_service.config_present? - raise "No PaaS configuration present" - end - unless @paas_config_service.s3_buckets.key?(@paas_instance_name) - raise "#{@paas_instance_name} instance name could not be found" - end - - bucket_config = @paas_config_service.s3_buckets[@paas_instance_name] - StorageConfiguration.new(bucket_config[:credentials]) - end - - def create_client - credentials = - Aws::Credentials.new( - @configuration.access_key_id, - @configuration.secret_access_key, - ) - Aws::S3::Client.new( - region: @configuration.region, - credentials:, - ) - end -end - -class StorageConfiguration - attr_reader :access_key_id, :secret_access_key, :bucket_name, :region - - def initialize(credentials) - @access_key_id = credentials[:aws_access_key_id] - @secret_access_key = credentials[:aws_secret_access_key] - @bucket_name = credentials[:bucket_name] - @region = credentials[:aws_region] - end - - def ==(other) - @access_key_id == other.access_key_id && - @secret_access_key == other.secret_access_key && - @bucket_name == other.bucket_name && - @region == other.region - end -end diff --git a/app/services/storage/archive_service.rb b/app/services/storage/archive_service.rb new file mode 100644 index 000000000..dcf3e8d75 --- /dev/null +++ b/app/services/storage/archive_service.rb @@ -0,0 +1,26 @@ +module Storage + class ArchiveService < StorageService + MAX_SIZE = 50 * (1024**2) # 50MiB + + def initialize(archive_io) + super() + @archive = Zip::File.open_buffer(archive_io) + end + + def list_files(folder) + @archive.glob(File.join(folder, "*.*")) + .map(&:name) + end + + def folder_present?(folder) + !list_files(folder).empty? + end + + def get_file_io(file_name) + entry = @archive.get_entry(file_name) + raise "File too large to be extracted" if entry.size > MAX_SIZE + + entry.get_input_stream + end + end +end diff --git a/app/services/storage/s3_service.rb b/app/services/storage/s3_service.rb new file mode 100644 index 000000000..aee76398d --- /dev/null +++ b/app/services/storage/s3_service.rb @@ -0,0 +1,80 @@ +module Storage + class S3Service < StorageService + attr_reader :configuration + + def initialize(config_service, paas_instance_name) + super() + @config_service = config_service + @instance_name = (paas_instance_name || "").to_sym + @configuration = create_configuration + @client = create_client + end + + def list_files(folder) + @client.list_objects_v2(bucket: @configuration.bucket_name, prefix: folder) + .flat_map { |response| response.contents.map(&:key) } + end + + def folder_present?(folder) + response = @client.list_objects_v2(bucket: @configuration.bucket_name, prefix: folder, max_keys: 1) + response.key_count == 1 + end + + def get_file_io(file_name) + @client.get_object(bucket: @configuration.bucket_name, key: file_name) + .body + end + + def write_file(file_name, data) + @client.put_object( + body: data, + bucket: @configuration.bucket_name, + key: file_name, + ) + end + + private + + def create_configuration + unless @config_service.s3_config_present? + raise "No S3 bucket is present in the PaaS configuration" + end + unless @config_service.s3_buckets.key?(@instance_name) + raise "#{@instance_name} instance name could not be found" + end + + bucket_config = @config_service.s3_buckets[@instance_name] + StorageConfiguration.new(bucket_config[:credentials]) + end + + def create_client + credentials = + Aws::Credentials.new( + @configuration.access_key_id, + @configuration.secret_access_key, + ) + Aws::S3::Client.new( + region: @configuration.region, + credentials:, + ) + end + end + + class StorageConfiguration + attr_reader :access_key_id, :secret_access_key, :bucket_name, :region + + def initialize(credentials) + @access_key_id = credentials[:aws_access_key_id] + @secret_access_key = credentials[:aws_secret_access_key] + @bucket_name = credentials[:bucket_name] + @region = credentials[:aws_region] + end + + def ==(other) + @access_key_id == other.access_key_id && + @secret_access_key == other.secret_access_key && + @bucket_name == other.bucket_name && + @region == other.region + end + end +end diff --git a/app/services/storage/storage_service.rb b/app/services/storage/storage_service.rb new file mode 100644 index 000000000..afb3d4a58 --- /dev/null +++ b/app/services/storage/storage_service.rb @@ -0,0 +1,19 @@ +module Storage + class StorageService + def list_files(_folder) + raise NotImplementedError + end + + def folder_present?(_folder) + raise NotImplementedError + end + + def get_file_io(_file_name) + raise NotImplementedError + end + + def write_file(_file_name, _data) + raise NotImplementedError + end + end +end diff --git a/app/services/storage_service.rb b/app/services/storage_service.rb deleted file mode 100644 index 0135a92a1..000000000 --- a/app/services/storage_service.rb +++ /dev/null @@ -1,17 +0,0 @@ -class StorageService - def list_files(_folder) - raise NotImplementedError - end - - def folder_present?(_folder) - raise NotImplementedError - end - - def get_file_io(_file_name) - raise NotImplementedError - end - - def write_file(_file_name, _data) - raise NotImplementedError - end -end diff --git a/config/initializers/rack_attack.rb b/config/initializers/rack_attack.rb index e130adc50..3bfcff86b 100644 --- a/config/initializers/rack_attack.rb +++ b/config/initializers/rack_attack.rb @@ -1,10 +1,11 @@ -require "paas_configuration_service" +require "configuration/configuration_service" +require "configuration/paas_configuration_service" if Rails.env.development? || Rails.env.test? Rack::Attack.cache.store = ActiveSupport::Cache::MemoryStore.new Rack::Attack.enabled = false else - redis_url = PaasConfigurationService.new.redis_uris[:"dluhc-core-#{Rails.env}-redis-rate-limit"] + redis_url = Configuration::PaasConfigurationService.new.redis_uris[:"dluhc-core-#{Rails.env}-redis-rate-limit"] Rack::Attack.cache.store = ActiveSupport::Cache::RedisCacheStore.new(url: redis_url) end diff --git a/lib/tasks/data_export.rake b/lib/tasks/data_export.rake index cf669f488..fb7af1466 100644 --- a/lib/tasks/data_export.rake +++ b/lib/tasks/data_export.rake @@ -4,7 +4,7 @@ namespace :core do format = args[:format] full_update = args[:full_update].present? && args[:full_update] == "true" - storage_service = S3StorageService.new(PaasConfigurationService.new, ENV["EXPORT_PAAS_INSTANCE"]) + storage_service = Storage::S3Service.new(Configuration::PaasConfigurationService.new, ENV["EXPORT_PAAS_INSTANCE"]) export_service = Exports::CaseLogExportService.new(storage_service) if format.present? && format == "CSV" diff --git a/lib/tasks/data_import.rake b/lib/tasks/data_import.rake index 738fbceb5..b8baac256 100644 --- a/lib/tasks/data_import.rake +++ b/lib/tasks/data_import.rake @@ -5,7 +5,7 @@ namespace :core do path = args[:path] raise "Usage: rake core:data_import['data_type', 'path/to/xml_files']" if path.blank? || type.blank? - storage_service = S3StorageService.new(PaasConfigurationService.new, ENV["IMPORT_PAAS_INSTANCE"]) + storage_service = Storage::S3Service.new(Configuration::PaasConfigurationService.new, ENV["IMPORT_PAAS_INSTANCE"]) case type when "organisation" diff --git a/lib/tasks/data_import_field.rake b/lib/tasks/data_import_field.rake index 7de94cd09..523ed6146 100644 --- a/lib/tasks/data_import_field.rake +++ b/lib/tasks/data_import_field.rake @@ -5,7 +5,7 @@ namespace :core do path = args[:path] raise "Usage: rake core:data_import_field['field','path/to/xml_files']" if path.blank? || field.blank? - storage_service = S3StorageService.new(PaasConfigurationService.new, ENV["IMPORT_PAAS_INSTANCE"]) + storage_service = Storage::S3Service.new(Configuration::PaasConfigurationService.new, ENV["IMPORT_PAAS_INSTANCE"]) # We only allow a reduced list of known fields to be updatable case field diff --git a/lib/tasks/full_import.rake b/lib/tasks/full_import.rake index 1c082b460..bc982b98e 100644 --- a/lib/tasks/full_import.rake +++ b/lib/tasks/full_import.rake @@ -6,9 +6,9 @@ namespace :core do archive_path = args[:archive_path] raise "Usage: rake core:full_import['path/to/archive']" if archive_path.blank? - s3_service = S3StorageService.new(PaasConfigurationService.new, ENV["IMPORT_PAAS_INSTANCE"]) + s3_service = Storage::S3Service.new(Configuration::PaasConfigurationService.new, ENV["IMPORT_PAAS_INSTANCE"]) archive_io = s3_service.get_file_io(archive_path) - archive_service = ArchiveStorageService.new(archive_io) + archive_service = Storage::ArchiveService.new(archive_io) import_list = [ Import.new(Imports::OrganisationImportService, :create_organisations, "institution"), diff --git a/spec/lib/tasks/data_export_spec.rb b/spec/lib/tasks/data_export_spec.rb index ae0acdb0d..00d3414d4 100644 --- a/spec/lib/tasks/data_export_spec.rb +++ b/spec/lib/tasks/data_export_spec.rb @@ -5,8 +5,8 @@ describe "rake core:data_export", type: task do subject(:task) { Rake::Task["core:data_export"] } let(:paas_instance) { "paas_export_instance" } - let(:storage_service) { instance_double(S3StorageService) } - let(:paas_config_service) { instance_double(PaasConfigurationService) } + let(:storage_service) { instance_double(Storage::S3Service) } + let(:paas_config_service) { instance_double(Configuration::PaasConfigurationService) } let(:export_service) { instance_double(Exports::CaseLogExportService) } before do @@ -14,8 +14,8 @@ describe "rake core:data_export", type: task do Rake::Task.define_task(:environment) task.reenable - allow(S3StorageService).to receive(:new).and_return(storage_service) - allow(PaasConfigurationService).to receive(:new).and_return(paas_config_service) + allow(Storage::S3Service).to receive(:new).and_return(storage_service) + allow(Configuration::PaasConfigurationService).to receive(:new).and_return(paas_config_service) allow(Exports::CaseLogExportService).to receive(:new).and_return(export_service) allow(ENV).to receive(:[]) allow(ENV).to receive(:[]).with("EXPORT_PAAS_INSTANCE").and_return(paas_instance) @@ -23,7 +23,7 @@ describe "rake core:data_export", type: task do context "when exporting case logs with no parameters" do it "starts the XML export process" do - expect(S3StorageService).to receive(:new).with(paas_config_service, paas_instance) + expect(Storage::S3Service).to receive(:new).with(paas_config_service, paas_instance) expect(Exports::CaseLogExportService).to receive(:new).with(storage_service) expect(export_service).to receive(:export_xml_case_logs) @@ -33,7 +33,7 @@ describe "rake core:data_export", type: task do context "when exporting case logs with CSV format" do it "starts the CSV export process" do - expect(S3StorageService).to receive(:new).with(paas_config_service, paas_instance) + expect(Storage::S3Service).to receive(:new).with(paas_config_service, paas_instance) expect(Exports::CaseLogExportService).to receive(:new).with(storage_service) expect(export_service).to receive(:export_csv_case_logs) diff --git a/spec/lib/tasks/data_import_spec.rb b/spec/lib/tasks/data_import_spec.rb index 9a88ef22e..e861c2ad2 100644 --- a/spec/lib/tasks/data_import_spec.rb +++ b/spec/lib/tasks/data_import_spec.rb @@ -5,16 +5,16 @@ describe "rake core:data_import", type: :task do subject(:task) { Rake::Task["core:data_import"] } let(:instance_name) { "paas_import_instance" } - let(:storage_service) { instance_double(S3StorageService) } - let(:paas_config_service) { instance_double(PaasConfigurationService) } + let(:storage_service) { instance_double(Storage::S3Service) } + let(:paas_config_service) { instance_double(Configuration::PaasConfigurationService) } before do Rake.application.rake_require("tasks/data_import") Rake::Task.define_task(:environment) task.reenable - allow(S3StorageService).to receive(:new).and_return(storage_service) - allow(PaasConfigurationService).to receive(:new).and_return(paas_config_service) + allow(Storage::S3Service).to receive(:new).and_return(storage_service) + allow(Configuration::PaasConfigurationService).to receive(:new).and_return(paas_config_service) allow(ENV).to receive(:[]) allow(ENV).to receive(:[]).with("IMPORT_PAAS_INSTANCE").and_return(instance_name) end @@ -29,7 +29,7 @@ describe "rake core:data_import", type: :task do end it "creates an organisation from the given XML file" do - expect(S3StorageService).to receive(:new).with(paas_config_service, instance_name) + expect(Storage::S3Service).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) @@ -47,7 +47,7 @@ describe "rake core:data_import", type: :task do end it "creates a user from the given XML file" do - expect(S3StorageService).to receive(:new).with(paas_config_service, instance_name) + expect(Storage::S3Service).to receive(:new).with(paas_config_service, instance_name) expect(Imports::UserImportService).to receive(:new).with(storage_service) expect(import_service).to receive(:create_users).with(fixture_path) @@ -65,7 +65,7 @@ describe "rake core:data_import", type: :task do end it "creates an organisation from the given XML file" do - expect(S3StorageService).to receive(:new).with(paas_config_service, instance_name) + expect(Storage::S3Service).to receive(:new).with(paas_config_service, instance_name) expect(Imports::DataProtectionConfirmationImportService).to receive(:new).with(storage_service) expect(import_service).to receive(:create_data_protection_confirmations).with(fixture_path) @@ -83,7 +83,7 @@ describe "rake core:data_import", type: :task do end it "creates an organisation la from the given XML file" do - expect(S3StorageService).to receive(:new).with(paas_config_service, instance_name) + expect(Storage::S3Service).to receive(:new).with(paas_config_service, instance_name) expect(Imports::OrganisationRentPeriodImportService).to receive(:new).with(storage_service) expect(import_service).to receive(:create_organisation_rent_periods).with(fixture_path) @@ -101,7 +101,7 @@ describe "rake core:data_import", type: :task do end it "creates case logs from the given XML file" do - expect(S3StorageService).to receive(:new).with(paas_config_service, instance_name) + expect(Storage::S3Service).to receive(:new).with(paas_config_service, instance_name) expect(Imports::CaseLogsImportService).to receive(:new).with(storage_service) expect(import_service).to receive(:create_logs).with(fixture_path) @@ -119,7 +119,7 @@ describe "rake core:data_import", type: :task do end it "creates a scheme from the given XML file" do - expect(S3StorageService).to receive(:new).with(paas_config_service, instance_name) + expect(Storage::S3Service).to receive(:new).with(paas_config_service, instance_name) expect(Imports::SchemeImportService).to receive(:new).with(storage_service) expect(import_service).to receive(:create_schemes).with(fixture_path) @@ -137,7 +137,7 @@ describe "rake core:data_import", type: :task do end it "creates a scheme location from the given XML file" do - expect(S3StorageService).to receive(:new).with(paas_config_service, instance_name) + expect(Storage::S3Service).to receive(:new).with(paas_config_service, instance_name) expect(Imports::SchemeLocationImportService).to receive(:new).with(storage_service) expect(import_service).to receive(:create_scheme_locations).with(fixture_path) diff --git a/spec/lib/tasks/date_import_field_spec.rb b/spec/lib/tasks/date_import_field_spec.rb index 5dd96ff28..69c58697a 100644 --- a/spec/lib/tasks/date_import_field_spec.rb +++ b/spec/lib/tasks/date_import_field_spec.rb @@ -5,16 +5,16 @@ describe "rake core:data_import_field", type: :task do subject(:task) { Rake::Task["core:data_import_field"] } let(:instance_name) { "paas_import_instance" } - let(:storage_service) { instance_double(S3StorageService) } - let(:paas_config_service) { instance_double(PaasConfigurationService) } + let(:storage_service) { instance_double(Storage::S3Service) } + let(:paas_config_service) { instance_double(Configuration::PaasConfigurationService) } before do Rake.application.rake_require("tasks/data_import_field") Rake::Task.define_task(:environment) task.reenable - allow(S3StorageService).to receive(:new).and_return(storage_service) - allow(PaasConfigurationService).to receive(:new).and_return(paas_config_service) + allow(Storage::S3Service).to receive(:new).and_return(storage_service) + allow(Configuration::PaasConfigurationService).to receive(:new).and_return(paas_config_service) allow(ENV).to receive(:[]) allow(ENV).to receive(:[]).with("IMPORT_PAAS_INSTANCE").and_return(instance_name) end @@ -32,7 +32,7 @@ describe "rake core:data_import_field", type: :task do let(:field) { "tenant_code" } it "properly configures the storage service" do - expect(S3StorageService).to receive(:new).with(paas_config_service, instance_name) + expect(Storage::S3Service).to receive(:new).with(paas_config_service, instance_name) task.invoke(field, fixture_path) end @@ -46,7 +46,7 @@ describe "rake core:data_import_field", type: :task do let(:field) { "lettings_allocation" } it "properly configures the storage service" do - expect(S3StorageService).to receive(:new).with(paas_config_service, instance_name) + expect(Storage::S3Service).to receive(:new).with(paas_config_service, instance_name) task.invoke(field, fixture_path) end @@ -60,7 +60,7 @@ describe "rake core:data_import_field", type: :task do let(:field) { "major_repairs" } it "properly configures the storage service" do - expect(S3StorageService).to receive(:new).with(paas_config_service, instance_name) + expect(Storage::S3Service).to receive(:new).with(paas_config_service, instance_name) task.invoke(field, fixture_path) end diff --git a/spec/lib/tasks/full_import_spec.rb b/spec/lib/tasks/full_import_spec.rb index 99f95f555..79fac87f3 100644 --- a/spec/lib/tasks/full_import_spec.rb +++ b/spec/lib/tasks/full_import_spec.rb @@ -5,19 +5,19 @@ require "zip" describe "rake core:full_import", type: :task do subject(:task) { Rake::Task["core:full_import"] } - let(:s3_service) { instance_double(S3StorageService) } - let(:archive_service) { instance_double(ArchiveStorageService) } - let(:paas_config_service) { instance_double(PaasConfigurationService) } + let(:s3_service) { instance_double(Storage::S3Service) } + let(:archive_service) { instance_double(Storage::ArchiveService) } + let(:paas_config_service) { instance_double(Configuration::PaasConfigurationService) } before do Rake.application.rake_require("tasks/full_import") Rake::Task.define_task(:environment) task.reenable - allow(PaasConfigurationService).to receive(:new).and_return(paas_config_service) - allow(S3StorageService).to receive(:new).and_return(s3_service) + allow(Configuration::PaasConfigurationService).to receive(:new).and_return(paas_config_service) + allow(Storage::S3Service).to receive(:new).and_return(s3_service) allow(s3_service).to receive(:get_file_io) - allow(ArchiveStorageService).to receive(:new).and_return(archive_service) + allow(Storage::ArchiveService).to receive(:new).and_return(archive_service) end context "when starting a full import" do diff --git a/spec/services/configuration/env_configuration_service_spec.rb b/spec/services/configuration/env_configuration_service_spec.rb new file mode 100644 index 000000000..491ecec7d --- /dev/null +++ b/spec/services/configuration/env_configuration_service_spec.rb @@ -0,0 +1,128 @@ +require "rails_helper" + +RSpec.describe Configuration::EnvConfigurationService do + subject(:config_service) { described_class.new(logger) } + + let(:logger) { instance_double(ActiveSupport::LogSubscriber) } + + context "when environment configurations are unavailable" do + before { allow(logger).to receive(:warn) } + + it "returns the S3 configuration as not present" do + expect(config_service.s3_config_present?).to be(false) + end + + it "returns the redis configuration as not present" do + expect(config_service.redis_config_present?).to be(false) + end + + it "does not retrieve any S3 bucket configuration" do + expect(config_service.s3_buckets).to be_a(Hash) + expect(config_service.s3_buckets).to be_empty + end + + it "does not retrieve any redis configuration" do + expect(config_service.redis_uris).to be_a(Hash) + expect(config_service.redis_uris).to be_empty + end + end + + context "when environment configurations are present but invalid" do + let(:env_variable) { "random text" } + + before do + allow(ENV).to receive(:[]).with("S3_CONFIG").and_return(env_variable) + allow(ENV).to receive(:[]).with("REDIS_CONFIG").and_return(env_variable) + allow(logger).to receive(:warn) + end + + it "logs an error when checking if the S3 config is present" do + expect(logger).to receive(:warn).with("Could not parse S3_CONFIG!") + config_service.s3_config_present? + end + + it "logs an error when checking if the Redis config is present" do + expect(logger).to receive(:warn).with("Could not parse REDIS_CONFIG!") + config_service.redis_config_present? + end + end + + context "when environment configurations are present with S3 configured" do + let(:s3_config) do + <<~JSON + [ + { + "instance_name": "bucket_1", + "credentials": { + "aws_access_key_id": "123", + "aws_secret_access_key": "456", + "aws_region": "eu-west-1", + "bucket_name": "my-bucket" + } + }, + { + "instance_name": "bucket_2", + "credentials": { + "aws_access_key_id": "789", + "aws_secret_access_key": "012", + "aws_region": "eu-west-2", + "bucket_name": "my-bucket2" + } + } + ] + JSON + end + + before do + allow(ENV).to receive(:[]).with("REDIS_CONFIG") + allow(ENV).to receive(:[]).with("S3_CONFIG").and_return(s3_config) + end + + it "returns the S3 configuration as present" do + expect(config_service.s3_config_present?).to be(true) + end + + it "returns the redis configuration as not present" do + expect(config_service.redis_config_present?).to be(false) + end + + it "does retrieve the S3 bucket configurations" do + s3_buckets = config_service.s3_buckets + + expect(s3_buckets).not_to be_empty + expect(s3_buckets.count).to be(2) + expect(s3_buckets).to have_key(:bucket_1) + expect(s3_buckets).to have_key(:bucket_2) + end + end + + context "when environment configurations are present with redis configured" do + let(:redis_config) do + <<-JSON + [{"instance_name": "redis_1", "credentials": {"uri": "redis_uri" }}] + JSON + end + + before do + allow(ENV).to receive(:[]).with("S3_CONFIG") + allow(ENV).to receive(:[]).with("REDIS_CONFIG").and_return(redis_config) + end + + it "returns the redis configuration as present" do + expect(config_service.redis_config_present?).to be(true) + end + + it "returns the S3 configuration as not present" do + expect(config_service.s3_config_present?).to be(false) + end + + it "does retrieve the redis configurations" do + redis_uris = config_service.redis_uris + + expect(redis_uris).not_to be_empty + expect(redis_uris.count).to be(1) + expect(redis_uris).to have_key(:redis_1) + expect(redis_uris[:redis_1]).to eq("redis_uri") + end + end +end diff --git a/spec/services/paas_configuration_service_spec.rb b/spec/services/configuration/paas_configuration_service_spec.rb similarity index 66% rename from spec/services/paas_configuration_service_spec.rb rename to spec/services/configuration/paas_configuration_service_spec.rb index 3338ad5ec..5e9e88046 100644 --- a/spec/services/paas_configuration_service_spec.rb +++ b/spec/services/configuration/paas_configuration_service_spec.rb @@ -1,6 +1,6 @@ require "rails_helper" -RSpec.describe PaasConfigurationService do +RSpec.describe Configuration::PaasConfigurationService do subject(:config_service) { described_class.new(logger) } let(:logger) { instance_double(ActiveSupport::LogSubscriber) } @@ -8,14 +8,38 @@ RSpec.describe PaasConfigurationService do context "when the paas configuration is unavailable" do before { allow(logger).to receive(:warn) } - it "returns the configuration as not present" do - expect(config_service.config_present?).to be(false) + it "returns the S3 configuration as not present" do + expect(config_service.s3_config_present?).to be(false) + end + + it "returns the redis configuration as not present" do + expect(config_service.redis_config_present?).to be(false) + end + + it "does not retrieve any S3 bucket configuration" do + expect(config_service.s3_buckets).to be_a(Hash) + expect(config_service.s3_buckets).to be_empty + end + + it "does not retrieve any redis configuration" do + expect(config_service.redis_uris).to be_a(Hash) + expect(config_service.redis_uris).to be_empty + end + end + + context "when the paas configuration is present but empty" do + before do + allow(ENV).to receive(:[]).with("VCAP_SERVICES").and_return("{}") end it "returns the S3 configuration as not present" do expect(config_service.s3_config_present?).to be(false) end + it "returns the redis configuration as not present" do + expect(config_service.redis_config_present?).to be(false) + end + it "does not retrieve any S3 bucket configuration" do expect(config_service.s3_buckets).to be_a(Hash) expect(config_service.s3_buckets).to be_empty @@ -27,7 +51,7 @@ RSpec.describe PaasConfigurationService do end end - context "when configuration is present but invalid" do + context "when the paas configuration is present but invalid" do let(:vcap_services) { "random text" } before do @@ -35,16 +59,40 @@ RSpec.describe PaasConfigurationService do allow(logger).to receive(:warn) end - it "logs an error" do + it "logs an error when checking if the S3 config is present" do expect(logger).to receive(:warn).with("Could not parse VCAP_SERVICES!") config_service.s3_config_present? end + + it "logs an error when checking if the Redis config is present" do + expect(logger).to receive(:warn).with("Could not parse VCAP_SERVICES!") + config_service.redis_config_present? + end end - context "when the paas configuration is present with S3 buckets" do + context "when the paas configuration is present with S3 configured" do let(:vcap_services) do - <<-JSON - {"aws-s3-bucket": [{"instance_name": "bucket_1"},{"instance_name": "bucket_2"}]} + <<~JSON + {"aws-s3-bucket": + [{ + "instance_name": "bucket_1", + "credentials": { + "aws_access_key_id": "123", + "aws_secret_access_key": "456", + "aws_region": "eu-west-1", + "bucket_name": "my-bucket" + } + }, + { + "instance_name": "bucket_2", + "credentials": { + "aws_access_key_id": "789", + "aws_secret_access_key": "012", + "aws_region": "eu-west-2", + "bucket_name": "my-bucket2" + } + }] + } JSON end @@ -52,14 +100,14 @@ RSpec.describe PaasConfigurationService do allow(ENV).to receive(:[]).with("VCAP_SERVICES").and_return(vcap_services) end - it "returns the configuration as present" do - expect(config_service.config_present?).to be(true) - end - it "returns the S3 configuration as present" do expect(config_service.s3_config_present?).to be(true) end + it "returns the redis configuration as not present" do + expect(config_service.redis_config_present?).to be(false) + end + it "does retrieve the S3 bucket configurations" do s3_buckets = config_service.s3_buckets @@ -70,26 +118,7 @@ RSpec.describe PaasConfigurationService do end end - context "when the paas configuration is present without S3 buckets" do - before do - allow(ENV).to receive(:[]).with("VCAP_SERVICES").and_return("{}") - end - - it "returns the configuration as present" do - expect(config_service.config_present?).to be(true) - end - - it "returns the S3 configuration as not present" do - expect(config_service.s3_config_present?).to be(false) - end - - it "does not retrieve any S3 bucket configuration" do - expect(config_service.s3_buckets).to be_a(Hash) - expect(config_service.s3_buckets).to be_empty - end - end - - context "when the paas configuration is present with redis configurations" do + context "when the paas configuration is present with redis configured" do let(:vcap_services) do <<-JSON {"redis": [{"instance_name": "redis_1", "credentials": {"uri": "redis_uri" }}]} @@ -100,14 +129,14 @@ RSpec.describe PaasConfigurationService do allow(ENV).to receive(:[]).with("VCAP_SERVICES").and_return(vcap_services) end - it "returns the configuration as present" do - expect(config_service.config_present?).to be(true) - end - it "returns the redis configuration as present" do expect(config_service.redis_config_present?).to be(true) end + it "returns the S3 configuration as not present" do + expect(config_service.s3_config_present?).to be(false) + end + it "does retrieve the redis configurations" do redis_uris = config_service.redis_uris @@ -117,23 +146,4 @@ RSpec.describe PaasConfigurationService do expect(redis_uris[:redis_1]).to eq("redis_uri") end end - - context "when the paas configuration is present without redis configuration" do - before do - allow(ENV).to receive(:[]).with("VCAP_SERVICES").and_return("{}") - end - - it "returns the configuration as present" do - expect(config_service.config_present?).to be(true) - end - - it "returns the redis configuration as not present" do - expect(config_service.redis_config_present?).to be(false) - end - - it "does not retrieve any redis uris from configuration" do - expect(config_service.redis_uris).to be_a(Hash) - expect(config_service.redis_uris).to be_empty - end - end end diff --git a/spec/services/exports/case_log_export_service_spec.rb b/spec/services/exports/case_log_export_service_spec.rb index fc59cde4e..82124f288 100644 --- a/spec/services/exports/case_log_export_service_spec.rb +++ b/spec/services/exports/case_log_export_service_spec.rb @@ -3,7 +3,7 @@ require "rails_helper" RSpec.describe Exports::CaseLogExportService do subject(:export_service) { described_class.new(storage_service) } - let(:storage_service) { instance_double(S3StorageService) } + let(:storage_service) { instance_double(Storage::S3Service) } let(:xml_export_file) { File.open("spec/fixtures/exports/general_needs_log.xml", "r:UTF-8") } let(:local_manifest_file) { File.open("spec/fixtures/exports/manifest.xml", "r:UTF-8") } diff --git a/spec/services/imports/case_logs_field_import_service_spec.rb b/spec/services/imports/case_logs_field_import_service_spec.rb index 7184ee648..e703c3786 100644 --- a/spec/services/imports/case_logs_field_import_service_spec.rb +++ b/spec/services/imports/case_logs_field_import_service_spec.rb @@ -3,7 +3,7 @@ require "rails_helper" RSpec.describe Imports::CaseLogsFieldImportService do subject(:import_service) { described_class.new(storage_service, logger) } - let(:storage_service) { instance_double(S3StorageService) } + let(:storage_service) { instance_double(Storage::S3Service) } let(:logger) { instance_double(ActiveSupport::Logger) } let(:real_2021_2022_form) { Form.new("config/forms/2021_2022.json", "2021_2022") } diff --git a/spec/services/imports/case_logs_import_service_spec.rb b/spec/services/imports/case_logs_import_service_spec.rb index 30a5cc41e..075244ecf 100644 --- a/spec/services/imports/case_logs_import_service_spec.rb +++ b/spec/services/imports/case_logs_import_service_spec.rb @@ -3,7 +3,7 @@ require "rails_helper" RSpec.describe Imports::CaseLogsImportService do subject(:case_log_service) { described_class.new(storage_service, logger) } - let(:storage_service) { instance_double(S3StorageService) } + let(:storage_service) { instance_double(Storage::S3Service) } let(:logger) { instance_double(ActiveSupport::Logger) } let(:real_2021_2022_form) { Form.new("config/forms/2021_2022.json", "2021_2022") } diff --git a/spec/services/imports/data_protection_confirmation_import_service_spec.rb b/spec/services/imports/data_protection_confirmation_import_service_spec.rb index db5a25046..cb14829b4 100644 --- a/spec/services/imports/data_protection_confirmation_import_service_spec.rb +++ b/spec/services/imports/data_protection_confirmation_import_service_spec.rb @@ -5,7 +5,7 @@ RSpec.describe Imports::DataProtectionConfirmationImportService do let(:old_org_id) { "7c5bd5fb549c09a2c55d7cb90d7ba84927e64618" } let(:old_id) { old_org_id } let(:import_file) { File.open("#{fixture_directory}/#{old_id}.xml") } - let(:storage_service) { instance_double(S3StorageService) } + let(:storage_service) { instance_double(Storage::S3Service) } let(:logger) { instance_double(ActiveSupport::Logger) } context "when importing data protection confirmations" do diff --git a/spec/services/imports/organisation_import_service_spec.rb b/spec/services/imports/organisation_import_service_spec.rb index 5eceea549..48a0d72bb 100644 --- a/spec/services/imports/organisation_import_service_spec.rb +++ b/spec/services/imports/organisation_import_service_spec.rb @@ -1,7 +1,7 @@ require "rails_helper" RSpec.describe Imports::OrganisationImportService do - let(:storage_service) { instance_double(S3StorageService) } + let(:storage_service) { instance_double(Storage::S3Service) } let(:logger) { instance_double(Rails::Rack::Logger) } let(:folder_name) { "organisations" } let(:filenames) { %w[my_folder/my_file1.xml my_folder/my_file2.xml] } diff --git a/spec/services/imports/organisation_rent_period_import_service_spec.rb b/spec/services/imports/organisation_rent_period_import_service_spec.rb index b85bb64e5..adb4899e9 100644 --- a/spec/services/imports/organisation_rent_period_import_service_spec.rb +++ b/spec/services/imports/organisation_rent_period_import_service_spec.rb @@ -5,7 +5,7 @@ RSpec.describe Imports::OrganisationRentPeriodImportService do let(:old_org_id) { "44026acc7ed5c29516b26f2a5deb639e5e37966d" } let(:old_id) { "ebd22326d33e389e9f1bfd546979d2c05f9e68d6" } let(:import_file) { File.open("#{fixture_directory}/#{old_id}.xml") } - let(:storage_service) { instance_double(S3StorageService) } + let(:storage_service) { instance_double(Storage::S3Service) } let(:logger) { instance_double(ActiveSupport::Logger) } context "when importing organisation rent periods" do diff --git a/spec/services/imports/scheme_import_service_spec.rb b/spec/services/imports/scheme_import_service_spec.rb index b6b98d25d..efba81a1c 100644 --- a/spec/services/imports/scheme_import_service_spec.rb +++ b/spec/services/imports/scheme_import_service_spec.rb @@ -3,7 +3,7 @@ require "rails_helper" RSpec.describe Imports::SchemeImportService do subject(:scheme_service) { described_class.new(storage_service, logger) } - let(:storage_service) { instance_double(S3StorageService) } + let(:storage_service) { instance_double(Storage::S3Service) } let(:logger) { instance_double(ActiveSupport::Logger) } let(:fixture_directory) { "spec/fixtures/imports/mgmtgroups" } diff --git a/spec/services/imports/scheme_location_import_service_spec.rb b/spec/services/imports/scheme_location_import_service_spec.rb index ebe421f45..70ff039cd 100644 --- a/spec/services/imports/scheme_location_import_service_spec.rb +++ b/spec/services/imports/scheme_location_import_service_spec.rb @@ -3,7 +3,7 @@ require "rails_helper" RSpec.describe Imports::SchemeLocationImportService do subject(:location_service) { described_class.new(storage_service, logger) } - let(:storage_service) { instance_double(S3StorageService) } + let(:storage_service) { instance_double(Storage::S3Service) } let(:logger) { instance_double(ActiveSupport::Logger) } let(:fixture_directory) { "spec/fixtures/imports/schemes" } diff --git a/spec/services/imports/user_import_service_spec.rb b/spec/services/imports/user_import_service_spec.rb index b9e3028c9..c4aea00bc 100644 --- a/spec/services/imports/user_import_service_spec.rb +++ b/spec/services/imports/user_import_service_spec.rb @@ -5,7 +5,7 @@ RSpec.describe Imports::UserImportService do let(:old_user_id) { "fc7625a02b24ae16162aa63ae7cb33feeec0c373" } let(:old_org_id) { "7c5bd5fb549c09a2c55d7cb90d7ba84927e64618" } let(:user_file) { File.open("#{fixture_directory}/#{old_user_id}.xml") } - let(:storage_service) { instance_double(S3StorageService) } + let(:storage_service) { instance_double(Storage::S3Service) } let(:logger) { instance_double(ActiveSupport::Logger) } let(:notify_client) { instance_double(Notifications::Client) } let(:devise_notify_mailer) { DeviseNotifyMailer.new } diff --git a/spec/services/archive_storage_service_spec.rb b/spec/services/storage/archive_service_spec.rb similarity index 98% rename from spec/services/archive_storage_service_spec.rb rename to spec/services/storage/archive_service_spec.rb index fb33e4dd3..52808262f 100644 --- a/spec/services/archive_storage_service_spec.rb +++ b/spec/services/storage/archive_service_spec.rb @@ -1,6 +1,6 @@ require "rails_helper" -RSpec.describe ArchiveStorageService do +RSpec.describe Storage::ArchiveService do subject(:archive_service) { described_class.new(archive_content) } let(:compressed_folder) { "my_directory" } diff --git a/spec/services/s3_storage_service_spec.rb b/spec/services/storage/s3_service_spec.rb similarity index 80% rename from spec/services/s3_storage_service_spec.rb rename to spec/services/storage/s3_service_spec.rb index c89b7c1bb..36a549b21 100644 --- a/spec/services/s3_storage_service_spec.rb +++ b/spec/services/storage/s3_service_spec.rb @@ -1,6 +1,6 @@ require "rails_helper" -RSpec.describe S3StorageService do +RSpec.describe Storage::S3Service do let(:instance_name) { "instance_1" } let(:bucket_name) { "bucket_1" } let(:vcap_services) do @@ -20,18 +20,24 @@ RSpec.describe S3StorageService do end context "when we create a storage service with no PaaS Configuration present" do - subject(:storage_service) { described_class.new(PaasConfigurationService.new, "random_instance") } + subject(:storage_service) { described_class.new(Configuration::PaasConfigurationService.new, "random_instance") } it "raises an exception" do - expect { storage_service }.to raise_error(RuntimeError, /No PaaS configuration present/) + expect { storage_service }.to raise_error(RuntimeError, "No S3 bucket is present in the PaaS configuration") end end - context "when we create a storage service with an unknown instance name" do - subject(:storage_service) { described_class.new(PaasConfigurationService.new, "random_instance") } + context "when we create a storage service and the S3 instance name is not found in the PaaS configuration" do + subject(:storage_service) { described_class.new(Configuration::PaasConfigurationService.new, "random_instance") } + + let(:vcap_services) do + <<-JSON + {"aws-s3-bucket": []} + JSON + end before do - allow(ENV).to receive(:[]).with("VCAP_SERVICES").and_return("{}") + allow(ENV).to receive(:[]).with("VCAP_SERVICES").and_return(vcap_services) end it "raises an exception" do @@ -40,7 +46,7 @@ RSpec.describe S3StorageService do end context "when we create a storage service with a valid instance name" do - subject(:storage_service) { described_class.new(PaasConfigurationService.new, instance_name) } + subject(:storage_service) { described_class.new(Configuration::PaasConfigurationService.new, instance_name) } before do allow(ENV).to receive(:[]) @@ -48,11 +54,11 @@ RSpec.describe S3StorageService do end it "creates a Storage Configuration" do - expect(storage_service.configuration).to be_an(StorageConfiguration) + expect(storage_service.configuration).to be_an(Storage::StorageConfiguration) end it "sets the expected parameters in the configuration" do - expected_configuration = StorageConfiguration.new( + expected_configuration = Storage::StorageConfiguration.new( { aws_access_key_id: "key_id", aws_region: "eu-west-2", @@ -65,7 +71,7 @@ RSpec.describe S3StorageService do end context "when we create a storage service and write a stubbed object" do - subject(:storage_service) { described_class.new(PaasConfigurationService.new, instance_name) } + subject(:storage_service) { described_class.new(Configuration::PaasConfigurationService.new, instance_name) } let(:filename) { "my_file" } let(:content) { "content" } @@ -100,7 +106,7 @@ RSpec.describe S3StorageService do end context "when we create a storage service" do - subject(:storage_service) { described_class.new(PaasConfigurationService.new, instance_name) } + subject(:storage_service) { described_class.new(Configuration::PaasConfigurationService.new, instance_name) } let(:s3_client_stub) { Aws::S3::Client.new(stub_responses: true) } From f0eaea0135d7e3afbfce2fc4a8ed2191b8b3734a Mon Sep 17 00:00:00 2001 From: natdeanlewissoftwire <94526761+natdeanlewissoftwire@users.noreply.github.com> Date: Thu, 25 Aug 2022 13:53:59 +0100 Subject: [PATCH 3/3] Cldc 1436 csv schemes info (#843) * Add BOMs before CSV info * add BOM to tests * DRYing * remove added blank line * add scheme and location columns to exported csv logs, remove scheme_id and location_id * reformat * update tests * linting * linting * use delegate to simplify code * update column names in expected csvs for tests * update to use csv_case_log_service * update tests * delegate scheme_owning and scheme_managing _organisaton_names * update tests * update spec variable names --- app/models/case_log.rb | 23 +++++++++++++++++++ app/services/csv/case_log_csv_service.rb | 12 ++++++---- spec/fixtures/files/case_logs_download.csv | 4 ++-- .../files/case_logs_download_non_support.csv | 4 ++-- spec/models/case_log_spec.rb | 10 ++++++++ .../services/csv/case_log_csv_service_spec.rb | 23 ++++++++++++++++++- 6 files changed, 66 insertions(+), 10 deletions(-) diff --git a/app/models/case_log.rb b/app/models/case_log.rb index 3f61ed4e4..a98296bb2 100644 --- a/app/models/case_log.rb +++ b/app/models/case_log.rb @@ -442,6 +442,29 @@ class CaseLog < ApplicationRecord created_by&.is_dpo end + delegate :service_name, :sensitive, :registered_under_care_act, :primary_client_group, :has_other_client_group, :secondary_client_group, :owning_organisation, :managing_organisation, :support_type, :intended_stay, :created_at, prefix: "scheme", to: :scheme, allow_nil: true + delegate :scheme_type, to: :scheme, allow_nil: true + + def scheme_code + scheme&.id ? "S#{scheme.id}" : nil + end + + def scheme_owning_organisation_name + scheme_owning_organisation&.name + end + + def scheme_managing_organisation_name + scheme_managing_organisation&.name + end + + delegate :postcode, :name, :units, :type_of_unit, :mobility_type, :startdate, prefix: "location", to: :location, allow_nil: true + delegate :location_admin_district, to: :location, allow_nil: true + + # This is not the location_code in the db, location.id is just called code in the UI + def location_code + location&.id + end + def self.to_csv(user = nil) Csv::CaseLogCsvService.new(user).to_csv end diff --git a/app/services/csv/case_log_csv_service.rb b/app/services/csv/case_log_csv_service.rb index ff6474b80..11c45ff21 100644 --- a/app/services/csv/case_log_csv_service.rb +++ b/app/services/csv/case_log_csv_service.rb @@ -1,6 +1,6 @@ module Csv class CaseLogCsvService - CSV_FIELDS_TO_OMIT = %w[hhmemb net_income_value_check sale_or_letting first_time_property_let_as_social_housing renttype needstype postcode_known is_la_inferred totchild totelder totadult net_income_known is_carehome previous_la_known is_previous_la_inferred age1_known age2_known age3_known age4_known age5_known age6_known age7_known age8_known letting_allocation_unknown details_known_2 details_known_3 details_known_4 details_known_5 details_known_6 details_known_7 details_known_8 rent_type wrent wscharge wpschrge wsupchrg wtcharge wtshortfall rent_value_check old_form_id old_id retirement_value_check tshortfall_known pregnancy_value_check hhtype new_old vacdays].freeze + CSV_FIELDS_TO_OMIT = %w[hhmemb net_income_value_check sale_or_letting first_time_property_let_as_social_housing renttype needstype postcode_known is_la_inferred totchild totelder totadult net_income_known is_carehome previous_la_known is_previous_la_inferred age1_known age2_known age3_known age4_known age5_known age6_known age7_known age8_known letting_allocation_unknown details_known_2 details_known_3 details_known_4 details_known_5 details_known_6 details_known_7 details_known_8 rent_type wrent wscharge wpschrge wsupchrg wtcharge wtshortfall rent_value_check old_form_id old_id retirement_value_check tshortfall_known pregnancy_value_check hhtype new_old vacdays scheme_id location_id].freeze def initialize(user) @user = user @@ -31,11 +31,13 @@ module Csv def set_csv_attributes metadata_fields = %w[id status created_at updated_at created_by_name is_dpo owning_organisation_name managing_organisation_name] metadata_id_fields = %w[managing_organisation_id owning_organisation_id created_by_id] - scheme_attributes = %w[scheme_id location_id] - intersecting_attributes = ordered_form_questions & CaseLog.attribute_names - scheme_attributes - remaining_attributes = CaseLog.attribute_names - intersecting_attributes - scheme_attributes + scheme_and_location_ids = %w[scheme_id location_id] + scheme_attributes = %w[scheme_code scheme_service_name scheme_sensitive scheme_type scheme_registered_under_care_act scheme_owning_organisation_name scheme_managing_organisation_name scheme_primary_client_group scheme_has_other_client_group scheme_secondary_client_group scheme_support_type scheme_intended_stay scheme_created_at] + location_attributes = %w[location_code location_postcode location_name location_units location_type_of_unit location_mobility_type location_admin_district location_startdate] + intersecting_attributes = ordered_form_questions & CaseLog.attribute_names - scheme_and_location_ids + remaining_attributes = CaseLog.attribute_names - intersecting_attributes - scheme_and_location_ids - @attributes = (metadata_fields + intersecting_attributes + remaining_attributes - metadata_id_fields + %w[unittype_sh] + scheme_attributes).uniq + @attributes = (metadata_fields + intersecting_attributes + remaining_attributes - metadata_id_fields + %w[unittype_sh] + scheme_and_location_ids + scheme_attributes + location_attributes).uniq move_is_inferred_fields @attributes -= CSV_FIELDS_TO_OMIT if @user.present? && !@user.support? diff --git a/spec/fixtures/files/case_logs_download.csv b/spec/fixtures/files/case_logs_download.csv index 62f6e4b64..483641117 100644 --- a/spec/fixtures/files/case_logs_download.csv +++ b/spec/fixtures/files/case_logs_download.csv @@ -1,2 +1,2 @@ -id,status,created_at,updated_at,created_by_name,is_dpo,owning_organisation_name,managing_organisation_name,needstype,renewal,startdate,rent_type,irproduct_other,tenancycode,propcode,age1,sex1,ecstat1,hhmemb,relat2,age2,sex2,retirement_value_check,ecstat2,armedforces,leftreg,illness,housingneeds_a,housingneeds_b,housingneeds_c,housingneeds_h,is_previous_la_inferred,prevloc,illness_type_1,illness_type_2,is_la_inferred,la,postcode_known,postcode_full,previous_la_known,wchair,preg_occ,cbl,earnings,incfreq,net_income_value_check,benefits,hb,period,brent,scharge,pscharge,supcharg,tcharge,offered,layear,ppostcode_full,mrcdate,declaration,ethnic,national,prevten,age3,sex3,ecstat3,age4,sex4,ecstat4,age5,sex5,ecstat5,age6,sex6,ecstat6,age7,sex7,ecstat7,age8,sex8,ecstat8,homeless,underoccupation_benefitcap,reservist,startertenancy,tenancylength,tenancy,rsnvac,unittype_gn,beds,waityear,reasonpref,chr,cap,reasonother,housingneeds_f,housingneeds_g,illness_type_3,illness_type_4,illness_type_8,illness_type_5,illness_type_6,illness_type_7,illness_type_9,illness_type_10,rp_homeless,rp_insan_unsat,rp_medwel,rp_hardship,rp_dontknow,tenancyother,property_owner_organisation,property_manager_organisation,sale_or_letting,purchaser_code,reason,majorrepairs,hbrentshortfall,property_relet,incref,sale_completion_date,first_time_property_let_as_social_housing,unitletas,builtype,voiddate,renttype,lettype,totchild,totelder,totadult,net_income_known,nocharge,is_carehome,household_charge,referral,tshortfall,chcharge,ppcodenk,age1_known,age2_known,age3_known,age4_known,age5_known,age6_known,age7_known,age8_known,ethnic_group,ethnic_other,letting_allocation_unknown,details_known_2,details_known_3,details_known_4,details_known_5,details_known_6,details_known_7,details_known_8,has_benefits,wrent,wscharge,wpschrge,wsupchrg,wtcharge,wtshortfall,refused,housingneeds,wchchrg,newprop,relat3,relat4,relat5,relat6,relat7,relat8,rent_value_check,old_form_id,lar,irproduct,old_id,joint,illness_type_0,tshortfall_known,sheltered,pregnancy_value_check,hhtype,new_old,vacdays,major_repairs_date_value_check,void_date_value_check,housingneeds_type,housingneeds_other,unittype_sh,scheme_id,location_id -{id},in_progress,2022-02-08 16:52:15 +0000,2022-02-08 16:52:15 +0000,Danny Rojas,No,DLUHC,DLUHC,Supported housing,,,,,,,,,,,,,,,,,,,,,,,No,,,,No,Westminster,,SE1 1TE,,2,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,0,0,0,,0,,,,,,,,,,,,,,,,,,,,,,,,,0,,,,,,,0,,,,,,,,,,,,,,,,,,,,9,,,,,,,6,{scheme_id},SE1 1TE +id,status,created_at,updated_at,created_by_name,is_dpo,owning_organisation_name,managing_organisation_name,needstype,renewal,startdate,rent_type,irproduct_other,tenancycode,propcode,age1,sex1,ecstat1,hhmemb,relat2,age2,sex2,retirement_value_check,ecstat2,armedforces,leftreg,illness,housingneeds_a,housingneeds_b,housingneeds_c,housingneeds_h,is_previous_la_inferred,prevloc,illness_type_1,illness_type_2,is_la_inferred,la,postcode_known,postcode_full,previous_la_known,wchair,preg_occ,cbl,earnings,incfreq,net_income_value_check,benefits,hb,period,brent,scharge,pscharge,supcharg,tcharge,offered,layear,ppostcode_full,mrcdate,declaration,ethnic,national,prevten,age3,sex3,ecstat3,age4,sex4,ecstat4,age5,sex5,ecstat5,age6,sex6,ecstat6,age7,sex7,ecstat7,age8,sex8,ecstat8,homeless,underoccupation_benefitcap,reservist,startertenancy,tenancylength,tenancy,rsnvac,unittype_gn,beds,waityear,reasonpref,chr,cap,reasonother,housingneeds_f,housingneeds_g,illness_type_3,illness_type_4,illness_type_8,illness_type_5,illness_type_6,illness_type_7,illness_type_9,illness_type_10,rp_homeless,rp_insan_unsat,rp_medwel,rp_hardship,rp_dontknow,tenancyother,property_owner_organisation,property_manager_organisation,sale_or_letting,purchaser_code,reason,majorrepairs,hbrentshortfall,property_relet,incref,sale_completion_date,first_time_property_let_as_social_housing,unitletas,builtype,voiddate,renttype,lettype,totchild,totelder,totadult,net_income_known,nocharge,is_carehome,household_charge,referral,tshortfall,chcharge,ppcodenk,age1_known,age2_known,age3_known,age4_known,age5_known,age6_known,age7_known,age8_known,ethnic_group,ethnic_other,letting_allocation_unknown,details_known_2,details_known_3,details_known_4,details_known_5,details_known_6,details_known_7,details_known_8,has_benefits,wrent,wscharge,wpschrge,wsupchrg,wtcharge,wtshortfall,refused,housingneeds,wchchrg,newprop,relat3,relat4,relat5,relat6,relat7,relat8,rent_value_check,old_form_id,lar,irproduct,old_id,joint,illness_type_0,tshortfall_known,sheltered,pregnancy_value_check,hhtype,new_old,vacdays,major_repairs_date_value_check,void_date_value_check,housingneeds_type,housingneeds_other,unittype_sh,scheme_id,location_id,scheme_code,scheme_service_name,scheme_sensitive,scheme_type,scheme_registered_under_care_act,scheme_owning_organisation_name,scheme_managing_organisation_name,scheme_primary_client_group,scheme_has_other_client_group,scheme_secondary_client_group,scheme_support_type,scheme_intended_stay,scheme_created_at,location_code,location_postcode,location_name,location_units,location_type_of_unit,location_mobility_type,location_admin_district,location_startdate +{id},in_progress,2022-02-08 16:52:15 +0000,2022-02-08 16:52:15 +0000,Danny Rojas,No,DLUHC,DLUHC,Supported housing,,,,,,,,,,,,,,,,,,,,,,,No,,,,No,Westminster,,SE1 1TE,,2,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,0,0,0,,0,,,,,,,,,,,,,,,,,,,,,,,,,0,,,,,,,0,,,,,,,,,,,,,,,,,,,,9,,,,,,,6,{scheme_id},SE1 1TE,{scheme_code},{scheme_service_name},{scheme_sensitive},Missing,No,DLUHC,DLUHC,{scheme_primary_client_group},,{scheme_secondary_client_group},{scheme_support_type},{scheme_intended_stay},2022-06-05 01:00:00 +0100,{location_code},SE1 1TE,Downing Street,20,Bungalow,Fitted with equipment and adaptations,Westminster,{location_startdate} diff --git a/spec/fixtures/files/case_logs_download_non_support.csv b/spec/fixtures/files/case_logs_download_non_support.csv index a2b12c89b..12aca6dcf 100644 --- a/spec/fixtures/files/case_logs_download_non_support.csv +++ b/spec/fixtures/files/case_logs_download_non_support.csv @@ -1,2 +1,2 @@ -id,status,created_at,updated_at,created_by_name,is_dpo,owning_organisation_name,managing_organisation_name,renewal,startdate,irproduct_other,tenancycode,propcode,age1,sex1,ecstat1,relat2,age2,sex2,ecstat2,armedforces,leftreg,illness,housingneeds_a,housingneeds_b,housingneeds_c,housingneeds_h,prevloc,illness_type_1,illness_type_2,la,postcode_full,wchair,preg_occ,cbl,earnings,incfreq,benefits,hb,period,brent,scharge,pscharge,supcharg,tcharge,offered,layear,ppostcode_full,mrcdate,declaration,ethnic,national,prevten,age3,sex3,ecstat3,age4,sex4,ecstat4,age5,sex5,ecstat5,age6,sex6,ecstat6,age7,sex7,ecstat7,age8,sex8,ecstat8,homeless,underoccupation_benefitcap,reservist,startertenancy,tenancylength,tenancy,rsnvac,unittype_gn,beds,waityear,reasonpref,chr,cap,reasonother,housingneeds_f,housingneeds_g,illness_type_3,illness_type_4,illness_type_8,illness_type_5,illness_type_6,illness_type_7,illness_type_9,illness_type_10,rp_homeless,rp_insan_unsat,rp_medwel,rp_hardship,rp_dontknow,tenancyother,property_owner_organisation,property_manager_organisation,purchaser_code,reason,majorrepairs,hbrentshortfall,property_relet,incref,sale_completion_date,unitletas,builtype,voiddate,lettype,nocharge,household_charge,referral,tshortfall,chcharge,ppcodenk,ethnic_group,ethnic_other,has_benefits,refused,housingneeds,wchchrg,newprop,relat3,relat4,relat5,relat6,relat7,relat8,lar,irproduct,joint,illness_type_0,sheltered,major_repairs_date_value_check,void_date_value_check,housingneeds_type,housingneeds_other,unittype_sh,scheme_id,location_id -{id},in_progress,2022-02-08 16:52:15 +0000,2022-02-08 16:52:15 +0000,Danny Rojas,No,DLUHC,DLUHC,,,,,,,,,,,,,,,,,,,,,,,Westminster,SE1 1TE,2,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,0,,,,,,,,0,0,,,,,,,,,,,,,,,,,,,6,{scheme_id},SE1 1TE +id,status,created_at,updated_at,created_by_name,is_dpo,owning_organisation_name,managing_organisation_name,renewal,startdate,irproduct_other,tenancycode,propcode,age1,sex1,ecstat1,relat2,age2,sex2,ecstat2,armedforces,leftreg,illness,housingneeds_a,housingneeds_b,housingneeds_c,housingneeds_h,prevloc,illness_type_1,illness_type_2,la,postcode_full,wchair,preg_occ,cbl,earnings,incfreq,benefits,hb,period,brent,scharge,pscharge,supcharg,tcharge,offered,layear,ppostcode_full,mrcdate,declaration,ethnic,national,prevten,age3,sex3,ecstat3,age4,sex4,ecstat4,age5,sex5,ecstat5,age6,sex6,ecstat6,age7,sex7,ecstat7,age8,sex8,ecstat8,homeless,underoccupation_benefitcap,reservist,startertenancy,tenancylength,tenancy,rsnvac,unittype_gn,beds,waityear,reasonpref,chr,cap,reasonother,housingneeds_f,housingneeds_g,illness_type_3,illness_type_4,illness_type_8,illness_type_5,illness_type_6,illness_type_7,illness_type_9,illness_type_10,rp_homeless,rp_insan_unsat,rp_medwel,rp_hardship,rp_dontknow,tenancyother,property_owner_organisation,property_manager_organisation,purchaser_code,reason,majorrepairs,hbrentshortfall,property_relet,incref,sale_completion_date,unitletas,builtype,voiddate,lettype,nocharge,household_charge,referral,tshortfall,chcharge,ppcodenk,ethnic_group,ethnic_other,has_benefits,refused,housingneeds,wchchrg,newprop,relat3,relat4,relat5,relat6,relat7,relat8,lar,irproduct,joint,illness_type_0,sheltered,major_repairs_date_value_check,void_date_value_check,housingneeds_type,housingneeds_other,unittype_sh,scheme_code,scheme_service_name,scheme_sensitive,scheme_type,scheme_registered_under_care_act,scheme_owning_organisation_name,scheme_managing_organisation_name,scheme_primary_client_group,scheme_has_other_client_group,scheme_secondary_client_group,scheme_support_type,scheme_intended_stay,scheme_created_at,location_code,location_postcode,location_name,location_units,location_type_of_unit,location_mobility_type,location_admin_district,location_startdate +{id},in_progress,2022-02-08 16:52:15 +0000,2022-02-08 16:52:15 +0000,Danny Rojas,No,DLUHC,DLUHC,,,,,,,,,,,,,,,,,,,,,,,Westminster,SE1 1TE,2,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,0,,,,,,,,0,0,,,,,,,,,,,,,,,,,,,6,{scheme_code},{scheme_service_name},{scheme_sensitive},Missing,No,DLUHC,DLUHC,{scheme_primary_client_group},,{scheme_secondary_client_group},{scheme_support_type},{scheme_intended_stay},2022-06-05 01:00:00 +0100,{location_code},SE1 1TE,Downing Street,20,Bungalow,Fitted with equipment and adaptations,Westminster,{location_startdate} diff --git a/spec/models/case_log_spec.rb b/spec/models/case_log_spec.rb index cbe1666cf..eadf2d6b0 100644 --- a/spec/models/case_log_spec.rb +++ b/spec/models/case_log_spec.rb @@ -2295,7 +2295,17 @@ RSpec.describe CaseLog do Timecop.freeze(Time.utc(2022, 6, 5)) case_log = FactoryBot.create(:case_log, needstype: 2, scheme:, location:, owning_organisation: scheme.owning_organisation, created_by: user) expected_content.sub!(/\{id\}/, case_log["id"].to_s) + expected_content.sub!(/\{scheme_code\}/, "S#{scheme['id']}") + expected_content.sub!(/\{scheme_service_name\}/, scheme["service_name"].to_s) + expected_content.sub!(/\{scheme_sensitive\}/, scheme["sensitive"].to_s) + expected_content.sub!(/\{scheme_primary_client_group\}/, scheme["primary_client_group"].to_s) + expected_content.sub!(/\{scheme_secondary_client_group\}/, scheme["secondary_client_group"].to_s) + expected_content.sub!(/\{scheme_support_type\}/, scheme["support_type"].to_s) + expected_content.sub!(/\{scheme_intended_stay\}/, scheme["intended_stay"].to_s) + expected_content.sub!(/\{location_code\}/, location["id"].to_s) + expected_content.sub!(/\{location_startdate\}/, location["startdate"].to_s) expected_content.sub!(/\{scheme_id\}/, scheme["service_name"].to_s) + expected_content.sub!(/\{location_id\}/, location["id"].to_s) end context "with a support user" do diff --git a/spec/services/csv/case_log_csv_service_spec.rb b/spec/services/csv/case_log_csv_service_spec.rb index 1815b5ea2..f92eab815 100644 --- a/spec/services/csv/case_log_csv_service_spec.rb +++ b/spec/services/csv/case_log_csv_service_spec.rb @@ -202,7 +202,28 @@ RSpec.describe Csv::CaseLogCsvService do housingneeds_other unittype_sh scheme_id - location_id] + location_id + scheme_code + scheme_service_name + scheme_sensitive + scheme_type + scheme_registered_under_care_act + scheme_owning_organisation_name + scheme_managing_organisation_name + scheme_primary_client_group + scheme_has_other_client_group + scheme_secondary_client_group + scheme_support_type + scheme_intended_stay + scheme_created_at + location_code + location_postcode + location_name + location_units + location_type_of_unit + location_mobility_type + location_admin_district + location_startdate] csv = CSV.parse(described_class.new(user).to_csv) expect(csv.first).to eq(expected_csv_attributes) end