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

          Attachments

            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