Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

exclude subjects still linked to set_member_subject in subject_remover #4306

Merged
merged 9 commits into from
Mar 27, 2024
2 changes: 1 addition & 1 deletion app/controllers/api/v1/subject_sets_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions app/workers/subject_removal_worker.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
13 changes: 11 additions & 2 deletions lib/subjects/remover.rb
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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?
Expand Down Expand Up @@ -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,
Expand Down
4 changes: 2 additions & 2 deletions spec/controllers/api/v1/subject_sets_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
11 changes: 11 additions & 0 deletions spec/lib/subjects/remover_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
yuenmichelle1 marked this conversation as resolved.
Show resolved Hide resolved
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)
Expand Down
58 changes: 50 additions & 8 deletions spec/workers/subject_removal_worker_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
yuenmichelle1 marked this conversation as resolved.
Show resolved Hide resolved
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
yuenmichelle1 marked this conversation as resolved.
Show resolved Hide resolved
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
Loading