Drag and drop folder gets uploaded with garbage

This may be a browser limitation - but I will investigate further to see if I can tell when a folder has been dropped and ignore it.

I've just triaged this issue, but please continue to look at it, Dan and Davo.

Hi Rajesh,

I can't reproduce this issue - which browser were you using?

thanks,
dan

Ubuntu 11.10 and Chromium certainly exhibits this problem.

I'm looking for a consistent way to spot folders - it looks like they all have type = "", but need to check further.

So far, I have discovered that folders seem to have size=0 under Windows 7 (but maybe not consistently, see link below) and size=4096 and unreadable by FileReader in Ubuntu 11.10 (type="" is returned for files with no/unknown extensions, so is not a helpful check and could also miss folders with a '.' in their name).

I've tried posting here:
http://stackoverflow.com/questions/8856628/detecting-folders-directories-in-javascript-filelist-objects

But haven't got any definite answers yet. I may have to resort to checking the file on the server side and returning an error.

Attaching patch which should fix the issue - it checks for and rejects any file which is composed completely of NULL characters (I can't imagine there being a situation where you want that and it certainly happens when you try to upload a folder, with certain browser / OS combinations).

Hmm - it really feels like a lot of work for us to be reading the whole contents of the file looking for data. (Very tenuous I know) but we'd spend ages reading a 100MB file with a load of null characters and then some data.. It'd be a nice if there were a 'quicker' way to spot them.

I realised that - on the other hand, I find it hard to imagine someone uploading a useful file that only contained 'null' characters even in the first 4k (besides, I think that the server finding out that the 100Mb file contains no useful data is probably better than a bunch of users downloading it to discover the fact).

There is no way for the javascript to determine if it has a folder (I even went to the trouble of reading the Chrome source code to confirm this - the basic libraries have an 'is_dir()' function, but the results of that never get passed any higher in the code). The only distinguishing features that I've found for folders is that they have null contents, are (usually) either 0 or 4096 bytes long (but I've seen reports of higher multiples of 4096 bytes) and they crash FileReader if you try and read their contents in the browser.

Thanks for providing spot-on patch, Davo
Please feel free to push it for integration review.

I'm not able to push for integration review (unless I'm misunderstanding one of the options on the buttons above).

Hello Davo,
I am pushing this for integration review, and will check with Michale, if you can push your bugs through integration review.

Thanks again, for working on awesome feature

Davo, you forgot to close the file handler fclose($fp), sometimes this could damage the uploaded file.

Dongsheng - sorry about that.

I've amended the commit and pushed it back into the repo.
Now has fclose($fp) before both of the return statements.

Thanks Davo, the rest looks good to me

Thanks for spotting that Dongsheng.

Thanks guys, this has been integrated now.

The error message that is shown is not the one that is added in the committed code. it just says "Error connecting to server"

I'll look into this and try to fix in time for next week's integration.

Hi guys,

I've been testing this just now.
In Chrome the changes work perfectly.
In Firefox I get [[serverconnection,error]]

I looked into what Firefox was doing differently and noted that it wasn't in fact making the XHR request to upload the file. The XHR request was being immediately aborted by the browser if the user was trying to upload just a folder.
To confirm this I reverted the changes are retested. Without the changes in Firefox I got exactly the same error. Whereas with Chrome it would now allow me to upload a folder.

At the moment the current changes are an improvement for Chrome, and as Firefox never allowed the user to upload an empty folder there is no benefit there.
We have to decide what to do with this issue, we either revert the changes and continue to look into it, or we accept the benefit they provide and open a second issue to see about handling Firefox better, although I'm not entirely sure that is necessary given it is a Firefox error and not a Moodle error.

Perhaps the best thing to do at the moment is test other browsers and see what happens: Firefox 4, Internet Explorer 8, Safari 5
What does everyone think?

Cheers
Sam

To reject this would be silly IMO.

Our testing process has identified a new (much less severe) issue with firefox. But compared to last week we have a net gain. It is common sense that this should be accepted and we must not allow our process make us think it shouldn't!

Re testing drag and drop support isn't available in IE (<10 Preview 2?) or Safari).†

Drag and drop wasn't working for me at all in chrome - the browser I usually run my tests in, and Raj watched as I tried to get it to work ...

Jason: i'd be interested in having a look at that problem if you have time. My guess would be that its related to JS caching

Hello guys,

I tested this again, and on FF it gives server connection error, although on chromium it works fine.

In addition to Sam's testing, on chromium (16.0.912.77 (Developer Build 118311 Linux) Ubuntu 10.04), user can drag and drop folder and it gets uploaded with garbage. But with this patch it gives proper error message.

So I agree with Dan, this is an improvement and it's solving the main issue, which is : Folder should not be uploaded.

My +1 to pass this and if we get time, we should fix the error message on FF. Although the main issue was to avoid uploading garbage, which seems to be working on all supported browsers.

I don't have any problem with passing it, my concern was that the error message was different to what was expected, but we can pass this one and raise another

Great - on that note I'm rewinding this back to testing and will mark it passed. Yay to having everyone in agreement!

Done thanks all

It is late here and I'm very tired but I didn't want to go to sleep before expressing my admiration for your amazing collaboration. Thanks!

Closing as fixed, heading to zzzZZZzzz, niao