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

Trailing comma in course/format/weeks/format.js breaks course JS in IE

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.3.1, 2.4
    • Fix Version/s: 2.3.3
    • Component/s: Course
    • Labels:
    • Testing Instructions:
      Hide

      This needs to be tested across browsers, especially IE8.

      1. Open a course which uses the Weekly course format.
      2. Open the error console in your browser.
        • If you are using IE8, press F12 and change the Browser mode to IE8 Compatibility View and click on the Script tab.
      3. Drag and drop some resources/activites.
      4. Drag and drop some sections.
      5. VERIFY no errors appear in the error console.
      Show
      This needs to be tested across browsers, especially IE8. Open a course which uses the Weekly course format. Open the error console in your browser. If you are using IE8, press F12 and change the Browser mode to IE8 Compatibility View and click on the Script tab. Drag and drop some resources/activites. Drag and drop some sections. VERIFY no errors appear in the error console.
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-35134_master

      Description

      IE doesn't like trailing commas in object (or array) definitions - and one seems to have turned up in 2.3/2.4dev, in course/format/weeks/format.js. This breaks subsequent javascript on courses using the weeks format.

        Gliffy Diagrams

          Activity

          Hide
          paul.n Paul Nicholls added a comment - - edited

          I've nudged the priority to "major" since it involves loss of functionality (albeit only in IE), even though the patch itself is incredibly trivial (quite literally just removing a comma).

          Show
          paul.n Paul Nicholls added a comment - - edited I've nudged the priority to "major" since it involves loss of functionality (albeit only in IE), even though the patch itself is incredibly trivial (quite literally just removing a comma).
          Hide
          salvetore Michael de Raadt added a comment -

          Thanks for submitting that, Paul.

          The change looks good. In JS, a trailing comma like that is an error, unlike in PHP.

          However I was not able to trigger any errors in IE8 or IE9 by dragging and dropping sections (which is where I think the code is used).

          Were you seeing actual errors?

          Show
          salvetore Michael de Raadt added a comment - Thanks for submitting that, Paul. The change looks good. In JS, a trailing comma like that is an error, unlike in PHP. However I was not able to trigger any errors in IE8 or IE9 by dragging and dropping sections (which is where I think the code is used). Were you seeing actual errors?
          Hide
          paul.n Paul Nicholls added a comment - - edited

          Hi Michael,
          The error only shows up in courses using the Weekly course format - and I was seeing it in IE8 (I'm not sure if IE9 still throws the error and aborts processing, as I don't have IE9 available) on page load. In a course using the Weekly format in IE8, the error caused JS to stop executing before the resource dragdrop was initialised - leaving all of the icons as the move up/down icon rather than the four-way drag icon.

          -Paul

          Show
          paul.n Paul Nicholls added a comment - - edited Hi Michael, The error only shows up in courses using the Weekly course format - and I was seeing it in IE8 (I'm not sure if IE9 still throws the error and aborts processing, as I don't have IE9 available) on page load. In a course using the Weekly format in IE8, the error caused JS to stop executing before the resource dragdrop was initialised - leaving all of the icons as the move up/down icon rather than the four-way drag icon. -Paul
          Hide
          salvetore Michael de Raadt added a comment -

          Thanks, Paul.

          I was able to see an error when the browser was in compatibility view.

          Show
          salvetore Michael de Raadt added a comment - Thanks, Paul. I was able to see an error when the browser was in compatibility view.
          Hide
          salvetore Michael de Raadt added a comment -

          Pushing directly to integration.

          Show
          salvetore Michael de Raadt added a comment - Pushing directly to integration.
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Integrated (23 and master), thanks!

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Integrated (23 and master), thanks!
          Hide
          timb Tim Barker added a comment -

          Tested as per the testing instructions and the test passed. Good work

          Show
          timb Tim Barker added a comment - Tested as per the testing instructions and the test passed. Good work
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Gutta cavat lapidem, non vi sed saepe cadendo - Ovidio

          This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads).

          Thanks!

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Gutta cavat lapidem, non vi sed saepe cadendo - Ovidio This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads). Thanks!

            People

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

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                12/Nov/12