From 6831dd64561eb397ab5040bfe9d503c39bfe820d Mon Sep 17 00:00:00 2001 From: Samuel Young Date: Wed, 17 Sep 2025 16:09:31 +0100 Subject: [PATCH] CLDC-4028: Ensure changes to dependent objects are included in export (#3105) * CLDC-4028: Export records where a related one has updated for instance, re-export all users in an org if the name updates rework the export code a little to allow for this to be expressed cleanly * CLDC-4028: Add tests --- .../exports/lettings_log_export_service.rb | 34 +++++++-- .../exports/organisation_export_service.rb | 29 +++----- .../exports/sales_log_export_service.rb | 34 +++++++-- app/services/exports/user_export_service.rb | 21 ++++-- app/services/exports/xml_export_service.rb | 10 +++ .../lettings_log_export_service_spec.rb | 73 ++++++++++++++++++- .../organisation_export_service_spec.rb | 37 ++++++++++ .../exports/sales_log_export_service_spec.rb | 71 ++++++++++++++++++ .../exports/user_export_service_spec.rb | 37 ++++++++++ 9 files changed, 304 insertions(+), 42 deletions(-) diff --git a/app/services/exports/lettings_log_export_service.rb b/app/services/exports/lettings_log_export_service.rb index 1ad44cbe4..4e1fe2019 100644 --- a/app/services/exports/lettings_log_export_service.rb +++ b/app/services/exports/lettings_log_export_service.rb @@ -30,14 +30,32 @@ module Exports "core_#{year}_#{year + 1}_apr_mar_#{base_number_str}_#{increment_str}".downcase end - def retrieve_resources(recent_export, full_update, year) - if !full_update && recent_export - params = { from: recent_export.started_at, to: @start_time } - LettingsLog.exportable.where("(updated_at >= :from AND updated_at <= :to) OR (values_updated_at IS NOT NULL AND values_updated_at >= :from AND values_updated_at <= :to)", params).filter_by_year(year) - else - params = { to: @start_time } - LettingsLog.exportable.where("updated_at <= :to", params).filter_by_year(year) - end + def retrieve_resources_from_range(range, year) + relation = LettingsLog.exportable.filter_by_year(year).left_joins(:created_by, :updated_by, :assigned_to, :owning_organisation, :managing_organisation) + + ids = relation + .where({ updated_at: range }) + .or( + relation.where.not(values_updated_at: nil).where(values_updated_at: range), + ) + .or( + relation.where.not({ created_by: { updated_at: nil } }).where({ created_by: { updated_at: range } }), + ) + .or( + relation.where.not({ updated_by: { updated_at: nil } }).where({ updated_by: { updated_at: range } }), + ) + .or( + relation.where.not({ assigned_to: { updated_at: nil } }).where({ assigned_to: { updated_at: range } }), + ) + .or( + relation.where.not({ owning_organisation: { updated_at: nil } }).where({ owning_organisation: { updated_at: range } }), + ) + .or( + relation.where.not({ managing_organisation: { updated_at: nil } }).where({ managing_organisation: { updated_at: range } }), + ) + .pluck(:id) + + LettingsLog.where(id: ids) end def apply_cds_transformation(lettings_log, export_mode) diff --git a/app/services/exports/organisation_export_service.rb b/app/services/exports/organisation_export_service.rb index 0e8967bbc..4cb98bc2c 100644 --- a/app/services/exports/organisation_export_service.rb +++ b/app/services/exports/organisation_export_service.rb @@ -25,24 +25,19 @@ module Exports "organisations_2024_2025_apr_mar_#{base_number_str}_#{increment_str}".downcase end - def retrieve_resources(recent_export, full_update, _year) - if !full_update && recent_export - params = { from: recent_export.started_at, to: @start_time } + def retrieve_resources_from_range(range, _year) + relation = Organisation.left_joins(:users, :organisation_name_changes) + ids = relation + .where({ updated_at: range }) + .or( + relation.where(organisation_name_changes: { created_at: range }), + ) + .or( + relation.where(users: { is_dpo: true, updated_at: range }), + ) + .pluck(:id) - Organisation - .where(updated_at: params[:from]..params[:to]) - .or( - Organisation.where(id: OrganisationNameChange.where(created_at: params[:from]..params[:to]).select(:organisation_id)), - ) - else - params = { to: @start_time } - - Organisation - .where("updated_at <= :to", params) - .or( - Organisation.where(id: OrganisationNameChange.where("created_at <= :to", params).select(:organisation_id)), - ) - end + Organisation.where(id: ids) end def build_export_xml(organisations) diff --git a/app/services/exports/sales_log_export_service.rb b/app/services/exports/sales_log_export_service.rb index 9cac14207..db525dde4 100644 --- a/app/services/exports/sales_log_export_service.rb +++ b/app/services/exports/sales_log_export_service.rb @@ -30,14 +30,32 @@ module Exports "core_sales_#{year}_#{year + 1}_apr_mar_#{base_number_str}_#{increment_str}".downcase end - def retrieve_resources(recent_export, full_update, year) - if !full_update && recent_export - params = { from: recent_export.started_at, to: @start_time } - SalesLog.exportable.where("(updated_at >= :from AND updated_at <= :to) OR (values_updated_at IS NOT NULL AND values_updated_at >= :from AND values_updated_at <= :to)", params).filter_by_year(year) - else - params = { to: @start_time } - SalesLog.exportable.where("updated_at <= :to", params).filter_by_year(year) - end + def retrieve_resources_from_range(range, year) + relation = SalesLog.exportable.filter_by_year(year).left_joins(:created_by, :updated_by, :assigned_to, :owning_organisation, :managing_organisation) + + ids = relation + .where({ updated_at: range }) + .or( + relation.where.not(values_updated_at: nil).where(values_updated_at: range), + ) + .or( + relation.where.not({ created_by: { updated_at: nil } }).where({ created_by: { updated_at: range } }), + ) + .or( + relation.where.not({ updated_by: { updated_at: nil } }).where({ updated_by: { updated_at: range } }), + ) + .or( + relation.where.not({ assigned_to: { updated_at: nil } }).where({ assigned_to: { updated_at: range } }), + ) + .or( + relation.where.not({ owning_organisation: { updated_at: nil } }).where({ owning_organisation: { updated_at: range } }), + ) + .or( + relation.where.not({ managing_organisation: { updated_at: nil } }).where({ managing_organisation: { updated_at: range } }), + ) + .pluck(:id) + + SalesLog.where(id: ids) end def apply_cds_transformation(sales_log, _export_mode) diff --git a/app/services/exports/user_export_service.rb b/app/services/exports/user_export_service.rb index 177daee1d..d79a4df49 100644 --- a/app/services/exports/user_export_service.rb +++ b/app/services/exports/user_export_service.rb @@ -25,14 +25,19 @@ module Exports "users_2024_2025_apr_mar_#{base_number_str}_#{increment_str}".downcase end - def retrieve_resources(recent_export, full_update, _year) - if !full_update && recent_export - params = { from: recent_export.started_at, to: @start_time } - User.where("(updated_at >= :from AND updated_at <= :to) OR (values_updated_at IS NOT NULL AND values_updated_at >= :from AND values_updated_at <= :to)", params) - else - params = { to: @start_time } - User.where("updated_at <= :to", params) - end + def retrieve_resources_from_range(range, _year) + relation = User.left_joins(organisation: :organisation_name_changes) + ids = relation + .where({ updated_at: range }) + .or( + relation.where.not(organisations: { updated_at: nil }).where(organisations: { updated_at: range }), + ) + .or( + relation.where(organisation_name_changes: { created_at: range }), + ) + .pluck(:id) + + User.where(id: ids) end def build_export_xml(users) diff --git a/app/services/exports/xml_export_service.rb b/app/services/exports/xml_export_service.rb index d499dd518..703d94f7d 100644 --- a/app/services/exports/xml_export_service.rb +++ b/app/services/exports/xml_export_service.rb @@ -31,6 +31,16 @@ module Exports Export.new(collection:, year:, started_at: @start_time, base_number:, increment_number:) end + def retrieve_resources(recent_export, full_update, year) + range = if !full_update && recent_export + recent_export.started_at..@start_time + else + ..@start_time + end + + retrieve_resources_from_range(range, year) + end + def write_export_archive(export, year, recent_export, full_update) archive = get_archive_name(year, export.base_number, export.increment_number) diff --git a/spec/services/exports/lettings_log_export_service_spec.rb b/spec/services/exports/lettings_log_export_service_spec.rb index c2197c027..38366e83d 100644 --- a/spec/services/exports/lettings_log_export_service_spec.rb +++ b/spec/services/exports/lettings_log_export_service_spec.rb @@ -1,6 +1,8 @@ require "rails_helper" RSpec.describe Exports::LettingsLogExportService do + include CollectionTimeHelper + subject(:export_service) { described_class.new(storage_service, start_time) } let(:storage_service) { instance_double(Storage::S3Service) } @@ -16,7 +18,7 @@ RSpec.describe Exports::LettingsLogExportService do let(:expected_manifest_filename) { "manifest.xml" } let(:start_time) { Time.zone.local(2022, 5, 1) } let(:organisation) { create(:organisation, name: "MHCLG", housing_registration_no: 1234) } - let(:user) { FactoryBot.create(:user, email: "test1@example.com", organisation:) } + let(:user) { create(:user, email: "test1@example.com", organisation:) } def replace_entity_ids(lettings_log, export_template) export_template.sub!(/\{id\}/, (lettings_log["id"] + Exports::LettingsLogExportService::LOG_ID_OFFSET).to_s) @@ -480,6 +482,75 @@ RSpec.describe Exports::LettingsLogExportService do end end end + + context "and one lettings log has not been updated in the time range" do + let(:expected_zip_filename) { "core_#{current_collection_start_year}_#{current_collection_end_year}_apr_mar_f0001_inc0001.zip" } + let(:start_time) { current_collection_start_date } + let!(:owning_organisation) { create(:organisation, name: "MHCLG owning", housing_registration_no: 1234) } + let!(:managing_organisation) { create(:organisation, name: "MHCLG managing", housing_registration_no: 1234) } + let!(:created_by_user) { create(:user, email: "test-created-by@example.com", organisation: managing_organisation) } + let!(:updated_by_user) { create(:user, email: "test-updated-by@example.com", organisation: managing_organisation) } + let!(:assigned_to_user) { create(:user, email: "test-assigned-to@example.com", organisation: managing_organisation) } + let!(:lettings_log) { create(:lettings_log, :completed, startdate: current_collection_start_date, created_by: created_by_user, updated_by: updated_by_user, assigned_to: assigned_to_user, owning_organisation:, managing_organisation:) } + + before do + # touch all the related records to ensure their updated_at value is outside the export range + Timecop.freeze(start_time + 1.month) + owning_organisation.touch + managing_organisation.touch + created_by_user.touch + updated_by_user.touch + assigned_to_user.touch + lettings_log.touch + Timecop.freeze(start_time) + end + + it "does not export the lettings log" do + expect(storage_service).not_to receive(:write_file).with(expected_zip_filename, any_args) + + export_service.export_xml_lettings_logs(collection_year: current_collection_start_year) + end + + it "does export the lettings log if created_by_user is updated" do + created_by_user.touch + + expect(storage_service).to receive(:write_file).with(expected_zip_filename, any_args) + + export_service.export_xml_lettings_logs(collection_year: current_collection_start_year) + end + + it "does export the lettings log if updated_by_user is updated" do + updated_by_user.touch + + expect(storage_service).to receive(:write_file).with(expected_zip_filename, any_args) + + export_service.export_xml_lettings_logs(collection_year: current_collection_start_year) + end + + it "does export the lettings log if assigned_to_user is updated" do + assigned_to_user.touch + + expect(storage_service).to receive(:write_file).with(expected_zip_filename, any_args) + + export_service.export_xml_lettings_logs(collection_year: current_collection_start_year) + end + + it "does export the lettings log if owning_organisation is updated" do + owning_organisation.touch + + expect(storage_service).to receive(:write_file).with(expected_zip_filename, any_args) + + export_service.export_xml_lettings_logs(collection_year: current_collection_start_year) + end + + it "does export the lettings log if managing_organisation is updated" do + managing_organisation.touch + + expect(storage_service).to receive(:write_file).with(expected_zip_filename, any_args) + + export_service.export_xml_lettings_logs(collection_year: current_collection_start_year) + end + end end context "when exporting a supported housing lettings logs in XML" do diff --git a/spec/services/exports/organisation_export_service_spec.rb b/spec/services/exports/organisation_export_service_spec.rb index 623cb359e..5664a15b5 100644 --- a/spec/services/exports/organisation_export_service_spec.rb +++ b/spec/services/exports/organisation_export_service_spec.rb @@ -1,6 +1,8 @@ require "rails_helper" RSpec.describe Exports::OrganisationExportService do + include CollectionTimeHelper + subject(:export_service) { described_class.new(storage_service, start_time) } let(:storage_service) { instance_double(Storage::S3Service) } @@ -283,5 +285,40 @@ RSpec.describe Exports::OrganisationExportService do expect(export_service.export_xml_organisations).to eq({ expected_zip_filename.gsub(".zip", "") => start_time }) end end + + context "and one organisation has not been updated in the time range" do + let(:dpo_user) { create(:user, email: "dpo@example.com", is_dpo: true, organisation:) } + let(:organisation) { create(:organisation, with_dsa: false) } + + before do + # touch all the related records to ensure their updated_at value is outside the export range + Timecop.freeze(start_time + 1.month) + organisation.touch + dpo_user.touch + Timecop.freeze(start_time) + end + + it "does not export the organisation" do + expect(storage_service).not_to receive(:write_file).with(expected_zip_filename, any_args) + + export_service.export_xml_organisations + end + + it "does export the organisation if an organisation name change is made" do + FactoryBot.create(:organisation_name_change, organisation:) + + expect(storage_service).to receive(:write_file).with(expected_zip_filename, any_args) + + export_service.export_xml_organisations + end + + it "does export the organisation if dpo_user is updated" do + dpo_user.touch + + expect(storage_service).to receive(:write_file).with(expected_zip_filename, any_args) + + export_service.export_xml_organisations + end + end end end diff --git a/spec/services/exports/sales_log_export_service_spec.rb b/spec/services/exports/sales_log_export_service_spec.rb index 15894ae3d..1d54f71e0 100644 --- a/spec/services/exports/sales_log_export_service_spec.rb +++ b/spec/services/exports/sales_log_export_service_spec.rb @@ -1,6 +1,8 @@ require "rails_helper" RSpec.describe Exports::SalesLogExportService do + include CollectionTimeHelper + subject(:export_service) { described_class.new(storage_service, start_time) } let(:storage_service) { instance_double(Storage::S3Service) } @@ -421,5 +423,74 @@ RSpec.describe Exports::SalesLogExportService do end end end + + context "and one sales log has not been updated in the time range" do + let(:expected_zip_filename) { "core_sales_#{current_collection_start_year}_#{current_collection_end_year}_apr_mar_f0001_inc0001.zip" } + let(:start_time) { current_collection_start_date } + let!(:owning_organisation) { create(:organisation, name: "MHCLG owning", housing_registration_no: 1234) } + let!(:managing_organisation) { create(:organisation, name: "MHCLG managing", housing_registration_no: 1234) } + let!(:created_by_user) { create(:user, email: "test-created-by@example.com", organisation: managing_organisation) } + let!(:updated_by_user) { create(:user, email: "test-updated-by@example.com", organisation: managing_organisation) } + let!(:assigned_to_user) { create(:user, email: "test-assigned-to@example.com", organisation: managing_organisation) } + let!(:sales_log) { create(:sales_log, :export, saledate: start_time, assigned_to: assigned_to_user, created_by: created_by_user, updated_by: updated_by_user, owning_organisation:, managing_organisation:) } + + before do + # touch all the related records to ensure their updated_at value is outside the export range + Timecop.freeze(start_time + 1.month) + owning_organisation.touch + managing_organisation.touch + created_by_user.touch + updated_by_user.touch + assigned_to_user.touch + sales_log.touch + Timecop.freeze(start_time) + end + + it "does not export the sales log" do + expect(storage_service).not_to receive(:write_file).with(expected_zip_filename, any_args) + + export_service.export_xml_sales_logs(collection_year: current_collection_start_year) + end + + it "does export the sales log if created_by_user is updated" do + created_by_user.touch + + expect(storage_service).to receive(:write_file).with(expected_zip_filename, any_args) + + export_service.export_xml_sales_logs(collection_year: current_collection_start_year) + end + + it "does export the sales log if updated_by_user is updated" do + updated_by_user.touch + + expect(storage_service).to receive(:write_file).with(expected_zip_filename, any_args) + + export_service.export_xml_sales_logs(collection_year: current_collection_start_year) + end + + it "does export the sales log if assigned_to_user is updated" do + assigned_to_user.touch + + expect(storage_service).to receive(:write_file).with(expected_zip_filename, any_args) + + export_service.export_xml_sales_logs(collection_year: current_collection_start_year) + end + + it "does export the sales log if owning_organisation is updated" do + owning_organisation.touch + + expect(storage_service).to receive(:write_file).with(expected_zip_filename, any_args) + + export_service.export_xml_sales_logs(collection_year: current_collection_start_year) + end + + it "does export the sales log if managing_organisation is updated" do + managing_organisation.touch + + expect(storage_service).to receive(:write_file).with(expected_zip_filename, any_args) + + export_service.export_xml_sales_logs(collection_year: current_collection_start_year) + end + end end end diff --git a/spec/services/exports/user_export_service_spec.rb b/spec/services/exports/user_export_service_spec.rb index bafbc7f77..f62664945 100644 --- a/spec/services/exports/user_export_service_spec.rb +++ b/spec/services/exports/user_export_service_spec.rb @@ -1,6 +1,8 @@ require "rails_helper" RSpec.describe Exports::UserExportService do + include CollectionTimeHelper + subject(:export_service) { described_class.new(storage_service, start_time) } let(:storage_service) { instance_double(Storage::S3Service) } @@ -235,5 +237,40 @@ RSpec.describe Exports::UserExportService do expect(export_service.export_xml_users).to eq({ expected_zip_filename.gsub(".zip", "") => start_time }) end end + + context "and one user has not been updated in the time range" do + let(:start_time) { current_collection_start_date } + let!(:user) { create(:user, organisation:) } + + before do + # touch all the related records to ensure their updated_at value is outside the export range + Timecop.freeze(start_time + 1.month) + organisation.touch + user.touch + Timecop.freeze(start_time) + end + + it "does not export the user" do + expect(storage_service).not_to receive(:write_file).with(expected_zip_filename, any_args) + + export_service.export_xml_users + end + + it "does export the user if organisation is updated" do + organisation.touch + + expect(storage_service).to receive(:write_file).with(expected_zip_filename, any_args) + + export_service.export_xml_users + end + + it "does export the user if an organisation name change is made" do + FactoryBot.create(:organisation_name_change, organisation:) + + expect(storage_service).to receive(:write_file).with(expected_zip_filename, any_args) + + export_service.export_xml_users + end + end end end