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

s3 driver includes file_name in link_for extras #216

Merged
merged 1 commit into from May 22, 2018

Conversation

Projects
None yet
3 participants
@jrochkind
Copy link
Contributor

jrochkind commented May 17, 2018

Some client code expects this, such as sufia and (identical) hyrax.

In sufia/hyrax if the filename isn't there, it ends up blank in the UI.

I also added the expires header (where applicable), because I noticed that the box driver has it, so I figured it couldn't hurt, although I am not aware of client code using it.

The sufia/hyrax client code REALLY wants a size key as well, but there doesn't seem to be a way to get it in the current architecture without another S3 API call. The size was already there displayed in the listing, so it's already been fetched, and there ought to be a way for the client code to get it... but I can't figure out what it is, so punting on that for now.

@jrgriffiniii jrgriffiniii self-requested a review May 17, 2018

@jrochkind

This comment has been minimized.

Copy link
Contributor Author

jrochkind commented May 17, 2018

Additionally this does not contain tests at present. It probably ought to, but I can't figure out what's going on with the existing spec code, and how I'd add a test for this. Advice welcome.

@jrochkind

This comment has been minimized.

Copy link
Contributor Author

jrochkind commented May 17, 2018

aha, failing travis tells me how to test haha.

@jrochkind jrochkind force-pushed the s3_includes_filename branch 2 times, most recently from b864032 to babddf0 May 21, 2018

@jrochkind

This comment has been minimized.

Copy link
Contributor Author

jrochkind commented May 22, 2018

closes #213

s3 driver includes file_name in link_for extras
Some client code expects this, such as [sufia](https://github.com/samvera/sufia/blob/63e3f1338fb409b7854491ab3c9467209e009aa6/app/assets/javascripts/sufia/browse_everything.js) and (identical) [hyrax](https://github.com/samvera/hyrax/blob/0fe39d4f4dcc24deb805c1e66527ac^C9df7741a/app/assets/javascripts/hyrax/browse_everything.js).

In sufia/hyrax if the filename isn't there, it ends up blank in the UI.

I also added the expires header (where applicable), because I noticed that the [box driver](https://github.com/samvera/browse-everything/blob/2c0a3447049f7ee925a0ee0759d246fa28c474ac/lib/browse_everything/driver/box.rb#L59) has it, so I figured it couldn't hurt, although I am not aware of client code using it.

The sufia/hyrax client code REALLY wants a `size` key as well, but there doesn't seem to be a way to get it in the current architecture without another S3 API call. The size was already there displayed in the listing, so it's already been fetched, and there ought to be a way for the client code to get it... but I can't figure out what it is, so punting on that for now.

@jrochkind jrochkind force-pushed the s3_includes_filename branch from babddf0 to a0f72f3 May 22, 2018

@jrgriffiniii jrgriffiniii merged commit d8e1ddb into master May 22, 2018

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.004%) to 99.288%
Details

@jrgriffiniii jrgriffiniii deleted the s3_includes_filename branch May 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.