CLDC-4078: Optimise OS API queries for records that are validated a lot

specifically, during bulk uploads an object is validated many times before saving

dirty checks only reset on save meaning after setting the UPRN the process_uprn_change code would run on every validation, leading to many calls

to solve this, add a flag to the log which blocks further UPRN calls if it tries to set the uprn twice to the same value. if different values are returned then continue checking

this offers a tradeoff between number of api calls and also should not break existing functionality

Gemfile.lock

@@ -411,7 +411,7 @@ GEM
     responders (3.1.1)
       actionpack (>= 5.2)
       railties (>= 5.2)
-    rexml (3.3.9)
+    rexml (3.4.4)
     roo (2.10.1)
       nokogiri (~> 1)
       rubyzip (>= 1.3.0, < 3.0.0)

app/models/lettings_log.rb

@@ -915,6 +915,7 @@ private
   def should_process_uprn_change?
     return unless uprn
     return unless startdate
+    return if skip_uprn_lookup
 
     uprn_changed? || startdate_changed?
   end
@@ -923,6 +924,7 @@ private
     return unless uprn_selection || select_best_address_match
     return unless startdate
     return unless form.start_year_2024_or_later?
+    return if skip_address_lookup
 
     if select_best_address_match
       address_line1_input.present? && postcode_full_input.present?

app/models/log.rb

@@ -57,7 +57,7 @@ class Log < ApplicationRecord
   scope :filter_by_owning_organisation_text_search, ->(param, _user) { where(owning_organisation: Organisation.search_by(param)) }
   scope :filter_by_managing_organisation_text_search, ->(param, _user) { where(managing_organisation: Organisation.search_by(param)) }
 
-  attr_accessor :skip_update_status, :skip_update_uprn_confirmed, :select_best_address_match, :skip_dpo_validation
+  attr_accessor :skip_update_status, :skip_update_uprn_confirmed, :select_best_address_match, :skip_dpo_validation, :skip_uprn_lookup, :skip_address_lookup
 
   delegate :present?, to: :address_options, prefix: true
 
@@ -74,6 +74,9 @@ class Log < ApplicationRecord
 
       presenter = UprnDataPresenter.new(service.result)
 
+      # the address for this uprn is already known, skip further lookups for this object
+      self.skip_uprn_lookup = true if address_line1 == presenter.address_line1 && address_line2 == presenter.address_line2 && town_or_city == presenter.town_or_city && postcode_full == presenter.postcode
+
       self.uprn_known = 1
       self.uprn_selection = uprn
       self.address_line1 = presenter.address_line1
@@ -98,9 +101,14 @@ class Log < ApplicationRecord
         presenter = AddressDataPresenter.new(service.result.first)
         os_match_threshold_for_bulk_upload = 0.7
         if presenter.match >= os_match_threshold_for_bulk_upload
+          # the address for this uprn is already known, skip further lookups for this object
+          self.skip_address_lookup = true if uprn_selection == presenter.uprn
+
           self.uprn_selection = presenter.uprn
         else
           select_manual_address_entry!
+          # this uprn cannot be used for lookup
+          self.skip_address_lookup = true
           return nil
         end
       end
@@ -119,7 +127,7 @@ class Log < ApplicationRecord
         self.uprn = uprn_selection
         self.uprn_confirmed = 1
         self.skip_update_uprn_confirmed = true
-        process_uprn_change!
+        process_uprn_change! unless skip_uprn_lookup
       end
     end
   end

app/models/sales_log.rb

@@ -435,6 +435,7 @@ class SalesLog < Log
   def should_process_uprn_change?
     return unless uprn
     return unless saledate
+    return if skip_uprn_lookup
 
     uprn_changed? || saledate_changed?
   end
@@ -443,6 +444,7 @@ class SalesLog < Log
     return unless uprn_selection || select_best_address_match
     return unless saledate
     return unless form.start_year_2024_or_later?
+    return if skip_address_lookup
 
     if select_best_address_match
       address_line1_input.present? && postcode_full_input.present?

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)

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 }
-
-        Organisation
-          .where(updated_at: params[:from]..params[:to])
+    def retrieve_resources_from_range(range, _year)
+      relation = Organisation.left_joins(:users, :organisation_name_changes)
+      ids = relation
+              .where({ updated_at: range })
               .or(
-            Organisation.where(id: OrganisationNameChange.where(created_at: params[:from]..params[:to]).select(:organisation_id)),
+                relation.where(organisation_name_changes: { created_at: range }),
               )
-      else
-        params = { to: @start_time }
-
-        Organisation
-          .where("updated_at <= :to", params)
               .or(
-            Organisation.where(id: OrganisationNameChange.where("created_at <= :to", params).select(:organisation_id)),
+                relation.where(users: { is_dpo: true, updated_at: range }),
               )
-      end
+              .pluck(:id)
+
+      Organisation.where(id: ids)
     end
 
     def build_export_xml(organisations)

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)

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)

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)
 

docs/Gemfile.lock

@@ -226,7 +226,7 @@ GEM
     rb-fsevent (0.11.2)
     rb-inotify (0.10.1)
       ffi (~> 1.0)
-    rexml (3.3.9)
+    rexml (3.4.2)
     rouge (3.26.0)
     ruby2_keywords (0.0.5)
     rubyzip (2.3.2)

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

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

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

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