Moodle
  1. Moodle
  2. MDL-36465

Drag and drop a section to general section causes JS error

    Details

    • Testing Instructions:
      Hide
      • Open a course with multiple sections
      • Turn editing on
      • Attempt to drag section 1 to section 2
        • Confirm that all section titles were updated
        • Confirm that the section was moved by JS
      • Reload the page
        • Confirm that the section was still moved
      • Attempt to drag section 2 to section 1
        • Confirm that all section titles were updated
        • Confirm that the section was moved by JS
      • Reload the page
        • Confirm that the section was still moved
      • Attempt to drag a section to the last position
        • Confirm that all section titles were updated
        • Confirm that the section was moved by JS
      • Reload the page
        • Confirm that the section was still moved
      • Attempt to drag section 1 to section 0 (General)
        • Confirm that it was not possible to drop on 0
        • Confirm that dropping anyway didn't do anything
      Show
      Open a course with multiple sections Turn editing on Attempt to drag section 1 to section 2 Confirm that all section titles were updated Confirm that the section was moved by JS Reload the page Confirm that the section was still moved Attempt to drag section 2 to section 1 Confirm that all section titles were updated Confirm that the section was moved by JS Reload the page Confirm that the section was still moved Attempt to drag a section to the last position Confirm that all section titles were updated Confirm that the section was moved by JS Reload the page Confirm that the section was still moved Attempt to drag section 1 to section 0 (General) Confirm that it was not possible to drop on 0 Confirm that dropping anyway didn't do anything
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull 2.4 Branch:
    • Pull 2.5 Branch:
    • Pull Master Branch:
      MDL-36465-master
    • Rank:
      45301

      Description

      When trying to drag and drop a section, in place of the general section, a javascript error occurs and the lightbox containing the loading icon is never hidden.

      Replication steps:

      1. Log in as admin/password
      2. Navigate to a course with existing activities
      3. Turn editing on
      4. Wait for page to reload
      5. Drag a lower section over section 0 (the top section) and release it

      Expected result: Either the section should move to become section 0 or the move should not be possible.

      Actual result: The section is allowed to be dropped on section 0, but the move does not complete. The loading icon appears and the section is blurred. On reloading, the moved section is restored to its original location. The following JS error is given.

      TypeError: node is null
      http://michael.moodle.local/moodle_master_test_mysql/theme/yui_combo.php?moodle/1352347236/course/dragdrop/dragdrop.js
      Line 105
      

      Line 105 is...

      return Number(node.get('id').replace(/section-/i, ''));
      

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          I was able to reproduce that. I've added replication steps and the JS error.

          Show
          Michael de Raadt added a comment - I was able to reproduce that. I've added replication steps and the JS error.
          Hide
          Andrew Nicols added a comment -

          It's not get_section_id() that's at fault, but the code calling it passing it an undefined object to get a sectionid on.
          It seems that when you drop on the general section, the code tries to get the section in position i-1; where i = 0 (line 202).

          Show
          Andrew Nicols added a comment - It's not get_section_id() that's at fault, but the code calling it passing it an undefined object to get a sectionid on. It seems that when you drop on the general section, the code tries to get the section in position i-1; where i = 0 (line 202).
          Hide
          Frédéric Massart added a comment -

          Here is something else I experienced which might be linked:

          2.4, non-editing teacher, on course page, with capability to move sections

          • The section is moved, but the JS fails and so the loading spinner does not disappear:
          Uncaught TypeError: Cannot call method 'swap' of null format.js:44
          M.course.format.swap_sections format.js:44
          Y.io.on.success yui_combo.php:204
          Y.Subscriber._notify simpleyui.js:10407
          Y.Subscriber.notify simpleyui.js:10436
          Y.CustomEvent._notify simpleyui.js:10122
          Y.CustomEvent._procSubs simpleyui.js:10228
          Y.CustomEvent.fireSimple simpleyui.js:10196
          Y.CustomEvent.fire simpleyui.js:10178
          ET.fire simpleyui.js:11234
          IO._evt simpleyui.js:17510
          IO.success simpleyui.js:17580
          IO._result simpleyui.js:17768
          (anonymous function) simpleyui.js:17793
          
          Show
          Frédéric Massart added a comment - Here is something else I experienced which might be linked: 2.4, non-editing teacher, on course page, with capability to move sections The section is moved, but the JS fails and so the loading spinner does not disappear: Uncaught TypeError: Cannot call method 'swap' of null format.js:44 M.course.format.swap_sections format.js:44 Y.io.on.success yui_combo.php:204 Y.Subscriber._notify simpleyui.js:10407 Y.Subscriber.notify simpleyui.js:10436 Y.CustomEvent._notify simpleyui.js:10122 Y.CustomEvent._procSubs simpleyui.js:10228 Y.CustomEvent.fireSimple simpleyui.js:10196 Y.CustomEvent.fire simpleyui.js:10178 ET.fire simpleyui.js:11234 IO._evt simpleyui.js:17510 IO.success simpleyui.js:17580 IO._result simpleyui.js:17768 (anonymous function) simpleyui.js:17793
          Hide
          Andrew Nicols added a comment -

          I'll need to produce some backport branches for 2.5, and 2.4. 2.3 will also be affected.

          Show
          Andrew Nicols added a comment - I'll need to produce some backport branches for 2.5, and 2.4. 2.3 will also be affected.
          Hide
          Andrew Nicols added a comment -

          Frédéric Massart, can you provide replication instructions for the issue you were seeing?

          Show
          Andrew Nicols added a comment - Frédéric Massart , can you provide replication instructions for the issue you were seeing?
          Hide
          Andrew Nicols added a comment -

          The attached patch changes our use of sectionlistselector to allsectionlistselector. We then loop through the list of all sections, and add a new class to all sections except the first and create a new selector moveablesectionlistselector which we use for the DD delegation.

          We need to keep a reference to the list of all sections because this is used to create a selection at other times which is then passed to the course formats which expect all sections to be listed (even the first). We can't (easily?) fix this without breaking compatibility which we obviously can't do for backport branches.

          Longer term, I wonder whether we're doing all of the course JS slightly wrong and should drastically simplify things, but that's not part of this issue.

          Show
          Andrew Nicols added a comment - The attached patch changes our use of sectionlistselector to allsectionlistselector. We then loop through the list of all sections, and add a new class to all sections except the first and create a new selector moveablesectionlistselector which we use for the DD delegation. We need to keep a reference to the list of all sections because this is used to create a selection at other times which is then passed to the course formats which expect all sections to be listed (even the first). We can't (easily?) fix this without breaking compatibility which we obviously can't do for backport branches. Longer term, I wonder whether we're doing all of the course JS slightly wrong and should drastically simplify things, but that's not part of this issue.
          Hide
          Marina Glancy added a comment -

          Hi Andrew, there see to be a little duplicated logic with function setup_for_section where we already determine which sections are movable

          Show
          Marina Glancy added a comment - Hi Andrew, there see to be a little duplicated logic with function setup_for_section where we already determine which sections are movable
          Hide
          Andrew Nicols added a comment -

          Updated as per your suggestion - thanks makes life a touch simpler!

          Show
          Andrew Nicols added a comment - Updated as per your suggestion - thanks makes life a touch simpler!
          Hide
          Marina Glancy added a comment -

          looks better now, thanks.
          But after you changed this, this piece of code seems to have not much sense:

          this.movablesectionlistselector = '.' + CSS.COURSECONTENT + ' .' + CSS.SECTIONDRAGGABLE;
          var nodeselector = this.movablesectionlistselector.slice(CSS.COURSECONTENT.length+2);
          

          Does not look to me as we need this 'movablesectionlistselector' thing at all.

          Also not sure I understand the purpose of renaming sectionlistselector into allsectionlistselector, except that it makes the diff bigger.

          After all this it seems like a small change but it was not so easy to find it. Well done!

          Show
          Marina Glancy added a comment - looks better now, thanks. But after you changed this, this piece of code seems to have not much sense: this .movablesectionlistselector = '.' + CSS.COURSECONTENT + ' .' + CSS.SECTIONDRAGGABLE; var nodeselector = this .movablesectionlistselector.slice(CSS.COURSECONTENT.length+2); Does not look to me as we need this 'movablesectionlistselector' thing at all. Also not sure I understand the purpose of renaming sectionlistselector into allsectionlistselector, except that it makes the diff bigger. After all this it seems like a small change but it was not so easy to find it. Well done!
          Hide
          Frédéric Massart added a comment - - edited

          Hi Andrew,

          Apparently this is not so much of an issue any more. I just replicated the steps I wrote in the description of this issue and it went OK. Though doing it one more time caused the following:

          Uncaught TypeError: Cannot call method 'get' of null yui_combo.php?moodle/-1/course/dragdrop/dragdrop-min.js:104
          Y.extend.get_section_id yui_combo.php?moodle/-1/course/dragdrop/dragdrop-min.js:104
          Y.io.on.success yui_combo.php?moodle/-1/course/dragdrop/dragdrop-min.js:198
          e.Subscriber._notify simpleyui-min.js:14
          e.Subscriber.notify simpleyui-min.js:14
          e.CustomEvent._notify simpleyui-min.js:13
          e.CustomEvent._procSubs simpleyui-min.js:13
          e.CustomEvent.fireSimple simpleyui-min.js:13
          e.CustomEvent.fire simpleyui-min.js:13
          E.fire simpleyui-min.js:14
          o._evt simpleyui-min.js:23
          o.success simpleyui-min.js:23
          o._result simpleyui-min.js:23
          (anonymous function) simpleyui-min.js:23
          

          I also noticed that the General section doesn't have a Drag & Drop handle, which means that either it should have one, or we should prevent dragging a section above the general one.

          I hope this help.

          Cheers,
          Fred

          Edit: Refreshing the page shows that nothing happened.

          Show
          Frédéric Massart added a comment - - edited Hi Andrew, Apparently this is not so much of an issue any more. I just replicated the steps I wrote in the description of this issue and it went OK. Though doing it one more time caused the following: Uncaught TypeError: Cannot call method 'get' of null yui_combo.php?moodle/-1/course/dragdrop/dragdrop-min.js:104 Y.extend.get_section_id yui_combo.php?moodle/-1/course/dragdrop/dragdrop-min.js:104 Y.io.on.success yui_combo.php?moodle/-1/course/dragdrop/dragdrop-min.js:198 e.Subscriber._notify simpleyui-min.js:14 e.Subscriber.notify simpleyui-min.js:14 e.CustomEvent._notify simpleyui-min.js:13 e.CustomEvent._procSubs simpleyui-min.js:13 e.CustomEvent.fireSimple simpleyui-min.js:13 e.CustomEvent.fire simpleyui-min.js:13 E.fire simpleyui-min.js:14 o._evt simpleyui-min.js:23 o.success simpleyui-min.js:23 o._result simpleyui-min.js:23 (anonymous function) simpleyui-min.js:23 I also noticed that the General section doesn't have a Drag & Drop handle, which means that either it should have one, or we should prevent dragging a section above the general one. I hope this help. Cheers, Fred Edit: Refreshing the page shows that nothing happened.
          Hide
          Andrew Nicols added a comment -

          Apologies for all of the noise there - using new tools and need to adjust my workflow a little.

          I've reduced the diff - I was trying to make it clear that the sectionlistselector included all sections, even those which are not moveable.

          Yup - we no longer need the moveable section list, and nodeselector is also a little pointless so I've simplified that too.

          Any chance of one last PR before pushing for IR?

          Cheers

          Show
          Andrew Nicols added a comment - Apologies for all of the noise there - using new tools and need to adjust my workflow a little. I've reduced the diff - I was trying to make it clear that the sectionlistselector included all sections, even those which are not moveable. Yup - we no longer need the moveable section list, and nodeselector is also a little pointless so I've simplified that too. Any chance of one last PR before pushing for IR? Cheers
          Hide
          Marina Glancy added a comment -

          awesome, thanks a lot. All looks great!

          Show
          Marina Glancy added a comment - awesome, thanks a lot. All looks great!
          Hide
          Sam Hemelryk added a comment -

          Thanks Andrew - this has been integrated now.

          Show
          Sam Hemelryk added a comment - Thanks Andrew - this has been integrated now.
          Hide
          Michael de Raadt added a comment -

          I tested this on 2.4, 2.5 and master.

          I ran into a few problems.

          • Sections are still cumbersome to shift, especially where the section is large and scrolling is needed to move it.
          • Sections were shifting and section titles were updating, but often the section content will not be swapped. Refreshing the page corrects the issue, bringing the content into the correct section.
          • Sometimes no drop-location appears but dropping the section still affects a swap.

          One thing that did work was not being able to drop on section 0. That was the original issue reported.

          I'm not sure what the eventual scope of this change was. Following the testing instructions, though, there are several points where the test fails.

          Show
          Michael de Raadt added a comment - I tested this on 2.4, 2.5 and master. I ran into a few problems. Sections are still cumbersome to shift, especially where the section is large and scrolling is needed to move it. Sections were shifting and section titles were updating, but often the section content will not be swapped. Refreshing the page corrects the issue, bringing the content into the correct section. Sometimes no drop-location appears but dropping the section still affects a swap. One thing that did work was not being able to drop on section 0. That was the original issue reported. I'm not sure what the eventual scope of this change was. Following the testing instructions, though, there are several points where the test fails.
          Hide
          Marina Glancy added a comment -

          Michael, there is another open issue about sections d&d: MDL-34209

          Show
          Marina Glancy added a comment - Michael, there is another open issue about sections d&d: MDL-34209
          Hide
          Marina Glancy added a comment -

          and btw another issue in this integration cycle (also failed) MDL-40811. It looks to me that this patch is not related to problems you see. Not sure though if they are current regression or existing bug

          Show
          Marina Glancy added a comment - and btw another issue in this integration cycle (also failed) MDL-40811 . It looks to me that this patch is not related to problems you see. Not sure though if they are current regression or existing bug
          Hide
          Michael de Raadt added a comment -

          I noted that the problems I encountered appeared in the stable releases, so I doubt this change was responsible. MDL-34209 does look relevant. Perhaps that should have been integrated before this issue.

          It would be good if Andrew could confirm any of this.

          Show
          Michael de Raadt added a comment - I noted that the problems I encountered appeared in the stable releases, so I doubt this change was responsible. MDL-34209 does look relevant. Perhaps that should have been integrated before this issue. It would be good if Andrew could confirm any of this.
          Hide
          Andrew Nicols added a comment -

          Cheers Michael,

          As you say, I suspect that the issues you've highlighted are unrelated bugs, in which case this issue should pass IMO.

          • The upstream DD.Scroll isn't brilliant and we may be potentially better to rewrite it. I know that Ryan Grove (YUI contributor) was looking at rewriting YUI DD entirely, but I don't know how far he's got with it. I'll have to look and see whether we can improve the scrolling more generally.
          • I've not seen the content issue you describe - I'll have another look at replicating that, but again I suspect it's unrelated to this issue
          • I suspect that a drop location is appearing, but that you may not see it. Possibly related to the issue Marina Glancy found in MDL-34209 (possibly not)

          MDL-40811 probably failed because of MDL-41208.

          I think that this issue should be renamed to reflect that it only addresses the zero-section issue, and probably then passed.

          Cheers,

          Andrew

          Show
          Andrew Nicols added a comment - Cheers Michael, As you say, I suspect that the issues you've highlighted are unrelated bugs, in which case this issue should pass IMO. The upstream DD.Scroll isn't brilliant and we may be potentially better to rewrite it. I know that Ryan Grove (YUI contributor) was looking at rewriting YUI DD entirely, but I don't know how far he's got with it. I'll have to look and see whether we can improve the scrolling more generally. I've not seen the content issue you describe - I'll have another look at replicating that, but again I suspect it's unrelated to this issue I suspect that a drop location is appearing, but that you may not see it. Possibly related to the issue Marina Glancy found in MDL-34209 (possibly not) MDL-40811 probably failed because of MDL-41208 . I think that this issue should be renamed to reflect that it only addresses the zero-section issue, and probably then passed. Cheers, Andrew
          Hide
          Michael de Raadt added a comment -

          If the fix, summary and testing instructions focus only on dropping onto section 0, I'm happy for this to pass.

          Show
          Michael de Raadt added a comment - If the fix, summary and testing instructions focus only on dropping onto section 0, I'm happy for this to pass.
          Hide
          Marina Glancy added a comment -

          Thanks Michael, I restarted the testing then. This issue is only about "general section" (= 0-section).
          The problems you listed are present on stable versions as well. I'm 99% sure it is the MDL-34209

          Show
          Marina Glancy added a comment - Thanks Michael, I restarted the testing then. This issue is only about "general section" (= 0-section). The problems you listed are present on stable versions as well. I'm 99% sure it is the MDL-34209
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Passing on behalf of Michael, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Passing on behalf of Michael, thanks!
          Hide
          Marina Glancy added a comment -

          And THANK YOU again for making Moodle better every day!

          Another weekly release has been released.

          Show
          Marina Glancy added a comment - And THANK YOU again for making Moodle better every day! Another weekly release has been released.

            People

            • Votes:
              7 Vote for this issue
              Watchers:
              12 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: