Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.9.3
    • Fix Version/s: 1.9.10
    • Component/s: SCORM
    • Labels:
      None
    • Environment:
      Debian Linux
    • Database:
      MySQL
    • Difficulty:
      Moderate
    • Affected Branches:
      MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_19_STABLE
    • Rank:
      31593

      Description

      In /mod/scorm/datamodels/scorm_12js.php, method LMSFinish() on line 185 writes back the session data through the StoreData() function. If successful, StoreData() returns the string "true". Currently the result of this call is captured but unhandled. Just below where it is called there is a hard coded return "true"; statement. So if StoreData() fails for whatever reason (proxy auth issue, network traffic, firewall block) in its HttpRequest attempt then all the student's work will be silently lost. That's a rather serious issue when it comes to online learning, and it's just happened to me several times during testing.

      I raised this issue once before nearly a year ago in http://tracker.moodle.org/browse/MDL-13716 (which has since been reviewed and closed)

      1. LMSFinish.php
        1 kB
        Jaime Liz
      2. patch.txt
        0.7 kB
        Jaime Liz
      3. scorm_12.js.php
        32 kB
        Ron Meske

        Activity

        Hide
        Piers Harding added a comment -

        Hi Peter - having a look at this I notice that StoreData() returns the correct thing if the HTTP call succeeds (either true or false), but would in effect return nothing or garbage in the HTTP call failed. It is easy to fix the return from LMSFinish() to just return the result of StoreData(), but what should the correct behaviour be in the case of an HTTP failure - "false" ?

        Cheers,
        Piers Harding.

        Show
        Piers Harding added a comment - Hi Peter - having a look at this I notice that StoreData() returns the correct thing if the HTTP call succeeds (either true or false), but would in effect return nothing or garbage in the HTTP call failed. It is easy to fix the return from LMSFinish() to just return the result of StoreData(), but what should the correct behaviour be in the case of an HTTP failure - "false" ? Cheers, Piers Harding.
        Hide
        Peter Chamberlin added a comment -

        LMSFinish() should return string "true" on success and string "false" for any other state, such as if additional API calls cannot be guaranteed to process correctly. If the HTTP call failed then I'd consider that a failure of LMSFinish as the SCO cannot be assured that data set with LMSSetValue() has been persisted. I believe the appropriate SCORM 1.2 error code would be "101" (General Exception). That would also allow me to give warning to the user that all their work has just been lost. It may be worth keeping the SCO<>LMS state connected in such a failure so that the SCO has the opportunity to retry LMSFinish(), or putting a multi-attempt fallback mechanism into the way session data is fed back to the database to handle situations of high load or temporary connectivity loss.

        Show
        Peter Chamberlin added a comment - LMSFinish() should return string "true" on success and string "false" for any other state, such as if additional API calls cannot be guaranteed to process correctly. If the HTTP call failed then I'd consider that a failure of LMSFinish as the SCO cannot be assured that data set with LMSSetValue() has been persisted. I believe the appropriate SCORM 1.2 error code would be "101" (General Exception). That would also allow me to give warning to the user that all their work has just been lost. It may be worth keeping the SCO<>LMS state connected in such a failure so that the SCO has the opportunity to retry LMSFinish(), or putting a multi-attempt fallback mechanism into the way session data is fed back to the database to handle situations of high load or temporary connectivity loss.
        Hide
        Jaime Liz added a comment -

        I have just changed the return statement to return the result of the DataStore function if it has been executed or just FALSE if it didn't get to be executed within the LMSFinish fuction

        Show
        Jaime Liz added a comment - I have just changed the return statement to return the result of the DataStore function if it has been executed or just FALSE if it didn't get to be executed within the LMSFinish fuction
        Hide
        Jaime Liz added a comment -

        here's the patch file Dan xD

        Show
        Jaime Liz added a comment - here's the patch file Dan xD
        Hide
        Dan Marsden added a comment -

        Hi Peter - do you think the path Jamie attached here is adequate? - or should we be returning an errorcode instead?

        Show
        Dan Marsden added a comment - Hi Peter - do you think the path Jamie attached here is adequate? - or should we be returning an errorcode instead?
        Hide
        Peter Chamberlin added a comment -

        It is adequate, as knowing about data write-back failure is the main thing when the communications layer fails. That can then trigger investigation into the cause. Addition error information would then be useful in debugging mode.

        Show
        Peter Chamberlin added a comment - It is adequate, as knowing about data write-back failure is the main thing when the communications layer fails. That can then trigger investigation into the cause. Addition error information would then be useful in debugging mode.
        Hide
        Dan Marsden added a comment -

        Thanks Peter - fix pushed into HEAD - can you please check it out before I push to stable?

        http://cvs.moodle.org/moodle/mod/scorm/datamodels/scorm_12.js.php?r1=1.23&r2=1.24

        Show
        Dan Marsden added a comment - Thanks Peter - fix pushed into HEAD - can you please check it out before I push to stable? http://cvs.moodle.org/moodle/mod/scorm/datamodels/scorm_12.js.php?r1=1.23&r2=1.24
        Hide
        Dan Marsden added a comment -

        just following up - has anyone been able to test and verify this fix works on their 1.9 install?

        thanks,

        Show
        Dan Marsden added a comment - just following up - has anyone been able to test and verify this fix works on their 1.9 install? thanks,
        Hide
        Matteo Scaramuccia added a comment -

        Hi Dan,
        I cannot test it right now but I want to highlight a possible issue regarding with the fix submitted in HEAD i.e. just looking at the code flow. Please, find below the comment at the fix and our proposal for the discussion.

        result is now proposed to be evaluated through the return value of StoreData; this return value is built with this logic below:

                result = DoRequest(myRequest,"<?php p($CFG->wwwroot) ?>/mod/scorm/datamodel.php","id=<?php p($id) ?>&sesskey=<?php p($USER->sesskey) ?>"+datastring);
                results = String(result).split('\n');
                errorCode = results[1];
                return results[0];
        

        Everything will work as expected (i.e. true or false) until the AJAX call will fail:

            if (httpReq.status == 200) {
                //popupwin(url+"\n"+param+"\n"+httpReq.responseText);
                return httpReq.responseText;
            } else {
                return httpReq.status;
            }
        

        so it could be IMHO better to code ''true if true, false otherwise'', e.g.:

        @@ -199,13 +199,20 @@
                                 setTimeout('top.document.location=top.next;',500);
                             }
                         }
        +                <?php
        +                    if (debugging('',DEBUG_DEVELOPER)) {
        +                        echo 'LogAPICall("LMSFinish", "AJAXResult", result, 0);';
        +                    }
        +                ?>
        +                result = ('true' == result ? 'true' : 'false');
                         <?php
                             if (debugging('',DEBUG_DEVELOPER)) {
                                 //echo 'alert("Finished SCORM 1.2");';
        +                        echo 'LogAPICall("LMSFinish", "result", result, 0);';
                                 echo 'LogAPICall("LMSFinish", param, "", 0);';
                             }
                         ?>
        -                return "true";
        +                return result;
                     } else {
                         errorCode = "301";
                     }
        

        Note: I've rearranged also the LogAPICall parameters order, according to the signature&logic found in scorm_12.js.php.
        Note: we're interested in the fix of this issue also in 1.9 branch.

        Show
        Matteo Scaramuccia added a comment - Hi Dan, I cannot test it right now but I want to highlight a possible issue regarding with the fix submitted in HEAD i.e. just looking at the code flow. Please, find below the comment at the fix and our proposal for the discussion. result is now proposed to be evaluated through the return value of StoreData ; this return value is built with this logic below: result = DoRequest(myRequest, "<?php p($CFG->wwwroot) ?>/mod/scorm/datamodel.php" , "id=<?php p($id) ?>&sesskey=<?php p($USER->sesskey) ?>" +datastring); results = String (result).split('\n'); errorCode = results[1]; return results[0]; Everything will work as expected (i.e. true or false ) until the AJAX call will fail: if (httpReq.status == 200) { //popupwin(url+ "\n" +param+ "\n" +httpReq.responseText); return httpReq.responseText; } else { return httpReq.status; } so it could be IMHO better to code '' true if true , false otherwise'', e.g.: @@ -199,13 +199,20 @@ setTimeout('top.document.location=top.next;',500); } } + <?php + if (debugging('',DEBUG_DEVELOPER)) { + echo 'LogAPICall( "LMSFinish" , "AJAXResult" , result, 0);'; + } + ?> + result = (' true ' == result ? ' true ' : ' false '); <?php if (debugging('',DEBUG_DEVELOPER)) { //echo 'alert( "Finished SCORM 1.2" );'; + echo 'LogAPICall( "LMSFinish" , "result" , result, 0);'; echo 'LogAPICall( "LMSFinish" , param, "", 0);'; } ?> - return " true " ; + return result; } else { errorCode = "301" ; } Note: I've rearranged also the LogAPICall parameters order, according to the signature&logic found in scorm_12.js.php . Note: we're interested in the fix of this issue also in 1.9 branch.
        Hide
        Ron Meske added a comment -

        I would like to express a sense of urgency for this. We have been experiencing this for the past few months where occasionaly a user would take a SCORM course and provide a screenprint of the final score showing completion, however there is absolutely no SCORM data, the status remains at Not Attempted. Based on less than 100 users, It is a small percentage of the total users at this time.

        However, we are about to setup a new instlation of Moodle that will serve over 10,000 user starting in March and we are very concerned that the number of users experiencing this issue is going to climb rather quickly just by volume.

        Show
        Ron Meske added a comment - I would like to express a sense of urgency for this. We have been experiencing this for the past few months where occasionaly a user would take a SCORM course and provide a screenprint of the final score showing completion, however there is absolutely no SCORM data, the status remains at Not Attempted. Based on less than 100 users, It is a small percentage of the total users at this time. However, we are about to setup a new instlation of Moodle that will serve over 10,000 user starting in March and we are very concerned that the number of users experiencing this issue is going to climb rather quickly just by volume.
        Hide
        Matteo Scaramuccia added a comment -

        Let me add my 2 cents:

        1. Moodle should correctly implement the LMSFinish('') return value 'cause of a specification that tells how to do that. That's the reason for filing this bug as well as the sense of my review proposal;
        2. Content vendors should correctly react on a failure of an LMSFinish('') call (= having got "false" as the return value) by adopting a recovery strategy like e.g. try to re-call it after n seconds to mitigate e.g. network issues OR using sometime LMSCommit('').
          Those things above just to avoid false myths, see also Best practices for the use of Commit in SCORM content.

        BTW, @Ron: if you're using Mastery Score in your packages, it could be possible that a failure in setting the cmi.core.lesson_status value could issue what described in MDL-21305.

        Show
        Matteo Scaramuccia added a comment - Let me add my 2 cents: Moodle should correctly implement the LMSFinish('') return value 'cause of a specification that tells how to do that. That's the reason for filing this bug as well as the sense of my review proposal; Content vendors should correctly react on a failure of an LMSFinish('') call (= having got "false" as the return value) by adopting a recovery strategy like e.g. try to re-call it after n seconds to mitigate e.g. network issues OR using sometime LMSCommit('') . Those things above just to avoid false myths, see also Best practices for the use of Commit in SCORM content . BTW, @Ron: if you're using Mastery Score in your packages, it could be possible that a failure in setting the cmi.core.lesson_status value could issue what described in MDL-21305 .
        Hide
        Ron Meske added a comment -

        I would like to point out the LMSCommit also needs this correction as it is also hardcoded to return true.

        Show
        Ron Meske added a comment - I would like to point out the LMSCommit also needs this correction as it is also hardcoded to return true.
        Hide
        Ron Meske added a comment -

        I also just looked at the scorm_13.js.php file and it also will need the corrections.

        Show
        Ron Meske added a comment - I also just looked at the scorm_13.js.php file and it also will need the corrections.
        Hide
        Ron Meske added a comment -

        After reviewing the flow of logic, the line: result = ('true' == result ? 'true' : 'false'); is not quite enough.

        If it is set to false, then errorCode should also be set to conform to SCORM standards. I recommend the following:

        result = ('true' == result) ? 'true' : 'false';
        errorCode = (result == 'true')? '0' : '101';

        Show
        Ron Meske added a comment - After reviewing the flow of logic, the line: result = ('true' == result ? 'true' : 'false'); is not quite enough. If it is set to false, then errorCode should also be set to conform to SCORM standards. I recommend the following: result = ('true' == result) ? 'true' : 'false'; errorCode = (result == 'true')? '0' : '101';
        Hide
        Ron Meske added a comment -

        We have modified scorm_12_js.php based on all the input on this ticket and have done some limited testing. Our modified version is attached.

        Show
        Ron Meske added a comment - We have modified scorm_12_js.php based on all the input on this ticket and have done some limited testing. Our modified version is attached.
        Hide
        Matteo Scaramuccia added a comment -

        Good catch Ron!
        I've missed the errorCode issue.

        Thanks,
        Matteo

        Show
        Matteo Scaramuccia added a comment - Good catch Ron! I've missed the errorCode issue. Thanks, Matteo
        Hide
        Dan Marsden added a comment -

        thanks to everyone who helped trace this - I've pushed the fix into 1.9Stable and HEAD - would appreciate it if someone sanity checked the commits!

        thanks!

        Show
        Dan Marsden added a comment - thanks to everyone who helped trace this - I've pushed the fix into 1.9Stable and HEAD - would appreciate it if someone sanity checked the commits! thanks!

          People

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

            Dates

            • Created:
              Updated:
              Resolved: