Handle ingest of File from IngestLocalFileJob #460
Conversation
grosscol
force-pushed
the
issue_456
branch
2 times, most recently
from
8462357
to
5ed0a1a
Compare
|
|
||
| # Assign label and title of File Set after original_name has been determined. | ||
| file_set.label ||= file_set.original_file.original_name | ||
| file_set.title = [file_set.label] if file_set.title.blank? |
There was a problem hiding this comment.
Setting file_set.title here does not get persisted according to the specs, and I'm baffled as to why that is the case.
| # @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) | ||
| name = file.respond_to?(:original_filename) ? file.original_filename : ::File.basename(file) |
There was a problem hiding this comment.
why don't we use file_set.label here (or pass it as a parameter)? Then we avoid repeating this test.
There was a problem hiding this comment.
Yes. I was reluctant to change any function signatures, but I did not notice that this one is marked private. I'll update this to do the respond_to check once and then pass along the result where needed.
|
Please add a couple of tests to this PR. |
grosscol
force-pushed
the
issue_456
branch
2 times, most recently
from
f2f8f8f
to
ba7b6ce
Compare
|
A few tests added. |
| described_class.perform_now(file_set.id, filename, 'image/png', 'bob') | ||
| expect(file_set.reload.original_file.mime_type).to eq 'image/png' | ||
| end | ||
|
|
||
| it 'does not decorate File when not given mime_type' do | ||
| allow(CurationConcerns::VersioningService).to receive(:create) |
There was a problem hiding this comment.
Why should VersioningService ever be called? wasn't versioning: false going to prevent it?
There was a problem hiding this comment.
The call to versioning: false is an instruction to the Hydra::Works machinery so that the CurationConcerns machinery can do its own versioning.
There was a problem hiding this comment.
👌 Thanks for the explanation.
Allow use of File in FileSetActor create_content. Don't depend on original_filename and content_type. Use AddFileToFileSet to avoid extra calls to save from IngestFileJob. Update yard docs to indicate File as valid param type. Add specs to cover cases for using ::File as input to FileSetActor functions.
| # @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) |
There was a problem hiding this comment.
label has already been set on line 52. Is there ever a case it gets here without a label set?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Handle ingest of File from IngestLocalFileJob
Allow use of File in FileSetActor create_content.
Don't depend on original_filename and content_type.
Use AddFileToFileSet to avoid extra calls to save from IngestFileJob.
Update yard docs to indicate File as valid param type.
closes #456