#1276, Refactor AsyncBatchDestroyJob to increase efficiency and resil…

…ience

The AsyncBatchDestroyJob has been refactored to utilise Rails's `find_in_batches`
method as opposed to a custom SQL query. This proves to make the process of record
deletion both more efficient and more resilient to database inconsistencies.

Previously, records that could not be deleted due to dependencies would raise an
exception, breaking the whole transaction. Now, these records are logged and
skipped over instead. Updates to associated tests have been made to verify and
reflect these changes.

app/jobs/async_batch_destroy_job.rb

@@ -11,18 +11,28 @@ def perform(model_class, sql_query, who_is)
     @model_class = model_class.constantize
     @who_is = who_is
     set_audit_log_data
-    begin
-      scoped_records = @model_class.find_by_sql(sql_query + " LIMIT #{BATCH_SIZE}")
-      @model_class.transaction do
-        scoped_records.each do |record|
-          raise ActiveRecord::RecordNotDestroyed.new(error_message_for(record), record) unless record.destroy
-        end
+
+    @model_class.where(id: record_ids(sql_query)).find_in_batches(batch_size: BATCH_SIZE) do |records|
+      records.each do |record|
+        delete_record(record)
       end
-    end until scoped_records.empty?
+    end
+  end
+
+  def record_ids(sql_query)
+    @model_class.find_by_sql(sql_query).pluck(:id)
+  end
+
+  def delete_record(record)
+    unless record.destroy
+      Rails.logger.warn "#{@model_class} ##{record.id} can't be deleted: #{record.errors.full_messages.to_sentence}"
+    end
+  rescue ActiveRecord::InvalidForeignKey => e
+    Rails.logger.error error_message_for(id: record.id, error_class: e.class)
   end
 
-  def error_message_for(record)
-    "#{model_class} ##{record.id} can't be deleted: #{record.errors.full_messages.to_sentence}"
+  def error_message_for(id:, error_class:)
+    "#{@model_class} ##{id} can't be deleted: #{error_class}"
   end
 
   def reschedule_at(_time, _attempts)

spec/jobs/async_batch_destroy_job_spec.rb

@@ -64,33 +64,65 @@
           let(:sql_query) { Contractor.all.to_sql }
           it 'error performed job' do
             expect do
-              expect { subject }.to raise_error(ActiveRecord::RecordNotDestroyed)
+              expect { subject }.not_to raise_error
             end.to change(LogicLog, :count).by 1
-            expect(LogicLog.last.msg).to start_with 'Error'
+            expect(LogicLog.last.msg).to start_with 'Success'
           end
         end
       end
 
       context 'when record cannot be destroyed' do
         let(:model_class) { 'Dialpeer' }
 
-        let!(:dialpeers) { FactoryBot.create_list(:dialpeer, 4) }
+        let!(:dialpeer_with_pricelist) { FactoryBot.create(:dialpeer) }
+        let!(:dialpeer) { FactoryBot.create(:dialpeer) }
         let!(:item) do
-          FactoryBot.create(:rate_management_pricelist_item, :with_pricelist, :filed_from_project, dialpeer: dialpeers.last)
+          FactoryBot.create(:rate_management_pricelist_item, :with_pricelist, :filed_from_project, dialpeer: dialpeer_with_pricelist)
         end
         let(:sql_query) { Dialpeer.all.to_sql }
+        let(:error_message) { "Dialpeer ##{dialpeer_with_pricelist.id} can't be deleted: Can't be deleted because linked to not applied Rate Management Pricelist(s) ##{item.pricelist_id}" }
+        let(:rails_logger) { Rails.logger }
 
-        it 'should raise validation error' do
-          error_message = "Dialpeer ##{dialpeers.last.id} can't be deleted: Can't be deleted because linked to not applied Rate Management Pricelist(s) ##{item.pricelist_id}"
-          expect { subject }.to raise_error(ActiveRecord::RecordNotDestroyed, error_message)
+        before do
+          allow(Rails).to receive(:logger).and_return(rails_logger)
+          allow(rails_logger).to receive(:info)
+        end
 
-          dialpeers.first(2).each do |dialpeer|
-            expect(Dialpeer).not_to be_exists(dialpeer.id)
-          end
+        it 'should delete dialpeer that do not have related pricelist' do
+          expect { subject }.not_to raise_error
+          expect { dialpeer_with_pricelist.reload }.not_to raise_error ActiveRecord::RecordNotFound
+          expect { dialpeer.reload }.to raise_error ActiveRecord::RecordNotFound
+        end
 
-          dialpeers.last(2).each do |dialpeer|
-            expect(Dialpeer).to be_exists(dialpeer.id)
-          end
+        it 'should log error through Rails logger' do
+          expect(rails_logger).to receive(:warn).once.and_wrap_original { |_m, arg| expect(arg).to eq error_message }
+          subject
+        end
+      end
+
+      context 'when Routing::Numberlist has related customer_auth' do
+        let(:model_class) { 'Routing::Numberlist' }
+        let(:sql_query) { Routing::Numberlist.where(id: [record_that_cant_be_deleted.id, record_that_can_be_deleted.id]).to_sql }
+        let!(:record_that_cant_be_deleted) { FactoryBot.create(:numberlist) }
+        let!(:record_that_can_be_deleted) { FactoryBot.create(:numberlist) }
+        let!(:customers_auth) { FactoryBot.create(:customers_auth, src_numberlist: record_that_cant_be_deleted) }
+
+        it 'should remove number_lists without related Customer Auth' do
+          subject
+
+          expect { record_that_can_be_deleted.reload }.to raise_error ActiveRecord::RecordNotFound
+        end
+      end
+
+      context 'when NumberList already deleted' do
+        let(:model_class) { 'Routing::Numberlist' }
+        let(:sql_query) { Routing::Numberlist.all.to_sql }
+        let!(:record) { FactoryBot.create(:numberlist) }
+
+        before { record.destroy! }
+
+        it 'should perform job without error' do
+          expect { subject }.not_to raise_error
         end
       end
     end

spec/models/routing/numberlist_spec.rb

@@ -149,4 +149,17 @@
       end
     end
   end
+
+  describe '.destroy!' do
+    subject { record.destroy! }
+
+    context 'when record has related customer_auth' do
+      let!(:record) { FactoryBot.create(:numberlist) }
+      let!(:customers_auth) { FactoryBot.create(:customers_auth, src_numberlist: record) }
+
+      it 'should destroy record' do
+        expect { subject }.to change(described_class, :count).by(-1)
+      end
+    end
+  end
 end