From a1095f95269e65f5ad0798221c75e6a1fd0241a8 Mon Sep 17 00:00:00 2001 From: Jack <113976590+bibblobcode@users.noreply.github.com> Date: Tue, 11 Jul 2023 14:43:39 +0200 Subject: [PATCH] CLDC-2490 Tidy up DSA model (#1766) * Remove new_data_protection_confirmation? flag * Update schema - drop index - add org and user data to dpc table * Persist org and user data at import * Do not show invalid emails * Persist user and org data while signing agreement * Add migration to persist org and user data on DPC * Rebase fix * fix typo --- .../create_log_actions_component.rb | 1 - ...rotection_confirmation_banner_component.rb | 1 - .../bulk_upload_lettings_logs_controller.rb | 1 - .../bulk_upload_sales_logs_controller.rb | 1 - app/controllers/organisations_controller.rb | 18 +++- app/helpers/data_sharing_agreement_helper.rb | 6 +- app/models/organisation.rb | 10 +- app/models/user.rb | 1 - app/models/validations/setup_validations.rb | 2 - app/models/validations/shared_validations.rb | 2 - app/services/feature_toggle.rb | 4 - ..._protection_confirmation_import_service.rb | 6 ++ .../logs/_create_for_org_actions.html.erb | 2 +- app/views/organisations/show.html.erb | 4 +- ...on_info_on_data_protection_confirmation.rb | 12 +++ ...ta_protection_confirmation_unique_index.rb | 14 +++ db/schema.rb | 9 +- lib/tasks/data_import.rake | 34 +++--- .../create_log_actions_component_spec.rb | 71 ++---------- ...tion_confirmation_banner_component_spec.rb | 54 ++++++++-- spec/lib/tasks/data_import_spec.rb | 38 ------- spec/models/organisation_spec.rb | 54 +++------- spec/models/user_spec.rb | 10 -- .../validations/setup_validations_spec.rb | 28 ----- .../validations/shared_validations_spec.rb | 28 ----- ...lk_upload_lettings_logs_controller_spec.rb | 12 --- .../bulk_upload_sales_logs_controller_spec.rb | 12 --- .../requests/organisations_controller_spec.rb | 102 +++++++----------- ...ection_confirmation_import_service_spec.rb | 20 +++- .../_create_for_org_actions.html.erb_spec.rb | 35 ++---- .../data_sharing_agreement.html.erb_spec.rb | 44 ++++++++ .../views/organisations/show.html.erb_spec.rb | 14 --- 32 files changed, 252 insertions(+), 398 deletions(-) create mode 100644 db/migrate/20230710100120_persist_user_and_organisation_info_on_data_protection_confirmation.rb create mode 100644 db/migrate/20230710101532_remove_data_protection_confirmation_unique_index.rb diff --git a/app/components/create_log_actions_component.rb b/app/components/create_log_actions_component.rb index 55795c94a..c61a85b80 100644 --- a/app/components/create_log_actions_component.rb +++ b/app/components/create_log_actions_component.rb @@ -13,7 +13,6 @@ class CreateLogActionsComponent < ViewComponent::Base def display_actions? return false if bulk_upload.present? - return true unless FeatureToggle.new_data_protection_confirmation? return true if user.support? user.organisation.data_protection_confirmed? diff --git a/app/components/data_protection_confirmation_banner_component.rb b/app/components/data_protection_confirmation_banner_component.rb index ed5fd1310..5757ad87f 100644 --- a/app/components/data_protection_confirmation_banner_component.rb +++ b/app/components/data_protection_confirmation_banner_component.rb @@ -13,7 +13,6 @@ class DataProtectionConfirmationBannerComponent < ViewComponent::Base end def display_banner? - return false unless FeatureToggle.new_data_protection_confirmation? return false if user.support? && organisation.blank? return true if org_without_dpo? diff --git a/app/controllers/bulk_upload_lettings_logs_controller.rb b/app/controllers/bulk_upload_lettings_logs_controller.rb index 035621e41..c9cdb0eaa 100644 --- a/app/controllers/bulk_upload_lettings_logs_controller.rb +++ b/app/controllers/bulk_upload_lettings_logs_controller.rb @@ -25,7 +25,6 @@ class BulkUploadLettingsLogsController < ApplicationController private def validate_data_protection_agrement_signed! - return unless FeatureToggle.new_data_protection_confirmation? return if @current_user.organisation.data_protection_confirmed? redirect_to lettings_logs_path diff --git a/app/controllers/bulk_upload_sales_logs_controller.rb b/app/controllers/bulk_upload_sales_logs_controller.rb index 7a7a20297..de70ffc5d 100644 --- a/app/controllers/bulk_upload_sales_logs_controller.rb +++ b/app/controllers/bulk_upload_sales_logs_controller.rb @@ -25,7 +25,6 @@ class BulkUploadSalesLogsController < ApplicationController private def validate_data_protection_agrement_signed! - return unless FeatureToggle.new_data_protection_confirmation? return if @current_user.organisation.data_protection_confirmed? redirect_to sales_logs_path diff --git a/app/controllers/organisations_controller.rb b/app/controllers/organisations_controller.rb index 98082bea9..53052dd1c 100644 --- a/app/controllers/organisations_controller.rb +++ b/app/controllers/organisations_controller.rb @@ -159,13 +159,10 @@ class OrganisationsController < ApplicationController end def data_sharing_agreement - return render_not_found unless FeatureToggle.new_data_protection_confirmation? - @data_protection_confirmation = current_user.organisation.data_protection_confirmation end def confirm_data_sharing_agreement - return render_not_found unless FeatureToggle.new_data_protection_confirmation? return render_not_found unless current_user.is_dpo? return render_not_found if @organisation.data_protection_confirmed? @@ -173,14 +170,25 @@ class OrganisationsController < ApplicationController @organisation.data_protection_confirmation.update!( confirmed: true, data_protection_officer: current_user, - # When it was signed created_at: Time.zone.now, + signed_at: Time.zone.now, + organisation_name: @organisation.name, + organisation_address: @organisation.address_row, + organisation_phone_number: @organisation.phone, + data_protection_officer_email: current_user.email, + data_protection_officer_name: current_user.name, ) else DataProtectionConfirmation.create!( - organisation: current_user.organisation, + organisation: @organisation, confirmed: true, data_protection_officer: current_user, + signed_at: Time.zone.now, + organisation_name: @organisation.name, + organisation_address: @organisation.address_row, + organisation_phone_number: @organisation.phone, + data_protection_officer_email: current_user.email, + data_protection_officer_name: current_user.name, ) end diff --git a/app/helpers/data_sharing_agreement_helper.rb b/app/helpers/data_sharing_agreement_helper.rb index e79819ffb..c6a088461 100644 --- a/app/helpers/data_sharing_agreement_helper.rb +++ b/app/helpers/data_sharing_agreement_helper.rb @@ -61,7 +61,11 @@ module DataSharingAgreementHelper end end - "12.2. For #{@org_name}: Name: #{@dpo_name}, Postal Address: #{@org_address}, E-mail address: #{@dpo_email}, Telephone number: #{@org_phone}" + if data_protection_confirmation&.confirmed? && @dpo_email.exclude?("@") # Do not show invalid email addresses + "12.2. For #{@org_name}: Name: #{@dpo_name}, Postal Address: #{@org_address}, Telephone number: #{@org_phone}" + else + "12.2. For #{@org_name}: Name: #{@dpo_name}, Postal Address: #{@org_address}, E-mail address: #{@dpo_email}, Telephone number: #{@org_phone}" + end end # rubocop:enable Rails/HelperInstanceVariable diff --git a/app/models/organisation.rb b/app/models/organisation.rb index 69aa59c97..ad74ac3e6 100644 --- a/app/models/organisation.rb +++ b/app/models/organisation.rb @@ -103,7 +103,7 @@ class Organisation < ApplicationRecord end def display_organisation_attributes - attrs = [ + [ { name: "Name", value: name, editable: true }, { name: "Organisation ID", value: "ORG#{id}", editable: false }, { name: "Address", value: address_string, editable: true }, @@ -112,13 +112,7 @@ class Organisation < ApplicationRecord { name: "Registration number", value: housing_registration_no || "", editable: false }, { name: "Rent periods", value: rent_period_labels, editable: false, format: :bullet }, { name: "Owns housing stock", value: holds_own_stock ? "Yes" : "No", editable: false }, - ].compact - - unless FeatureToggle.new_data_protection_confirmation? - attrs << { name: "Data protection agreement", value: data_protection_agreement_string, editable: false } - end - - attrs + ] end def has_managing_agents? diff --git a/app/models/user.rb b/app/models/user.rb index c9ef7cd02..2cf45de45 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -186,7 +186,6 @@ protected private def send_data_protection_confirmation_reminder - return unless FeatureToggle.new_data_protection_confirmation? return unless persisted? return unless is_dpo? diff --git a/app/models/validations/setup_validations.rb b/app/models/validations/setup_validations.rb index bb76a7668..04fb2a2d7 100644 --- a/app/models/validations/setup_validations.rb +++ b/app/models/validations/setup_validations.rb @@ -49,8 +49,6 @@ module Validations::SetupValidations end def validate_managing_organisation_data_sharing_agremeent_signed(record) - return unless FeatureToggle.new_data_protection_confirmation? - if record.managing_organisation_id_changed? && record.managing_organisation.present? && !record.managing_organisation.data_protection_confirmed? record.errors.add :managing_organisation_id, I18n.t("validations.setup.managing_organisation.data_sharing_agreement_not_signed") end diff --git a/app/models/validations/shared_validations.rb b/app/models/validations/shared_validations.rb index 3b361e9d6..f9cd25e46 100644 --- a/app/models/validations/shared_validations.rb +++ b/app/models/validations/shared_validations.rb @@ -118,8 +118,6 @@ module Validations::SharedValidations end def validate_owning_organisation_data_sharing_agremeent_signed(record) - return unless FeatureToggle.new_data_protection_confirmation? - if record.owning_organisation_id_changed? && record.owning_organisation.present? && !record.owning_organisation.data_protection_confirmed? record.errors.add :owning_organisation_id, I18n.t("validations.setup.owning_organisation.data_sharing_agreement_not_signed") end diff --git a/app/services/feature_toggle.rb b/app/services/feature_toggle.rb index 113834b0f..d7df829ad 100644 --- a/app/services/feature_toggle.rb +++ b/app/services/feature_toggle.rb @@ -30,10 +30,6 @@ class FeatureToggle !Rails.env.production? end - def self.new_data_protection_confirmation? - true - end - def self.deduplication_flow_enabled? !Rails.env.production? && !Rails.env.staging? end diff --git a/app/services/imports/data_protection_confirmation_import_service.rb b/app/services/imports/data_protection_confirmation_import_service.rb index e2694ecba..abf187f3c 100644 --- a/app/services/imports/data_protection_confirmation_import_service.rb +++ b/app/services/imports/data_protection_confirmation_import_service.rb @@ -38,6 +38,12 @@ module Imports old_id: record_field_value(xml_document, "id"), old_org_id: record_field_value(xml_document, "institution"), created_at: record_field_value(xml_document, "change-date").to_time(:utc), + signed_at: record_field_value(xml_document, "change-date").to_time(:utc), + organisation_name: org.name, + organisation_address: org.address_row, + organisation_phone_number: org.phone, + data_protection_officer_email: dp_officer.email, + data_protection_officer_name: dp_officer.name, ) end diff --git a/app/views/logs/_create_for_org_actions.html.erb b/app/views/logs/_create_for_org_actions.html.erb index dc0469c44..f6ed9ad82 100644 --- a/app/views/logs/_create_for_org_actions.html.erb +++ b/app/views/logs/_create_for_org_actions.html.erb @@ -1,5 +1,5 @@
- <% if !FeatureToggle.new_data_protection_confirmation? || @organisation.data_protection_confirmed? %> + <% if @organisation.data_protection_confirmed? %> <% if current_page?(controller: 'organisations', action: 'lettings_logs') %> <%= govuk_button_to "Create a new lettings log for this organisation", lettings_logs_path(lettings_log: { owning_organisation_id: @organisation.id }, method: :post) %> <% end %> diff --git a/app/views/organisations/show.html.erb b/app/views/organisations/show.html.erb index e39338651..07af1970a 100644 --- a/app/views/organisations/show.html.erb +++ b/app/views/organisations/show.html.erb @@ -33,9 +33,7 @@ <% end %> <% end %> <% end %> - <% if FeatureToggle.new_data_protection_confirmation? %> - <%= data_sharing_agreement_row(organisation: @organisation, user: current_user, summary_list:) %> - <% end %> + <%= data_sharing_agreement_row(organisation: @organisation, user: current_user, summary_list:) %> <% end %> <% if FeatureToggle.merge_organisations_enabled? %>

Is your organisation merging with another? <%= govuk_link_to "Let us know using this form", merge_request_organisation_path(@organisation) %>

diff --git a/db/migrate/20230710100120_persist_user_and_organisation_info_on_data_protection_confirmation.rb b/db/migrate/20230710100120_persist_user_and_organisation_info_on_data_protection_confirmation.rb new file mode 100644 index 000000000..c721d4b62 --- /dev/null +++ b/db/migrate/20230710100120_persist_user_and_organisation_info_on_data_protection_confirmation.rb @@ -0,0 +1,12 @@ +class PersistUserAndOrganisationInfoOnDataProtectionConfirmation < ActiveRecord::Migration[7.0] + def change + change_table :data_protection_confirmations, bulk: true do |t| + t.column :signed_at, :datetime + t.column :organisation_name, :string + t.column :organisation_address, :string + t.column :organisation_phone_number, :string + t.column :data_protection_officer_email, :string + t.column :data_protection_officer_name, :string + end + end +end diff --git a/db/migrate/20230710101532_remove_data_protection_confirmation_unique_index.rb b/db/migrate/20230710101532_remove_data_protection_confirmation_unique_index.rb new file mode 100644 index 000000000..5b6203730 --- /dev/null +++ b/db/migrate/20230710101532_remove_data_protection_confirmation_unique_index.rb @@ -0,0 +1,14 @@ +class RemoveDataProtectionConfirmationUniqueIndex < ActiveRecord::Migration[7.0] + def up + remove_index :data_protection_confirmations, + %i[organisation_id data_protection_officer_id confirmed], + unique: true + end + + def down + add_index :data_protection_confirmations, + %i[organisation_id data_protection_officer_id confirmed], + unique: true, + name: "data_protection_confirmations_unique" + end +end diff --git a/db/schema.rb b/db/schema.rb index 3a47ae8aa..111396ef3 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.0].define(version: 2023_06_29_125541) do +ActiveRecord::Schema[7.0].define(version: 2023_07_10_101532) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -52,8 +52,13 @@ ActiveRecord::Schema[7.0].define(version: 2023_06_29_125541) do t.string "old_org_id" t.datetime "created_at", null: false t.datetime "updated_at", null: false + t.datetime "signed_at" + t.string "organisation_name" + t.string "organisation_address" + t.string "organisation_phone_number" + t.string "data_protection_officer_email" + t.string "data_protection_officer_name" t.index ["data_protection_officer_id"], name: "dpo_user_id" - t.index ["organisation_id", "data_protection_officer_id", "confirmed"], name: "data_protection_confirmations_unique", unique: true t.index ["organisation_id"], name: "index_data_protection_confirmations_on_organisation_id" end diff --git a/lib/tasks/data_import.rake b/lib/tasks/data_import.rake index ea19d96b5..6e0fdd4b9 100644 --- a/lib/tasks/data_import.rake +++ b/lib/tasks/data_import.rake @@ -29,26 +29,20 @@ namespace :core do 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 + desc "Persist user and org data on data sharing confirmations" + task persist_user_and_org_data_on_data_sharing_confirmations: :environment do |_task| + DataProtectionConfirmation.all.includes(:data_protection_officer, :organisation).each do |dpc| + dpc.update!( + organisation_name: dpc.organisation.name, + organisation_address: dpc.organisation.address_row, + signed_at: dpc.created_at, + organisation_phone_number: dpc.organisation.phone, + data_protection_officer_email: dpc.data_protection_officer.email, + data_protection_officer_name: dpc.data_protection_officer.name, + ) + print "." end + + puts "done" end end diff --git a/spec/components/create_log_actions_component_spec.rb b/spec/components/create_log_actions_component_spec.rb index 3ae7851eb..a52f851f0 100644 --- a/spec/components/create_log_actions_component_spec.rb +++ b/spec/components/create_log_actions_component_spec.rb @@ -20,10 +20,8 @@ RSpec.describe CreateLogActionsComponent, type: :component do context "when bulk upload nil" do let(:bulk_upload) { nil } - context "when flag disabled" do - before do - allow(FeatureToggle).to receive(:new_data_protection_confirmation?).and_return(false) - end + context "when support user" do + let(:user) { create(:user, :support) } it "renders actions" do expect(component.display_actions?).to eq(true) @@ -65,12 +63,16 @@ RSpec.describe CreateLogActionsComponent, type: :component do end end - context "when flag enabled" do - before do - allow(FeatureToggle).to receive(:new_data_protection_confirmation?).and_return(true) + context "when not support user" do + context "without data sharing agreement" do + let(:user) { create(:user, organisation: create(:organisation, :without_dpc)) } + + it "does not render actions" do + expect(component).not_to be_display_actions + end end - context "when support user" do + context "when has data sharing agremeent" do let(:user) { create(:user, :support) } it "renders actions" do @@ -112,59 +114,6 @@ RSpec.describe CreateLogActionsComponent, type: :component do end end end - - context "when not support user" do - context "without data sharing agreement" do - let(:user) { create(:user, organisation: create(:organisation, :without_dpc)) } - - it "does not render actions" do - expect(component).not_to be_display_actions - end - end - - context "when has data sharing agremeent" do - let(:user) { create(:user, :support) } - - it "renders actions" do - expect(component.display_actions?).to eq(true) - end - - it "returns create button copy" do - expect(component.create_button_copy).to eq("Create a new lettings log") - end - - it "returns create button href" do - render - expect(component.create_button_href).to eq("/lettings-logs") - end - - it "returns upload button copy" do - expect(component.upload_button_copy).to eq("Upload lettings logs in bulk") - end - - it "returns upload button href" do - render - expect(component.upload_button_href).to eq("/lettings-logs/bulk-upload-logs/start") - end - - context "when sales log type" do - let(:log_type) { "sales" } - - it "renders actions" do - expect(component.display_actions?).to eq(true) - end - - it "returns create button copy" do - expect(component.create_button_copy).to eq("Create a new sales log") - end - - it "returns create button href" do - render - expect(component.create_button_href).to eq("/sales-logs") - end - end - end - end end end end diff --git a/spec/components/data_protection_confirmation_banner_component_spec.rb b/spec/components/data_protection_confirmation_banner_component_spec.rb index c78724aa3..be10f9689 100644 --- a/spec/components/data_protection_confirmation_banner_component_spec.rb +++ b/spec/components/data_protection_confirmation_banner_component_spec.rb @@ -8,10 +8,9 @@ RSpec.describe DataProtectionConfirmationBannerComponent, type: :component do let(:user) { create(:user) } let(:organisation) { user.organisation } - context "when flag disabled" do - before do - allow(FeatureToggle).to receive(:new_data_protection_confirmation?).and_return(false) - end + context "when user is support and organisation is blank" do + let(:user) { create(:user, :support) } + let(:organisation) { nil } it "does not display banner" do expect(component.display_banner?).to eq(false) @@ -19,15 +18,52 @@ RSpec.describe DataProtectionConfirmationBannerComponent, type: :component do end end - context "when flag enabled", :aggregate_failures do + context "when org does not have a DPO" do before do - allow(FeatureToggle).to receive(:new_data_protection_confirmation?).and_return(true) + organisation.users.where(is_dpo: true).destroy_all end - context "when user is support and organisation is blank" do - let(:user) { create(:user, :support) } - let(:organisation) { nil } + it "displays the banner" do + expect(component.display_banner?).to eq(true) + expect(render).to have_link( + "Contact helpdesk to assign a data protection officer", + href: "https://dluhcdigital.atlassian.net/servicedesk/customer/portal/6/group/11", + ) + expect(render).to have_selector("p", text: "To create logs your organisation must state a data protection officer. They must sign the Data Sharing Agreement.") + end + end + + context "when org has a DPO" do + context "when org does not have a signed data sharing agreement" do + context "when user is not a DPO" do + let(:organisation) { create(:organisation, :without_dpc) } + let(:user) { create(:user, organisation:) } + let!(:dpo) { create(:user, :data_protection_officer, organisation:) } + + it "displays the banner and shows DPOs" do + expect(component.display_banner?).to eq(true) + expect(render.css("a")).to be_empty + expect(render).to have_selector("p", text: "Your data protection officer must accept the Data Sharing Agreement on CORE before you can create any logs.") + expect(render).to have_selector("p", text: "You can ask: #{dpo.name}") + end + end + + context "when user is a DPO" do + let(:organisation) { create(:organisation, :without_dpc) } + let(:user) { create(:user, :data_protection_officer, organisation:) } + + it "displays the banner and asks to sign" do + expect(component.display_banner?).to eq(true) + expect(render).to have_link( + "Read the Data Sharing Agreement", + href: "/organisations/#{organisation.id}/data-sharing-agreement", + ) + expect(render).to have_selector("p", text: "Your organisation must accept the Data Sharing Agreement before you can create any logs.") + end + end + end + context "when org has a signed data sharing agremeent" do it "does not display banner" do expect(component.display_banner?).to eq(false) expect(render.content).to be_empty diff --git a/spec/lib/tasks/data_import_spec.rb b/spec/lib/tasks/data_import_spec.rb index c251477b9..b8b56801a 100644 --- a/spec/lib/tasks/data_import_spec.rb +++ b/spec/lib/tasks/data_import_spec.rb @@ -176,42 +176,4 @@ describe "data import", type: :task do expect { task.invoke("unknown_type", "my_path") }.to raise_error(/Type unknown_type is not supported/) end end - - 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 - 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) - - allow(Storage::ArchiveService).to receive(:new).and_return(archive_service) - allow(archive_service).to receive(:folder_present?).with("dataprotect").and_return(true) - - _org_without_old_org_id = create(:organisation, old_org_id: nil) - _organisation_without_dp = create(:organisation, :without_dpc, old_org_id: nil) - 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") - - task.invoke - end - end end diff --git a/spec/models/organisation_spec.rb b/spec/models/organisation_spec.rb index 1b0567a0f..0cdfc014e 100644 --- a/spec/models/organisation_spec.rb +++ b/spec/models/organisation_spec.rb @@ -234,47 +234,19 @@ RSpec.describe Organisation, type: :model do describe "display_organisation_attributes" do let(:organisation) { create(:organisation) } - context "when new_data_protection_confirmation flag enabled" do - before do - allow(FeatureToggle).to receive(:new_data_protection_confirmation?).and_return(true) - end - - it "does not include data protection agreement" do - expect(organisation.display_organisation_attributes).to eq( - [{ editable: true, name: "Name", value: "DLUHC" }, - { editable: false, name: "Organisation ID", value: "ORG#{organisation.id}" }, - { editable: true, - name: "Address", - value: "2 Marsham Street\nLondon\nSW1P 4DF" }, - { editable: true, name: "Telephone number", value: nil }, - { editable: false, name: "Type of provider", value: "Local authority" }, - { editable: false, name: "Registration number", value: "1234" }, - { editable: false, format: :bullet, name: "Rent periods", value: %w[All] }, - { editable: false, name: "Owns housing stock", value: "Yes" }], - ) - end - end - - context "when new_data_protection_confirmation flag disabled" do - before do - allow(FeatureToggle).to receive(:new_data_protection_confirmation?).and_return(false) - end - - it "includes data protection agreement" do - expect(organisation.display_organisation_attributes).to eq( - [{ editable: true, name: "Name", value: "DLUHC" }, - { editable: false, name: "Organisation ID", value: "ORG#{organisation.id}" }, - { editable: true, - name: "Address", - value: "2 Marsham Street\nLondon\nSW1P 4DF" }, - { editable: true, name: "Telephone number", value: nil }, - { editable: false, name: "Type of provider", value: "Local authority" }, - { editable: false, name: "Registration number", value: "1234" }, - { editable: false, format: :bullet, name: "Rent periods", value: %w[All] }, - { editable: false, name: "Owns housing stock", value: "Yes" }, - { editable: false, name: "Data protection agreement", value: "Accepted" }], - ) - end + it "does not include data protection agreement" do + expect(organisation.display_organisation_attributes).to eq( + [{ editable: true, name: "Name", value: "DLUHC" }, + { editable: false, name: "Organisation ID", value: "ORG#{organisation.id}" }, + { editable: true, + name: "Address", + value: "2 Marsham Street\nLondon\nSW1P 4DF" }, + { editable: true, name: "Telephone number", value: nil }, + { editable: false, name: "Type of provider", value: "Local authority" }, + { editable: false, name: "Registration number", value: "1234" }, + { editable: false, format: :bullet, name: "Rent periods", value: %w[All] }, + { editable: false, name: "Owns housing stock", value: "Yes" }], + ) end end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 69c5f8a7e..b9fb91c06 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -382,16 +382,6 @@ RSpec.describe User, type: :model do args: [user], ) end - - context "when feature flag disabled" do - before do - allow(FeatureToggle).to receive(:new_data_protection_confirmation?).and_return(false) - end - - it "does not send the email" do - expect { user.update!(is_dpo: true) }.not_to enqueue_job(ActionMailer::MailDeliveryJob) - end - end end context "when updating to non dpo" do diff --git a/spec/models/validations/setup_validations_spec.rb b/spec/models/validations/setup_validations_spec.rb index 696667b43..20f6b1d25 100644 --- a/spec/models/validations/setup_validations_spec.rb +++ b/spec/models/validations/setup_validations_spec.rb @@ -444,34 +444,6 @@ RSpec.describe Validations::SetupValidations do end describe "#validate_managing_organisation_data_sharing_agremeent_signed" do - before do - allow(FeatureToggle).to receive(:new_data_protection_confirmation?).and_return(false) - end - - it "is valid if the DSA is signed" do - log = build(:lettings_log, :in_progress, owning_organisation: create(:organisation)) - - expect(log).to be_valid - end - - it "is valid when owning_organisation nil" do - log = build(:lettings_log, owning_organisation: nil) - - expect(log).to be_valid - end - - it "is not valid if the DSA is not signed" do - log = build(:lettings_log, owning_organisation: create(:organisation, :without_dpc)) - - expect(log).to be_valid - end - end - - context "when flag enabled" do - before do - allow(FeatureToggle).to receive(:new_data_protection_confirmation?).and_return(true) - end - it "is valid if the Data Protection Confirmation is signed" do log = build(:lettings_log, :in_progress, managing_organisation: create(:organisation)) diff --git a/spec/models/validations/shared_validations_spec.rb b/spec/models/validations/shared_validations_spec.rb index 8751f443a..52b325c29 100644 --- a/spec/models/validations/shared_validations_spec.rb +++ b/spec/models/validations/shared_validations_spec.rb @@ -177,34 +177,6 @@ RSpec.describe Validations::SharedValidations do %i[sales_log lettings_log].each do |log_type| describe "validate_owning_organisation_data_sharing_agremeent_signed" do - before do - allow(FeatureToggle).to receive(:new_data_protection_confirmation?).and_return(false) - end - - it "is valid if the DSA is signed" do - log = build(log_type, :in_progress, owning_organisation: create(:organisation)) - - expect(log).to be_valid - end - - it "is valid when owning_organisation nil" do - log = build(log_type, owning_organisation: nil) - - expect(log).to be_valid - end - - it "is not valid if the DSA is not signed" do - log = build(log_type, owning_organisation: create(:organisation, :without_dpc)) - - expect(log).to be_valid - end - end - - context "when flag enabled" do - before do - allow(FeatureToggle).to receive(:new_data_protection_confirmation?).and_return(true) - end - it "is valid if the Data Protection Confirmation is signed" do log = build(log_type, :in_progress, owning_organisation: create(:organisation)) diff --git a/spec/requests/bulk_upload_lettings_logs_controller_spec.rb b/spec/requests/bulk_upload_lettings_logs_controller_spec.rb index db5a3c4a6..f901cdb7e 100644 --- a/spec/requests/bulk_upload_lettings_logs_controller_spec.rb +++ b/spec/requests/bulk_upload_lettings_logs_controller_spec.rb @@ -18,18 +18,6 @@ RSpec.describe BulkUploadLettingsLogsController, type: :request do expect(response).to redirect_to("/lettings-logs") end - - context "when feature flag disabled" do - before do - allow(FeatureToggle).to receive(:new_data_protection_confirmation?).and_return(false) - end - - it "does not redirect to lettings index page" do - get "/lettings-logs/bulk-upload-logs/start", params: {} - - expect(response).not_to redirect_to("/lettings-logs") - end - end end context "when not in crossover period" do diff --git a/spec/requests/bulk_upload_sales_logs_controller_spec.rb b/spec/requests/bulk_upload_sales_logs_controller_spec.rb index c7de6598e..3220ff885 100644 --- a/spec/requests/bulk_upload_sales_logs_controller_spec.rb +++ b/spec/requests/bulk_upload_sales_logs_controller_spec.rb @@ -18,18 +18,6 @@ RSpec.describe BulkUploadSalesLogsController, type: :request do expect(response).to redirect_to("/sales-logs") end - - context "when feature flag disabled" do - before do - allow(FeatureToggle).to receive(:new_data_protection_confirmation?).and_return(false) - end - - it "does not redirect to lettings index page" do - get "/lettings-logs/bulk-upload-logs/start", params: {} - - expect(response).not_to redirect_to("/sales-logs") - end - end end context "when not in crossover period" do diff --git a/spec/requests/organisations_controller_spec.rb b/spec/requests/organisations_controller_spec.rb index 9dc90fb8d..7ba7be1b2 100644 --- a/spec/requests/organisations_controller_spec.rb +++ b/spec/requests/organisations_controller_spec.rb @@ -1501,26 +1501,9 @@ RSpec.describe OrganisationsController, type: :request do sign_in user end - context "when flag not enabled" do - before do - allow(FeatureToggle).to receive(:new_data_protection_confirmation?).and_return(false) - end - - it "returns not found" do - get "/organisations/#{organisation.id}/data-sharing-agreement", headers: headers - expect(response).to have_http_status(:not_found) - end - end - - context "when flag enabled" do - before do - allow(FeatureToggle).to receive(:new_data_protection_confirmation?).and_return(true) - end - - it "returns ok" do - get "/organisations/#{organisation.id}/data-sharing-agreement", headers: headers - expect(response).to have_http_status(:ok) - end + it "returns ok" do + get "/organisations/#{organisation.id}/data-sharing-agreement", headers: headers + expect(response).to have_http_status(:ok) end end end @@ -1541,10 +1524,8 @@ RSpec.describe OrganisationsController, type: :request do sign_in user end - context "when flag not enabled" do - before do - allow(FeatureToggle).to receive(:new_data_protection_confirmation?).and_return(false) - end + context "when user not dpo" do + let(:user) { create(:user, is_dpo: false) } it "returns not found" do post "/organisations/#{organisation.id}/data-sharing-agreement", headers: headers @@ -1552,12 +1533,8 @@ RSpec.describe OrganisationsController, type: :request do end end - context "when flag enabled" do - before do - allow(FeatureToggle).to receive(:new_data_protection_confirmation?).and_return(true) - end - - context "when user not dpo" do + context "when user is dpo" do + context "when the organisation has a non-confirmed confirmation" do let(:user) { create(:user, is_dpo: false) } it "returns not found" do @@ -1566,49 +1543,50 @@ RSpec.describe OrganisationsController, type: :request do end end - context "when user is dpo" do - context "when the organisation has a non-confirmed confirmation" do - let(:user) { create(:user, is_dpo: false) } + context "when the organisation does not have a confirmation" do + before do + Timecop.freeze(Time.zone.local(2022, 2, 1)) + end - it "returns not found" do - post "/organisations/#{organisation.id}/data-sharing-agreement", headers: headers - expect(response).to have_http_status(:not_found) - end + after do + Timecop.unfreeze end - context "when the organisation does not have a confirmation" do - let(:user) { create(:user, is_dpo: true, organisation:) } + let(:user) { create(:user, is_dpo: true, organisation:) } - it "returns redirects to details page" do - post "/organisations/#{organisation.id}/data-sharing-agreement", headers: headers + it "returns redirects to details page" do + post "/organisations/#{organisation.id}/data-sharing-agreement", headers: headers - expect(response).to redirect_to("/organisations/#{organisation.id}/details") - expect(flash[:notice]).to eq("You have accepted the Data Sharing Agreement") - expect(flash[:notification_banner_body]).to eq("Your organisation can now submit logs.") - end + expect(response).to redirect_to("/organisations/#{organisation.id}/details") + expect(flash[:notice]).to eq("You have accepted the Data Sharing Agreement") + expect(flash[:notification_banner_body]).to eq("Your organisation can now submit logs.") + end - it "creates a data sharing agreement" do - expect(organisation.reload.data_protection_confirmation).to be_nil + it "creates a data sharing agreement" do + expect(organisation.reload.data_protection_confirmation).to be_nil - post("/organisations/#{organisation.id}/data-sharing-agreement", headers:) + post("/organisations/#{organisation.id}/data-sharing-agreement", headers:) - data_protection_confirmation = organisation.reload.data_protection_confirmation + data_protection_confirmation = organisation.reload.data_protection_confirmation - expect(data_protection_confirmation.organisation.address_row).to eq(organisation.address_row) - expect(data_protection_confirmation.organisation.name).to eq(organisation.name) - expect(data_protection_confirmation.organisation.phone).to eq(organisation.phone) - expect(data_protection_confirmation.data_protection_officer).to eq(user) - end + expect(data_protection_confirmation.organisation).to eq(organisation) + expect(data_protection_confirmation.data_protection_officer).to eq(user) + expect(data_protection_confirmation.signed_at).to eq(Time.zone.local(2022, 2, 1)) + expect(data_protection_confirmation.organisation_name).to eq(organisation.name) + expect(data_protection_confirmation.organisation_address).to eq(organisation.address_row) + expect(data_protection_confirmation.organisation_phone_number).to eq(organisation.phone) + expect(data_protection_confirmation.data_protection_officer_email).to eq(user.email) + expect(data_protection_confirmation.data_protection_officer_name).to eq(user.name) + end - context "when the user has already accepted the agreement" do - before do - create(:data_protection_confirmation, data_protection_officer: user, organisation: user.organisation) - end + context "when the user has already accepted the agreement" do + before do + create(:data_protection_confirmation, data_protection_officer: user, organisation: user.organisation) + end - it "returns not found" do - post "/organisations/#{organisation.id}/data-sharing-agreement", headers: headers - expect(response).to have_http_status(:not_found) - end + it "returns not found" do + post "/organisations/#{organisation.id}/data-sharing-agreement", headers: headers + expect(response).to have_http_status(:not_found) 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 eb386c117..8b021cca8 100644 --- a/spec/services/imports/data_protection_confirmation_import_service_spec.rb +++ b/spec/services/imports/data_protection_confirmation_import_service_spec.rb @@ -29,7 +29,7 @@ RSpec.describe Imports::DataProtectionConfirmationImportService do end context "when the organisation does exist" do - let!(:organisation) { create(:organisation, :without_dpc, old_org_id:) } + let!(:organisation) { create(:organisation, :without_dpc, old_org_id:, phone: "123") } context "when a data protection officer with matching name does not exists for the organisation" do it "creates an inactive data protection officer" do @@ -40,12 +40,18 @@ RSpec.describe Imports::DataProtectionConfirmationImportService do expect(data_protection_officer.active).to be false end - it "successfully create a data protection confirmation record with the expected data" do + it "successfully create a data protection confirmation record with the expected data", :aggregate_failures do import_service.create_data_protection_confirmations("data_protection_directory") confirmation = Organisation.find_by(old_org_id:).data_protection_confirmation - expect(confirmation.data_protection_officer.name).to eq("John Doe") expect(confirmation.confirmed).to be_truthy + + expect(confirmation.data_protection_officer.name).to eq("John Doe") + expect(confirmation.data_protection_officer_name).to eq("John Doe") + expect(confirmation.organisation_address).to eq("2 Marsham Street, London, SW1P 4DF") + expect(confirmation.organisation_name).to eq("DLUHC") + expect(confirmation.organisation_phone_number).to eq("123") expect(Time.zone.local_to_utc(confirmation.created_at)).to eq(Time.utc(2018, 0o6, 0o5, 10, 36, 49)) + expect(Time.zone.local_to_utc(confirmation.signed_at)).to eq(Time.utc(2018, 0o6, 0o5, 10, 36, 49)) end end @@ -60,7 +66,15 @@ RSpec.describe Imports::DataProtectionConfirmationImportService do confirmation = Organisation.find_by(old_org_id:).data_protection_confirmation expect(confirmation.data_protection_officer.id).to eq(data_protection_officer.id) expect(confirmation.confirmed).to be_truthy + + expect(confirmation.data_protection_officer.name).to eq(data_protection_officer.name) + expect(confirmation.data_protection_officer_name).to eq(data_protection_officer.name) + expect(confirmation.data_protection_officer_email).to eq(data_protection_officer.email) + expect(confirmation.organisation_address).to eq("2 Marsham Street, London, SW1P 4DF") + expect(confirmation.organisation_name).to eq("DLUHC") + expect(confirmation.organisation_phone_number).to eq("123") expect(Time.zone.local_to_utc(confirmation.created_at)).to eq(Time.utc(2018, 0o6, 0o5, 10, 36, 49)) + expect(Time.zone.local_to_utc(confirmation.signed_at)).to eq(Time.utc(2018, 0o6, 0o5, 10, 36, 49)) end context "when the data protection record has already been imported previously" do diff --git a/spec/views/logs/_create_for_org_actions.html.erb_spec.rb b/spec/views/logs/_create_for_org_actions.html.erb_spec.rb index f554778c2..1f2918218 100644 --- a/spec/views/logs/_create_for_org_actions.html.erb_spec.rb +++ b/spec/views/logs/_create_for_org_actions.html.erb_spec.rb @@ -11,40 +11,21 @@ RSpec.describe "logs/_create_for_org_actions.html.erb" do let(:user) { create(:user) } - context "when flag disabled" do - before do - allow(FeatureToggle).to receive(:new_data_protection_confirmation?).and_return(false) - end - - it "shows create buttons" do + context "with data sharing agreement" do + it "does include create log buttons" do render - expect(fragment).to have_button("Create a new lettings log for this organisation") expect(fragment).to have_button("Create a new sales log for this organisation") end end - context "when flag enabled" do - before do - allow(FeatureToggle).to receive(:new_data_protection_confirmation?).and_return(true) - end - - context "with data sharing agreement" do - it "does include create log buttons" do - render - expect(fragment).to have_button("Create a new lettings log for this organisation") - expect(fragment).to have_button("Create a new sales log for this organisation") - end - end - - context "without data sharing agreement" do - let(:user) { create(:user, organisation: create(:organisation, :without_dpc)) } + context "without data sharing agreement" do + let(:user) { create(:user, organisation: create(:organisation, :without_dpc)) } - it "does not include create log buttons" do - render - expect(fragment).not_to have_button("Create a new lettings log for this organisation") - expect(fragment).not_to have_button("Create a new sales log for this organisation") - end + it "does not include create log buttons" do + render + expect(fragment).not_to have_button("Create a new lettings log for this organisation") + expect(fragment).not_to have_button("Create a new sales log for this organisation") end end end diff --git a/spec/views/organisations/data_sharing_agreement.html.erb_spec.rb b/spec/views/organisations/data_sharing_agreement.html.erb_spec.rb index b20cecf4d..9fda45274 100644 --- a/spec/views/organisations/data_sharing_agreement.html.erb_spec.rb +++ b/spec/views/organisations/data_sharing_agreement.html.erb_spec.rb @@ -65,6 +65,50 @@ RSpec.describe "organisations/data_sharing_agreement.html.erb", :aggregate_failu # Shows DPO and org details in 12.2 expect(fragment).to have_content("12.2. For #{organisation.name}: Name: #{dpo.name}, Postal Address: #{organisation.address_row}, E-mail address: #{dpo.email}, Telephone number: #{organisation.phone}") end + + context "when user email not valid" do + let(:dpo) do + u = User.new( + name: "test", + organisation:, + is_dpo: true, + encrypted_password: SecureRandom.hex(10), + email: SecureRandom.uuid, + confirmed_at: Time.zone.now, + active: false, + ) + u.save!(validate: false) + u + end + + let(:data_protection_confirmation) do + create( + :data_protection_confirmation, + organisation:, + created_at: Time.zone.now - 1.day, + data_protection_officer: dpo, + ) + end + + it "renders dynamic content" do + render + + # dpo name + expect(fragment).to have_content("Name: #{dpo.name}") + + # org details + expect(fragment).to have_content("#{organisation.name} of #{organisation.address_row} (“CORE Data Provider”)") + # header + expect(fragment).to have_css("h2", text: "#{organisation.name} and Department for Levelling Up, Housing and Communities") + # does not show action buttons + expect(fragment).not_to have_button(text: "Accept this agreement") + expect(fragment).not_to have_link(text: "Cancel", href: "/organisations/#{organisation.id}/details") + # sees signed_at date + expect(fragment).to have_content("9th day of January 2023") + # Shows DPO and org details in 12.2 + expect(fragment).to have_content("12.2. For #{organisation.name}: Name: #{dpo.name}, Postal Address: #{organisation.address_row}, Telephone number: #{organisation.phone}") + end + end end end diff --git a/spec/views/organisations/show.html.erb_spec.rb b/spec/views/organisations/show.html.erb_spec.rb index 47ec10df4..142097c27 100644 --- a/spec/views/organisations/show.html.erb_spec.rb +++ b/spec/views/organisations/show.html.erb_spec.rb @@ -15,20 +15,6 @@ RSpec.describe "organisations/show.html.erb" do let(:organisation_without_dpc) { create(:organisation, :without_dpc) } let(:organisation_with_dsa) { create(:organisation) } - context "when flag disabled" do - let(:user) { create(:user, organisation: organisation_without_dpc) } - - before do - allow(FeatureToggle).to receive(:new_data_protection_confirmation?).and_return(false) - end - - it "does not include data sharing agreement row" do - render - - expect(fragment).not_to have_content("Data Sharing Agreement") - end - end - context "when dpo" do let(:user) { create(:user, is_dpo: true, organisation: organisation_without_dpc) }