Pass in organisations to skip instead of using an env var

app/jobs/email_missing_addresses_csv_job.rb

@@ -5,8 +5,8 @@ class EmailMissingAddressesCsvJob < ApplicationJob
   EXPIRATION_TIME = 72.hours.to_i
   MISSING_ADDRESSES_THRESHOLD = 50
 
-  def perform(user_ids, organisation, log_type)
-    csv_service = Csv::MissingAddressesCsvService.new(organisation)
+  def perform(user_ids, organisation, log_type, skip_uprn_issue_organisations)
+    csv_service = Csv::MissingAddressesCsvService.new(organisation, skip_uprn_issue_organisations)
     case log_type
     when "lettings"
       csv_string = csv_service.create_missing_lettings_addresses_csv

app/services/csv/missing_addresses_csv_service.rb

@@ -1,7 +1,8 @@
 module Csv
   class MissingAddressesCsvService
-    def initialize(organisation)
+    def initialize(organisation, skip_uprn_issue_organisations)
       @organisation = organisation
+      @skip_uprn_issue_organisations = skip_uprn_issue_organisations
     end
 
     def create_missing_lettings_addresses_csv
@@ -14,7 +15,7 @@ module Csv
       .where(needstype: 1, town_or_city: nil, uprn_known: [0, nil])
       .where.not(address_line1: nil)
 
-      logs_with_wrong_uprn = if JSON.parse(ENV["SKIP_UPRN_ISSUE_ORG_IDS"]).include?(@organisation.id)
+      logs_with_wrong_uprn = if @skip_uprn_issue_organisations.include?(@organisation.id)
                                []
                              else
                                @organisation.managed_lettings_logs
@@ -53,7 +54,7 @@ module Csv
       .where(town_or_city: nil, uprn_known: [0, nil])
       .where.not(address_line1: nil)
 
-      logs_with_wrong_uprn = if JSON.parse(ENV["SKIP_UPRN_ISSUE_ORG_IDS"]).include?(@organisation.id)
+      logs_with_wrong_uprn = if @skip_uprn_issue_organisations.include?(@organisation.id)
                                []
                              else
                                @organisation.sales_logs

lib/tasks/send_missing_addresses_csv.rake

@@ -1,6 +1,8 @@
 namespace :correct_addresses do
   desc "Send missing lettings addresses csv"
-  task :send_missing_addresses_lettings_csv, %i[] => :environment do |_task, _args|
+  task :send_missing_addresses_lettings_csv, %i[skip_uprn_issue_organisations] => :environment do |_task, args|
+    skip_uprn_issue_organisations = args[:skip_uprn_issue_organisations]&.split(" ")&.map(&:to_i) || []
+
     Organisation.all.each do |organisation|
       logs_impacted_by_missing_address = organisation.managed_lettings_logs
       .imported_2023_with_old_form_id
@@ -11,7 +13,7 @@ namespace :correct_addresses do
       .where(needstype: 1, town_or_city: nil, uprn_known: [0, nil])
       .where.not(address_line1: nil).count
 
-      logs_impacted_by_uprn_issue = if JSON.parse(ENV["SKIP_UPRN_ISSUE_ORG_IDS"]).include?(organisation.id)
+      logs_impacted_by_uprn_issue = if skip_uprn_issue_organisations.include?(organisation.id)
                                       []
                                     else
                                       organisation.managed_lettings_logs
@@ -25,7 +27,7 @@ namespace :correct_addresses do
       if logs_impacted_by_missing_address >= missing_addresses_threshold || logs_impacted_by_missing_town_or_city >= missing_addresses_threshold || logs_impacted_by_uprn_issue.any?
         data_coordinators = organisation.users.where(role: 2).filter_by_active
         users_to_contact = data_coordinators.any? ? data_coordinators : organisation.users.filter_by_active
-        EmailMissingAddressesCsvJob.perform_later(users_to_contact.map(&:id), organisation, "lettings")
+        EmailMissingAddressesCsvJob.perform_later(users_to_contact.map(&:id), organisation, "lettings", skip_uprn_issue_organisations)
         Rails.logger.info("Sending missing lettings addresses CSV for #{organisation.name} to #{users_to_contact.map(&:email).join(', ')}")
       else
         Rails.logger.info("Missing addresses below threshold for #{organisation.name}")
@@ -34,7 +36,9 @@ namespace :correct_addresses do
   end
 
   desc "Send missing sales addresses csv"
-  task :send_missing_addresses_sales_csv, %i[] => :environment do |_task, _args|
+  task :send_missing_addresses_sales_csv, %i[skip_uprn_issue_organisations] => :environment do |_task, args|
+    skip_uprn_issue_organisations = args[:skip_uprn_issue_organisations]&.split(" ")&.map(&:to_i) || []
+
     Organisation.all.each do |organisation|
       logs_impacted_by_missing_address = organisation.sales_logs
       .imported_2023_with_old_form_id
@@ -45,7 +49,7 @@ namespace :correct_addresses do
       .where(town_or_city: nil, uprn_known: [0, nil])
       .where.not(address_line1: nil).count
 
-      logs_impacted_by_uprn_issue = if JSON.parse(ENV["SKIP_UPRN_ISSUE_ORG_IDS"]).include?(organisation.id)
+      logs_impacted_by_uprn_issue = if skip_uprn_issue_organisations.include?(organisation.id)
                                       []
                                     else
                                       organisation.sales_logs
@@ -57,7 +61,7 @@ namespace :correct_addresses do
       if logs_impacted_by_missing_address >= missing_addresses_threshold || logs_impacted_by_missing_town_or_city >= missing_addresses_threshold || logs_impacted_by_uprn_issue.any?
         data_coordinators = organisation.users.where(role: 2).filter_by_active
         users_to_contact = data_coordinators.any? ? data_coordinators : organisation.users.filter_by_active
-        EmailMissingAddressesCsvJob.perform_later(users_to_contact.map(&:id), organisation, "sales")
+        EmailMissingAddressesCsvJob.perform_later(users_to_contact.map(&:id), organisation, "sales", skip_uprn_issue_organisations)
         Rails.logger.info("Sending missing sales addresses CSV for #{organisation.name} to #{users_to_contact.map(&:email).join(', ')}")
       else
         Rails.logger.info("Missing addresses below threshold for #{organisation.name}")

spec/jobs/email_missing_addresses_csv_job_spec.rb

@@ -29,38 +29,38 @@ describe EmailMissingAddressesCsvJob do
   context "when sending missing lettings logs csv" do
     it "uses an appropriate filename in S3" do
       expect(storage_service).to receive(:write_file).with(/missing-lettings-logs-addresses-#{organisation.name}-.*\.csv/, anything)
-      job.perform(users.map(&:id), organisation, "lettings")
+      job.perform(users.map(&:id), organisation, "lettings", [1, 2])
     end
 
     it "creates a MissingAddressesCsvService with the correct organisation and calls create missing lettings logs adresses csv" do
-      expect(Csv::MissingAddressesCsvService).to receive(:new).with(organisation)
+      expect(Csv::MissingAddressesCsvService).to receive(:new).with(organisation, [1, 2])
       expect(missing_addresses_csv_service).to receive(:create_missing_lettings_addresses_csv)
-      job.perform(users.map(&:id), organisation, "lettings")
+      job.perform(users.map(&:id), organisation, "lettings", [1, 2])
     end
 
     it "sends emails to all the provided users" do
       expect(mailer).to receive(:send_missing_lettings_addresses_csv_download_mail).with(users[0], test_url, instance_of(Integer))
       expect(mailer).to receive(:send_missing_lettings_addresses_csv_download_mail).with(users[1], test_url, instance_of(Integer))
-      job.perform(users.map(&:id), organisation, "lettings")
+      job.perform(users.map(&:id), organisation, "lettings", [1, 2])
     end
   end
 
   context "when sending missing sales logs csv" do
     it "uses an appropriate filename in S3" do
       expect(storage_service).to receive(:write_file).with(/missing-sales-logs-addresses-#{organisation.name}-.*\.csv/, anything)
-      job.perform(users.map(&:id), organisation, "sales")
+      job.perform(users.map(&:id), organisation, "sales", [2, 3])
     end
 
     it "creates a MissingAddressesCsvService with the correct organisation and calls create missing sales logs adresses csv" do
-      expect(Csv::MissingAddressesCsvService).to receive(:new).with(organisation)
+      expect(Csv::MissingAddressesCsvService).to receive(:new).with(organisation, [2, 3])
       expect(missing_addresses_csv_service).to receive(:create_missing_sales_addresses_csv)
-      job.perform(users.map(&:id), organisation, "sales")
+      job.perform(users.map(&:id), organisation, "sales", [2, 3])
     end
 
     it "sends emails to all the provided users" do
       expect(mailer).to receive(:send_missing_sales_addresses_csv_download_mail).with(users[0], test_url, instance_of(Integer))
       expect(mailer).to receive(:send_missing_sales_addresses_csv_download_mail).with(users[1], test_url, instance_of(Integer))
-      job.perform(users.map(&:id), organisation, "sales")
+      job.perform(users.map(&:id), organisation, "sales", [2, 3])
     end
   end
 end

spec/lib/tasks/send_missing_addresses_csv_spec.rb

@@ -59,7 +59,7 @@ RSpec.describe "correct_addresses" do
         end
 
         it "enqueues the job with correct organisations" do
-          expect { task.invoke }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_coordinator.id, data_coordinator2.id), organisation, "lettings")
+          expect { task.invoke }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_coordinator.id, data_coordinator2.id), organisation, "lettings", [])
         end
 
         it "prints out the jobs enqueued" do
@@ -78,7 +78,7 @@ RSpec.describe "correct_addresses" do
         end
 
         it "enqueues the job with correct organisations" do
-          expect { task.invoke }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_provider.id, data_provider2.id), organisation, "lettings")
+          expect { task.invoke }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_provider.id, data_provider2.id), organisation, "lettings", [])
         end
 
         it "prints out the jobs enqueued" do
@@ -109,7 +109,7 @@ RSpec.describe "correct_addresses" do
         end
 
         it "enqueues the job with correct organisations" do
-          expect { task.invoke }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_coordinator.id, data_coordinator2.id), organisation, "lettings")
+          expect { task.invoke }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_coordinator.id, data_coordinator2.id), organisation, "lettings", [])
         end
 
         it "prints out the jobs enqueued" do
@@ -128,7 +128,7 @@ RSpec.describe "correct_addresses" do
         end
 
         it "enqueues the job with correct organisations" do
-          expect { task.invoke }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_provider.id, data_provider2.id), organisation, "lettings")
+          expect { task.invoke }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_provider.id, data_provider2.id), organisation, "lettings", [])
         end
 
         it "prints out the jobs enqueued" do
@@ -159,7 +159,7 @@ RSpec.describe "correct_addresses" do
         end
 
         it "enqueues the job with correct organisations" do
-          expect { task.invoke }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_coordinator.id, data_coordinator2.id), organisation, "lettings")
+          expect { task.invoke }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_coordinator.id, data_coordinator2.id), organisation, "lettings", [])
         end
 
         it "prints out the jobs enqueued" do
@@ -178,7 +178,7 @@ RSpec.describe "correct_addresses" do
         end
 
         it "enqueues the job with correct organisations" do
-          expect { task.invoke }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_provider.id, data_provider2.id), organisation, "lettings")
+          expect { task.invoke }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_provider.id, data_provider2.id), organisation, "lettings", [])
         end
 
         it "prints out the jobs enqueued" do
@@ -198,7 +198,7 @@ RSpec.describe "correct_addresses" do
         end
 
         it "enqueues the job with correct organisations" do
-          expect { task.invoke }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_provider.id, data_provider2.id), organisation, "lettings")
+          expect { task.invoke }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_provider.id, data_provider2.id), organisation, "lettings", [])
         end
 
         it "prints out the jobs enqueued" do
@@ -208,14 +208,26 @@ RSpec.describe "correct_addresses" do
         end
       end
 
-      context "when org is included in SKIP_UPRN_ISSUE_ORG_IDS list" do
+      context "when org is included in skip_uprn_issue_organisations list" do
         before do
           create_list(:lettings_log, 5, :imported, startdate: Time.zone.local(2023, 9, 9), uprn: "12", propcode: "12", needstype: 1, owning_organisation: organisation, managing_organisation: organisation, created_by: organisation.users.first)
-          allow(ENV).to receive(:[]).with("SKIP_UPRN_ISSUE_ORG_IDS").and_return([organisation.id].to_json)
         end
 
         it "does not enqueue the job" do
-          expect { task.invoke }.not_to enqueue_job(EmailMissingAddressesCsvJob)
+          expect { task.invoke(organisation.id.to_s) }.not_to enqueue_job(EmailMissingAddressesCsvJob)
+        end
+      end
+
+      context "when skip_uprn_issue_organisations list is provided" do
+        let!(:data_provider) { create(:user, :data_provider, organisation:, email: "data_provider3@example.com") }
+        let!(:data_provider2) { create(:user, :data_provider, organisation:, email: "data_provider4@example.com") }
+
+        before do
+          create_list(:lettings_log, 5, :imported, startdate: Time.zone.local(2023, 9, 9), uprn: "12", propcode: "12", needstype: 1, owning_organisation: organisation, managing_organisation: organisation, created_by: organisation.users.first)
+        end
+
+        it "does enqueues the job with correct skip_uprn_issue_organisations" do
+          expect { task.invoke("100 400") }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_provider.id, data_provider2.id), organisation, "lettings", [100, 400])
         end
       end
     end
@@ -278,7 +290,7 @@ RSpec.describe "correct_addresses" do
         end
 
         it "enqueues the job with correct organisations" do
-          expect { task.invoke }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_coordinator.id, data_coordinator2.id), organisation, "sales")
+          expect { task.invoke("70 90") }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_coordinator.id, data_coordinator2.id), organisation, "sales", [70, 90])
         end
 
         it "prints out the jobs enqueued" do
@@ -297,7 +309,7 @@ RSpec.describe "correct_addresses" do
         end
 
         it "enqueues the job with correct organisations" do
-          expect { task.invoke }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_provider.id, data_provider2.id), organisation, "sales")
+          expect { task.invoke }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_provider.id, data_provider2.id), organisation, "sales", [])
         end
 
         it "prints out the jobs enqueued" do
@@ -328,7 +340,7 @@ RSpec.describe "correct_addresses" do
         end
 
         it "enqueues the job with correct organisations" do
-          expect { task.invoke }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_coordinator.id, data_coordinator2.id), organisation, "sales")
+          expect { task.invoke }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_coordinator.id, data_coordinator2.id), organisation, "sales", [])
         end
 
         it "prints out the jobs enqueued" do
@@ -347,7 +359,7 @@ RSpec.describe "correct_addresses" do
         end
 
         it "enqueues the job with correct organisations" do
-          expect { task.invoke }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_provider.id, data_provider2.id), organisation, "sales")
+          expect { task.invoke }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_provider.id, data_provider2.id), organisation, "sales", [])
         end
 
         it "prints out the jobs enqueued" do
@@ -378,7 +390,7 @@ RSpec.describe "correct_addresses" do
         end
 
         it "enqueues the job with correct organisations" do
-          expect { task.invoke }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_coordinator.id, data_coordinator2.id), organisation, "sales")
+          expect { task.invoke }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_coordinator.id, data_coordinator2.id), organisation, "sales", [])
         end
 
         it "prints out the jobs enqueued" do
@@ -397,7 +409,7 @@ RSpec.describe "correct_addresses" do
         end
 
         it "enqueues the job with correct organisations" do
-          expect { task.invoke }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_provider.id, data_provider2.id), organisation, "sales")
+          expect { task.invoke }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_provider.id, data_provider2.id), organisation, "sales", [])
         end
 
         it "prints out the jobs enqueued" do
@@ -417,7 +429,7 @@ RSpec.describe "correct_addresses" do
         end
 
         it "enqueues the job with correct organisations" do
-          expect { task.invoke }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_provider.id, data_provider2.id), organisation, "sales")
+          expect { task.invoke }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_provider.id, data_provider2.id), organisation, "sales", [])
         end
 
         it "prints out the jobs enqueued" do
@@ -427,14 +439,26 @@ RSpec.describe "correct_addresses" do
         end
       end
 
-      context "when org is included in SKIP_UPRN_ISSUE_ORG_IDS list" do
+      context "when org is included in skip_uprn_issue_organisations list" do
         before do
           create_list(:sales_log, 5, :completed, :imported, saledate: Time.zone.local(2023, 9, 9), uprn_known: 1, uprn: "12", purchid: "12", owning_organisation: organisation, created_by: organisation.users.first)
-          allow(ENV).to receive(:[]).with("SKIP_UPRN_ISSUE_ORG_IDS").and_return([organisation.id].to_json)
         end
 
         it "does not enqueue the job" do
-          expect { task.invoke }.not_to enqueue_job(EmailMissingAddressesCsvJob)
+          expect { task.invoke("#{organisation.id} 4") }.not_to enqueue_job(EmailMissingAddressesCsvJob)
+        end
+      end
+
+      context "when skip_uprn_issue_organisations list is provided" do
+        let!(:data_provider) { create(:user, :data_provider, organisation:, email: "data_provider3@example.com") }
+        let!(:data_provider2) { create(:user, :data_provider, organisation:, email: "data_provider4@example.com") }
+
+        before do
+          create_list(:sales_log, 5, :completed, :imported, saledate: Time.zone.local(2023, 9, 9), uprn_known: 1, uprn: "12", purchid: "12", owning_organisation: organisation, created_by: organisation.users.first)
+        end
+
+        it "does enqueues the job with correct skip_uprn_issue_organisations" do
+          expect { task.invoke("100 400") }.to enqueue_job(EmailMissingAddressesCsvJob).with(include(data_provider.id, data_provider2.id), organisation, "sales", [100, 400])
         end
       end
     end

spec/services/csv/missing_addresses_csv_service_spec.rb

@@ -3,7 +3,8 @@ require "rails_helper"
 RSpec.describe Csv::MissingAddressesCsvService do
   let(:organisation) { create(:organisation, name: "Address org") }
   let(:user) { create(:user, organisation:, email: "testy@example.com") }
-  let(:service) { described_class.new(organisation) }
+  let(:service) { described_class.new(organisation, skip_uprn_issue_organisations) }
+  let(:skip_uprn_issue_organisations) { [100, 200] }
 
   before do
     body_1 = {
@@ -137,10 +138,8 @@ RSpec.describe Csv::MissingAddressesCsvService do
         expect(csv).to eq(expected_content)
       end
 
-      context "and the organisation is in the SKIP_UPRN_ISSUE_ORG_IDS list" do
-        before do
-          allow(ENV).to receive(:[]).with("SKIP_UPRN_ISSUE_ORG_IDS").and_return([organisation.id].to_json)
-        end
+      context "and the organisation is marked as an organisation to skip" do
+        let(:skip_uprn_issue_organisations) { [organisation.id] }
 
         it "returns nil" do
           expect(service.create_missing_lettings_addresses_csv).to be_nil
@@ -276,10 +275,8 @@ RSpec.describe Csv::MissingAddressesCsvService do
         expect(csv).to eq(expected_content)
       end
 
-      context "and the organisation is in the SKIP_UPRN_ISSUE_ORG_IDS list" do
-        before do
-          allow(ENV).to receive(:[]).with("SKIP_UPRN_ISSUE_ORG_IDS").and_return([organisation.id].to_json)
-        end
+      context "and the organisation is marked as an organisation to skip" do
+        let(:skip_uprn_issue_organisations) { [organisation.id] }
 
         it "returns nil" do
           expect(service.create_missing_sales_addresses_csv).to be_nil