From 818e0d34ffebb0b72d9ec0252e4059c28db72ce0 Mon Sep 17 00:00:00 2001 From: Oluwatoyosi Oyegoke <34948675+Tooyosi@users.noreply.github.com> Date: Wed, 27 Mar 2024 17:32:01 +0000 Subject: [PATCH] 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 --- .../api/v1/subject_sets_controller.rb | 2 +- app/workers/subject_removal_worker.rb | 4 +- lib/subjects/remover.rb | 13 ++++- .../api/v1/subject_sets_controller_spec.rb | 4 +- spec/lib/subjects/remover_spec.rb | 11 ++++ spec/workers/subject_removal_worker_spec.rb | 58 ++++++++++++++++--- 6 files changed, 77 insertions(+), 15 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..3a9010ea4 100644 --- a/app/workers/subject_removal_worker.rb +++ b/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 diff --git a/lib/subjects/remover.rb b/lib/subjects/remover.rb index 7783ecaa4..68262698d 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,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, 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/lib/subjects/remover_spec.rb b/spec/lib/subjects/remover_spec.rb index e2ca8c837..4dfe67f36 100644 --- a/spec/lib/subjects/remover_spec.rb +++ b/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) diff --git a/spec/workers/subject_removal_worker_spec.rb b/spec/workers/subject_removal_worker_spec.rb index eacf95e9f..202244f15 100644 --- a/spec/workers/subject_removal_worker_spec.rb +++ b/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