knownasilya / ember-plupload

Ember component for handling uploads using plupload
MIT License
87 stars 53 forks source link

upload-queue and MaxFileSize Filter #12

Open AdrienNguyenWorldline opened 9 years ago

AdrienNguyenWorldline commented 9 years ago

I Have looked at upload-queue.js When we are using a file size filter the onError is called.

onError: function (uploader, error) {
      if (error.file) {
        var file = this.findProperty('id', error.file.id);
        set(file, 'error', error.file);
      } else {
        set(this, 'error', error);
      }
    }

I see that the goal is to put file related errors attached to the file and global errors to the queue itself.

In our case we have an error related file (error.file not null). But the var file = this.findProperty('id', error.file.id); returns undefined. thus making set(file, 'error', error.file);fail

I have no idea why we have no files in queue at this time. But I tried adding the error globally in that case

  onError: function (uploader, error) {
    if (error.file) {
      var file = this.findProperty('id', error.file.id);
      if(file !== undefined){
        set(file, 'error', error.file);
      }else{
        set(this, 'error', error);
      }
    } else {
      set(this, 'error', error);
    }
  }
tim-evans commented 9 years ago

Hmm... this is strange. Could you provide a pull request of a test that shows this happening?

The easiest way to add a test for this would be to add a test case to the end of tests/unit/upload-queue-test.js that follows the following format:

import File from "ember-plupload/system/file";

test('files get associated with the correct error', function (assert) {
  var queue = UploadQueue.create();
  var uploader = queue.configure();
  // Pass in the file added by plupload here
  var file = {
    id: 1,
    name: 'my-file'
  };
  queue.pushObject(File.create({
    uploader: uploader,
    file: file
  }));

  // Pass in the object that you see being triggered by plupload here
  uploader.Error(uploader, {});
  assert.ok(get(file, 'error'));
});
tim-evans commented 9 years ago

@AdrienNguyenWorldline please let me know if the solution is sufficient for your needs. Thanks!

AdrienNguyenWorldline commented 9 years ago

This is perfect and also allow me to have directly access to the file and it's error if there is one.

However upgrading to version 0.6.10 now gives me SYNTAX_ERR: SYNTAX_ERR: DOMException 12 where successful uploads where happening before.

tim-evans commented 9 years ago

Oh dear. This sounds like the dynamic stylesheet I added. What browser are you using again? (And version, please!)

On May 26, 2015, at 8:32 AM, AdrienNguyenWorldline notifications@github.com wrote:

This is perfect and also allow me to have directly access to the file and it's error if there is one.

However upgrading to version 0.6.10 now gives me SYNTAX_ERR: SYNTAX_ERR: DOMException 12 where successful uploads where happening before.

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

AdrienNguyenWorldline commented 9 years ago

Both Chrome 42.0.2311.135 and Firefox 37.0.1 fail. The error is thrown by plupload.full.min.js though.

tim-evans commented 9 years ago

Hmm. If you can provide the template that you're using for uploads, then I should be able to help you more. I'll need to turn on a debug build of plupload to see exactly where it's failing

On May 26, 2015, at 9:59 AM, AdrienNguyenWorldline notifications@github.com wrote:

Both Chrome 42.0.2311.135 and Firefox 37.0.1 fail. The error is thrown by plupload.full.min.js though.

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

AdrienNguyenWorldline commented 9 years ago

We have a simplie template : {{#pl-uploader extensions=formats runtimes="html5,flash" action="/api/docs/upload" headers=uploadHeaders for="newDoc" when-queued="uploadDoc" max-file-size=maxSize as |queue features|}} <a id="newDoc" class="btn btn-primary">{{format-message (intl-get 'documents.browse')}}</a><br/> {{/pl-uploader}}

with an uploadDoc callback function that manages the upload trigger via a file.upload() promise

tim-evans commented 9 years ago

@AdrienNguyenWorldline Thanks! I'll see if I can break it

tim-evans commented 9 years ago

At a first glance, your template is providing a few things that will no longer work (see the changelog for details on that).

Specifically, the headers and URL are now set in the action:

file.upload('/api/docs/upload', {
  headers: uploadHeaders
})
tim-evans commented 9 years ago

See https://github.com/paddle8/ember-plupload/blob/master/CHANGELOG.md#051-may-9-2015

AdrienNguyenWorldline commented 9 years ago

With the action parameters moved to the upload method call everything works well. However I found out that only calling with url as single parameters fails :

file.upload('/api/docs/upload')

see https://github.com/paddle8/ember-plupload/pull/16 for unit test.

tim-evans commented 9 years ago

@AdrienNguyenWorldline Merged and fixed. Thanks for the tests :)