From b81d7dba484adcdbd361d4dd12b6c073a9844094 Mon Sep 17 00:00:00 2001
From: Theron Toomey <ttoomey@greenriver.org>
Date: Mon, 30 Dec 2024 12:11:38 -0500
Subject: [PATCH] 7135 fixes for GenerateClientRoiAuthorizationsTask (#5019)

* Fix bug where ROI records where deleted, keeping only the last batch
* Fix bug where ROI records might be left in place after the client record data changed
* Improved tests
---
 .../client_roi_authorization.rb               |   2 +
 ...generate_client_roi_authorizations_task.rb |  22 ++-
 ...ate_client_roi_authorizations_task_spec.rb | 168 +++++++++++++-----
 3 files changed, 137 insertions(+), 55 deletions(-)

diff --git a/app/models/grda_warehouse/client_roi_authorization.rb b/app/models/grda_warehouse/client_roi_authorization.rb
index ced5049b0e8..af4131928aa 100644
--- a/app/models/grda_warehouse/client_roi_authorization.rb
+++ b/app/models/grda_warehouse/client_roi_authorization.rb
@@ -14,6 +14,8 @@ class ClientRoiAuthorization < GrdaWarehouseBase
     PARTIAL_STATUS = 'partial'.freeze
     FULL_STATUS = 'full'.freeze
 
+    scope :with_invalid_client, -> { left_outer_joins(:destination_client).where(c_t[:id].eq(nil)) }
+
     def active?(date: Date.current)
       case status
       when PARTIAL_STATUS, FULL_STATUS
diff --git a/app/models/grda_warehouse/tasks/generate_client_roi_authorizations_task.rb b/app/models/grda_warehouse/tasks/generate_client_roi_authorizations_task.rb
index 337a4bce096..f8679dc1ca5 100644
--- a/app/models/grda_warehouse/tasks/generate_client_roi_authorizations_task.rb
+++ b/app/models/grda_warehouse/tasks/generate_client_roi_authorizations_task.rb
@@ -13,16 +13,20 @@ def self.perform(...)
     end
 
     # @param client_ids [Array<Integer>, nil] client ids to rebuild. Rebuild all clients if nil
-    def perform(client_ids: nil)
+    # @param batch_size [Integer] number of records to process in each batch
+    def perform(client_ids: nil, batch_size: 500)
       with_lock do
         scope = destination_client_scope
         scope = scope.where(id: client_ids) unless client_ids.nil?
-        scope.find_in_batches do |batch|
+        scope.find_in_batches(batch_size: batch_size) do |batch|
           values = []
+          missing_ids = []
           batch.each do |client|
             result = process_client(client)
+            missing_ids << client.id if result.nil?
             values << result if result
           end
+
           GrdaWarehouse::ClientRoiAuthorization.import(
             values,
             on_duplicate_key_update: {
@@ -30,11 +34,15 @@ def perform(client_ids: nil)
               columns: values.first&.keys&.excluding(:destination_client_id),
             },
           )
-          # cleanup orphans
-          orphan_scope = GrdaWarehouse::ClientRoiAuthorization.
-            where.not(destination_client_id: destination_client_scope.select(:id))
-          orphan_scope = orphan_scope.where(destination_client_id: client_ids) if client_ids
-          orphan_scope.delete_all
+
+          # cleanup auth records for clients that have lost ROI status (but client still exists). This might be due to data correction rather than revocation
+          GrdaWarehouse::ClientRoiAuthorization.where(destination_client: missing_ids).delete_all
+        end
+
+        # cleanup orphaned auth records where the client record no-longer exists at all
+        orphan_ids = GrdaWarehouse::ClientRoiAuthorization.with_invalid_client.pluck(:id)
+        orphan_ids.each_slice(batch_size) do |ids|
+          GrdaWarehouse::ClientRoiAuthorization.where(id: ids).delete_all
         end
       end
     end
diff --git a/spec/models/grda_warehouse/tasks/generate_client_roi_authorizations_task_spec.rb b/spec/models/grda_warehouse/tasks/generate_client_roi_authorizations_task_spec.rb
index 60ffa54001e..1f355787b25 100644
--- a/spec/models/grda_warehouse/tasks/generate_client_roi_authorizations_task_spec.rb
+++ b/spec/models/grda_warehouse/tasks/generate_client_roi_authorizations_task_spec.rb
@@ -1,81 +1,153 @@
-# spec/models/grda_warehouse/tasks/generate_client_roi_authorizations_task_spec.rb
 require 'rails_helper'
 
 RSpec.describe GrdaWarehouse::Tasks::GenerateClientRoiAuthorizationsTask, type: :model do
   let(:task) { described_class.new }
+  let(:today) { Date.current }
 
-  describe '#process_client' do
-    let(:client) { create(:hud_client) }
+  # Shared contexts for common test setups
+  shared_context 'with release duration settings' do |duration, period = nil|
+    before do
+      allow(GrdaWarehouse::Hud::Client).to receive(:release_duration).and_return(duration)
+      allow(GrdaWarehouse::Hud::Client).to receive(:consent_validity_period).and_return(period) if period
+    end
+  end
 
-    context 'when client has valid release' do
-      before do
-        allow(client).to receive(:release_valid?).and_return(true)
-        allow(client).to receive(:revoked_consent?).and_return(false)
-        allow(client).to receive(:partial_release?).and_return(false)
-      end
+  describe '#perform' do
+    let!(:destination_clients) do
+      5.times.map { create(:hud_client, consent_form_signed_on: today) }
+    end
+
+    # set a batch size lower than total number of clients to check interactions
+    let(:batch_size) { 3 }
+
+    before do
+      allow(task).to receive(:roi_status).and_return('full_status')
+    end
 
-      it 'returns authorization attributes with full status' do
-        result = task.send(:process_client, client)
-        expect(result[:status]).to eq(GrdaWarehouse::ClientRoiAuthorization::FULL_STATUS)
+    context 'with no existing ROI records' do
+      it 'creates appropriate auth records' do
+        expect do
+          task.perform(batch_size: batch_size)
+        end.to change { GrdaWarehouse::ClientRoiAuthorization.count }.by(5)
       end
     end
 
-    context 'when client has revoked consent' do
+    context 'with existing ROI records' do
       before do
-        allow(client).to receive(:revoked_consent?).and_return(true)
+        destination_clients.map do |client|
+          GrdaWarehouse::ClientRoiAuthorization.create!(
+            destination_client_id: client.id,
+            status: 'full_status',
+          )
+        end
       end
 
-      it 'returns authorization attributes with revoked status' do
-        result = task.send(:process_client, client)
-        expect(result[:status]).to eq(GrdaWarehouse::ClientRoiAuthorization::REVOKED_STATUS)
+      it 'does not change valid records' do
+        expect do
+          task.perform(batch_size: batch_size)
+        end.to(not_change { GrdaWarehouse::ClientRoiAuthorization.count })
       end
-    end
 
-    context 'when client has partial release' do
-      before do
-        allow(client).to receive(:revoked_consent?).and_return(false)
-        allow(client).to receive(:partial_release?).and_return(true)
+      context 'when a client is orphaned' do
+        before do
+          GrdaWarehouse::Hud::Client.where(id: destination_clients.last.id).delete_all
+        end
+
+        it 'removes the orphaned auth record' do
+          expect do
+            task.perform(batch_size: batch_size)
+          end.to change { GrdaWarehouse::ClientRoiAuthorization.count }.by(-1)
+        end
       end
 
-      it 'returns authorization attributes with partial status' do
-        result = task.send(:process_client, client)
-        expect(result[:status]).to eq(GrdaWarehouse::ClientRoiAuthorization::PARTIAL_STATUS)
+      context 'when client loses roi status' do
+        before do
+          allow(task).to receive(:roi_status) do |client|
+            client.id == destination_clients.last.id ? nil : 'full_status'
+          end
+        end
+
+        it 'removes the auth record' do
+          expect do
+            task.perform(batch_size: batch_size)
+          end.to change { GrdaWarehouse::ClientRoiAuthorization.count }.by(-1)
+        end
       end
     end
   end
 
-  describe '#roi_expiry_date' do
-    let(:client) { create(:hud_client, consent_form_signed_on: Date.current) }
+  describe '#process_client' do
+    let(:client) { create(:hud_client, consent_form_signed_on: today) }
+    subject(:processed_result) { task.send(:process_client, client) }
 
-    context 'when release duration is One Year' do
-      before do
-        allow(GrdaWarehouse::Hud::Client).to receive(:release_duration).and_return('One Year')
-        allow(GrdaWarehouse::Hud::Client).to receive(:consent_validity_period).and_return(1.year)
-      end
+    context 'with different release statuses' do
+      [
+        {
+          scenario: 'full release',
+          release_valid: true,
+          revoked_consent: false,
+          partial_release: false,
+          expected_status: 'full',
+        },
+        {
+          scenario: 'revoked consent',
+          release_valid: false,
+          revoked_consent: true,
+          partial_release: false,
+          expected_status: 'revoked',
+        },
+        {
+          scenario: 'partial release',
+          release_valid: false,
+          revoked_consent: false,
+          partial_release: true,
+          expected_status: 'partial',
+        },
+      ].each do |test_case|
+        context "when client has #{test_case[:scenario]}" do
+          before do
+            allow(client).to receive(:release_valid?).and_return(test_case[:release_valid])
+            allow(client).to receive(:revoked_consent?).and_return(test_case[:revoked_consent])
+            allow(client).to receive(:partial_release?).and_return(test_case[:partial_release])
+          end
 
-      it 'returns date one year from signing' do
-        expect(task.send(:roi_expiry_date, client)).to eq(client.consent_form_signed_on + 1.year)
+          it 'returns correct status' do
+            expect(processed_result[:status]).to eq(test_case[:expected_status])
+          end
+        end
       end
     end
+  end
 
-    context 'when release duration is Use Expiration Date' do
-      before do
-        allow(GrdaWarehouse::Hud::Client).to receive(:release_duration).and_return('Use Expiration Date')
-        client.consent_expires_on = Date.current + 6.months
-      end
+  describe '#roi_expiry_date' do
+    let(:client) { create(:hud_client, consent_form_signed_on: today) }
+    subject(:expiry_date) { task.send(:roi_expiry_date, client) }
 
-      it 'returns the explicit expiration date' do
-        expect(task.send(:roi_expiry_date, client)).to eq(client.consent_expires_on)
-      end
+    context 'with one year duration' do
+      include_context 'with release duration settings', 'One Year', 1.year
+
+      it { is_expected.to eq(client.consent_form_signed_on + 1.year) }
     end
 
-    context 'when release duration is Indefinite' do
-      before do
-        allow(GrdaWarehouse::Hud::Client).to receive(:release_duration).and_return('Indefinite')
-      end
+    context 'with explicit expiration date' do
+      include_context 'with release duration settings', 'Use Expiration Date'
+
+      before { client.consent_expires_on = today + 6.months }
+
+      it { is_expected.to eq(client.consent_expires_on) }
+    end
+
+    context 'with indefinite duration' do
+      include_context 'with release duration settings', 'Indefinite'
+
+      it { is_expected.to be_nil }
+    end
+
+    context 'with invalid duration' do
+      include_context 'with release duration settings', 'Invalid Duration'
 
-      it 'returns nil' do
-        expect(task.send(:roi_expiry_date, client)).to be_nil
+      it 'raises an error' do
+        expect { expiry_date }.to raise_error(/unknown release duration/)
       end
     end
   end