Moodle
  1. Moodle
  2. MDL-31052

update Course AJAX/javascript to YUI3 and replace section_classes.js

    Details

    • Testing Instructions:
      Hide

      See the individual bugs for the actual test instructions

      • MDL-31222 has no test insructions as it is not possible to test this in isolation
      • MDL-31216 has test instructions for the drag/drop moving
      • MDL-31096 has test instructions for the rest of the resources and sections toolbox areas
      Show
      See the individual bugs for the actual test instructions MDL-31222 has no test insructions as it is not possible to test this in isolation MDL-31216 has test instructions for the drag/drop moving MDL-31096 has test instructions for the rest of the resources and sections toolbox areas
    • Difficulty:
      Difficult
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-31052-js-rewrite
    • Rank:
      37466

      Description

      MDL-31096 (rewrite JS) is composed of two commits.
      https://git.luns.net.uk/?p=moodle.git;a=commit;h=9eb9fb1e07c10610aa7ad5a41bfd5a2ae83f2bb6
      The first adds a new coursebase JS module which allows JS modules to register themselves and be called later. This will be important later on when Davo adds dynamic creation of files within the course. He'll be able to simply extend moodle-course-coursebase and then call:

      // code to create new course module
      var newmodule = Y.Node.create(response.newmodule);
      M.course.coursebase.invoke_function('setup_for_resource', newmodule);
      

      which will then call any registered JS module which defines setup_for_resource. At present both the general toolbox and the drag/drop module do so. There's also a function for setup_for_section so that a new dynamically created section can also be handled appropriately.
      https://git.luns.net.uk/?p=moodle.git;a=commit;h=2e7630c1c5aed537396daa004e297dd8c1cef9c1
The second commit is a complete rewrite of the old javascript including:

      • lib/ajax/ajaxcourse.js
        * lib/ajax/section_classes.js


      both of which have been removed entirely.

      The original drag/drop functionality is completed removed in these patches, but is added back later by Ruslan's commits.

      https://git.luns.net.uk/?p=moodle.git;a=commit;h=aed1c3facfb5abc2c0340641c22d0851fab994e9
      We've also included a master-only patch for MDL-25990. Another patch should be written for current stable branches.

      MDL-31222 Create a core YUI3 Module for drag-drop
      This patch creates a core drag-drop module M.core.dragdrop that can be used from everywhere (where drag-drop is required), it provides some methods with general functionality, like creating the dragger element, locking it when dragging is not allowed, making initial setup for dragging element, tracking moving direction, visually shifting elements to insert a draggable one in between.

      MDL-31216 Create dragdrop course module

      This patch is applying new drag-drop module to the course page. It contains two modules, both of which is extending M.core.dragdrop. The first one is applying dragdrop to sections. Its init method goes through each existing section and making them draggable and drop target. drop_hit is doing ajax request to update the section order on back-end, and then resort sections order on the frontend. The second module making resources (modules) draggable and also makes each section a drop target, so that resource can be dragged to empty section. Each module is using a separate drag-drop group, so that YUI will not get confused and insert resource between sections for example.

      MDL-31720 Refactor course/rest.php
      This just makes course/rest.php cleanser and removes obsolete bits of code.

      MDL-31263 Apply dragdrop to course blocks
      Relatively large thing, but well-commented hopefully. This allows to drag blocks. It works on the course page for the moment, but on a way to be used everywhere (as you may guess from the way how it is initialised). It is implemented the similar way to section/resources dragdrop. The tricky bit is back-end to store the result, looks pretty hacky attempting to deceive Moodle that we deal with the normal page with blocks, etc., if you have suggestion how to simplify, please let me know . Another tricky bit is drop_over, which is checking if the block region is empty (last block has been dragged out) and the opposite scenario when block is being dragged towards empty region.
      A big TODO is to write wiki pages for drag-drop and Andrew's COURSEBASE stuff, so that people would know how to use it and start using straight away.

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          That sounds pretty significant. I'm looking forward to seeing the result.

          Show
          Michael de Raadt added a comment - That sounds pretty significant. I'm looking forward to seeing the result.
          Hide
          Andrew Nicols added a comment -

          I'm separating out the resource and section parts of this task into subtasks as I suspect they'll probably end up being written as two YUI modules.

          Barring the drag-n-drop which Ruslan is working on, I've written the resource module.

          Show
          Andrew Nicols added a comment - I'm separating out the resource and section parts of this task into subtasks as I suspect they'll probably end up being written as two YUI modules. Barring the drag-n-drop which Ruslan is working on, I've written the resource module.
          Hide
          Andrew Nicols added a comment -

          MDL-31096 will fix this marker issue for 2.3 onwards

          Show
          Andrew Nicols added a comment - MDL-31096 will fix this marker issue for 2.3 onwards
          Hide
          Ruslan Kabalin added a comment -

          Function from MDL-31255 is required.

          Show
          Ruslan Kabalin added a comment - Function from MDL-31255 is required.
          Hide
          Andrew Nicols added a comment -

          We've completely rewritten and gotten rid of a number of old yui2 JavaScript and replaced it with brand spanking new YUI3 modules.

          Show
          Andrew Nicols added a comment - We've completely rewritten and gotten rid of a number of old yui2 JavaScript and replaced it with brand spanking new YUI3 modules.
          Show
          Andrew Nicols added a comment - The top three commits in the diffurl relate to the changes we've been making and should correspond to the subtasks of this bug. The individual diffs are: MDL-31096 - https://git.luns.net.uk/?p=moodle.git;a=commitdiff;h=a02372f9263e0647feea4926c412971a09413710 MDL-31222 - https://git.luns.net.uk/?p=moodle.git;a=commitdiff;h=61d750c49e28fcd473f89d9b2e1476d51edb4883 MDL-31216 - https://git.luns.net.uk/?p=moodle.git;a=commitdiff;h=007626010feb0777f39e654f82a1eb826e0f4063
          Hide
          Andrew Nicols added a comment -

          I've put together a screencast which demonstrates the updated/improved functionality at http://goo.gl/mPxR7 and opened a discussion at http://moodle.org/mod/forum/discuss.php?d=195058

          Show
          Andrew Nicols added a comment - I've put together a screencast which demonstrates the updated/improved functionality at http://goo.gl/mPxR7 and opened a discussion at http://moodle.org/mod/forum/discuss.php?d=195058
          Hide
          Michael de Raadt added a comment -

          I'm assigning Martin as peer reviewer so it can be considered for an upcoming major release.

          Show
          Michael de Raadt added a comment - I'm assigning Martin as peer reviewer so it can be considered for an upcoming major release.
          Show
          Andrew Nicols added a comment - I've updated the patch with some minor fixes and improvements we've come across whilst testing and extending this to support moving blocks. MDL-31096 : https://git.luns.net.uk/?p=moodle.git;a=commitdiff;h=756de1d4f44913f75cbf008be9bc63f50e9bbe1f MDL-31222 : https://git.luns.net.uk/?p=moodle.git;a=commitdiff;h=dec5c82b374a0854495d93a7a25858e1e7c7d03a MDL-31216 : https://git.luns.net.uk/?p=moodle.git;a=commitdiff;h=6d99f8d76f6590fcb18d17822a2e16000e4d274d
          Hide
          Ruslan Kabalin added a comment -

          Updated the branch, added MDL-25990 and MDL-31720.

          I think it can be reviewed now. Though I still have not entirely finished with blocks dragdrop (well, they just do not work on absolutely every page yet), it will be easy to add it later.

          Show
          Ruslan Kabalin added a comment - Updated the branch, added MDL-25990 and MDL-31720 . I think it can be reviewed now. Though I still have not entirely finished with blocks dragdrop (well, they just do not work on absolutely every page yet), it will be easy to add it later.
          Show
          Ruslan Kabalin added a comment - The current list of diffs: MDL-31096 : https://git.luns.net.uk/?p=moodle.git;a=commitdiff;h=756de1d4f44913f75cbf008be9bc63f50e9bbe1f MDL-31222 : https://git.luns.net.uk/?p=moodle.git;a=commitdiff;h=1bd44256ad59e3b17db10504d1dc300353c4975c MDL-31216 : https://git.luns.net.uk/?p=moodle.git;a=commitdiff;h=4169cbb6c7fea09112dba96673c2513c6f9970b2 MDL-31720 : https://git.luns.net.uk/?p=moodle.git;a=commitdiff;h=fe5c2c445e2052fd864e75bb91c1aaa6eb103c3f Also the branch fixes: MDL-25990 : https://git.luns.net.uk/?p=moodle.git;a=commitdiff;h=14ce3a6cbccac0c0c83f5402983a8f6007b75c7a
          Hide
          Dan Poltawski added a comment -

          Could you think about testing with the change in MDL-22504 - there is going to be some concurrency here.

          Also could you see wether there is an impact on the ou subpage contrib module?

          Show
          Dan Poltawski added a comment - Could you think about testing with the change in MDL-22504 - there is going to be some concurrency here. Also could you see wether there is an impact on the ou subpage contrib module?
          Hide
          Dan Poltawski added a comment -

          Hi Guys,

          I'm starting work on this now. What would be really helpful for me and other reviewers is if you can do a 'written summary' of the changes, bug numbers and diffs aren't really any more descriptive than reviewing the commits themselves. Also there is a lot of history of diffs here. So a high level summary of changes would be useful.

          Show
          Dan Poltawski added a comment - Hi Guys, I'm starting work on this now. What would be really helpful for me and other reviewers is if you can do a 'written summary' of the changes, bug numbers and diffs aren't really any more descriptive than reviewing the commits themselves. Also there is a lot of history of diffs here. So a high level summary of changes would be useful.
          Hide
          Dan Poltawski added a comment -

          I don't see the block drag/drop thing?

          Show
          Dan Poltawski added a comment - I don't see the block drag/drop thing?
          Hide
          Davo Smith added a comment -

          On the MDL-22504 (course drag and drop upload) front - there are basically 2 things my javascript code needs to be able to do (in relation to course AJAX):
          1. detect if AJAX course editing is active (currently done by looking for the object 'main', but a better method would be appreciated)
          2. once I've constructed a new course module element (an 'li' tag with icon, link and non-AJAX editing icons) and added it to the course page DOM, I need to call a function to transform it into an AJAX-editable item (with the new icons and registered with any internal lists you have in your code)

          Are you able to provide a couple of functions to do this?

          Show
          Davo Smith added a comment - On the MDL-22504 (course drag and drop upload) front - there are basically 2 things my javascript code needs to be able to do (in relation to course AJAX): 1. detect if AJAX course editing is active (currently done by looking for the object 'main', but a better method would be appreciated) 2. once I've constructed a new course module element (an 'li' tag with icon, link and non-AJAX editing icons) and added it to the course page DOM, I need to call a function to transform it into an AJAX-editable item (with the new icons and registered with any internal lists you have in your code) Are you able to provide a couple of functions to do this?
          Hide
          Dan Poltawski added a comment -

          Some initial comments:

          1. Despite on the box looking like 'just a code improvement' this also has some really nice usability improvements too. Great work!
            • The sections move around and give a visual hint of where they are going
            • Progress spinners to give feedback that something is happening in the backend
            • The visual clues to where a course module moves
            • The fact that things can't be dragged outside of their bounds
          2. How does the 'restriction of where things can be dragged' work? Will it break on a course format which splits the sections up into two columns?
          3. Discussing with Martin - we'd want to ensure ajax course editing was on by default with this being a robust solution. Our users love this drag/drop stuff, but its previously been a bit creaky.
          Show
          Dan Poltawski added a comment - Some initial comments: Despite on the box looking like 'just a code improvement' this also has some really nice usability improvements too. Great work! The sections move around and give a visual hint of where they are going Progress spinners to give feedback that something is happening in the backend The visual clues to where a course module moves The fact that things can't be dragged outside of their bounds How does the 'restriction of where things can be dragged' work? Will it break on a course format which splits the sections up into two columns? Discussing with Martin - we'd want to ensure ajax course editing was on by default with this being a robust solution. Our users love this drag/drop stuff, but its previously been a bit creaky.
          Hide
          Andrew Nicols added a comment -

          We've updated all commits for this new functionality. Here's a quick summary of my parts. Ruslan will comment on his bits too.

          MDL-31096 (rewrite JS) is composed of two commits.

          https://git.luns.net.uk/?p=moodle.git;a=commit;h=9eb9fb1e07c10610aa7ad5a41bfd5a2ae83f2bb6
          The first adds a new coursebase JS module which allows JS modules to register themselves and be called later. This will be important later on when Davo adds dynamic creation of files within the course. He'll be able to simply extend moodle-course-coursebase and then call:

          // code to create new course module
          var newmodule = Y.Node.create(response.newmodule);
          M.course.coursebase.invoke_function('setup_for_resource', newmodule);
          

          which will then call any registered JS module which defines setup_for_resource. At present both the general toolbox and the drag/drop module do so. There's also a function for setup_for_section so that a new dynamically created section can also be handled appropriately.

          https://git.luns.net.uk/?p=moodle.git;a=commit;h=2e7630c1c5aed537396daa004e297dd8c1cef9c1
          The second commit is a complete rewrite of the old javascript including:

          • lib/ajax/ajaxcourse.js
          • lib/ajax/section_classes.js
            both of which have been removed entirely.

          The original drag/drop functionality is completed removed in these patches, but is added back later by Ruslan's commits.

          https://git.luns.net.uk/?p=moodle.git;a=commit;h=aed1c3facfb5abc2c0340641c22d0851fab994e9
          We've also included a master-only patch for MDL-25990. Another patch should be written for current stable branches.

          Ruslan will give a quick overview of the second part of this

          Show
          Andrew Nicols added a comment - We've updated all commits for this new functionality. Here's a quick summary of my parts. Ruslan will comment on his bits too. MDL-31096 (rewrite JS) is composed of two commits. https://git.luns.net.uk/?p=moodle.git;a=commit;h=9eb9fb1e07c10610aa7ad5a41bfd5a2ae83f2bb6 The first adds a new coursebase JS module which allows JS modules to register themselves and be called later. This will be important later on when Davo adds dynamic creation of files within the course. He'll be able to simply extend moodle-course-coursebase and then call: // code to create new course module var newmodule = Y.Node.create(response.newmodule); M.course.coursebase.invoke_function('setup_for_resource', newmodule); which will then call any registered JS module which defines setup_for_resource. At present both the general toolbox and the drag/drop module do so. There's also a function for setup_for_section so that a new dynamically created section can also be handled appropriately. https://git.luns.net.uk/?p=moodle.git;a=commit;h=2e7630c1c5aed537396daa004e297dd8c1cef9c1 The second commit is a complete rewrite of the old javascript including: lib/ajax/ajaxcourse.js lib/ajax/section_classes.js both of which have been removed entirely. The original drag/drop functionality is completed removed in these patches, but is added back later by Ruslan's commits. https://git.luns.net.uk/?p=moodle.git;a=commit;h=aed1c3facfb5abc2c0340641c22d0851fab994e9 We've also included a master-only patch for MDL-25990 . Another patch should be written for current stable branches. Ruslan will give a quick overview of the second part of this
          Hide
          Ruslan Kabalin added a comment -

          Right, my turn to present the rest of updates, thanks Andrew.

          MDL-31222 Create a core YUI3 Module for drag-drop (diff)

          This patch creates a core drag-drop module M.core.dragdrop that can be used from everywhere (where drag-drop is required), it provides some methods with general functionality, like creating the dragger element, locking it when dragging is not allowed, making initial setup for dragging element, tracking moving direction, visually shifting elements to insert a draggable one in between.

          MDL-31216 Create dragdrop course module (diff)

          This patch is applying new drag-drop module to the course page. It contains two modules, both of which is extending M.core.dragdrop. The first one is applying dragdrop to sections. Its init method goes through each existing section and making them draggable and drop target. drop_hit is doing ajax request to update the section order on back-end, and then resort sections order on the frontend. The second module making resources (modules) draggable and also makes each section a drop target, so that resource can be dragged to empty section. Each module is using a separate drag-drop group, so that YUI will not get confused and insert resource between sections for example.

          MDL-31720 Refactor course/rest.php (diff)

          This just makes course/rest.php cleanser and removes obsolete bits of code.

          MDL-31263 Apply dragdrop to course blocks (diff)

          Relatively large thing, but well-commented hopefully. This allows to drag blocks. It works on the course page for the moment, but on a way to be used everywhere (as you may guess from the way how it is initialised). It is implemented the similar way to section/resources dragdrop. The tricky bit is back-end to store the result, looks pretty hacky attempting to deceive Moodle that we deal with the normal page with blocks, etc., if you have suggestion how to simplify, please let me know . Another tricky bit is drop_over, which is checking if the block region is empty (last block has been dragged out) and the opposite scenario when block is being dragged towards empty region.

          A big TODO is to write wiki pages for drag-drop and Andrew's COURSEBASE stuff, so that people would know how to use it and start using straight away.

          Show
          Ruslan Kabalin added a comment - Right, my turn to present the rest of updates, thanks Andrew. MDL-31222 Create a core YUI3 Module for drag-drop ( diff ) This patch creates a core drag-drop module M.core.dragdrop that can be used from everywhere (where drag-drop is required), it provides some methods with general functionality, like creating the dragger element, locking it when dragging is not allowed, making initial setup for dragging element, tracking moving direction, visually shifting elements to insert a draggable one in between. MDL-31216 Create dragdrop course module ( diff ) This patch is applying new drag-drop module to the course page. It contains two modules, both of which is extending M.core.dragdrop. The first one is applying dragdrop to sections. Its init method goes through each existing section and making them draggable and drop target. drop_hit is doing ajax request to update the section order on back-end, and then resort sections order on the frontend. The second module making resources (modules) draggable and also makes each section a drop target, so that resource can be dragged to empty section. Each module is using a separate drag-drop group, so that YUI will not get confused and insert resource between sections for example. MDL-31720 Refactor course/rest.php ( diff ) This just makes course/rest.php cleanser and removes obsolete bits of code. MDL-31263 Apply dragdrop to course blocks ( diff ) Relatively large thing, but well-commented hopefully. This allows to drag blocks. It works on the course page for the moment, but on a way to be used everywhere (as you may guess from the way how it is initialised). It is implemented the similar way to section/resources dragdrop. The tricky bit is back-end to store the result, looks pretty hacky attempting to deceive Moodle that we deal with the normal page with blocks, etc., if you have suggestion how to simplify, please let me know . Another tricky bit is drop_over, which is checking if the block region is empty (last block has been dragged out) and the opposite scenario when block is being dragged towards empty region. A big TODO is to write wiki pages for drag-drop and Andrew's COURSEBASE stuff, so that people would know how to use it and start using straight away.
          Hide
          Dan Poltawski added a comment -

          Added the changeset decription to the bug description

          Show
          Dan Poltawski added a comment - Added the changeset decription to the bug description
          Hide
          Dan Poltawski added a comment - - edited

          Had a good look at this code and I think this is a big improvement and much more maintainable and great we have block drag/drop back!

          This is what I think needs to be fixed before we integrate:

          1. Site frontpage is not working (this is a new feature):
            • Problems:
              • When no topic section Notice: Undefined variable: modnamesused in /Users/danp/git/moodle/index.php on line 249
              • JS errors on load with Uncaught TypeError: Cannot call method 'hasClass' of null
              • Trying to click on the show/hide button on an activity:
                Uncaught TypeError: Cannot call method 'hasClass' of null
                Y.extend.toggle_hide_resourceyui_combo.php:228
                b.Subscriber._notifyyui_combo.php:13
                b.Subscriber.notifyyui_combo.php:14
                b.CustomEvent._notifyyui_combo.php:13
                b.CustomEvent._procSubsyui_combo.php:13
                b.CustomEvent.fireSimpleyui_combo.php:13
                b.CustomEvent.fireyui_combo.php:13
                r._createWrapper.C.fn
                
            • Solutions
              • Don't load the JS on site front page for now (it wasn't working in the existing code so this is new functionality)
              • Fix this up and code much more defensively to be robust at dealing problems like variances in course format such as this. The problem here is the JS disables access to the 'non-JS' functionality, so if this breaks it is not possible to do actions without disabling JS in your browser. Note that I tested with the grid course format and we had similar issues there.
          2. When rearranging weekly sections the week doesn't update. I fixed this here: https://github.com/danpoltawski/moodle/commit/1ab4b0b0eac0f6ee93a9ece3aaf669d21a9a2ac1
          3. When '$CFG->enablecourseajax' is disabled things change in the UI but silently fail in the background.
          4. course_format_ajax_support() - I think [for now] it should be observed for the 'toolbox' things as it will prevent issues like I noted in 1).

          Post Integration Cleanup/Considerations

          1. This change is an improvement over the existing course ajax, but its still tightly coupled with course formats I think - the section rearranging JS in particular. (For example the week topic name change I commited above). I think we need a way for course formats to be able to 'plugin' to this. I think this is a post-integration task as the formats are in flux.
          2. When we 'dock' a block the 'grab handle' is left there but the block can't be grabbed. Pretty minor since it reverts to the non-JS move icon when page is reloaded.
          3. Calling $PAGE->blocks->process_url_move(); and the small GET param hack. I think we could refactor and add a block API function (e..g move_block()) to do this with explicit params function and then use this in lib/ajax/blocks.php and convert process_url_move() to call that.
          Show
          Dan Poltawski added a comment - - edited Had a good look at this code and I think this is a big improvement and much more maintainable and great we have block drag/drop back! This is what I think needs to be fixed before we integrate: Site frontpage is not working (this is a new feature): Problems: When no topic section Notice: Undefined variable: modnamesused in /Users/danp/git/moodle/index.php on line 249 JS errors on load with Uncaught TypeError: Cannot call method 'hasClass' of null Trying to click on the show/hide button on an activity: Uncaught TypeError: Cannot call method 'hasClass' of null Y.extend.toggle_hide_resourceyui_combo.php:228 b.Subscriber._notifyyui_combo.php:13 b.Subscriber.notifyyui_combo.php:14 b.CustomEvent._notifyyui_combo.php:13 b.CustomEvent._procSubsyui_combo.php:13 b.CustomEvent.fireSimpleyui_combo.php:13 b.CustomEvent.fireyui_combo.php:13 r._createWrapper.C.fn Solutions Don't load the JS on site front page for now (it wasn't working in the existing code so this is new functionality) Fix this up and code much more defensively to be robust at dealing problems like variances in course format such as this. The problem here is the JS disables access to the 'non-JS' functionality, so if this breaks it is not possible to do actions without disabling JS in your browser. Note that I tested with the grid course format and we had similar issues there. When rearranging weekly sections the week doesn't update. I fixed this here: https://github.com/danpoltawski/moodle/commit/1ab4b0b0eac0f6ee93a9ece3aaf669d21a9a2ac1 When '$CFG->enablecourseajax' is disabled things change in the UI but silently fail in the background. First of all, ignore for now that we are getting rid of this in MDL-32509 (there is at least going to be $CFG->enableajax to respect) and make the JS loading respect this. (We will need to look at all of this in MDL-32509 ) I made course/rest.php throw moodle exceptions which gives debugging info about this at least: https://github.com/danpoltawski/moodle/commit/340c6ae9f81c50c0a0b298be55b0da59b08421bf I didn't test how the exceptions are dealt with in non-developer mode. I also made the course/toolbox not hide errors: https://github.com/danpoltawski/moodle/commit/728d8bd448c007a3e277469d3f56f8fe6099d934 course_format_ajax_support() - I think [for now] it should be observed for the 'toolbox' things as it will prevent issues like I noted in 1). Post Integration Cleanup/Considerations This change is an improvement over the existing course ajax, but its still tightly coupled with course formats I think - the section rearranging JS in particular. (For example the week topic name change I commited above). I think we need a way for course formats to be able to 'plugin' to this. I think this is a post-integration task as the formats are in flux. When we 'dock' a block the 'grab handle' is left there but the block can't be grabbed. Pretty minor since it reverts to the non-JS move icon when page is reloaded. Calling $PAGE->blocks->process_url_move(); and the small GET param hack. I think we could refactor and add a block API function (e..g move_block()) to do this with explicit params function and then use this in lib/ajax/blocks.php and convert process_url_move() to call that.
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          Dan asked me to take a look at this if I got the chance and I've managed to find a few minutes to have a look at the code (have only very breifly tested it).
          Adding to the list of things Dan has already noted:

          1. course/lib.php
            • Just as an idea perhaps it is worth passing the desired moodle_page object in as an argument to include_course_ajax rather than using the global $PAGE. I don't see any bugs with the current approach at all, but what does strike me about that function is that it is purely setting the moodle_page object up so that it is ready for course ajax. There are a couple of places where we use two moodle_page objects (block editing is an example of one, theme switching another) but I don't think under any foreseeable circumstance this method would be called in the situation. So... totally up to you if you want to switch to an arg or not but it will be more futureproof if you do (although an easy conversion in the future so no probs)
            • Given you are loading so many strings all from the moodle component why not convert all of those string_for_js calls to a single strings_for_js call? (of course you can keep the conditional and iterative ones separate)
            • set_section_visible phpdocs need updating
          2. lang/en/moodle.php This would be the perfect time to move hide/showtopicsfromothers and hide/showweeksfromothers to their respective course format plugins and change the name to something like hide/showfromothers.
          3. lib/yui/blocks/blocks.js I wonder whether this should be renamed to blockdragdrop.js or something the more closely reflects what it is doing. I even wonder whether calling it courseblockdd.js or something as it is only usable in a course view situation.
          4. lib/ajax/blocks.php I havn't had a really good chance to look into and test this file, however I think it needs to be very carefully looked at, although I can't put my finger on why the code there looks very fragile. If nothing else this script needs to check if the user is editing. As an example open a course as an admin and turn editing on, open a second tab and turn editing off in that tab, back in the first tab move a block. Note that it will be a success despite editing being turned off when the request was made. This is a very minor issue, but an issue none the less and perhaps a security issue.

          More significant areas where I think discussion/ideas are needed.
          blocks.js
          I don't like the hardcoding of CSS classes that is going on here, block-region for instance isn't required to be used by all theme's and in fact isn't used by the boxxie theme. The region-pre and region-post classes are other examples of very hardcoded classes that are likely to break custom themes. Themers can define any block regions they like, as few or as many. Luckily in core only pre and post are used, the exception to that is the mymobile theme which uses a myblocks region.
          I'm only having a quick look over these changes and am not going to look at the existing course ajax code, nor am I familiar with it. I suppose if it is doing this same hardcoding then its not a regression and theres probably no problem with it, providing it is documented.
          However this would be the perfect oppertunity to look for a solution to this issue.
          Themer's being able to define their own block regions and take full control of their layout is a big selling point to theme's in Moodle so we should look for a solution that allows the drag and drop of blocks to work based upon what they define. Getting the block regions a page supports is easy enough, you can inspect the theme config layouts, find the layout that is going to be used and read its regions and defaultregion values (of course has to be completely respectful of parents). Getting the containing elements would require changes or smart thinking, one solution that immediately jumps to mind is to alter core_renderer::blocks_for_region to wrap all blocks in a div with a class/id that is predicatable when the region names are known, e.g. block-region-$region-wrap. The only other concern I thought of in regards to this was that theme's can choose how much layout they print if there are no blocks in a region but I see that block_manager::region_has_content checks if the user is editing so that should not be a problem.
          As a general rule I think hardcoding things produced by a renderer in core is fine (not necessarily ideal but ok) however things that are determined/controlled by the theme should not be hardcoded.

          Ideas:
          It may be worth asking a theme if they support course editing/block drag and drop in the same way we they can choose to use the dock or not. It can default to true no probs, but that way it gives theme's an easy way out if they don't want to make use of them (likley the mymobile theme is a candidate for that).

          While looking into this I did find a bug with the dock when editing is enabled, if you turn editing on, dock a block, and then undock it you loose all of the editing controls.
          This isn't something you have caused as I can reproduce it on a fresh site.

          This is a real nice improvement thanks guys, and you can bet it won't be long before people start asking to see the drag+drop of blocks made possible on other pages.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, Dan asked me to take a look at this if I got the chance and I've managed to find a few minutes to have a look at the code (have only very breifly tested it). Adding to the list of things Dan has already noted: course/lib.php Just as an idea perhaps it is worth passing the desired moodle_page object in as an argument to include_course_ajax rather than using the global $PAGE. I don't see any bugs with the current approach at all, but what does strike me about that function is that it is purely setting the moodle_page object up so that it is ready for course ajax. There are a couple of places where we use two moodle_page objects (block editing is an example of one, theme switching another) but I don't think under any foreseeable circumstance this method would be called in the situation. So... totally up to you if you want to switch to an arg or not but it will be more futureproof if you do (although an easy conversion in the future so no probs) Given you are loading so many strings all from the moodle component why not convert all of those string_for_js calls to a single strings_for_js call? (of course you can keep the conditional and iterative ones separate) set_section_visible phpdocs need updating lang/en/moodle.php This would be the perfect time to move hide/showtopicsfromothers and hide/showweeksfromothers to their respective course format plugins and change the name to something like hide/showfromothers. lib/yui/blocks/blocks.js I wonder whether this should be renamed to blockdragdrop.js or something the more closely reflects what it is doing. I even wonder whether calling it courseblockdd.js or something as it is only usable in a course view situation. lib/ajax/blocks.php I havn't had a really good chance to look into and test this file, however I think it needs to be very carefully looked at, although I can't put my finger on why the code there looks very fragile. If nothing else this script needs to check if the user is editing. As an example open a course as an admin and turn editing on, open a second tab and turn editing off in that tab, back in the first tab move a block. Note that it will be a success despite editing being turned off when the request was made. This is a very minor issue, but an issue none the less and perhaps a security issue. More significant areas where I think discussion/ideas are needed. blocks.js I don't like the hardcoding of CSS classes that is going on here, block-region for instance isn't required to be used by all theme's and in fact isn't used by the boxxie theme. The region-pre and region-post classes are other examples of very hardcoded classes that are likely to break custom themes. Themers can define any block regions they like, as few or as many. Luckily in core only pre and post are used, the exception to that is the mymobile theme which uses a myblocks region. I'm only having a quick look over these changes and am not going to look at the existing course ajax code, nor am I familiar with it. I suppose if it is doing this same hardcoding then its not a regression and theres probably no problem with it, providing it is documented. However this would be the perfect oppertunity to look for a solution to this issue. Themer's being able to define their own block regions and take full control of their layout is a big selling point to theme's in Moodle so we should look for a solution that allows the drag and drop of blocks to work based upon what they define. Getting the block regions a page supports is easy enough, you can inspect the theme config layouts, find the layout that is going to be used and read its regions and defaultregion values (of course has to be completely respectful of parents). Getting the containing elements would require changes or smart thinking, one solution that immediately jumps to mind is to alter core_renderer::blocks_for_region to wrap all blocks in a div with a class/id that is predicatable when the region names are known, e.g. block-region-$region-wrap. The only other concern I thought of in regards to this was that theme's can choose how much layout they print if there are no blocks in a region but I see that block_manager::region_has_content checks if the user is editing so that should not be a problem. As a general rule I think hardcoding things produced by a renderer in core is fine (not necessarily ideal but ok) however things that are determined/controlled by the theme should not be hardcoded. Ideas: It may be worth asking a theme if they support course editing/block drag and drop in the same way we they can choose to use the dock or not. It can default to true no probs, but that way it gives theme's an easy way out if they don't want to make use of them (likley the mymobile theme is a candidate for that). While looking into this I did find a bug with the dock when editing is enabled, if you turn editing on, dock a block, and then undock it you loose all of the editing controls. This isn't something you have caused as I can reproduce it on a fresh site. This is a real nice improvement thanks guys, and you can bet it won't be long before people start asking to see the drag+drop of blocks made possible on other pages. Cheers Sam
          Hide
          Dan Poltawski added a comment -

          In my git repo (git://github.com/danpoltawski/moodle.git) I have two branches related to this now, course-js-rewrite has my fixes on top of your branch (including a new one I just spotted: https://github.com/danpoltawski/moodle/commit/cc43af89aa04e46b23eed806aee2847e0b348271 )

          I have also rebased Davos work onto this branch and converted it to use your callback, seems to work
          dragdrop-upload-on-top-of-MDL-22504

          Show
          Dan Poltawski added a comment - In my git repo (git://github.com/danpoltawski/moodle.git) I have two branches related to this now, course-js-rewrite has my fixes on top of your branch (including a new one I just spotted: https://github.com/danpoltawski/moodle/commit/cc43af89aa04e46b23eed806aee2847e0b348271 ) I have also rebased Davos work onto this branch and converted it to use your callback, seems to work dragdrop-upload-on-top-of- MDL-22504
          Hide
          Dan Poltawski added a comment -

          Found two new issues whilst playing around:

          • In standard theme, drag out all blocks from left hand column, then you are not able to drag them back
          • Breaks badly in binarius theme

          I've linked loads of old bugs to this issue, hoping that they'll fix them all.

          Show
          Dan Poltawski added a comment - Found two new issues whilst playing around: In standard theme, drag out all blocks from left hand column, then you are not able to drag them back Breaks badly in binarius theme I've linked loads of old bugs to this issue, hoping that they'll fix them all.
          Hide
          Andrew Nicols added a comment -

          Thanks both for your speedy reviews!

          Dan's comments first:

          1. Site frontpage is not working (this is a new feature):

          • This is now fixed. I had already fixed this, but I'd done so on the wrong branch and forgot to transport the commit over when I applied it on the correct branch. The fix is extremely trivial
          • I've made the show/hide activity button more resilient in both locations that this was failing.
          • The grid format (and probably others) fail at present because:
            • We've had to add a class to the existing course formats to identify the show/hide button and visibility. This will need to be added to course formats like grid which don't have it already.
            • We've renamed a string which the format is using. Sam has also picked up on this
            • There's one other JS error when using the grid format which I've not yet tracked down... still working on it.

          3. When '$CFG->enablecourseajax' is disabled things change in the UI but silently fail in the background.

          This is something we'll need to look into more. Will try and do this today!

          4. course_format_ajax_support() - I think [for now] it should be observed for the 'toolbox' things as it will prevent issues like I noted in 1).

          This should be trivial to add and we'll look at doing this today.

          Post Integration Cleanup/Considerations

          1. We've already made some big changes to make course formats more hookable (hopefully). With the inclusion of the coursebase code, it should be much easier for a course format to add hooks etc for when other code calls things.

          We have tried to decouple course formats as much as possible here, and we could do so further, but it could be at the cost of maintainability of third-party course formats. For example, toolboxes.js handles both the section visiblity (show/hide) button, and section highlighting. At present the topic format supports highlighting, but the weekly does not. I should imagine that many third-party plugins also support section highlighting. We could:

          • move the highlighting code to the topic course format as a separate JS module which is only called by the topic format and other formats can either include the same JS, or copy it; or
          • leave it as is and ensure that it doesn't break things if toggle_highlight does not exist.

          However, I've just tried to load a some course format JS and I'm not sure whether the existing YUI requires code, or the moodle code support YUI modules in a course format. As an example, try adding 'moodle-course-format-grid-toolbox' to the requirements in toolboxes.js. It's converted to 'course-format/grid/grid.js' by YUI which is converted to a URL like:
          http://kumquat.lancs.ac.uk/moodle/theme/yui_combo.php?moodle/295/course-format/grid/grid.js
          which gives a response like:
          // Combo resource moodle/295/course-format/grid/grid.js (/home/nicols/git/software/moodle/mod/course-format/yui/grid/grid.js) not found!

          I guess we'd have to only allow one YUI module per course format, e.g. moodle-course-format-grid, and modify theme/yui_combo.php to understand course formats appropriately.

          Sam

          2. lang/en/moodle.php

          Agreed. I did consider doing this before, but at present the strings are contextually specific. E.g. 'Hide topic from others'.

          I'll update my parts of the code and work with Ruslan to push an updated version tonight.

          Andrew

          Show
          Andrew Nicols added a comment - Thanks both for your speedy reviews! Dan's comments first: 1. Site frontpage is not working (this is a new feature): This is now fixed. I had already fixed this, but I'd done so on the wrong branch and forgot to transport the commit over when I applied it on the correct branch. The fix is extremely trivial I've made the show/hide activity button more resilient in both locations that this was failing. The grid format (and probably others) fail at present because: We've had to add a class to the existing course formats to identify the show/hide button and visibility. This will need to be added to course formats like grid which don't have it already. We've renamed a string which the format is using. Sam has also picked up on this There's one other JS error when using the grid format which I've not yet tracked down... still working on it. 3. When '$CFG->enablecourseajax' is disabled things change in the UI but silently fail in the background. This is something we'll need to look into more. Will try and do this today! 4. course_format_ajax_support() - I think [for now] it should be observed for the 'toolbox' things as it will prevent issues like I noted in 1). This should be trivial to add and we'll look at doing this today. Post Integration Cleanup/Considerations 1. We've already made some big changes to make course formats more hookable (hopefully). With the inclusion of the coursebase code, it should be much easier for a course format to add hooks etc for when other code calls things. We have tried to decouple course formats as much as possible here, and we could do so further, but it could be at the cost of maintainability of third-party course formats. For example, toolboxes.js handles both the section visiblity (show/hide) button, and section highlighting. At present the topic format supports highlighting, but the weekly does not. I should imagine that many third-party plugins also support section highlighting. We could: move the highlighting code to the topic course format as a separate JS module which is only called by the topic format and other formats can either include the same JS, or copy it; or leave it as is and ensure that it doesn't break things if toggle_highlight does not exist. However, I've just tried to load a some course format JS and I'm not sure whether the existing YUI requires code, or the moodle code support YUI modules in a course format. As an example, try adding 'moodle-course-format-grid-toolbox' to the requirements in toolboxes.js. It's converted to 'course-format/grid/grid.js' by YUI which is converted to a URL like: http://kumquat.lancs.ac.uk/moodle/theme/yui_combo.php?moodle/295/course-format/grid/grid.js which gives a response like: // Combo resource moodle/295/course-format/grid/grid.js (/home/nicols/git/software/moodle/mod/course-format/yui/grid/grid.js) not found! I guess we'd have to only allow one YUI module per course format, e.g. moodle-course-format-grid, and modify theme/yui_combo.php to understand course formats appropriately. Sam 2. lang/en/moodle.php Agreed. I did consider doing this before, but at present the strings are contextually specific. E.g. 'Hide topic from others'. – I'll update my parts of the code and work with Ruslan to push an updated version tonight. Andrew
          Hide
          Andrew Nicols added a comment -

          Ignore my comment about yui not supporting course format JS:

          moodle-format_grid-example

          would map to
          course/format/grid/yui/example/example.js

          Show
          Andrew Nicols added a comment - Ignore my comment about yui not supporting course format JS: moodle-format_grid-example would map to course/format/grid/yui/example/example.js
          Hide
          Andrew Nicols added a comment -

          Some notes:

          • site frontpage fixed
          • enablecourseajax and other related checks added back
          • course_format_ajax_support checks added back
          • strings converted to strings_for_js
          • set_section_visible phpdocs updated
          • strings moved to relevant course formats
          • fixed drag-drop to work with two-column themes (binarius) as well as three-column themes

          For third-party course formats to work properly with the changes, they will need to add some classes:

          • section show/hide will require the 'editing_showhide' on it's anchor
          • section move up will require the 'moveup' class on it's anchor
          • section move down will require the 'movewon' class on it's anchor

          Ruslan will probably add some more comments shortly and we'll aim to push another set of patches by this evening.

          Show
          Andrew Nicols added a comment - Some notes: site frontpage fixed enablecourseajax and other related checks added back course_format_ajax_support checks added back strings converted to strings_for_js set_section_visible phpdocs updated strings moved to relevant course formats fixed drag-drop to work with two-column themes (binarius) as well as three-column themes For third-party course formats to work properly with the changes, they will need to add some classes: section show/hide will require the 'editing_showhide' on it's anchor section move up will require the 'moveup' class on it's anchor section move down will require the 'movewon' class on it's anchor Ruslan will probably add some more comments shortly and we'll aim to push another set of patches by this evening.
          Hide
          Andrew Nicols added a comment -

          All,

          Sorry that this took longer than expected. I think we've covered most of the notes you above. Here's a quick overview of the changes:

          • site frontpage now supports ajax:
            • block moving
            • resource changes (excluding moving)
          • weekly names now update when swapping sections (thanks Dan)
          • now respect enablecourseajax and other ajax enabled settings
          • added exceptions and exception handling (thanks Dan)
          • course_format_ajax_support now respected
          • de-docked blocks can now be dragged again
          • converted all string_for_js to strings_for_js where possible
          • set_section_visible docs update
          • course format language changes/moves
          • can now drag blocks back to left-hand column
          • binarius theme (and other single block-column themes) issues addressed

          In addition we have:

          • split out section and resource code
          • allowed for specification of an alternative URL (e.g. /course/rest.php) to allow modules such as mod_page to utilise the drag/drop functionality without hacking core
          • updated /course/rest.php to allow it to be extended for the same purpose
          • lightboxes - added a lightbox to blocks and sections to assure user that 'something is happening'
          • spinners - added spinners for resource changes to assure user
          • converted MDL-31215 (rename resource) to use an inline textbox
          • removed drag/drop icon entirely from blocks. You can now select anywhere on the block title. Cursor changes in locations where you can drag/drop

          We have not:

          • changed course/lib.php to take a moodle_page object
          • renamed lib/yui/blocks/blocks.js as it should be possible to extend this to add other toolbox commands (e.g. show/hide and delete) in the same code
          • hardcoding of CSS classes for block-regions - how obvious way to do this at present

          Course Formats:

          • Many of these will need to be updated to add:
            • editing_moveup class to move up icon
            • editing_movedown class to move down icon
            • <div class="left side"></div> to the section
            • language strings for 'showfromothers' and 'hidefromothers'
          • Other things to consider
            • any section numbers which are non-standard may need to be looked at more specifically

          At present, there are lots of commits and we haven't squashed down yet - sorry!

          Cheers,

          Andrew and Ruslan

          Show
          Andrew Nicols added a comment - All, Sorry that this took longer than expected. I think we've covered most of the notes you above. Here's a quick overview of the changes: site frontpage now supports ajax: block moving resource changes (excluding moving) weekly names now update when swapping sections (thanks Dan) now respect enablecourseajax and other ajax enabled settings added exceptions and exception handling (thanks Dan) course_format_ajax_support now respected de-docked blocks can now be dragged again converted all string_for_js to strings_for_js where possible set_section_visible docs update course format language changes/moves can now drag blocks back to left-hand column binarius theme (and other single block-column themes) issues addressed In addition we have: split out section and resource code allowed for specification of an alternative URL (e.g. /course/rest.php) to allow modules such as mod_page to utilise the drag/drop functionality without hacking core updated /course/rest.php to allow it to be extended for the same purpose lightboxes - added a lightbox to blocks and sections to assure user that 'something is happening' spinners - added spinners for resource changes to assure user converted MDL-31215 (rename resource) to use an inline textbox removed drag/drop icon entirely from blocks. You can now select anywhere on the block title. Cursor changes in locations where you can drag/drop We have not: changed course/lib.php to take a moodle_page object renamed lib/yui/blocks/blocks.js as it should be possible to extend this to add other toolbox commands (e.g. show/hide and delete) in the same code hardcoding of CSS classes for block-regions - how obvious way to do this at present Course Formats: Many of these will need to be updated to add: editing_moveup class to move up icon editing_movedown class to move down icon <div class="left side"></div> to the section language strings for 'showfromothers' and 'hidefromothers' Other things to consider any section numbers which are non-standard may need to be looked at more specifically At present, there are lots of commits and we haven't squashed down yet - sorry! Cheers, Andrew and Ruslan
          Hide
          Andrew Nicols added a comment -

          As per Dan's request in IM, I've rebase the commits to squash relevant commits down together to leave eight (rather than 33).

          There should be no (relevant) code differences between this and the previous version.

          Andrew

          Show
          Andrew Nicols added a comment - As per Dan's request in IM, I've rebase the commits to squash relevant commits down together to leave eight (rather than 33). There should be no (relevant) code differences between this and the previous version. Andrew
          Hide
          Dan Poltawski added a comment -

          Hi Guys,

          For next time, can I please ask that you refrain from adding new features when we are still trying to get the original issues ironed out. Appreciate the work, but it makes it hard for us to get it landed with a moving target.

          Show
          Dan Poltawski added a comment - Hi Guys, For next time, can I please ask that you refrain from adding new features when we are still trying to get the original issues ironed out. Appreciate the work, but it makes it hard for us to get it landed with a moving target.
          Hide
          Dan Poltawski added a comment -

          Hi Eloy,

          Submitting this for integration.

          I think the commits are split up in logical steps which explain what is happening. The basic essence is replacing the old YUI2 JS.

          I think there is some post-integration work to be done, but right now my thought from testing and reviewing is that its much more solid than the existing JS and i'd like to get it landed.

          Remaining issues:

          1. The block regions need to be wrapped in a generic div that is not theme dependent and can be used in this. At the moment this fails gracefully. This should be fairly straight forward to add.
          2. Themes declarining non-support for the JS features
          Show
          Dan Poltawski added a comment - Hi Eloy, Submitting this for integration. I think the commits are split up in logical steps which explain what is happening. The basic essence is replacing the old YUI2 JS. I think there is some post-integration work to be done, but right now my thought from testing and reviewing is that its much more solid than the existing JS and i'd like to get it landed. Remaining issues: The block regions need to be wrapped in a generic div that is not theme dependent and can be used in this. At the moment this fails gracefully. This should be fairly straight forward to add. Themes declarining non-support for the JS features
          Hide
          Gareth J Barnard added a comment -

          Hi,

          Sounds good that this is being improved .

          I currently override 'section_class.prototype.swap_with_section' so that I can perform an additional swap 'YAHOO.util.DDM.swapNode(this.getEl().previousSibling, sectionIn.getEl().previousSibling);' due to my course format Collapsed Topics operation. Will I be able to alter my code to still do this in some shape or form?

          Thanks,

          Gareth

          Show
          Gareth J Barnard added a comment - Hi, Sounds good that this is being improved . I currently override 'section_class.prototype.swap_with_section' so that I can perform an additional swap 'YAHOO.util.DDM.swapNode(this.getEl().previousSibling, sectionIn.getEl().previousSibling);' due to my course format Collapsed Topics operation. Will I be able to alter my code to still do this in some shape or form? Thanks, Gareth
          Hide
          Gareth J Barnard added a comment -

          Hi,

          To clarify why I ask . This is because my format uses a table to structure the sections - because of browser issues with the toggling mechanism. So, each section is made up of two elements ('tr' tags) that comprise a logical section - the toggle and the section itself. Therefore when drag and droping I need to swap both nodes rather than just one - my code currently in 'https://github.com/gjb2048/moodle-format_topcoll/blob/master/tc_section_classes.js'- this may move to a 'js' folder soon.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Hi, To clarify why I ask . This is because my format uses a table to structure the sections - because of browser issues with the toggling mechanism. So, each section is made up of two elements ('tr' tags) that comprise a logical section - the toggle and the section itself. Therefore when drag and droping I need to swap both nodes rather than just one - my code currently in 'https://github.com/gjb2048/moodle-format_topcoll/blob/master/tc_section_classes.js'- this may move to a 'js' folder soon. Cheers, Gareth
          Hide
          Ruslan Kabalin added a comment -

          Will I be able to alter my code to still do this in some shape or form?

          Hi Gareth, we do not support that at the moment. But we have just looked at your code with Andrew, I will come up with solution for this tomorrow.

          Show
          Ruslan Kabalin added a comment - Will I be able to alter my code to still do this in some shape or form? Hi Gareth, we do not support that at the moment. But we have just looked at your code with Andrew, I will come up with solution for this tomorrow.
          Hide
          Gareth J Barnard added a comment - - edited

          Dear Ruslan,

          Thank you . If there is anything I can do to help / answer, please let me know.

          Please note that the comment is slightly wrong in my code, so should be...

              // Swap the sections.
              YAHOO.util.DDM.swapNode(this.getEl(), sectionIn.getEl());  // Swaps the section.
              YAHOO.util.DDM.swapNode(this.getEl().previousSibling, sectionIn.getEl().previousSibling);  // Swaps the toggle.
          

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - - edited Dear Ruslan, Thank you . If there is anything I can do to help / answer, please let me know. Please note that the comment is slightly wrong in my code, so should be... // Swap the sections. YAHOO.util.DDM.swapNode( this .getEl(), sectionIn.getEl()); // Swaps the section. YAHOO.util.DDM.swapNode( this .getEl().previousSibling, sectionIn.getEl().previousSibling); // Swaps the toggle. Cheers, Gareth
          Hide
          Eloy Lafuente (stronk7) added a comment -

          In LUNS we TRUST!

          Show
          Eloy Lafuente (stronk7) added a comment - In LUNS we TRUST!
          Hide
          Dan Poltawski added a comment -

          Asssing myself as tester because the actual testing is going to be coordinated in other bugs.

          Show
          Dan Poltawski added a comment - Asssing myself as tester because the actual testing is going to be coordinated in other bugs.
          Hide
          Dan Poltawski added a comment -

          Well, thats not a good sign (i've just pushed a fix for it):

          Coding error detected, it must be fixed by a programmer: Invalid parameter $url, has to be full url or in shortened form starting with /.

          URL: /

          Debug info: null

          Stack trace: * line 1156 of \lib\pagelib.php: coding_exception thrown

          • line 42 of \lib\ajax\blocks.php: call
          Show
          Dan Poltawski added a comment - Well, thats not a good sign (i've just pushed a fix for it): Coding error detected, it must be fixed by a programmer: Invalid parameter $url, has to be full url or in shortened form starting with /. URL: / Debug info: null Stack trace: * line 1156 of \lib\pagelib.php: coding_exception thrown line 42 of \lib\ajax\blocks.php: call
          Hide
          Ruslan Kabalin added a comment -

          Gareth: I created the issue MDL-32657 for custom format swapping. The fix is ready, will push it for revision tomorrow.

          Show
          Ruslan Kabalin added a comment - Gareth: I created the issue MDL-32657 for custom format swapping. The fix is ready, will push it for revision tomorrow.
          Hide
          Gareth J Barnard added a comment -

          Thank you so much Ruslan , my format would not have been able to perform correctly at the detriment of the users - appreciated.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Thank you so much Ruslan , my format would not have been able to perform correctly at the detriment of the users - appreciated. Cheers, Gareth
          Hide
          Gareth J Barnard added a comment - - edited

          I have also just noticed that the function 'section_class.prototype.init_buttons' in 'section_classes.js' has a 'if (main.getString('courseformat', this.sectionId) != "weeks" && this.sectionId > 0)' where upon it sets the wording of the current topic as such:

                  if (YAHOO.util.Dom.hasClass(this.getEl(),'current')) {
                      highlightbutton = main.mk_button('div', main.portal.icons['marked'], main.getString('marked', this.sectionId));
                  } else {
                      highlightbutton = main.mk_button('div', main.portal.icons['marker'], main.getString('marker', this.sectionId));
                  }
          

          I have the functionality of being able to set the 'current week' and therefore need to be able to manipulate this code so that I display 'weeks' and not 'topics' - although I do realise that this functionality will disappear in the forthcoming 2.3 paged course formats and I will then have to move into my own format if I wish to keep it.

          But pragmatically, if the wording was changed to 'section' then I would not have to perform any changes as it would be generic enough to cover both topic and week based formats .

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - - edited I have also just noticed that the function 'section_class.prototype.init_buttons' in 'section_classes.js' has a 'if (main.getString('courseformat', this.sectionId) != "weeks" && this.sectionId > 0)' where upon it sets the wording of the current topic as such: if (YAHOO.util.Dom.hasClass( this .getEl(),'current')) { highlightbutton = main.mk_button('div', main.portal.icons['marked'], main.getString('marked', this .sectionId)); } else { highlightbutton = main.mk_button('div', main.portal.icons['marker'], main.getString('marker', this .sectionId)); } I have the functionality of being able to set the 'current week' and therefore need to be able to manipulate this code so that I display 'weeks' and not 'topics' - although I do realise that this functionality will disappear in the forthcoming 2.3 paged course formats and I will then have to move into my own format if I wish to keep it. But pragmatically, if the wording was changed to 'section' then I would not have to perform any changes as it would be generic enough to cover both topic and week based formats . Cheers, Gareth
          Hide
          Ruslan Kabalin added a comment -

          Gareth: I see your point. Just think it might be better if we continue any format-related discussion in MDL-32657, to keep all related communication in a single place.

          Show
          Ruslan Kabalin added a comment - Gareth: I see your point. Just think it might be better if we continue any format-related discussion in MDL-32657 , to keep all related communication in a single place.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          This has been near becoming rejected, because it's not the best code you are able to produce.

          But, luckily, at the end, it has landed and has been spread to all repos out there.

          Many thanks and, don't forget it, keep improving your skills, you can!

          Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - This has been near becoming rejected, because it's not the best code you are able to produce. But, luckily, at the end, it has landed and has been spread to all repos out there. Many thanks and, don't forget it, keep improving your skills, you can! Closing, ciao
          Hide
          Gareth J Barnard added a comment - - edited

          Looking at the 'https://github.com/danpoltawski/moodle/tree/MDL-31052-js-rewrite' branch for this tracker, there is a coding fault in '/course/format/weeks/format.php' where

                  $strweekhide = get_string('hidefromothers', 'format_topics');
                  $strweekshow = get_string('showfromothers', 'format_topics');
          

          should be

                  $strweekhide = get_string('hidefromothers', 'format_weeks');
                  $strweekshow = get_string('showfromothers', 'format_weeks');
          

          Lines 50 & 51.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - - edited Looking at the 'https://github.com/danpoltawski/moodle/tree/MDL-31052-js-rewrite' branch for this tracker, there is a coding fault in '/course/format/weeks/format.php' where $strweekhide = get_string('hidefromothers', 'format_topics'); $strweekshow = get_string('showfromothers', 'format_topics'); should be $strweekhide = get_string('hidefromothers', 'format_weeks'); $strweekshow = get_string('showfromothers', 'format_weeks'); Lines 50 & 51. Cheers, Gareth
          Show
          Gareth J Barnard added a comment - Also found in master -> https://github.com/moodle/moodle/blob/master/course/format/weeks/format.php
          Hide
          Andrew Nicols added a comment -

          Hi Gareth,

          This has now been integrated so any new issues on this should be opened as new bugs.

          Thanks,

          Andrew

          Show
          Andrew Nicols added a comment - Hi Gareth, This has now been integrated so any new issues on this should be opened as new bugs. Thanks, Andrew
          Hide
          Gareth J Barnard added a comment -

          Dear Andrew,

          Ok, will do. Just thought this one could be reopened to save the paperwork

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Andrew, Ok, will do. Just thought this one could be reopened to save the paperwork Cheers, Gareth
          Hide
          Gareth J Barnard added a comment -

          MDL-32728 raised. One more step in Tim Hunt's tracker sweepstake.

          Show
          Gareth J Barnard added a comment - MDL-32728 raised. One more step in Tim Hunt's tracker sweepstake.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: