From 677714c336e95069ddf6817146f34a5c00f45f45 Mon Sep 17 00:00:00 2001 From: Jack <113976590+bibblobcode@users.noreply.github.com> Date: Wed, 28 Jun 2023 08:52:00 +0100 Subject: [PATCH] Fix bug in DPC import script (#1723) * Add test as a proof of the bug we have in prod * Refactor script to create users with unique email * Add rake task to import missing data protection confirmations --- ..._protection_confirmation_import_service.rb | 19 +- lib/tasks/data_import.rake | 23 ++ spec/lib/tasks/data_import_spec.rb | 284 ++++++++++-------- ...ection_confirmation_import_service_spec.rb | 7 +- 4 files changed, 204 insertions(+), 129 deletions(-) diff --git a/app/services/imports/data_protection_confirmation_import_service.rb b/app/services/imports/data_protection_confirmation_import_service.rb index 30d28b647..e2694ecba 100644 --- a/app/services/imports/data_protection_confirmation_import_service.rb +++ b/app/services/imports/data_protection_confirmation_import_service.rb @@ -8,6 +8,10 @@ module Imports def create_data_protection_confirmation(xml_document) org = Organisation.find_by(old_org_id: record_field_value(xml_document, "institution")) + + return log_org_must_exist if org.blank? + return log_dpc_already_present(org) if org.data_protection_confirmed? + dp_officer = User.find_by( name: record_field_value(xml_document, "dp-user"), organisation: org, @@ -20,6 +24,9 @@ module Imports organisation: org, is_dpo: true, encrypted_password: SecureRandom.hex(10), + email: SecureRandom.uuid, + confirmed_at: Time.zone.now, + active: false, ) dp_officer.save!(validate: false) end @@ -32,14 +39,18 @@ module Imports old_org_id: record_field_value(xml_document, "institution"), created_at: record_field_value(xml_document, "change-date").to_time(:utc), ) - rescue ActiveRecord::RecordNotUnique - id = record_field_value(xml_document, "id") - dp_officer_name = record_field_value(xml_document, "dp-user") - @logger.warn("Data protection confirmation #{id} created by #{dp_officer_name} for #{org.name} is already present, skipping.") end def record_field_value(xml_document, field) field_value(xml_document, "dataprotect", field) end + + def log_dpc_already_present(org) + @logger.warn("Data protection confirmation for #{org.name} with id #{org.id} is already present, skipping.") + end + + def log_org_must_exist + @logger.error("Organisation must exist") + end end end diff --git a/lib/tasks/data_import.rake b/lib/tasks/data_import.rake index 0dede82a3..ea19d96b5 100644 --- a/lib/tasks/data_import.rake +++ b/lib/tasks/data_import.rake @@ -28,4 +28,27 @@ namespace :core do raise "Type #{type} is not supported by data_import" end end + + desc "Import missing data sharing agreement XMLs from legacy CORE" + task :data_protection_confirmation_import, %i[type path] => :environment do |_task| + orgs_without_dpc = Organisation.includes(:data_protection_confirmation) + .where.not(id: DataProtectionConfirmation.all.select(:organisation_id)) + .where.not(old_org_id: nil) + + puts "Processing #{orgs_without_dpc.count} orgs without data protection confirmation" + s3_service = Storage::S3Service.new(Configuration::PaasConfigurationService.new, ENV["IMPORT_PAAS_INSTANCE"]) + + orgs_without_dpc.each do |org| + archive_path = "#{org.old_org_id}.zip" + archive_io = s3_service.get_file_io(archive_path) + archive_service = Storage::ArchiveService.new(archive_io) + if archive_service.folder_present?("dataprotect") + Rails.logger.info("Start importing folder dataprotect") + Imports::DataProtectionConfirmationImportService.new(archive_service).create_data_protection_confirmations("dataprotect") + Rails.logger.info("Imported") + else + Rails.logger.info("dataprotect does not exist, skipping import") + end + end + end end diff --git a/spec/lib/tasks/data_import_spec.rb b/spec/lib/tasks/data_import_spec.rb index 462f2740e..c251477b9 100644 --- a/spec/lib/tasks/data_import_spec.rb +++ b/spec/lib/tasks/data_import_spec.rb @@ -1,177 +1,217 @@ require "rails_helper" require "rake" -describe "rake core:data_import", type: :task do - subject(:task) { Rake::Task["core:data_import"] } - +describe "data import", type: :task do let(:instance_name) { "paas_import_instance" } - let(:storage_service) { instance_double(Storage::S3Service) } let(:paas_config_service) { instance_double(Configuration::PaasConfigurationService) } + let(:storage_service) { instance_double(Storage::S3Service) } - before do - Rake.application.rake_require("tasks/data_import") - Rake::Task.define_task(:environment) - task.reenable - - 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 - - context "when importing organisation data" do - let(:type) { "organisation" } - let(:import_service) { instance_double(Imports::OrganisationImportService) } - let(:fixture_path) { "spec/fixtures/imports/organisations" } + describe "core:data_import" do + subject(:task) { Rake::Task["core:data_import"] } before do - allow(Imports::OrganisationImportService).to receive(:new).and_return(import_service) + Rake.application.rake_require("tasks/data_import") + Rake::Task.define_task(:environment) + task.reenable + + 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 - it "creates an organisation from the given XML file" do - 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) + context "when importing organisation data" do + let(:type) { "organisation" } + let(:import_service) { instance_double(Imports::OrganisationImportService) } + let(:fixture_path) { "spec/fixtures/imports/organisations" } - task.invoke(type, fixture_path) - end - end + before do + allow(Imports::OrganisationImportService).to receive(:new).and_return(import_service) + end - context "when importing user data" do - let(:type) { "user" } - let(:import_service) { instance_double(Imports::UserImportService) } - let(:fixture_path) { "spec/fixtures/imports/users" } + it "creates an organisation from the given XML file" do + 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) - before do - allow(Imports::UserImportService).to receive(:new).and_return(import_service) + task.invoke(type, fixture_path) + end end - it "creates a user from the given XML file" do - 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) + context "when importing user data" do + let(:type) { "user" } + let(:import_service) { instance_double(Imports::UserImportService) } + let(:fixture_path) { "spec/fixtures/imports/users" } - task.invoke(type, fixture_path) - end - end + before do + allow(Imports::UserImportService).to receive(:new).and_return(import_service) + end - context "when importing data protection confirmation data" do - let(:type) { "data-protection-confirmation" } - let(:import_service) { instance_double(Imports::DataProtectionConfirmationImportService) } - let(:fixture_path) { "spec/fixtures/imports/data_protection_confirmations" } + it "creates a user from the given XML file" do + 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) - before do - allow(Imports::DataProtectionConfirmationImportService).to receive(:new).and_return(import_service) + task.invoke(type, fixture_path) + end end - it "creates an organisation from the given XML file" do - 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) + context "when importing data protection confirmation data" do + let(:type) { "data-protection-confirmation" } + let(:import_service) { instance_double(Imports::DataProtectionConfirmationImportService) } + let(:fixture_path) { "spec/fixtures/imports/data_protection_confirmations" } - task.invoke(type, fixture_path) - end - end + before do + allow(Imports::DataProtectionConfirmationImportService).to receive(:new).and_return(import_service) + end - context "when importing organisation rent period data" do - let(:type) { "organisation-rent-periods" } - let(:import_service) { instance_double(Imports::OrganisationRentPeriodImportService) } - let(:fixture_path) { "spec/fixtures/imports/organisation_rent_periods" } + it "creates an organisation from the given XML file" do + 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) - before do - allow(Imports::OrganisationRentPeriodImportService).to receive(:new).and_return(import_service) + task.invoke(type, fixture_path) + end end - it "creates an organisation la from the given XML file" do - 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) + context "when importing organisation rent period data" do + let(:type) { "organisation-rent-periods" } + let(:import_service) { instance_double(Imports::OrganisationRentPeriodImportService) } + let(:fixture_path) { "spec/fixtures/imports/organisation_rent_periods" } - task.invoke(type, fixture_path) - end - end + before do + allow(Imports::OrganisationRentPeriodImportService).to receive(:new).and_return(import_service) + end - context "when importing lettings logs" do - let(:type) { "lettings-logs" } - let(:import_service) { instance_double(Imports::LettingsLogsImportService) } - let(:fixture_path) { "spec/fixtures/imports/lettings_logs" } + it "creates an organisation la from the given XML file" do + 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) - before do - allow(Imports::LettingsLogsImportService).to receive(:new).and_return(import_service) + task.invoke(type, fixture_path) + end end - it "creates lettings logs from the given XML file" do - expect(Storage::S3Service).to receive(:new).with(paas_config_service, instance_name) - expect(Imports::LettingsLogsImportService).to receive(:new).with(storage_service) - expect(import_service).to receive(:create_logs).with(fixture_path) + context "when importing lettings logs" do + let(:type) { "lettings-logs" } + let(:import_service) { instance_double(Imports::LettingsLogsImportService) } + let(:fixture_path) { "spec/fixtures/imports/lettings_logs" } + + before do + allow(Imports::LettingsLogsImportService).to receive(:new).and_return(import_service) + end - task.invoke(type, fixture_path) + it "creates lettings logs from the given XML file" do + expect(Storage::S3Service).to receive(:new).with(paas_config_service, instance_name) + expect(Imports::LettingsLogsImportService).to receive(:new).with(storage_service) + expect(import_service).to receive(:create_logs).with(fixture_path) + + task.invoke(type, fixture_path) + end end - end - context "when importing sales logs" do - let(:type) { "sales-logs" } - let(:import_service) { instance_double(Imports::SalesLogsImportService) } - let(:fixture_path) { "spec/fixtures/imports/sales_logs" } + context "when importing sales logs" do + let(:type) { "sales-logs" } + let(:import_service) { instance_double(Imports::SalesLogsImportService) } + let(:fixture_path) { "spec/fixtures/imports/sales_logs" } - before do - allow(Imports::SalesLogsImportService).to receive(:new).and_return(import_service) + before do + allow(Imports::SalesLogsImportService).to receive(:new).and_return(import_service) + end + + it "creates sales logs from the given XML file" do + expect(Storage::S3Service).to receive(:new).with(paas_config_service, instance_name) + expect(Imports::SalesLogsImportService).to receive(:new).with(storage_service) + expect(import_service).to receive(:create_logs).with(fixture_path) + + task.invoke(type, fixture_path) + end end - it "creates sales logs from the given XML file" do - expect(Storage::S3Service).to receive(:new).with(paas_config_service, instance_name) - expect(Imports::SalesLogsImportService).to receive(:new).with(storage_service) - expect(import_service).to receive(:create_logs).with(fixture_path) + context "when importing scheme data" do + let(:type) { "scheme" } + let(:import_service) { instance_double(Imports::SchemeImportService) } + let(:fixture_path) { "spec/fixtures/imports/schemes" } + + before do + allow(Imports::SchemeImportService).to receive(:new).and_return(import_service) + end + + it "creates a scheme from the given XML file" do + 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) - task.invoke(type, fixture_path) + task.invoke(type, fixture_path) + end end - end - context "when importing scheme data" do - let(:type) { "scheme" } - let(:import_service) { instance_double(Imports::SchemeImportService) } - let(:fixture_path) { "spec/fixtures/imports/schemes" } + context "when importing scheme location data" do + let(:type) { "scheme-location" } + let(:import_service) { instance_double(Imports::SchemeLocationImportService) } + let(:fixture_path) { "spec/fixtures/imports/organisations" } - before do - allow(Imports::SchemeImportService).to receive(:new).and_return(import_service) + before do + allow(Imports::SchemeLocationImportService).to receive(:new).and_return(import_service) + end + + it "creates a scheme location from the given XML file" do + 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) + + task.invoke(type, fixture_path) + end end - it "creates a scheme from the given XML file" do - 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) + it "raises an exception if no parameters are provided" do + expect { task.invoke }.to raise_error(/Usage/) + end - task.invoke(type, fixture_path) + 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 - context "when importing scheme location data" do - let(:type) { "scheme-location" } - let(:import_service) { instance_double(Imports::SchemeLocationImportService) } - let(:fixture_path) { "spec/fixtures/imports/organisations" } + describe "core:data_protection_confirmation_import" do + subject(:task) { Rake::Task["core:data_protection_confirmation_import"] } + + let(:import_service) { instance_double(Imports::DataProtectionConfirmationImportService) } + # let(:fixture_path) { "spec/fixtures/imports/data_protection_confirmations" } + let(:storage_service) { instance_double(Storage::S3Service) } + let(:archive_service) { instance_double(Storage::ArchiveService) } + + let!(:organisation_without_dpc_with_old_org_id) { create(:organisation, :without_dpc, old_org_id: 1) } before do - allow(Imports::SchemeLocationImportService).to receive(:new).and_return(import_service) - end + Rake.application.rake_require("tasks/data_import") + Rake::Task.define_task(:environment) + task.reenable + + 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) + allow(Imports::DataProtectionConfirmationImportService).to receive(:new).and_return(import_service) - it "creates a scheme location from the given XML file" do - 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) + allow(Storage::ArchiveService).to receive(:new).and_return(archive_service) + allow(archive_service).to receive(:folder_present?).with("dataprotect").and_return(true) - task.invoke(type, fixture_path) + _org_without_old_org_id = create(:organisation, old_org_id: nil) + _organisation_without_dp = create(:organisation, :without_dpc, old_org_id: nil) 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 "creates an organisation from the given XML file" do + expect(Storage::S3Service).to receive(:new).with(paas_config_service, instance_name) + expect(storage_service).to receive(:get_file_io).with("#{organisation_without_dpc_with_old_org_id.old_org_id}.zip") + expect(Imports::DataProtectionConfirmationImportService).to receive(:new).with(archive_service) + expect(import_service).to receive(:create_data_protection_confirmations).with("dataprotect") - 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/) + task.invoke + end end end 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 400822462..eb386c117 100644 --- a/spec/services/imports/data_protection_confirmation_import_service_spec.rb +++ b/spec/services/imports/data_protection_confirmation_import_service_spec.rb @@ -23,7 +23,7 @@ RSpec.describe Imports::DataProtectionConfirmationImportService do context "when the organisation in the import file doesn't exist in the system" do it "does not create a data protection confirmation" do - expect(logger).to receive(:error).with(/Organisation must exist/) + expect(logger).to receive(:error).with("Organisation must exist") import_service.create_data_protection_confirmations("data_protection_directory") end end @@ -32,11 +32,12 @@ RSpec.describe Imports::DataProtectionConfirmationImportService do let!(:organisation) { create(:organisation, :without_dpc, old_org_id:) } context "when a data protection officer with matching name does not exists for the organisation" do - it "creates a data protection officer without sign in credentials" do + it "creates an inactive data protection officer" do expect { import_service.create_data_protection_confirmations("data_protection_directory") } .to change(User, :count).by(1) data_protection_officer = User.find_by(organisation:, is_dpo: true) - expect(data_protection_officer.email).to eq("") + expect(data_protection_officer.confirmed_at).not_to be_nil + expect(data_protection_officer.active).to be false end it "successfully create a data protection confirmation record with the expected data" do