-
Notifications
You must be signed in to change notification settings - Fork 68
Image Gallery Modernization #954
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
Conversation
| @@ -0,0 +1,32 @@ | |||
| class BaseUploader < CarrierWave::Uploader::Base | |||
| storage :fog | |||
| # cache_storage :file | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: figure out if we need to or should enable this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
after reading about this, I'm still not sure of impact of setting or leaving it. seems file either way, since it's just temp storage before upload.
| } | ||
| end | ||
|
|
||
| before(:each) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to do: remove whole file and replace with gallery system test
spec/rails_helper.rb
Outdated
|
|
||
| config.before(:each, type: :system, js: true) do | ||
| stub_civicrm | ||
| StorageHelpers.mock_fog! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to figure out if we should always be mocking this in tests or just when we need it
8c22843 to
63ffb6c
Compare
hartsick
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me! sorry for not being able to give more substantive feedback. I think we should get this deployed to staging and see how it behaves. nice sleuthing!
| end | ||
|
|
||
| def id_partition # mimics paperclips mapping function | ||
| ("%09d" % model.id).scan(/\d{3}/).join("/") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fun
| <%= f.label :image, "Upload new image" %> | ||
| <%= image_tag(@upload.image.try(:url)) if @upload.image? %> | ||
| <%= f.file_field :image %> | ||
| <%= f.hidden_field :image_cache %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does this field do? wondering if it's related to the cache_storage setting that was left unset
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah. this is supposed to keep the image in the field if the form fails, and absolutely isn't working at least partially because of this. just going to remove it for now
| before do | ||
| end | ||
|
|
||
| it "uploads to the expected directory" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
think these are the specs you were talking about filling in after behavior was sorted, but flagging in case not!
|
Also, could you make a note in our deploy prep doc about changes we'll want to make to the deployed environment? (removal of env vars, etc) |
63ffb6c to
49ce0cd
Compare
| stub_request(:get, %r{fakeimages/test.png}).to_return(status: 200, body: fixture_file_upload("test-image.png", "image/png").tempfile.to_io, headers: { content_type: "image/png" }) | ||
| stub_request(:any, %r{/action_pages/featured_images/([0-9]+)/([0-9]+)/([0-9]+)/original/test.png}).to_return(status: 200, body: "", headers: {}) | ||
| FactoryBot.create(:action_page, remote_featured_image_url: "https://example.com/fakeimages/test.png") | ||
| file_name = "test-image.png" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the tests i've updated in this file pass when run by themselves but not when in the entire suite
49ce0cd to
afcbbc7
Compare
afcbbc7 to
db37abb
Compare
This introduces a minor new user flow for image management in action center. Previously an admin could upload a banner while creating/editing an action; I've had to migrate/modernize almost everything to do with file uploads in this PR, so there's a new menu item in the admin nav just for gallery management.
The index page has a form which allows for one file upload, which is submitted asynchronously, and similarly images can be deleted from the gallery here.
after this is merged, we'll only need the following env variables for uploads:
Draft until some file migration in our storage takes place and I confirm that everything's working as expected! (also just noticing that I have a few more things to remove/clean up and that I need to update the system tests -- will probably self-review this before I mark as ready to go)