waynehoover / s3_direct_upload

Direct Upload to Amazon S3 With CORS
MIT License
652 stars 333 forks source link

Oversized files should be pre-empted prior to upload #91

Open uberllama opened 11 years ago

uberllama commented 11 years ago

I noticed during a rudimentary test today that the max_file_size param correctly limits uploads with s3, but an error is only thrown when s3 reaches its policy limit. Oversized files should not be addable in the first place, or should give an immediate error and be removed from the queue.

waynehoover commented 11 years ago

We could check this easily in everything but IE < 10, i'll look into it.

uberllama commented 11 years ago

Cool. I thought it might fit in a beforeAdd callback, but when I tried it didn't do what I was expecting: the call back ran as soon as the dom loaded, instead of on adding files to the queue.

uberllama commented 11 years ago

I noticed there's a new file_added binding in the last version. Perhaps we could do it there? What's the intended difference between before_add and file_added?

kcollignon commented 11 years ago

I am in desperate need of this feature as well! Need to alert the user beforehand.

waynehoover commented 11 years ago

@uberllama The file_added callback was actually not needed and I have removed it, I had forgotten about before_add.

Here is how you can test your file before upload:

jQuery ->
  $("#s3uploader").S3Uploader
    before_add: (file) ->
      console.log file.size
      if(file.size > 104857600) ##100 megabytes
        alert("too big")
        false
      else
        true

I have tested this and it doesn't run as soon as the DOM is loaded, can you confirm?

kcollignon commented 11 years ago

Does this test the file size as each individual uploaded file gets run? Ideally I'd like track or alert the user of the entire uploaded file size including all files selected, so I can prevent images from being uploaded, then one fails, etc.

uberllama commented 11 years ago

It tests each file and allows valid ones to go through.

I did a full browser test this morning and encountered an issue. The code doesn't work in IE8 and 9 as expected, but it creates a non-dismissable alert on mobile Safari which is very concerning. I literally have to force quit Safari on the phone.

Here's my non-coffeescript block:

    $('#s3_uploader').S3Uploader(
        { 
            remove_completed_progress_bar: false,
            progress_bar_target: $('#uploads_container'),
            before_add: function(file) {
                if (file.size > 104857600) {
                    alert("Maximum file size is 100 MB");
                    return false;
                } else {
                    return true;
                }
            }
        }
    );
waynehoover commented 11 years ago

This won't work on old browsers because they don't have file API's needed, so the file object is unavailable. The only option for them in sever side validation, or a flash uploader solution.

As far as mobile safari giving an alert that doesn't close, I'm not sure that sounds like a safari bug...

uberllama commented 11 years ago

The Safari issue feels like the method isn't actually returning correctly.

nathany commented 11 years ago

It would be helpful if the max_file_size being configured in the form helper was available to client side code for doing this check.

nathany commented 11 years ago

It seems like before_add isn't called in IE9, which is probably a good thing (don't have to worry about checking for IE10+)

nathany commented 11 years ago

We display a "file too big" validation error in different ways depending on mobile/desktop and I imagine everyone will have a slightly different way. Just putting max_file_size into the data attributes would be a great first step.

On our end, I'm using the template method pattern with this code for before_add and a subclass providing the implementation of fail:

max_file_size = s3_uploader_form.data('max-file-size')
if(max_file_size? and file.size > max_file_size)
  @fail(s3_uploader_form, "#{file.name} is too big, please choose a smaller file.")
  false
else
  true

However this approach doesn't handle localization.