exclude subjects still linked to set_member_subject in subject_remover (

#4306)

* exclude subjects still linked to set_member_subject in subject_remover

* don't call cleanup if subject is linked to other subject sets in SubjectRemoverWorker

* hound cleanups

* hound cleanups

* implement fix on remover.rb, add new test to remover_spec

* hound cleanups

* pr review: remove type error in remover.rb and stub response properly

* more pr cleanups

app/controllers/api/v1/subject_sets_controller.rb

@@ -60,7 +60,7 @@ def destroy
 
     reset_workflow_retired_counts(affected_workflow_ids)
     subject_ids.each_with_index do |subject_id, index|
-      SubjectRemovalWorker.perform_in(index.seconds, subject_id)
+      SubjectRemovalWorker.perform_in(index.seconds, subject_id, controlled_resource.id)
     end
 
     deleted_resource_response

app/workers/subject_removal_worker.rb

@@ -5,9 +5,9 @@ class SubjectRemovalWorker
 
   sidekiq_options queue: :data_low
 
-  def perform(subject_id)
+  def perform(subject_id, subject_set_id=nil)
     return unless Flipper.enabled?(:remove_orphan_subjects)
 
-    Subjects::Remover.new(subject_id).cleanup
+    Subjects::Remover.new(subject_id, nil, subject_set_id).cleanup
   end
 end

lib/subjects/remover.rb

@@ -1,10 +1,11 @@
 module Subjects
   class Remover
-    attr_reader :subject_id, :panoptes_client
+    attr_reader :subject_id, :panoptes_client, :subject_set_id
 
-    def initialize(subject_id, client=nil)
+    def initialize(subject_id, client=nil, subject_set_id=nil)
       @subject_id = subject_id
       @panoptes_client = client || Panoptes::Client.new(env: Rails.env)
+      @subject_set_id = subject_set_id
     end
 
     def cleanup
@@ -29,6 +30,8 @@ def cleanup
     private
 
     def can_be_removed?
+      return false if belongs_to_other_subject_set?
+
       return false if has_been_collected_or_classified?
 
       return false if has_been_talked_about?
@@ -56,6 +59,12 @@ def has_been_collected_or_classified?
       !orphan_subject
     end
 
+    def belongs_to_other_subject_set?
+      return false if subject_set_id.nil?
+
+      orphan_subject.set_member_subjects.where.not(subject_set_id: subject_set_id).count.positive?
+    end
+
     def has_been_talked_about?
       panoptes_client.discussions(
         focus_id: subject_id,

spec/controllers/api/v1/subject_sets_controller_spec.rb

@@ -410,13 +410,13 @@
 
     it 'calls the subject removal worker' do
       subject_set.subjects.each_with_index do |s, index|
-        allow(SubjectRemovalWorker).to receive(:perform_in).with(index.seconds, s.id)
+        allow(SubjectRemovalWorker).to receive(:perform_in).with(index.seconds, s.id, subject_set.id)
       end
       delete_resources
       subject_set.subjects.each_with_index do |s, index|
         expect(SubjectRemovalWorker)
           .to have_received(:perform_in)
-          .with(index.seconds, s.id)
+          .with(index.seconds, s.id, subject_set.id)
       end
     end
   end

spec/lib/subjects/remover_spec.rb

@@ -105,6 +105,17 @@
         expect { SetMemberSubject.find(sms_ids) }.to raise_error(ActiveRecord::RecordNotFound)
       end
 
+      context 'with multiple subject sets' do
+        let(:alternate_subject_set) { create(:subject_set) }
+        let(:remover) { described_class.new(subject.id, panoptes_client, alternate_subject_set.id) }
+        let(:new_sms) { create(:set_member_subject, subject: subject, subject_set: alternate_subject_set) }
+
+        it 'does not remove subjects associated with multiple set_member_subjects' do
+          remover.cleanup
+          expect(Subject.where(id: subject.id)).to exist
+        end
+      end
+
       it "should remove the associated media resources" do
         locations = subjects.map { |s| create(:medium, linked: s) }
         media_ids = subject.reload.locations.map(&:id)

spec/workers/subject_removal_worker_spec.rb

@@ -3,18 +3,60 @@
 RSpec.describe SubjectRemovalWorker do
   let(:subject_id) { 1 }
   let(:feature_name) { "remove_orphan_subjects" }
-  let(:remover) { instance_double(Subjects::Remover) }
+  let(:panoptes_client) { instance_double(Panoptes::Client) }
+  let(:subject_remover) { described_class.new }
+  let(:remover) { instance_double(Subjects::Remover, panoptes_client: panoptes_client) }
 
-  it 'should call the orphan remover cleanup when enabled' do
-    Flipper.enable(feature_name)
-    expect(Subjects::Remover).to receive(:new).with(subject_id).and_return(remover)
-    expect(remover).to receive(:cleanup)
-    subject.perform(subject_id)
+  describe 'flipper enabled' do
+    let(:project) { create(:project) }
+    let!(:subjects) { create_list(:subject, 2) }
+    let!(:subject_sets) { create_list(:subject_set, 2, project: project) }
+    let!(:subject_set) { create(:subject_set, project: project) }
+    let(:discussions_url) { 'https://talk-staging.zooniverse.org/discussions' }
+    let!(:first_subject) { subjects.first }
+    let!(:second_subject) { subjects.second }
+
+    def stub_discussions_request(subject_id)
+      stub_request(:get, discussions_url)
+        .with(query: { focus_id: subject_id, focus_type: 'Subject' })
+        .to_return(status: 200, body: '{"discussions": []}', headers: {})
+    end
+
+    before do
+      Flipper.enable(feature_name)
+    end
+
+    context 'when running orphan remover cleanup function' do
+      it 'calls the orphan remover cleanup when enabled' do
+        allow(Subjects::Remover).to receive(:new).with(subject_id, nil, nil).and_return(remover)
+        allow(remover).to receive(:cleanup)
+        subject_remover.perform(subject_id)
+        expect(Subjects::Remover).to have_received(:new)
+        expect(remover).to have_received(:cleanup)
+      end
+    end
+
+    context 'when deleting subjects' do
+      it 'deletes subject not associated with set_member_subject' do
+        stub_discussions_request(first_subject.id)
+        subject_remover.perform(first_subject.id, subject_set.id)
+        expect(Subject.where(id: first_subject.id)).not_to exist
+      end
+
+      it 'does not delete subjects associated with another set_member_subject' do
+        create(:set_member_subject, subject: second_subject, subject_set: subject_sets.first)
+        create(:set_member_subject, subject: second_subject, subject_set: subject_sets.last)
+        stub_discussions_request(second_subject.id)
+        subject_remover.perform(second_subject.id, subject_sets.first.id)
+        expect(Subject.where(id: second_subject.id)).to exist
+        expect(SetMemberSubject.where(subject_set_id: subject_sets.first.id, subject_id: second_subject.id)).to exist
+      end
+    end
   end
 
   it 'should not call the orphan remover cleanup when disabled' do
-    expect(Subjects::Remover).not_to receive(:new)
+    allow(Subjects::Remover).to receive(:new)
+    subject_remover.perform(subject_id)
     expect(remover).not_to receive(:cleanup)
-    subject.perform(subject_id)
   end
 end