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

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

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

spec/lib/tasks/data_import_spec.rb

@@ -1,177 +1,217 @@
 require "rails_helper"
 require "rake"
 
-describe "rake core:data_import", type: :task do
+describe "data import", type: :task do
-  subject(:task) { Rake::Task["core:data_import"] }
-
   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
+  describe "core:data_import" do
-    Rake.application.rake_require("tasks/data_import")
+    subject(:task) { Rake::Task["core: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" }
 
     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
+    context "when importing organisation data" do
-      expect(Storage::S3Service).to receive(:new).with(paas_config_service, instance_name)
+      let(:type) { "organisation" }
-      expect(Imports::OrganisationImportService).to receive(:new).with(storage_service)
+      let(:import_service) { instance_double(Imports::OrganisationImportService) }
-      expect(import_service).to receive(:create_organisations).with(fixture_path)
+      let(:fixture_path) { "spec/fixtures/imports/organisations" }
 
-      task.invoke(type, fixture_path)
+      before do
-    end
+        allow(Imports::OrganisationImportService).to receive(:new).and_return(import_service)
-  end
+      end
 
-  context "when importing user data" do
+      it "creates an organisation from the given XML file" do
-    let(:type) { "user" }
+        expect(Storage::S3Service).to receive(:new).with(paas_config_service, instance_name)
-    let(:import_service) { instance_double(Imports::UserImportService) }
+        expect(Imports::OrganisationImportService).to receive(:new).with(storage_service)
-    let(:fixture_path) { "spec/fixtures/imports/users" }
+        expect(import_service).to receive(:create_organisations).with(fixture_path)
 
-    before do
+        task.invoke(type, fixture_path)
-      allow(Imports::UserImportService).to receive(:new).and_return(import_service)
+      end
     end
 
-    it "creates a user from the given XML file" do
+    context "when importing user data" do
-      expect(Storage::S3Service).to receive(:new).with(paas_config_service, instance_name)
+      let(:type) { "user" }
-      expect(Imports::UserImportService).to receive(:new).with(storage_service)
+      let(:import_service) { instance_double(Imports::UserImportService) }
-      expect(import_service).to receive(:create_users).with(fixture_path)
+      let(:fixture_path) { "spec/fixtures/imports/users" }
 
-      task.invoke(type, fixture_path)
+      before do
-    end
+        allow(Imports::UserImportService).to receive(:new).and_return(import_service)
-  end
+      end
 
-  context "when importing data protection confirmation data" do
+      it "creates a user from the given XML file" do
-    let(:type) { "data-protection-confirmation" }
+        expect(Storage::S3Service).to receive(:new).with(paas_config_service, instance_name)
-    let(:import_service) { instance_double(Imports::DataProtectionConfirmationImportService) }
+        expect(Imports::UserImportService).to receive(:new).with(storage_service)
-    let(:fixture_path) { "spec/fixtures/imports/data_protection_confirmations" }
+        expect(import_service).to receive(:create_users).with(fixture_path)
 
-    before do
+        task.invoke(type, fixture_path)
-      allow(Imports::DataProtectionConfirmationImportService).to receive(:new).and_return(import_service)
+      end
     end
 
-    it "creates an organisation from the given XML file" do
+    context "when importing data protection confirmation data" do
-      expect(Storage::S3Service).to receive(:new).with(paas_config_service, instance_name)
+      let(:type) { "data-protection-confirmation" }
-      expect(Imports::DataProtectionConfirmationImportService).to receive(:new).with(storage_service)
+      let(:import_service) { instance_double(Imports::DataProtectionConfirmationImportService) }
-      expect(import_service).to receive(:create_data_protection_confirmations).with(fixture_path)
+      let(:fixture_path) { "spec/fixtures/imports/data_protection_confirmations" }
 
-      task.invoke(type, fixture_path)
+      before do
-    end
+        allow(Imports::DataProtectionConfirmationImportService).to receive(:new).and_return(import_service)
-  end
+      end
 
-  context "when importing organisation rent period data" do
+      it "creates an organisation from the given XML file" do
-    let(:type) { "organisation-rent-periods" }
+        expect(Storage::S3Service).to receive(:new).with(paas_config_service, instance_name)
-    let(:import_service) { instance_double(Imports::OrganisationRentPeriodImportService) }
+        expect(Imports::DataProtectionConfirmationImportService).to receive(:new).with(storage_service)
-    let(:fixture_path) { "spec/fixtures/imports/organisation_rent_periods" }
+        expect(import_service).to receive(:create_data_protection_confirmations).with(fixture_path)
 
-    before do
+        task.invoke(type, fixture_path)
-      allow(Imports::OrganisationRentPeriodImportService).to receive(:new).and_return(import_service)
+      end
     end
 
-    it "creates an organisation la from the given XML file" do
+    context "when importing organisation rent period data" do
-      expect(Storage::S3Service).to receive(:new).with(paas_config_service, instance_name)
+      let(:type) { "organisation-rent-periods" }
-      expect(Imports::OrganisationRentPeriodImportService).to receive(:new).with(storage_service)
+      let(:import_service) { instance_double(Imports::OrganisationRentPeriodImportService) }
-      expect(import_service).to receive(:create_organisation_rent_periods).with(fixture_path)
+      let(:fixture_path) { "spec/fixtures/imports/organisation_rent_periods" }
 
-      task.invoke(type, fixture_path)
+      before do
-    end
+        allow(Imports::OrganisationRentPeriodImportService).to receive(:new).and_return(import_service)
-  end
+      end
 
-  context "when importing lettings logs" do
+      it "creates an organisation la from the given XML file" do
-    let(:type) { "lettings-logs" }
+        expect(Storage::S3Service).to receive(:new).with(paas_config_service, instance_name)
-    let(:import_service) { instance_double(Imports::LettingsLogsImportService) }
+        expect(Imports::OrganisationRentPeriodImportService).to receive(:new).with(storage_service)
-    let(:fixture_path) { "spec/fixtures/imports/lettings_logs" }
+        expect(import_service).to receive(:create_organisation_rent_periods).with(fixture_path)
 
-    before do
+        task.invoke(type, fixture_path)
-      allow(Imports::LettingsLogsImportService).to receive(:new).and_return(import_service)
+      end
     end
 
-    it "creates lettings logs from the given XML file" do
+    context "when importing lettings logs" do
-      expect(Storage::S3Service).to receive(:new).with(paas_config_service, instance_name)
+      let(:type) { "lettings-logs" }
-      expect(Imports::LettingsLogsImportService).to receive(:new).with(storage_service)
+      let(:import_service) { instance_double(Imports::LettingsLogsImportService) }
-      expect(import_service).to receive(:create_logs).with(fixture_path)
+      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
+    context "when importing sales logs" do
-    let(:type) { "sales-logs" }
+      let(:type) { "sales-logs" }
-    let(:import_service) { instance_double(Imports::SalesLogsImportService) }
+      let(:import_service) { instance_double(Imports::SalesLogsImportService) }
-    let(:fixture_path) { "spec/fixtures/imports/sales_logs" }
+      let(:fixture_path) { "spec/fixtures/imports/sales_logs" }
 
-    before do
+      before do
-      allow(Imports::SalesLogsImportService).to receive(:new).and_return(import_service)
+        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
+    context "when importing scheme data" do
-      expect(Storage::S3Service).to receive(:new).with(paas_config_service, instance_name)
+      let(:type) { "scheme" }
-      expect(Imports::SalesLogsImportService).to receive(:new).with(storage_service)
+      let(:import_service) { instance_double(Imports::SchemeImportService) }
-      expect(import_service).to receive(:create_logs).with(fixture_path)
+      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
+    context "when importing scheme location data" do
-    let(:type) { "scheme" }
+      let(:type) { "scheme-location" }
-    let(:import_service) { instance_double(Imports::SchemeImportService) }
+      let(:import_service) { instance_double(Imports::SchemeLocationImportService) }
-    let(:fixture_path) { "spec/fixtures/imports/schemes" }
+      let(:fixture_path) { "spec/fixtures/imports/organisations" }
 
-    before do
+      before do
-      allow(Imports::SchemeImportService).to receive(:new).and_return(import_service)
+        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
+    it "raises an exception if no parameters are provided" do
-      expect(Storage::S3Service).to receive(:new).with(paas_config_service, instance_name)
+      expect { task.invoke }.to raise_error(/Usage/)
-      expect(Imports::SchemeImportService).to receive(:new).with(storage_service)
+    end
-      expect(import_service).to receive(:create_schemes).with(fixture_path)
 
-      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
+  describe "core:data_protection_confirmation_import" do
-    let(:type) { "scheme-location" }
+    subject(:task) { Rake::Task["core:data_protection_confirmation_import"] }
-    let(:import_service) { instance_double(Imports::SchemeLocationImportService) }
+
-    let(:fixture_path) { "spec/fixtures/imports/organisations" }
+    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)
+      Rake.application.rake_require("tasks/data_import")
-    end
+      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
+      allow(Storage::ArchiveService).to receive(:new).and_return(archive_service)
-      expect(Storage::S3Service).to receive(:new).with(paas_config_service, instance_name)
+      allow(archive_service).to receive(:folder_present?).with("dataprotect").and_return(true)
-      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)
+      _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
+    it "creates an organisation from the given XML file" do
-    expect { task.invoke("one_parameter") }.to raise_error(/Usage/)
+      expect(Storage::S3Service).to receive(:new).with(paas_config_service, instance_name)
-  end
+      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
+      task.invoke
-    expect { task.invoke("unknown_type", "my_path") }.to raise_error(/Type unknown_type is not supported/)
+    end
   end
 end

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