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

Handle ingest of File from IngestLocalFileJob #460

Merged
merged 2 commits into from
Nov 24, 2015
Merged
Show file tree
Hide file tree
Changes from all 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
Original file line number Diff line number Diff line change
Expand Up @@ -46,14 +46,18 @@ def create_metadata(upload_set_id, work, file_set_params = {})
# Simultaneously moving a preservation copy to the repostiory.
# TODO: create a job to monitor this directory and prune old files that
# have made it to the repo
# @param [ActionDigest::HTTP::UploadedFile, Tempfile] file the file uploaded by the user.
# @param [File, ActionDigest::HTTP::UploadedFile, Tempfile] file the file uploaded by the user.
def create_content(file)
file_set.label ||= file.original_filename
# Assign label and title of File Set is necessary.
file_set.label ||= file.respond_to?(:original_filename) ? file.original_filename : ::File.basename(file)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doing file_set.title = [file_set.label] if file_set.title.blank? here satisfies the tests. Putting the same line in the IngestFileJob does not persist the title.

The failing test is https://github.com/projecthydra-labs/curation_concerns/blob/master/spec/actors/curation_concerns/file_set_actor_spec.rb#L97

file_set.title = [file_set.label] if file_set.title.blank?

# Need to save the file_set in order to get an id
return false unless file_set.save

working_file = copy_file_to_working_directory(file, file_set.id)
IngestFileJob.perform_later(file_set.id, working_file, file.content_type, user.user_key)
mime_type = file.respond_to?(:content_type) ? file.content_type : nil
IngestFileJob.perform_later(file_set.id, working_file, mime_type, user.user_key)
make_derivative(file_set.id, working_file)
true
end
Expand Down Expand Up @@ -103,11 +107,13 @@ def make_derivative(file_set_id, working_file)
CharacterizeJob.perform_later(file_set_id, working_file)
end

# @param [ActionDispatch::Http::UploadedFile] file
# @param [File, ActionDispatch::Http::UploadedFile] file
# @param [String] id the identifer
# @return [String] path of the working file
def copy_file_to_working_directory(file, id)
copy_stream_to_working_directory(id, file.original_filename, file)
# file_set.label not gaurunteed to be set at this point (e.g. if called from update_content)
file_set.label ||= file.respond_to?(:original_filename) ? file.original_filename : ::File.basename(file)
Copy link
Contributor

Choose a reason for hiding this comment

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

label has already been set on line 52. Is there ever a case it gets here without a label set?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see that it could get called from update_content, but in that case file_set never gets saved, so it's not required, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The spec seemed to imply that a potential use case was updating a file_set which had no content to begin with so the label would not have already been set. I assumed the file_set to be saved after the update.

copy_stream_to_working_directory(id, file_set.label, file)
end

# @param [FileSet] file_set the resource
Expand Down
20 changes: 15 additions & 5 deletions curation_concerns-models/app/jobs/ingest_file_job.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,23 @@ class IngestFileJob < ActiveJob::Base

def perform(file_set_id, filename, mime_type, user_key)
file_set = FileSet.find(file_set_id)
file = Hydra::Derivatives::IoDecorator.new(File.open(filename, "rb"))
file.mime_type = mime_type
file.original_name = File.basename(filename)

# Tell UploadFileToGenericFile service to skip versioning because versions will be minted by VersionCommitter (called by save_characterize_and_record_committer) when necessary
Hydra::Works::UploadFileToFileSet.call(file_set, file, versioning: false)
file = File.open(filename, "rb")
# If mime-type is known, wrap in an IO decorator
# Otherwise allow Hydra::Works service to determine mime_type
if mime_type
file = Hydra::Derivatives::IoDecorator.new(file)
file.mime_type = mime_type
file.original_name = File.basename(filename)
end

# Tell AddFileToFileSet service to skip versioning because versions will be minted by VersionCommitter (called by save_characterize_and_record_committer) when necessary
Hydra::Works::AddFileToFileSet.call(file_set, file, :original_file, versioning: false)

# Persist changes to the file_set
file_set.save!

# Do post file ingest actions
user = User.find_by_user_key(user_key)
CurationConcerns::VersioningService.create(file_set.original_file, user)
CurationConcerns.config.callback.run(:after_create_content, file_set, user)
Expand Down
18 changes: 18 additions & 0 deletions spec/actors/curation_concerns/file_set_actor_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
let(:file_set) { create(:file_set) }
let(:actor) { described_class.new(file_set, user) }
let(:uploaded_file) { fixture_file_upload('/world.png', 'image/png') }
let(:local_file) { File.open(File.join(fixture_path, 'world.png')) }

describe 'creating metadata and content' do
let(:upload_set_id) { nil }
Expand Down Expand Up @@ -80,6 +81,23 @@
actor.create_content(uploaded_file)
end

context 'using ::File' do
before do
allow(CharacterizeJob).to receive(:perform_later)
allow(IngestFileJob).to receive(:perform_later)
actor.create_content(local_file)
end

it 'sets the label and title' do
expect(file_set.label).to eq(File.basename(local_file))
expect(file_set.title).to eq([File.basename(local_file)])
end

it 'does not set the mime_type' do
expect(file_set.mime_type).to be_nil
end
end

context 'when file_set.title is empty and file_set.label is not' do
let(:file) { 'world.png' }
let(:long_name) { 'an absurdly long title that goes on way to long and messes up the display of the page which should not need to be this big in order to show this impossibly long, long, long, oh so long string' }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@
post :update, id: resource, file_set: { title: nil, depositor: nil }, format: :json
}
it "returns 422 and the errors" do
expect(response).to respond_unprocessable_entity(errors: { "some_field": ["This is not valid. Fix it."] })
expect(response).to respond_unprocessable_entity(errors: { some_field: ["This is not valid. Fix it."] })
end
end
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@
before { post :update, id: resource, generic_work: { title: [] }, format: :json }

it "returns 422 and the errors" do
expect(response).to respond_unprocessable_entity(errors: { "title": ["Your work must have a title."] })
expect(response).to respond_unprocessable_entity(errors: { title: ["Your work must have a title."] })
end
end
end
Expand Down
18 changes: 15 additions & 3 deletions spec/jobs/ingest_file_job_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,21 @@
let(:filename) { fixture_file_path('/world.png') }
let(:user) { create(:user) }

it 'uses the provided mime_type' do
described_class.perform_now(file_set.id, filename, 'image/png', 'bob')
expect(file_set.reload.original_file.mime_type).to eq 'image/png'
context 'when givin a mime_type' do
it 'uses a provided mime_type' do
described_class.perform_now(file_set.id, filename, 'image/png', 'bob')
expect(file_set.reload.original_file.mime_type).to eq 'image/png'
end
end

context 'when not given a mime_type' do
it 'does not decorate File when not given mime_type' do
# Mocking CC Versioning here as it will be the versioning machinery called by the job.
# The parameter versioning: false instructs the machinery in Hydra::Works NOT to do versioning. So it can be handled later on.
allow(CurationConcerns::VersioningService).to receive(:create)
expect(Hydra::Works::AddFileToFileSet).to receive(:call).with(file_set, instance_of(::File), :original_file, versioning: false)
described_class.perform_now(file_set.id, filename, nil, 'bob')
end
end

context 'with two existing versions from different users' do
Expand Down