Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-31321

Drag and drop upload breaks if multiple filemanager/filepicker elements present

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.3
    • Component/s: Forms Library
    • Labels:
    • Testing Instructions:
      Hide

      I'm not sure if there is a standard form with multiple filemanager elements in it, so the first thing to do is to open up 'mod/forum/post_form.php', duplicate the line '$mform->addElement('filemanager' ... =>FILE_INTERNAL));' and change the name from 'attachments' => 'attachments2'.
      If you try to post a message to a forum it should now have 2 filemanager elements.
      Drag and drop a file onto each of the filemanager elements in turn.
      They should both accept a file and upload it correctly
      (Note: only the top filemanager contents will appear in the post when the form is submitted, as the forum code is not written to handle the file uploaded via 'attachments2')

      Show
      I'm not sure if there is a standard form with multiple filemanager elements in it, so the first thing to do is to open up 'mod/forum/post_form.php', duplicate the line '$mform->addElement('filemanager' ... =>FILE_INTERNAL));' and change the name from 'attachments' => 'attachments2'. If you try to post a message to a forum it should now have 2 filemanager elements. Drag and drop a file onto each of the filemanager elements in turn. They should both accept a file and upload it correctly (Note: only the top filemanager contents will appear in the post when the form is submitted, as the forum code is not written to handle the file uploaded via 'attachments2')
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-31321_dndupload_multiple_elements

      Description

      Find a page with multiple filemanager or filepicker elements on a page (I adjusted the code of the forum post form to get this - I can't think of a standard Moodle page that has this, at the moment).

      Try to drag & drop some files onto the first file element on the page - they will be uploaded to the 2nd element instead.

        Gliffy Diagrams

          Activity

          Hide
          davosmith Davo Smith added a comment -

          Need to create a separate instance of the drag and drop object for each filemanager/filepicker - currently they all share a single instance (hence the problems).

          Show
          davosmith Davo Smith added a comment - Need to create a separate instance of the drag and drop object for each filemanager/filepicker - currently they all share a single instance (hence the problems).
          Hide
          davosmith Davo Smith added a comment -

          I've basically wrapped the whole of the internal code in a 'var dnduploadhelper = {' inside the init function, which will ensure there is a new instance available for each filemanager / filepicker element.

          Note that as I've created this patch based on the master branch, it will clash with my fixes for MDL-31114, MDL-31113 and MDL-31110 due to the change in indentation caused by wrapping all the code this way.

          Show
          davosmith Davo Smith added a comment - I've basically wrapped the whole of the internal code in a 'var dnduploadhelper = {' inside the init function, which will ensure there is a new instance available for each filemanager / filepicker element. Note that as I've created this patch based on the master branch, it will clash with my fixes for MDL-31114 , MDL-31113 and MDL-31110 due to the change in indentation caused by wrapping all the code this way.
          Hide
          salvetore Michael de Raadt added a comment -

          Thanks for putting forward a patch, Davo.

          I'll see if we can get this peer-reviewed and sent for integration soon.

          Show
          salvetore Michael de Raadt added a comment - Thanks for putting forward a patch, Davo. I'll see if we can get this peer-reviewed and sent for integration soon.
          Hide
          davosmith Davo Smith added a comment -

          Rebased onto master.
          Note, the 'diff' url is not much help in this case. If you do a 'diff -w' (or tick 'ignore space change' in gitk) you get a much better idea of what is going on (I've wrapped everything in a 'helper' object, which has caused an indentation change that completely confuses diff).

          Show
          davosmith Davo Smith added a comment - Rebased onto master. Note, the 'diff' url is not much help in this case. If you do a 'diff -w' (or tick 'ignore space change' in gitk) you get a much better idea of what is going on (I've wrapped everything in a 'helper' object, which has caused an indentation change that completely confuses diff).
          Hide
          poltawski Dan Poltawski added a comment -

          Or in fact use --ignore-space-change with git command line

          Looks good to me!

          Show
          poltawski Dan Poltawski added a comment - Or in fact use --ignore-space-change with git command line Looks good to me!
          Hide
          poltawski Dan Poltawski added a comment -

          Davo could you fill out the testing instructions for this?

          I'm submitting for integration now without the testing instructions and hoping Davo will fill them out to stop me getting in trouble

          Show
          poltawski Dan Poltawski added a comment - Davo could you fill out the testing instructions for this? I'm submitting for integration now without the testing instructions and hoping Davo will fill them out to stop me getting in trouble
          Hide
          nebgor Aparup Banerjee added a comment -

          is this very YUI-ful patch only for master & 22 ? ( not 21 right?)

          Show
          nebgor Aparup Banerjee added a comment - is this very YUI-ful patch only for master & 22 ? ( not 21 right?)
          Hide
          poltawski Dan Poltawski added a comment -

          Master only in fact. The dnduplod hasn't made it into 2.2

          Show
          poltawski Dan Poltawski added a comment - Master only in fact. The dnduplod hasn't made it into 2.2
          Hide
          nebgor Aparup Banerjee added a comment -

          ah yep just noticed that. also the diff was so misleading on this! thanks

          Show
          nebgor Aparup Banerjee added a comment - ah yep just noticed that. also the diff was so misleading on this! thanks
          Hide
          nebgor Aparup Banerjee added a comment -

          thanks for the changes here (and Dan, for my newly acquired git-fu skills)

          patch has been integrated into master only. (affects 22 has been removed)

          Show
          nebgor Aparup Banerjee added a comment - thanks for the changes here (and Dan, for my newly acquired git-fu skills) patch has been integrated into master only. (affects 22 has been removed)
          Hide
          gerry Gerard Caulfield added a comment -

          Very nice fix, well done. Test passed.

          Show
          gerry Gerard Caulfield added a comment - Very nice fix, well done. Test passed.
          Hide
          stronk7 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
          stronk7 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:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                25/Jun/12