Moodle

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

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Minor 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
  • Pull Master Diff URL:

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.

Issue Links

blocks

Bug - A problem which impairs or prevents Moodle from functioning correctly. MDL-33292 Add ability to drag/drop the chooser dialogues (module chooser)

  • Minor - Minor loss of function where workaround is possible
  • Closed - The issue is considered finished, the resolution is correct. Issues which are closed should not be reopened.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
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
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
Permalink
Andrew Nicols added a comment -

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

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

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

Show
Ruslan Kabalin added a comment - Thanks Sam for revision, testing instruction has been updated. Ready for integration now.
Hide
Permalink
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
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
Permalink
Ruslan Kabalin added a comment -

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

Show
Ruslan Kabalin added a comment - Thanks Dan, got it now, added missing bit to instruction.
Hide
Permalink
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
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
Permalink
Ruslan Kabalin added a comment -

Thanks Dan, function names are using snakecase now.

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

Integrated, thanks Ruslan

Show
Dan Poltawski added a comment - Integrated, thanks Ruslan
Hide
Permalink
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
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
Permalink
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
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
Permalink
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
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

Vote (0)
Watch (3)

Dates

  • Created:
    Updated:
    Resolved: