Moodle
  1. Moodle
  2. MDL-29766

Add drag and drop capabilities to the Filemanager element

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1.3
    • Fix Version/s: 2.3
    • Component/s: Files API
    • Labels:
    • Testing Instructions:
      Hide

      Test 1A: (Browser Detection)
      1. Verify that the upload repository is enabled and visible in site settings
      2. Go to a Forum
      3. Create a new post
      4. In attachment area see if 'You can drag and drop files...' is displayed continue..

      Expected Result
      • Visible on Google Chrome and Firefox (continue with tests 1B onwards) and IE10 Preview
      • Invisible on IE (less than 10), Safari, Opera (continue with test 2A)

      Test 1B: (Basic Functionality)
      1. Drag an image from your desktop (or a folder) into the attachments area and drop
      2. Submit the forum post

      Expected Result
      • Attachments area should change colour when dragged into box and then dropped and the file appear in the listing
      • When viewing the submitted post, the file should be attached and displayed

      Test 1C: (File Management)
      1. Drag an image from your desktop (or a folder) into the attachments area and drop
      2. Attempt to delete the file from the filemanager
      3. Drag and drop another file into the attachments area
      4. Rename the file

      Expected Result
      • Files renamed and deleted without problems

      Test 1D: (Max Filesize)
      1. Change the forum settings to set max file size of only 10KB
      2. Go to forum and drag and drop a file more than 10KB
      3. Go to forum and drag and drop a file less than 10KB

      Expected Result
      • Large file is not uploaded and an error message is returned informing the user its larger than max file size
      • Small file is uploaded successfully

      Test 1E: (Repository settings)
      1. Disable upload file repository in site settings
      2. Verify the You can drag and drop files...' message is not displayed
      3. Attempt to drag a file into the attachments box

      Expected Result
      • Drag and drop message is not displayed
      • When dragging files over the attachments box, the browser does not attempt to upload the file

      Test 1F: (Filepicker)
      1. Edit your  a user profile
      2. Scroll down to the user picture section
      3. In attachment area see if 'You can drag and drop files...' is displayed?
      4. Drag a picture into the box and submit form
      Expected Result
      • Picture is uploaded succesfully and displayed

      Test 1G: (Invalid file type)
      1. Go to Admin > Users > Accounts > Upload User Pictures
      2. Drag and drop a text file to the file attachment box
      Expected Result
      • An error message is reported that a text file is not accepted in this field

      Test 1H: (Subfolders)
      1. Add a folder resource to a course
      2. Drag and drop a file into the file area
      3. Click the 'create folder' button to create a subfolder, give it a name and press OK
      4. Click on the created folder
      5. Add some files in the created folder
      6. Click on the 'files' link to come out of the subfolder
      7. Drag and drop some more files into the top level folder
      8. Submit the folder resource
      Expected Result
      • All files should be uploaded and in place in the correct folders

      Test 2A: (Unsupported Browsers)
      1. Go to a Forum
      2. Create a new post
      3. In attachment area verify that 'You can drag and drop files...' is not displaed
      4. Upload a file using the repository plugins
      5. Try to delete files and rename files as normal,
      Expected Result
      • All behaviour works as previously to drag/drop

      Show
      Test 1A: (Browser Detection) 1. Verify that the upload repository is enabled and visible in site settings 2. Go to a Forum 3. Create a new post 4. In attachment area see if 'You can drag and drop files...' is displayed continue.. Expected Result • Visible on Google Chrome and Firefox (continue with tests 1B onwards) and IE10 Preview • Invisible on IE (less than 10), Safari, Opera (continue with test 2A) Test 1B: (Basic Functionality) 1. Drag an image from your desktop (or a folder) into the attachments area and drop 2. Submit the forum post Expected Result • Attachments area should change colour when dragged into box and then dropped and the file appear in the listing • When viewing the submitted post, the file should be attached and displayed Test 1C: (File Management) 1. Drag an image from your desktop (or a folder) into the attachments area and drop 2. Attempt to delete the file from the filemanager 3. Drag and drop another file into the attachments area 4. Rename the file Expected Result • Files renamed and deleted without problems Test 1D: (Max Filesize) 1. Change the forum settings to set max file size of only 10KB 2. Go to forum and drag and drop a file more than 10KB 3. Go to forum and drag and drop a file less than 10KB Expected Result • Large file is not uploaded and an error message is returned informing the user its larger than max file size • Small file is uploaded successfully Test 1E: (Repository settings) 1. Disable upload file repository in site settings 2. Verify the You can drag and drop files...' message is not displayed 3. Attempt to drag a file into the attachments box Expected Result • Drag and drop message is not displayed • When dragging files over the attachments box, the browser does not attempt to upload the file • Test 1F: (Filepicker) 1. Edit your  a user profile 2. Scroll down to the user picture section 3. In attachment area see if 'You can drag and drop files...' is displayed? 4. Drag a picture into the box and submit form Expected Result • Picture is uploaded succesfully and displayed Test 1G: (Invalid file type) 1. Go to Admin > Users > Accounts > Upload User Pictures 2. Drag and drop a text file to the file attachment box Expected Result • An error message is reported that a text file is not accepted in this field Test 1H: (Subfolders) 1. Add a folder resource to a course 2. Drag and drop a file into the file area 3. Click the 'create folder' button to create a subfolder, give it a name and press OK 4. Click on the created folder 5. Add some files in the created folder 6. Click on the 'files' link to come out of the subfolder 7. Drag and drop some more files into the top level folder 8. Submit the folder resource Expected Result • All files should be uploaded and in place in the correct folders Test 2A: (Unsupported Browsers) 1. Go to a Forum 2. Create a new post 3. In attachment area verify that 'You can drag and drop files...' is not displaed 4. Upload a file using the repository plugins 5. Try to delete files and rename files as normal, Expected Result • All behaviour works as previously to drag/drop
    • Affected Branches:
      MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      dragdrop-for-master
    • Rank:
      1294

      Description

      Allow the filemanager element to accept files dragged and dropped from the desktop (using new HTML 5 features)

        Issue Links

          Activity

          Davo Smith created issue -
          Davo Smith made changes -
          Field Original Value New Value
          Fix Version/s 2.1.3 [ 11251 ]
          Hide
          Davo Smith added a comment -

          This appears to be working solidly and I've hopefully implemented this in a fairly sensible way.

          Please let me know if anything needs changing.

          This is disabled by default and needs to be enabled via 'Site admin > Development > Experimental > Experimental settings > Enable drag and drop upload'

          Show
          Davo Smith added a comment - This appears to be working solidly and I've hopefully implemented this in a fairly sensible way. Please let me know if anything needs changing. This is disabled by default and needs to be enabled via 'Site admin > Development > Experimental > Experimental settings > Enable drag and drop upload'
          Davo Smith made changes -
          Pull Master Diff URL https://github.com/davosmith/moodle/commit/e74580a09dec272e4d7ca789d4b06fe89333a35d
          Pull Master Branch MDL-29766_Filemanager_dndupload
          Pull from Repository git://github.com/davosmith/moodle.git
          Labels patch
          Dan Poltawski made changes -
          Assignee moodle.com [ moodle.com ] Dan Poltawski [ poltawski ]
          Hide
          Dan Poltawski added a comment -

          Assigning to me to review and get sent for integration

          Show
          Dan Poltawski added a comment - Assigning to me to review and get sent for integration
          Hide
          Ruslan Kabalin added a comment -

          Hello Davo,

          That is a great feature, thanks a lot for contributing it! I have some comments however.

          1. Since it is pretty small and handy feature, I do not think that the experimental setting for enabling/disabling is required.
          2. I do not see the reason why not using Y.io for ajax request. I think you can replace FormData with the hash of parameters that will be converted to required format using build_querystring function. See blocks/navigation/yui/navigation/navigation.js for example (AjaxLoad)
          3. There are some tabs used instead of spaces, please ensure that all tabs are converted to spaces.

          Cheers,
          Ruslan

          Show
          Ruslan Kabalin added a comment - Hello Davo, That is a great feature, thanks a lot for contributing it! I have some comments however. Since it is pretty small and handy feature, I do not think that the experimental setting for enabling/disabling is required. I do not see the reason why not using Y.io for ajax request. I think you can replace FormData with the hash of parameters that will be converted to required format using build_querystring function. See blocks/navigation/yui/navigation/navigation.js for example (AjaxLoad) There are some tabs used instead of spaces, please ensure that all tabs are converted to spaces. Cheers, Ruslan
          Hide
          Davo Smith added a comment -

          Thanks for the feedback Rusian. To answer your points:

          1. Are you suggesting I ditch the setting altogether, or just move it out of 'experimental'? (e.g. into 'Appearance > AJAX and Javascript' and, possibly, defaulting to 'On'?) I was trying to play on the safe side here, as, although I am fairly confident that this code works well, I didn't want to risk breaking file uploads, without a way to switch it off again.

          2. The example you gave (navigation.js) does not include file upload in the Y.io request. If you can find an example that does include file upload (and doesn't involve serialising a form with a standard 'upload' button), then I'd be happy to adapt my code. As things stand, I tried to use Y.io, but it didn't work (and this patch I found on the Internet https://github.com/ghinch/yui3/commit/6c28aa24bb982d9aa95065ff145f90812bd709da#diff-1 suggests that it is not down to me misunderstanding the YUI code). Besides, this method also leaves the possibility of extending the code to include progress bars, at some point in the future (the 'progress' event is not supported by YUI).

          3. I thought I'd told Emacs not to do that! I've fixed that on a new branch and will update the issue to reflect the new branch location (nothing else has changed)

          Show
          Davo Smith added a comment - Thanks for the feedback Rusian. To answer your points: 1. Are you suggesting I ditch the setting altogether, or just move it out of 'experimental'? (e.g. into 'Appearance > AJAX and Javascript' and, possibly, defaulting to 'On'?) I was trying to play on the safe side here, as, although I am fairly confident that this code works well, I didn't want to risk breaking file uploads, without a way to switch it off again. 2. The example you gave (navigation.js) does not include file upload in the Y.io request. If you can find an example that does include file upload (and doesn't involve serialising a form with a standard 'upload' button), then I'd be happy to adapt my code. As things stand, I tried to use Y.io, but it didn't work (and this patch I found on the Internet https://github.com/ghinch/yui3/commit/6c28aa24bb982d9aa95065ff145f90812bd709da#diff-1 suggests that it is not down to me misunderstanding the YUI code). Besides, this method also leaves the possibility of extending the code to include progress bars, at some point in the future (the 'progress' event is not supported by YUI). 3. I thought I'd told Emacs not to do that! I've fixed that on a new branch and will update the issue to reflect the new branch location (nothing else has changed)
          Davo Smith made changes -
          Pull Master Branch MDL-29766_Filemanager_dndupload MDL-29766_Filemanager_dndupload2
          Hide
          Dan Poltawski added a comment -

          Hi Davo,

          I've not reviewed this fully yet, but just wanted to make this comment:

          I agree that I don't think there is a need for a switch on this. I think we just have it there (will try and get someone from HQ to +1 this). We have too many settings in Moodle and this doesn't change existing behaviour (well other than accidental behaviour) so I think we should avoid adding yet another option.

          Show
          Dan Poltawski added a comment - Hi Davo, I've not reviewed this fully yet, but just wanted to make this comment: I agree that I don't think there is a need for a switch on this. I think we just have it there (will try and get someone from HQ to +1 this). We have too many settings in Moodle and this doesn't change existing behaviour (well other than accidental behaviour) so I think we should avoid adding yet another option.
          Hide
          Ruslan Kabalin added a comment -

          Thanks Davo for quick reply.

          1. Yep, I meant removing it completely. Let us see what HQ thinks. You could do some browser compliance validation in the code if required (e.g. check if FormData can be instantiated).

          2. Ah, I missed the point that this is actually a file upload )) Then your approach is probably fine, unless this may work: http://yuilibrary.com/yui/docs/io/#uploading-files-in-an-html-form

          Show
          Ruslan Kabalin added a comment - Thanks Davo for quick reply. 1. Yep, I meant removing it completely. Let us see what HQ thinks. You could do some browser compliance validation in the code if required (e.g. check if FormData can be instantiated). 2. Ah, I missed the point that this is actually a file upload )) Then your approach is probably fine, unless this may work: http://yuilibrary.com/yui/docs/io/#uploading-files-in-an-html-form
          Hide
          Davo Smith added a comment -

          @Rusian:

          1. More or less the first line of my javascript code checks for 'FormData' and 'FileReader', it then aborts if these are not found (the 'browsersupport' function).
          2. I had a look at that part of the YUI interface, but, unless I've misunderstood it, you need an actual form on the page to serialise (which I could create and populate with most fields, but I don't think you can get file data from a 'DataTransfer' object and attach it to a 'File' input element, ready for YUI to serialise).

          Show
          Davo Smith added a comment - @Rusian: 1. More or less the first line of my javascript code checks for 'FormData' and 'FileReader', it then aborts if these are not found (the 'browsersupport' function). 2. I had a look at that part of the YUI interface, but, unless I've misunderstood it, you need an actual form on the page to serialise (which I could create and populate with most fields, but I don't think you can get file data from a 'DataTransfer' object and attach it to a 'File' input element, ready for YUI to serialise).
          Hide
          Sam Marshall added a comment -

          @Davo

          1. I didn't try it but this sounds like a really really great feature, I hope it can go into Moodle 2.2.

          2. IMO there should be no on/off option as it doesn't change behaviour in any fundamental way, but see next.

          3. Regarding existing Moodle controls on file upload feature:

          a. Is there any way to turn this off or to turn it off for certain users? If so, does this honour it?
          b. Does this honour the filesize limit or file type restrictions or whatever else you can put into filemanager (can't remember offhand)?

          4. I didn't look at the code, but if there is something that totally looks like it should use Y.io but it can't due to limitation of that interface, you should add a comment that explains 'this would use Y.io but currently it doesn't support x' (could link to that page you mentioned). that way it stops people complaining about it when they do review and also indicates why it was that way so that it can be changed later if a newer YUI version adds the feature.

          5. Consider wording of 'Drag and drop upload enabled'. I'm not an editor but perhaps 'You can upload files by dragging them to this box' or something like that would be friendlier.

          Show
          Sam Marshall added a comment - @Davo 1. I didn't try it but this sounds like a really really great feature, I hope it can go into Moodle 2.2. 2. IMO there should be no on/off option as it doesn't change behaviour in any fundamental way, but see next. 3. Regarding existing Moodle controls on file upload feature: a. Is there any way to turn this off or to turn it off for certain users? If so, does this honour it? b. Does this honour the filesize limit or file type restrictions or whatever else you can put into filemanager (can't remember offhand)? 4. I didn't look at the code, but if there is something that totally looks like it should use Y.io but it can't due to limitation of that interface, you should add a comment that explains 'this would use Y.io but currently it doesn't support x' (could link to that page you mentioned). that way it stops people complaining about it when they do review and also indicates why it was that way so that it can be changed later if a newer YUI version adds the feature. 5. Consider wording of 'Drag and drop upload enabled'. I'm not an editor but perhaps 'You can upload files by dragging them to this box' or something like that would be friendlier.
          Hide
          Davo Smith added a comment -

          @Sam - thanks for the feedback:

          1. Me too
          2. I think that seems to be the general preference, I'll remove the setting.

          3. a) I'll double-check, but if the control is not enabled, then I'd not expect my code to run
          b) I certainly respect the filesize limitation, but not any file type limitations - I'll look into adding that.

          4. The code includes the following comment already:
          // This would be an ideal place to use the Y.io function
          // however, this does not support data encoded using the
          // FormData object, which is needed to transfer data from
          // the DataTransfer object into an XMLHTTPRequest

          5. I'd like to make the wording a little friendlier, but I was worried about the amount of screen space available, which is why I went for a shortened version and a help icon to explain in more detail. I'll see if I can find some wording that would fit in the space, but be a little clearer.

          Show
          Davo Smith added a comment - @Sam - thanks for the feedback: 1. Me too 2. I think that seems to be the general preference, I'll remove the setting. 3. a) I'll double-check, but if the control is not enabled, then I'd not expect my code to run b) I certainly respect the filesize limitation, but not any file type limitations - I'll look into adding that. 4. The code includes the following comment already: // This would be an ideal place to use the Y.io function // however, this does not support data encoded using the // FormData object, which is needed to transfer data from // the DataTransfer object into an XMLHTTPRequest 5. I'd like to make the wording a little friendlier, but I was worried about the amount of screen space available, which is why I went for a shortened version and a help icon to explain in more detail. I'll see if I can find some wording that would fit in the space, but be a little clearer.
          Hide
          Dan Poltawski added a comment -

          @Davo - if you can work on these items soon i'd really like to submit it for integration

          Show
          Dan Poltawski added a comment - @Davo - if you can work on these items soon i'd really like to submit it for integration
          Hide
          Davo Smith added a comment -

          Pretty much done, I'll create a new branch tonight. Got distracted last night when I discovered that the filemanager element ignores the allowed_types setting (bug reported separately).

          Show
          Davo Smith added a comment - Pretty much done, I'll create a new branch tonight. Got distracted last night when I discovered that the filemanager element ignores the allowed_types setting (bug reported separately).
          Hide
          Davo Smith added a comment -

          I've addressed the points raised by Sam (and created a new branch with the combined patch):

          2. On/off option removed

          3.a) I couldn't find a specific setting, but as I go via the 'upload' repository type, permissions are checked at that point
          b) Filetypes and sizes are both checked

          5. Wording changed as suggested

          This still only works for the filemanager element, not the filepicker element, but that should be a very quick change to lib/form/filepicker.js & lib/form/filepicker.php, once everyone is happy with this version.

          I'll make the changes in a separate commit.

          Show
          Davo Smith added a comment - I've addressed the points raised by Sam (and created a new branch with the combined patch): 2. On/off option removed 3.a) I couldn't find a specific setting, but as I go via the 'upload' repository type, permissions are checked at that point b) Filetypes and sizes are both checked 5. Wording changed as suggested This still only works for the filemanager element, not the filepicker element, but that should be a very quick change to lib/form/filepicker.js & lib/form/filepicker.php, once everyone is happy with this version. I'll make the changes in a separate commit.
          Hide
          Dan Poltawski added a comment -

          Just tested the latest version of the patch and it doesn't seem to be working. The box turns green, but the upload doens't seem to happen?

          Show
          Dan Poltawski added a comment - Just tested the latest version of the patch and it doesn't seem to be working. The box turns green, but the upload doens't seem to happen?
          Hide
          Davo Smith added a comment -

          Have you cleared the caches?

          Are there any js error messages?

          Show
          Davo Smith added a comment - Have you cleared the caches? Are there any js error messages?
          Hide
          Dan Poltawski added a comment -

          Cleared caches, error mesasage:

          Uncaught SyntaxError: Unexpected token <
          Y.extend.uploadfile.xhr.onreadystatechange

          Show
          Dan Poltawski added a comment - Cleared caches, error mesasage: Uncaught SyntaxError: Unexpected token < Y.extend.uploadfile.xhr.onreadystatechange
          Hide
          Davo Smith added a comment -

          Hmm... I don't seem to be able to reproduce this on my local computer - I've tried adding files to a forum post in both Firefox 7 and Chromium 15 (running under Ubuntu) and it worked fine, with no errors.

          As there is only 1 '<' character in the code 'if (self.uploadingcount <= 0) {' and I can't see anything wrong with that, the only thing I can guess is that there is something wrong with the JSON string being returned. Are you able to use the 'network' tab of the Chrome developer tools/Firebug to see if there is anything obviously wrong being returned from the upload AJAX call?

          Show
          Davo Smith added a comment - Hmm... I don't seem to be able to reproduce this on my local computer - I've tried adding files to a forum post in both Firefox 7 and Chromium 15 (running under Ubuntu) and it worked fine, with no errors. As there is only 1 '<' character in the code 'if (self.uploadingcount <= 0) {' and I can't see anything wrong with that, the only thing I can guess is that there is something wrong with the JSON string being returned. Are you able to use the 'network' tab of the Chrome developer tools/Firebug to see if there is anything obviously wrong being returned from the upload AJAX call?
          Hide
          Davo Smith added a comment -

          I've added a second commit to the MDL-29766_Filemanager_dndupload3 branch which adds drag and drop to the Filepicker element (in addition to the Filemanager element).

          Show
          Davo Smith added a comment - I've added a second commit to the MDL-29766 _Filemanager_dndupload3 branch which adds drag and drop to the Filepicker element (in addition to the Filemanager element).
          Hide
          Davo Smith added a comment -

          I notice that we're only 1 week away from a Moodle 2.2 feature freeze - is there any chance this could be reviewed in time to make it in?

          Show
          Davo Smith added a comment - I notice that we're only 1 week away from a Moodle 2.2 feature freeze - is there any chance this could be reviewed in time to make it in?
          Hide
          Dan Poltawski added a comment -

          I would like to try and get this in, and will bend the integrators ears to try and do so.

          However, i'm having problems with it on the new branch again - when I drag and drop the files the interface isn't updated, although the file does seem to be uploaded. I will try and get to the bottom of this issue.

          Show
          Dan Poltawski added a comment - I would like to try and get this in, and will bend the integrators ears to try and do so. However, i'm having problems with it on the new branch again - when I drag and drop the files the interface isn't updated, although the file does seem to be uploaded. I will try and get to the bottom of this issue.
          Hide
          Dan Poltawski added a comment -

          Hmm, when I turn 'cache javascript' off i'm not even getting the ability to drag/drop

          Show
          Dan Poltawski added a comment - Hmm, when I turn 'cache javascript' off i'm not even getting the ability to drag/drop
          Hide
          Dan Poltawski added a comment -

          Weird caching problems, sorry.

          The line in question is:

          var result = JSON.parse(xhr.responseText);
          filemanager.js:952 Uncaught SyntaxError: Unexpected token <

          Show
          Dan Poltawski added a comment - Weird caching problems, sorry. The line in question is: var result = JSON.parse(xhr.responseText); filemanager.js:952 Uncaught SyntaxError: Unexpected token <
          Hide
          Dan Poltawski added a comment -

          I'm running chrome 15.0.874.106, also on ubntu

          Show
          Dan Poltawski added a comment - I'm running chrome 15.0.874.106, also on ubntu
          Hide
          Dan Poltawski added a comment -

          I just console.logged xhr.responseText:

          <div class="notifytiny">optional_param_array() expects array parameters only: accepted_types<ul style="text-align: left"><li>line 619 of /lib/moodlelib.php: call to debugging()</li><li>line 49 of /repository/repository_ajax.php: call to optional_param_array()</li></ul></div><div class="notifytiny">optional_param_array() expects array parameters only: accepted_types<ul style="text-align: left"><li>line 619 of /lib/moodlelib.php: call to debugging()</li><li>line 47 of /repository/upload/lib.php: call to optional_param_array()</li><li>line 261 of /repository/repository_ajax.php: call to repository_upload->upload()</li></ul></div>{"url":"http:\/\/homer.lancs.ac.uk\/moodle\/draftfile.php\/5\/user\/draft\/698436952\/goodnews.png","id":698436952,"file":"goodnews.png"}
          
          Show
          Dan Poltawski added a comment - I just console.logged xhr.responseText: <div class= "notifytiny" >optional_param_array() expects array parameters only: accepted_types<ul style= "text-align: left" ><li>line 619 of /lib/moodlelib.php: call to debugging()</li><li>line 49 of /repository/repository_ajax.php: call to optional_param_array()</li></ul></div><div class= "notifytiny" >optional_param_array() expects array parameters only: accepted_types<ul style= "text-align: left" ><li>line 619 of /lib/moodlelib.php: call to debugging()</li><li>line 47 of /repository/upload/lib.php: call to optional_param_array()</li><li>line 261 of /repository/repository_ajax.php: call to repository_upload->upload()</li></ul></div>{ "url" : "http:\/\/homer.lancs.ac.uk\/moodle\/draftfile.php\/5\/user\/draft\/698436952\/goodnews.png" , "id" :698436952, "file" : "goodnews.png" }
          Hide
          Dan Poltawski added a comment -

          Davo: I think you were testing in non DEBUG DEVELOPER mode - since you thats a developer debug message.

          Show
          Dan Poltawski added a comment - Davo: I think you were testing in non DEBUG DEVELOPER mode - since you thats a developer debug message.
          Hide
          Dan Poltawski added a comment -

          I've pushed a fix for that: https://git.luns.net.uk/?p=moodle.git;a=commitdiff;h=216edf476da3395574841f0c4aa583c731ae5bc8

          I'm going to continue to review and prepare for persuading the integrators to integrate

          Show
          Dan Poltawski added a comment - I've pushed a fix for that: https://git.luns.net.uk/?p=moodle.git;a=commitdiff;h=216edf476da3395574841f0c4aa583c731ae5bc8 I'm going to continue to review and prepare for persuading the integrators to integrate
          Hide
          Davo Smith added a comment - - edited

          I've got the server set to 'DEBUG DEVELOPER' (and display debug messages), but strangely enough when stepping through in the debugger, it jumps right over the line where it should display that warning (despite clearly being needed).

          In this case, your fix will work fine (as there is a check to see if there is an item '* ' in the array), but, as far as I can tell, 'accepted_types' is defined to be EITHER an array of file extensions OR a single item '* ' (but I guess this clashes with the recently-introduced 'optional_param_array' function, hence the problems).

          Show
          Davo Smith added a comment - - edited I've got the server set to 'DEBUG DEVELOPER' (and display debug messages), but strangely enough when stepping through in the debugger, it jumps right over the line where it should display that warning (despite clearly being needed). In this case, your fix will work fine (as there is a check to see if there is an item '* ' in the array), but, as far as I can tell, 'accepted_types' is defined to be EITHER an array of file extensions OR a single item '* ' (but I guess this clashes with the recently-introduced 'optional_param_array' function, hence the problems).
          Hide
          Dan Poltawski added a comment -

          Hi Davo,

          Looking at this further it seems like we are introducing lots of the same copy and paste bits of code between both sets of functionality (filemanager and filepicker). This will cause us headaches later (like my previous fix was the same between both files).

          Is there any reason why we could not abstract this functionality into a single JS module which would apply to both scenarios?

          Show
          Dan Poltawski added a comment - Hi Davo, Looking at this further it seems like we are introducing lots of the same copy and paste bits of code between both sets of functionality (filemanager and filepicker). This will cause us headaches later (like my previous fix was the same between both files). Is there any reason why we could not abstract this functionality into a single JS module which would apply to both scenarios?
          Hide
          Davo Smith added a comment -

          I'd thought about that, but I was worried that there are a number of subtle differences between the two implementations, as well as wanting to keep the code encapsulated within the respective elements.

          I could try for a rewrite later tonight that pulled the code out into a separate class.

          Show
          Davo Smith added a comment - I'd thought about that, but I was worried that there are a number of subtle differences between the two implementations, as well as wanting to keep the code encapsulated within the respective elements. I could try for a rewrite later tonight that pulled the code out into a separate class.
          Hide
          Dan Poltawski added a comment -

          I think it would be more sustainable to do so considering there is so much shared code

          Show
          Dan Poltawski added a comment - I think it would be more sustainable to do so considering there is so much shared code
          Hide
          Martin Dougiamas added a comment -

          Hiya, love the idea, but it's really late to be trying to land it for 2.2, it still seems a little "fresh". If you feel really sure it's not going to cause any regressions in any browser environment then we could try and squeeze it in, but there's just very little integrator time left this week. Make it clean and perfect!

          Otherwise it can land in master directly after 2.2 for 2.3.

          Show
          Martin Dougiamas added a comment - Hiya, love the idea, but it's really late to be trying to land it for 2.2, it still seems a little "fresh". If you feel really sure it's not going to cause any regressions in any browser environment then we could try and squeeze it in, but there's just very little integrator time left this week. Make it clean and perfect! Otherwise it can land in master directly after 2.2 for 2.3.
          Hide
          Ben Reynolds added a comment -

          Grown man here, begging that it will get into 2.2.

          Show
          Ben Reynolds added a comment - Grown man here, begging that it will get into 2.2.
          Hide
          Davo Smith added a comment -

          Here is a new version of this patch, which separates out the drag and drop code into a shared file 'lib/form/dndupload.js'

          This looks to be working solidly on Firefox / Chrome.

          I am fairly confident that it will not cause regressions in other browsers, for the following reasons:

          • The drag and drop javascript is the last thing loaded by the filemanager/filepicker, so, whilst I don't anticipate any errors, they would not stop the existing functionality from working
          • The first thing that is checked is the existence of the necessary FileReader and FormData functions - if the browser does not support these, then the drag and drop javascript code immediately exits, without doing anything that could cause an error.
          • The code does not affect the filemanager/filepicker javascript, other than adding a call to an initialiser for the drag and drop (which itself only adds event handlers onto one of the DIV elements on the page).
          • This adds extra functionality, without taking away anything that is already present - if it does not work with all browsers (which it doesn't) then the users of those browsers are not having any worse an experience than they were before.

          On the other hand, I'd understand if this didn't quite make it into 2.2 (although it would be very nice to see it there).

          Show
          Davo Smith added a comment - Here is a new version of this patch, which separates out the drag and drop code into a shared file 'lib/form/dndupload.js' This looks to be working solidly on Firefox / Chrome. I am fairly confident that it will not cause regressions in other browsers, for the following reasons: The drag and drop javascript is the last thing loaded by the filemanager/filepicker, so, whilst I don't anticipate any errors, they would not stop the existing functionality from working The first thing that is checked is the existence of the necessary FileReader and FormData functions - if the browser does not support these, then the drag and drop javascript code immediately exits, without doing anything that could cause an error. The code does not affect the filemanager/filepicker javascript, other than adding a call to an initialiser for the drag and drop (which itself only adds event handlers onto one of the DIV elements on the page). This adds extra functionality, without taking away anything that is already present - if it does not work with all browsers (which it doesn't) then the users of those browsers are not having any worse an experience than they were before. On the other hand, I'd understand if this didn't quite make it into 2.2 (although it would be very nice to see it there).
          Hide
          Dan Poltawski added a comment -

          Thanks for this Davo - I think its looking great.

          I have a few small comments which i'm prepared to work on if you don't have time (sorry for drip feeing you comments, its not intentional!):
          1) Some commenting of the various functions in the JS file just to make things absolutely clear
          2) This code is repeated on all the drag event functions, perhaps it itself could be in a function they all call:

              if (!this.hasfiles(e) || this.reachedmaxfiles()) {
                      return true;
                  }
          
                  e.preventDefault();
                  e.stopPropagation();
          

          3) Could a 'drag the files' here message go into the 'drag to box' itself?

          I plan to have a mamoth testing in as many browsers as I can find session tomorrow

          Show
          Dan Poltawski added a comment - Thanks for this Davo - I think its looking great. I have a few small comments which i'm prepared to work on if you don't have time (sorry for drip feeing you comments, its not intentional!): 1) Some commenting of the various functions in the JS file just to make things absolutely clear 2) This code is repeated on all the drag event functions, perhaps it itself could be in a function they all call: if (! this .hasfiles(e) || this .reachedmaxfiles()) { return true ; } e.preventDefault(); e.stopPropagation(); 3) Could a 'drag the files' here message go into the 'drag to box' itself? I plan to have a mamoth testing in as many browsers as I can find session tomorrow
          Hide
          Davo Smith added a comment -

          I can add a few comments (and the gpl notice), but not until tonight. If you have time during the day, then feel free...
          I'm not convinced that 'if function () return true;' is much smaller than the code there already, but if you prefer, feel free to change.

          The content of the box gets overwritten when the files change, that is why the message didn't go there.

          Show
          Davo Smith added a comment - I can add a few comments (and the gpl notice), but not until tonight. If you have time during the day, then feel free... I'm not convinced that 'if function () return true;' is much smaller than the code there already, but if you prefer, feel free to change. The content of the box gets overwritten when the files change, that is why the message didn't go there.
          Hide
          Davo Smith added a comment -

          Added some more comments (and GPL notice) to the main dndupload.js file.
          Moved the hasfiles / maxfiles checks and the preventdefault calls into a separate function.
          Moved the 'you can drag and drop a file' notice inside the filepicker element (it is not practical to do this for the filemanager element, without making the filemanager code dependent on the dndupload code - and it is likely to look messy anyway, as the filemanager often starts with files already present).

          Show
          Davo Smith added a comment - Added some more comments (and GPL notice) to the main dndupload.js file. Moved the hasfiles / maxfiles checks and the preventdefault calls into a separate function. Moved the 'you can drag and drop a file' notice inside the filepicker element (it is not practical to do this for the filemanager element, without making the filemanager code dependent on the dndupload code - and it is likely to look messy anyway, as the filemanager often starts with files already present).
          Davo Smith made changes -
          Dan Poltawski made changes -
          Testing Instructions Use HTML 5 compliant browser (Chrome, Firefox, ?Safari?)
          Go to a page with a filemanager element (forum post, file resource creation page, etc.)
          Look for 'Drag and drop upload enabled' message above the filemanager element
          Drag some files from your desktop onto the filemanager element
          Submit the form - the files should be successfully attached.
          *Test 1A: (Browser Detection)*
          1. Verify that the upload repository is enabled and visible in site settings
          2. Go to a Forum
          3. Create a new post
          4. In attachment area see if 'You can drag and drop files...' is displayed continue..

          +Expected Result+
          • Visible on Google Chrome and Firefox (continue with tests 1B onwards)
          • Invisible on IE, Safari, Opera (continue with test 2A)



          *Test 1B: (Basic Functionality)*
          1. Drag an image from your desktop (or a folder) into the attachments area and drop
          2. Submit the forum post

          +Expected Result+
          • Attachments area should change colour when dragged into box and then dropped and the file appear in the listing
          • When viewing the submitted post, the file should be attached and displayed


          *Test 1C: (File Management)*
          1. Drag an image from your desktop (or a folder) into the attachments area and drop
          2. Attempt to delete the file from the filemanager
          3. Drag and drop another file into the attachments area
          4. Rename the file

          +Expected Result+
          • Files renamed and deleted without problems


          *Test 1D: (Max Filesize)*
          1. Change the forum settings to set max file size of only 10KB
          2. Go to forum and drag and drop a file more than 10KB
          3. Go to forum and drag and drop a file less than 10KB

          +Expected Result+
          • Large file is not uploaded and an error message is returned informing the user its larger than max file size
          • Small file is uploaded successfully



          *Test 1E: (Repository settings)*
          1. Disable upload file repository in site settings
          2. Verify the You can drag and drop files...' message is not displayed
          3. Attempt to drag a file into the attachments box

          +Expected Result+
          • Drag and drop message is not displayed
          • When dragging files over the attachments box, the browser does not attempt to upload the file



          *Test 1F: (Filepicker)*
          1. Edit your  a user profile
          2. Scroll down to the user picture section
          3. In attachment area see if 'You can drag and drop files...' is displayed?
                  4. Drag a picture into the box and submit form
          +Expected Result+
          • Picture is uploaded succesfully and displayed

          *Test 2A: (Unsupported Browsers)*
          1. Go to a Forum
          2. Create a new post
          3. In attachment area verify that 'You can drag and drop files...' is not displaed
                  4. Upload a file using the repository plugins
                  5. Try to delete files and rename files as normal,
          +Expected Result+
          • All behaviour works as previously to drag/drop
          Hide
          Dan Poltawski added a comment -

          OK, i've done quite a bit of work on this and I am submitting this patch for integration. I've started some quite extensive test instructions above.

          I've also tested on Chrome and Firefox (the two browsers which support drag and drop) as well as Safari, IE7/8/9 and Opera. Though of course i'd like more testers

          Davo, I have done quite extensive updates to your patches including in order to safe bouncing around, the main changes are:

          • Retrieve upload repository from data we already have in JS, rather than requiring another DB query per form item
          • Converted to Moodle coding style (underscores for functions, variables all one work, added phpdoc style docs). This is the agreed style, even if the rest of the javascript files don't say it
          • Changed parameter passing to use specific drag/drop params to make things more explicit
          • Added a loading spinner to show progress when waiting for upload to complete
          • Ensure that only maxfiles uploads are allowed and the interface is updated to reflect that
          • Removed the handling of accepted_params (since it isn't observed at all on the serverside and we wenre' doing anything with it in the normal filepicker (I will file a bug for this)

          This feature is really great - I hope we can get it in for 2.2

          Show
          Dan Poltawski added a comment - OK, i've done quite a bit of work on this and I am submitting this patch for integration. I've started some quite extensive test instructions above. I've also tested on Chrome and Firefox (the two browsers which support drag and drop) as well as Safari, IE7/8/9 and Opera. Though of course i'd like more testers Davo, I have done quite extensive updates to your patches including in order to safe bouncing around, the main changes are: Retrieve upload repository from data we already have in JS, rather than requiring another DB query per form item Converted to Moodle coding style (underscores for functions, variables all one work, added phpdoc style docs). This is the agreed style, even if the rest of the javascript files don't say it Changed parameter passing to use specific drag/drop params to make things more explicit Added a loading spinner to show progress when waiting for upload to complete Ensure that only maxfiles uploads are allowed and the interface is updated to reflect that Removed the handling of accepted_params (since it isn't observed at all on the serverside and we wenre' doing anything with it in the normal filepicker (I will file a bug for this) This feature is really great - I hope we can get it in for 2.2
          Dan Poltawski made changes -
          Status Open [ 1 ] Waiting for integration review [ 10010 ]
          Pull Master Diff URL https://github.com/davosmith/moodle/compare/MDL-29766_Filemanager_dndupload5 https://git.luns.net.uk/?p=moodle.git;a=commitdiff;h=4f556e8543dd931eb725cbf24c90fcc91db8811b;hp=735de1c276df38517d4693eac77b448572ea49df
          Pull Master Branch MDL-29766_Filemanager_dndupload5 dragdrop-for-master
          Pull from Repository git://github.com/davosmith/moodle.git git://git.luns.net.uk/moodle
          Fix Version/s 2.2 [ 10656 ]
          Fix Version/s 2.1.3 [ 11251 ]
          Show
          Dan Poltawski added a comment - My changes: https://git.luns.net.uk/?p=moodle.git;a=commitdiff;h=f25ba6bd87dcfef7f4298b0d9e7ab870968dd52a
          Hide
          Davo Smith added a comment -

          I thought the upload repository did obey the 'accepted_types' parameter; if I set 'accepted_types' to array('.jpg', '.png', '.gif'), then it will not accept files of any other type. I'd prefer to keep it in, even if it is not used very often.

          I thought I my code already checked for max_files - I'm not sure what you've changed in that section.

          For testing purposes, you should also check that creating a folder (the 'resource' settings screen filemanager allows folders), selecting that folder, then dragging and dropping a file, results in it ending up inside the folder (it does, when I tested it, but still worth adding to the list of tests).

          Apart from that - thanks for tidying up the code; I'll try to make sure I stick closer to the coding guidelines in the future.

          The spinner is a good addition - ideally, I'd add a progress bar as well, but maybe that's a future enhancement.

          Show
          Davo Smith added a comment - I thought the upload repository did obey the 'accepted_types' parameter; if I set 'accepted_types' to array('.jpg', '.png', '.gif'), then it will not accept files of any other type. I'd prefer to keep it in, even if it is not used very often. I thought I my code already checked for max_files - I'm not sure what you've changed in that section. For testing purposes, you should also check that creating a folder (the 'resource' settings screen filemanager allows folders), selecting that folder, then dragging and dropping a file, results in it ending up inside the folder (it does, when I tested it, but still worth adding to the list of tests). Apart from that - thanks for tidying up the code; I'll try to make sure I stick closer to the coding guidelines in the future. The spinner is a good addition - ideally, I'd add a progress bar as well, but maybe that's a future enhancement.
          Hide
          Dan Poltawski added a comment -

          I thought the upload repository did obey the 'accepted_types' parameter; if I set 'accepted_types' to array('.jpg', '.png', '.gif'), then it will not accept files of any other type. I'd prefer to keep it in, even if it is not used very often.

          Hmm, I had a look at the repository code and also testing it. The accepted_types was not being passed to the upload repo - but in fact I can see the problem now, its in the upload repository code as an optional_param().

          I think its currently failing on master because that hasn't been converted to to optional_param_array.
          This is pretty messy (and explains why my testing of that wasn't working). Its also a bit crazy that we pass this as a param - since if I wanted to upload something of another type i'd just need to change that param and post it!

          I thought I my code already checked for max_files - I'm not sure what you've changed in that section.

          The problem was that the file-picker was not updating its file count and also that the upload function was not returning true so the 'uploaded files' counter was not increasing.

          Show
          Dan Poltawski added a comment - I thought the upload repository did obey the 'accepted_types' parameter; if I set 'accepted_types' to array('.jpg', '.png', '.gif'), then it will not accept files of any other type. I'd prefer to keep it in, even if it is not used very often. Hmm, I had a look at the repository code and also testing it. The accepted_types was not being passed to the upload repo - but in fact I can see the problem now, its in the upload repository code as an optional_param(). I think its currently failing on master because that hasn't been converted to to optional_param_array. This is pretty messy (and explains why my testing of that wasn't working). Its also a bit crazy that we pass this as a param - since if I wanted to upload something of another type i'd just need to change that param and post it! I thought I my code already checked for max_files - I'm not sure what you've changed in that section. The problem was that the file-picker was not updating its file count and also that the upload function was not returning true so the 'uploaded files' counter was not increasing.
          Hide
          Dan Poltawski added a comment -

          Nope, i'm going crazy, it is working, just the only place I can find to test it out is the upload uesr pictures form

          Show
          Dan Poltawski added a comment - Nope, i'm going crazy, it is working, just the only place I can find to test it out is the upload uesr pictures form
          Hide
          Davo Smith added a comment -

          I thought the filepicker was limited to only one file anyway - that is what I was assuming in the code I have written.

          On the other hand, yes 'uploadfile' is supposed to return 'true' when it accepts the file upload.

          Show
          Davo Smith added a comment - I thought the filepicker was limited to only one file anyway - that is what I was assuming in the code I have written. On the other hand, yes 'uploadfile' is supposed to return 'true' when it accepts the file upload.
          Hide
          Dan Poltawski added a comment -

          Sorry, I meant filemanager, not filepicker. When you do the upload in the filemanager the count of uploaded files was only updated on calling the filepicker callback function, so the number of files was never being updated.

          Show
          Dan Poltawski added a comment - Sorry, I meant filemanager, not filepicker. When you do the upload in the filemanager the count of uploaded files was only updated on calling the filepicker callback function, so the number of files was never being updated.
          Hide
          Dan Poltawski added a comment -

          I've fixed up the accepted_types problem and updated the branch. The changes are:
          https://git.luns.net.uk/?p=moodle.git;a=commitdiff;h=92dcd5fafd4f8c6702e53a1c43c31667bc971272

          Show
          Dan Poltawski added a comment - I've fixed up the accepted_types problem and updated the branch. The changes are: https://git.luns.net.uk/?p=moodle.git;a=commitdiff;h=92dcd5fafd4f8c6702e53a1c43c31667bc971272
          Hide
          Dan Poltawski added a comment -

          Added some more testing instructions for accepted_types test and subfolders

          Show
          Dan Poltawski added a comment - Added some more testing instructions for accepted_types test and subfolders
          Dan Poltawski made changes -
          Testing Instructions *Test 1A: (Browser Detection)*
          1. Verify that the upload repository is enabled and visible in site settings
          2. Go to a Forum
          3. Create a new post
          4. In attachment area see if 'You can drag and drop files...' is displayed continue..

          +Expected Result+
          • Visible on Google Chrome and Firefox (continue with tests 1B onwards)
          • Invisible on IE, Safari, Opera (continue with test 2A)



          *Test 1B: (Basic Functionality)*
          1. Drag an image from your desktop (or a folder) into the attachments area and drop
          2. Submit the forum post

          +Expected Result+
          • Attachments area should change colour when dragged into box and then dropped and the file appear in the listing
          • When viewing the submitted post, the file should be attached and displayed


          *Test 1C: (File Management)*
          1. Drag an image from your desktop (or a folder) into the attachments area and drop
          2. Attempt to delete the file from the filemanager
          3. Drag and drop another file into the attachments area
          4. Rename the file

          +Expected Result+
          • Files renamed and deleted without problems


          *Test 1D: (Max Filesize)*
          1. Change the forum settings to set max file size of only 10KB
          2. Go to forum and drag and drop a file more than 10KB
          3. Go to forum and drag and drop a file less than 10KB

          +Expected Result+
          • Large file is not uploaded and an error message is returned informing the user its larger than max file size
          • Small file is uploaded successfully



          *Test 1E: (Repository settings)*
          1. Disable upload file repository in site settings
          2. Verify the You can drag and drop files...' message is not displayed
          3. Attempt to drag a file into the attachments box

          +Expected Result+
          • Drag and drop message is not displayed
          • When dragging files over the attachments box, the browser does not attempt to upload the file



          *Test 1F: (Filepicker)*
          1. Edit your  a user profile
          2. Scroll down to the user picture section
          3. In attachment area see if 'You can drag and drop files...' is displayed?
                  4. Drag a picture into the box and submit form
          +Expected Result+
          • Picture is uploaded succesfully and displayed

          *Test 2A: (Unsupported Browsers)*
          1. Go to a Forum
          2. Create a new post
          3. In attachment area verify that 'You can drag and drop files...' is not displaed
                  4. Upload a file using the repository plugins
                  5. Try to delete files and rename files as normal,
          +Expected Result+
          • All behaviour works as previously to drag/drop
          *Test 1A: (Browser Detection)*
          1. Verify that the upload repository is enabled and visible in site settings
          2. Go to a Forum
          3. Create a new post
          4. In attachment area see if 'You can drag and drop files...' is displayed continue..

          +Expected Result+
          • Visible on Google Chrome and Firefox (continue with tests 1B onwards)
          • Invisible on IE, Safari, Opera (continue with test 2A)



          *Test 1B: (Basic Functionality)*
          1. Drag an image from your desktop (or a folder) into the attachments area and drop
          2. Submit the forum post

          +Expected Result+
          • Attachments area should change colour when dragged into box and then dropped and the file appear in the listing
          • When viewing the submitted post, the file should be attached and displayed


          *Test 1C: (File Management)*
          1. Drag an image from your desktop (or a folder) into the attachments area and drop
          2. Attempt to delete the file from the filemanager
          3. Drag and drop another file into the attachments area
          4. Rename the file

          +Expected Result+
          • Files renamed and deleted without problems


          *Test 1D: (Max Filesize)*
          1. Change the forum settings to set max file size of only 10KB
          2. Go to forum and drag and drop a file more than 10KB
          3. Go to forum and drag and drop a file less than 10KB

          +Expected Result+
          • Large file is not uploaded and an error message is returned informing the user its larger than max file size
          • Small file is uploaded successfully



          *Test 1E: (Repository settings)*
          1. Disable upload file repository in site settings
          2. Verify the You can drag and drop files...' message is not displayed
          3. Attempt to drag a file into the attachments box

          +Expected Result+
          • Drag and drop message is not displayed
          • When dragging files over the attachments box, the browser does not attempt to upload the file



          *Test 1F: (Filepicker)*
          1. Edit your  a user profile
          2. Scroll down to the user picture section
          3. In attachment area see if 'You can drag and drop files...' is displayed?
                  4. Drag a picture into the box and submit form
          +Expected Result+
          • Picture is uploaded succesfully and displayed

          *Test 1G: (Invalid file type)*
          1. Go to Admin > Users > Accounts > Upload User Pictures
          2. Drag and drop a text file to the file attachment box
          +Expected Result+
          • An error message is reported that a text file is not accepted in this field

          *Test 1H: (Subfolders)*
          1. Add a folder resource to a course
          2. Drag and drop a file into the file area
                  3. Click the 'create folder' button to create a subfolder, give it a name and press OK
                  4. Click on the created folder
                  5. Add some files in the created folder
                  6. Click on the 'files' link to come out of the subfolder
                  7. Drag and drop some more files into the top level folder
                  8. Submit the folder resource
          +Expected Result+
          • All files should be uploaded and in place in the correct folders

          *Test 2A: (Unsupported Browsers)*
          1. Go to a Forum
          2. Create a new post
          3. In attachment area verify that 'You can drag and drop files...' is not displaed
                  4. Upload a file using the repository plugins
                  5. Try to delete files and rename files as normal,
          +Expected Result+
          • All behaviour works as previously to drag/drop
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          PS: Note this is the last message of this type that you will receive along the whole November month, because we are running continuous integration this weeks while QA tests for release of Moodle 2.2 (Dec 1st) are being performed.

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao PS: Note this is the last message of this type that you will receive along the whole November month, because we are running continuous integration this weeks while QA tests for release of Moodle 2.2 (Dec 1st) are being performed.
          Eloy Lafuente (stronk7) made changes -
          Currently in integration Yes [ 10041 ]
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi,

          I'm moving this out from integration. It is a cool feature, but it's new and we are 5 days away from major release so cannot accept it right now, sorry.

          It will be re-considered after Moodle 2.2 release, but being sticky with the process it seems that 2.3 is the primary (correct) target version for this. Let's see if that can be accelerated (but not here, plz, discuss that elsewhere).

          Many thanks for all the hard work, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi, I'm moving this out from integration. It is a cool feature, but it's new and we are 5 days away from major release so cannot accept it right now, sorry. It will be re-considered after Moodle 2.2 release, but being sticky with the process it seems that 2.3 is the primary (correct) target version for this. Let's see if that can be accelerated (but not here, plz, discuss that elsewhere). Many thanks for all the hard work, ciao
          Eloy Lafuente (stronk7) made changes -
          Status Waiting for integration review [ 10010 ] Reopened [ 4 ]
          Eloy Lafuente (stronk7) made changes -
          Currently in integration Yes [ 10041 ]
          Eloy Lafuente (stronk7) made changes -
          Fix Version/s 2.2.1 [ 11456 ]
          Fix Version/s 2.2 [ 10656 ]
          Hide
          Martin Dougiamas added a comment -

          For 2.3. And maybe later 2.2.1 once it's really well tested.

          Show
          Martin Dougiamas added a comment - For 2.3. And maybe later 2.2.1 once it's really well tested.
          Martin Dougiamas made changes -
          Status Reopened [ 4 ] Waiting for integration review [ 10010 ]
          Fix Version/s 2.3 [ 10657 ]
          Fix Version/s 2.2.1 [ 11456 ]
          Hide
          Davo Smith added a comment -

          Is there anything more needed to get this integrated?

          Show
          Davo Smith added a comment - Is there anything more needed to get this integrated?
          Hide
          Dan Poltawski added a comment -

          Hi Davo,

          Just needs rebasing and resubmitting - I have been holding off doing this because I think we are still in bug fix only mode in master.

          Show
          Dan Poltawski added a comment - Hi Davo, Just needs rebasing and resubmitting - I have been holding off doing this because I think we are still in bug fix only mode in master.
          Sam Hemelryk made changes -
          Labels patch integration_held patch
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Eloy Lafuente (stronk7) made changes -
          Currently in integration Yes [ 10041 ]
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday.

          This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody.

          This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P

          Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday. This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody. This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao
          Eloy Lafuente (stronk7) made changes -
          Currently in integration Yes [ 10041 ]
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Sam Hemelryk made changes -
          Currently in integration Yes [ 10041 ]
          Hide
          Dan Poltawski added a comment -

          Rebased onto master.

          Show
          Dan Poltawski added a comment - Rebased onto master.
          Hide
          Dan Poltawski added a comment -

          I've added a note about IE10 preview - it is supposed to support drag/drop now so it would be good to specifically test that (I haven't yet)

          Show
          Dan Poltawski added a comment - I've added a note about IE10 preview - it is supposed to support drag/drop now so it would be good to specifically test that (I haven't yet)
          Dan Poltawski made changes -
          Testing Instructions *Test 1A: (Browser Detection)*
          1. Verify that the upload repository is enabled and visible in site settings
          2. Go to a Forum
          3. Create a new post
          4. In attachment area see if 'You can drag and drop files...' is displayed continue..

          +Expected Result+
          • Visible on Google Chrome and Firefox (continue with tests 1B onwards)
          • Invisible on IE, Safari, Opera (continue with test 2A)



          *Test 1B: (Basic Functionality)*
          1. Drag an image from your desktop (or a folder) into the attachments area and drop
          2. Submit the forum post

          +Expected Result+
          • Attachments area should change colour when dragged into box and then dropped and the file appear in the listing
          • When viewing the submitted post, the file should be attached and displayed


          *Test 1C: (File Management)*
          1. Drag an image from your desktop (or a folder) into the attachments area and drop
          2. Attempt to delete the file from the filemanager
          3. Drag and drop another file into the attachments area
          4. Rename the file

          +Expected Result+
          • Files renamed and deleted without problems


          *Test 1D: (Max Filesize)*
          1. Change the forum settings to set max file size of only 10KB
          2. Go to forum and drag and drop a file more than 10KB
          3. Go to forum and drag and drop a file less than 10KB

          +Expected Result+
          • Large file is not uploaded and an error message is returned informing the user its larger than max file size
          • Small file is uploaded successfully



          *Test 1E: (Repository settings)*
          1. Disable upload file repository in site settings
          2. Verify the You can drag and drop files...' message is not displayed
          3. Attempt to drag a file into the attachments box

          +Expected Result+
          • Drag and drop message is not displayed
          • When dragging files over the attachments box, the browser does not attempt to upload the file



          *Test 1F: (Filepicker)*
          1. Edit your  a user profile
          2. Scroll down to the user picture section
          3. In attachment area see if 'You can drag and drop files...' is displayed?
                  4. Drag a picture into the box and submit form
          +Expected Result+
          • Picture is uploaded succesfully and displayed

          *Test 1G: (Invalid file type)*
          1. Go to Admin > Users > Accounts > Upload User Pictures
          2. Drag and drop a text file to the file attachment box
          +Expected Result+
          • An error message is reported that a text file is not accepted in this field

          *Test 1H: (Subfolders)*
          1. Add a folder resource to a course
          2. Drag and drop a file into the file area
                  3. Click the 'create folder' button to create a subfolder, give it a name and press OK
                  4. Click on the created folder
                  5. Add some files in the created folder
                  6. Click on the 'files' link to come out of the subfolder
                  7. Drag and drop some more files into the top level folder
                  8. Submit the folder resource
          +Expected Result+
          • All files should be uploaded and in place in the correct folders

          *Test 2A: (Unsupported Browsers)*
          1. Go to a Forum
          2. Create a new post
          3. In attachment area verify that 'You can drag and drop files...' is not displaed
                  4. Upload a file using the repository plugins
                  5. Try to delete files and rename files as normal,
          +Expected Result+
          • All behaviour works as previously to drag/drop
          *Test 1A: (Browser Detection)*
          1. Verify that the upload repository is enabled and visible in site settings
          2. Go to a Forum
          3. Create a new post
          4. In attachment area see if 'You can drag and drop files...' is displayed continue..

          +Expected Result+
          • Visible on Google Chrome and Firefox (continue with tests 1B onwards) and IE10 Preview
          • Invisible on IE (less than 10), Safari, Opera (continue with test 2A)



          *Test 1B: (Basic Functionality)*
          1. Drag an image from your desktop (or a folder) into the attachments area and drop
          2. Submit the forum post

          +Expected Result+
          • Attachments area should change colour when dragged into box and then dropped and the file appear in the listing
          • When viewing the submitted post, the file should be attached and displayed


          *Test 1C: (File Management)*
          1. Drag an image from your desktop (or a folder) into the attachments area and drop
          2. Attempt to delete the file from the filemanager
          3. Drag and drop another file into the attachments area
          4. Rename the file

          +Expected Result+
          • Files renamed and deleted without problems


          *Test 1D: (Max Filesize)*
          1. Change the forum settings to set max file size of only 10KB
          2. Go to forum and drag and drop a file more than 10KB
          3. Go to forum and drag and drop a file less than 10KB

          +Expected Result+
          • Large file is not uploaded and an error message is returned informing the user its larger than max file size
          • Small file is uploaded successfully



          *Test 1E: (Repository settings)*
          1. Disable upload file repository in site settings
          2. Verify the You can drag and drop files...' message is not displayed
          3. Attempt to drag a file into the attachments box

          +Expected Result+
          • Drag and drop message is not displayed
          • When dragging files over the attachments box, the browser does not attempt to upload the file



          *Test 1F: (Filepicker)*
          1. Edit your  a user profile
          2. Scroll down to the user picture section
          3. In attachment area see if 'You can drag and drop files...' is displayed?
                  4. Drag a picture into the box and submit form
          +Expected Result+
          • Picture is uploaded succesfully and displayed

          *Test 1G: (Invalid file type)*
          1. Go to Admin > Users > Accounts > Upload User Pictures
          2. Drag and drop a text file to the file attachment box
          +Expected Result+
          • An error message is reported that a text file is not accepted in this field

          *Test 1H: (Subfolders)*
          1. Add a folder resource to a course
          2. Drag and drop a file into the file area
                  3. Click the 'create folder' button to create a subfolder, give it a name and press OK
                  4. Click on the created folder
                  5. Add some files in the created folder
                  6. Click on the 'files' link to come out of the subfolder
                  7. Drag and drop some more files into the top level folder
                  8. Submit the folder resource
          +Expected Result+
          • All files should be uploaded and in place in the correct folders

          *Test 2A: (Unsupported Browsers)*
          1. Go to a Forum
          2. Create a new post
          3. In attachment area verify that 'You can drag and drop files...' is not displaed
                  4. Upload a file using the repository plugins
                  5. Try to delete files and rename files as normal,
          +Expected Result+
          • All behaviour works as previously to drag/drop
          Eloy Lafuente (stronk7) made changes -
          Labels integration_held patch patch
          Sam Hemelryk made changes -
          Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
          Integrator samhemelryk
          Hide
          Sam Hemelryk added a comment -

          Thanks guys this has been integrated now, excellent code btw!
          It also led me to reverting MDL-31000.

          The following are my notes on the integration:

          Changes made during integration:

          • Fixed up a couple of whitespace issues in dndupload.js
          • Wrapped the options.filemanager or options.callback alert with M.cfg.developerdebug
          • get_upload_repositoryid The repositories array key is no longer guaranteed to be it's id, it now uses repositories[i].id as the id (which is always correct)
          • A useful improvement would be when processing files for upload if the number of files selected for upload is greater than the max files that can be uploaded show a confirm box about uploading just the limited number of the user selecting more files.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Thanks guys this has been integrated now, excellent code btw! It also led me to reverting MDL-31000 . The following are my notes on the integration: Changes made during integration: Fixed up a couple of whitespace issues in dndupload.js Wrapped the options.filemanager or options.callback alert with M.cfg.developerdebug get_upload_repositoryid The repositories array key is no longer guaranteed to be it's id, it now uses repositories [i] .id as the id (which is always correct) A useful improvement would be when processing files for upload if the number of files selected for upload is greater than the max files that can be uploaded show a confirm box about uploading just the limited number of the user selecting more files. Cheers Sam
          Sam Hemelryk made changes -
          Status Integration review in progress [ 10004 ] Waiting for testing [ 10005 ]
          Hide
          Ben Reynolds added a comment -

          I would be glad to test this in my Windows 7 64-bit version, but the git stuff confuses me. Which/what/how to get the appropriate version?
          I'm not exactly a naive user, but when it comes to git, I'm a newb.

          Show
          Ben Reynolds added a comment - I would be glad to test this in my Windows 7 64-bit version, but the git stuff confuses me. Which/what/how to get the appropriate version? I'm not exactly a naive user, but when it comes to git, I'm a newb.
          Hide
          Sam Hemelryk added a comment -

          Hi Ben,

          Any help testing would be greatly appreciated.
          At the moment this code is only on our integration server which means you will need to use the code from there.
          I'm going to assume you have git set up on your machine already and that you have a basic understanding of where to start, if not Google can help with that, or look to github.com who have some great tutorials on getting started.

          If you already have a moodle git repository that you would like to use (based of either git.moodle.org/moodle.git or github.com/moodle/moodle.git) then you can use the following two commands to check out the current integration work:

          git fetch git://git.moodle.org/integration.git master
          git checkout -b integration_master FETCH_HEAD
          

          If you are looking to set up new git repository to test this then use the following command which will create a new git repository and directory called integration containing the code you need to test with:

          git clone git://git.moodle.org/integration.git integration
          

          Yell out if you have any more questions.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Ben, Any help testing would be greatly appreciated. At the moment this code is only on our integration server which means you will need to use the code from there. I'm going to assume you have git set up on your machine already and that you have a basic understanding of where to start, if not Google can help with that, or look to github.com who have some great tutorials on getting started. If you already have a moodle git repository that you would like to use (based of either git.moodle.org/moodle.git or github.com/moodle/moodle.git) then you can use the following two commands to check out the current integration work: git fetch git: //git.moodle.org/integration.git master git checkout -b integration_master FETCH_HEAD If you are looking to set up new git repository to test this then use the following command which will create a new git repository and directory called integration containing the code you need to test with: git clone git: //git.moodle.org/integration.git integration Yell out if you have any more questions. Cheers Sam
          Rajesh Taneja made changes -
          Status Waiting for testing [ 10005 ] Testing in progress [ 10011 ]
          Tester rajeshtaneja
          Hide
          Rajesh Taneja added a comment -

          Thanks Dan, for adding this feature.

          Found few issues, but passing this test as they can be fixed as part of other bugs and all steps in test instructions work Great.
          Issues:

          1. TEST 1D: Loading animation remains after clicking ok on error message, and if you try to upload same file again another loading image is displayed (now shows 2 loading images) and it keeps going on as you try loading more big files.
          2. TEST 1G: It do give warning for doc or txt type files, but let me upload exe, lnk files (Not sure if this is a bug)
          3. For a single file upload user can upload multiple files (checked on forum post)
            • Related to drag and drop
              • Select multiple files
              • Drag and drop (multiple selected files) on file area
              • Multiple files get uploaded and get saved
            • Trying to upload two files separately using DND
              • Upload a file using DND
              • Try uploading another file (I used PDF)
              • PDF opens (Which user don't expect IMO)
              • Press browser back button, no file is visible
              • Drag and drop another file, you will see two file. (This is related to file manager issue mentioned below.)
              • Save post and both files get uploaded.
            • Related to file manager
              • upload a file (File will be visible in file manager)
              • Press browser back button
              • Press browser forward button
              • No file is visible in file manager
              • upload another file
              • You should see two files uploaded
              • Save and both files get saved.
          4. Tried to upload folder and it uploads garbage with folder name (checked on forum post)
            • Select a folder
            • drag and drop to file area
            • shows uploaded folder, but uploaded data is garbage, nice to show error message (if possible)
              Few suggestions:
          5. With change of color, it will be nice to change text stating "Drop the files in this area" (Like gmail)
          6. After maximum file is reached, DND event should show error message that you can't upload more then x files
          7. Uploading a folder should show error

          Great work Dan, please let me know if you want me to create issues.

          Show
          Rajesh Taneja added a comment - Thanks Dan, for adding this feature. Found few issues, but passing this test as they can be fixed as part of other bugs and all steps in test instructions work Great. Issues: TEST 1D: Loading animation remains after clicking ok on error message, and if you try to upload same file again another loading image is displayed (now shows 2 loading images) and it keeps going on as you try loading more big files. TEST 1G: It do give warning for doc or txt type files, but let me upload exe, lnk files (Not sure if this is a bug) For a single file upload user can upload multiple files (checked on forum post) Related to drag and drop Select multiple files Drag and drop (multiple selected files) on file area Multiple files get uploaded and get saved Trying to upload two files separately using DND Upload a file using DND Try uploading another file (I used PDF) PDF opens (Which user don't expect IMO) Press browser back button, no file is visible Drag and drop another file, you will see two file. (This is related to file manager issue mentioned below.) Save post and both files get uploaded. Related to file manager upload a file (File will be visible in file manager) Press browser back button Press browser forward button No file is visible in file manager upload another file You should see two files uploaded Save and both files get saved. Tried to upload folder and it uploads garbage with folder name (checked on forum post) Select a folder drag and drop to file area shows uploaded folder, but uploaded data is garbage, nice to show error message (if possible) Few suggestions: With change of color, it will be nice to change text stating "Drop the files in this area" (Like gmail) After maximum file is reached, DND event should show error message that you can't upload more then x files Uploading a folder should show error Great work Dan, please let me know if you want me to create issues.
          Rajesh Taneja made changes -
          Status Testing in progress [ 10011 ] Tested [ 10006 ]
          Hide
          Rajesh Taneja added a comment -

          Sorry, couldn't test it on ie10 preview, because of lack of resources

          Show
          Rajesh Taneja added a comment - Sorry, couldn't test it on ie10 preview, because of lack of resources
          Hide
          Davo Smith added a comment -

          Rajesh - thanks for the detailed feedback. I'll try to address these issues ASAP. If you were able to create some issues in tracker and assign them to me that would probably make things easier to keep track of.

          In terms of getting fixes to these issues into core, should they be submitted as separate patches on top of the version Dan tidied up for me, or should we make a new patch including the original code and these fixes?

          Anyway - really pleased to see this has made it into core and hoping that I can now take this code and use it as a basis for the course drag & drop upload that started this all off ( see MDL-22504 )

          Show
          Davo Smith added a comment - Rajesh - thanks for the detailed feedback. I'll try to address these issues ASAP. If you were able to create some issues in tracker and assign them to me that would probably make things easier to keep track of. In terms of getting fixes to these issues into core, should they be submitted as separate patches on top of the version Dan tidied up for me, or should we make a new patch including the original code and these fixes? Anyway - really pleased to see this has made it into core and hoping that I can now take this code and use it as a basis for the course drag & drop upload that started this all off ( see MDL-22504 )
          Hide
          Rajesh Taneja added a comment -

          Thanks for the awesome patch Davo
          I will create issues for you and you can probably provide patches (fixing the issue) on top of next weekly release.

          Show
          Rajesh Taneja added a comment - Thanks for the awesome patch Davo I will create issues for you and you can probably provide patches (fixing the issue) on top of next weekly release.
          Rajesh Taneja made changes -
          Link This issue testing discovered MDL-31110 [ MDL-31110 ]
          Rajesh Taneja made changes -
          Link This issue testing discovered MDL-31111 [ MDL-31111 ]
          Rajesh Taneja made changes -
          Link This issue testing discovered MDL-31112 [ MDL-31112 ]
          Rajesh Taneja made changes -
          Link This issue testing discovered MDL-31113 [ MDL-31113 ]
          Rajesh Taneja made changes -
          Link This issue testing discovered MDL-31114 [ MDL-31114 ]
          Hide
          Rajesh Taneja added a comment -

          Hello Davo,

          I have created all issues and linked them to this bug. Couldn't assign it to you, will ask Michael to help me with it.

          Thanks again, for all the Good work

          Show
          Rajesh Taneja added a comment - Hello Davo, I have created all issues and linked them to this bug. Couldn't assign it to you, will ask Michael to help me with it. Thanks again, for all the Good work
          Hide
          Eloy Lafuente (stronk7) added a comment -

          This virus has been spread upstream, everybody will be infected soon. Congrats, you did it!

          Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - This virus has been spread upstream, everybody will be infected soon. Congrats, you did it! Closing, ciao
          Eloy Lafuente (stronk7) made changes -
          Status Tested [ 10006 ] Closed [ 6 ]
          Resolution Fixed [ 1 ]
          Currently in integration Yes [ 10041 ]
          Integration date 12/Jan/12
          Hide
          Ben Reynolds added a comment -

          I only got halfway through Sam's instructions about git, but I am glad to see Eloy's note. Well done!

          Show
          Ben Reynolds added a comment - I only got halfway through Sam's instructions about git, but I am glad to see Eloy's note. Well done!
          Hide
          Martin Dougiamas added a comment -

          Woo! Thanks all!

          Show
          Martin Dougiamas added a comment - Woo! Thanks all!
          Martin Dougiamas made changes -
          Parent MDL-22504 [ 36377 ]
          Rank (Obsolete) 9360000000
          Issue Type Sub-task [ 5 ] New Feature [ 2 ]
          Hide
          Ben Reynolds added a comment -

          Happiness and applause.

          Show
          Ben Reynolds added a comment - Happiness and applause.
          Martin Dougiamas made changes -
          Link This issue has been marked as being related by MDL-33005 [ MDL-33005 ]

            People

            • Votes:
              10 Vote for this issue
              Watchers:
              12 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: