Batch parent relationship removal for improved performance

Previously, bulk parent removal processed all relationships in a single
job, which could timeout for large imports.

Now the form enqueues multiple jobs, each processing a batch of 100
parent relationships. Each batch job:
- Identifies parent relationships to remove
- Invalidates associated consents (if remove_option is "all")
- Destroys the identified relationships
- Cleans up orphaned parents
- Checks if all jobs are complete and updates import status

Author: murugapl

app/controllers/imports/bulk_remove_parents_controller.rb

@@ -51,14 +51,7 @@ def set_import
     end
 
     def set_consents
-      @consents =
-        Consent
-          .includes(patient: { parent_relationships: :parent })
-          .joins(patient: :parent_relationships)
-          .merge(@import.patients)
-          .merge(@import.parent_relationships)
-          .where("consents.parent_id = parent_relationships.parent_id")
-          .not_invalidated
+      @consents = @import.parent_relationship_consents
     end
 
     def success_flash_text

app/forms/bulk_remove_parents_form.rb

@@ -4,6 +4,8 @@ class BulkRemoveParentsForm
   include ActiveModel::Model
   include ActiveModel::Attributes
 
+  BATCH_SIZE = 100
+
   attr_accessor :current_user
   attr_reader :import, :consents
 
@@ -25,12 +27,18 @@ def initialize(import:, consents:, current_user:, **attributes)
   def save!
     return false unless valid?
     import.update!(status: :removing_parent_relationships)
-    BulkRemoveParentRelationshipsJob.perform_later(
-      import.to_global_id.to_s,
-      consents.map(&:id),
-      current_user.id,
-      remove_option
-    )
+
+    import
+      .parent_relationship_ids
+      .each_slice(BATCH_SIZE) do |batch_ids|
+        BulkRemoveParentRelationshipsJob.perform_later(
+          import.to_global_id.to_s,
+          batch_ids,
+          current_user.id,
+          remove_option
+        )
+      end
+
     true
   end
 

app/jobs/bulk_remove_parent_relationships_job.rb

@@ -3,19 +3,71 @@
 class BulkRemoveParentRelationshipsJob < ApplicationJob
   queue_as :imports
 
-  def perform(import_global_id, consent_ids, user_id, remove_option)
+  def perform(
+    import_global_id,
+    parent_relationship_ids_batch,
+    user_id,
+    remove_option
+  )
     import = GlobalID::Locator.locate(import_global_id)
-    consents = Consent.where(id: consent_ids)
-
-    if remove_option == "unconsented_only"
-      import.destroy_parent_relationships_without_consent!(consents)
-    else
-      import.destroy_parent_relationships_and_invalidate_consents!(
-        User.find(user_id),
-        consents
-      )
+    user = User.find(user_id)
+
+    ActiveRecord::Base.transaction do
+      parent_relationships =
+        import
+          .parent_relationships
+          .where(id: parent_relationship_ids_batch)
+          .includes(:parent, :patient)
+
+      consents =
+        import.parent_relationship_consents(scope: parent_relationships)
+
+      parent_relationships_to_remove =
+        if remove_option == "unconsented_only"
+          consented_pairs = consents.pluck(:patient_id, :parent_id).to_set
+
+          parent_relationships.reject do |pr|
+            consented_pairs.include?([pr.patient_id, pr.parent_id])
+          end
+        else
+          parent_relationships
+        end
+
+      invalidate_consents!(user, consents) if remove_option == "all"
+
+      parents_to_check = parent_relationships_to_remove.map(&:parent)
+      patient_ids = parent_relationships_to_remove.map(&:patient_id)
+
+      parent_relationships_to_remove.each(&:destroy!)
+
+      parents_to_check.each do |parent|
+        next if parent.destroyed?
+        next if parent.parent_relationships.exists?
+        next if parent.consents.exists?
+
+        parent.destroy!
+      end
+
+      StatusUpdaterJob.perform_bulk(patient_ids.zip)
     end
 
+    mark_complete_if_finished(import, remove_option)
+  end
+
+  private
+
+  def invalidate_consents!(user, consents)
+    timestamp = Time.current.to_fs(:long)
+    invalidation_note =
+      "Consent invalidated on #{timestamp} " \
+        "because #{user.full_name} removed all parent-child relationships from an import."
+
+    consents.update_all(notes: invalidation_note, invalidated_at: Time.current)
+  end
+
+  def mark_complete_if_finished(import, remove_option)
+    return unless import.remaining_parent_relationships(remove_option:).empty?
+
     import.update!(status: :processed)
   end
 end

app/models/patient_import.rb

@@ -128,61 +128,23 @@ def commit_changesets(changesets)
     end
   end
 
-  def destroy_parent_relationships_and_invalidate_consents!(
-    current_user,
-    consents
-  )
-    ActiveRecord::Base.transaction do
-      parent_relationships =
-        self.parent_relationships.includes(:parent, :patient)
-
-      timestamp = Time.current.to_fs(:long)
-      user_name = current_user&.full_name || "system"
-      invalidation_note =
-        "Consent invalidated on #{timestamp} " \
-          "because #{user_name} removed all parent-child relationships from an import."
-
-      # Invalidate consents first before destroying parent relationships, which
-      # will make consents return an empty set.
-      consents.update_all(
-        notes: invalidation_note,
-        invalidated_at: Time.current
-      )
-
-      patient_ids_to_update = consents.map(&:patient_id).uniq
-
-      parents_to_check = parent_relationships.map(&:parent)
-
-      parent_relationships.destroy_all
-
-      parents_to_check.each do |parent|
-        if !parent.destroyed? && parent.parent_relationships.empty? &&
-             parent.consents.empty?
-          parent.destroy!
-        end
-      end
-
-      StatusUpdaterJob.perform_bulk(patient_ids_to_update.zip)
+  def remaining_parent_relationships(remove_option:)
+    if remove_option == "unconsented_only"
+      parent_relationships -
+        parent_relationship_consents.map(&:parent_relationship)
+    else
+      parent_relationships
     end
   end
 
-  def destroy_parent_relationships_without_consent!(consents)
-    ActiveRecord::Base.transaction do
-      parent_relationships_without_consents =
-        parent_relationships.includes(:parent, :patient) -
-          consents.map(&:parent_relationship).uniq
-
-      parents_to_check = parent_relationships_without_consents.map(&:parent)
-
-      parent_relationships_without_consents.each(&:destroy!)
-
-      parents_to_check.each do |parent|
-        if !parent.destroyed? && parent.parent_relationships.empty? &&
-             parent.consents.empty?
-          parent.destroy!
-        end
-      end
-    end
+  def parent_relationship_consents(scope: parent_relationships)
+    Consent
+      .includes(patient: { parent_relationships: :parent })
+      .joins(patient: :parent_relationships)
+      .merge(patients)
+      .merge(scope)
+      .where("consents.parent_id = parent_relationships.parent_id")
+      .not_invalidated
   end
 
   private

spec/features/bulk_remove_parents_spec.rb

@@ -181,10 +181,9 @@ def then_i_should_see_the_parent_relationships_without_associated_consents_are_b
   end
 
   def and_the_bulk_remove_job_should_be_enqueued_with_remove_option(option)
-    consent_ids = @consent ? [@consent.id] : []
     expect(BulkRemoveParentRelationshipsJob).to have_been_enqueued.with(
       @class_import.to_global_id.to_s,
-      consent_ids,
+      @class_import.parent_relationship_ids,
       @user.id,
       option
     )

spec/jobs/bulk_remove_parent_relationships_job_spec.rb

@@ -4,7 +4,7 @@
   subject(:perform_job) do
     described_class.new.perform(
       import.to_global_id.to_s,
-      consents.map(&:id),
+      import.parent_relationship_ids,
       user.id,
       remove_option
     )
@@ -17,26 +17,21 @@
 
   let(:user) { create(:user, team:) }
 
-  let(:consents) do
-    [
-      create(
-        :consent,
-        :given,
-        parent: import.parent_relationships.includes(:parent).first.parent,
-        patient: import.parent_relationships.includes(:patient).first.patient
-      ),
-      create(
-        :consent,
-        :refused,
-        parent: import.parent_relationships.includes(:parent).second.parent,
-        patient: import.parent_relationships.includes(:patient).second.patient
-      )
-    ]
-  end
-
   before do
     import.process!
     CommitImportJob.drain
+    create(
+      :consent,
+      :given,
+      parent: import.parent_relationships.includes(:parent).first.parent,
+      patient: import.parent_relationships.includes(:patient).first.patient
+    )
+    create(
+      :consent,
+      :refused,
+      parent: import.parent_relationships.includes(:parent).second.parent,
+      patient: import.parent_relationships.includes(:patient).second.patient
+    )
   end
 
   describe "#perform" do
@@ -68,7 +63,7 @@
 
       it "invalidates consents" do
         perform_job
-        consents.each { |consent| expect(consent.reload).to be_invalidated }
+        Consent.all.find_each { expect(it.reload).to be_invalidated }
       end
 
       it "updates import status" do