Moodle
  1. Moodle
  2. MDL-39219

SCORM JSON.parse not supported by IE7

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4.3, 2.5
    • Fix Version/s: 2.4.4
    • Component/s: SCORM
    • Rank:
      49826

      Description

      I am fully aware that IE7 is no longer supported by Moodle.

      However, simply changing the 3 occurrences, in mod/scorm/module.js, of:

      JSON.parse

      to:

      Y.JSON.parse

      Changes the SCORM module from not supporting IE7 at all, to fully working (and continuing to work in all other browsers).

      I'll understand if this is rejected, but as I've just been asked to fix this for a client (and it is a very small change), I thought I'd share the patch here as well.

        Activity

        Hide
        Davo Smith added a comment -

        Note - this was also raised in the forums at:

        https://moodle.org/mod/forum/discuss.php?d=222790

        Show
        Davo Smith added a comment - Note - this was also raised in the forums at: https://moodle.org/mod/forum/discuss.php?d=222790
        Hide
        Matteo Scaramuccia added a comment -

        Added Dan as a watcher.
        Nice proposal Davo!

        I was the first to tell about the "IE7 support" issue in the forum but... I'll vote for this cleaned proposal, having also read http://yuilibrary.com/yui/docs/json/ - need time to see how it works in Moodle and I'll use this issue as an opportunity to learn something around the way YUI works (not sure where those modules are loaded in Moodle, probably course/dnduploadlib.php via 'json').

        Show
        Matteo Scaramuccia added a comment - Added Dan as a watcher. Nice proposal Davo! I was the first to tell about the "IE7 support" issue in the forum but... I'll vote for this cleaned proposal, having also read http://yuilibrary.com/yui/docs/json/ - need time to see how it works in Moodle and I'll use this issue as an opportunity to learn something around the way YUI works (not sure where those modules are loaded in Moodle, probably course/dnduploadlib.php via 'json' ).
        Hide
        Dan Marsden added a comment -

        Thanks Davo - looks good to me - this is consistent with other Moodle code (using Y.JSON instead of JSON)

        only thing missing is testing instructions - feel free to submit for integration once those have been added... feel free to submit patches for other SCORM bugs too...

        Show
        Dan Marsden added a comment - Thanks Davo - looks good to me - this is consistent with other Moodle code (using Y.JSON instead of JSON) only thing missing is testing instructions - feel free to submit for integration once those have been added... feel free to submit patches for other SCORM bugs too...
        Hide
        Davo Smith added a comment -

        Dan, I can't promise to fix any other SCORM bugs, but will try to submit to core any fixes I'm asked to make for clients.

        Show
        Davo Smith added a comment - Dan, I can't promise to fix any other SCORM bugs, but will try to submit to core any fixes I'm asked to make for clients.
        Hide
        Dan Marsden added a comment -

        was worth a try

        Show
        Dan Marsden added a comment - was worth a try
        Hide
        Dan Poltawski added a comment -

        Integrated to master and 24 - thanks Davo.

        TO TESTER: if its really hard for you to get an ie7 install then I am OK with you not testing with it (not officially supported, I trust Davo has done it), but we should at least test supported browsers to ensure there are not regressions.

        Show
        Dan Poltawski added a comment - Integrated to master and 24 - thanks Davo. TO TESTER: if its really hard for you to get an ie7 install then I am OK with you not testing with it (not officially supported, I trust Davo has done it), but we should at least test supported browsers to ensure there are not regressions.
        Hide
        Ankit Agarwal added a comment -

        Tested on ie7, chromium 25, chrome 26, firefox 20 , across 24 and master. was able to see scorm package without any issue.
        Thanks

        Show
        Ankit Agarwal added a comment - Tested on ie7, chromium 25, chrome 26, firefox 20 , across 24 and master. was able to see scorm package without any issue. Thanks
        Hide
        Davo Smith added a comment -

        Unfortunately, I think this fix has caused a regression on at least one site (the site which the fix was created for in the first place).

        In some situations (and I'm not quite sure which situations), the YUI JSON module is not loaded.

        It's only a few lines of code to fix - I assume I should create a new issue, rather than updating here?

        Show
        Davo Smith added a comment - Unfortunately, I think this fix has caused a regression on at least one site (the site which the fix was created for in the first place). In some situations (and I'm not quite sure which situations), the YUI JSON module is not loaded. It's only a few lines of code to fix - I assume I should create a new issue, rather than updating here?
        Hide
        Davo Smith added a comment -

        For the moment, I've just updated the branches above, but will cherry-pick across to a new branch, if needed.

        Show
        Davo Smith added a comment - For the moment, I've just updated the branches above, but will cherry-pick across to a new branch, if needed.
        Hide
        Dan Poltawski added a comment -

        Davo: do you happen to have a failure scenario to reproduce this? I'm gonna pull it in.

        Show
        Dan Poltawski added a comment - Davo: do you happen to have a failure scenario to reproduce this? I'm gonna pull it in.
        Hide
        Davo Smith added a comment -

        I'm afraid I was unable to track down why the JSON module was not loaded on the site in question (or, for that matter, exactly why it managed to be loaded when testing locally). It may have been to do with AJAX settings, but I'm not sure.

        What I do know, is that when I pushed the attached fix out to the client the problem went away again (as you would expect, given the symptoms).

        Show
        Davo Smith added a comment - I'm afraid I was unable to track down why the JSON module was not loaded on the site in question (or, for that matter, exactly why it managed to be loaded when testing locally). It may have been to do with AJAX settings, but I'm not sure. What I do know, is that when I pushed the attached fix out to the client the problem went away again (as you would expect, given the symptoms).
        Hide
        Matteo Scaramuccia added a comment - - edited

        Hi All,
        see my comment above, https://tracker.moodle.org/browse/MDL-39219?focusedCommentId=216886&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-216886, when I was guessing where json is loaded, available for all the YUI code.
        Quickly bottom-up from the code: course/lib.php::include_course_ajax => course/dnduploadlib.php::dndupload_add_to_course, the first blocked before including the required YUI modules, json included, when course/lib.php::course_ajax_enabled return false.

        I'm with Davo to think about AJAX.

        Show
        Matteo Scaramuccia added a comment - - edited Hi All, see my comment above, https://tracker.moodle.org/browse/MDL-39219?focusedCommentId=216886&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-216886 , when I was guessing where json is loaded, available for all the YUI code. Quickly bottom-up from the code: course/lib.php::include_course_ajax => course/dnduploadlib.php::dndupload_add_to_course , the first blocked before including the required YUI modules, json included, when course/lib.php::course_ajax_enabled return false . I'm with Davo to think about AJAX.
        Hide
        Dan Poltawski added a comment -

        I've pulled the fix in, thanks

        Show
        Dan Poltawski added a comment - I've pulled the fix in, thanks
        Hide
        Dan Poltawski added a comment -

        Did basic regression test myself with the fruit quiz package after the last fix.

        Show
        Dan Poltawski added a comment - Did basic regression test myself with the fruit quiz package after the last fix.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        I feel myself really alone tonight! So was time to push your fixes upstream!

        "Lest we forget. We will remember them."

        Thanks and ciao!

        Show
        Eloy Lafuente (stronk7) added a comment - I feel myself really alone tonight! So was time to push your fixes upstream! "Lest we forget. We will remember them." Thanks and ciao!

          People

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

            Dates

            • Created:
              Updated:
              Resolved: