Support multiple file uploads about refile HOT 47 CLOSED

I've had some time to think about this, and I think I have a pretty decent way forward for this. Basically along @hubertlepicki's original suggestion. Posting this here for critique.

We add a macro to ActiveRecord called accepts_attachments_for, to be used like this:

class Post < ActiveRecord::Base
  has_many :images

  accepts_attachments_for :images, attachment: :file
end

Where attachment: :file names the column in the Image model that has the attachment, the default being :file, so it could have been omitted in this case.

This would generate two methods on Post, called images_files and images_files=. These can then be used in a form like this:

form_for @post do |form|
  form.attachment_field :images_files, multiple: true, direct: true
end

If we want to get really fancy we could allow:

form_for @post do |form|
  form.attachment_field :images, multiple: :file, direct: true
end

But this might just be more confusing.

We build in support into the JS to pick up on the multiple attribute, and do the right thing. With the recent changes in how the caching works, it should be relatively simple to preserve form redisplay behaviour.

Obviously this assumes that Image has no required fields aside from file, but this is reasonable, IMO.

IMO this macro would be small and simple enough to include it in Refile itself, without putting it in another gem.

Thoughts?

from refile.

@kirillsalykin this is how I'm handling removing existing uploads, this also allows me to set extra properties on the uploads.

app/models/upload.rb

class Upload < ActiveRecord::Base
  belongs_to :uploadable, polymorphic: true
  attachment :file

  after_destroy :remove_file

  private

    def remove_file
      file.delete
    end

end

In the container model (Page)

has_many :uploads, as: :uploadable, dependent: :destroy
accepts_attachments_for :uploads, attachment: :file
accepts_nested_attributes_for :uploads, allow_destroy: true

In the view

<%= f.fields_for :uploads do |upload| %>
  <%= attachment_image_tag(upload.object, :file, :small) %>
  <%= upload.input :primary %>
  <%= upload.input :_destroy, as: :boolean %>
<% end %>

from refile.

How does this interact with presigning? Could be tricky.

from refile.

Would love for this one to happen! :) Currently using S3_direct_upload JQuery library and CarrierWave's remote_resource_url=. 👍

from refile.

• N from me! I think this is the only thing holding me back.

Walter

On Dec 12, 2014, at 12:07 PM, Steve Robinson [email protected] wrote:

Would love for this one to happen! :) Currently using S3_direct_upload JQuery library and CarrierWave's remote_resource_url=.

—
Reply to this email directly or view it on GitHub.

from refile.

I can probably give you a hand with this one and maybe some other tickets like the issue #8 and couple others. But before I would start I'd like to have your green light, @jnicklas and some agreement on how that would work, especially in case of storing the file IDs in database.

At the moment, refile is using single column in database where String with ID of the file is stored. I guess that will have to be changed. From top of my head, there are 3 ways to do it:

1. Store serialized list of IDs in database, and de-serialize it as needed
2. Use custom PostgreSQL types (hstore or array)
3. Do not store file ID in the associated record but have association and separate model for all of Refile's uploads

1. would be good if we were never to search for the associated records by the file ID (but we might, depending on how we implement #8 or other metadata persistence mechanism)
2. we're limiting ourselves to say Postgres and Mongo (sic) or other noSQL databases that will be able to implement it
3. we're limiting ourselves to relational database and maybe there can be performance implications

With all the pros and cons, I am thinking 3) would be best option. Custom table for attachment ID and all the metadata you might want to extract from it, say to implement #8.

What's your view, @jnicklas ?

from refile.

I think I might also misunderstand the whole concept. You might just want multiple attachments to the same record, but with different column names, i.e. not multiple attachments of avatars, but say avatar and bio for user.

from refile.

@hubertlepicki I like the idea of one attachment per db row. Let's say I want to build a carousel of images that relate to another object called Car. In the edit form for a Car, I would like to upload multiple images that will be associated with the Car. Later, I want to add a description and other metadata to each image individually.

To get a presigned S3 URL, what about mounting a sinatra endpoint that only returns a presigned url? The javascript can request a presigned url for each file it detects and then loop through and upload.

from refile.

I think I might also misunderstand the whole concept. You might just want multiple attachments to the same record, but with different column names, i.e. not multiple attachments of avatars, but say avatar and bio for user.

No, you understood it correctly. Having avatar and bio for user as attachments already works. We are specifically talking about files sent through via <input type="file" multiple/>

I had a discussion about this with a couple of colleagues today, and we basically arrived at the same conclusion as @hubertlepicki, we all prefer option (3). There is often a need to store additional data about each file, for example a position, or a caption, and this is far easier this way. Also, deleting individual attached files becomes much easier if they map to separate records.

Thinking about this even further, it doesn't really work to support this without it becoming quite ORM specific.

My thinking currently is that we should not support attachments with multiple files in Refile itself at all. Instead we can create a separate refile-multiple gem which maps an array of files to associated records in the database, and can properly handle caching so that form redisplays work. I think this should be fairly easy to write and would be tremendously useful.

As for presigning, I had the same idea as @kevinwmerritt, that we could provide an endpoint in the Rack app which could generate a signature and send it as JSON.

I think it would also make sense to have support for multiple file uploads in the Refile JavaScript library.

We just leave the part of attaching the uploaded files to models to another library.

from refile.

That sounds good!

from refile.

Regarding idea number 3:

I already use it with CarrierWave, where an Album model would have many Photos.

Those Photos actually have the uploader attached. Same can work with Refile.

from refile.

I can confirm that the technique described in the following blog post about CarrierWave is adaptable to the Refile gem:

http://u.osu.edu/hasnan.1/2014/03/30/rails-4-multiple-file-upload-with-carrierwave-nested-form-and-jquery-file-upload/

(I speculate that this is what Piotr Usewicz was referring to.)

from refile.

I'd tried the solution that @cpowell cited above before. It's good but I've got a problem with presigning. My context is:

version.rb
class Version < ActiveRecord::Base
  has_many :attach_files
end

attach_file.rb
class AttachFile  < ActiveRecord::Base
  attachment :file
  belongs_to :version
end

I'm trying to upload multiple files for a version in one form. It would be awesome if each file is uploaded to fog (such as S3) without hitting app in presigned mode.

from refile.

Sounds really good to me!

from refile.

+1 for @jnicklas's proposal.

Any other attributes that may need to be edited on :images records could be handled by accepts_nested_attributes_for after the initial upload as well.

from refile.

+1 for @jnicklas's proposal.

from refile.

+1 for @jnicklas's proposal.

from refile.

hi all)+100500 to support multiple file uploads ) i make a software, where i have 1 ticket which must have more than 1 file... as i understand right..Refile cant help to do it now,yeah?

from refile.

👍

from refile.

👍

from refile.

👍

from refile.

👍

from refile.

We are looking for a multiple file upload solution.

from refile.

👍

from refile.

👍

from refile.

(rails 3)
not good way for multiply upload https://gist.github.com/eddiefisher/1e92a4e35f6fd8c60c29

from refile.

I have a different proposal, which wouldn't have to be ORM-specific, it's based on @hubertlepicki's option (1) (serialization). How about serializing the ID and all metadata in a single column (it can remain <attachment>_id, or we can rename it to <attachment>_data).

{
  "id": 1,
  "filename": "image.jpg",
  "content_type": "image/jpeg",
  "size": 12563
}

This would indeed be a backwards incompatible change, but I think it could solve some of our problems, so bear with me.

For multiple uploads, we would just store a serialized array of each file, and I think it shouldn't be difficult to manage creating/updating/deleting of images. This way we wouldn't have to tie ourselved to an ORM. Also, I can't imagine that someone would want to query this data (if they would, they could just make the column json instead of text type, and I believe it should work), so I think we're not losing anything with serialization. And I think the implementation would be simple enough to keep it in Refile itself. Also, if someone wants to store additional data, I think should be able to store it in the same column somehow (we would have to think about this); if not, they can just make a separate model and attachment using Refile's current behaviour, they just wouldn't be able to use multiple-upload file field out-of-the-box.

Also, If we had this, any additional metadata we want to store wouldn't require users to create separate columns in their tables. Separate columns like these is one of the reasons I flew away from Paperclip to CarrierWave. So we could just chuck in #168. It also trivially solves #176.

Thoughts?

from refile.

@janko-m I did consider that alternative, the reason that I think I prefer the accepts_attachments_for solution is that I think it is more practical for users, and also easier to implement.

If we go with allowing multiple images on a column we need to support this everywhere, including in places where it doesn't really makes sense. How would attachment_image_tag work with this kind of solution, for example? With the current design of Refile, it would also be pretty awkward to generate URLs to these files. It might be doable by changing the API quite a bit, but it's an invasive change. attachment_url(@post, :image)[0] or attachment_url(@post, :image).each do |url| just looks weird in a view, IMO. Putting the attachments on a separate model allows us to simply avoid all these awkward questions in the first place.

Also, I just don't think it's as useful. I think this is the kind of thing which seems convenient until you realize that there are a ton of different things you might want to do with the uploaded attachments, all of which are certainly doable if we serialize, but they all become a little awkward. Thing like:

• Sorting the attachments
• Deleting individual attachments
• Adding captions/votes or other other metadata
• Tagging people in uploaded pictures
• All kinds of tracking information (number of downloads, etc)
• Adding additional files

I think a lot of people are going to start out with a mindset of not needing any of that stuff, until they do need it, and then we've kind of painted them into a corner.

from refile.

Yeah, you're right, it indeed is weird with the current design. And in that case @post.image should probably also be an array, which is also a bit weird. I still am not able to visualize the downsides of it, though. It wouldn't change the current API when using single upload, it would just behave a differently on multiple upload. I will give it some more thought with your examples in mind.

from refile.

A question for people on this thread:

I've started working on this, it's coming along nicely. However, I ran into some design decisions I'm unsure about.

1. When directly uploading multiple files, should it trigger the upload:start, upload:complete, et… events for each file individually, or just once for all of them? My personal preference on this is that triggering them per file is much, much better, so I'm fairly confident that this is the right choice, but I still want to make a note that we could go a different route here. Note that we already have to handle the fact that there might be multiple upload fields in a single form, so assuming we get one single event is a bad assumption. One possibility is to provide both events, but this might just be a bit messy.
2. What happens in a multiple upload when one of the files is invalid or for some other reason cannot be uploaded. The problem is particularly tricky with form redisplays. What if I upload two files, one of which is invalid, should the other file still be preserved? Or should both files be dropped? My feeling on this is that file uploads should be "all or nothing", either all of the files are valid, or none of them are. It feels strange to me that we can have a partially successful upload. But again, I'd like to hear your feedback on this.

from refile.

Another question:

When editing a record and uploading multiple files via this method, should the new files be appended to the list of associated files, or should they replace the existing files? My thinking is that there should probably be a config option for this.

EDIT: in this case, what should the default value of this config option be? (My thinking is append since it's more in-line with accepts_nested_attributes_for).

from refile.

1. I agree, I would prefer per-file triggers. In my use case, I would like to have a counter that tracks current uploads, but can handle that myself if necessary. I don't have a need for an overall trigger as long as I have that counter so I can check if there are zero active uploads left. Others may have a different use case though.
2. I went back and forth, but I think I disagree. I think as a user I would prefer to be presented with an error for each of the failing files, but not have to re-upload the files which were successful. NOTE: It is a fact that users don't read the page. For this reason, I could be wrong. Also for this reason, your way could result in confused users.
3. My first thought was a config option, so I am glad you felt the same way. I think this is something that will be particular to each use case. I also agree with your default value.

from refile.

@brandoncc thank you for your well thought-out feedback. You make a good point about the second argument, I'm also on the fence, and I think there's a case to be made for both options. Maybe this should be a config option as well?

from refile.

I think this comes down to how much "convention over configuration" do you want to have in refile? If you are going for mostly convention over configuration, then this may be one of the things that becomes a convention and people just have to live with. If you would prefer to offer a lot of configuration, then this does seem to be another prime candidate for configuration options.

I think in a perfect world, this would have a sensible default, with a way to configure/override that default. I think in the end, it comes down to the amount of code that each configured code path will require to be maintained. If it doesn't add a ton of code which will then need to be maintained, I say offer a configuration option for it.

I think this sounds like a non-answer, but I did my best to make both arguments, so they can be weighed out fairly.

from refile.

@brandoncc I think that making question (3) configurable would not add a lot of complexity. On the other hand, making question (2) configurable is probably going to be considerably more difficult.

I've also realized that these two questions interact in a strange way, what happens when I edit an existing record, and upload multiple files, one of which is invalid. With the mix of answers I chose, we end up with pretty strange behaviour, where the files which were previously attached remain, but the newly uploaded files are cleared. I think this is pretty unintuitive.

On the other hand, I think the behaviour of the file input should be the same both before and after I press "Submit". When I attach a couple of files, and then click the file input a second time and select other files, the original files are cleared. It feels to me like this behaviour should be the same when I open the file select dialog after submitting.

I think I might be changing my mind towards:

1. Per file
2. All or nothing
3. Overwrite existing files without a config option

Maybe that's much less flexible, but it seems like the most intuitive behaviour from a user's perspective, I think.

from refile.

1. I agree
2. I can live with as a user
3. I have a hard time with this one. Real life example:

I own/created pawnstocker.com. Say a user posts an item for sale this week, and in two weeks it hasn't sold. They decide they want to add another picture of the item. Should they have to re-upload the original pictures? What if they don't have the original pictures anymore but don't want to lose them? Also, if the pictures are large and their internet is slow (as is the case at my work), this adds time that they may not want to spend uploading the images all over again.

To be honest, if #3 is set this way with no way to override, it is probably a deal breaker for my use case. That does not mean that it is not the right decision for refile though.

from refile.

@brandoncc thank you for your thoughtful input. I'll mull it over a bit.

from refile.

I just modified #211 to add a append: true|false config option, which default to false, contrary to what I first suggested. I'm going with the "all-or-nothing" option for (2) for now, if we'd like to change this later we can add a config option, but I feel like it's probably not worth it.

I feel pretty good about #211 now. Everyone who has weighed in with a 👍 on this thread: I'd appreciate it a lot if you'd take the time to test out #211 and see how it works for you, and if you run into any issues. I'd like to get this merged pretty soon.

from refile.

I wish I could test it for you but I am using Cloudinary and their Carrierwave module in my app. Hopefully some others can. I will definitely be trying it out in my next app. Thanks for all of your hard work!

from refile.

@jnicklas I may be seeing an issue with <%= form.attachment_field :uploads_files, multiple: true %> without any existing uploads the value of the hidden field created is [], if I view the form after I've uploaded files the value is [{}, {}] where there is an empty {} for each file I uploaded.

If I submit the form without any changes it deletes all of the previous uploads. If I change the value back to [] with the dev tools things work as expected.

from refile.

@UnderpantsGnome that's strange. Are you using direct, or presigned uploads, or submitting the form manually?

from refile.

@jnicklas just plain form submits for now. Planning to move to direct once I get things in place. I have a crude fix in a fork, not sure if it would cause issues in other places. (looks like it breaks "Multiple file uploads Upload files via form redisplay")

from refile.

@UnderpantsGnome Are you solved this issue with array of empty objects in hidden field after save multiple files?

from refile.

@yozzh I have a working version here not sure if that's a sane enough change for @jnicklas to want to use.

from refile.

@UnderpantsGnome what if you create a PR with your changes?

This issue forces us to use JS input value nullification.
Also I was able to prevent image being removed via reject_if option (please note I do not use accepts_attachments_for macro):

class Image
  belongs_to :imageable, polymorphic: true
  attachment :picture
end

class Category
  has_one :image, as: :imageable
  accepts_nested_attributes_for :image,
                                reject_if: ->(attr) { JSON.parse(attr[:picture]).empty? }
end

from refile.

If I submit the form without any changes it deletes all of the previous uploads. If I change the value back to [] with the dev tools things work as expected.

Experiencing the same issue now. Not direct upload, not presigned, just submitting form.

Also, is there any way how i can automate removing images by checking not needed?
Something similar to

check_box_tag 'article[remove_image_files_ids[]]', image.id?

Thanks!

from refile.

@rakelblujeans it's not released yet. You need to use the master branch to use it.

from refile.