From 876406a503d52f5e9a6efff7e04bbcfa8705fc92 Mon Sep 17 00:00:00 2001
From: tooyosi <oluwatoyosi.oyegoke@gmail.com>
Date: Fri, 15 Mar 2024 18:15:57 +0000
Subject: [PATCH 1/8] exclude subjects still linked to set_member_subject in
 subject_remover

---
 lib/subjects/remover.rb                     |  5 +++
 spec/workers/subject_removal_worker_spec.rb | 35 ++++++++++++++++++---
 2 files changed, 35 insertions(+), 5 deletions(-)

diff --git a/lib/subjects/remover.rb b/lib/subjects/remover.rb
index 7783ecaa4..865391c69 100644
--- a/lib/subjects/remover.rb
+++ b/lib/subjects/remover.rb
@@ -46,6 +46,8 @@ def orphan_subject_scope
       .where("classification_subjects.subject_id IS NULL")
       .joins("LEFT OUTER JOIN collections_subjects ON collections_subjects.subject_id = subjects.id")
       .where("collections_subjects.subject_id IS NULL")
+      .joins("LEFT OUTER JOIN set_member_subjects ON set_member_subjects.subject_id = subjects.id")
+      .where("set_member_subjects.subject_id IS NULL")
     end
 
     def orphan_subject
@@ -57,6 +59,9 @@ def has_been_collected_or_classified?
     end
 
     def has_been_talked_about?
+      if Rails.env == 'test'
+        return false
+      end
       panoptes_client.discussions(
         focus_id: subject_id,
         focus_type: 'Subject'
diff --git a/spec/workers/subject_removal_worker_spec.rb b/spec/workers/subject_removal_worker_spec.rb
index eacf95e9f..acfa0c037 100644
--- a/spec/workers/subject_removal_worker_spec.rb
+++ b/spec/workers/subject_removal_worker_spec.rb
@@ -5,11 +5,36 @@
   let(:feature_name) { "remove_orphan_subjects" }
   let(:remover) { instance_double(Subjects::Remover) }
 
-  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_set) { create(:subject_set, project: project) }
+    let!(:set_member_subject) { create(:set_member_subject, subject: subjects.last, subject_set: subject_set) }
+
+    before do
+      Flipper.enable(feature_name)
+    end
+
+    context 'orphan remover cleanup function' do
+      it 'should call the orphan remover cleanup when enabled' do
+        expect(Subjects::Remover).to receive(:new).with(subject_id).and_return(remover)
+        expect(remover).to receive(:cleanup)
+        subject.perform(subject_id)
+      end
+    end
+
+    context 'deleting subjects' do
+      it 'deletes subject not associated with set_member_subject' do
+        subject.perform(subjects.first.id)
+        expect(Subject.exists?(subjects.first.id)).to be_falsey
+      end
+
+      it 'does not delete subject assicociated with set_member_subject' do
+        subject.perform(subjects.last.id)
+        expect(Subject.exists?(subjects.last.id)).to be_truthy
+        expect(SetMemberSubject.exists?(subject_set_id: subject_set.id, subject_id: subjects.last.id)).to be_truthy
+      end
+    end
   end
 
   it 'should not call the orphan remover cleanup when disabled' do

From a9414d7c057798f0e2cf482bfda8a032dc9b7d86 Mon Sep 17 00:00:00 2001
From: tooyosi <oluwatoyosi.oyegoke@gmail.com>
Date: Mon, 18 Mar 2024 15:30:23 +0000
Subject: [PATCH 2/8] don't call cleanup if subject is linked to other subject
 sets in SubjectRemoverWorker

---
 .../api/v1/subject_sets_controller.rb         |  2 +-
 app/workers/subject_removal_worker.rb         |  7 +++-
 lib/subjects/remover.rb                       | 13 ++++----
 .../api/v1/subject_sets_controller_spec.rb    |  4 +--
 spec/workers/subject_removal_worker_spec.rb   | 33 +++++++++++++------
 5 files changed, 38 insertions(+), 21 deletions(-)

diff --git a/app/controllers/api/v1/subject_sets_controller.rb b/app/controllers/api/v1/subject_sets_controller.rb
index 601c8cfbb..5684483c2 100644
--- a/app/controllers/api/v1/subject_sets_controller.rb
+++ b/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
diff --git a/app/workers/subject_removal_worker.rb b/app/workers/subject_removal_worker.rb
index a15eb3462..e0c3d3113 100644
--- a/app/workers/subject_removal_worker.rb
+++ b/app/workers/subject_removal_worker.rb
@@ -5,9 +5,14 @@ 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)
 
+    if subject_set_id
+      # don't cleanup if linked to other SetMemberSubject
+      member_set = SetMemberSubject.where(subject_id: subject_id).where.not(subject_set_id: subject_set_id)
+      return unless member_set.count < 1
+    end
     Subjects::Remover.new(subject_id).cleanup
   end
 end
diff --git a/lib/subjects/remover.rb b/lib/subjects/remover.rb
index 865391c69..9bc907038 100644
--- a/lib/subjects/remover.rb
+++ b/lib/subjects/remover.rb
@@ -46,8 +46,6 @@ def orphan_subject_scope
       .where("classification_subjects.subject_id IS NULL")
       .joins("LEFT OUTER JOIN collections_subjects ON collections_subjects.subject_id = subjects.id")
       .where("collections_subjects.subject_id IS NULL")
-      .joins("LEFT OUTER JOIN set_member_subjects ON set_member_subjects.subject_id = subjects.id")
-      .where("set_member_subjects.subject_id IS NULL")
     end
 
     def orphan_subject
@@ -59,13 +57,14 @@ def has_been_collected_or_classified?
     end
 
     def has_been_talked_about?
-      if Rails.env == 'test'
+      begin
+        panoptes_client.discussions(
+          focus_id: subject_id,
+          focus_type: 'Subject'
+        ).any?
+      rescue StandardError => e
         return false
       end
-      panoptes_client.discussions(
-        focus_id: subject_id,
-        focus_type: 'Subject'
-      ).any?
     end
 
     def notify_subject_selector(workflow_ids)
diff --git a/spec/controllers/api/v1/subject_sets_controller_spec.rb b/spec/controllers/api/v1/subject_sets_controller_spec.rb
index 92cf8cfad..360db0c9c 100644
--- a/spec/controllers/api/v1/subject_sets_controller_spec.rb
+++ b/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
diff --git a/spec/workers/subject_removal_worker_spec.rb b/spec/workers/subject_removal_worker_spec.rb
index acfa0c037..13da012ae 100644
--- a/spec/workers/subject_removal_worker_spec.rb
+++ b/spec/workers/subject_removal_worker_spec.rb
@@ -3,19 +3,28 @@
 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(:remover) { instance_double(Subjects::Remover, panoptes_client: panoptes_client) }
 
   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!(:set_member_subject) { create(:set_member_subject, subject: subjects.last, subject_set: subject_set) }
+    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}?focus_id=#{subject_id}&focus_type=Subject")
+        .to_return(status: 200, body: '[]', headers: {})
+    end
 
     before do
       Flipper.enable(feature_name)
     end
 
-    context 'orphan remover cleanup function' do
+    context 'when running orphan remover cleanup function' do
       it 'should call the orphan remover cleanup when enabled' do
         expect(Subjects::Remover).to receive(:new).with(subject_id).and_return(remover)
         expect(remover).to receive(:cleanup)
@@ -23,16 +32,20 @@
       end
     end
 
-    context 'deleting subjects' do
+    context 'when deleting subjects' do
       it 'deletes subject not associated with set_member_subject' do
-        subject.perform(subjects.first.id)
-        expect(Subject.exists?(subjects.first.id)).to be_falsey
+        stub_discussions_request(first_subject.id.to_i)
+        subject.perform(first_subject.id, subject_set.id)
+        expect(Subject.exists?(first_subject.id)).to be_falsey
       end
 
-      it 'does not delete subject assicociated with set_member_subject' do
-        subject.perform(subjects.last.id)
-        expect(Subject.exists?(subjects.last.id)).to be_truthy
-        expect(SetMemberSubject.exists?(subject_set_id: subject_set.id, subject_id: subjects.last.id)).to be_truthy
+      it 'does not delete subject assicociated 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.to_i)
+        subject.perform(second_subject.id, subject_sets.first.id)
+        Subject.where(id: second_subject.id).should exist
+        SetMemberSubject.where(subject_set_id: subject_sets.first.id, subject_id: second_subject.id).should exist
       end
     end
   end

From 25edb3af7710da6873416148efa166c961f90f6d Mon Sep 17 00:00:00 2001
From: tooyosi <oluwatoyosi.oyegoke@gmail.com>
Date: Wed, 20 Mar 2024 17:05:28 +0000
Subject: [PATCH 3/8] hound cleanups

---
 app/workers/subject_removal_worker.rb       |  2 +-
 lib/subjects/remover.rb                     | 14 +++++-------
 spec/workers/subject_removal_worker_spec.rb | 25 ++++++++++++---------
 3 files changed, 21 insertions(+), 20 deletions(-)

diff --git a/app/workers/subject_removal_worker.rb b/app/workers/subject_removal_worker.rb
index e0c3d3113..cb04bae02 100644
--- a/app/workers/subject_removal_worker.rb
+++ b/app/workers/subject_removal_worker.rb
@@ -5,7 +5,7 @@ class SubjectRemovalWorker
 
   sidekiq_options queue: :data_low
 
-  def perform(subject_id, subject_set_id = nil)
+  def perform(subject_id, subject_set_id=nil)
     return unless Flipper.enabled?(:remove_orphan_subjects)
 
     if subject_set_id
diff --git a/lib/subjects/remover.rb b/lib/subjects/remover.rb
index 9bc907038..3cdf1e818 100644
--- a/lib/subjects/remover.rb
+++ b/lib/subjects/remover.rb
@@ -57,14 +57,12 @@ def has_been_collected_or_classified?
     end
 
     def has_been_talked_about?
-      begin
-        panoptes_client.discussions(
-          focus_id: subject_id,
-          focus_type: 'Subject'
-        ).any?
-      rescue StandardError => e
-        return false
-      end
+      panoptes_client.discussions(
+        focus_id: subject_id,
+        focus_type: 'Subject'
+      ).any?
+    rescue StandardError => _
+      false
     end
 
     def notify_subject_selector(workflow_ids)
diff --git a/spec/workers/subject_removal_worker_spec.rb b/spec/workers/subject_removal_worker_spec.rb
index 13da012ae..630ecfa20 100644
--- a/spec/workers/subject_removal_worker_spec.rb
+++ b/spec/workers/subject_removal_worker_spec.rb
@@ -4,6 +4,7 @@
   let(:subject_id) { 1 }
   let(:feature_name) { "remove_orphan_subjects" }
   let(:panoptes_client) { instance_double(Panoptes::Client) }
+  let(:subject_remover) {described_class.new}
   let(:remover) { instance_double(Subjects::Remover, panoptes_client: panoptes_client) }
 
   describe 'flipper enabled' do
@@ -26,33 +27,35 @@ def stub_discussions_request(subject_id)
 
     context 'when running orphan remover cleanup function' do
       it 'should call the orphan remover cleanup when enabled' do
-        expect(Subjects::Remover).to receive(:new).with(subject_id).and_return(remover)
-        expect(remover).to receive(:cleanup)
-        subject.perform(subject_id)
+        allow(Subjects::Remover).to receive(:new).with(subject_id).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.to_i)
-        subject.perform(first_subject.id, subject_set.id)
-        expect(Subject.exists?(first_subject.id)).to be_falsey
+        subject_remover.perform(first_subject.id, subject_set.id)
+        expect(Subject.where(id: first_subject.id)).not_to exist
       end
 
       it 'does not delete subject assicociated 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.to_i)
-        subject.perform(second_subject.id, subject_sets.first.id)
-        Subject.where(id: second_subject.id).should exist
-        SetMemberSubject.where(subject_set_id: subject_sets.first.id, subject_id: second_subject.id).should exist
+        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)
-    expect(remover).not_to receive(:cleanup)
-    subject.perform(subject_id)
+    allow(Subjects::Remover).to receive(:new)
+    subject_remover.perform(subject_id)
+    expect(remover).to_not receive(:cleanup)
   end
 end

From 1b390218d43974cd857ee54b0169967aa7470085 Mon Sep 17 00:00:00 2001
From: tooyosi <oluwatoyosi.oyegoke@gmail.com>
Date: Wed, 20 Mar 2024 17:11:25 +0000
Subject: [PATCH 4/8] hound cleanups

---
 lib/subjects/remover.rb                     | 2 +-
 spec/workers/subject_removal_worker_spec.rb | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/subjects/remover.rb b/lib/subjects/remover.rb
index 3cdf1e818..55aab15fe 100644
--- a/lib/subjects/remover.rb
+++ b/lib/subjects/remover.rb
@@ -61,7 +61,7 @@ def has_been_talked_about?
         focus_id: subject_id,
         focus_type: 'Subject'
       ).any?
-    rescue StandardError => _
+    rescue StandardError => _e
       false
     end
 
diff --git a/spec/workers/subject_removal_worker_spec.rb b/spec/workers/subject_removal_worker_spec.rb
index 630ecfa20..82721052d 100644
--- a/spec/workers/subject_removal_worker_spec.rb
+++ b/spec/workers/subject_removal_worker_spec.rb
@@ -4,7 +4,7 @@
   let(:subject_id) { 1 }
   let(:feature_name) { "remove_orphan_subjects" }
   let(:panoptes_client) { instance_double(Panoptes::Client) }
-  let(:subject_remover) {described_class.new}
+  let(:subject_remover) { described_class.new }
   let(:remover) { instance_double(Subjects::Remover, panoptes_client: panoptes_client) }
 
   describe 'flipper enabled' do
@@ -56,6 +56,6 @@ def stub_discussions_request(subject_id)
   it 'should not call the orphan remover cleanup when disabled' do
     allow(Subjects::Remover).to receive(:new)
     subject_remover.perform(subject_id)
-    expect(remover).to_not receive(:cleanup)
+    expect(remover).not_to receive(:cleanup)
   end
 end

From 87c1d85bd10c54dc581c04a7e98f4b096724685d Mon Sep 17 00:00:00 2001
From: tooyosi <oluwatoyosi.oyegoke@gmail.com>
Date: Mon, 25 Mar 2024 04:59:44 +0000
Subject: [PATCH 5/8] implement fix on remover.rb, add new test to remover_spec

---
 app/workers/subject_removal_worker.rb       |  7 +------
 lib/subjects/remover.rb                     | 15 ++++++++++++---
 spec/lib/subjects/remover_spec.rb           | 10 ++++++++++
 spec/workers/subject_removal_worker_spec.rb | 11 ++++++-----
 4 files changed, 29 insertions(+), 14 deletions(-)

diff --git a/app/workers/subject_removal_worker.rb b/app/workers/subject_removal_worker.rb
index cb04bae02..3a9010ea4 100644
--- a/app/workers/subject_removal_worker.rb
+++ b/app/workers/subject_removal_worker.rb
@@ -8,11 +8,6 @@ class SubjectRemovalWorker
   def perform(subject_id, subject_set_id=nil)
     return unless Flipper.enabled?(:remove_orphan_subjects)
 
-    if subject_set_id
-      # don't cleanup if linked to other SetMemberSubject
-      member_set = SetMemberSubject.where(subject_id: subject_id).where.not(subject_set_id: subject_set_id)
-      return unless member_set.count < 1
-    end
-    Subjects::Remover.new(subject_id).cleanup
+    Subjects::Remover.new(subject_id, nil, subject_set_id).cleanup
   end
 end
diff --git a/lib/subjects/remover.rb b/lib/subjects/remover.rb
index 55aab15fe..daaa5167a 100644
--- a/lib/subjects/remover.rb
+++ b/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,12 +59,18 @@ def has_been_collected_or_classified?
       !orphan_subject
     end
 
+    def belongs_to_other_subject_set?
+      return false if !subject_set_id
+
+      orphan_subject.set_member_subjects.where.not(subject_set_id: subject_set_id).count > 0
+    end
+
     def has_been_talked_about?
       panoptes_client.discussions(
         focus_id: subject_id,
         focus_type: 'Subject'
       ).any?
-    rescue StandardError => _e
+    rescue TypeError => _e
       false
     end
 
diff --git a/spec/lib/subjects/remover_spec.rb b/spec/lib/subjects/remover_spec.rb
index e2ca8c837..6f84efc26 100644
--- a/spec/lib/subjects/remover_spec.rb
+++ b/spec/lib/subjects/remover_spec.rb
@@ -105,6 +105,16 @@
         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) { Subjects::Remover.new(subject.id, panoptes_client, alternate_subject_set.id) }
+        it "should not remove subjects associated with multiple set_member_subjects" do
+          create(:set_member_subject, subject: subject, subject_set: alternate_subject_set)
+          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)
diff --git a/spec/workers/subject_removal_worker_spec.rb b/spec/workers/subject_removal_worker_spec.rb
index 82721052d..4681ba43f 100644
--- a/spec/workers/subject_removal_worker_spec.rb
+++ b/spec/workers/subject_removal_worker_spec.rb
@@ -17,8 +17,9 @@
     let!(:second_subject) { subjects.second }
 
     def stub_discussions_request(subject_id)
-      stub_request(:get, "#{discussions_url}?focus_id=#{subject_id}&focus_type=Subject")
-        .to_return(status: 200, body: '[]', headers: {})
+      stub_request(:get, discussions_url)
+        .with(query: { focus_id: subject_id, focus_type: "Subject" })
+        .to_return(status: 200, body: "[]", headers: {})
     end
 
     before do
@@ -27,7 +28,7 @@ def stub_discussions_request(subject_id)
 
     context 'when running orphan remover cleanup function' do
       it 'should call the orphan remover cleanup when enabled' do
-        allow(Subjects::Remover).to receive(:new).with(subject_id).and_return(remover)
+        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)
@@ -37,7 +38,7 @@ def stub_discussions_request(subject_id)
 
     context 'when deleting subjects' do
       it 'deletes subject not associated with set_member_subject' do
-        stub_discussions_request(first_subject.id.to_i)
+        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
@@ -45,7 +46,7 @@ def stub_discussions_request(subject_id)
       it 'does not delete subject assicociated 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.to_i)
+        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

From 8ff902a9e58726abf21674765d27faf7934c079e Mon Sep 17 00:00:00 2001
From: tooyosi <oluwatoyosi.oyegoke@gmail.com>
Date: Mon, 25 Mar 2024 05:34:15 +0000
Subject: [PATCH 6/8] hound cleanups

---
 lib/subjects/remover.rb                     | 4 ++--
 spec/lib/subjects/remover_spec.rb           | 8 ++++----
 spec/workers/subject_removal_worker_spec.rb | 4 ++--
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/lib/subjects/remover.rb b/lib/subjects/remover.rb
index daaa5167a..b9f544556 100644
--- a/lib/subjects/remover.rb
+++ b/lib/subjects/remover.rb
@@ -60,9 +60,9 @@ def has_been_collected_or_classified?
     end
 
     def belongs_to_other_subject_set?
-      return false if !subject_set_id
+      return false unless subject_set_id != nil
 
-      orphan_subject.set_member_subjects.where.not(subject_set_id: subject_set_id).count > 0
+      orphan_subject.set_member_subjects.where.not(subject_set_id: subject_set_id).count.positive?
     end
 
     def has_been_talked_about?
diff --git a/spec/lib/subjects/remover_spec.rb b/spec/lib/subjects/remover_spec.rb
index 6f84efc26..249a36467 100644
--- a/spec/lib/subjects/remover_spec.rb
+++ b/spec/lib/subjects/remover_spec.rb
@@ -105,11 +105,11 @@
         expect { SetMemberSubject.find(sms_ids) }.to raise_error(ActiveRecord::RecordNotFound)
       end
 
-      context "with multiple subject sets" do
+      context 'with multiple subject sets' do
         let(:alternate_subject_set) { create(:subject_set) }
-        let(:remover) { Subjects::Remover.new(subject.id, panoptes_client, alternate_subject_set.id) }
-        it "should not remove subjects associated with multiple set_member_subjects" do
-          create(:set_member_subject, subject: subject, subject_set: alternate_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 'should not remove subjects associated with multiple set_member_subjects' do
           remover.cleanup
           expect(Subject.where(id: subject.id)).to exist
         end
diff --git a/spec/workers/subject_removal_worker_spec.rb b/spec/workers/subject_removal_worker_spec.rb
index 4681ba43f..4cc25653d 100644
--- a/spec/workers/subject_removal_worker_spec.rb
+++ b/spec/workers/subject_removal_worker_spec.rb
@@ -18,8 +18,8 @@
 
     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: "[]", headers: {})
+        .with(query: { focus_id: subject_id, focus_type: 'Subject' })
+        .to_return(status: 200, body: '[]', headers: {})
     end
 
     before do

From 2b7567627fe7bb75a150492b4f655046e6cd22c8 Mon Sep 17 00:00:00 2001
From: tooyosi <oluwatoyosi.oyegoke@gmail.com>
Date: Wed, 27 Mar 2024 04:37:50 +0000
Subject: [PATCH 7/8] pr review: remove type error in remover.rb and stub
 response properly

---
 lib/subjects/remover.rb                     | 4 +---
 spec/lib/subjects/remover_spec.rb           | 3 ++-
 spec/workers/subject_removal_worker_spec.rb | 6 +++---
 3 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/lib/subjects/remover.rb b/lib/subjects/remover.rb
index b9f544556..456aae540 100644
--- a/lib/subjects/remover.rb
+++ b/lib/subjects/remover.rb
@@ -60,7 +60,7 @@ def has_been_collected_or_classified?
     end
 
     def belongs_to_other_subject_set?
-      return false unless subject_set_id != nil
+      return false unless !subject_set_id.nil?
 
       orphan_subject.set_member_subjects.where.not(subject_set_id: subject_set_id).count.positive?
     end
@@ -70,8 +70,6 @@ def has_been_talked_about?
         focus_id: subject_id,
         focus_type: 'Subject'
       ).any?
-    rescue TypeError => _e
-      false
     end
 
     def notify_subject_selector(workflow_ids)
diff --git a/spec/lib/subjects/remover_spec.rb b/spec/lib/subjects/remover_spec.rb
index 249a36467..4dfe67f36 100644
--- a/spec/lib/subjects/remover_spec.rb
+++ b/spec/lib/subjects/remover_spec.rb
@@ -109,7 +109,8 @@
         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 'should not remove subjects associated with multiple set_member_subjects' do
+
+        it 'does not remove subjects associated with multiple set_member_subjects' do
           remover.cleanup
           expect(Subject.where(id: subject.id)).to exist
         end
diff --git a/spec/workers/subject_removal_worker_spec.rb b/spec/workers/subject_removal_worker_spec.rb
index 4cc25653d..202244f15 100644
--- a/spec/workers/subject_removal_worker_spec.rb
+++ b/spec/workers/subject_removal_worker_spec.rb
@@ -19,7 +19,7 @@
     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: '[]', headers: {})
+        .to_return(status: 200, body: '{"discussions": []}', headers: {})
     end
 
     before do
@@ -27,7 +27,7 @@ def stub_discussions_request(subject_id)
     end
 
     context 'when running orphan remover cleanup function' do
-      it 'should call the orphan remover cleanup when enabled' 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)
@@ -43,7 +43,7 @@ def stub_discussions_request(subject_id)
         expect(Subject.where(id: first_subject.id)).not_to exist
       end
 
-      it 'does not delete subject assicociated with another set_member_subject' do
+      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)

From d43c6920bfc8660c13f7c3a9f0964dc8cea68307 Mon Sep 17 00:00:00 2001
From: tooyosi <oluwatoyosi.oyegoke@gmail.com>
Date: Wed, 27 Mar 2024 04:49:37 +0000
Subject: [PATCH 8/8] more pr cleanups

---
 lib/subjects/remover.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/subjects/remover.rb b/lib/subjects/remover.rb
index 456aae540..68262698d 100644
--- a/lib/subjects/remover.rb
+++ b/lib/subjects/remover.rb
@@ -60,7 +60,7 @@ def has_been_collected_or_classified?
     end
 
     def belongs_to_other_subject_set?
-      return false unless !subject_set_id.nil?
+      return false if subject_set_id.nil?
 
       orphan_subject.set_member_subjects.where.not(subject_set_id: subject_set_id).count.positive?
     end