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

Fix batch replay #4108

Merged
merged 5 commits into from
May 5, 2020
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions app/jobs/ingest_batch_entry_job.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,11 @@ class IngestBatchEntryJob < ActiveJob::Base
def perform(batch_entry)
# Validation checking that it is okay to ingest this batch entry
if batch_entry.media_object_pid.present? && MediaObject.exists?(batch_entry.media_object_pid)
published_error(batch_entry)
return
mo = MediaObject.find(batch_entry.media_object_pid)
if mo.published?
published_error(batch_entry)
return
end
end


Expand Down
6 changes: 3 additions & 3 deletions lib/avalon/batch/ingest.rb
Original file line number Diff line number Diff line change
Expand Up @@ -104,15 +104,15 @@ def replay_entry(entry, position)
previous_entry = @previous_entries[position - 1]
# Case 0, determine if we even have an updated at all
# If the payload is the same it means no change and complete true means the migration ran
return nil if previous_entry.payload == entry.fields.to_json && previous_entry.complete
return nil if previous_entry.payload == entry.to_json && previous_entry.complete

# Case 1, if there is a payload change and the item never completed, reset to pending
# Also reset to pending if there is an error
reset_to_pending(previous_entry, entry) if !previous_entry.complete || previous_entry.error

# Case 2, if there is a published media object we cannot replay
unless previous_entry.media_object_pid.nil?
mo = MediaObject.find(previous_entry.media_object_pid)
mo = MediaObject.find(previous_entry.media_object_pid) if MediaObject.exists?(previous_entry.media_object_pid)
Copy link
Member Author

@cjcolvar cjcolvar May 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this guard to avoid a NotFoundError which would have broken the whole batch registration process, but now I'm thinking that this should be handled in a separate PR since it might include more code changes and need a test.

I would think that if the batch entry has a media object id but the media object can't be found then the batch entry should probably be reset_to_pending so that it will be recreated. @joncameron Do you have an opinion on this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went ahead and removed it so this PR can go ahead and be merged, but I think it would be good to create a follow-up issue or PR so it doesn't get lost.

unless mo.nil? # meaning the media_object has been deleted since last time this ran
if mo.published?
published_error(previous_entry)
Expand All @@ -129,7 +129,7 @@ def replay_entry(entry, position)
# @param [BatchEntries] the previous batch entry to reset_to_pending
# @return [BatchEntries] the reset entrt
def reset_to_pending(previous_entry, entry)
previous_entry.payload = entry.fields.to_json
previous_entry.payload = entry.to_json
previous_entry.error = false
previous_entry.error_message = false
previous_entry.complete = false
Expand Down
2 changes: 1 addition & 1 deletion spec/factories/batch_entries.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,6 @@
current_status { 'registered' }
error_message {}
media_object_pid { 'kfd39dnw' }
payload { "{\"publish\":false,\"hidden\":false,\"fields\":{\"title\":[\"Dolorum\"],\"creator\":[\"Carroll, Nora\"],\"date_issued\":[\"2012\"],\"other_identifier\":[\"ABC123\"],\"other_identifier_type\":[\"local\"],\"related_item_url\":[\"http://www.example.com/text.pdf\"],\"related_item_label\":[\"Example Item PDF\"],\"rights_statement\":[\"http://rightsstatements.org/vocab/InC/1.0/\"],\"terms_of_use\":[\"Terms of Use Language\"],\"language\":[\"English\"],\"physical_description\":[\"16mm Reel\"],\"note\":[\"This is a test general note\"],\"note_type\":[\"general\"],\"abstract\":[\"Test abstract\"],\"statement_of_responsibility\":[\"Test Statement of Responsibility\"]},\"files\":[{\"file\":\"spec/fixtures/jazz-performance.mp3\",\"offset\":\"00:00:00.500\",\"label\":\"Quis quo\",\"date_digitized\":\"2015-10-30\"}],\"position\":3,\"user_key\":\"frances.dickens@reichel.com\",\"collection\":\"zc77sq08x\"}" }
payload { "{\"publish\":false,\"hidden\":false,\"fields\":{\"title\":[\"Dolorum\"],\"creator\":[\"Carroll, Nora\"],\"date_issued\":[\"2012\"],\"other_identifier\":[\"ABC123\"],\"other_identifier_type\":[\"local\"],\"related_item_url\":[\"http://www.example.com/text.pdf\"],\"related_item_label\":[\"Example Item PDF\"],\"rights_statement\":[\"http://rightsstatements.org/vocab/InC/1.0/\"],\"terms_of_use\":[\"Terms of Use Language\"],\"language\":[\"English\"],\"physical_description\":[\"16mm Reel\"],\"note\":[\"This is a test general note\"],\"note_type\":[\"general\"],\"abstract\":[\"Test abstract\"],\"statement_of_responsibility\":[\"Test Statement of Responsibility\"]},\"files\":[{\"file\":\"spec/fixtures/jazz-performance.mp3\",\"offset\":\"00:00:00.500\",\"label\":\"Quis quo\",\"date_digitized\":\"2015-10-30\"}],\"position\":3,\"user_key\":\"frances.dickens@reichel.com\",\"collection\":\"#{batch_registries.collection}\"}" }
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,26 @@
require 'rails_helper'

describe IngestBatchEntryJob do
let(:batch_entry) { FactoryBot.create(:batch_entries) }
let!(:collection) { FactoryBot.create(:collection, id: 'zc77sq08x') }
let(:collection) { FactoryBot.create(:collection) }
let(:batch_registry) { FactoryBot.create(:batch_registries, collection: collection.id) }
let(:batch_entry) { FactoryBot.create(:batch_entries, batch_registries: batch_registry) }

describe "perform" do
it 'runs' do
expect { described_class.perform_now(batch_entry) }.to change { MediaObject.count }.by(1)
end

context 'when media object exists and is published' do
let(:published_media_object) { FactoryBot.build(:published_media_object) }

before do
allow(MediaObject).to receive(:exists?).with(batch_entry.media_object_pid).and_return(true)
allow(MediaObject).to receive(:find).with(batch_entry.media_object_pid).and_return(published_media_object)
end

it 'sets an error state' do
expect { described_class.perform_now(batch_entry) }.to change { batch_entry.error }.from(false).to(true)
end
end
end
end