Moodle
  1. Moodle
  2. MDL-31112

Drag and drop folder gets uploaded with garbage

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.2.1
    • Fix Version/s: 2.3
    • Component/s: Files API
    • Labels:
    • Testing Instructions:
      Hide
      1. open course -> forum
      2. Add a new post and fill required fields (Don't upload file)
      3. Try drag and drop folder on file area
        • Select folder on desktop
        • Drag and drop folder on file area
      4. Error message should be visible.
      Show
      open course -> forum Add a new post and fill required fields (Don't upload file) Try drag and drop folder on file area Select folder on desktop Drag and drop folder on file area Error message should be visible.
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
      MDL-31112_dndupload_folder_detection
    • Rank:
      37541

      Description

      Drag and drop a folder in file manager shows uploaded file with folder name, which is garbage.

      1. open course -> forum
      2. Add a new post and fill required fields (Don't upload file)
      3. Try drag and drop folder on file area
        • Select folder on desktop
        • Drag and drop folder on file area
      4. file with folder name is uploaded in file manager which is garbage

        Issue Links

          Activity

          Hide
          Davo Smith added a comment -

          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.

          Show
          Davo Smith added a comment - 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.
          Hide
          Michael de Raadt added a comment -

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

          Show
          Michael de Raadt added a comment - I've just triaged this issue, but please continue to look at it, Dan and Davo.
          Hide
          Dan Poltawski added a comment -

          Hi Rajesh,

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

          thanks,
          dan

          Show
          Dan Poltawski added a comment - Hi Rajesh, I can't reproduce this issue - which browser were you using? thanks, dan
          Hide
          Davo Smith added a comment -

          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.

          Show
          Davo Smith added a comment - 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.
          Hide
          Davo Smith added a comment -

          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.

          Show
          Davo Smith added a comment - 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.
          Hide
          Davo Smith added a comment -

          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).

          Show
          Davo Smith added a comment - 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).
          Hide
          Dan Poltawski added a comment -

          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.

          Show
          Dan Poltawski added a comment - 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.
          Hide
          Davo Smith added a comment -

          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.

          Show
          Davo Smith added a comment - 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.
          Hide
          Rajesh Taneja added a comment -

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

          Show
          Rajesh Taneja added a comment - Thanks for providing spot-on patch, Davo Please feel free to push it for integration review.
          Hide
          Davo Smith added a comment -

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

          Show
          Davo Smith added a comment - I'm not able to push for integration review (unless I'm misunderstanding one of the options on the buttons above).
          Hide
          Rajesh Taneja added a comment -

          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

          Show
          Rajesh Taneja added a comment - 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
          Hide
          Dongsheng Cai added a comment -

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

          Show
          Dongsheng Cai added a comment - Davo, you forgot to close the file handler fclose($fp), sometimes this could damage the uploaded file.
          Hide
          Davo Smith added a comment -

          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.

          Show
          Davo Smith added a comment - 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.
          Hide
          Dongsheng Cai added a comment -

          Thanks Davo, the rest looks good to me

          Show
          Dongsheng Cai added a comment - Thanks Davo, the rest looks good to me
          Hide
          Rajesh Taneja added a comment -

          Thanks for spotting that Dongsheng.

          Show
          Rajesh Taneja added a comment - Thanks for spotting that Dongsheng.
          Hide
          Sam Hemelryk added a comment -

          Thanks guys, this has been integrated now.

          Show
          Sam Hemelryk added a comment - Thanks guys, this has been integrated now.
          Hide
          Jason Fowler added a comment -

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

          Show
          Jason Fowler added a comment - The error message that is shown is not the one that is added in the committed code. it just says "Error connecting to server"
          Hide
          Davo Smith added a comment -

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

          Show
          Davo Smith added a comment - I'll look into this and try to fix in time for next week's integration.
          Hide
          Sam Hemelryk added a comment -

          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

          Show
          Sam Hemelryk added a comment - 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
          Hide
          Dan Poltawski added a comment -

          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).†

          Show
          Dan Poltawski added a comment - 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).†
          Hide
          Jason Fowler added a comment -

          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 ...

          Show
          Jason Fowler added a comment - 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 ...
          Hide
          Dan Poltawski added a comment -

          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

          Show
          Dan Poltawski added a comment - 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
          Hide
          Rajesh Taneja added a comment -

          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.

          Show
          Rajesh Taneja added a comment - 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.
          Hide
          Jason Fowler added a comment -

          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

          Show
          Jason Fowler added a comment - 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
          Hide
          Sam Hemelryk added a comment -

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

          Show
          Sam Hemelryk added a comment - Great - on that note I'm rewinding this back to testing and will mark it passed. Yay to having everyone in agreement!
          Hide
          Sam Hemelryk added a comment -

          Done thanks all

          Show
          Sam Hemelryk added a comment - Done thanks all
          Hide
          Eloy Lafuente (stronk7) added a comment -

          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

          Show
          Eloy Lafuente (stronk7) added a comment - 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

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: