Moodle
  1. Moodle
  2. MDL-35134

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

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major 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
    • Rank:
      43764

      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.

        Activity

        Hide
        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 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
        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
        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 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 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
        Michael de Raadt added a comment -

        Thanks, Paul.

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

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

        Pushing directly to integration.

        Show
        Michael de Raadt added a comment - Pushing directly to integration.
        Hide
        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
        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
        Eloy Lafuente (stronk7) added a comment -

        Integrated (23 and master), thanks!

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

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

        Show
        Tim Barker added a comment - Tested as per the testing instructions and the test passed. Good work
        Hide
        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
        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: