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

Test coverage for Hydra::Works::GenericFile::Derivatives. #183

Merged
merged 4 commits into from
Aug 10, 2015

Conversation

tampakis
Copy link
Member

@tampakis tampakis commented Aug 3, 2015

Closes #152.

  • Included all sufia-models derivatives in GenericFile::Derivatives.
  • GenericFile now directly contains all of the derivatives defined for GenericFile.
  • Update readme to show how to configure additional derivatives.
  • Skip ffmpeg-related specs and sofice specs for Travis because they are not configured. Is this something we should consider supporting in CI?
  • Ensure that files are readable. Some of the processors in H::Derivatives pass raw binary strings instead of IO objects.
  • Re-add mime_type as an option for AddFileToGenericFileService because IO objects don't respond to .mime_type. All H:Derivative processors pass the mime_type option to the output_file_service.

@grosscol
Copy link
Member

grosscol commented Aug 4, 2015

Instead of adding mime_type to the signature to accommodate the HydraDerivatives use, we're going to refactor HydraDerivatives to wrap the IO objects in a class that has their attributes. This will be similar to how CurationConcerns operates.

@mjgiarlo
Copy link
Member

mjgiarlo commented Aug 4, 2015

This PR is blocked by the hydra-derivatives work @grosscol referenced above. Once that's done, this PR should depend upon that work and change the way it handles MIME types.

@@ -59,6 +59,70 @@ f1.content = "The quick brown fox jumped over the lazy dog."
bf1.save
```

## Extending Directly Contained Derivatives
Copy link
Member

Choose a reason for hiding this comment

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

For simplicity's sake, can we name this heading something like "Custom Derivatives"? While I appreciate the specificity around how the derivs will be contained (in LDP parlance), I suspect many users of Hydra::Works may not have much knowledge of the intricacies of LDP.

if file.respond_to? :mime_type
return file.mime_type
elsif file.respond_to? :content_type
return file.content_type
elsif file.respond_to? :path
return Hydra::PCDM::GetMimeTypeForFile.call(file.path)
elsif mime_type
Copy link
Member

Choose a reason for hiding this comment

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

If the mime_type is provided shouldn't it have precedence over the result from GetMimeTypeForFile? That's basically a wild ass guess based on file extension.

Copy link
Member Author

Choose a reason for hiding this comment

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

We are working on getting rid of providing the mime_type option, but I agree.

Remove datastream from makes_derivatives.

update to new signature

move tests over from curation_concerns.

get persist_derivatives pending specs to pass

update readme
# @option opts [Boolean] update_existing when set to false, always adds a new file. When set to true, performs a create_or_update
# @option opts [Boolean] versioning whether to create new version entries

def self.call(object, file, opts={})
type = opts.fetch(:type, ::RDF::URI("http://pcdm.org/use#ServiceFile"))
def self.call(object, file, destination_name, opts={})
Copy link
Member

Choose a reason for hiding this comment

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

I don't believe opts can be filled in from the output_file_service.call from Hydra::Derivatives. Only a custom processor would be able to fill this in now. As it stood, the only opt that was ever passed was :mime_type, and that has been addressed.

Basically, update_existing and versioning always get set to true for persisting derivatives. I would like to have versioning set to false instead. I would not like to have versions of thumbnails.

Hydra::Derivatives will not pass opts to output_file_service.
Do not version deriviatives such as thumbnails by default.
Change default value of versioning to false.
@grosscol grosscol removed the Blocked label Aug 7, 2015
@tpendragon
Copy link
Contributor

This looks okay to me, but I haven't dug too deep into the derivatives stuff. @mjgiarlo @tampakis?

@tpendragon
Copy link
Contributor

@mjgiarlo
Copy link
Member

mjgiarlo commented Aug 9, 2015

👍

@tampakis
Copy link
Member Author

The processor is no longer needed because of the RetrieveSourceFile service in Derivatives. I just updated the gemfiles and gemspecs. Feel free to rebase. I'll be on a plane during the call today.

mjgiarlo added a commit that referenced this pull request Aug 10, 2015
Test coverage for Hydra::Works::GenericFile::Derivatives.
@mjgiarlo mjgiarlo merged commit e5678f6 into master Aug 10, 2015
@mjgiarlo mjgiarlo deleted the derivatives_spec_refactor branch August 10, 2015 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants