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

Upgrading to Shrine 3.x #3257

Merged
merged 5 commits into from
Feb 28, 2021
Merged

Upgrading to Shrine 3.x #3257

merged 5 commits into from
Feb 28, 2021

Conversation

akhsunna
Copy link
Contributor

Considering that derivatives plugin replaced versions plugin in shrine 3.x, some code should be updated.


Also, I added link_name option to RailsAdmin::Config::Fields::Types::FileUpload. I hope it's okay. I did it because currently links for shrine fields don't look nice:

image

@prem-prakash
Copy link
Contributor

prem-prakash commented Sep 29, 2020

tested here, did not work

when there is already an attached file, the attachment data is not loaded on a hidden field, and if the user do not upload a new file, it gives an error:

Shrine::InvalidFile ("" is not a valid IO object (it doesn't respond to #read, #eof?, #rewind, #close)):

@prem-prakash
Copy link
Contributor

I was able to make it work.
It seems that the option cache: true is necessary for the plugin model

  plugin :model, cache: true # false won't work

@akhsunna
Copy link
Contributor Author

akhsunna commented Oct 7, 2020

So I fixed the cache_method and tests for ruby 2.2.

Travis CI: I'm not sure why the tests randomly fail on the "nested form" part :(
Code Climate: this is how it was before, so probably we can ignore these issues within this pull request

original: io,
thumb: FakeIO.another_version(io, :thumb),
}
if Gem.loaded_specs['shrine'].version >= Gem::Version.create('3')
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't there be an else with the versions plugin?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thumb method will work only with derivatives
https://github.com/sferik/rails_admin/blob/e4d970e96a845f20ed898ebd6456062bb13a422c/lib/rails_admin/config/fields/types/shrine.rb#L10-L13

It won't work with version of shrine older than v3. So I just skipped tests about image versions:
https://github.com/sferik/rails_admin/blob/e4d970e96a845f20ed898ebd6456062bb13a422c/spec/rails_admin/config/fields/types/shrine_spec.rb#L14-L20

@prem-prakash, do you think we need to support thumbs for older versions of shrine too?

Copy link
Contributor

Choose a reason for hiding this comment

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

@akhsunna I would say that it would be a good practice at least to not break with older versions, what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, maybe it's worth it :) Will add it soon

@mshibuya mshibuya merged commit 6181283 into railsadminteam:master Feb 28, 2021
@mshibuya
Copy link
Member

Merged in, since this is necessary in attempt to run Ruby 3.0 CI build.
Thanks for the contribution!

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.

4 participants