Moodle
  1. Moodle
  2. MDL-31321

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

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor 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
    • Rank:
      37790

      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.

        Activity

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

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

        Looks good to me!

        Show
        Dan Poltawski added a comment - Or in fact use --ignore-space-change with git command line Looks good to me!
        Hide
        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
        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
        Aparup Banerjee added a comment -

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

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

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

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

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

        Show
        Aparup Banerjee added a comment - ah yep just noticed that. also the diff was so misleading on this! thanks
        Hide
        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
        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
        Gerard Caulfield added a comment -

        Very nice fix, well done. Test passed.

        Show
        Gerard Caulfield added a comment - Very nice fix, well done. Test passed.
        Hide
        Eloy Lafuente (stronk7) added a comment -

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

        Closing as fixed, heading to zzzZZZzzz, niao

        Show
        Eloy Lafuente (stronk7) added a comment - It is late here and I'm very tired but I didn't want to go to sleep before expressing my admiration for your amazing collaboration. Thanks! Closing as fixed, heading to zzzZZZzzz, niao

          People

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

            Dates

            • Created:
              Updated:
              Resolved: