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

Drag-drop global listener may conflict with other Drag-drop instances

    Details

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

      For regression testing, make sure that drag-drop of sections, resources and blocks still work as before after this patch is applied.

      To test the feature itself, try adding a separate drag-drop instance on the page that already uses moodle-core-dragdrop module (e.g. course/view.php). You may use MDL-33292 for testing, it will not work without this patch.

      Show
      For regression testing, make sure that drag-drop of sections, resources and blocks still work as before after this patch is applied. To test the feature itself, try adding a separate drag-drop instance on the page that already uses moodle-core-dragdrop module (e.g. course/view.php). You may use MDL-33292 for testing, it will not work without this patch.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-33192-master-1

      Description

      Drag-drop global listener may conflict with other Drag-drop instances that do not use moodle-core-dragdrop YUI module. inGroup check should reply on drag instance where applicable, not on drop instance only (inGroup). Custom inGroup function for drag instances needs to be added to moodle-core-dragdrop module.

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              samhemelryk Sam Hemelryk added a comment -

              Hi Ruslan,

              The changes look fine. I have to admit without seeing the conflict I can only speculate on its need or effectiveness.
              However as this doesn't appear to break anything I think get it up for integration.
              Although could you please update the test instruction to include testing the two areas moodle-core-dragdrop is being used so that we can at least be positive its not breaking anything.

              Cheers
              Sam

              Show
              samhemelryk Sam Hemelryk added a comment - Hi Ruslan, The changes look fine. I have to admit without seeing the conflict I can only speculate on its need or effectiveness. However as this doesn't appear to break anything I think get it up for integration. Although could you please update the test instruction to include testing the two areas moodle-core-dragdrop is being used so that we can at least be positive its not breaking anything. Cheers Sam
              Hide
              dobedobedoh Andrew Nicols added a comment -

              FYI, something that was breaking without this fix was MDL-33292

              Show
              dobedobedoh Andrew Nicols added a comment - FYI, something that was breaking without this fix was MDL-33292
              Hide
              kabalin Ruslan Kabalin added a comment -

              Thanks Sam for revision, testing instruction has been updated. Ready for integration now.

              Show
              kabalin Ruslan Kabalin added a comment - Thanks Sam for revision, testing instruction has been updated. Ready for integration now.
              Hide
              poltawski Dan Poltawski added a comment -

              Ruslan, I don't think you have taken what Sam was getting at with he testing instructions. What could break by doing this change? Where should we test for regressions?

              Until the related issue is integrated the important point is that we don't introduce regressions.

              Show
              poltawski Dan Poltawski added a comment - Ruslan, I don't think you have taken what Sam was getting at with he testing instructions. What could break by doing this change? Where should we test for regressions? Until the related issue is integrated the important point is that we don't introduce regressions.
              Hide
              kabalin Ruslan Kabalin added a comment -

              Thanks Dan, got it now, added missing bit to instruction.

              Show
              kabalin Ruslan Kabalin added a comment - Thanks Dan, got it now, added missing bit to instruction.
              Hide
              poltawski Dan Poltawski added a comment -

              Hi Ruslan,

              inGroup is using camelCase rather than underscores. Its both inconsistent with the other code and against our guidelines.

              Please make it consistent.

              Show
              poltawski Dan Poltawski added a comment - Hi Ruslan, inGroup is using camelCase rather than underscores. Its both inconsistent with the other code and against our guidelines. Please make it consistent.
              Hide
              kabalin Ruslan Kabalin added a comment -

              Thanks Dan, function names are using snakecase now.

              Show
              kabalin Ruslan Kabalin added a comment - Thanks Dan, function names are using snakecase now.
              Hide
              poltawski Dan Poltawski added a comment -

              Integrated, thanks Ruslan

              Show
              poltawski Dan Poltawski added a comment - Integrated, thanks Ruslan
              Hide
              abgreeve Adrian Greeve added a comment -

              I tried out the different drag and drop sections. I didn't have any luck with the patch from MDL-33292, but I didn't spot any regressions either.
              Test passed

              Show
              abgreeve Adrian Greeve added a comment - I tried out the different drag and drop sections. I didn't have any luck with the patch from MDL-33292 , but I didn't spot any regressions either. Test passed
              Hide
              abgreeve Adrian Greeve added a comment -

              I managed to get patch MDL-33292 working. There were conflicts with the rebase that was stopping it from working.

              Show
              abgreeve Adrian Greeve added a comment - I managed to get patch MDL-33292 working. There were conflicts with the rebase that was stopping it from working.
              Hide
              poltawski Dan Poltawski added a comment -

              Congratulations!

              Your work has made into the latest Moodle release!

              You are only authorised to celebrate after testing 15 Moodle 2.3 QA tests, thanks!

              Show
              poltawski Dan Poltawski added a comment - Congratulations! Your work has made into the latest Moodle release! You are only authorised to celebrate after testing 15 Moodle 2.3 QA tests, thanks!

                People

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

                  Dates

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